Page MenuHomePhabricator

Build mark as read/unread functionality into data controller
Closed, ResolvedPublic

Description

Building off of work in https://phabricator.wikimedia.org/T287298, add ability to mark notifications as read or unread, to be called from the notification center or a rich notification quick action.

API documentation: https://www.mediawiki.org/wiki/Notifications/API action=echomarkread section

  • Add fetcher method (or audit and lean on any existing ones, see here) for marking multiple notification IDs as read or unread, using list and unreadlist parameters.
  • Add fetcher method (or audit and lean on any existing ones) for marking all notifications as read, using the all=true parameter.
  • Clean out any old unused mark as read code.

There's a couple of approaches for syncing with Core Data on this. For method 1 (mark specific IDs as read):

  1. The simple approach (let's lean towards this one):

Upon user marking a notification as read or unread, pull the managed object for that ID and flip the flag optimistically and save (on background context, so that we can keep the view context read-only). Then make the API call to mark as read or unread. If the API call fails, toggle the flag back to where it was. Note this may result in glitchy UI on this failure case.

  1. The harder approach:

Each notification object has a readLocally, readRemotely, and attemptsMarkReadUnread flag. The view state is based on the readLocally flag. Immediately toggle the readLocally flag, then make the API call. If it fails, increment attemptsMarkReadUnread. Then have a process that occasionally scoops up all core data objects with readLocally = 1 and readRemotely = 0 and attempts to mark them in the API again, up until a attemptsMarkReadUnread is hit. If this is successful set readRemotely = 1 and attemptsMarkReadUnread = 0, if it fails, set attemptsMarkReadUnread = 0 and readLocally = 0. The inverse of this will need happen for marking as unread.

For method 2 (mark all as read):

We could just fetch all unread local notifications and mark their flags as read. Alternatively we could call the syncing method setup up in https://phabricator.wikimedia.org/T287298 step 7.

Event Timeline

LGoto triaged this task as Medium priority.Jul 26 2021, 6:42 PM

Notes from engineering sync:

Let's rework this description to see if we can lean on any of the current notifications code, rather than redoing.

General idea was:

Current notifications data can be deleted (no need to worry about migration)
Current notifications code for syncing and marking as read can possibly be salvaged, we'll just want to remove it from polling to reacting to app notification center actions.

API digging notes: I'm often triggering a "missing token" parameter when attempting to mark as read across multiple foreign wikis. I think this may be a bug within the API, so I think it's preferable for us to trigger marking as read separately, across the different subdomains to sidestep this.

For example, marking all as read on the server:

Gather all unread notification wikis in app notification center, and call:

https://fr.wikipedia.org/w/api.php?action=echomarkread&format=json&all=1
https://en.wikipedia.org/w/api.php?action=echomarkread&format=json&all=1
etc.

For marking individual wikis as read, divide them up by wiki and call:

https://fr.wikipedia.org/w/api.php?action=echomarkread&format=json&list=1|2|3
https://en.wikipedia.org/w/api.php?action=echomarkread&format=json&list=4|5|6|7
etc.

We can make the code for API calling, but we can't fully test this until the UI for marking as read is in place. Moving into Ready for Dev.

This isn't really testable until we get the UI portion of this in, so moving to PM signoff.

JMinor claimed this task.
JMinor added a subscriber: JMinor.

This will be tested and accepted via the UI ticket.