From 0d57aa9762a5755199f18f5f3ec0ec11a1f87105 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartosz=20Dziewo=C5=84ski?= Date: Tue, 17 Aug 2021 22:23:27 +0200 Subject: [PATCH] Automatic topic subscriptions (on any edit) Bug: T284836 Change-Id: Ia42ad087218fd91a0cdd1664157d1049738e3c01 --- extension.json | 5 +++ includes/Hooks/HookUtils.php | 35 +++++++++++++-- includes/Hooks/PreferenceHooks.php | 7 +++ includes/Notifications/EventDispatcher.php | 25 ++++++++++- includes/SubscriptionStore.php | 52 ++++++++++++++++++---- sql/discussiontools_subscription.json | 2 +- tests/phpunit/MockEventDispatcher.php | 11 +++++ tests/phpunit/MockSubscriptionStore.php | 6 +-- 8 files changed, 126 insertions(+), 17 deletions(-) diff --git a/extension.json b/extension.json index f1d4c1331..cfaed26bf 100644 --- a/extension.json +++ b/extension.json @@ -497,6 +497,7 @@ "discussiontools-replytool": 1, "discussiontools-sourcemodetoolbar": 1, "discussiontools-topicsubscription": 1, + "discussiontools-autotopicsub": 1, "discussiontools-abtest": "" }, "config": { @@ -532,6 +533,10 @@ "value": "default", "description": "Override availability of DiscussionTools topic subscription feature. 'default', 'available', or 'unavailable'." }, + "DiscussionTools_autotopicsub": { + "value": "default", + "description": "Override availability of DiscussionTools automatic topic subscription feature. 'default', 'available', or 'unavailable'." + }, "DiscussionToolsEnableTopicSubscriptionBackend": { "value": true, "description": "Enable the topic subscription backend. This controls whether the feature can be tried out using the query parameter, and whether the Echo events will be created. Do not enable this unless the database tables exist. Do enable it before making the 'topicsubscription' feature available." diff --git a/includes/Hooks/HookUtils.php b/includes/Hooks/HookUtils.php index c5f665690..2e6742490 100644 --- a/includes/Hooks/HookUtils.php +++ b/includes/Hooks/HookUtils.php @@ -23,6 +23,8 @@ class HookUtils { public const NEWTOPICTOOL = 'newtopictool'; public const SOURCEMODETOOLBAR = 'sourcemodetoolbar'; public const TOPICSUBSCRIPTION = 'topicsubscription'; + public const AUTOTOPICSUB = 'autotopicsub'; + /** * @var string[] List of all sub-features. Will be used to generate: * - Feature override global: $wgDiscussionTools_FEATURE @@ -34,6 +36,7 @@ class HookUtils { self::NEWTOPICTOOL, self::SOURCEMODETOOLBAR, self::TOPICSUBSCRIPTION, + self::AUTOTOPICSUB, ]; /** @@ -52,7 +55,7 @@ class HookUtils { return false; } - if ( $feature === self::TOPICSUBSCRIPTION && !$user->isRegistered() ) { + if ( ( $feature === self::TOPICSUBSCRIPTION || $feature === self::AUTOTOPICSUB ) && !$user->isRegistered() ) { // Users must be logged in to use topic subscription return false; } @@ -229,7 +232,10 @@ class HookUtils { // Topic subscription is not available on your own talk page, as you will // get 'edit-user-talk' notifications already. (T276996) - if ( $feature === self::TOPICSUBSCRIPTION && $title->equals( $output->getUser()->getTalkPage() ) ) { + if ( + ( $feature === self::TOPICSUBSCRIPTION || $feature === self::AUTOTOPICSUB ) && + $title->equals( $output->getUser()->getTalkPage() ) + ) { return false; } @@ -250,7 +256,7 @@ class HookUtils { RequestContext::getMain()->getRequest()->getRawVal( 'dtenable' ); if ( - $feature === self::TOPICSUBSCRIPTION && + ( $feature === self::TOPICSUBSCRIPTION || $feature === self::AUTOTOPICSUB ) && !$dtConfig->get( 'DiscussionToolsEnableTopicSubscriptionBackend' ) ) { // Can't be enabled via query, because the tables may not exist yet (T280082) @@ -328,4 +334,27 @@ class HookUtils { self::isFeatureEnabledForOutput( $out, self::NEWTOPICTOOL ) ); } + + /** + * Check if we should be adding automatic topic subscriptions for this user on this page. + * + * @param UserIdentity $user + * @param Title $title + * @return bool + */ + public static function shouldAddAutoSubscription( UserIdentity $user, Title $title ): bool { + // This duplicates the logic from isFeatureEnabledForOutput(), + // because we don't have access to the request or the output here. + + // Topic subscription is not available on your own talk page, as you will + // get 'edit-user-talk' notifications already. (T276996) + // (can't use User::getTalkPage() to check because this is a UserIdentity) + if ( $title->inNamespace( NS_USER_TALK ) && $title->getText() === $user->getName() ) { + return false; + } + + // Check if the user has automatic subscriptions enabled, and the tools are enabled on the page. + return static::isAvailableForTitle( $title ) && + static::isFeatureEnabledForUser( $user, self::AUTOTOPICSUB ); + } } diff --git a/includes/Hooks/PreferenceHooks.php b/includes/Hooks/PreferenceHooks.php index aac23383c..5ed092f40 100644 --- a/includes/Hooks/PreferenceHooks.php +++ b/includes/Hooks/PreferenceHooks.php @@ -95,6 +95,13 @@ class PreferenceHooks implements ]; } + if ( isset( $preferences['discussiontools-' . HookUtils::AUTOTOPICSUB] ) ) { + // Hide automatic subscriptions when subscriptions are disabled + $preferences['discussiontools-' . HookUtils::AUTOTOPICSUB]['hide-if'] = [ + '===', 'discussiontools-' . HookUtils::TOPICSUBSCRIPTION, '' + ]; + } + $preferences['discussiontools-showadvanced'] = [ 'type' => 'api', ]; diff --git a/includes/Notifications/EventDispatcher.php b/includes/Notifications/EventDispatcher.php index 39b976825..6b561bba9 100644 --- a/includes/Notifications/EventDispatcher.php +++ b/includes/Notifications/EventDispatcher.php @@ -266,6 +266,9 @@ class EventDispatcher { ], 'agent' => $user, ]; + + $titleForSubscriptions = Title::castFromPageIdentity( $title )->createFragmentTarget( $heading->getText() ); + static::addAutoSubscription( $user, $titleForSubscriptions, $heading->getName() ); } } @@ -282,6 +285,26 @@ class EventDispatcher { } ); } + /** + * Add an automatic subscription to the given item, assuming the user has automatic subscriptions + * enabled. + * + * @param UserIdentity $user + * @param Title $title + * @param string $itemName + */ + protected static function addAutoSubscription( UserIdentity $user, Title $title, string $itemName ) { + $dtConfig = MediaWikiServices::getInstance()->getConfigFactory()->makeConfig( 'discussiontools' ); + + if ( + $dtConfig->get( 'DiscussionToolsEnableTopicSubscriptionBackend' ) && + HookUtils::shouldAddAutoSubscription( $user, $title ) + ) { + $subscriptionStore = MediaWikiServices::getInstance()->getService( 'DiscussionTools.SubscriptionStore' ); + $subscriptionStore->addAutoSubscriptionForUser( $user, $title, $itemName ); + } + } + /** * Return all users subscribed to a comment * @@ -295,7 +318,7 @@ class EventDispatcher { $subscriptionStore = MediaWikiServices::getInstance()->getService( 'DiscussionTools.SubscriptionStore' ); $subscriptionItems = $subscriptionStore->getSubscriptionItemsForTopic( $commentName, - SubscriptionStore::STATE_SUBSCRIBED + [ SubscriptionStore::STATE_SUBSCRIBED, SubscriptionStore::STATE_AUTOSUBSCRIBED ] ); // Update notified timestamps diff --git a/includes/SubscriptionStore.php b/includes/SubscriptionStore.php index 3a21eea27..35ccc1389 100644 --- a/includes/SubscriptionStore.php +++ b/includes/SubscriptionStore.php @@ -26,6 +26,7 @@ class SubscriptionStore { */ public const STATE_UNSUBSCRIBED = 0; public const STATE_SUBSCRIBED = 1; + public const STATE_AUTOSUBSCRIBED = 2; /** @var ILBFactory */ private $lbFactory; @@ -69,14 +70,14 @@ class SubscriptionStore { * @param IDatabase $db * @param UserIdentity|null $user * @param array|null $itemNames - * @param int|null $state One of SubscriptionStore::STATE_* constants + * @param int|int[]|null $state One of (or an array of) SubscriptionStore::STATE_* constants * @return IResultWrapper|false */ private function fetchSubscriptions( IDatabase $db, ?UserIdentity $user = null, ?array $itemNames = null, - ?int $state = null + $state = null ) { $conditions = []; @@ -111,14 +112,14 @@ class SubscriptionStore { /** * @param UserIdentity $user * @param array|null $itemNames - * @param int|null $state One of SubscriptionStore::STATE_* constants + * @param int[]|null $state Array of SubscriptionStore::STATE_* constants * @param array $options * @return SubscriptionItem[] */ public function getSubscriptionItemsForUser( UserIdentity $user, ?array $itemNames = null, - ?int $state = null, + ?array $state = null, array $options = [] ): array { // Only a registered user can be subscribed @@ -151,13 +152,13 @@ class SubscriptionStore { /** * @param string $itemName - * @param int|null $state One of SubscriptionStore::STATE_* constants + * @param int[]|null $state An array of SubscriptionStore::STATE_* constants * @param array $options * @return array */ public function getSubscriptionItemsForTopic( string $itemName, - ?int $state = null, + ?array $state = null, array $options = [] ): array { $options += [ 'forWrite' => false ]; @@ -239,12 +240,14 @@ class SubscriptionStore { * @param UserIdentity $user * @param LinkTarget $target * @param string $itemName + * @param int $state * @return bool */ public function addSubscriptionForUser( UserIdentity $user, LinkTarget $target, - string $itemName + string $itemName, + int $state = self::STATE_SUBSCRIBED ): bool { if ( $this->readOnlyMode->isReadOnly() ) { return false; @@ -265,12 +268,12 @@ class SubscriptionStore { 'sub_title' => $target->getDBkey(), 'sub_section' => $target->getFragment(), 'sub_item' => $itemName, - 'sub_state' => self::STATE_SUBSCRIBED, + 'sub_state' => $state, 'sub_created' => $dbw->timestamp(), ], [ [ 'sub_user', 'sub_item' ] ], [ - 'sub_state' => self::STATE_SUBSCRIBED, + 'sub_state' => $state, ], __METHOD__ ); @@ -306,6 +309,37 @@ class SubscriptionStore { return (bool)$dbw->affectedRows(); } + /** + * @param UserIdentity $user + * @param LinkTarget $target + * @param string $itemName + * @return bool + */ + public function addAutoSubscriptionForUser( + UserIdentity $user, + LinkTarget $target, + string $itemName + ): bool { + // Check for existing subscriptions. + // Note that this includes subscriptions with state=STATE_UNSUBSCRIBED. + $subscriptionItems = $this->getSubscriptionItemsForUser( + $user, + [ $itemName ], + null, + [ 'forWrite' => true ] + ); + if ( $subscriptionItems ) { + return false; + } + + return $this->addSubscriptionForUser( + $user, + $target, + $itemName, + self::STATE_AUTOSUBSCRIBED + ); + } + /** * @param string $field Timestamp field name * @param UserIdentity|null $user diff --git a/sql/discussiontools_subscription.json b/sql/discussiontools_subscription.json index e749d1165..db26b931f 100644 --- a/sql/discussiontools_subscription.json +++ b/sql/discussiontools_subscription.json @@ -49,7 +49,7 @@ }, { "name": "sub_state", - "comment": "0: unsubscribed; 1: subscribed", + "comment": "0: unsubscribed; 1: subscribed, 2: auto-subscribed", "type": "integer", "options": { "notnull": true, diff --git a/tests/phpunit/MockEventDispatcher.php b/tests/phpunit/MockEventDispatcher.php index 0e2ad7adc..e8e44ad4b 100644 --- a/tests/phpunit/MockEventDispatcher.php +++ b/tests/phpunit/MockEventDispatcher.php @@ -7,6 +7,7 @@ use MediaWiki\Extension\DiscussionTools\Notifications\EventDispatcher; use MediaWiki\Page\PageIdentity; use MediaWiki\Revision\RevisionRecord; use MediaWiki\User\UserIdentity; +use Title; class MockEventDispatcher extends EventDispatcher { @@ -50,4 +51,14 @@ class MockEventDispatcher extends EventDispatcher { public static function addCommentChangeTag( RevisionRecord $newRevRecord ) { } + /** + * No-op for testing + * + * @param UserIdentity $user + * @param Title $title + * @param string $itemName + */ + protected static function addAutoSubscription( UserIdentity $user, Title $title, string $itemName ) { + } + } diff --git a/tests/phpunit/MockSubscriptionStore.php b/tests/phpunit/MockSubscriptionStore.php index 2c46e43e0..e23a2cbdf 100644 --- a/tests/phpunit/MockSubscriptionStore.php +++ b/tests/phpunit/MockSubscriptionStore.php @@ -16,14 +16,14 @@ class MockSubscriptionStore extends SubscriptionStore { /** * @param UserIdentity $user Unused, required for inheritance * @param array|null $itemNames Unused, required for inheritance - * @param int|null $state Unused, required for inheritance + * @param int[]|null $state Unused, required for inheritance * @param array $options Unused, required for inheritance - * @return array + * @return SubscriptionItem[] */ public function getSubscriptionItemsForUser( UserIdentity $user, ?array $itemNames = null, - ?int $state = null, + ?array $state = null, array $options = [] ): array { return [];