static-user-onboarding-steps

[GoodGuyMarco]

This PR fixes the behaviour discussed here: https://element-io.atlassian.net/browse/PSB-237

The user on-boarding can be tested by adding following key-value pair to the localStorage (see https://github.com/matrix-org/matrix-react-sdk/blob/develop/cypress/e2e/user-onboarding/user-onboarding-new.ts#L33):

localStorage.setItem("mx_registration_time", "1656633601")

Checklist

• Tests written for new code (and old code if feasible)
• Linter and other CI checks pass
• Sign-off given on the changes (see CONTRIBUTING.md)

Notes: User on-boarding tasks now appear in a static order.

Signed-off-by: Marco Bartelt marco.bartelt.ext@bwi.de

---

Here's what your changelog entry will look like:

✨ Features

• User on-boarding tasks now appear in a static order. (#9799). Contributed by @GoodGuyMarco.

[weeman1337]

Thank you for your contribution @GoodGuyMarco

Tested on the deployment. Looks good to me 👍

Sonar is still complaining about test coverage. Can you extend the tests? You can find a detailed report here.

Design already approved it in PSB-237

[GoodGuyMarco]

Thank you for your review @weeman1337 👍

Currently working on the missing unit tests and wondering why we delay rendering of the UserOnboardingList component with a timeout after the initial sync is completed (line 54-69 in UserOnboardingPage). Any ideas?

[weeman1337]

Ping @justjanne ☝️ Do you know about why this timeout is there?

[justjanne]

@weeman1337

NOTE: I'm on vacation, so I don't have access to my work system right now. As result, the following is going by memory, without checking against my notes from when this was added.

The specification from design was to fade the list in after fading in the header. To make sure we fade in all parts in the correct order and with correct timing, a combination of timeouts and css animations was used.

Timeouts were used in places where otherwise an interactive element might just end up invisible but interactable.

We also had the design specification to sort items based on whether they were completed or not.

Before changing timing or UX on the onboarding list, make sure to check your modifications with the design team.

[weeman1337]

It seems that Janne is on holiday at least for this week. Will drop her a message.
The timeout is out of scope of this PR anyway I would say.

[GoodGuyMarco]

I've looked into unit testing UserOnboardingPage for a couple of hours now and came to the conclusion that it is not suitable for a unit test as it requires lots of mocking. This would result in an unreadable and unmaintainable test in my honest opinion. Writing bad tests just to pass a coverage target is not a good practice. Furthermore my code changes don't even change the behaviour of this component.

Any ideas on how to proceed? @weeman1337

This is what I got so far but it runs in a timeout somehow:

describe("UserOnboardingPage", () => {
    it("should render the new user on-boarding experience", async () => {
        // First of all we need to affect the result of `showUserOnboardingPage(useCase)` which requires to mock the function `userRegisteredAfter` on the client ...
        stubClient();
        const client = MatrixClientPeg.get();
        // ... but this does not work and I don't know why.
        client.userRegisteredAfter = () => true;

        // The `useCase` argument of `showUserOnboardingPage(useCase)` needs mocking as well and I found no way how to mock the return value of:
        // `const useCase = useSettingValue<UseCase | null>("FTUE.useCaseSelection");` ... so just overriding it in here, which works, but is kind of ugly.
        await SettingsStore.setValue("FTUE.useCaseSelection", null, SettingLevel.ACCOUNT, UseCase.Skip);

        // const Page = UserOnboardingPage;
        const Page = wrapInMatrixClientContext(UserOnboardingPage);
        render(<Page />);

        expect(true).toBe(true);
    });
});

[weeman1337]

@GoodGuyMarco I've looked into unit testing UserOnboardingPage and came to the conclusion that it is suitable for a unit test 😉 The component contains logic that can and should be tested.

You can use this as a starting point:

`UserOnboardingPage-test.tsx`

/*
Copyright 2022 The Matrix.org Foundation C.I.C.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

    http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

import React from "react";
import { act, render, RenderResult } from "@testing-library/react";

import { filterConsole, stubClient } from "../../../test-utils";
import { UserOnboardingPage } from "../../../../src/components/views/user-onboarding/UserOnboardingPage";
import { MatrixClientPeg } from "../../../../src/MatrixClientPeg";
import SdkConfig from "../../../../src/SdkConfig";

jest.mock("../../../../src/components/structures/EmbeddedPage", () => ({
    __esModule: true,
    default: jest.fn().mockImplementation(({ url }) => <div>{url}</div>),
}));

jest.mock("../../../../src/components/structures/HomePage", () => ({
    __esModule: true,
    default: jest.fn().mockImplementation(() => <div>home page</div>),
}));

describe("UserOnboardingPage", () => {
    let restoreConsole: () => void;

    const renderComponent = async (): Promise<RenderResult> => {
        const renderResult = render(<UserOnboardingPage />);
        await act(async () => {
            jest.runAllTimers();
        });
        return renderResult;
    };

    beforeAll(() => {
        restoreConsole = filterConsole(
            // unrelated for this test
            "could not update user onboarding context",
        );
    });

    beforeEach(() => {
        stubClient();
        jest.useFakeTimers();
    });

    afterEach(() => {
        jest.useRealTimers();
    });

    afterAll(() => {
        restoreConsole();
    });

    describe("when the user registered before the cutoff date", () => {
        beforeEach(() => {
            jest.spyOn(MatrixClientPeg, "userRegisteredAfter").mockReturnValue(false);
        });

        it("should render the home page", async () => {
            expect((await renderComponent()).queryByText("home page")).toBeInTheDocument();
        });
    });

    describe("when the user registered after the cutoff date", () => {
        beforeEach(() => {
            jest.spyOn(MatrixClientPeg, "userRegisteredAfter").mockReturnValue(true);
        });

        describe("and there is an explicit home page configured", () => {
            beforeEach(() => {
                jest.spyOn(SdkConfig, "get").mockReturnValue({
                    embedded_pages: {
                        home_url: "https://example.com/home",
                    },
                });
            });

            it("should render the configured page", async () => {
                expect((await renderComponent()).queryByText("https://example.com/home")).toBeInTheDocument();
            });
        });
    });
});

[weeman1337]

Please re-request review once the UserOnboardingPage test is there.

[GoodGuyMarco]

@weeman1337 sorry for my initial frustration with the tests and thank you for your help once again 😇

[weeman1337]

Looks good now 👍 Tested on the deployment. Works