notifs: Warn about notifications soon to be disabled (not yet with banner) #5812
Conversation
chrisbobbe
force-pushed
the
pr-warn-notifs-disabled-soon
branch
2 times, most recently
from
72f4659 to
0f421ee
Compare
src/account/AccountItem.js
Outdated
| return; | ||
| } | ||
|
|
||
| if (expiryWarning != null) { |
There was a problem hiding this comment.
Can there be both an expiry warning and a single-notif-problem? In that case we'd probably want to prioritize the active problem, right?
src/api/registerForEvents.js
Outdated
| ...rawInitialData, | ||
|
|
||
| realm_push_notifications_enabled_end_timestamp: 1706037926, |
There was a problem hiding this comment.
This looks like it overrides the value from the server with this hard-coded value. Was this perhaps intended as temporary for local testing?
There was a problem hiding this comment.
Eep! Thanks for catching this; it was for local testing, yes.
| // See https://chat.zulip.org/#narrow/stream/107-kandra/topic/notification.20plan.20expiration.20warnings/near/1719677 | ||
| export const kPushNotificationsEnabledEndDoc: URL = new URL( | ||
| 'https://zulip.com/help/self-hosted-billing#upgrades-for-legacy-customers', |
There was a problem hiding this comment.
This link is to a private stream. Generally we don't put such a reference into our codebase.
In this case, I think no reference is needed. The information I take from following the link is basically that this is the link Alya asks to put here. That's the kind of information that can be relevant at PR discussion time, or perhaps in a commit message, but isn't needed in the tree itself. (And we can say it directly, rather than via a link.)
There was a problem hiding this comment.
Oops, indeed; thanks for the catch.
src/settings/SettingsScreen.js
Outdated
| let subtitle = undefined; | ||
| if (expiryWarning != null) { | ||
| subtitle = expiryWarning.reactText; | ||
| } else if (problem != null) { | ||
| subtitle = notifProblemShortReactText(problem, realmName); | ||
| } |
There was a problem hiding this comment.
Similar question about prioritizing the two kinds of warnings.
chrisbobbe
force-pushed
the
pr-warn-notifs-disabled-soon
branch
from
0f421ee to
8bc5235
Compare
|
Thanks for the review! Revision pushed. |
chrisbobbe
force-pushed
the
pr-warn-notifs-disabled-soon
branch
from
8bc5235 to
e6731b8
Compare
And drop the reference to withGetText, which we shouldn't need in a Hooks-based world. Indeed that HOC isn't even used anymore; we'll remove it next.
It looks like our last use of this was removed in 8a0e486.
We'll also start using this in useNotificationReportsByIdentityKey, to check for a new NotificationProblem that will apply when the server is about to stop enabling push notifications.
…nner) Soon, we'll add a snoozable banner on the home screen. But for now: - Warning icon / subtitle text on the "Notifications" row on the settings screen - Warning row in the notification-settings screen - Warning icon on the "Pick account" screen
|
Thanks for the revision! Looks good; merging. |
gnprice
force-pushed
the
pr-warn-notifs-disabled-soon
branch
from
e6731b8 to
b30969b
Compare
Soon, we'll add a snoozable banner. But for now:
(I reused some React Hooks testing infrastructure from an old draft branch I had lying around.)