Skip to content

Commit

Permalink
notifs: Show in more places that the org hasn't enabled push notifica…
Browse files Browse the repository at this point in the history
…tions

In particular, the message

> Push notifications are not enabled for {realmName}.

can now appear in these places:

- on the "Notifications" button in the settings screen
- in the dialog shown when you tap a warning icon on the active
  account in the "Pick account" screen.

Before this, both of those places would just say:

> Notifications for this account may not arrive.

which was less definitive than we would like. (If the server hasn't
enabled push notifications, then they definitely won't arrive.)

In this commit, we also start using those same two places to give
other actionable feedback as it applies:

> Notifications are disabled in system settings.

> Notifications require Google Play Services, which is unavailable.

For the "Google Play Services" message, this commit also causes it
to appear on the "Troubleshooting" button in the notification
settings screen. (When the "not enabled for {realmName}" case or the
"disabled in system settings" case applies, the notification
settings screen already has appropriate messaging, since before this
commit, that doesn't involve the "Troubleshooting" button.)

Fixes-partly: zulip#5785
  • Loading branch information
chrisbobbe committed Jan 3, 2024
1 parent fb041a1 commit ac32a7a
Show file tree
Hide file tree
Showing 4 changed files with 82 additions and 19 deletions.
31 changes: 27 additions & 4 deletions src/account/AccountItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,18 @@ import Touchable from '../common/Touchable';
import ZulipTextIntl from '../common/ZulipTextIntl';
import { IconAlertTriangle, IconDone, IconTrash } from '../common/Icons';
import { useGlobalDispatch, useGlobalSelector } from '../react-redux';
import { getIsActiveAccount } from './accountsSelectors';
import { getIsActiveAccount, tryGetActiveAccountState } from './accountsSelectors';
import type { AccountStatus } from './AccountPickScreen';
import { kWarningColor } from '../styles/constants';
import { TranslationContext } from '../boot/TranslationProvider';
import { accountSwitch } from './accountActions';
import { useNavigation } from '../react-navigation';
import {
chooseNotifProblemForShortText,
notifProblemShortText,
} from '../settings/NotifTroubleshootingScreen';
import { getRealmName } from '../directSelectors';
import { getHaveServerData } from '../haveServerDataSelectors';

const styles = createStyleSheet({
wrapper: {
Expand Down Expand Up @@ -73,10 +79,27 @@ export default function AccountItem(props: Props): Node {
const backgroundItemColor = isLoggedIn ? 'hsla(177, 70%, 47%, 0.1)' : 'hsla(0,0%,50%,0.1)';
const textColor = isLoggedIn ? BRAND_COLOR : 'gray';

const activeAccountState = useGlobalSelector(tryGetActiveAccountState);
// The fallback text '(unknown organization name)' is never expected to
// appear in the UI. As of writing, notifProblemShortText doesn't use its
// realmName param except for a NotificationProblem which we only select
// when we have server data for the relevant account. When we have that,
// `realmName` will be the real realm name, not the fallback.
// TODO(#5005) look for server data even when this item's account is not
// the active one.
const realmName =
isActiveAccount && activeAccountState != null && getHaveServerData(activeAccountState)
? getRealmName(activeAccountState)
: '(unknown organization name)';
const singleNotifProblem = chooseNotifProblemForShortText({ report: notificationReport });

const handlePressNotificationWarning = React.useCallback(() => {
if (singleNotifProblem == null) {
return;
}
Alert.alert(
_('Notifications'),
_('Notifications for this account may not arrive.'),
_(notifProblemShortText(singleNotifProblem, realmName)),
[
{ text: _('Cancel'), style: 'cancel' },
{
Expand All @@ -90,7 +113,7 @@ export default function AccountItem(props: Props): Node {
],
{ cancelable: true },
);
}, [email, realm, navigation, dispatch, _]);
}, [email, singleNotifProblem, realm, realmName, navigation, dispatch, _]);

return (
<Touchable style={styles.wrapper} onPress={() => props.onSelect(props.account)}>
Expand All @@ -112,7 +135,7 @@ export default function AccountItem(props: Props): Node {
<ZulipTextIntl style={styles.signedOutText} text="Signed out" numberOfLines={1} />
)}
</View>
{notificationReport.problems.length > 0 && (
{singleNotifProblem != null && (
<Pressable style={styles.icon} hitSlop={12} onPress={handlePressNotificationWarning}>
{({ pressed }) => (
<IconAlertTriangle
Expand Down
29 changes: 29 additions & 0 deletions src/settings/NotifTroubleshootingScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,35 @@ export enum NotificationProblem {
// - Android notification sound file missing (#5484)
}

const notifProblemsHighToLowShortTextPrecedence: $ReadOnlyArray<NotificationProblem> = [
NotificationProblem.SystemSettingsDisabled,
NotificationProblem.GooglePlayServicesNotAvailable,
NotificationProblem.ServerHasNotEnabled,
NotificationProblem.TokenNotAcked,
NotificationProblem.TokenUnknown,
];

invariant(
new Set(notifProblemsHighToLowShortTextPrecedence).size
=== [...NotificationProblem.members()].length,
'notifProblemsHighToLowShortTextPrecedence missing or duplicate members',
);

/**
* Which of a NotificationReport's `problem`s to show short text for if any.
*
* Use this with notifProblemShortText and notifProblemShortReactText,
* where there's room for just one problem's short text, like in the
* NavRow leading to the notification settings screen.
*/
export const chooseNotifProblemForShortText = (args: {|
// eslint-disable-next-line no-use-before-define
report: { +problems: NotificationReport['problems'], ... },
|}): NotificationProblem | null => {
const { report } = args;
return notifProblemsHighToLowShortTextPrecedence.find(p => report.problems.includes(p)) ?? null;
};

/**
* A one-line summary of a NotificationProblem, as LocalizableText.
*
Expand Down
19 changes: 10 additions & 9 deletions src/settings/PerAccountNotificationSettingsGroup.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
useNotificationReportsByIdentityKey,
NotificationProblem,
notifProblemShortReactText,
chooseNotifProblemForShortText,
} from './NotifTroubleshootingScreen';
import { keyOfIdentity } from '../account/accountMisc';
import { ApiError } from '../api/apiErrors';
Expand Down Expand Up @@ -172,15 +173,15 @@ export default function PerAccountNotificationSettingsGroup(props: Props): Node
/>,
<NavRow
key="troubleshooting"
{...(() =>
notificationReport.problems.length > 0 && {
leftElement: {
type: 'icon',
Component: IconAlertTriangle,
color: kWarningColor,
},
subtitle: 'Notifications for this account may not arrive.',
})()}
{...(() => {
const problem = chooseNotifProblemForShortText({ report: notificationReport });
return (
problem != null && {
leftElement: { type: 'icon', Component: IconAlertTriangle, color: kWarningColor },
subtitle: notifProblemShortReactText(problem, realmName),
}
);
})()}
title={troubleshootingPageTitle}
onPress={handleTroubleshootingPress}
/>,
Expand Down
22 changes: 16 additions & 6 deletions src/settings/SettingsScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,14 @@ import { kMinSupportedVersion, kServerSupportDocUrl } from '../common/ServerComp
import { kWarningColor } from '../styles/constants';
import { showErrorAlert } from '../utils/info';
import { TranslationContext } from '../boot/TranslationProvider';
import { useNotificationReportsByIdentityKey } from './NotifTroubleshootingScreen';
import {
useNotificationReportsByIdentityKey,
chooseNotifProblemForShortText,
notifProblemShortReactText,
} from './NotifTroubleshootingScreen';
import { keyOfIdentity } from '../account/accountMisc';
import languages from './languages';
import { getRealmName } from '../directSelectors';

type Props = $ReadOnly<{|
navigation: AppNavigationProp<'settings'>,
Expand All @@ -50,6 +55,7 @@ export default function SettingsScreen(props: Props): Node {
const notificationReportsByIdentityKey = useNotificationReportsByIdentityKey();
const notificationReport = notificationReportsByIdentityKey.get(keyOfIdentity(identity));
invariant(notificationReport, 'SettingsScreen: expected notificationReport');
const realmName = useSelector(getRealmName);

const dispatch = useDispatch();
const _ = React.useContext(TranslationContext);
Expand Down Expand Up @@ -92,11 +98,15 @@ export default function SettingsScreen(props: Props): Node {
<NavRow
leftElement={{ type: 'icon', Component: IconNotifications }}
title="Notifications"
{...(() =>
notificationReport.problems.length > 0 && {
leftElement: { type: 'icon', Component: IconAlertTriangle, color: kWarningColor },
subtitle: 'Notifications for this account may not arrive.',
})()}
{...(() => {
const problem = chooseNotifProblemForShortText({ report: notificationReport });
return (
problem != null && {
leftElement: { type: 'icon', Component: IconAlertTriangle, color: kWarningColor },
subtitle: notifProblemShortReactText(problem, realmName),
}
);
})()}
onPress={() => {
navigation.push('notifications');
}}
Expand Down

0 comments on commit ac32a7a

Please sign in to comment.