MM-50354 - do not show push disabled notification once acknowledged #7495

pvev · 2023-08-10T10:49:15Z

Summary

This PR makes sure to show only once the notification Alert about push notifications being disabled in the server.

Ticket Link

https://mattermost.atlassian.net/browse/MM-50354

Checklist

Added or updated unit tests (required for all new features)
Has UI changes
Includes text changes and localization file updates
Have tested against the 5 core themes to ensure consistency between them.

Device Information

This PR was tested on:

Screenshots

Before:

Screen.Recording.2023-08-09.at.23.21.22.mov

Screen.Recording.2023-08-09.at.23.24.33.mov

After:

Screen.Recording.2023-08-10.at.11.59.35.mov

Screen.Recording.2023-08-09.at.23.06.42.mov

Screen.Recording.2023-08-09.at.23.16.18.mov

Release Note

NONE

app/utils/helpers.ts

pvev · 2023-08-16T06:25:37Z

@larkox @enahum friendly reminder on this one :)

app/screens/home/channel_list/categories_list/header/header.test.tsx

app/managers/session_manager.ts

app/queries/app/global.ts

larkox · 2023-08-16T08:49:24Z

app/utils/push_proxy.ts

@@ -23,7 +37,14 @@ export function canReceiveNotifications(serverUrl: string, verification: string,
    }
 }

-export function alertPushProxyError(intl: IntlShape) {
+const handleAlertResponse = async (buttonIndex: number, serverUrl: string) => {
+    if (buttonIndex === 0) {


There is only one button, and always sets the ack, so... how would it show the warnings in the rest of the product? Shouldn't be two buttons like "OK" and "Dont remember me anymore"? And maybe that second button only available when checking the warning after login? 0/5

the way I approached is like, for users that are already logged in the warning will still show once logged in and they will be able to discard it once clicked "ok". For those who log in for the first time the warning will show just once and will reduce the annoying part of showing it in different places and several times.

@esethna Thoughts about this?

@esethna so, there are basically three places where we inform user that the push can not be received due to configuration: initial alert window on loading the app, channels header alert icon, servers list alert icon.

Current implementation works like this:

first app loading shows users the alert window notifying that due to configuration the push alerts can not be received. Users clicks ok, log in and then the header bar and the server show the icon which if clicked will show the Alert again, and again, etc.

My approach with the changes in this PR is:

For existing users that are already logged in in a server which has the push disabled, the alert icon in the header and server list will be shown, once they click it and the alert window is shown and click 'ok' acknowledging the warning, these alert icons won't show again.

For new users the initial alert window will be shown, but once discarded (which is forcefully discarded since there is only one button) the other two places (channel header and server list) won't ever be shown since the alert was already acknowledged.

So, what @larkox is proposing is that this Alert Window should have two buttons, "Ok" meaning like just hiding the window and the "Don't remember again" which will hide the alert icons from the channels header and server lists and won't show the Alert again. We would like to read your thoughts on this. Thanks!

@johndavidlugtu looping you in here for this conversation given it's in the realm of potentially missing notifications due to server configuration (ie push notifications are disabled) and this PR makes some changes to how we are informing end users about that.

ok, understood, will proceed with that change then and update this PR. thanks @johndavidlugtu 👍

hey @esethna @johndavidlugtu the change is now ready and working as expected:

Screen.Recording.2023-10-11.at.11.57.58.mov

hey @johndavidlugtu @esethna friendly reminder on this one :)

Thanks for the ping @pvev

Can we update the copy to say "Due to the configuration of this server..."

Otherwise, lgtm!

done @johndavidlugtu

app/managers/session_manager.ts

app/utils/helpers.ts

…remove

app/utils/helpers.ts

app/utils/push_proxy.ts

app/screens/home/channel_list/categories_list/header/index.ts

app/screens/home/channel_list/servers/servers_list/server_item/index.ts

enahum · 2023-08-21T15:59:01Z

app/managers/session_manager.ts

@@ -121,6 +121,7 @@ class SessionManager {
        WebsocketManager.invalidateClient(serverUrl);

        if (removeServer) {
+            await removePushDisabledInServerAcknowledged(createKeyFromServerUrl(serverUrl));


this is great for when the server gets removed.. but once a server actually starts sending notifications correctly, we should also remove it. I don't see that logic anywhere in the code.

The canReceiveNotifications function is the one that receives the config value and depending on it shows or hides the Alert. It is verified in every place where the notification is shown:

mattermost-mobile/app/screens/home/channel_list/servers/servers_list/server_item/server_item.tsx

Lines 263 to 264 in 7244f95

canReceiveNotifications(server.url, result.canReceiveNotifications as string, intl);

loginToServer(theme, server.url, displayName, data.config!, data.license!);

mattermost-mobile/app/screens/server/index.tsx

Line 310 in 7244f95

canReceiveNotifications(serverUrl, result.canReceiveNotifications as string, intl);

Is this what you are referring to?

app/utils/push_proxy.ts

app/managers/session_manager.ts

larkox

Approving, but my concern about the first notification not having a "remind me later" option is still there. If you could verify with product before merging I would appreciate it.

pvev · 2023-08-23T08:46:20Z

Approving, but my concern about the first notification not having a "remind me later" option is still there. If you could verify with product before merging I would appreciate it.

sure @larkox . Thanks!

…-if-disabled

johndavidlugtu

Good, thanks @pvev

MM-50354 - do not show push disabled notification once acknowledged

0da1b5c

mm-cloud-bot added the release-note-none label Aug 10, 2023

pvev requested review from larkox and enahum August 10, 2023 10:49

pvev added the 2: Dev Review Requires review by a core commiter label Aug 10, 2023

enahum requested changes Aug 10, 2023

View reviewed changes

app/utils/helpers.ts Outdated Show resolved Hide resolved

enhance regular expression to support optional subpath

7d3115c

pvev requested a review from enahum August 10, 2023 11:54

larkox requested changes Aug 16, 2023

View reviewed changes

enahum reviewed Aug 16, 2023

View reviewed changes

app/managers/session_manager.ts Outdated Show resolved Hide resolved

app/utils/helpers.ts Outdated Show resolved Hide resolved

add unit test; refactor function; not remove on logout but on server …

7244f95

…remove

pvev requested review from larkox and enahum August 21, 2023 15:45

enahum requested changes Aug 21, 2023

View reviewed changes

implement pr feedback; reuse existing keyfromurl function; fix imports

d4cd0cb

enahum approved these changes Aug 22, 2023

View reviewed changes

app/managers/session_manager.ts Outdated Show resolved Hide resolved

larkox approved these changes Aug 23, 2023

View reviewed changes

pvev requested a review from esethna August 31, 2023 08:16

pvev added 2: PM Review Requires review by a product manager and removed 2: Dev Review Requires review by a core commiter labels Aug 31, 2023

esethna requested review from johndavidlugtu and removed request for esethna August 31, 2023 15:36

pvev added 2 commits October 9, 2023 16:28

Merge branch 'main' into MM-50354-not-show-push-notifications-warning…

499114f

…-if-disabled

do not remove inside app alerts

41ca619

pvev force-pushed the MM-50354-not-show-push-notifications-warning-if-disabled branch from c4b6937 to 41ca619 Compare October 10, 2023 15:13

pvev added 2 commits October 18, 2023 12:12

Update alert text

89fc2e8

capitalize back the alert text

6dcc999

johndavidlugtu approved these changes Oct 25, 2023

View reviewed changes

pvev merged commit 196372e into main Oct 25, 2023
1 check passed

pvev deleted the MM-50354-not-show-push-notifications-warning-if-disabled branch October 25, 2023 09:50

amyblais added this to the v2.11.0 milestone Oct 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MM-50354 - do not show push disabled notification once acknowledged #7495

MM-50354 - do not show push disabled notification once acknowledged #7495

pvev commented Aug 10, 2023 •

edited

Loading

pvev commented Aug 16, 2023

larkox Aug 16, 2023

pvev Aug 18, 2023

larkox Aug 18, 2023

pvev Aug 23, 2023

esethna Aug 31, 2023

pvev Oct 6, 2023

pvev Oct 11, 2023

pvev Oct 17, 2023

johndavidlugtu Oct 17, 2023

pvev Oct 18, 2023 •

edited

Loading

enahum Aug 21, 2023

pvev Aug 22, 2023 •

edited

Loading

larkox left a comment

pvev commented Aug 23, 2023

johndavidlugtu left a comment

	canReceiveNotifications(server.url, result.canReceiveNotifications as string, intl);
	loginToServer(theme, server.url, displayName, data.config!, data.license!);

MM-50354 - do not show push disabled notification once acknowledged #7495

MM-50354 - do not show push disabled notification once acknowledged #7495

Conversation

pvev commented Aug 10, 2023 • edited Loading

Summary

Ticket Link

Checklist

Device Information

Screenshots

After:

Release Note

pvev commented Aug 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvev Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvev Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

larkox left a comment

Choose a reason for hiding this comment

pvev commented Aug 23, 2023

johndavidlugtu left a comment

Choose a reason for hiding this comment

pvev commented Aug 10, 2023 •

edited

Loading

pvev Oct 18, 2023 •

edited

Loading

pvev Aug 22, 2023 •

edited

Loading