Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.34k stars 2.77k forks source link

[HOLD for payment 2024-06-28] [$250] Add automated tests for `getGroupChatName()` #40189

Closed marcaaron closed 2 months ago

marcaaron commented 5 months ago

cc @roryabraham coming from https://github.com/Expensify/App/pull/39757

Let's perform the following checks and add automated testing in this manner:

Issue OwnerCurrent Issue Owner: @miljakljajic
roryabraham commented 5 months ago

love it - once https://github.com/Expensify/App/pull/39757 is merged will you make this external then?

marcaaron commented 5 months ago

Gonna give first licks to @s77rt since they have a ton of context at this point. But if they don't want to work on it we can make it External.

s77rt commented 5 months ago

To not block on this let's make it external for now (I will be C+ if C+ is needed here). I will pick this up if no one took it (once I get more work done)

ShridharGoel commented 5 months ago

Verify that the routes for Group participants invites, participant details, and role selection works as expected. Test corner cases like deep link, etc.

Does this mean manual verification or via tests only?

Verify that the performance of the options list in InviteReportParticipantsPage is adequate.

Same question for this.

ShridharGoel commented 5 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Add tests for getGroupChatName() to verify that it is working correctly.

What is the root cause of that problem?

New tests.

What changes do you think we should make in order to solve the problem?

Add UI and unit tests.

Some examples of UI tests:

We can also have a test to check the alternateText.

Unit tests:

In these tests, we can also test for cases when participants's list has only one item. In this, long form of display name should show.

We can also test for case when formattedLogin is used.

Example code for UI test:

describe('Group name', () => {
    afterEach(() => {
        jest.clearAllMocks();
        Onyx.clear();

        // Unsubscribe to pusher channels
        PusherHelper.teardown();
    });

    it('Check if group name is showing correctly in LHN', () =>
        signInAndGetApp("A, B, C, D")
            .then(() => {
                // Verify the sidebar links are rendered
                const sidebarLinksHintText = Localize.translateLocal('sidebarScreen.listOfChats');
                const sidebarLinks = screen.queryAllByLabelText(sidebarLinksHintText);
                expect(sidebarLinks).toHaveLength(1);

                // Verify there is only one option in the sidebar (Optional)
                const optionRowsHintText = Localize.translateLocal('accessibilityHints.navigatesToChat');
                const optionRows = screen.queryAllByAccessibilityHint(optionRowsHintText);
                expect(optionRows).toHaveLength(1);

                // Get the display name text
                const displayNameHintText = Localize.translateLocal('accessibilityHints.chatUserDisplayNames');
                const displayNameText = screen.queryByLabelText(displayNameHintText);

                // A, B, C, D are the display names of the participants in the group chat
                return waitFor(() => expect(displayNameText?.props?.children?.[0]).toBe("A, B, C, D"));
            }));
});

Note that the names taken will be diff combinations, so that we can test the sorting logic as well.

signInAndGetApp code ``` function signInAndGetApp(reportName: string | null = null): Promise { // Render the App and sign in as a test user. render(); return waitForBatchedUpdatesWithAct() .then(async () => { await waitForBatchedUpdatesWithAct(); const hintText = Localize.translateLocal('loginForm.loginForm'); const loginForm = screen.queryAllByLabelText(hintText); expect(loginForm).toHaveLength(1); await act(async () => { await TestHelper.signInWithTestUser(USER_A_ACCOUNT_ID, USER_A_EMAIL, undefined, undefined, 'A'); }); return waitForBatchedUpdatesWithAct(); }) .then(() => { User.subscribeToUserEvents(); return waitForBatchedUpdates(); }) .then(async () => { const TEN_MINUTES_AGO = subMinutes(new Date(), 10); reportActionCreatedDate = format(addSeconds(TEN_MINUTES_AGO, 30), CONST.DATE.FNS_DB_FORMAT_STRING); reportAction2CreatedDate = format(addSeconds(TEN_MINUTES_AGO, 60), CONST.DATE.FNS_DB_FORMAT_STRING); // Simulate setting a group chat and personal details await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${REPORT_ID}`, { reportID: REPORT_ID, reportName: reportName, lastReadTime: reportActionCreatedDate, lastVisibleActionCreated: reportAction2CreatedDate, lastMessageText: 'Test', participantAccountIDs: [USER_A_ACCOUNT_ID, USER_B_ACCOUNT_ID, USER_C_ACCOUNT_ID, USER_D_ACCOUNT_ID], groupChatAdminLogins: USER_A_EMAIL, lastActorAccountID: USER_B_ACCOUNT_ID, type: CONST.REPORT.TYPE.CHAT, chatType: CONST.REPORT.CHAT_TYPE.GROUP, }); const createdReportActionID = NumberUtils.rand64().toString(); await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${REPORT_ID}`, { [createdReportActionID]: { actionName: CONST.REPORT.ACTIONS.TYPE.CREATED, automatic: false, created: format(TEN_MINUTES_AGO, CONST.DATE.FNS_DB_FORMAT_STRING), reportActionID: createdReportActionID, message: [ { style: 'strong', text: '__FAKE__', type: 'TEXT', }, { style: 'normal', text: 'created this report', type: 'TEXT', }, ], }, }); await Onyx.merge(ONYXKEYS.PERSONAL_DETAILS_LIST, { [USER_A_ACCOUNT_ID]: TestHelper.buildPersonalDetails(USER_A_EMAIL, USER_A_ACCOUNT_ID, 'A'), [USER_B_ACCOUNT_ID]: TestHelper.buildPersonalDetails(USER_B_EMAIL, USER_B_ACCOUNT_ID, 'B'), [USER_C_ACCOUNT_ID]: TestHelper.buildPersonalDetails(USER_C_EMAIL, USER_C_ACCOUNT_ID, 'C'), [USER_D_ACCOUNT_ID]: TestHelper.buildPersonalDetails(USER_D_EMAIL, USER_D_ACCOUNT_ID, 'D'), }); // We manually setting the sidebar as loaded since the onLayout event does not fire in tests AppActions.setSidebarLoaded(); return waitForBatchedUpdatesWithAct(); }); } ```
Screenshot 2024-04-13 at 8 42 13 PM

Example code of unit tests:

describe('getGroupChatName tests', () => {
    it('Should show all participants name if count <= 5 and shouldApplyLimit is false', async function () {
        const report = {
            ...LHNTestUtils.getFakeReport([1, 2, 3, 4]),
            chatType: CONST.REPORT.CHAT_TYPE.GROUP,
        };

        await Onyx.merge(ONYXKEYS.PERSONAL_DETAILS_LIST, participantsPersonalDetails);
        expect(ReportUtils.getGroupChatName(report?.participantAccountIDs ?? [])).toEqual("(833) 240-3627, floki@vikings.net, Lagertha, Ragnar");
    });

    it('Should show 5 participants name if count > 5 and shouldApplyLimit is true', async function () {
        const report = {
            ...LHNTestUtils.getFakeReport([1, 2, 3, 4, 5, 6, 7, 8]),
            chatType: CONST.REPORT.CHAT_TYPE.GROUP,
        };

        await Onyx.merge(ONYXKEYS.PERSONAL_DETAILS_LIST, participantsPersonalDetails);
        expect(ReportUtils.getGroupChatName(report?.participantAccountIDs ?? [], true)).toEqual("(833) 240-3627, floki@vikings.net, Lagertha, Lagertha, Ragnar");
    });
});
melvin-bot[bot] commented 5 months ago

Job added to Upwork: https://www.upwork.com/jobs/~012f0f59dd6db6b8bd

melvin-bot[bot] commented 5 months ago

Current assignee @s77rt is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 5 months ago

Triggered auto assignment to @miljakljajic (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

melvin-bot[bot] commented 5 months ago

:warning: It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time :warning:

s77rt commented 5 months ago

@ShridharGoel Thanks for the proposal. Overall this looks good to me 👍

🎀 👀 🎀 C+ reviewed Link to proposal

melvin-bot[bot] commented 5 months ago

Current assignee @marcaaron is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

melvin-bot[bot] commented 5 months ago

📣 @s77rt 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 5 months ago

📣 @ShridharGoel 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

marcaaron commented 5 months ago

Thanks very much for your proposal @ShridharGoel 🙇

Does this mean manual verification or via tests only?

Manual verification is sufficient. As far as I can tell we don't have automated tests for complex navigation flows.

Rest of the proposal looks 💎

ShridharGoel commented 5 months ago

Thanks, link to the PR: https://github.com/Expensify/App/pull/40658

ShridharGoel commented 5 months ago

While testing, I found these bugs:

Thoughts on these bugs?

Should we create issues for these? I’ll like to work on these if possible.

marcaaron commented 5 months ago

Thanks @ShridharGoel, but they're not bugs.

This shouldn’t be allowed, either leave button shouldn’t show in this case, or we can ask the user about whom to make the admin when leave button is clicked.

This is allowed. An admin gets promoted automatically.

Clicking on the leave button doesn’t show any confirmation dialog.

Because there's nothing to confirm.

melvin-bot[bot] commented 4 months ago

This issue has not been updated in over 15 days. @s77rt, @miljakljajic, @marcaaron, @ShridharGoel eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

s77rt commented 4 months ago

Bumped the PR

melvin-bot[bot] commented 3 months ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 3 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.0-9 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-06-28. :confetti_ball:

For reference, here are some details about the assignees on this issue:

melvin-bot[bot] commented 3 months ago

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

s77rt commented 3 months ago

No regression test needed. We only adds tests.

melvin-bot[bot] commented 2 months ago

@s77rt, @miljakljajic, @marcaaron, @ShridharGoel Whoops! This issue is 2 days overdue. Let's get this updated quick!

miljakljajic commented 2 months ago

@s77rt paid - @ShridharGoel please accept the new offer, it seems there was an error with the original one.

marcaaron commented 2 months ago

Not overdue. Seems like we're almost done with this one.

miljakljajic commented 2 months ago

@ShridharGoel bump

ShridharGoel commented 2 months ago

This was accepted few days back, thanks.

miljakljajic commented 2 months ago

Paid and ready to close