Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.03k stars 2.54k forks source link

[HOLD for payment 2024-04-09] [$500] [MEDIUM] IOU - Money request page displays skeleton in online #26424

Closed lanitochka17 closed 1 month ago

lanitochka17 commented 9 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!


Action Performed:

  1. Launch app
  2. Tap profile icon
  3. Tap Preferences
  4. Toggle on force offline
  5. Navigate back to LHN
  6. Tap IOU report for request money received
  7. Note the IOU report page keeps loading
  8. Tap Pay elsewhere
  9. Navigate back to LHN and again visit IOU to note page keeps loading
  10. Navigate to LHN
  11. Tap profile
  12. Tap preferences
  13. Toggle off force offline
  14. Navigate back to LHN
  15. Tap IOU report

Expected Result:

Output of @mountiny & I discussing this one here for reference.

Actual Result:

In online, in IOU report for request money, the page keeps on loading

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.60-1

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/78819774/b3257238-8c59-4e60-a84b-8dce2d6bc3cf

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/~012efb1896a46c34c4
  • Upwork Job ID: 1707346943173816320
  • Last Price Increase: 2024-02-13
  • Automatic offers:
    • abdulrahuman5196 | Reviewer | 0
    • dukenv0307 | Contributor | 0
Issue OwnerCurrent Issue Owner: @trjExpensify
dukenv0307 commented 6 months ago

Proposal

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

In online, in IOU report for request money, the page keeps on loading

What is the root cause of that problem?

This is a feature request, I understand that we're trying to achieve 3 things below:

  1. We have the report total loaded in the iouReport data, so we should show the report total in the report where possible when offline.
  2. The iouActions we can't load would still show with a skeleton UI as we do today (we agreed just the one request preview component appearing as a skeleton is fine).
  3. When a new request is added to the request when offline and appears at 50% opacity in the pending create state, so should the report total in the expanded header area.

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

  1. Since MoneyReportView is rendered base on CREATED action, we can generate a CREATE action if the report is IOU report and the created actionn doesn't exist in ReportActionList

https://github.com/Expensify/App/blob/9b71f2391c04a00e3a9a69904f1a1f02517494f6/src/pages/home/report/ReportActionItem.js#L622

  1. This is existing behavior
  2. Fix here so that it will additionally check if there's any pending money request, if yes there'll be pendingAction there as well and it will display at 50% opacity
  3. If we only want to display report total at 50% opacity, in MoneyReportView we can check if at least once transaction is pending we will set opacity for report total to 50%.

What alternative solutions did you explore? (Optional)

For 1, an alternative is if there's no CREATED report action, we can append it ourselves from the front-end, and later when we open the iou report and the real CREATED report action comes, we can replace the appended one. But fixing from the back-end to additionally return CREATED report action is more straight-forward and clean.

  1. In ReportActionsView,
    • we can create a new variable to add CREATED action if the report is MoneyRequestReport and the last report action isn't the created action.
const reportActionsToDisplay = useMemo(() => {
    if (!ReportUtils.isMoneyRequestReport(props.report) || !_.size(props.reportActions) || !ReportActionsUtils.isCreatedAction(_.last(props.reportActions))) {
        return props.reportActions;
    }

    const optimisticCreatedAction = ReportUtils.buildOptimisticCreatedReportAction(report.ownerAccountID, DateUtils.subtractMillisecondsFromDateTime(_.last(props.reportActions).created, 1));
    return [...props.reportActions, optimisticCreatedAction];

}, [props.reportActions, props.report])

https://github.com/Expensify/App/blob/9bf0e9f3c1a13450b74f156cc18325cad492112d/src/pages/home/report/ReportActionsList.js#L128

OPTIONAL: when generate the created action, we can add a flag to know this action is optimistic or not

  1. Pass reportActionsToDisplay as sortedReportActions prop here

https://github.com/Expensify/App/blob/9bf0e9f3c1a13450b74f156cc18325cad492112d/src/pages/home/report/ReportActionsView.js#L259

trjExpensify commented 6 months ago

Proposals to review :)

abdulrahuman5196 commented 6 months ago

Hi, Will review today just back from weekend

melvin-bot[bot] commented 6 months ago

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

trjExpensify commented 6 months ago

@abdulrahuman5196 how's the review going?

abdulrahuman5196 commented 6 months ago

Hi, Will review again in my morning and update tomorrow

abdulrahuman5196 commented 6 months ago

Checking now

abdulrahuman5196 commented 6 months ago

@dukenv0307 Can you explain more on the first point? I am unable to understand this. What is this has to do with showing total from the IOU report data? And do double check the code pointer?

In OpenApp/ReconnectApp, we need to return the CREATED report action of the iou report as well, currently we only return 1 money request report action initially. Once we fix to add CREATED report action in the back-end, then the report total will show fine, according to this logic

dukenv0307 commented 6 months ago

@abdulrahuman5196 The total is displayed in MoneyRequestView which is the created action of the IOU report. So in OpenApp API, we should return the CREATED action of the report to make this can be displayed when we force offline before we go to the IOU report. If not, skeleton will display when we go to this report in offline mode.

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

mvtglobally commented 6 months ago

Issue not reproducible during KI retests. (First week)

melvin-bot[bot] commented 6 months ago

@trjExpensify, @abdulrahuman5196 Whoops! This issue is 2 days overdue. Let's get this updated quick!

abdulrahuman5196 commented 6 months ago

Issue not reproducible during KI retests. (First week)

I highly doubt this, It is not reproducible? Was it fixed somewhere?

dukenv0307 commented 6 months ago

The current bug is still reproducible.

trjExpensify commented 6 months ago

I agree. How are we getting on with the proposal review @abdulrahuman5196?

melvin-bot[bot] commented 6 months ago

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

abdulrahuman5196 commented 6 months ago

I still need to test the first point from the explanation. Will update here once checked.

abdulrahuman5196 commented 6 months ago

Reviewing again

abdulrahuman5196 commented 6 months ago

@dukenv0307

The total is displayed in MoneyRequestView which is the created action of the IOU report. So in OpenApp API, we should return the CREATED action of the report to make this can be displayed when we force offline before we go to the IOU report. If not, skeleton will display when we go to this report in offline mode.

Got it on what you are mentioning. Currently we use the 'CREATED' type of reportAction to show the Total header, but that is not loaded if we go to the money request page on offline case.

But the concern from OP was

We have the report total loaded in the iouReport data, 
so we should show the report total in the report where possible when offline.

So, we should check if its possible to show the total amount header with Total data used from other places(like iouReport data pointed in OP) apart from the CREATED type of reportAction.

abdulrahuman5196 commented 6 months ago

We don't have proposal to approve. Requested existing proposal to see if we can have any other way as suggested here - https://github.com/Expensify/App/issues/26424#issuecomment-1825535407

dukenv0307 commented 6 months ago

@abdulrahuman5196 What is the problem with this case? I don't see the problem in this comment https://github.com/Expensify/App/issues/26424#issuecomment-1825535407 ? Can you explain in more detail on that?

abdulrahuman5196 commented 6 months ago

@abdulrahuman5196 What is the problem with this case? I don't see the problem in this comment #26424 (comment) ? Can you explain in more detail on that?

The question was to, Instead of making BE change to return ReportAction of the 'CREATED' type on OpenApp, can we use other existing data source and display the header? As pointed in the OP. @dukenv0307

dukenv0307 commented 6 months ago

@abdulrahuman5196 This is the easiest way that we can do it, if not we should check all IOU requests in the IOU report to re-calculate the possible total and I think this is not optimal. Additionally, that will break the logic which is used to show the created action.

abdulrahuman5196 commented 6 months ago

@dukenv0307 No. You will already have the 'Total' data in the previous screens and I suppose report data will also be present. For example.

Screenshot 2023-11-27 at 4 31 13 PM

And also for MoneyRequestView we only provide the report

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

melvin-bot[bot] commented 6 months ago

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

trjExpensify commented 6 months ago

What's the latest here? @dukenv0307 are you making updates to the proposal or something else?

dukenv0307 commented 6 months ago

Will update tomorrow.

mvtglobally commented 6 months ago

Issue not reproducible during KI retests. (Second week)

dukenv0307 commented 6 months ago

No. You will already have the 'Total' data in the previous screens and I suppose report data will also be present. For example.

@abdulrahuman5196 I know that it only uses report data but as I mentioned above, MoneyRequestView is rendered as the created action of IOU report so I think it should be fixed in back-end to return created action of IOU report instead of the fixing workaround in front-end.

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

cc @mountiny @trjExpensify what do you think about this?

mountiny commented 6 months ago

@dukenv0307 can you clarify what you mean exactly I see that we return created report action for expense/ iou reports

image
dukenv0307 commented 6 months ago

@mountiny To display total of iou report when we offline before open this report we should return created action in OpenApp API.

mountiny commented 6 months ago

the total is not stored in the created report action, maybe we talking about something else

dukenv0307 commented 6 months ago

@mountiny You're right but as I mentioned above it's displayed when ReportActionItem is created action. The same with TaskView, the data is stored in report.

mountiny commented 6 months ago

Sorry is there some summary of the current problem?

trjExpensify commented 6 months ago

Bump @dukenv0307 on @mountiny question!

dukenv0307 commented 6 months ago

@mountiny Yes the current problem here is for IOU report, MoneyRequestView is displayed as the created action of this report so we need to return this in OpenApp API.

mountiny commented 6 months ago

@dukenv0307 I see, we cannot really do that until report pagination is implemented so different solution should be looked into

melvin-bot[bot] commented 5 months ago

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

trjExpensify commented 5 months ago

@dukenv0307 let us know your thoughts on an alt!

dukenv0307 commented 5 months ago

@mountiny Since TaskView, MoneyRequestView, and MoneyReportView only depend on report data and shouldShowHorizontalRule, we can move this into ReportActionList before here. And in ReportActionItem we will not display these view if the action is created action.

https://github.com/Expensify/App/blob/bbe73efdfab056d5ab6d864941fc3da44ec1c084/src/pages/home/report/ReportActionsList.js#L453

mountiny commented 5 months ago

Lets make a formal proposal and let @abdulrahuman5196 review it

dukenv0307 commented 5 months ago

@abdulrahuman5196 Updated propsal with another solution for first point. Please help to review again.

mvtglobally commented 5 months ago

Issue not reproducible during KI retests. (third week)

trjExpensify commented 5 months ago

@abdulrahuman5196 can you review this please? https://github.com/Expensify/App/issues/26424#issuecomment-1842864977

melvin-bot[bot] commented 5 months ago

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

abdulrahuman5196 commented 5 months ago

@dukenv0307 The updated point 1 in the proposal will cause a larger code refractor right? Or is it a considerable change?

Could you provide some high level information on the changes required as per the updated proposal?

dukenv0307 commented 5 months ago

@abdulrahuman5196 In the second thought, I think it's impossible to bring MoneyReportView component to the high level because although this component only depends on report data, the wrap component of this depends on reportAction and move MoneyReportView to the higher component will make the height of list report action is less than.

https://github.com/Expensify/App/blob/9b71f2391c04a00e3a9a69904f1a1f02517494f6/src/pages/home/report/ReportActionItem.js#L621-L625

abdulrahuman5196 commented 5 months ago

Got it. Let me also give a check on current code base to check the feasibility.

melvin-bot[bot] commented 5 months ago

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

abdulrahuman5196 commented 5 months ago

Will check and update on this today https://github.com/Expensify/App/issues/26424#issuecomment-1855793632

abdulrahuman5196 commented 5 months ago

Reviewing now