From 72a7cd5c592d098aeb04e69b8ab804d1034cfef3 Mon Sep 17 00:00:00 2001 From: Demian Katz <demian.katz@villanova.edu> Date: Mon, 25 Jul 2016 13:18:38 -0400 Subject: [PATCH] Support for case-insensitive tags. - Wired up case sensitivity setting through factories - Simplified tag searching vs. exact lookups to reduce redundancy - Made Tags::getByText more flexible for more accurate/efficient deletion - Adjusted upgrade script so that existing "duplicate tag" fix accounts for case-insensitivity --- config/vufind/config.ini | 3 + module/VuFind/config/module.config.php | 4 +- module/VuFind/src/VuFind/Db/Row/Resource.php | 20 ++- module/VuFind/src/VuFind/Db/Row/User.php | 47 +----- module/VuFind/src/VuFind/Db/Table/Factory.php | 30 ++++ .../src/VuFind/Db/Table/ResourceTags.php | 41 +++-- module/VuFind/src/VuFind/Db/Table/Tags.php | 146 +++++++++++++++--- .../VuFind/src/VuFind/Search/Tags/Results.php | 56 ++----- .../templates/upgrade/fixduplicatetags.phtml | 8 +- 9 files changed, 221 insertions(+), 134 deletions(-) diff --git a/config/vufind/config.ini b/config/vufind/config.ini index 83b02699af2..1ff0eff0bdb 100644 --- a/config/vufind/config.ini +++ b/config/vufind/config.ini @@ -1477,6 +1477,9 @@ tags = enabled ; This controls the maximum length of a single tag; it should correspond with the ; field size in the tags database table. max_tag_length = 64 +; This controls whether tags are case-sensitive (true) or always forced to be +; represented as lowercase strings (false -- the default). +case_sensitive_tags = false ; If this setting is set to false, users will not be presented with a search ; drop-down or advanced search link when searching/viewing tags. This is recommended ; when using a multi-backend system (e.g. Solr + Summon + WorldCat). If set to diff --git a/module/VuFind/config/module.config.php b/module/VuFind/config/module.config.php index c59aaf79216..fc98dae4e1b 100644 --- a/module/VuFind/config/module.config.php +++ b/module/VuFind/config/module.config.php @@ -336,6 +336,8 @@ $config = [ 'abstract_factories' => ['VuFind\Db\Table\PluginFactory'], 'factories' => [ 'resource' => 'VuFind\Db\Table\Factory::getResource', + 'resourcetags' => 'VuFind\Db\Table\Factory::getResourceTags', + 'tags' => 'VuFind\Db\Table\Factory::getTags', 'user' => 'VuFind\Db\Table\Factory::getUser', 'userlist' => 'VuFind\Db\Table\Factory::getUserList', ], @@ -344,10 +346,8 @@ $config = [ 'comments' => 'VuFind\Db\Table\Comments', 'oairesumption' => 'VuFind\Db\Table\OaiResumption', 'record' => 'VuFind\Db\Table\Record', - 'resourcetags' => 'VuFind\Db\Table\ResourceTags', 'search' => 'VuFind\Db\Table\Search', 'session' => 'VuFind\Db\Table\Session', - 'tags' => 'VuFind\Db\Table\Tags', 'userresource' => 'VuFind\Db\Table\UserResource', 'userstats' => 'VuFind\Db\Table\UserStats', 'userstatsfields' => 'VuFind\Db\Table\UserStatsFields', diff --git a/module/VuFind/src/VuFind/Db/Row/Resource.php b/module/VuFind/src/VuFind/Db/Row/Resource.php index 0885dc9c78c..8d936d65ca4 100644 --- a/module/VuFind/src/VuFind/Db/Row/Resource.php +++ b/module/VuFind/src/VuFind/Db/Row/Resource.php @@ -95,8 +95,8 @@ class Resource extends RowGateway implements \VuFind\Db\Table\DbTableAwareInterf /** * Remove a tag from the current resource. * - * @param string $tagText The tag to save. - * @param \VuFind\Db\Row\User $user The user posting the tag. + * @param string $tagText The tag to delete. + * @param \VuFind\Db\Row\User $user The user deleting the tag. * @param string $list_id The list associated with the tag * (optional). * @@ -107,12 +107,16 @@ class Resource extends RowGateway implements \VuFind\Db\Table\DbTableAwareInterf $tagText = trim($tagText); if (!empty($tagText)) { $tags = $this->getDbTable('Tags'); - $tag = $tags->getByText($tagText); - - $linker = $this->getDbTable('ResourceTags'); - $linker->destroyLinks( - $this->id, $user->id, $list_id, $tag->id - ); + $tagIds = []; + foreach ($tags->getByText($tagText, false, false) as $tag) { + $tagIds[] = $tag->id; + } + if (!empty($tagIds)) { + $linker = $this->getDbTable('ResourceTags'); + $linker->destroyLinks( + $this->id, $user->id, $list_id, $tagIds + ); + } } } diff --git a/module/VuFind/src/VuFind/Db/Row/User.php b/module/VuFind/src/VuFind/Db/Row/User.php index 2ac011cd522..69dda547eaf 100644 --- a/module/VuFind/src/VuFind/Db/Row/User.php +++ b/module/VuFind/src/VuFind/Db/Row/User.php @@ -229,51 +229,8 @@ class User extends RowGateway implements \VuFind\Db\Table\DbTableAwareInterface, public function getTags($resourceId = null, $listId = null, $source = DEFAULT_SEARCH_BACKEND ) { - $userId = $this->id; - $callback = function ($select) use ($userId, $resourceId, $listId, $source) { - $select->columns( - [ - 'id' => new Expression( - 'min(?)', ['tags.id'], - [Expression::TYPE_IDENTIFIER] - ), - 'tag', - 'cnt' => new Expression( - 'COUNT(DISTINCT(?))', ['rt.resource_id'], - [Expression::TYPE_IDENTIFIER] - ) - ] - ); - $select->join( - ['rt' => 'resource_tags'], 'tags.id = rt.tag_id', [] - ); - $select->join( - ['r' => 'resource'], 'rt.resource_id = r.id', [] - ); - $select->join( - ['ur' => 'user_resource'], 'r.id = ur.resource_id', [] - ); - $select->group(['tag']) - ->order(['tag']); - - $select->where->equalTo('ur.user_id', $userId) - ->equalTo('rt.user_id', $userId) - ->equalTo( - 'ur.list_id', 'rt.list_id', - Predicate::TYPE_IDENTIFIER, Predicate::TYPE_IDENTIFIER - ) - ->equalTo('r.source', $source); - - if (!is_null($resourceId)) { - $select->where->equalTo('r.record_id', $resourceId); - } - if (!is_null($listId)) { - $select->where->equalTo('rt.list_id', $listId); - } - }; - - $table = $this->getDbTable('Tags'); - return $table->select($callback); + return $this->getDbTable('Tags') + ->getForUser($this->id, $resourceId, $listId, $source); } /** diff --git a/module/VuFind/src/VuFind/Db/Table/Factory.php b/module/VuFind/src/VuFind/Db/Table/Factory.php index 845de1d4de4..389455d6e65 100644 --- a/module/VuFind/src/VuFind/Db/Table/Factory.php +++ b/module/VuFind/src/VuFind/Db/Table/Factory.php @@ -53,6 +53,36 @@ class Factory return new Resource($sm->getServiceLocator()->get('VuFind\DateConverter')); } + /** + * Construct the Tags table. + * + * @param ServiceManager $sm Service manager. + * + * @return User + */ + public static function getTags(ServiceManager $sm) + { + $config = $sm->getServiceLocator()->get('VuFind\Config')->get('config'); + $caseSensitive = isset($config->Social->case_sensitive_tags) + && $config->Social->case_sensitive_tags; + return new Tags($caseSensitive); + } + + /** + * Construct the ResourceTags table. + * + * @param ServiceManager $sm Service manager. + * + * @return User + */ + public static function getResourceTags(ServiceManager $sm) + { + $config = $sm->getServiceLocator()->get('VuFind\Config')->get('config'); + $caseSensitive = isset($config->Social->case_sensitive_tags) + && $config->Social->case_sensitive_tags; + return new ResourceTags($caseSensitive); + } + /** * Construct the User table. * diff --git a/module/VuFind/src/VuFind/Db/Table/ResourceTags.php b/module/VuFind/src/VuFind/Db/Table/ResourceTags.php index afee8149d76..c1935e5e4b3 100644 --- a/module/VuFind/src/VuFind/Db/Table/ResourceTags.php +++ b/module/VuFind/src/VuFind/Db/Table/ResourceTags.php @@ -39,12 +39,22 @@ use Zend\Db\Sql\Expression; */ class ResourceTags extends Gateway { + /** + * Are tags case sensitive? + * + * @var bool + */ + protected $caseSensitive; + /** * Constructor + * + * @param bool $caseSensitive Are tags case sensitive? */ - public function __construct() + public function __construct($caseSensitive = false) { parent::__construct('resource_tags', 'VuFind\Db\Row\ResourceTags'); + $this->caseSensitive = $caseSensitive; } /** @@ -153,8 +163,12 @@ class ResourceTags extends Gateway $select->join( ['t' => 'tags'], 'resource_tags.tag_id = t.id', [] ); - $select->where->equalTo('t.tag', $tag) - ->where->equalTo('resource_tags.user_id', $userId); + if ($this->caseSensitive) { + $select->where->equalTo('t.tag', $tag); + } else { + $select->where->literal('lower(t.tag) = lower(?)', [$tag]); + } + $select->where->equalTo('resource_tags.user_id', $userId); if (null !== $listId) { $select->where->equalTo('resource_tags.list_id', $listId); } @@ -204,8 +218,8 @@ class ResourceTags extends Gateway * @param string $user ID of user removing links * @param string $list ID of list to unlink (null for ALL matching * tags, 'none' for tags not in a list, true for tags only found in a list) - * @param string $tag ID of tag to unlink (null for ALL matching - * tags) + * @param string|array $tag ID or array of IDs of tag(s) to unlink (null + * for ALL matching tags) * * @return void */ @@ -233,7 +247,11 @@ class ResourceTags extends Gateway } } if (null !== $tag) { - $select->where->equalTo('tag_id', $tag); + if (is_array($tag)) { + $select->where->in('tag_id', $tag); + } else { + $select->where->equalTo('tag_id', $tag); + } } }; @@ -354,7 +372,6 @@ class ResourceTags extends Gateway */ public function getUniqueTags($userId = null, $resourceId = null, $tagId = null) { - $callback = function ($select) use ($userId, $resourceId, $tagId) { $select->columns( [ @@ -383,7 +400,10 @@ class ResourceTags extends Gateway $select->join( ['t' => 'tags'], 'resource_tags.tag_id = t.id', - ["tag" => "tag"] + [ + 'tag' => + $this->caseSensitive ? 'tag' : new Expression('lower(tag)') + ] ); if (null !== $userId) { $select->where->equalTo('resource_tags.user_id', $userId); @@ -502,7 +522,10 @@ class ResourceTags extends Gateway $select->join( ['t' => 'tags'], 'resource_tags.tag_id = t.id', - ["tag" => "tag"] + [ + 'tag' => + $this->caseSensitive ? 'tag' : new Expression('lower(tag)') + ] ); $select->join( ['u' => 'user'], diff --git a/module/VuFind/src/VuFind/Db/Table/Tags.php b/module/VuFind/src/VuFind/Db/Table/Tags.php index 9d0529954c1..6688286c3a9 100644 --- a/module/VuFind/src/VuFind/Db/Table/Tags.php +++ b/module/VuFind/src/VuFind/Db/Table/Tags.php @@ -26,7 +26,9 @@ * @link https://vufind.org Main Site */ namespace VuFind\Db\Table; -use Zend\Db\Sql\Expression, Zend\Db\Sql\Select; +use Zend\Db\Sql\Expression, + Zend\Db\Sql\Predicate\Predicate, + Zend\Db\Sql\Select; /** * Table Definition for tags @@ -39,32 +41,52 @@ use Zend\Db\Sql\Expression, Zend\Db\Sql\Select; */ class Tags extends Gateway { + /** + * Are tags case sensitive? + * + * @var bool + */ + protected $caseSensitive; + /** * Constructor + * + * @param bool $caseSensitive Are tags case sensitive? */ - public function __construct() + public function __construct($caseSensitive = false) { parent::__construct('tags', 'VuFind\Db\Row\Tags'); + $this->caseSensitive = $caseSensitive; } /** * Get the row associated with a specific tag string. * - * @param string $tag Tag to look up. - * @param bool $create Should we create the row if it does not exist? + * @param string $tag Tag to look up. + * @param bool $create Should we create the row if it does not exist? + * @param bool $firstOnly Should we return the first matching row (true) + * or the entire result set (in case of multiple matches)? * - * @return \VuFind\Db\Row\Tags|null Matching row if found or created, null - * otherwise. + * @return mixed Matching row/result set if found or created, null otherwise. */ - public function getByText($tag, $create = true) + public function getByText($tag, $create = true, $firstOnly = true) { - $result = $this->select(['tag' => $tag])->current(); - if (empty($result) && $create) { - $result = $this->createRow(); - $result->tag = $tag; - $result->save(); + $cs = $this->caseSensitive; + $callback = function ($select) use ($tag, $cs) { + if ($cs) { + $select->where->equalTo('tag', $tag); + } else { + $select->where->literal('lower(tag) = lower(?)', [$tag]); + } + }; + $result = $this->select($callback); + if (count($result) == 0 && $create) { + $row = $this->createRow(); + $row->tag = $cs ? $tag : mb_strtolower($tag, 'UTF8'); + $row->save(); + return $firstOnly ? $row : [$row]; } - return $result; + return $firstOnly ? $result->current() : $result; } /** @@ -87,18 +109,19 @@ class Tags extends Gateway /** * Get all resources associated with the provided tag query. * - * @param string $query Search query + * @param string $q Search query * @param string $source Record source (optional limiter) * @param string $sort Resource field to sort on (optional) * @param int $offset Offset for results * @param int $limit Limit for results (null for none) + * @param bool $fuzzy Are we doing an exact or fuzzy search? * * @return array */ - public function fuzzyResourceSearch($query, $source = null, $sort = null, - $offset = 0, $limit = null + public function resourceSearch($q, $source = null, $sort = null, + $offset = 0, $limit = null, $fuzzy = true ) { - $cb = function ($select) use ($query, $source, $sort, $offset, $limit) { + $cb = function ($select) use ($q, $source, $sort, $offset, $limit, $fuzzy) { $select->columns( [ new Expression( @@ -117,7 +140,13 @@ class Tags extends Gateway 'rt.resource_id = resource.id', '*' ); - $select->where->literal('lower(tags.tag) like lower(?)', [$query]); + if ($fuzzy) { + $select->where->literal('lower(tags.tag) like lower(?)', [$q]); + } else if (!$this->caseSensitive) { + $select->where->literal('lower(tags.tag) = lower(?)', [$q]); + } else { + $select->where->equalTo('tags.tag', $q); + } if (!empty($source)) { $select->where->equalTo('source', $source); @@ -181,7 +210,9 @@ class Tags extends Gateway // SELECT (do not add table prefixes) $select->columns( [ - 'id', 'tag', + 'id', + 'tag' => $this->caseSensitive + ? 'tag' : new Expression('lower(tag)'), 'cnt' => new Expression( 'COUNT(DISTINCT(?))', ["rt.user_id"], [Expression::TYPE_IDENTIFIER] @@ -210,16 +241,77 @@ class Tags extends Gateway $select->where->isNotNull('rt.list_id'); } else if ($list === false) { $select->where->isNull('rt.list_id'); - } else if (!is_null($list)) { + } else if (null !== $list) { $select->where->equalTo('rt.list_id', $list); } - if (!is_null($user)) { + if (null !== $user) { $select->where->equalTo('rt.user_id', $user); } } ); } + /** + * Get a list of all tags generated by the user in favorites lists. Note that + * the returned list WILL NOT include tags attached to records that are not + * saved in favorites lists. + * + * @param string $userId User ID to look up. + * @param string $resourceId Filter for tags tied to a specific resource (null + * for no filter). + * @param int $listId Filter for tags tied to a specific list (null for no + * filter). + * @param string $source Filter for tags tied to a specific record source. + * + * @return \Zend\Db\ResultSet\AbstractResultSet + */ + public function getForUser($userId, $resourceId = null, $listId = null, + $source = DEFAULT_SEARCH_BACKEND + ) { + $callback = function ($select) use ($userId, $resourceId, $listId, $source) { + $select->columns( + [ + 'id' => new Expression( + 'min(?)', ['tags.id'], + [Expression::TYPE_IDENTIFIER] + ), + 'tag' => $this->caseSensitive + ? 'tag' : new Expression('lower(tag)'), + 'cnt' => new Expression( + 'COUNT(DISTINCT(?))', ['rt.resource_id'], + [Expression::TYPE_IDENTIFIER] + ) + ] + ); + $select->join( + ['rt' => 'resource_tags'], 'tags.id = rt.tag_id', [] + ); + $select->join( + ['r' => 'resource'], 'rt.resource_id = r.id', [] + ); + $select->join( + ['ur' => 'user_resource'], 'r.id = ur.resource_id', [] + ); + $select->group(['tag'])->order([new Expression('lower(tag)')]); + + $select->where->equalTo('ur.user_id', $userId) + ->equalTo('rt.user_id', $userId) + ->equalTo( + 'ur.list_id', 'rt.list_id', + Predicate::TYPE_IDENTIFIER, Predicate::TYPE_IDENTIFIER + ) + ->equalTo('r.source', $source); + + if (null !== $resourceId) { + $select->where->equalTo('r.record_id', $resourceId); + } + if (null !== $listId) { + $select->where->equalTo('rt.list_id', $listId); + } + }; + return $this->select($callback); + } + /** * Get a subquery used for flagging tag ownership (see getForResource). * @@ -264,7 +356,9 @@ class Tags extends Gateway $callback = function ($select) use ($sort, $limit, $extra_where) { $select->columns( [ - 'id', 'tag', + 'id', + 'tag' => $this->caseSensitive + ? 'tag' : new Expression('lower(tag)'), 'cnt' => new Expression( 'COUNT(DISTINCT(?))', ['resource_tags.resource_id'], [Expression::TYPE_IDENTIFIER] @@ -333,7 +427,7 @@ class Tags extends Gateway /** * Get a list of duplicate tags (this should never happen, but past bugs - * have introduced problems). + * and the introduction of case-insensitive tags have introduced problems). * * @return mixed */ @@ -351,7 +445,9 @@ class Tags extends Gateway ) ] ); - $select->group('tag'); + $select->group( + $this->caseSensitive ? 'tag' : new Expression('lower(tag)') + ); $select->having->greaterThan('cnt', 1); }; return $this->select($callback); @@ -399,7 +495,7 @@ class Tags extends Gateway protected function fixDuplicateTag($tag) { // Make sure this really is a duplicate. - $result = $this->select(['tag' => $tag]); + $result = $this->getByText($tag, false, false); if (count($result) < 2) { return; } diff --git a/module/VuFind/src/VuFind/Search/Tags/Results.php b/module/VuFind/src/VuFind/Search/Tags/Results.php index 5e35794018f..5cb79bf9231 100644 --- a/module/VuFind/src/VuFind/Search/Tags/Results.php +++ b/module/VuFind/src/VuFind/Search/Tags/Results.php @@ -46,7 +46,7 @@ class Results extends BaseResults * * @return string */ - protected function formatTagQuery($q) + protected function formatFuzzyQuery($q) { // Change unescaped asterisks to percent signs to translate more common // wildcard character into format used by database. @@ -54,16 +54,20 @@ class Results extends BaseResults } /** - * Return resources associated with the user tag query, using fuzzy matching. + * Return resources associated with the user tag query. + * + * @param bool $fuzzy Is this a fuzzy query or an exact match? * * @return array */ - protected function performFuzzyTagSearch() + protected function performTagSearch($fuzzy) { $table = $this->getTable('Tags'); - $query = $this->formatTagQuery($this->getParams()->getDisplayQuery()); - $rawResults = $table->fuzzyResourceSearch( - $query, null, $this->getParams()->getSort() + $query = $fuzzy + ? $this->formatFuzzyQuery($this->getParams()->getDisplayQuery()) + : $this->getParams()->getDisplayQuery(); + $rawResults = $table->resourceSearch( + $query, null, $this->getParams()->getSort(), 0, null, $fuzzy ); // How many results were there? @@ -72,39 +76,9 @@ class Results extends BaseResults // Apply offset and limit if necessary! $limit = $this->getParams()->getLimit(); if ($this->resultTotal > $limit) { - $rawResults = $table->fuzzyResourceSearch( + $rawResults = $table->resourceSearch( $query, null, $this->getParams()->getSort(), - $this->getStartRecord() - 1, $limit - ); - } - - return $rawResults->toArray(); - } - - /** - * Return resources associated with the user tag query, using exact matching. - * - * @return array - */ - protected function performExactTagSearch() - { - $table = $this->getTable('Tags'); - $tag = $table->getByText($this->getParams()->getDisplayQuery(), false); - if (empty($tag)) { - $this->resultTotal = 0; - return []; - } - $rawResults = $tag->getResources(null, $this->getParams()->getSort()); - - // How many results were there? - $this->resultTotal = count($rawResults); - - // Apply offset and limit if necessary! - $limit = $this->getParams()->getLimit(); - if ($this->resultTotal > $limit) { - $rawResults = $tag->getResources( - null, $this->getParams()->getSort(), $this->getStartRecord() - 1, - $limit + $this->getStartRecord() - 1, $limit, $fuzzy ); } @@ -123,16 +97,14 @@ class Results extends BaseResults // we are coming in from a search, in which case we want to do a fuzzy // search that supports wildcards, or else we are coming in from a tag // link, in which case we want to do an exact match. - $rawResults = $this->getParams()->isFuzzyTagSearch() - ? $this->performFuzzyTagSearch() - : $this->performExactTagSearch(); + $results = $this->performTagSearch($this->getParams()->isFuzzyTagSearch()); // Retrieve record drivers for the selected items. $callback = function ($row) { return ['id' => $row['record_id'], 'source' => $row['source']]; }; $this->results = $this->getServiceLocator()->get('VuFind\RecordLoader') - ->loadBatch(array_map($callback, $rawResults)); + ->loadBatch(array_map($callback, $results)); } /** diff --git a/themes/bootstrap3/templates/upgrade/fixduplicatetags.phtml b/themes/bootstrap3/templates/upgrade/fixduplicatetags.phtml index 3b6188eddac..c403778a3a4 100644 --- a/themes/bootstrap3/templates/upgrade/fixduplicatetags.phtml +++ b/themes/bootstrap3/templates/upgrade/fixduplicatetags.phtml @@ -8,12 +8,14 @@ <h2><?=$this->transEsc('Upgrade VuFind')?></h2> <?=$this->flashmessages()?> -<p>Due to a bug in earlier versions of VuFind, you have some duplicate tags -in your database. It is recommended that you fix these. Click Submit to proceed.</p> +<p>Some duplicate tags have been detected in your database. This may be due to a bug in an earlier +version of VuFind, or it may be related to a change to your case_sensitive_tags setting in the [Social] +section of VuFind. If you want case-sensitive tags, make sure that the setting is on and try again; +otherwise, it is recommended that you fix these. Click Submit to proceed.</p> <p>If you do not wish to fix the problem at this time, click the Skip button.</p> -<p>See <a target="_jira" href="https://vufind.org/jira/browse/VUFIND-805">https://vufind.org/jira/browse/VUFIND-805</a> for more details.</p> +<p>See <a target="_jira" href="https://vufind.org/jira/browse/VUFIND-805">https://vufind.org/jira/browse/VUFIND-805</a> and <a target="_jira" href="https://vufind.org/jira/browse/VUFIND-1187">https://vufind.org/jira/browse/VUFIND-1187</a> for more details.</p> <br /> -- GitLab