notifs: Live-update state of whether realm has enabled notifications #5807
Conversation
src/realm/realmReducer.js
Outdated
| if (data.push_notifications_enabled !== undefined) { | ||
| result.pushNotificationsEnabled = data.push_notifications_enabled; |
There was a problem hiding this comment.
nit: let's put these in the same order as the properties appear in the REGISTER_COMPLETE case above — that helps make it easy to compare the two and spot any gaps or discrepancies
There was a problem hiding this comment.
Doing that review now:
- There's what at first glance looks like a gap that
state.webPublicStreamsEnableddoesn't get updated. But the story is that that's a server setting and there's no event to update it. - That's it! Otherwise everything lines up.
I'll push a quick commit on top of the PR branch that makes explicit what's happening there. Then it should probably go before the main commit here.
src/account/accountsReducer.js
Outdated
| } | ||
| case EventTypes.realm: { |
There was a problem hiding this comment.
nit:
| } | |
| case EventTypes.realm: { | |
| } | |
| case EventTypes.realm: { |
and also between the end of this case and the default case
(Arguably there should have been a blank line already separating the existing case from the default case. But perhaps it didn't seem as needed when there was just one non-default case.)
a47d5ff to
c46625f
Compare
|
Thanks for the review! Revision pushed, and I plan to merge this once #5806 is in, as requested. |
Making sure that when the realm goes from disabling to enabling notifications, we clear out the state of whether the user has dismissed the banner on the home screen -- just like we do when the /register response informs us of this change. That's so that if notifications become disabled again, a new banner will be shown. Fixes: zulip#5805
c46625f to
cbdf900
Compare
|
Done. |
This PR is stacked on top of my current revision for #5806.
Fixes: #5805