[HOLD for payment 2024-06-06] [$250] Implement offline behavior and error handling for UpdateGroupChatName

[marcaaron]

Similar to #39983

We are now able to add a custom Group chat name. However, we do not handle any failure case yet.

Let's take whatever we are doing for the update "workspace room" name flow and apply the same error handling here.

In the backend, we currently send exceptions like this:

Onyx::handleException($e, 'AD9240FD-A780-425D-B98D-96DD6E4251C1', OnyxKeys::COLLECTION_REPORT.$reportID, ['errorFields', 'reportName']);

Which will put an error field on the report if updating the name fails.

We need that error to lead the user back to the "Settings" > "Name" field (RBR) and display an error message back to the user.

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0128ec154dad5b4df3
• Upwork Job ID: 1786226453820362752
• Last Price Increase: 2024-05-03
• Automatic offers:
• dukenv0307 | Reviewer | 0
• nkdengineer | Contributor | 0

Issue Owner

Current Issue Owner: @jliexpensify

[s77rt]

Cases to check (not limited to):

1. The value that we validate vs. the value that we send to the BE are different (one is trimmed the other is not)
2. Is the limit 255 or 256 (isValidReportName uses 255 while maxLength uses 256)
3. See if we can remove isValidReportName function (maxLength prop should be enough)
4. Clear error after trying to set a new name

Screen.Recording.2024-04-11.at.8.47.27.AM.mov

[marcaaron]

This can come off HOLD now

[marcaaron]

On looking some more I am not sure if we need any BE change for this. This is the only check we have that might throw...

So, in reality the BE is more restrictive than the frontend atm. But I think that is fine.

Let's just update the frontend and make sure we handle any possible error (though I'm not sure what that would be really besides user losing access to the report - and that is quite rare).

[melvin-bot]

Triggered auto assignment to @jliexpensify (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]

⚠️ 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 ⚠️

[melvin-bot]

Triggered auto assignment to Design team member for new feature review - @dannymcclain (NewFeature)

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0128ec154dad5b4df3

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @dukenv0307 (External)

[nkdengineer]

Proposal

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

Implement offline behavior and error handling for UpdateGroupChatName

What is the root cause of that problem?

This is a new feature

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

1. To implement offline behavior, we should update pendingFields of reportName in optimisticData and reset it to null in successData

const optimisticData: OnyxUpdate[] = [
    {
        onyxMethod: Onyx.METHOD.MERGE,
        key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
        value: {
            reportName,
            pendingFields: {
                reportName: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE
            }
        },
    },
];

const successData: OnyxUpdate[] = [
    {
        onyxMethod: Onyx.METHOD.MERGE,
        key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
        value: {
            pendingFields: {
                reportName: null
            }
        },
    },
];

2. To handle the error, we can use the error that is returned by BE or we can create a new translation key for this error in FE. And we also should reset the group chat name in failureData as we do when we update the room name.

const failureData: OnyxUpdate[] = [
    {
        onyxMethod: Onyx.METHOD.MERGE,
        key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
        value: {
            reportName: getReport(reportID)?.reportName,
            errors: {
                reportName: 'Invalid group chat name'
            }
        },
    },
];

App/src/libs/actions/Report.ts

Line 635 in ee8f9c6

value: {reportName},

And here, we should only get the latest error to avoid duplicate error is displayed

App/src/pages/settings/Report/ReportSettingsPage.tsx

Line 74 in ee8f9c6

errors={report?.errorFields?.reportName}

What alternative solutions did you explore? (Optional)

NA

[dannymcclain]

@marcaaron do you need anything specific from design for this other than a review? It looks like we should be able to just follow our standard error patterns that we're using elsewhere, yeah?

[marcaaron]

Nope, I think we're good!

[dukenv0307]

@nkdengineer We already have the PR to handle errors: #40587, so when users update the new name, the previous error will be cleared. I see in your proposal you update pendingFields, but you don't use it anywhere, right? I don't actually know what should we do on that issue

[nkdengineer]

@dukenv0307 Thanks for your feedback

In ReportSettingPage, we've already had the logic to use pendingFields here. So we only need to update the pendingFields in optimisticData

App/src/pages/settings/Report/ReportSettingsPage.tsx

Lines 73 to 75 in 4c243a2

	pendingAction={report?.pendingFields?.reportName}
	errors={report?.errorFields?.reportName}
	errorRowStyles={[styles.ph5]}

[dukenv0307]

Thanks @nkdengineer, your proposal looks good to me. We don't need to update error in failureData since BE already returned error message. The PR: #40587 is merged -> we won't face with duplicated error case.

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[marcaaron]

I think we should set the reportName field back to an empty string and not null does that also work?

we can use the error that is returned by BE or we can create a new translation key for this error in FE

Let's just ignore whatever the backend is sending as a message and use something generic. There are so few ways this could go wrong that it isn't worth optimizing for right now.

[melvin-bot]

📣 @dukenv0307 🎉 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]

📣 @nkdengineer 🎉 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 📖

[nkdengineer]

@dukenv0307 The PR is here.

[melvin-bot]

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

[melvin-bot]

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

• Implement offline behavior and error handling for UpdateGroupChatName #41826

If no regressions arise, payment will be issued on 2024-06-06. 🎊

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

• @dukenv0307 requires payment automatic offer (Reviewer)
• @nkdengineer requires payment automatic offer (Contributor)

[melvin-bot]

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:

• [@dukenv0307] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
• [@jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

[jliexpensify]

Payment Summary

• C: @nkdengineer $250
• C+: @dukenv0307 $250

Upwork job

Just waiting on the checklist

[dukenv0307]

BugZero Checklist:

• The PR that introduced the bug has been identified. Link to the PR: N/A
• The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: N/A
• A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
• Determine if we should create a regression test for this bug. Yes
• If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression test:

1. Create a group
2. Go to force offline mode
3. Try to edit group name with an invalid name (example: adoadoadoajdoajdoadoajdoajadoadoadoajdoajdoadoajdoajadoadoadoajdoajdoadoajdoajadoadoadoajdoajdoadoaỏ)
4. Verify that: group name in Settings is grayed out
5. Back to online mode
6. Verify that: group name returns back and the error message appears

Do we 👍 or 👎

[marcaaron]

That sounds like a good test case to me 👍

[jliexpensify]

Paid, job closed. Thanks everyone!