Determine new notifications in extension and display #4046

tonisevener · 2021-10-01T16:48:17Z

Phabricator:
https://phabricator.wikimedia.org/T288773
https://phabricator.wikimedia.org/T285353
& Some of https://phabricator.wikimedia.org/T287033

Notes

This extends the work done in #4026 to pull the latest unread push notifications from the notifications API, filters out previously seen notifications via an app container cache file and displays the results. If the result is a single new unread notification, it modifies the push content to display it's header value. There is also some extra handling for talk page notification types (adds subtitle, bundles if necessary) based on the Figma mocks. For all other situations, it defaults to "New activity on Wikipedia" text.

After review, please wait for #4044 to be reviewed and merged. Then update this with feature/notifications and merge into that feature branch.

These steps should be tested through the Beta Cluster via the Staging scheme.

Test Steps

Fresh install from this branch and log in from an account that has all checkboxes checked on desktop Wikipedia notifications preferences. Go to Settings > Notifications and enable notifications.
Do something to trigger a notification to yourself from another account on desktop Wikipedia. I tend to test by thanking an edit, undoing an edit, or writing on my talk page with another account.
Wait for notifications to appear on your phone. On Staging scheme / beta cluster desktop it takes about 10 seconds for me lately.
The first notification will likely say "New activity on Wikipedia", since all notifications appear new to the device, and it sees it as condensing multiple notification types.
Trigger another notification (thanks or undo edit).
Now you should see a push with content specific push notification content.
Go to your user talk page and write on it.
Now you should see a push with content specific push notification content, plus a "New message" subtitle.
Get a couple of browser tabs open with editing your talk page. Try to post two updates back to back to trigger a batched talk page notification
Now you should see a push with batched content, "N new messages on {Title}", as well a "New messages" subtitle.
Go to desktop notifications preferences, uncheck "Thanks" from the app column.
Trigger a thanks again to yourself. Confirm no notification appears on your device.

staykids · 2021-10-12T17:45:22Z

@tonisevener Just a head's up – merge conflict in this one (surrounding the prefixed localized string changes in #4044).

tonisevener · 2021-10-13T21:52:41Z

Just a head's up – merge conflict in this one (surrounding the prefixed localized string changes in #4044).

@staykids Thanks, this should be fixed now.

staykids · 2021-10-25T18:06:00Z

@tonisevener Looks like a few more merge conflicts have snuck into this one.

…ache

- audit public keyword uses - move date determination from model controller into model computed property

…nsAPIController

…notifications from RemoteNotificationAPIController directly

…xtension - still needs testing

…rything is localized properly

mcleinman · 2021-11-05T23:35:29Z

Snagging this PR from you, @staykids

mcleinman

This looks great! Just a bunch of small suggested tweaks.

WMF Framework/SharedContainerCache.swift

WMF Framework/Remote Notifications/Model/RemoteNotificationsModelController.swift

Wikipedia/Code/WMFAppViewController.m

WMF Framework/Remote Notifications/RemoteNotificationsAPIController.swift

NotificationServiceExtension/NotificationService.swift

- Clean up and refactor business logic out of NotificationService and move into separate helper class - Add ability to generate mock notifications, write unit tests against new helper class - Add Remote Notifications directory to localization script so that new helper class strings are localized. - Small naming and comment tweaks

- Even with one talk page notification, we still need "New message" subtitle according to designs.

tonisevener · 2021-11-10T22:44:26Z

@mcleinman This is ready for another look. I went through the test steps and had to push one little bug fix commit after the refactor. I didn't receive push notifications through the beta cluster though, so I leaned on the push utility. It could be that backend is stalling or not working at the moment, or I just wasn't patient enough.

NotificationServiceExtension/NotificationService.swift

mcleinman

Just a couple small ones - one I think helps readability, others are nice to have.

WMF Framework/Remote Notifications/NotificationServiceHelper.swift

mcleinman · 2021-11-16T00:48:58Z

Code looks great, will be testing this tomorrow.

mcleinman

Code looks great, and tests great! Thank you!

tonisevener force-pushed the T288662-data-connection branch from 4e23a7d to 285bea6 Compare October 1, 2021 17:46

tonisevener force-pushed the T287310-2 branch from 0ded15e to c342821 Compare October 1, 2021 18:21

tonisevener added the Dependent PR PR is dependent on another PR - merge dependent PR first and update branch before merging label Oct 1, 2021

tonisevener requested review from a team and staykids and removed request for a team October 1, 2021 19:01

tonisevener force-pushed the T288662-data-connection branch 2 times, most recently from 285bea6 to 9924f8d Compare October 5, 2021 20:12

tonisevener force-pushed the T287310-2 branch from ae5df2a to 9434c04 Compare October 5, 2021 20:20

Base automatically changed from T288662-data-connection to feature/notifications October 18, 2021 23:24

tonisevener mentioned this pull request Oct 19, 2021

Implement notifications import operation #4047

Merged

tonisevener removed the Dependent PR PR is dependent on another PR - merge dependent PR first and update branch before merging label Oct 19, 2021

tonisevener added 10 commits October 26, 2021 07:59

Refactor WidgetController cache methods into generic SharedContainerC…

73cffaa

…ache

Prework for consuming network notifications from extension

56ce214

- audit public keyword uses - move date determination from model controller into model computed property

Add ability to fetch unread push notifications from RemoteNotificatio…

990835d

…nsAPIController

Create shared PushNotificationsCache and write with primary app language

d88ed79

Rework NotificationService to use shared cache and fetch unread push …

0f5affb

…notifications from RemoteNotificationAPIController directly

Use cache to determine push notification content to show in service e…

1aae4d7

…xtension - still needs testing

Fix bugs

395938a

Add specific handling for talk page notification types, make sure eve…

c403595

…rything is localized properly

Replace in the news push tap with basic push tap to Notification Center

df22d8f

Add last reminder TODO.

e60b954

tonisevener force-pushed the T287310-2 branch from 80537d5 to e60b954 Compare October 26, 2021 13:33

mcleinman requested review from mcleinman and removed request for staykids November 5, 2021 23:35

tonisevener mentioned this pull request Nov 8, 2021

Notifications - add primary and secondary routing from cells #4064

Merged

tonisevener changed the base branch from feature/notifications to main November 9, 2021 20:58

Merge branch 'main' into T287310-2

5d6ac9f

mcleinman reviewed Nov 10, 2021

View reviewed changes

tonisevener self-assigned this Nov 10, 2021

tonisevener force-pushed the T287310-2 branch from 716bb20 to 08485d7 Compare November 10, 2021 21:57

tonisevener removed their assignment Nov 10, 2021

Fixes after manual testing

fbdad5a

- Even with one talk page notification, we still need "New message" subtitle according to designs.

Merge branch 'main' into T287310-2

7b73b92

mcleinman reviewed Nov 12, 2021

View reviewed changes

NotificationServiceExtension/NotificationService.swift Outdated Show resolved Hide resolved

NotificationServiceExtension/NotificationService.swift Outdated Show resolved Hide resolved

NotificationServiceExtension/NotificationService.swift Outdated Show resolved Hide resolved

mcleinman reviewed Nov 12, 2021

View reviewed changes

WMF Framework/Remote Notifications/NotificationServiceHelper.swift Outdated Show resolved Hide resolved

PR feedback

574d6e0

mcleinman approved these changes Nov 16, 2021

View reviewed changes

Merge branch 'main' into T287310-2

951e952

mcleinman merged commit 33cae1e into main Nov 16, 2021

mcleinman deleted the T287310-2 branch November 16, 2021 22:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Determine new notifications in extension and display #4046

Determine new notifications in extension and display #4046

tonisevener commented Oct 1, 2021 •

edited

staykids commented Oct 12, 2021

tonisevener commented Oct 13, 2021

staykids commented Oct 25, 2021

mcleinman commented Nov 5, 2021

mcleinman left a comment

tonisevener commented Nov 10, 2021

mcleinman left a comment

mcleinman commented Nov 16, 2021

mcleinman left a comment

Determine new notifications in extension and display #4046

Determine new notifications in extension and display #4046

Conversation

tonisevener commented Oct 1, 2021 • edited

Notes

Test Steps

staykids commented Oct 12, 2021

tonisevener commented Oct 13, 2021

staykids commented Oct 25, 2021

mcleinman commented Nov 5, 2021

mcleinman left a comment

Choose a reason for hiding this comment

tonisevener commented Nov 10, 2021

mcleinman left a comment

Choose a reason for hiding this comment

mcleinman commented Nov 16, 2021

mcleinman left a comment

Choose a reason for hiding this comment

tonisevener commented Oct 1, 2021 •

edited