[BONUS PAYOUT ONLY] [$1000] Chat - Redundant "Unexpected error" below the IOU" message after split from 2 devices

[kbecciv]

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. Log in with the same account in the main testing device and a secondary device
2. Disable the internet connection in the main testing device
3. In the secondary device Split a bill with 2 users, this should create a new group and 2 new 1:1 conversations
4. In the main testing device (should be offline) create the SAME split bill with the 2 users
5. Enable the internet connection in the main testing device. The split bill request will be sent and you will get an error in the main device only.
6. Duplicated chats are display in the LHN
7. Open any dup chat with red dot

Expected Result:

PR https://github.com/Expensify/App/pull/18328 says "We shouldn't need to use a generic error message for failure data for the request money / send money flow when creating a New Chat, just like split bill. Otherwise, it will just be redundant with the error sent by the API.". And step 8 of the PR says "Make sure there is no "Unexpected error" below the IOU" (second error message). In scenario above (steps) as chat is invalid then no need for second redundant message under IOU

Actual Result:

Second redundant "unexpected error" message under IOU is displayed. is redundant, he second error message under IOU is redundant as chat is invalid and will be deleted.

Workaround:

Unknown

Platforms:

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

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

Version Number: 1.3.13.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

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/93399543/32da3c48-f47e-4552-a251-2ba1afb49000

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0136c6f52250bef4a6
• Upwork Job ID: 1658967606698582016
• Last Price Increase: 2023-06-09

[melvin-bot[bot]]

Triggered auto assignment to @jliexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot[bot]]

Bug0 Triage Checklist (Main S/O)

• [ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• [ ] This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• [ ] This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• [ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[therealsujitk]

Proposal

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

The issue here is each ReportActionItem displays an error even if the report is invalid, in which case only one error is sufficient.

What is the root cause of that problem?

The problem is we are displaying the error without checking if the report is valid or not in the code below.

https://github.com/Expensify/App/blob/9b48b8e03f937fbff79188e335ae1a2db469422e/src/pages/home/report/ReportActionItem.js#L352-L357

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

To fix this, we simply check if the report is valid and use it to decide whether to display error messages or not.

const isReportValid = !lodashGet(this.props.report, 'errorFields.addWorkspaceRoom') && !lodashGet(this.props.report, 'errorFields.createChat');

<OfflineWithFeedback
    onClose={() => ReportActions.clearReportActionErrors(this.props.report.reportID, this.props.action)}
    pendingAction={this.props.draftMessage ? null : this.props.action.pendingAction}
    errors={this.props.action.errors}
    shouldShowErrorMessages={isReportValid}     // Added this line
    errorRowStyles={[styles.ml10, styles.mr2]}
    needsOffscreenAlphaCompositing={this.props.action.actionName === CONST.REPORT.ACTIONS.TYPE.IOU}
>...</OfflineWithFeedback>

What alternative solutions did you explore? (Optional)

• We can remove the error field in failureData if a new report is being created but failed and create a forceOpacity prop in the OfflineWithFeedback component to force the opacity effect.
• We can set the error field's timestamp to null and render the error message if it is not null.

[mountiny]

cc @yuwenmemon would you be bale to handle this one since it seems like related to your PR, thanks!

[jliexpensify]

Not overdue, waiting on input from @yuwenmemon

[jliexpensify]

Not overdue, waiting on Yuwen's input

[yuwenmemon]

@therealsujitk's proposal seems good to me.

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~0136c6f52250bef4a6

[melvin-bot[bot]]

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

[melvin-bot[bot]]

📣 @therealsujitk You have been assigned to this job by @yuwenmemon! Please apply to this job in Upwork 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]]

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

[melvin-bot[bot]]

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

[jliexpensify]

Invited @parasharrajat , @therealsujitk - can you share your Upworks profile?

[therealsujitk]

Sure, [redacted]

[therealsujitk]

@parasharrajat & @yuwenmemon, I'm not sure if you were notified of my mention in the PR since you aren't assigned to it yet. Can you kindly take a look at the suggested changes I've made in my PR and if I should go through with it. I'd like to get your opinion before the weekend so there's time for you to review the PR by monday.

[parasharrajat]

I will check this today.

[therealsujitk]

Just a reminder to check the comments in the PR before the weekend if possible 🙃, sorry to be a bother.

[yuwenmemon]

@therealsujitk If you take the PR out of the draft then it will automatically assign us. Just a note for the future!

[parasharrajat]

I tested this but I couldn't put my head around the problem completely and the solution. At this stage, I might be delaying this PR. I request to reassign to another C+. I am busy with other PRs. Hope that is fine.

@yuwenmemon

[s77rt]

Will take this one!

[melvin-bot[bot]]

📣 @s77rt You have been assigned to this job by @yuwenmemon! Please apply to this job in Upwork 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 📖

[yuwenmemon]

Let's start over again. @s77rt you made a great point in the original PR about fixing the failure data. @therealsujitk does this make sense to you?

[therealsujitk]

Personally it doesn't due to the reasons I've started here - https://github.com/Expensify/App/pull/19188#issuecomment-1555363608, I feel it's not the right way from a UI/UX perspective (I feel the component should be greyed out in this case, i.e. the alpha effect). Maybe we can get someone who did the UI to confirm?

However, It's still an option so if you really want it this way we can proceed. I did add it as an alternate solution to my proposal after all.

[s77rt]

@therealsujitk Thanks for the quick follow up. You make a good point regarding the opacity, we should keep that but without showing an error. This may requires changes to OfflineWithFeedback.

Another option is to not keep the report actions at all, i.e. if we make a request to create a chat report and a report action but this failed, then we can just show the chat with an error and not show the report action at all (update failureData to null).

I think we should look after option one as it's the closest to the current behaviour but we may check option two with the team if we think it's better.

[therealsujitk]

This may requires changes to OfflineWithFeedback.

Sure, this can be done too. I thought of that but realised using the existing report errors was a much easier solution (which is what I did in the closed PR, plus any relevant errors may be useful for debugging an issue that comes up in future, maybe an error that occurred different from the create error). Which solution do you think is better?

If we do decide to make changes to OfflineWithFeedback, any prop names in mind?

Another option is to not keep the report actions at all, i.e. if we make a request to create a chat report and a report action but this failed, then we can just show the chat with an error and not show the report action at all (update failureData to null).

This I'm not so sure about, there may be information that the user would want to refer to, for example if he/she wants to know the IOU amount that failed or the IOU description. Or if they typed a really really long message and we just erased it for them.

Let's confirm with @yuwenmemon before proceeding.

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[s77rt]

@therealsujitk

plus any relevant errors may be useful for debugging an issue that comes up in future

I don't think we should optimise for cases that may never exist. I doubt the errors will be useful in anyway so let's not create them.

If we do decide to make changes to OfflineWithFeedback, any prop names in mind?

Does not matter. This is a PR thing.

---

You can explore any other options you see fit and not necessary the outlined two above.

[melvin-bot[bot]]

@yuwenmemon @s77rt @jliexpensify this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[s77rt]

We had an accepted proposal but we are looking for a possible better alternative.

[therealsujitk]

@yuwenmemon waiting to hear your thoughts on this, currently the best way would be to modify OfflineWithFeedback with a forceError prop and remove the error from errorFields.

[s77rt]

Not overdue. Waiting for proposals...

[s77rt]

Not overdue. Still looking for proposals...

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[melvin-bot[bot]]

@yuwenmemon @s77rt @jliexpensify this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[s77rt]

Still looking for proposals...

[yuwenmemon]

Hmmm... @therealsujitk why can't we fix this by updating the optimistic/success/failure data for the request?

[therealsujitk]

@yuwenmemon we can, that's the alternative solution.

1. Remove the error field in failureData where it's not required to remove redundant errors.
2. Add a prop to OfflineWithFeedback to force the opacity effect because that's the right behaviour (Component should have the effect if the API request failed or hasn't succeeded yet).

My original proposal didn't remove the failureData and simply set shouldShowErrorMessages to false which gave the same result but was a much easier solution.

[yuwenmemon]

@s77rt What do you think?

[s77rt]

@therealsujitk How would the new prop be used? i.e. when to pass the prop?

Another alternative solution, can we just update the errors field to something like an empty object? so technically we have an error but no errors message? This will trigger the opacity but show no message. (May not be the best idea but I think that would work too and it's not hard to work on)

[therealsujitk]

@therealsujitk How would the new prop be used? i.e. when to pass the prop?

I was thinking we could use a forceOpacity optional prop (default: false) and use that prop in the line below.

https://github.com/Expensify/App/blob/71017e42617bedefa04628bef5eb18d1b28df1fb/src/components/OfflineWithFeedback.js#L89-L89

const needsOpacity = (isOfflinePendingAction && !isUpdateOrDeleteError) || isAddError || props.forceOpacity;

This prop would be set to true if the report has a errorFields.createChat.

Another alternative solution, can we just update the errors field to something like an empty object? so technically we have an error but no errors message? This will trigger the opacity but show no message. (May not be the best idea but I think that would work too and it's not hard to work on)

This wouldn't work without modifying OfflineWithFeedback again because we check if error is empty and not undefined or null.

https://github.com/Expensify/App/blob/71017e42617bedefa04628bef5eb18d1b28df1fb/src/components/OfflineWithFeedback.js#L85

[therealsujitk]

@s77rt btw, what is the downside to keeping the error field?

[s77rt]

@therealsujitk That approach is equally complex as the first one. This works it's okay but I would really prefer to avoid that as it would lead to two control points (set the error, decide if the error should be shown). I'm looking to do this implicitly by just controlling one thing and that is the errors field. Can we just set the error message to an empty string?

• errors: null No error message and no opacity reduction
• errors: [timestamp: ''] No error message but apply opacity reduction (We are looking for this)
• errors: [timestemp: 'message'] Error message and apply opacity reduction

@yuwenmemon Thoughts here?

[therealsujitk]

Can we just set the error message to an empty string?

This would also require some modifications to OfflineWithFeedback, otherwise it'll just show a red dot and a close button with no message.

[s77rt]

@therealsujitk Sure but it won't be much changes in OfflineWithFeedback we will just check if the error message is not empty not to render DotIndicatorMessage, etc. Do you think this is a better approach? if so, can you please update your proposal.

[therealsujitk]

@s77rt I've added it to my proposal, to check for a valid error message something like this will have to be done.

_.empty(Object.values(props.action.error || {}).filter(e => e != null))

It'll also be safer to pass a filtered object to DotMessageIndicator. All options will require quite a few changes.

[s77rt]

I think we can work on that in the PR.

Let's wait for @yuwenmemon thoughts on https://github.com/Expensify/App/issues/18839#issuecomment-1577306110

[therealsujitk]

Summary

Method 1

• Add all the missing error fields in failureData #19188 (comment).
• Set OfflineWithFeedback's shouldShowErrorMessages prop to false in ReportItem if lodashGet(this.props.report, 'errorFields.createChat') exists.

Method 2

• Remove error fields in failureData where it could be redundant.
• Modify OfflineWithFeedback to take a forceOpacity optional prop which will be set to true in ReportItem if lodashGet(this.props.report, 'errorFields.createChat') exists.

  const needsOpacity = (isOfflinePendingAction && !isUpdateOrDeleteError) || isAddError || props.forceOpacity;

Method 3

• Set the error fields in failureData to { [timestamp]: null } to set an error with no message.
• Modify OfflineWithFeedback to show only valid error messages.

  const errors = _.omit(props.errors, v => v !== null);
  const hasErrorMessages = !_.isEmpty(errors);

[jliexpensify]

Not overdue! Waiting on @yuwenmemon

[s77rt]

While waiting for @yuwenmemon

@therealsujitk I have a question regarding method 3, can we just do:

const hasErrorMessages = hasErrors && _.some(props.errors);

[therealsujitk]

@s77rt yes this would work, I've never used the underscore library before so I wasn't aware of this. The reason I did it that way is because we need to pass a filtered object (object with no null values) down to DotIndicatorMessage. I've updated the summary to use a function from underscore which makes it look a lot cleaner.