[$250] Implement offline behavior and error handling for `UpdateGroupChatName`

[marcaaron]

Similar to https://github.com/Expensify/App/issues/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: @dukenv0307

[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

https://github.com/Expensify/App/assets/16493223/d7baff8a-31e4-4b27-82d6-309b3fd263af

[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[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[bot]]

: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:

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[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'
            }
        },
    },
];

https://github.com/Expensify/App/blob/ee8f9c661f269f405a5ae8ba47ea07f42a306c6c/src/libs/actions/Report.ts#L635

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

https://github.com/Expensify/App/blob/ee8f9c661f269f405a5ae8ba47ea07f42a306c6c/src/pages/settings/Report/ReportSettingsPage.tsx#L74

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: https://github.com/Expensify/App/issues/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

https://github.com/Expensify/App/blob/4c243a201197bb201e933c82e405a936b0fe27af/src/pages/settings/Report/ReportSettingsPage.tsx#L73-L75

[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: https://github.com/Expensify/App/issues/40587 is merged -> we won't face with duplicated error case.

🎀👀🎀 C+ reviewed

[melvin-bot[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[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[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.