Skip to content

Commit

Permalink
notifs: Add "silence warnings" setting for server-setup banner
Browse files Browse the repository at this point in the history
Fixes: zulip#5785
  • Loading branch information
chrisbobbe committed Dec 15, 2023
1 parent 1fe2fe6 commit b4151ce
Show file tree
Hide file tree
Showing 15 changed files with 107 additions and 11 deletions.
2 changes: 2 additions & 0 deletions src/__tests__/lib/exampleData.js
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,7 @@ export const makeAccount = (
zulipVersion: zulipVersionInner,
ackedPushToken,
lastDismissedServerPushSetupNotice,
silenceServerPushSetupWarnings: null,
});
};

Expand Down Expand Up @@ -648,6 +649,7 @@ export const plusReduxState: GlobalState & PerAccountState = reduxState({
zulipVersion: recentZulipVersion,
zulipFeatureLevel: recentZulipFeatureLevel,
lastDismissedServerPushSetupNotice: null,
silenceServerPushSetupWarnings: false,
},
],
realm: {
Expand Down
8 changes: 6 additions & 2 deletions src/account/AccountItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,8 @@ type Props = $ReadOnly<{|
|}>;

export default function AccountItem(props: Props): Node {
const { email, realm, isLoggedIn, notificationReport } = props.account;
const { email, realm, isLoggedIn, notificationReport, silenceServerPushSetupWarnings } =
props.account;

const _ = React.useContext(TranslationContext);
const navigation = useNavigation();
Expand Down Expand Up @@ -91,7 +92,10 @@ export default function AccountItem(props: Props): Node {
isActiveAccount && activeAccountState != null && getHaveServerData(activeAccountState)
? getRealmName(activeAccountState)
: '(unknown organization name)';
const singleNotifProblem = chooseNotifProblemForShortText({ report: notificationReport });
const singleNotifProblem = chooseNotifProblemForShortText({
report: notificationReport,
ignoreServerHasNotEnabled: silenceServerPushSetupWarnings ?? false,
});

const handlePressNotificationWarning = React.useCallback(() => {
if (singleNotifProblem == null) {
Expand Down
5 changes: 4 additions & 1 deletion src/account/AccountPickScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ export type AccountStatus = {|
// list of problems that are likely to prevent notifications from working.
// We'll use it to warn on each account item that has problems.
+notificationReport: { +problems: NotificationReport['problems'], ... },

+silenceServerPushSetupWarnings: boolean | null,
|};

/**
Expand All @@ -45,7 +47,7 @@ function useAccountStatuses(): $ReadOnlyArray<AccountStatus> {
const accounts = useGlobalSelector(getAccounts);
const notificationReportsByIdentityKey = useNotificationReportsByIdentityKey();

return accounts.map(({ realm, email, apiKey }) => {
return accounts.map(({ realm, email, apiKey, silenceServerPushSetupWarnings }) => {
const notificationReport = notificationReportsByIdentityKey.get(
keyOfIdentity({ realm, email }),
);
Expand All @@ -56,6 +58,7 @@ function useAccountStatuses(): $ReadOnlyArray<AccountStatus> {
email,
isLoggedIn: apiKey !== '',
notificationReport,
silenceServerPushSetupWarnings,
};
});
}
Expand Down
6 changes: 6 additions & 0 deletions src/account/accountActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
ACCOUNT_REMOVE,
LOGIN_SUCCESS,
DISMISS_SERVER_PUSH_SETUP_NOTICE,
SET_SILENCE_SERVER_PUSH_SETUP_WARNINGS,
} from '../actionConstants';
import { registerAndStartPolling } from '../events/eventActions';
import { resetToMainTabs } from '../nav/navActions';
Expand All @@ -22,6 +23,11 @@ export const dismissServerPushSetupNotice = (): PerAccountAction => ({
date: new Date(),
});

export const setSilenceServerPushSetupWarnings = (value: boolean): PerAccountAction => ({
type: SET_SILENCE_SERVER_PUSH_SETUP_WARNINGS,
value,
});

const accountSwitchPlain = (identity: Identity): AllAccountsAction => ({
type: ACCOUNT_SWITCH,
identity,
Expand Down
9 changes: 9 additions & 0 deletions src/account/accountsReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
LOGOUT,
DISMISS_SERVER_PUSH_SETUP_NOTICE,
ACCOUNT_REMOVE,
SET_SILENCE_SERVER_PUSH_SETUP_WARNINGS,
} from '../actionConstants';
import { EventTypes } from '../api/eventTypes';
import type { AccountsState, Identity, Action, Account } from '../types';
Expand Down Expand Up @@ -49,6 +50,9 @@ export default (state: AccountsState = initialState, action: Action): AccountsSt
lastDismissedServerPushSetupNotice: action.data.realm_push_notifications_enabled
? null
: state[0].lastDismissedServerPushSetupNotice,
silenceServerPushSetupWarnings: action.data.realm_push_notifications_enabled
? null
: state[0].silenceServerPushSetupWarnings ?? false,
});

case ACCOUNT_SWITCH: {
Expand Down Expand Up @@ -77,6 +81,7 @@ export default (state: AccountsState = initialState, action: Action): AccountsSt
zulipVersion: null,
zulipFeatureLevel: null,
lastDismissedServerPushSetupNotice: null,
silenceServerPushSetupWarnings: null,
},
...state,
];
Expand Down Expand Up @@ -126,6 +131,10 @@ export default (state: AccountsState = initialState, action: Action): AccountsSt
return updateActiveAccount(state, { lastDismissedServerPushSetupNotice: action.date });
}

case SET_SILENCE_SERVER_PUSH_SETUP_WARNINGS: {
return updateActiveAccount(state, { silenceServerPushSetupWarnings: action.value });
}

case ACCOUNT_REMOVE: {
const shouldRemove = a =>
keyOfIdentity(identityOfAccount(a)) === keyOfIdentity(action.identity);
Expand Down
3 changes: 3 additions & 0 deletions src/account/accountsSelectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,9 @@ export const getZulipFeatureLevel = (state: PerAccountState): number => {
return zulipFeatureLevel;
};

export const getSilenceServerPushSetupWarnings = (state: PerAccountState): boolean | null =>
getAccount(state).silenceServerPushSetupWarnings;

/**
* The auth object for this account, if logged in; else undefined.
*
Expand Down
2 changes: 2 additions & 0 deletions src/actionConstants.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ export const REFRESH_SERVER_EMOJI_DATA: 'REFRESH_SERVER_EMOJI_DATA' = 'REFRESH_S

export const DISMISS_SERVER_PUSH_SETUP_NOTICE: 'DISMISS_SERVER_PUSH_SETUP_NOTICE' =
'DISMISS_SERVER_PUSH_SETUP_NOTICE';
export const SET_SILENCE_SERVER_PUSH_SETUP_WARNINGS: 'SET_SILENCE_SERVER_PUSH_SETUP_WARNINGS' =
'SET_SILENCE_SERVER_PUSH_SETUP_WARNINGS';

export const INIT_TOPICS: 'INIT_TOPICS' = 'INIT_TOPICS';

Expand Down
8 changes: 8 additions & 0 deletions src/actionTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ import {
DISMISS_SERVER_COMPAT_NOTICE,
REGISTER_PUSH_TOKEN_START,
REGISTER_PUSH_TOKEN_END,
SET_SILENCE_SERVER_PUSH_SETUP_WARNINGS,
} from './actionConstants';

import type { UserMessageFlag } from './api/modelTypes';
Expand Down Expand Up @@ -176,6 +177,11 @@ type DismissServerPushSetupNoticeAction = $ReadOnly<{|
date: Date,
|}>;

type SetSilenceServerPushSetupWarningsAction = $ReadOnly<{|
type: typeof SET_SILENCE_SERVER_PUSH_SETUP_WARNINGS,
value: boolean,
|}>;

/** We learned the device token from the system. See `SessionState`. */
type GotPushTokenAction = $ReadOnly<{|
type: typeof GOT_PUSH_TOKEN,
Expand Down Expand Up @@ -671,6 +677,7 @@ export type PerAccountAction =
// state.session
| DismissServerCompatNoticeAction
| DismissServerPushSetupNoticeAction
| SetSilenceServerPushSetupWarningsAction
| ToggleOutboxSendingAction
;

Expand Down Expand Up @@ -797,6 +804,7 @@ export function isPerAccountApplicableAction(action: Action): boolean {
case CLEAR_TYPING:
case DISMISS_SERVER_COMPAT_NOTICE:
case DISMISS_SERVER_PUSH_SETUP_NOTICE:
case SET_SILENCE_SERVER_PUSH_SETUP_WARNINGS:
case TOGGLE_OUTBOX_SENDING:
(action: PerAccountAction);
(action: PerAccountApplicableAction);
Expand Down
5 changes: 4 additions & 1 deletion src/common/ServerPushSetupBanner.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import subWeeks from 'date-fns/subWeeks';

import ZulipBanner from './ZulipBanner';
import { useSelector, useDispatch } from '../react-redux';
import { getAccount } from '../account/accountsSelectors';
import { getAccount, getSilenceServerPushSetupWarnings } from '../account/accountsSelectors';
import { getRealm } from '../directSelectors';
import { getRealmName } from '../selectors';
import { dismissServerPushSetupNotice } from '../account/accountActions';
Expand Down Expand Up @@ -38,12 +38,15 @@ export default function ServerPushSetupBanner(props: Props): Node {
state => getAccount(state).lastDismissedServerPushSetupNotice,
);
const pushNotificationsEnabled = useSelector(state => getRealm(state).pushNotificationsEnabled);
const silenceServerPushSetupWarnings = useSelector(getSilenceServerPushSetupWarnings);
const realmName = useSelector(getRealmName);

let visible = false;
let text = '';
if (pushNotificationsEnabled) {
// don't show
} else if (silenceServerPushSetupWarnings === true) {
// don't show
} else if (
lastDismissedServerPushSetupNotice !== null
// TODO: Could rerender this component on an interval, to give an
Expand Down
12 changes: 10 additions & 2 deletions src/settings/NotifTroubleshootingScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -160,9 +160,17 @@ invariant(
export const chooseNotifProblemForShortText = (args: {|
// eslint-disable-next-line no-use-before-define
report: { +problems: NotificationReport['problems'], ... },
ignoreServerHasNotEnabled?: boolean,
|}): NotificationProblem | null => {
const { report } = args;
return notifProblemsHighToLowShortTextPrecedence.find(p => report.problems.includes(p)) ?? null;
const { report, ignoreServerHasNotEnabled = false } = args;
const result = notifProblemsHighToLowShortTextPrecedence.find(p => {
if (p === NotificationProblem.ServerHasNotEnabled && ignoreServerHasNotEnabled) {
return false;
}
return report.problems.includes(p);
});

return result ?? null;
};

/**
Expand Down
21 changes: 19 additions & 2 deletions src/settings/PerAccountNotificationSettingsGroup.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,19 @@ import type { Node } from 'react';
import invariant from 'invariant';

import type { AppNavigationMethods } from '../nav/AppNavigator';
import { useGlobalSelector, useSelector } from '../react-redux';
import { useDispatch, useGlobalSelector, useSelector } from '../react-redux';
import { getAuth, getSettings } from '../selectors';
import SwitchRow from '../common/SwitchRow';
import * as api from '../api';
import NavRow from '../common/NavRow';
import TextRow from '../common/TextRow';
import { IconAlertTriangle } from '../common/Icons';
import { kWarningColor } from '../styles/constants';
import { getIdentity, getZulipFeatureLevel } from '../account/accountsSelectors';
import {
getIdentity,
getSilenceServerPushSetupWarnings,
getZulipFeatureLevel,
} from '../account/accountsSelectors';
import { getGlobalSession, getGlobalSettings, getRealm, getRealmName } from '../directSelectors';
import ZulipText from '../common/ZulipText';
import RowGroup from '../common/RowGroup';
Expand All @@ -29,6 +33,7 @@ import { ApiError } from '../api/apiErrors';
import { showErrorAlert } from '../utils/info';
import * as logging from '../utils/logging';
import { TranslationContext } from '../boot/TranslationProvider';
import { setSilenceServerPushSetupWarnings } from '../account/accountActions';

type Props = $ReadOnly<{|
navigation: AppNavigationMethods,
Expand All @@ -40,6 +45,7 @@ type Props = $ReadOnly<{|
export default function PerAccountNotificationSettingsGroup(props: Props): Node {
const { navigation } = props;

const dispatch = useDispatch();
const _ = React.useContext(TranslationContext);

const auth = useSelector(getAuth);
Expand All @@ -53,6 +59,7 @@ export default function PerAccountNotificationSettingsGroup(props: Props): Node
const realmName = useSelector(getRealmName);
const zulipFeatureLevel = useSelector(getZulipFeatureLevel);
const pushNotificationsEnabled = useSelector(state => getRealm(state).pushNotificationsEnabled);
const silenceServerPushSetupWarnings = useSelector(getSilenceServerPushSetupWarnings);
const offlineNotification = useSelector(state => getSettings(state).offlineNotification);
const onlineNotification = useSelector(state => getSettings(state).onlineNotification);
const streamNotification = useSelector(state => getSettings(state).streamNotification);
Expand All @@ -61,6 +68,10 @@ export default function PerAccountNotificationSettingsGroup(props: Props): Node

const pushToken = useGlobalSelector(state => getGlobalSession(state).pushToken);

const handleSilenceWarningsChange = React.useCallback(() => {
dispatch(setSilenceServerPushSetupWarnings(!silenceServerPushSetupWarnings));
}, [dispatch, silenceServerPushSetupWarnings]);

// Helper variable so that we can refer to the button correctly in
// other UI text.
const troubleshootingPageTitle = 'Troubleshooting';
Expand Down Expand Up @@ -207,6 +218,12 @@ export default function PerAccountNotificationSettingsGroup(props: Props): Node
);
}}
/>,
<SwitchRow
key="silence-warnings"
label="Silence warnings about disabled mobile push notifications"
value={silenceServerPushSetupWarnings ?? false}
onValueChange={handleSilenceWarningsChange}
/>,
];

return (
Expand Down
8 changes: 5 additions & 3 deletions src/storage/__tests__/migrations-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,8 @@ describe('migrations', () => {
// What `base` becomes after all migrations.
const endBase = {
...base52,
migrations: { version: 60 },
migrations: { version: 61 },
accounts: base52.accounts.map(a => ({ ...a, silenceServerPushSetupWarnings: null })),
};

for (const [desc, before, after] of [
Expand Down Expand Up @@ -282,7 +283,7 @@ describe('migrations', () => {
[
'check 57 with an `undefined` in state.accounts',
{ ...base52, migrations: { version: 56 }, accounts: [...base37.accounts, undefined] },
{ ...endBase, accounts: [...base37.accounts] },
{ ...endBase, accounts: [...endBase.accounts] },
],
[
'check 58 with a malformed Account in state.accounts',
Expand All @@ -299,8 +300,9 @@ describe('migrations', () => {
...base37.accounts,
],
},
{ ...endBase, accounts: [...base37.accounts] },
{ ...endBase, accounts: [...endBase.accounts] },
],
// 61 covered by whole
]) {
test(desc, async () => {
// $FlowIgnore[incompatible-exact]
Expand Down
6 changes: 6 additions & 0 deletions src/storage/migrations.js
Original file line number Diff line number Diff line change
Expand Up @@ -517,6 +517,12 @@ const migrationsInner: {| [string]: (LessPartialState) => LessPartialState |} =
// Discard invalid enum values from `state.mute`.
'60': dropCache,

// Add silenceServerPushSetupWarnings to accounts.
'61': state => ({
...state,
accounts: state.accounts.map(a => ({ ...a, silenceServerPushSetupWarnings: null })),
}),

// TIP: When adding a migration, consider just using `dropCache`.
// (See its jsdoc for guidance on when that's the right answer.)
};
Expand Down
22 changes: 22 additions & 0 deletions src/types.js
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,28 @@ export type Account = $ReadOnly<{|
* apply.
*/
lastDismissedServerPushSetupNotice: Date | null,

/**
* The setting to silence prominent warnings about disabled notifications.
*
* ("Disabled" meaning in particular that the realm hasn't enabled
* notifications; i.e., RealmState.pushNotificationsEnabled is false.)
*
* Users will set this if they want something more permanent than the
* ServerPushSetupBanner's "Dismiss" button. That button only snoozes the
* banner (for two weeks, as of writing), but this setting makes the
* banner never appear. (The banner's information will still be available
* on the "Notifications" screen.)
*
* Will be `null` when notifications are enabled for the realm, or when
* we've never had server data for this account. Otherwise false, unless
* the user has switched it to true, which they can do until the realm
* enables notifications. (If true when the realm enables notifications,
* we intentionally forget the `true` by setting this to `null`. That's to
* make sure it's false if notifications are disabled later, because that
* disabling might be a surprise that the user wants to know about.)
*/
silenceServerPushSetupWarnings: boolean | null,
|}>;

/**
Expand Down
1 change: 1 addition & 0 deletions static/translations/messages_en.json
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,7 @@
"Recipients": "Recipients",
"Delete message": "Delete message",
"System settings for Zulip": "System settings for Zulip",
"Silence warnings about disabled mobile push notifications": "Silence warnings about disabled mobile push notifications",
"Troubleshooting": "Troubleshooting",
"Send a test notification": "Send a test notification",
"Failed to send test notification": "Failed to send test notification",
Expand Down

0 comments on commit b4151ce

Please sign in to comment.