Page MenuHomePhabricator

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

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).

Event Timeline

daniel created this task.Nov 5 2018, 8:02 PM
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 added a subscriber: tosfos.Dec 2 2019, 6:26 PM
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

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 added subscribers: Vedmaka, Clarakosi.

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.

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

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

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

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

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/

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

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

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

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

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