Page MenuHomePhabricator
Create Task
Maniphest T208777

Factor notification-related methods out of User and Title, introduce WatchlistNotificationManager
Closed, ResolvedPublic
Actions

Description

The following methods should be factored out into a stateless service, WatchlistNotificationManager:

  • User::clearNotification
  • Title::getNotificationTimestamp

The new service would wrap a WatchedItemStore and a TalkPageNotificationManager (T239640).

Details

Related Changes in Gerrit:
SubjectRepoBranchLines +/-
WatchlistNotificationManager: Add deprecations and release notesmediawiki/coremaster+13 -1
Add new WatchlistNotificationManager servicemediawiki/coremaster+926 -111
Customize query in gerrit

Related Objects
Search...

Event Timeline

daniel created this task.Nov 5 2018, 8:02 PM
• CCicalese_WMF moved this task from Inbox to Later on the Platform Team Legacy board.Nov 6 2018, 5:58 PM
• CCicalese_WMF edited projects, added Platform Team Legacy (Later); removed Platform Team Legacy.
Tgr subscribed.Nov 19 2018, 10:39 PM

Related: T128351: Notifications should be in core

• WDoranWMF moved this task from Later to Mop Column on the Platform Team Legacy board.Jul 5 2019, 3:43 PM
• WDoranWMF removed a project: Platform Team Legacy (Later).
• WDoranWMF edited projects, added Platform Engineering (Decoupling (CDP2)); removed Platform Engineering (Needs Cleaning - Code Health (TEC13)).Jul 9 2019, 6:59 PM
• WDoranWMF moved this task from Decoupling (CDP2) to mop on the Platform Engineering board.Jul 26 2019, 6:55 PM
• WDoranWMF edited projects, added Core Platform Team Initiatives (Decoupling (CDP2)); removed Platform Engineering (Decoupling (CDP2)).
daniel renamed this task from Decide how to factor notification-related methods out of User and Title to Factor notification-related methods out of User and Title.Dec 2 2019, 5:54 PM
daniel updated the task description. (Show Details)
daniel renamed this task from Factor notification-related methods out of User and Title to Factor notification-related methods out of User and Title, introduce WatchlistNotificationManager.Dec 2 2019, 5:57 PM
daniel updated the task description. (Show Details)
tosfos subscribed.Dec 2 2019, 6:26 PM
daniel added a subscriber: • MSchottlender-WMF.Dec 2 2019, 9:36 PM
daniel added a comment.Dec 4 2019, 10:20 AM

We should probably also revive T208766: Deprecate User::isWatched, isTempWatched, addWatch, and removeWatch

• CCicalese_WMF removed daniel as the assignee of this task.Dec 16 2019, 6:14 PM
tosfos assigned this task to Vedmaka.Dec 16 2019, 6:25 PM
Vedmaka mentioned this in T239640: Introduce TalkPageNotificationManager service.Feb 19 2020, 3:54 PM
daniel added a comment.Mar 27 2020, 10:33 AM

Merge with T239640: Introduce TalkPageNotificationManager service?

DannyS712 subscribed.May 16 2020, 4:59 AM
In T208777#6004612, @daniel wrote:

Merge with T239640: Introduce TalkPageNotificationManager service?

Given that T239640 has been merged and the new service deployed, I think this needs to be separate.
@Vedmaka are you working on this?

• Clarakosi claimed this task.May 18 2020, 3:17 PM
• Clarakosi added a project: Platform Team Workboards (Clinic Duty Team).
• Clarakosi added subscribers: Vedmaka, • Clarakosi.
In T208777#6142119, @DannyS712 wrote:
In T208777#6004612, @daniel wrote:

Merge with T239640: Introduce TalkPageNotificationManager service?

Given that T239640 has been merged and the new service deployed, I think this needs to be separate.

+1

@Vedmaka are you working on this?

I'm picking this up under Clinic Duty but do let me know if you were already working on it.

• Clarakosi moved this task from Inbox to Doing(WIP:5) on the Platform Team Workboards (Clinic Duty Team) board.May 18 2020, 3:18 PM
DannyS712 added a comment.May 18 2020, 4:52 PM
In T208777#6145162, @Clarakosi wrote:
In T208777#6142119, @DannyS712 wrote:
In T208777#6004612, @daniel wrote:

Merge with T239640: Introduce TalkPageNotificationManager service?

Given that T239640 has been merged and the new service deployed, I think this needs to be separate.

+1

@Vedmaka are you working on this?

I'm picking this up under Clinic Duty but do let me know if you were already working on it.

I was working on it at a preliminary level :) If you want me to do this, feel free to assign it to me

DannyS712 added a comment.May 18 2020, 4:58 PM

Notes:
User::clearAllNotifications should probably also be moved
Suggest waiting until after HookContainer/HookRunner migrations, since User::clearNotification runs a hook
Services to be injected:

  • ReadOnlyMode
  • PermissionManager
  • HookContainer
  • TalkPageNotificationManager
  • RevisionLookup
  • WatchedItemStore
• Clarakosi removed a project: Platform Team Workboards (Clinic Duty Team).May 18 2020, 5:37 PM

I was working on it at a preliminary level :) If you want me to do this, feel free to assign it to me

Assigning it to you :)

• Clarakosi reassigned this task from • Clarakosi to DannyS712.May 18 2020, 5:37 PM
Restricted Application added a project: User-DannyS712. · View Herald TranscriptMay 18 2020, 5:37 PM
DannyS712 moved this task from Unsorted to Next on the User-DannyS712 board.EditedMay 18 2020, 5:43 PM

More notes:
User::clearNotification accepts a title by reference - why? It doesn't appear to be replaced, so accepting it as a value should be enough
New service should only need UserIdentity and LinkTarget, rather than User and Title
Title::getNotificationTimestamp includes its own caching - since the new service has to cache values for a user-title combination, rather than a title caching values for a user, the caching logic will be a bit trickier

DannyS712 mentioned this in T253435: UserClearNewTalkNotification hook: document UserIdentity instead of User.May 23 2020, 5:05 AM
gerritbot added a comment.May 23 2020, 8:43 AM

Change 598150 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Add new WatchlistNotificationManager service

https://gerrit.wikimedia.org/r/598150

gerritbot added a project: Patch-For-Review.May 23 2020, 8:43 AM
DannyS712 moved this task from Next to Awaiting review and deployment on the User-DannyS712 board.May 23 2020, 8:46 AM
DannyS712 added a comment.May 23 2020, 10:08 AM
In T208777#6145872, @Clarakosi wrote:

I was working on it at a preliminary level :) If you want me to do this, feel free to assign it to me

Assigning it to you :)

Patch pending at https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/598150/

• Clarakosi closed subtask T239640: Introduce TalkPageNotificationManager service as Resolved.May 26 2020, 8:47 PM
• Clarakosi added a project: Platform Team Workboards (External Code Reviews).May 26 2020, 9:49 PM
• Clarakosi moved this task from External Code Review Needed to External Code Review In Progress on the Platform Team Workboards (External Code Reviews) board.
DannyS712 closed subtask T253435: UserClearNewTalkNotification hook: document UserIdentity instead of User as Resolved.May 29 2020, 10:48 AM
gerritbot added a comment.Jun 4 2020, 8:24 PM

Change 598150 merged by jenkins-bot:
[mediawiki/core@master] Add new WatchlistNotificationManager service

https://gerrit.wikimedia.org/r/598150

gerritbot added a comment.Jun 4 2020, 8:51 PM

Change 602493 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] WatchlistNotificationManager: Add deprecations and release notes

https://gerrit.wikimedia.org/r/602493

ReleaseTaggerBot added a project: MW-1.35-notes (1.35.0-wmf.36; 2020-06-09).Jun 4 2020, 9:00 PM
gerritbot added a comment.Jun 5 2020, 2:31 AM

Change 602493 merged by jenkins-bot:
[mediawiki/core@master] WatchlistNotificationManager: Add deprecations and release notes

https://gerrit.wikimedia.org/r/602493

DannyS712 closed this task as Resolved.Jun 5 2020, 2:32 AM
DannyS712 removed a project: Patch-For-Review.
DannyS712 mentioned this in T254542: Replace uses of deprecated User and Title methods with WatchlistNotificationManager.

Follow-up to replace uses it at T254542: Replace uses of deprecated User and Title methods with WatchlistNotificationManager

DannyS712 mentioned this in T233385: Expand phpunit tests for User.php.Jun 6 2020, 4:51 AM
DannyS712 moved this task from Awaiting review and deployment to Merged on the User-DannyS712 board.Jul 9 2020, 11:16 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits