notifs: Add snoozable banner for notifs-soon-to-expire

[chrisbobbe]

This is stacked on #5812. I'll post a screenshot soon.

[chrisbobbe]

The snooze period is a week, and the "Learn more" button opens https://zulip.com/help/self-hosted-billing#upgrades-for-legacy-customers , like the warnings in #5812.

[gnprice]

Thanks for building this! The code all LGTM, modulo a couple of small comments below.

@alya, any feedback on the UX?

[gnprice]

Suggested change

	accounts: (base52.accounts.map(a => ({
	...a,
	silenceServerPushSetupWarnings: false,
	})): $ReadOnlyArray<{|
	+email: string,
	+apiKey: string,
	+realm: URL,
	+ackedPushToken: string | null,
	+zulipFeatureLevel: number | null,
	+zulipVersion: ZulipVersion | null,
	+lastDismissedServerPushSetupNotice: Date | null,
	+userId: UserId | null,
	+silenceServerPushSetupWarnings: boolean,
	|}>),
	// $FlowIgnore[prop-missing] same type-lie as in the test, below at end
	accounts: base52.accounts.map(a => ({
	...a,
	silenceServerPushSetupWarnings: false,
	})),

Weird that for this one piece of data, Flow puts the error up here; for the rest, the error is down at the bottom where we pretend that these old states meet the current type, and so we have $FlowIgnore comments there accordingly:

      // $FlowIgnore[incompatible-exact]
      // $FlowIgnore[incompatible-type]
      /* $FlowIgnore[prop-missing]
         this really is a lie -- and kind of central to migration */
      const incomingState: $Rest<GlobalState, { ... }> = before;

But the same discrepancy applies to basically all of these except the one or two at the end. So best to keep succinct how we tell Flow to avert its eyes.

[gnprice]

Can both banners show at once? That seems like it'd be annoying.

Looking at the implementation, I think the answer is that they could only if pushNotificationsEnabledEndTimestamp is non-null while pushNotificationsEnabled is false. That shouldn't happen — the server shouldn't do that — but rather than rely on the server in that respect, I think it'd be good to add a little conditional in pushNotificationsEnabledEndTimestampWarning so that we ignore the end timestamp if things aren't enabled in the first place. Can be a small prep commit.

[chrisbobbe]

Thanks for the review! Revision pushed.

[gnprice]

Thanks! The code all now looks good; ready to merge pending any feedback from @alya on the UI.

[alya]

Works for me! Thinking about this experience, I'm going to add a bit of an intro to https://zulip.com/help/self-hosted-billing#upgrades-for-legacy-customers so that it explains more clearly that we made a change.

@timabbott any additional thoughts here?

[alya]

Help center PR: zulip/zulip#28688

[timabbott]

Let's use something like this for the language:

{realm_name} is scheduled to switch to a plan that does not include mobile push notifications on {date}.

(I'm not sure whether a link words in this context). I think that makes more clear what the exact situation is -- one could imagine reading the "will be disabled" and at first assuming some other weird possibility about the cause (E.g. your organization got a spam complaint or something). Obviously the link will clarify, but I think the initial reaction is important,

[timabbott]

So I wonder if we should make the time window that a dismissal lasts substantially longer for non-administrators than it is for administrators? Like maybe 2 months for normal users? We probably do want it to be finite, since in theory there could be separate situations 6 months apart that cause notifications to be potentially expiring. But I think it's mostly administrators and owners who would benefit from an additional reminder of the impending change before it actually happens, and I can see it being frustrating for people who might be in the middle of their procurement process for all their employees to get the notice a second time.

[chrisbobbe]

Thanks for the reviews! Revision pushed, with:

• That new text: notifs: Add snoozable banner for notifs-soon-to-expire #5814 (comment)
• Admins/owners are notified at T-minus 15 days; others are notified at T-minus 10 days
• Snooze period is 8 days instead of 7

(These last two were discussed on CZO.)

[alya]

Could you post an updated screenshot, please?

[gnprice]

Thanks! The code changes look good. Ready to merge pending an updated screenshot and any remaining UX/product feedback on that.

[chrisbobbe]

cc @alya @gnprice

The link on "plan", where present, is a small touch target, and in the case of the "Settings" and "Notifications" screens, it appears on a surface that has a different behavior on touch. But I checked manually and it is possible to get the touch response on "plan" if you're precise about where your finger goes.

When you tap the link on "plan", it opens https://zulip.com/plans/#self-hosted .

As a reminder, when you tap any of these, it opens https://zulip.com/help/self-hosted-billing#upgrades-for-legacy-customers :

• "Details" on the dialog on the "Pick account" screen
• "Learn more" on the banner on the home screen
• The relevant row on the "Notifications" screen

[chrisbobbe]

Revision pushed.

[chrisbobbe]

Revision pushed.

[gnprice]

Thanks! Looks good; merging.