Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

notifs: Warn about notifications soon to be disabled (not yet with banner) #5812

Merged
merged 5 commits into from
Jan 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@
"@octokit/core": "^3.4.0",
"@rollup/plugin-babel": "^5.2.0",
"@rollup/plugin-node-resolve": "^13.0.4",
"@testing-library/react-hooks": "^8.0.1",
"@types/react-native": "~0.67.6",
"@vusion/webfonts-generator": "^0.8.0",
"ast-types": "^0.16.1",
Expand Down
1 change: 1 addition & 0 deletions src/__tests__/lib/exampleData.js
Original file line number Diff line number Diff line change
Expand Up @@ -817,6 +817,7 @@ export const action = Object.freeze({
realm_presence_disabled: true,
realm_private_message_policy: 3,
realm_push_notifications_enabled: true,
realm_push_notifications_enabled_end_timestamp: 1704926069,
realm_send_welcome_emails: true,
realm_signup_notifications_stream_id: 3,
realm_upload_quota_mib: 10,
Expand Down
45 changes: 44 additions & 1 deletion src/__tests__/reactUtils-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@ import React from 'react';
import type { ComponentType } from 'react';
// $FlowFixMe[untyped-import]
import { create, act } from 'react-test-renderer';
// $FlowFixMe[untyped-import]
import * as ReactHooksTesting from '@testing-library/react-hooks';

import { fakeSleep } from './lib/fakeTimers';
import { useHasStayedTrueForMs } from '../reactUtils';
import { useDateRefreshedAtInterval, useHasStayedTrueForMs } from '../reactUtils';

describe('useHasNotChangedForMs', () => {
// This gets indirect coverage because useHasStayedTrueForMs calls it, and
Expand Down Expand Up @@ -33,6 +35,11 @@ describe('useHasStayedTrueForMs', () => {
* repeatedly
* - boring details like how the mock component is implemented
*/
// We wrote these tests a long time before our first experiment with
// @testing-library/react-hooks (for useDateRefreshedAtInterval), in which
// we let the library take care of defining and rendering a component. The
// setup for these older tests is much more verbose.
//
// I'm not totally clear on everything `act` does, but react-test-renderer
// seems to recommend it strongly enough that we actually get errors if we
// don't use it. Following links --
Expand Down Expand Up @@ -249,3 +256,39 @@ describe('useHasStayedTrueForMs', () => {
});
}
});

test('useDateRefreshedAtInterval', async () => {
const interval = 60_000;

function sleep(ms: number): Promise<void> {
return ReactHooksTesting.act(() => fakeSleep(ms));
}

// https://react-hooks-testing-library.com/reference/api#renderhook
const { result } = ReactHooksTesting.renderHook(() => useDateRefreshedAtInterval(interval));

let value = result.current;
expect(result.error).toBeUndefined();

await sleep(interval / 10);
expect(result.error).toBeUndefined();
expect(result.current).toBe(value);

await sleep(interval / 10);
expect(result.error).toBeUndefined();
expect(result.current).toBe(value);

await sleep(interval);
expect(result.error).toBeUndefined();
expect(result.current).not.toBe(value);
expect((result.current - value) * 1000).toBeGreaterThanOrEqual(interval);
value = result.current;

await sleep(interval / 10);
expect(result.error).toBeUndefined();
expect(result.current).toBe(value);

await sleep(interval / 10);
expect(result.error).toBeUndefined();
expect(result.current).toBe(value);
});
92 changes: 69 additions & 23 deletions src/account/AccountItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,15 @@ import { accountSwitch } from './accountActions';
import { useNavigation } from '../react-navigation';
import {
chooseNotifProblemForShortText,
kPushNotificationsEnabledEndDoc,
notifProblemShortText,
pushNotificationsEnabledEndTimestampWarning,
} from '../settings/NotifTroubleshootingScreen';
import { getRealmName } from '../directSelectors';
import { getGlobalSettings, getRealmName } from '../directSelectors';
import { getHaveServerData } from '../haveServerDataSelectors';
import { useDateRefreshedAtInterval } from '../reactUtils';
import { openLinkWithUserPreference } from '../utils/openLink';
import * as logging from '../utils/logging';

const styles = createStyleSheet({
wrapper: {
Expand Down Expand Up @@ -69,6 +74,8 @@ export default function AccountItem(props: Props): Node {
const navigation = useNavigation();
const dispatch = useGlobalDispatch();

const globalSettings = useGlobalSelector(getGlobalSettings);

const isActiveAccount = useGlobalSelector(state => getIsActiveAccount(state, { email, realm }));

// Don't show the "remove account" button (the "trash" icon) for the
Expand All @@ -80,6 +87,8 @@ 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 dateNow = useDateRefreshedAtInterval(60_000);

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
Expand All @@ -88,36 +97,73 @@ export default function AccountItem(props: Props): Node {
// `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)';
let realmName = '(unknown organization name)';
let expiryWarning = null;
if (isActiveAccount && activeAccountState != null && getHaveServerData(activeAccountState)) {
realmName = getRealmName(activeAccountState);
expiryWarning = silenceServerPushSetupWarnings
? null
: pushNotificationsEnabledEndTimestampWarning(activeAccountState, dateNow);
}
const singleNotifProblem = chooseNotifProblemForShortText({
report: notificationReport,
ignoreServerHasNotEnabled: silenceServerPushSetupWarnings,
});

const handlePressNotificationWarning = React.useCallback(() => {
if (singleNotifProblem == null) {
if (expiryWarning == null && singleNotifProblem == null) {
logging.warn('AccountItem: Notification warning pressed with nothing to show');
return;
}

if (singleNotifProblem != null) {
Alert.alert(
_('Notifications'),
_(notifProblemShortText(singleNotifProblem, realmName)),
[
{ text: _('Cancel'), style: 'cancel' },
{
text: _('Details'),
onPress: () => {
dispatch(accountSwitch({ realm, email }));
navigation.push('notifications');
},
style: 'default',
},
],
{ cancelable: true },
);
return;
}
Alert.alert(
_('Notifications'),
_(notifProblemShortText(singleNotifProblem, realmName)),
[
{ text: _('Cancel'), style: 'cancel' },
{
text: _('Details'),
onPress: () => {
dispatch(accountSwitch({ realm, email }));
navigation.push('notifications');

if (expiryWarning != null) {
Alert.alert(
_('Notifications'),
_(expiryWarning.text),
[
{ text: _('Cancel'), style: 'cancel' },
{
text: _('Details'),
onPress: () => {
openLinkWithUserPreference(kPushNotificationsEnabledEndDoc, globalSettings);
},
style: 'default',
},
style: 'default',
},
],
{ cancelable: true },
);
}, [email, singleNotifProblem, realm, realmName, navigation, dispatch, _]);
],
{ cancelable: true },
);
}
}, [
email,
singleNotifProblem,
expiryWarning,
realm,
realmName,
globalSettings,
navigation,
dispatch,
_,
]);

return (
<Touchable style={styles.wrapper} onPress={() => props.onSelect(props.account)}>
Expand All @@ -139,7 +185,7 @@ export default function AccountItem(props: Props): Node {
<ZulipTextIntl style={styles.signedOutText} text="Signed out" numberOfLines={1} />
)}
</View>
{singleNotifProblem != null && (
{(singleNotifProblem != null || expiryWarning != null) && (
<Pressable style={styles.icon} hitSlop={12} onPress={handlePressNotificationWarning}>
{({ pressed }) => (
<IconAlertTriangle
Expand Down
21 changes: 0 additions & 21 deletions src/boot/TranslationProvider.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,27 +12,6 @@ import messages from '../i18n/messages';
// $FlowFixMe[incompatible-type] could put a well-typed mock value here, to help write tests
export const TranslationContext: React.Context<GetText> = React.createContext(undefined);

/**
* Provide `_` to the wrapped component, passing other props through.
*
* This can be useful when the component is already using its `context`
* property for a different context provider. When that isn't the case,
* simply saying `context: TranslationContext` may be more convenient.
* Or in a function component, `const _ = useContext(TranslationContext);`.
*/
export function withGetText<P: { +_: GetText, ... }, C: React.ComponentType<P>>(
WrappedComponent: C,
): React.ComponentType<$ReadOnly<$Exact<$Diff<React.ElementConfig<C>, {| _: GetText |}>>>> {
// eslint-disable-next-line func-names
return function (props: $Exact<$Diff<React.ElementConfig<C>, {| _: GetText |}>>): React.Node {
return (
<TranslationContext.Consumer>
{_ => <WrappedComponent _={_} {...props} />}
</TranslationContext.Consumer>
);
};
}

const makeGetText = (intl: IntlShape): GetText => {
const _ = (message, values_) => {
const text = typeof message === 'object' ? message.text : message;
Expand Down
7 changes: 4 additions & 3 deletions src/common/ServerPushSetupBanner.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
notifProblemShortReactText,
} from '../settings/NotifTroubleshootingScreen';
import type { AppNavigationMethods } from '../nav/AppNavigator';
import { useDateRefreshedAtInterval } from '../reactUtils';

type Props = $ReadOnly<{|
navigation: AppNavigationMethods,
Expand All @@ -41,6 +42,8 @@ export default function ServerPushSetupBanner(props: Props): Node {
const silenceServerPushSetupWarnings = useSelector(getSilenceServerPushSetupWarnings);
const realmName = useSelector(getRealmName);

const dateNow = useDateRefreshedAtInterval(60_000);

let visible = false;
let text = '';
if (pushNotificationsEnabled) {
Expand All @@ -49,9 +52,7 @@ export default function ServerPushSetupBanner(props: Props): Node {
// don't show
} else if (
lastDismissedServerPushSetupNotice !== null
// TODO: Could rerender this component on an interval, to give an
// upper bound on how outdated this `new Date()` can be.
&& lastDismissedServerPushSetupNotice >= subWeeks(new Date(), 2)
&& lastDismissedServerPushSetupNotice >= subWeeks(dateNow, 2)
) {
// don't show
} else {
Expand Down
26 changes: 26 additions & 0 deletions src/reactUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -149,3 +149,29 @@ export const useHasStayedTrueForMs = (value: boolean, duration: number): boolean
// https://reactjs.org/docs/hooks-effect.html#tip-optimizing-performance-by-skipping-effects
export const useConditionalEffect = (cb: () => void | (() => void), value: boolean): void =>
React.useEffect(() => (value ? cb() : undefined), [value, cb]);

/**
* A Date, as React state that refreshes regularly at an interval.
*
* Use this in a React component that relies on the current date, to make it
* periodically rerender with a refreshed date value.
*
* Don't change the value of refreshIntervalMs.
*
* This uses `setTimeout`, which is subject to slightly late timeouts:
* https://developer.mozilla.org/en-US/docs/Web/API/setTimeout#reasons_for_delays_longer_than_specified
* so don't expect millisecond precision.
*/
export const useDateRefreshedAtInterval = (refreshIntervalMs: number): Date => {
useDebugAssertConstant(refreshIntervalMs);

const [date, setDate] = React.useState(new Date());
useConditionalEffect(
React.useCallback(() => {
setDate(new Date());
}, []),
useHasNotChangedForMs(date, refreshIntervalMs),
);

return date;
};
12 changes: 12 additions & 0 deletions src/realm/__tests__/realmReducer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ describe('realmReducer', () => {
messageContentDeleteLimitSeconds: action.data.realm_message_content_delete_limit_seconds,
messageContentEditLimitSeconds: action.data.realm_message_content_edit_limit_seconds,
pushNotificationsEnabled: action.data.realm_push_notifications_enabled,
pushNotificationsEnabledEndTimestamp:
action.data.realm_push_notifications_enabled_end_timestamp,
webPublicStreamsEnabled: action.data.server_web_public_streams_enabled,
createPublicStreamPolicy: action.data.realm_create_public_stream_policy,
createPrivateStreamPolicy: action.data.realm_create_private_stream_policy,
Expand Down Expand Up @@ -408,6 +410,16 @@ describe('realmReducer', () => {
check(false, false);
});

describe('pushNotificationsEnabledEndTimestamp / push_notifications_enabled_end_timestamp', () => {
const check = mkCheck(
'pushNotificationsEnabledEndTimestamp',
'push_notifications_enabled_end_timestamp',
);
check(123, null);
check(null, 123);
check(123, 234);
});

describe('create{Private,Public}StreamPolicy / create_stream_policy', () => {
// TODO(server-5.0): Stop expecting create_stream_policy; remove.

Expand Down
7 changes: 7 additions & 0 deletions src/realm/realmReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ const initialState = {
messageContentDeleteLimitSeconds: null,
messageContentEditLimitSeconds: 1,
pushNotificationsEnabled: true,
pushNotificationsEnabledEndTimestamp: null,
createPublicStreamPolicy: CreatePublicOrPrivateStreamPolicy.MemberOrAbove,
createPrivateStreamPolicy: CreatePublicOrPrivateStreamPolicy.MemberOrAbove,
webPublicStreamsEnabled: false,
Expand Down Expand Up @@ -146,6 +147,8 @@ export default (
messageContentDeleteLimitSeconds: action.data.realm_message_content_delete_limit_seconds,
messageContentEditLimitSeconds: action.data.realm_message_content_edit_limit_seconds,
pushNotificationsEnabled: action.data.realm_push_notifications_enabled,
pushNotificationsEnabledEndTimestamp:
action.data.realm_push_notifications_enabled_end_timestamp ?? null,
createPublicStreamPolicy:
action.data.realm_create_public_stream_policy
?? action.data.realm_create_stream_policy
Expand Down Expand Up @@ -255,6 +258,10 @@ export default (
if (data.push_notifications_enabled !== undefined) {
result.pushNotificationsEnabled = data.push_notifications_enabled;
}
if (data.push_notifications_enabled_end_timestamp !== undefined) {
result.pushNotificationsEnabledEndTimestamp =
data.push_notifications_enabled_end_timestamp;
}
if (data.create_stream_policy !== undefined) {
// TODO(server-5.0): Stop expecting create_stream_policy; simplify.
result.createPublicStreamPolicy = data.create_stream_policy;
Expand Down
1 change: 1 addition & 0 deletions src/reduxTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,7 @@ export type RealmState = {|
+messageContentDeleteLimitSeconds: number | null,
+messageContentEditLimitSeconds: number,
+pushNotificationsEnabled: boolean,
+pushNotificationsEnabledEndTimestamp: number | null,
+createPublicStreamPolicy: CreatePublicOrPrivateStreamPolicyT,
+createPrivateStreamPolicy: CreatePublicOrPrivateStreamPolicyT,
+webPublicStreamsEnabled: boolean,
Expand Down
Loading
Loading