Rework how the onboarding notifications task works

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, (event: MatrixEvent) => string | null> = {
     },
 };
 
-class NotifierClass {
+export const enum NotifierEvent {
+    NotificationHiddenChange = "notification_hidden_change",
+}
+
+interface EmittedEvents {
+    [NotifierEvent.NotificationHiddenChange]: (hidden: boolean) => void;
+}
+
+class NotifierClass extends TypedEventEmitter<keyof EmittedEvents, EmittedEvents> {
     private notifsByRoom: Record<string, Notification[]> = {};
 
     // 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 {

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<T>(defaultValue: T, callback: (cli: Matri
     return value;
 }
 
+function useShowNotificationsPrompt(): boolean {
+    const [value, setValue] = useState<boolean>(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],
     );
 }

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(),
         },
     },
 ];

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",

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<void> => {
+    await SettingsStore.setValue("notificationsEnabled", null, SettingLevel.DEVICE, true);
     const cli = MatrixClientPeg.safeGet();
     const eventType = getLocalNotificationAccountDataEventType(cli.deviceId!);
     cli.setAccountData(eventType, {

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,15 +32,15 @@ describe("useUserOnboardingTasks", () => {
                 hasAvatar: false,
                 hasDevices: false,
                 hasDmRooms: false,
-                hasNotificationsEnabled: false,
+                showNotificationsPrompt: false,
             },
         },
         {
             context: {
                 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 <MatrixClientContext.Provider value={cli}>{props.children}</MatrixClientContext.Provider>;
+            },
+        });
+        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));
+    });
 });