[HOLD for payment 2024-10-11] [$150] Expense - Combined report is more grayed out than the transaction thread

lanitochka17 commented 3 months ago

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: 9.0.12-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause - Internal Team

Action Performed:

Go to staging.new.expensify.com
Go to DM with user that has no unsettled expense
Go offline
Submit an expense
Click on the expense preview to go to expense report (combined report)
Take note of the grayness
Submit another expense
Click on the expense preview to go to transaction thread
Note the difference in report grayness between Step 6 and 8

Expected Result:

The grayness of the combined report and the transaction thread should be the same

Actual Result:

The combined report is more grayed out than the transaction thread

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/user-attachments/assets/ff11dea1-2e7e-43ee-8433-adf0ea8e2b72

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~010079feefd518ffb7
Upwork Job ID: 1821119086877872721
Last Price Increase: 2024-09-23
Automatic offers:

dukenv0307 | Reviewer | 104088606
nkdengineer | Contributor | 104088608

Issue Owner

Current Issue Owner: @dylanexpensify

melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @dylanexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

Krishna2323 commented 3 months ago

Edited by proposal-police: This proposal was edited at 2024-08-15 11:53:52 UTC.

Proposal

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

When we use MoneyRequestView inside ReportActionItemContentCreated, we are wrapping it with OfflineWithFeedback but the content inside MoneyRequestView already covers the state for pending action for each field expect ReceiptEmptyState

https://github.com/Expensify/App/blob/4adf0c540c31cebeb178df1f0f857a750b871eb6/src/pages/home/report/ReportActionItemContentCreated.tsx#L148-L179

Expense - Combined report is more grayed out than the transaction thread
What is the root cause of that problem?

ReceiptEmptyState isn't wrapped with OfflineWithFeedback. https://github.com/Expensify/App/blob/4adf0c540c31cebeb178df1f0f857a750b871eb6/src/components/ReportActionItem/MoneyRequestView.tsx#L473-L489

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

Wrap ReceiptEmptyState with OfflineWithFeedback like we do for ReportActionItemImage.
Do not wrap MoneyRequestView inside ReportActionItemContentCreated with OfflineWithFeedback.

We can also wrap ReceiptAudit with OfflineWithFeedback.

        <View>
            {transactionThreadReport && !isEmptyObject(transactionThreadReport) ? (
                <>
                    <OfflineWithFeedback pendingAction={action.pendingAction}>
                        <MoneyReportView
                            report={report}
                            policy={policy}
                            isCombinedReport
                            shouldShowTotal={transactionCurrency !== report.currency}
                            shouldHideThreadDividerLine={shouldHideThreadDividerLine}
                        />
                    </OfflineWithFeedback>
                    <ShowContextMenuContext.Provider value={contextValue}>
                        <View>
                            <MoneyRequestView
                                report={transactionThreadReport}
                                shouldShowAnimatedBackground={false}
                            />
                            {renderThreadDivider}
                        </View>
                    </ShowContextMenuContext.Provider>
                </>
            ) : (
                <OfflineWithFeedback pendingAction={action.pendingAction}>
                    <MoneyReportView
                        report={report}
                        policy={policy}
                        shouldHideThreadDividerLine={shouldHideThreadDividerLine}
                    />
                </OfflineWithFeedback>
            )}
        </View>

What alternative solutions did you explore? (Optional)

We can wrap MoneyRequestView with OfflineWithFeedback, pending action will be pendingAction={action.pendingAction} https://github.com/Expensify/App/blob/c880993cb8b40fc3f7e0daf0014cc04fca508be1/src/pages/home/report/ReportActionItemContentCreated.tsx#L108-L114
As suggested by dukenv0307, we can return undefined when pendingAction present. https://github.com/Expensify/App/blob/c880993cb8b40fc3f7e0daf0014cc04fca508be1/src/components/ReportActionItem/MoneyRequestView.tsx#L299-L300

const getPendingFieldAction = (fieldPath: TransactionPendingFieldsKey) => pendingAction ? undefined : transaction?.pendingFields?.[fieldPath]

We need to remove the pending action from receipt's OfflineWithFeedback,. https://github.com/Expensify/App/blob/c880993cb8b40fc3f7e0daf0014cc04fca508be1/src/components/ReportActionItem/MoneyRequestView.tsx#L458

Result

https://github.com/user-attachments/assets/38bde864-c229-48fa-a5e5-ab7d033631f7

nkdengineer commented 3 months ago

Edited by proposal-police: This proposal was edited at 2024-08-07 11:17:24 UTC.

Proposal

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

The combined report is more grayed out than the transaction thread

What is the root cause of that problem?

In the combined report, we wrap the MoneyRequestView with an OfflineWithFeedback. So all item in MoneyRequestView is grayed out.

https://github.com/Expensify/App/blob/4adf0c540c31cebeb178df1f0f857a750b871eb6/src/pages/home/report/ReportActionItemContentCreated.tsx#L150

But in the transaction thread report, we don't do it here

https://github.com/Expensify/App/blob/4adf0c540c31cebeb178df1f0f857a750b871eb6/src/pages/home/report/ReportActionItemContentCreated.tsx#L106-L108

And in MoneyRequestView both Receipt labels and Receipt empty component isn't grayed out

https://github.com/Expensify/App/blob/4adf0c540c31cebeb178df1f0f857a750b871eb6/src/components/ReportActionItem/MoneyRequestView.tsx#L422

https://github.com/Expensify/App/blob/4adf0c540c31cebeb178df1f0f857a750b871eb6/src/components/ReportActionItem/MoneyRequestView.tsx#L474

https://github.com/Expensify/App/blob/d43f6e1817dcc72634b0768f8aa20b4b0f1a0021/src/components/ReportActionItem/MoneyRequestView.tsx#L457

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

We should wrap the component here with OfflineWithFeedback and pass pendingAction prop as action.pendingAction

https://github.com/Expensify/App/blob/4adf0c540c31cebeb178df1f0f857a750b871eb6/src/pages/home/report/ReportActionItemContentCreated.tsx#L106-L108

We also need to return the pending field action here as undefined if the pendingAction is not undefined to avoid the duplicate pending action because if pendingAction exists, we already wrapped OfflineWithFeedback outside the MoneyRequestView

https://github.com/Expensify/App/blob/f9306d578f42140729646bbe94417266a0e1a3c4/src/components/ReportActionItem/MoneyRequestView.tsx#L291 OPTIONAL: We can also wrap the receipt label and receipt empty state with OfflineWithFeedback

https://github.com/Expensify/App/blob/4adf0c540c31cebeb178df1f0f857a750b871eb6/src/components/ReportActionItem/MoneyRequestView.tsx#L422

https://github.com/Expensify/App/blob/4adf0c540c31cebeb178df1f0f857a750b871eb6/src/components/ReportActionItem/MoneyRequestView.tsx#L474

We also need to update pendingFields for receipt when we replace the receipt here and delete the receipt here

and then also use getPendingFieldAction for receipt as well

getPendingFieldAction('receipt');

https://github.com/Expensify/App/blob/d43f6e1817dcc72634b0768f8aa20b4b0f1a0021/src/components/ReportActionItem/MoneyRequestView.tsx#L457

We also need to do the same way for MoneyReportView

What alternative solutions did you explore? (Optional)

NA

nyomanjyotisa commented 3 months ago

Proposal

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

Combined report is more grayed out than the transaction thread

What is the root cause of that problem?

When displaying the report we wrap it with OfflineWithFeedback https://github.com/Expensify/App/blob/4adf0c540c31cebeb178df1f0f857a750b871eb6/src/pages/home/report/ReportActionItemContentCreated.tsx#L148-L150

But when displaying it in transaction thread we don't wrap it with OfflineWithFeedback https://github.com/Expensify/App/blob/4adf0c540c31cebeb178df1f0f857a750b871eb6/src/pages/home/report/ReportActionItemContentCreated.tsx#L105-L115

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

wrap it with OfflineWithFeedback when displaying it in transaction thread to make it consistent

        return (
            <OfflineWithFeedback pendingAction={action.pendingAction}>
                <ShowContextMenuContext.Provider value={contextValue}>
                    <View>
                        <MoneyRequestView
                            report={report}
                            shouldShowAnimatedBackground
                        />
                        {renderThreadDivider}
                    </View>
                </ShowContextMenuContext.Provider>
            </OfflineWithFeedback>
        );

What alternative solutions did you explore? (Optional)

Krishna2323 commented 3 months ago

Proposal Updated

Updated RCA and solution

nkdengineer commented 3 months ago

Updated proposal https://github.com/Expensify/App/issues/46200#issuecomment-2250543281 to add alternative solution.

Krishna2323 commented 3 months ago

Proposal Updated

Minor update in solution section and added result video

melvin-bot[bot] commented 3 months ago

@dylanexpensify Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 3 months ago

@dylanexpensify 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

dylanexpensify commented 3 months ago

reviewing today!

melvin-bot[bot] commented 3 months ago

Job added to Upwork: https://www.upwork.com/jobs/~010079feefd518ffb7

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

Upwork job price has been updated to $125

dylanexpensify commented 3 months ago

External, but lowered price for ease of fix

nkdengineer commented 3 months ago

Updated proposal with another fix in the main solution.

dukenv0307 commented 3 months ago

Thanks for your proposals.

@Krishna2323 Your proposal will work, but it cause the inconsistency between other components like MoneyReportView. We use both OfflineWithFeedback outside and inside. The outside OfflineWithFeedback is used for create action, the inside one is used for update action.

@nyomanjyotisa Your solution is incorrect since it can cause the double greyed out problem

So I like the way to keep things consistent like @nkdengineer's solution, we should remove the fallbackPendingAction and wrap receipt label and receipt empty state with OfflineWithFeedback

Let's go with @nkdengineer's solution

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 3 months ago

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

dukenv0307 commented 3 months ago

@dylanexpensify BTW, The issue can be easy to fix, but finding a consistent solution that doesn't cause any regression is not too easy. Should we keep the initial price (250$) as usual? Thanks

Krishna2323 commented 3 months ago

Your proposal will work, but it cause the inconsistency between other components like MoneyReportView. We use both OfflineWithFeedback outside and inside. The outside OfflineWithFeedback is used for create action, the inside one is used for update action.

@dukenv0307, I don't think that's the case. For instance, we can look at the TaskView component, which is also not wrapped with OfflineWithFeedback and handles the pending state view inside the component. Also, if we wrap the MoneyRequestView with OfflineWithFeedback in ReportActionItemContentCreated, it will cause regression as it will gray out any field twice if created and edited offline. Could you please verify this too? 🙏🏻

https://github.com/user-attachments/assets/4aad5fa9-cc70-4640-93cd-3824301d29c7

https://github.com/Expensify/App/blob/4adf0c540c31cebeb178df1f0f857a750b871eb6/src/pages/home/report/ReportActionItemContentCreated.tsx#L137-L146

dukenv0307 commented 3 months ago

Thank you @Krishna2323. But can we just prioritize using create pending action.

 const getPendingFieldAction = (fieldPath: TransactionPendingFieldsKey) => pendingAction ? undefined : transaction?.pendingFields?.[fieldPath]

BTW, we use the same pattern in MoneyReportView, don't we face with this issue?

Krishna2323 commented 3 months ago

@dukenv0307, I will check MoneyReportView and update, I believe the same issue will occur in that component too.

The issue will not be solved by prioritizing create pending action.

dukenv0307 commented 3 months ago

@nkdengineer Do you want to update the proposal?

nkdengineer commented 3 months ago

Thank you @Krishna2323. But can we just prioritize using create pending action.

@dukenv0307 You're right. I tested when we edit a field of money request the pendingAction of the transaction doesn't change.

nkdengineer commented 3 months ago

Updated proposal

Krishna2323 commented 3 months ago

Thank you @Krishna2323. But can we just prioritize using create pending action.

@dukenv0307, sorry I misunderstood that. I was browsing through mobile and didn't see the code and thought you were suggesting prioritizing pendingAction like.

const getPendingFieldAction = (fieldPath: TransactionPendingFieldsKey) => pendingAction ?? transaction?.pendingFields?.[fieldPath]

The code wasn't fully visible so I misunderstood that. Screenshot_2024-08-08-14-25-36-34_40deb401b9ffe8e1df2f1cc5ba480b12

That will work but I still think we should do the same way we do for TaskView. MoneyRequestView can be used as a component also so I think we should not change its implementation.

https://github.com/Expensify/App/blob/53eee53ac8bfbdfd4d8eeaf16940e3b165483373/src/pages/TransactionDuplicate/Confirmation.tsx#L80-L87

dukenv0307 commented 3 months ago

Let's see what happens if we don't wrap MoneyRequestView inside OfflineWithFeedback

We need to wrap all components inside MoneyRequestView with OfflineWithFeedback
When someone adds a component like

<Text>{
abc
}</Text>

we need to wrap it in OfflineWithFeedback. I believe it's bad pattern since the simple text isn't related the OfflineWithFeedback

So I want to use the following pattern that we already use in the App.

Wrap the parent with OfflineWithFeedback -> In case we create offline, the whole page can be grayed
Wrap the necessary child components that can be updated on offline mode with OfflineWithFeedback -> In case we update offline, it can be grayed

dukenv0307 commented 3 months ago

We can wait for @MariaHCD's response

Krishna2323 commented 3 months ago

@dukenv0307, what do you think about wrapping every component inside MoneyRequestView with a common OfflineWithFeedback and only returning the pending action for a specific field when the parent pending action isn't there, like you suggested here?

melvin-bot[bot] commented 3 months ago

@MariaHCD @dylanexpensify @dukenv0307 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!

dukenv0307 commented 3 months ago

@MariaHCD Can you check my comment above? Thanks

melvin-bot[bot] commented 3 months ago

@MariaHCD, @dylanexpensify, @dukenv0307 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

dukenv0307 commented 3 months ago

@MariaHCD any updates?

MariaHCD commented 3 months ago

Apologies for the delay, reviewing this today

melvin-bot[bot] commented 3 months ago

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

Krishna2323 commented 3 months ago

@dukenv0307, while working on another issue, I found that the receipt's OfflineWithFeedback uses pendingAction (transaction?.pendingAction). This will introduce another regression if we create a request with the receipt offline, causing the receipt to be greyed out twice. Please see the video below.

I still believe that wrapping MoneyRequestView isn't a good idea and can cause regressions. We've already found two regressions, but if we still want to proceed with that solution, I have added alternative solution in my proposal to fix both regressions and wrap MoneyRequestView with OfflineWithFeedback.

https://github.com/Expensify/App/blob/c880993cb8b40fc3f7e0daf0014cc04fca508be1/src/components/ReportActionItem/MoneyRequestView.tsx#L458

https://github.com/user-attachments/assets/0297d96d-c8fd-43e2-91c2-73f341ad5df1

cc: @MariaHCD

MariaHCD commented 3 months ago

Thanks for the discussion, all! Just to clarify my understanding of where we are - so far @nkdengineer's proposal was selected which would wrap the MoneyRequestView component with OfflineWithFeedback

But this approach seems to cause regressions?

While @Krishna2323's updated proposal looks to fix these regressions as well as fix the original reported bug in this issue.

Is my understanding correct?

If so, @dukenv0307 what are the considerations between the two proposals here?

dukenv0307 commented 2 months ago

I'm reviewing again

dukenv0307 commented 2 months ago

@Krishna2323 Why do I face that issue when I already check the pendingAction from the parent?

pendingAction ? undefined : transaction?.pendingFields?.[fieldPath]

https://github.com/Expensify/App/issues/46200#issuecomment-2275177757

Krishna2323 commented 2 months ago

@dukenv0307, because the pending action is directly used for receipt. We don't use getPendingFieldAction in case of receipt because the receipt is only greyed out when IOU action is created offline.

https://github.com/Expensify/App/blob/c880993cb8b40fc3f7e0daf0014cc04fca508be1/src/components/ReportActionItem/MoneyRequestView.tsx#L458

nkdengineer commented 2 months ago

the receipt is only greyed out when IOU action is created offline.

I believe the receipt should be greyed out when we update it offline like the behavior when we update other fields of transaction.

https://github.com/Expensify/App/blob/c880993cb8b40fc3f7e0daf0014cc04fca508be1/src/components/ReportActionItem/MoneyRequestView.tsx#L458

We should update pendingFields for receipt when we replace the receipt in optimistic data, remove it in success data and failure data in replaceReceipt function and then use getPendingFieldAction for receipt here as well https://github.com/Expensify/App/blob/d43f6e1817dcc72634b0768f8aa20b4b0f1a0021/src/libs/actions/IOU.ts#L7441

Updated my alternative solution to include that

Krishna2323 commented 2 months ago

We should update pendingFields for receipt when we replace the receipt in optimistic data

I believe this isn't the original issue (maybe not an issue) we are trying to solve.

cc: @dukenv0307

dukenv0307 commented 2 months ago

@Krishna2323 We should find a solution to make things consistent with other places. If we just remove the pendingAction for Receipt, It can't be grayed out when users edit it.

@MariaHCD I want to confirm the expectation

Users edit receipt in offline mode, should we greyed out the new receipt?

Current behavior: It's not greyed out

Krishna2323 commented 2 months ago

We should find a solution to make things consistent with other places. If we just remove the pendingAction for Receipt, It can't be grayed out when users edit it.

I agree but that's how it works currently and I think thats completely a different issue from what we are trying to solve, isn't it?

The grayness of the combined report and the transaction thread should be the same

dukenv0307 commented 2 months ago

@Krishna2323 Thank you. Let's see @MariaHCD's response

melvin-bot[bot] commented 2 months ago

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

dukenv0307 commented 2 months ago

@MariaHCD friendly bump

melvin-bot[bot] commented 2 months ago

@MariaHCD @dylanexpensify @dukenv0307 this issue is now 4 weeks old, please consider:

Finding a contributor to fix the bug
Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

MariaHCD commented 2 months ago

Users edit receipt in offline mode, should we greyed out the new receipt? Current behavior: It's not greyed out

Hey @Expensify/design! We're looking to confirm whether the receipt should be greyed out when an expense is edited while offline.

dubielzyk-expensify commented 2 months ago

Good question. @shawnborton might be better positioned to answer, but to me what I see in this recording looks right where it's faded out given it's uploaded in offline mode.

shawnborton commented 2 months ago

Makes sense to me, I agree.

Expensify / App