[$500] IOU - IOU preview becomes more grayed out when creating another request to a pending request

[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!

---

Version Number: v1.4.24-7 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 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

Action Performed:

1. Go offline.
2. Create a manual request with user with no prior chat.
3. Go to the main chat.
4. Create another manual request.

Expected Result:

When creating another request, the IOU preview retains the same level of grayness.

Actual Result:

When creating another request to a pending request in offline mode, the IOU preview becomes more grayed out.

Workaround:

Unknown

Platforms:

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

• [x] Android: Native
• [x] Android: mWeb Chrome
• [x] iOS: Native
• [x] iOS: mWeb Safari
• [x] MacOS: Chrome / Safari
• [x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/93399543/aaec4d18-8414-4e4d-9f5e-7c9a63be6b32

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01e1dc43fea738c267
• Upwork Job ID: 1745777854328131584
• Last Price Increase: 2024-01-12
• Automatic offers:
• cubuspl42 | Reviewer | 28117033
• Tony-MK | Contributor | 28117034

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~01e1dc43fea738c267

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[dukenv0307]

Proposal

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

When creating another request to a pending request in offline mode, the IOU preview becomes more grayed out.

What is the root cause of that problem?

We have two OfflineWithFeedback when we display ReportPreview. One in ReportActionItem which displays grey out for pendingAction of props.action, one in ReportPreview which display gray out for pendingAction of iouReport.pendingFields.preview.

https://github.com/Expensify/App/blob/a3088d5c5aa5ddc6713f9103a3589ef1f4500d2a/src/pages/home/report/ReportActionItem.js#L694

https://github.com/Expensify/App/blob/a3088d5c5aa5ddc6713f9103a3589ef1f4500d2a/src/components/ReportActionItem/ReportPreview.js#L261

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

In ReportActionItem we should pass shouldDisableOpacity as true into OfflineWithFeedback if report action is report preview. And then in ReportPreview we will update the pendingAction pass into OfflineWithFeedback

pendingAction={props.action.pendingAction || (props.action.isOptimisticAction ? CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD : '') || lodashGet(props, 'iouReport.pendingFields.preview')}

https://github.com/Expensify/App/blob/a3088d5c5aa5ddc6713f9103a3589ef1f4500d2a/src/pages/home/report/ReportActionItem.js#L694

What alternative solutions did you explore? (Optional)

Or we can create a util to get the pendingAction of report action. And then remove OfflineWithFeedback in ReportPreview and update pendingAction pass to OfflineWithFeedback in ReportActionItem

pendingAction={
    !_.isUndefined(props.draftMessage) ? null : props.action.pendingAction || (props.action.isOptimisticAction ? CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD : '') || lodashGet(props, 'iouReport.pendingFields.preview') 
}

[Tony-MK]

Proposal

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

IOU preview becomes more grayed out when creating another request to a pending request

What is the root cause of that problem?

When the first money request is created offline, the bug does not occur because the ReportPreview component only dims when iouReport.pendingFields.preview is present.

While the props.action.pendingAction will be set to "add".

https://github.com/Expensify/App/blob/9533df38782001586ae1d7e6b4c513984fdf94de/src/pages/home/report/ReportActionItem.js#L727-L729

The ReportActionItem sets its OfflineWithFeedback component pendingAction prop to props.action.pendingAction.

However, if props.action.pendingAction is not present and if props.action.isOptimisticAction is true then the pendingAction prop to will be set to CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD.

After another money requests are created offline, the iouReport.pendingFields.creatChat is unchanged, and iouReport.pendingFields.preview is added as "update".

The iouReport.pendingFields will look like this.

Therefore, the root cause of the problem is the ReportPreview component sets its OfflineWithFeedback component to dimm, even if the component ReportActionItem's OfflineWithFeedback component is already dimming.

Hence, the ReportPreview component will be double-dimmed.

This is why the ReportPreview's pendingAction prop uses iouReport.pendingFields.preview rather than iouReport.pendingFields.createChat.

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

We can not disable dimming for ReportActionItem because when isWhisper is true, it will not dim its child components such as DisplayNames.

This is because the ReportPreview component is the only component doing the dimming when the props.action.actionName is equal to REPORTPREVIEW and not ReportActionItem.

Due to the previous fact and the only component that uses ReportPreview is the ReportActionItem component, I suggest we only do minor changes to the ReportPreview component.

Hence, we should enable the shouldDisableOpacity prop to the OfflineWithFeedback child component of ReportPreview using a condition that will only be true if the props.action has a pendingAction or isOptimisticAction.

https://github.com/Expensify/App/blob/fd45dea98390e74e7953a764a56c32c4da541a68/src/components/ReportActionItem/ReportPreview.js#L261

Therefore, the props for the OfflineWithFeedback in ReportPreview code snippet above should look like the code snippet below.

<OfflineWithFeedback 
      pendingAction={lodashGet(props, 'iouReport.pendingFields.preview')}> // No Change
      shouldDisableOpacity={props.action.pendingAction || props.action.isOptimisticAction} // Only change
>

Result

[sonialiap]

@cubuspl42 what do you think of the above proposals?

[cubuspl42]

Both proposals agree that the right place to fix this issue is adjusting the properties passed OfflineWithFeedback (within ReportPreview): pendingAction and shouldDisableOpacity.

@Tony-MK Is there an observable difference when using the logic you suggest as compared to the proposal by @dukenv0307? Can we observe different dimming behavior?

[Tony-MK]

Thank you for the review, @cubuspl42.

My prospal aims to fix the fact that the part below won't be dimmed if we disable the opacity of the ReportActionItem component if it is a ReportPreview.

https://github.com/Expensify/App/blob/9533df38782001586ae1d7e6b4c513984fdf94de/src/pages/home/report/ReportActionItem.js#L736-L758

Therefore, the only the fix it is by using shouldDisableOpacity only at the ReportPreview component, not at the ReportActionItem component.

Both proposals agree that the right place to fix this issue is by adjusting the properties passed OfflineWithFeedback (within ReportPreview): pendingAction and shouldDisableOpacity.

My proposal only suggests modifying the shouldDisableOpacity for the ReportPreview component.

Unlike the other proposal which suggests changing the shouldDisableOpacity for the ReportActionItem and pendingAction for the ReportPreview.

In conclusion, that is the main visual difference noticed when testing.

What do you think? 🤔

[cubuspl42]

I think I wasn't clear. By "observable" I meant observable by a user; you started explaining the code.

That's what I want to understand: whether the differences between our two proposals are purely technical (code-oriented), or is there an observable dimming difference.

What I ask can be explained with non-code terms only and (perfectly!) screenshots. Unless the answer is "the solutions are observable equivalent", of course.

[Tony-MK]

My apologies, @cubuspl42. I should have realized you wanted screenshots.

While testing scenarios, I noticed this difference in dimming behavior with the avatar and name components of the ReportActionItem.

This scenario can be reproduced if you created a chat or workspace online and then created a money request offline.

This dimming behavior is different from the current staging environment.

Staging

My Changes

Other Changes

[dukenv0307]

@cubuspl42 My alternative solution doesn't face the issue above.

Additionally, we should bring the pendingAction from ReportPreview to ReportActionItem to be consistent for this case:

1. Create a request in online
2. Go offline
3. Create another request Actually result: only report preview is grey out (this is inconsistency when we edit a message in offline).

Expected result: both display name and report preview should be grey out.

For my main solution, we can add shouldDisableOpacity={!!lodashGet(props, 'iouReport.pendingFields.preview')} in ReportActionItem

[cubuspl42]

C+ reviewed 🎀 👀 🎀

I approve the proposal by @Tony-MK.

It was the second in the timeline order, but it was closer to the proper solution and the proposal text has a clear structure with a visible effort to make it easily understandable.

[melvin-bot[bot]]

Triggered auto assignment to @srikarparsi, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[dukenv0307]

Create a request in online Go offline Create another request Actually result: only report preview is grey out (this is inconsistency when we edit a message in offline). Expected result: both display name and report preview should be grey out.

@cubuspl42 The proposal from @Tony-MK will make only report preview is grey out for this case. Actually, it's the behavior on staging now but I think we should update to make this the same when we edit a comment is offline.

[cubuspl42]

@dukenv0307 Thanks! We'll make sure to test all cases that we mentioned here.

Your proposal was also good. in this case, I decided to select based on the proposal structure and initial solution correctness (they first noticed that we need a conditional shouldDisableOpacity logic).

[dukenv0307]

Or we can create a util to get the pendingAction of report action. And then remove OfflineWithFeedback in ReportPreview and update pendingAction pass to OfflineWithFeedback in ReportActionItem

@cubuspl42 What do you think about my alternative solution, I will remove the OfflineWithFeedback in ReportPreview and add this into pendingAction of ReportActionItem. That will not use shouldDisableOpacity and also fixed the case that I mentioned above.

[cubuspl42]

Come on, you know how this works. I'm impressed by your motivation and I know you do a lot of good work on the Expensify project. I know your proposals are often selected, as even I have personally reviewed a few of your PRs recently. But there seems to be some tension every time your proposal is not selected.

I told you that we'll take care of the case you mentioned. Both proposals had some strengths and weaknesses, I concluded that the other one's ratio is better in this case. Let's move on.

[dukenv0307]

@cubuspl42 I'm sorry if that makes you uncomfortable

But there seems to be some tension every time your proposal is not selected.

When I work on Expensify, I always feel free with any decision if the other proposal is better. I will only state my personal opinion if I feel my proposal is better.

I agree that the wording and structure in my proposal are not good but the main idea is correct and I'm the first one who post this idea. Besides, my alternative solution will fix another case above that isn't fixed in the second proposal.

Here is my opinion.

Anyway, I'm happy with the final decision of the team.

[cubuspl42]

You have the right to your opinion, but the opinion that one's proposal is better is typically "a default" one. As I see it, it's necessary to show what C+ missed to argue for changing their decision (after it's already made), not just disagree. Disagreeing with non-selecting of one's proposal is also "a default".

I think this is on the edge of breaking the rules against excessive commenting if not already breaking it. It was just my feedback, let's move on at least in this one.

[melvin-bot[bot]]

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

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

[cubuspl42]

@Tony-MK Let me know when the PR is ready

[Tony-MK]

@cubuspl42, the PR is ready for review. Thank you.

[Tony-MK]

Hey everyone, I think @MelvinBot has forgotten about this issue 🤣 because PR was deployed to production two weeks ago.

Kindly, could we finish off the issue? 🤔

[srikarparsi]

Hey! Yeah, @cubuspl42 is there anything left to do with this issue or is this ready for payment?

[cubuspl42]

@srikarparsi I confirm, the PR was deployed 2 weeks ago. We're ready for payment 🙂

[srikarparsi]

Cool thanks! @sonialiap do you think you could help with paying @Tony-MK $500 for coding the PR and @cubuspl42 $500 for reviewing the PR whenever you get a chance?

[sonialiap]

Payment summary:

• @Tony-MK $500 for coding the PR - paid ✔️
• @cubuspl42 $500 for reviewing the PR - paid ✔️