[HOLD for payment 2023-11-21][$1000] Offline - Create task in a room with admin only

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Login to two accounts, Alice and Bob, in two separate browser tabs.
2. Alice creates a workspace and invites Bob as a non-Admin member.
3. Bob goes to the #announce room for the new workspace.
4. Bob goes offline.
5. Alice goes to the #announce room for the new workspace, opens room settings, and under Who can post, sets Admins only.
6. Since Bob is offline, he does not receive an update that the room has become read-only for non-admins. Therefore, he can still attempt to create a task offline in the #announce room.

Expected result

When Bob comes back online, the task should appear "greyed out" with a "red brick road" error. Any actions Bob attempted to take on that task (such as leaving a reaction), should fail.

Actual Result:

The task appears to have been completed normally for Bob, unless he tries to create a thread with that task as the root, in which case he'll see a 404 page.

Workaround:

Don't do that, Bob!

Platforms:

Which of our officially supported platforms is this issue occurring on?

• [ ] Android / native
• [ ] Android / Chrome
• [ ] iOS / native
• [ ] iOS / Safari
• [x] MacOS / Chrome / Safari
• [ ] MacOS / Desktop

Version Number: 1.3.39-5 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/43996225/1983e5a9-e67b-4a0f-9736-7fbea6a98fa5

https://github.com/Expensify/App/assets/43996225/7e1602b3-b70a-43ce-9722-c66a23fd0a93

Expensify/Expensify Issue URL: Issue reported by: @namhihi237 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1689010436307099

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01afb147c594f572e5
• Upwork Job ID: 1679478215688933376
• Last Price Increase: 2023-07-13
• Automatic offers:
• namhihi237 | Contributor | 26516237
• namhihi237 | Reporter | 26516238

[thienlnam]

Ready to review proposals cc @parasharrajat

[namhihi237]

@thienlnam @parasharrajat before we already reviewed once here ^^

[melvin-bot[bot]]

📣 @parasharrajat Please request via NewDot manual requests for the Reviewer role ($1000)

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

Offer link Upwork job

[namhihi237]

@thienlnam The PR needs to be translated, Please help add a copy label. Thanks

[parasharrajat]

@namhihi237 Please ask for translations on Slack.

[namhihi237]

Ask on slack

[melvin-bot[bot]]

Triggered auto assignment to @gabrielessner (Waiting for copy), see https://stackoverflow.com/c/expensify/questions/7025/ for more details.

[thienlnam]

@gabrielessner Could you please check the copy for when a task was failed to be created?

Here are some of the existing error copy

        error: {
            invalidSplit: 'Split amounts do not equal total amount',
            other: 'Unexpected error, please try again later',
            genericCreateFailureMessage: 'Unexpected error requesting money, please try again later',
            genericDeleteFailureMessage: 'Unexpected error deleting the money request, please try again later',
            genericEditFailureMessage: 'Unexpected error editing the money request, please try again later',
            genericSmartscanFailureMessage: 'Transaction is missing fields',
        },

Maybe something like: Unexpected error creating a task, please try again later?

[gabrielessner]

i am in the middle of and fully focused on re-writing roughly 30 help articles for the help dot migration, i wont be bale to get to this until tomorrow.

[parasharrajat]

@thienlnam We are adding errors for parent report action where task action is shown. But what the task report which user created offline.

Currently, backend is deleting this report which is fine but not aligned with other pages in our app.

In the new chat case, if the new chat is invalid an RBR error will be shown on chat instead of deleting the chat from the backend.

But on the task report, there are two issues.

1. First, the task report will convert to Hmm, page not found as it is deleted. Check at 0.23 sec.

https://user-images.githubusercontent.com/39086067/265939092-b0632f42-e977-4dc5-baf4-43d501d58927.mov

2. The app will crash at some point when the data goes undefined from an object. This is due to a new pattern where we create two Onyx subscriptions and the nested one is dependent on the data from the wrapper Onyx subscription. Thus if wrapper data is changed to undefined the nested subscription will fail.

This is what happens here App/src/components/LHNOptionsList/OptionRowLHNData.js https://github.com/Expensify/App/pull/26848#issuecomment-1707895412

I am not sure if we should call it a anti-pattern as there is no other way to get dependent data from Onyx.

---

Shouldn't we instead send an error for the task report as well which will be shown as RBR like we do for the new chat?

[thienlnam]

Thanks for the summary - I figured this might be the case.

Currently, backend is deleting this report which is fine but not aligned with other pages in our app.

Technically the report doesn't exist in the first place, but the BE does clear the report from onyx on failure. This looks like what we have to do internally is set an error on the report itself and then the front-end PR will have to handle clearing that error / removing the report from onyx locally

[gabrielessner]

    error: {
        invalidSplit: 'Split amounts do not equal total amount',

This one is fine

        other: 'Unexpected error, please try again later',

This one is so vague im afraid its not very helpful - can we maybe update it to say 'Unexpected error, please refresh the page and try again later',

        genericCreateFailureMessage: 'Unexpected error requesting money, please try again later',
        genericDeleteFailureMessage: 'Unexpected error deleting the money request, please try again later',
        genericEditFailureMessage: 'Unexpected error editing the money request, please try again later',

Im fine with these ones

genericSmartscanFailureMessage: 'Transaction is missing fields',

is it possible to include the missing fields in the error message?

@thienlnam

[thienlnam]

@gabrielessner Ah I actually just need the single copy check - those are just examples I provided that already exist currently

This is the one we want to verify: Unexpected error creating a task, please try again later

[gabrielessner]

got it, sorry that wasn't clear to me earlier.

Unexpected error creating a task, please try again later

What is causing this error? Can we explain whats causing the error in the actual message or is that too much information? Would logging out / logging back in potentially fix it and if so, can we include that in the message? @thienlnam

[thienlnam]

What is causing this error? Can we explain whats causing the error in the actual message or is that too much information?

This is kind of a general catch all error, so it could be a number of things. Logging out might fix it but might not depending on the issue but we don't have that context

[gabrielessner]

Logging out might fix it but might not

Ok, cool. If logging out might fix it should we highlight that in the error? Like for example, we could say:

Unexpected error creating a task, please try logging out and logging back in. If the error still persists, please try again later

If you don't like that idea, then I am not sure what else we could say and Im fine with just leaving as is @thienlnam

[namhihi237]

@parasharrajat @thienlnam I have tried to request money with an invalid phone. The report of requested money also delete

https://github.com/Expensify/App/assets/39086067/9e7cad44-cc16-48b2-a783-e4f133cbae71

[namhihi237]

And a new chat scenario, It's a bit different because we're not creating a thread. But the report also delete when click other report.

https://github.com/Expensify/App/assets/39086067/748d7692-b9db-48a9-b49c-5f9f78990f0e

[parasharrajat]

I see. I am just wondering if showing Hmm, it's not there should be the expected behavior or if we should show an RBR error before deleting the report.

For Task, request money, we create n number of chats. So IMO, we should show errors for those chats instead of deleting those.

@thienlnam I would love to hear the next steps for this. What do you think we should do?

1. Only report action errors and clear the report as of now.
2. Report action error as well as Report Creation Error and not deleting the report until the RBR error is cleared.

[namhihi237]

@thienlnam have we agreed to the translation here?

[thienlnam]

Took a look at this with some fresh eyes

As a crash is happening, it means that fullReport is undefined which should not be the case. First, we should see why fullReport is getting undefined. It is getting deleted from Onyx somewhere.

I took a look at the BE again, and we're not intentionally deleting the report from onyx. I think this just happens when we clear it from onyx in the failure data https://github.com/Expensify/App/blob/c9dc4badcb742a08b3420a13b8b256a259f44660/src/libs/actions/Task.js#L134-L136

Let's be consistent with other flows like request money which seems to be the second option

Example with Request Money

1. On failure, instead of deleting the report let's add an error on the task errorFields: { createChat: ErrorUtils.getMicroSecondOnyxError('report.genericCreateReportFailureMessage'), },

2. Add the error on the report action as well errors: ErrorUtils.getMicroSecondOnyxError('iou.error.genericCreateFailureMessage'), },

[thienlnam]

@thienlnam have we agreed to the translation here?

Yes, let's continue with Unexpected error creating a task, please try again later since most cases logging out won't fix the error

[namhihi237]

@thienlnam As I mentioned above, the flow of request money also delete request when an error.

So I think if we want consistency with other flows like request money. I think it's flow 1 Only report action errors and clear the report as of now.

Can see when we call API open report status returns 403 and set null report.

[thienlnam]

So then we don't actually clear the report, and it gets removed when we try to open the report?

[namhihi237]

@thienlnam yes, when we open report it will be deleted.

[thienlnam]

Okay, let's still do https://github.com/Expensify/App/issues/22608#issuecomment-1713078004

But not remove the report data in onyx until OpenReport is called by clicking on the failed task

[parasharrajat]

I agree with https://github.com/Expensify/App/issues/22608#issuecomment-1713078004. I thought that the backend was removing the task report on failure. If this is frontend then it can be handled on frontend easily.

@namhihi237 let's do this change.

[namhihi237]

Sure @thienlnam @parasharrajat Yes, I have updated. And there is a note that when the task is created, it will open the task thread. So it will always call openReport when creating a task.

[parasharrajat]

Ok, so you are saying that due to this action, openReport call will be queued which deleted the task report as a result.

This is the reason, I see task reports getting deleted after going online.

@thienlnam We will have to update OpenAPI as well to return the error or prevent it from sending Onyxpayload to remove the task report.

[thienlnam]

Hmm, I don't know if we want to update OpenReport to work differently depending on the type of report. Though I do see it makes it difficult when OpenReport deletes the report from onyx if it doesn't exist.

However, how do thread errors solve this? I imagine they have the same problem because when you create a thread you automatically are redirected to it calling openReport

[namhihi237]

@thienlnam I can't imagine a situation where an open thread fails. Should we do the same as request money and delete the thread when there is an error?

[parasharrajat]

I checked that when new Chat creation fails, openReport does not delete the report instead returns the error. This tells me that it behaving differently already.

[parasharrajat]

I am fine both ways. Either we should keep the report until RBR is cleared or delete it ASAP when it fails. The only doubt is shouldn't new chat creation failure or new task report creation failure be treated in the same way? IMO, we should keep all types of report creation failure behavior in the same way. But it seems that new chat creation failure behaves differently than other reports.

The reason to treat these differently might be that these have parent reports but new chat does not.

Please let me know the correct expected behavior.

[thienlnam]

IMO, we should keep all types of report creation failure behavior in the same way

I agree, the more we can keep consistent between these reports the better.

I'll take a look in the BE later and see what is different between the new chat creation failure.

[namhihi237]

wait decide which direction we will decide to go. If we follow a similar flow of deleting the report when there is an error, then the PR is ready and we will keep the delete report thread in dataFailure.

[parasharrajat]

@thienlnam Did you manage to look at the backend?

[thienlnam]

I did, and it does look like if we don't have access to a report from the back-end then we send a pusher update to clear the report.

But it's not task specific, and should be happening to failed chats as well.

I checked that when new Chat creation fails, openReport does not delete the report instead returns the error. This tells me that it behaving differently already.

I would be curious to see if this was still true if you switched to a different chat, and then back to the failed chat and if it ended up getting removed

[namhihi237]

@thienlnam after we switch to a different chat the wrong chat before has removed

[parasharrajat]

Yeah, you are correct.

New Chat Creation failure

https://github.com/Expensify/App/assets/24370807/3b8cf93a-1493-4dad-a3b3-670b915da9eb

But it is a little different from task report creation failure. We can see that here the task report is immediately deleted even if that is active in contrast to new chat where the new chat page will be shown with an error until the user moves out.

New Task Creation failure

https://github.com/Expensify/App/assets/24370807/50ae5376-c988-4b0d-9e10-b01c3eb030c4

[parasharrajat]

So this is the conclusion. We are good with removing the task report but the behavior should be the same as the new chat.

i.e.

1. When the task report creation fails and that report is active, the user should see that creation error.
2. when the user moves out of that task report, it gets deleted.
3. Hmm, its not there page should not be shown to the user while he is at it.

This is exactly happening in the new Chat failure scenario. Until the user moves out of the chat screen, a failure error is shown to him.

@thienlnam @namhihi237

[namhihi237]

Currently, the API returns differently when calling open report.

1. With new chat => return error
2. With task report => return key report with value null So do we need to update the API, or we will use the trick below FE which is in the openReport function we can check to see if it is an error report task. If it is correct, we will reassign it with failure data. However, I do not recommend this method because it will cause confusion.

[parasharrajat]

@thienlnam Thoughts?

[isabelastisser]

Bump @thienlnam !

[thienlnam]

Yeah I don't like the idea of doing a front-end hack though I still am finding that the BE is doing the same thing on openReport.

I tested locally removing

    {
        onyxMethod: Onyx.METHOD.SET,
        key: `${ONYXKEYS.COLLECTION.REPORT}${optimisticTaskReport.reportID}`,
        value: null,
    },

in App, and the reportData is still there in my testing. Could this just be the case?

https://github.com/Expensify/App/assets/30609178/e5d4dbe2-0430-4fc6-bb76-946bcf62a3ec

[namhihi237]

I'm not sure but it looks like we changed it recently. When creating a task report fails, it no longer calls the Open Report API, even if we switch to another report and select it again.

Without remove:

  {
        onyxMethod: Onyx.METHOD.SET,
        key: `${ONYXKEYS.COLLECTION.REPORT}${optimisticTaskReport.reportID}`,
        value: null,
    },

https://github.com/Expensify/App/assets/39086067/ab0fbf85-1a27-490e-a5f3-f86e37db7d8e

[isabelastisser]

Hey @parasharrajat @namhihi237 @thienlnam , what are the next steps here?

[parasharrajat]

I will check the issue status tomorrow and share the next step. It seems we got stalled due to discussion.

[melvin-bot[bot]]

@parasharrajat, @thienlnam, @isabelastisser, @namhihi237 Whoops! This issue is 2 days overdue. Let's get this updated quick!