From 3df4ed65e519b492b5d0049cf6d6916cf54694a8 Mon Sep 17 00:00:00 2001 From: STran Date: Thu, 31 Oct 2024 08:25:50 -0700 Subject: [PATCH] Parameterize ChangeTags::buildTagFilterSelector to support various tag sets Why: `ChangeTags::buildTagFilterSelector` is an opinionated chain of calls that results in the markup for a select input with specific tag options (explicitly and software defined tags that have hits). In order to support customization to the `HTMLTagFilter` widget, add support for parameters. These parameters will support filtering for active-only tags or not and choosing between all on-wiki tags or software-defined tags only. What: - Support an `activeOnly` parameter, which will either show all defined tags or only tags that have hits (active) + For legibility, add `TAG_SET_ACTIVE_ONLY` and `TAG_SET_ALL` constants to support this parameter - Support a `useAllTags` parameter, which if true will use all tags and if which false will only use software-defined tags + For legibility, add `USE_ALL_TAGS` and `USE_SOFTWARE_TAGS_ONLY` constants to support this parameter Bug: T378622 Change-Id: Ib6ba27944cdf22bdb05dbfd34b2e5f8727261da7 --- includes/changetags/ChangeTags.php | 101 +++++++++++++++--- .../includes/changetags/ChangeTagsTest.php | 92 ++++++++++++++++ 2 files changed, 176 insertions(+), 17 deletions(-) diff --git a/includes/changetags/ChangeTags.php b/includes/changetags/ChangeTags.php index 9f34a2c6af2..728b3ba63db 100644 --- a/includes/changetags/ChangeTags.php +++ b/includes/changetags/ChangeTags.php @@ -139,6 +139,29 @@ class ChangeTags { public const DISPLAY_TABLE_ALIAS = 'changetagdisplay'; + /** + * Constants that can be used to set the `activeOnly` parameter for calling + * self::buildCustomTagFilterSelect in order to improve function/parameter legibility + * + * If TAG_SET_ACTIVE_ONLY is used then the hit count for each tag will be checked against + * and only tags with hits will be returned + * Otherwise if TAG_SET_ALL is used then all tags will be returned regardlesss of if they've + * ever been used or not + */ + public const TAG_SET_ACTIVE_ONLY = true; + public const TAG_SET_ALL = false; + + /** + * Constants that can be used to set the `useAllTags` parameter for calling + * self::buildCustomTagFilterSelect in order to improve function/parameter legibility + * + * If USE_ALL_TAGS is used then all on-wiki tags will be returned + * Otherwise if USE_SOFTWARE_TAGS_ONLY is used then only mediawiki core-defined tags + * will be returned + */ + public const USE_ALL_TAGS = true; + public const USE_SOFTWARE_TAGS_ONLY = false; + /** * Loads defined core tags, checks for invalid types (if not array), * and filters for supported and enabled (if $all is false) tags only. @@ -717,7 +740,8 @@ class ChangeTags { } /** - * Build a text box to select a change tag + * Build a text box to select a change tag. The tag set can be customized via the $activeOnly + * and $useAllTags parameters and defaults to all active tags. * * @param string $selected Tag to select by default * @param bool $ooui Use an OOUI TextInputWidget as selector instead of a non-OOUI input field @@ -725,10 +749,15 @@ class ChangeTags { * @param IContextSource|null $context * @note Even though it takes null as a valid argument, an IContextSource is preferred * in a new code, as the null value can change in the future + * @param bool $activeOnly Whether to filter for tags that have been used or not + * @param bool $useAllTags Whether to use all known tags or to only use software defined tags + * These map to ChangeTagsStore->listDefinedTags and ChangeTagsStore->getSoftwareTags respectively * @return array an array of (label, selector) */ public static function buildTagFilterSelector( - $selected = '', $ooui = false, ?IContextSource $context = null + $selected = '', $ooui = false, ?IContextSource $context = null, + bool $activeOnly = self::TAG_SET_ACTIVE_ONLY, + bool $useAllTags = self::USE_ALL_TAGS ) { if ( !$context ) { $context = RequestContext::getMain(); @@ -741,7 +770,13 @@ class ChangeTags { return []; } - $tags = self::getChangeTagList( $context, $context->getLanguage() ); + $tags = self::getChangeTagList( + $context, + $context->getLanguage(), + $activeOnly, + $useAllTags + ); + $autocomplete = []; foreach ( $tags as $tagInfo ) { $autocomplete[ $tagInfo['label'] ] = $tagInfo['name']; @@ -1259,6 +1294,7 @@ class ChangeTags { /** * Get information about change tags, without parsing messages, for tag filter dropdown menus. + * By default, this will return explicitly-defined and software-defined tags that are currently active (have hits) * * Message contents are the raw values (->plain()), because parsing messages is expensive. * Even though we're not parsing messages, building a data structure with the contents of @@ -1280,24 +1316,49 @@ class ChangeTags { * * @param MessageLocalizer $localizer * @param Language $lang + * @param bool $activeOnly + * @param bool $useAllTags * @return array[] Information about each tag */ - public static function getChangeTagListSummary( MessageLocalizer $localizer, Language $lang ) { + public static function getChangeTagListSummary( + MessageLocalizer $localizer, + Language $lang, + bool $activeOnly = self::TAG_SET_ACTIVE_ONLY, + bool $useAllTags = self::USE_ALL_TAGS + ) { + $changeTagStore = MediaWikiServices::getInstance()->getChangeTagsStore(); + + if ( $useAllTags ) { + $tagKeys = $changeTagStore->listDefinedTags(); + $cacheKey = 'tags-list-summary'; + } else { + $tagKeys = $changeTagStore->getSoftwareTags( true ); + $cacheKey = 'core-software-tags-summary'; + } + + // if $tagHitCounts exists, check against it later to determine whether or not to omit tags + $tagHitCounts = null; + if ( $activeOnly ) { + $tagHitCounts = $changeTagStore->tagUsageStatistics(); + } else { + // The full set of tags should use a different cache key than the subset + $cacheKey .= '-all'; + } + $cache = MediaWikiServices::getInstance()->getMainWANObjectCache(); return $cache->getWithSetCallback( - $cache->makeKey( 'tags-list-summary', $lang->getCode() ), + $cache->makeKey( $cacheKey, $lang->getCode() ), WANObjectCache::TTL_DAY, - static function ( $oldValue, &$ttl, array &$setOpts ) use ( $localizer ) { - $changeTagStore = MediaWikiServices::getInstance()->getChangeTagsStore(); - $tagHitCounts = $changeTagStore->tagUsageStatistics(); - + static function ( $oldValue, &$ttl, array &$setOpts ) use ( $localizer, $tagKeys, $tagHitCounts ) { $result = []; - // Only list tags that are still actively defined - foreach ( $changeTagStore->listDefinedTags() as $tagName ) { - // Only list tags with more than 0 hits - $hits = $tagHitCounts[$tagName] ?? 0; - if ( $hits <= 0 ) { - continue; + foreach ( $tagKeys as $tagName ) { + // Only list tags that are still actively defined + if ( $tagHitCounts !== null ) { + // Only list tags with more than 0 hits + $hits = $tagHitCounts[$tagName] ?? 0; + if ( $hits <= 0 ) { + continue; + } } $labelMsg = self::tagShortDescriptionMessage( $tagName, $localizer ); @@ -1329,10 +1390,16 @@ class ChangeTags { * * @param MessageLocalizer $localizer * @param Language $lang + * @param bool $activeOnly + * @param bool $useAllTags * @return array[] Same as getChangeTagListSummary(), with messages parsed, stripped and truncated */ - public static function getChangeTagList( MessageLocalizer $localizer, Language $lang ) { - $tags = self::getChangeTagListSummary( $localizer, $lang ); + public static function getChangeTagList( + MessageLocalizer $localizer, Language $lang, + bool $activeOnly = self::TAG_SET_ACTIVE_ONLY, bool $useAllTags = self::USE_ALL_TAGS + ) { + $tags = self::getChangeTagListSummary( $localizer, $lang, $activeOnly, $useAllTags ); + foreach ( $tags as &$tagInfo ) { if ( $tagInfo['labelMsg'] ) { // Use localizer with the correct page title to parse plain message from the cache. diff --git a/tests/phpunit/includes/changetags/ChangeTagsTest.php b/tests/phpunit/includes/changetags/ChangeTagsTest.php index b756c7c1791..c960e24ccb5 100644 --- a/tests/phpunit/includes/changetags/ChangeTagsTest.php +++ b/tests/phpunit/includes/changetags/ChangeTagsTest.php @@ -2,6 +2,7 @@ use MediaWiki\Language\RawMessage; use MediaWiki\MainConfigNames; +use MediaWiki\MediaWikiServices; use Wikimedia\Rdbms\Platform\ISQLPlatform; /** @@ -28,6 +29,97 @@ class ChangeTagsTest extends MediaWikiIntegrationTestCase { // TODO most methods are not tested + public function testBuildTagFilterSelector_allTags() { + // Set `activeOnly` to false + // Expect that at least all the software defined tags are returned + $allTags = MediaWikiServices::getInstance()->getChangeTagsStore()->listDefinedTags(); + $allTagsList = ChangeTags::getChangeTagListSummary( + RequestContext::getMain(), + RequestContext::getMain()->getLanguage(), + ChangeTags::TAG_SET_ALL + ); + $this->assertTrue( + count( $allTagsList ) >= count( $allTags ), + '`activeOnly` is false, expect all software tags' + ); + } + + public function testBuildTagFilterSelector_allSoftwareTags() { + // Set both `activeOnly` and `useAllTags` to false + // Expect that only software defined tags are returned + $allSoftwareTags = MediaWikiServices::getInstance()->getChangeTagsStore()->getSoftwareTags( true ); + $allSoftwareTagsList = ChangeTags::getChangeTagListSummary( + RequestContext::getMain(), + RequestContext::getMain()->getLanguage(), + ChangeTags::TAG_SET_ALL, + ChangeTags::USE_SOFTWARE_TAGS_ONLY + ); + $this->assertTrue( + count( $allSoftwareTagsList ) == count( $allSoftwareTags ), + '`activeOnly` and `useAllTags` are false, expect only software tags' + ); + } + + public function testBuildTagFilterSelector_activeOnlyNoHits() { + // Enable and test `activeOnly` and expect no tags returned, + // as there are currently no tagged edits in the test database + $emptyTagListSummary = ChangeTags::getChangeTagListSummary( + RequestContext::getMain(), + RequestContext::getMain()->getLanguage(), + ChangeTags::TAG_SET_ACTIVE_ONLY + ); + $this->assertCount( 0, $emptyTagListSummary, '`activeOnly` is true and no hits, expect no tags' ); + + // Assert that by default, an empty select is returned, as no tags have been used yet + $this->assertEquals( + [ + '', + '' + ], + ChangeTags::buildTagFilterSelector( + '', false, RequestContext::getMain() + ) + ); + } + + public function testBuildTagFilterSelector_activeOnly() { + // Disable patrolling so reverts will happen without approval + $this->overrideConfigValues( [ MainConfigNames::UseRCPatrol => false ] ); + + // Make an edit and replace the content, adding the `mw-replace` tag to the revision + $page = $this->getExistingTestPage(); + $this->editPage( $page, '1' ); + $this->editPage( + $page, '0', '', NS_MAIN, $this->getTestUser()->getUser() + ); + + // Ensure all deferred updates are run + DeferredUpdates::doUpdates(); + + // Assert that only the `mw-replace` tag is returned + $replaceOnlyTagList = ChangeTags::getChangeTagListSummary( + RequestContext::getMain(), + RequestContext::getMain()->getLanguage() + ); + $this->assertCount( 1, $replaceOnlyTagList, '`activeOnly` is true with 1 hit, return 1 tag' ); + $this->assertEquals( + 'mw-replace', $replaceOnlyTagList[0]['name'], + '`activeOnly` is true with 1 hit, return expected tag' + ); + + // Assert that the tag is reflected in the default markup returned + $this->assertEquals( + [ + '', + '' + ], + ChangeTags::buildTagFilterSelector( + '', false, RequestContext::getMain() + ), + '`activeOnly` is true with 1 hit, return expected tag markup' + ); + } + /** @dataProvider provideModifyDisplayQuery */ public function testModifyDisplayQuery( $origQuery,