From 02047243f0aa8eb1c35808d4271b39ae656cb124 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Tue, 30 Jul 2024 15:55:08 +0100 Subject: [PATCH] Rework how the onboarding notifications task works (#12839) * Rework how the onboarding notifications task works Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * Add test Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * Iterate Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --------- Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --- src/Notifier.ts | 12 +++++- src/hooks/useUserOnboardingContext.ts | 26 +++++++++---- src/hooks/useUserOnboardingTasks.ts | 10 +++-- src/i18n/strings/en_EN.json | 4 +- src/toasts/DesktopNotificationsToast.ts | 6 ++- test/hooks/useUserOnboardingTasks-test.tsx | 44 +++++++++++++++++++++- 6 files changed, 85 insertions(+), 17 deletions(-) diff --git a/src/Notifier.ts b/src/Notifier.ts index dbef9bccf47..d482091c3a4 100644 --- a/src/Notifier.ts +++ b/src/Notifier.ts @@ -29,6 +29,7 @@ import { IRoomTimelineData, M_LOCATION, EventType, + TypedEventEmitter, } from "matrix-js-sdk/src/matrix"; import { logger } from "matrix-js-sdk/src/logger"; import { PermissionChanged as PermissionChangedEvent } from "@matrix-org/analytics-events/types/typescript/PermissionChanged"; @@ -103,7 +104,15 @@ const msgTypeHandlers: Record string | null> = { }, }; -class NotifierClass { +export const enum NotifierEvent { + NotificationHiddenChange = "notification_hidden_change", +} + +interface EmittedEvents { + [NotifierEvent.NotificationHiddenChange]: (hidden: boolean) => void; +} + +class NotifierClass extends TypedEventEmitter { private notifsByRoom: Record = {}; // A list of event IDs that we've received but need to wait until @@ -357,6 +366,7 @@ class NotifierClass { if (persistent && global.localStorage) { global.localStorage.setItem("notifications_hidden", String(hidden)); } + this.emit(NotifierEvent.NotificationHiddenChange, hidden); } public shouldShowPrompt(): boolean { diff --git a/src/hooks/useUserOnboardingContext.ts b/src/hooks/useUserOnboardingContext.ts index 1d622173f65..db1b8081f84 100644 --- a/src/hooks/useUserOnboardingContext.ts +++ b/src/hooks/useUserOnboardingContext.ts @@ -18,15 +18,17 @@ import { logger } from "matrix-js-sdk/src/logger"; import { ClientEvent, MatrixClient } from "matrix-js-sdk/src/matrix"; import { useCallback, useEffect, useMemo, useRef, useState } from "react"; -import { Notifier } from "../Notifier"; +import { Notifier, NotifierEvent } from "../Notifier"; import DMRoomMap from "../utils/DMRoomMap"; import { useMatrixClientContext } from "../contexts/MatrixClientContext"; +import { useSettingValue } from "./useSettings"; +import { useEventEmitter } from "./useEventEmitter"; export interface UserOnboardingContext { hasAvatar: boolean; hasDevices: boolean; hasDmRooms: boolean; - hasNotificationsEnabled: boolean; + showNotificationsPrompt: boolean; } const USER_ONBOARDING_CONTEXT_INTERVAL = 5000; @@ -82,6 +84,18 @@ function useUserOnboardingContextValue(defaultValue: T, callback: (cli: Matri return value; } +function useShowNotificationsPrompt(): boolean { + const [value, setValue] = useState(Notifier.shouldShowPrompt()); + useEventEmitter(Notifier, NotifierEvent.NotificationHiddenChange, () => { + setValue(Notifier.shouldShowPrompt()); + }); + const setting = useSettingValue("notificationsEnabled"); + useEffect(() => { + setValue(Notifier.shouldShowPrompt()); + }, [setting]); + return value; +} + export function useUserOnboardingContext(): UserOnboardingContext { const hasAvatar = useUserOnboardingContextValue(false, async (cli) => { const profile = await cli.getProfileInfo(cli.getUserId()!); @@ -96,12 +110,10 @@ export function useUserOnboardingContext(): UserOnboardingContext { const dmRooms = DMRoomMap.shared().getUniqueRoomsWithIndividuals() ?? {}; return Boolean(Object.keys(dmRooms).length); }); - const hasNotificationsEnabled = useUserOnboardingContextValue(false, async () => { - return Notifier.isPossible(); - }); + const showNotificationsPrompt = useShowNotificationsPrompt(); return useMemo( - () => ({ hasAvatar, hasDevices, hasDmRooms, hasNotificationsEnabled }), - [hasAvatar, hasDevices, hasDmRooms, hasNotificationsEnabled], + () => ({ hasAvatar, hasDevices, hasDmRooms, showNotificationsPrompt }), + [hasAvatar, hasDevices, hasDmRooms, showNotificationsPrompt], ); } diff --git a/src/hooks/useUserOnboardingTasks.ts b/src/hooks/useUserOnboardingTasks.ts index 8dc06efa5b8..176242b1243 100644 --- a/src/hooks/useUserOnboardingTasks.ts +++ b/src/hooks/useUserOnboardingTasks.ts @@ -136,14 +136,18 @@ const tasks: UserOnboardingTask[] = [ id: "permission-notifications", title: _t("onboarding|enable_notifications"), description: _t("onboarding|enable_notifications_description"), - completed: (ctx: UserOnboardingContext) => ctx.hasNotificationsEnabled, + completed: (ctx: UserOnboardingContext) => !ctx.showNotificationsPrompt, action: { label: _t("onboarding|enable_notifications_action"), onClick: (ev: ButtonEvent) => { PosthogTrackers.trackInteraction("WebUserOnboardingTaskEnableNotifications", ev); - Notifier.setEnabled(true); + defaultDispatcher.dispatch({ + action: Action.ViewUserSettings, + initialTabId: UserTab.Notifications, + }); + Notifier.setPromptHidden(true); }, - hideOnComplete: true, + hideOnComplete: !Notifier.isPossible(), }, }, ]; diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index 025fbf89640..a542fc7c601 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -1652,8 +1652,8 @@ "download_brand_desktop": "Download %(brand)s Desktop", "download_f_droid": "Get it on F-Droid", "download_google_play": "Get it on Google Play", - "enable_notifications": "Turn on notifications", - "enable_notifications_action": "Enable notifications", + "enable_notifications": "Turn on desktop notifications", + "enable_notifications_action": "Open settings", "enable_notifications_description": "Don’t miss a reply or important message", "explore_rooms": "Explore Public Rooms", "find_community_members": "Find and invite your community members", diff --git a/src/toasts/DesktopNotificationsToast.ts b/src/toasts/DesktopNotificationsToast.ts index 8d02b657b4b..676d79842d3 100644 --- a/src/toasts/DesktopNotificationsToast.ts +++ b/src/toasts/DesktopNotificationsToast.ts @@ -20,9 +20,11 @@ import GenericToast from "../components/views/toasts/GenericToast"; import ToastStore from "../stores/ToastStore"; import { MatrixClientPeg } from "../MatrixClientPeg"; import { getLocalNotificationAccountDataEventType } from "../utils/notifications"; +import SettingsStore from "../settings/SettingsStore"; +import { SettingLevel } from "../settings/SettingLevel"; -const onAccept = (): void => { - Notifier.setEnabled(true); +const onAccept = async (): Promise => { + await SettingsStore.setValue("notificationsEnabled", null, SettingLevel.DEVICE, true); const cli = MatrixClientPeg.safeGet(); const eventType = getLocalNotificationAccountDataEventType(cli.deviceId!); cli.setAccountData(eventType, { diff --git a/test/hooks/useUserOnboardingTasks-test.tsx b/test/hooks/useUserOnboardingTasks-test.tsx index f2d65382a4d..26a2ba4b756 100644 --- a/test/hooks/useUserOnboardingTasks-test.tsx +++ b/test/hooks/useUserOnboardingTasks-test.tsx @@ -14,9 +14,16 @@ See the License for the specific language governing permissions and limitations under the License. */ +import React from "react"; import { renderHook } from "@testing-library/react-hooks"; +import { waitFor } from "@testing-library/react"; import { useUserOnboardingTasks } from "../../src/hooks/useUserOnboardingTasks"; +import { useUserOnboardingContext } from "../../src/hooks/useUserOnboardingContext"; +import { stubClient } from "../test-utils"; +import MatrixClientContext from "../../src/contexts/MatrixClientContext"; +import DMRoomMap from "../../src/utils/DMRoomMap"; +import PlatformPeg from "../../src/PlatformPeg"; describe("useUserOnboardingTasks", () => { it.each([ @@ -25,7 +32,7 @@ describe("useUserOnboardingTasks", () => { hasAvatar: false, hasDevices: false, hasDmRooms: false, - hasNotificationsEnabled: false, + showNotificationsPrompt: false, }, }, { @@ -33,7 +40,7 @@ describe("useUserOnboardingTasks", () => { hasAvatar: true, hasDevices: false, hasDmRooms: false, - hasNotificationsEnabled: true, + showNotificationsPrompt: true, }, }, ])("sequence should stay static", async ({ context }) => { @@ -46,4 +53,37 @@ describe("useUserOnboardingTasks", () => { expect(result.current[3].id).toBe("setup-profile"); expect(result.current[4].id).toBe("permission-notifications"); }); + + it("should mark desktop notifications task completed on click", async () => { + jest.spyOn(PlatformPeg, "get").mockReturnValue({ + supportsNotifications: jest.fn().mockReturnValue(true), + maySendNotifications: jest.fn().mockReturnValue(false), + } as any); + + const cli = stubClient(); + cli.pushRules = { + global: { + override: [ + { + rule_id: ".m.rule.master", + enabled: false, + actions: [], + default: true, + }, + ], + }, + }; + DMRoomMap.makeShared(cli); + const context = renderHook(() => useUserOnboardingContext(), { + wrapper: (props) => { + return {props.children}; + }, + }); + const { result, rerender } = renderHook(() => useUserOnboardingTasks(context.result.current)); + expect(result.current[4].id).toBe("permission-notifications"); + await waitFor(() => expect(result.current[4].completed).toBe(false)); + result.current[4].action!.onClick!({ type: "click" } as any); + rerender(); + await waitFor(() => expect(result.current[4].completed).toBe(true)); + }); });