Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
3 of 6 tasks
lanitochka17 opened this issue Aug 31, 2023 · 173 comments
Closed
3 of 6 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@lanitochka17
Copy link

lanitochka17 commented Aug 31, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] 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.

  • We have the report total loaded in the iouReport data, so we should show the report total in the report where possible when offline.
    -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).
    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.

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?

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

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

Bug6184136_loading_redmi_secondaey_receivrr.mp4

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
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 31, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 31, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Aug 31, 2023

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

@trjExpensify
Copy link
Contributor

When I come back online, they eventually load and the skeleton UI goes away, so that's contrary to the bug report. When we can't load the content, the skeleton UI is the correct pattern to use.

RPReplay_Final1693568080.MP4

I'm curious why the report total extended header in the iouReport isn't available to view offline in the first place though, and whether we can improve that in the spirit of offline-first?

CC: @mountiny @luacmartins @JmillsExpensify

@luacmartins
Copy link
Contributor

It seems like a regression. I agree they should be working offline.

@mountiny
Copy link
Contributor

mountiny commented Sep 1, 2023

I'm curious why the report total extended header in the iouReport isn't available to view offline in the first place though, and whether we can improve that in the spirit of offline-first?

Yep I think in offline it can work like that, online we probably still need to show the skeleton

@melvin-bot melvin-bot bot added the Overdue label Sep 4, 2023
@trjExpensify
Copy link
Contributor

Can you expand on that, @mountiny? Why do we need to show the skeleton for the report total when online?

@melvin-bot melvin-bot bot removed the Overdue label Sep 4, 2023
@mountiny
Copy link
Contributor

mountiny commented Sep 4, 2023

No we dont have to, sorry we were probably talking about different things

@trjExpensify
Copy link
Contributor

Cool, so we agree with the below? Then next question, is this going to need backend changes?

Expected results
The report total in the expanded header area should be available to view when offline.

Actual Results
The report total doesn't load, and shows a skeleton UI in its place when offline.

@mountiny
Copy link
Contributor

mountiny commented Sep 4, 2023

The report total in the expanded header area should be available to view when offline.

What if you have exisitng IOU report and you sign out sign back in, you dont go to the IOU report so you dont have the report actions but you have the total.

You are offline, You request more, the total is updated and you see that offline made request but you dont see the previosu messages and there is no indication the report has more report actions while total is more. Is that fine as a feedback to the user while offline?

@trjExpensify
Copy link
Contributor

You might have to type that out again in steps for me to follow it completely sorry man. Let's park this and focus on the deploy for a sec.

@melvin-bot melvin-bot bot added the Overdue label Sep 11, 2023
@trjExpensify
Copy link
Contributor

Okaaaay. @mountiny, circling back here. Mind rephrasing? 😄

@melvin-bot melvin-bot bot removed the Overdue label Sep 11, 2023
@mountiny
Copy link
Contributor

  1. You fresh sign in
  2. you have the IOU report data and its total ($50) locally with user B
  3. You go offline
  4. you request more money $10 from the user B
  5. We wont show a skeleton, one report action done optimistically with $10 and total of $60

If there is no skeleton, then it might be confusing to the user as there is only one report action and they cannot see the others

@trjExpensify
Copy link
Contributor

Why wouldn't it just work like this as it does now? 😕

yfMvCuNr58.mp4

Interestingly, the report total updates as well. Perhaps that should be at 50% opacity as well though, as it's pending the update? CC: @JmillsExpensify @shawnborton

@shawnborton
Copy link
Contributor

Yeah, I think that makes sense to me.

@mountiny
Copy link
Contributor

@trjExpensify In this case you have visited the report before you went offline so you had the £50 report action. You could not have that if you would sig in and not open the report and went offline and then go to the report.

Its edge case but thats what we trying to cover here

@trjExpensify
Copy link
Contributor

I don't think I did visit the iouReport, no. Here's another one:

ofdi8s8pOz.mp4

@mountiny
Copy link
Contributor

@trjExpensify yeah but you just created it locally so the request is there. You would have to create the request and report, then sign out and sign back in, request from global create from the same person, the historical report actions will be missing I believe because they do not load during openApp

@melvin-bot melvin-bot bot added the Overdue label Sep 13, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 14, 2023

@trjExpensify this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

@melvin-bot
Copy link

melvin-bot bot commented Sep 15, 2023

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

@trjExpensify
Copy link
Contributor

Going to chat with Vit about this on Monday.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Sep 15, 2023
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Feb 16, 2024
@dukenv0307
Copy link
Contributor

@abdulrahuman5196 The PR is ready for review.

@aldo-expensify
Copy link
Contributor

Reassigning to @mountiny since he has a lot more context of this

@greg-schroeder greg-schroeder changed the title [$500] IOU - Money request page displays skeleton in online [$500] [MEDIUM] IOU - Money request page displays skeleton in online Feb 20, 2024
@trjExpensify
Copy link
Contributor

PR in active review.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Apr 2, 2024
@melvin-bot melvin-bot bot changed the title [$500] [MEDIUM] IOU - Money request page displays skeleton in online [HOLD for payment 2024-04-09] [$500] [MEDIUM] IOU - Money request page displays skeleton in online Apr 2, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Apr 2, 2024
Copy link

melvin-bot bot commented Apr 2, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Apr 2, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.58-8 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-04-09. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Apr 2, 2024

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@abdulrahuman5196 / @dukenv0307] The PR that introduced the bug has been identified. Link to the PR:
  • [@abdulrahuman5196 / @dukenv0307] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@abdulrahuman5196 / @dukenv0307] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@abdulrahuman5196 / @dukenv0307] Determine if we should create a regression test for this bug.
  • [@abdulrahuman5196 / @dukenv0307] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@trjExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@mountiny
Copy link
Contributor

mountiny commented Apr 2, 2024

$500 to @dukenv0307 and to @abdulrahuman5196

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Apr 8, 2024
@trjExpensify
Copy link
Contributor

👋 checklist time please, @abdulrahuman5196!

In the meantime, payment summary will be as follows:

@abdulrahuman5196
Copy link
Contributor

The PR that introduced the bug has been identified. Link to the PR:
The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

Not a regression.

Determine if we should create a regression test for this bug.

Yes.

If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

  1. Create some money requests in a chat
  2. Open the IOU report and send some messages
  3. Open another report and send a message
  4. Logout and login again with the same account
  5. Without opening any other report, go offline
  6. Open IOU report that we created above
  7. Verify that the total is displayed and at least one money request appears

@trjExpensify Added checklist. And I am still getting paid via upwork. Kindly process on the same.

@melvin-bot melvin-bot bot added the Overdue label Apr 12, 2024
Copy link

melvin-bot bot commented Apr 15, 2024

@trjExpensify, @mountiny, @abdulrahuman5196, @dukenv0307 Huh... This is 4 days overdue. Who can take care of this?

@trjExpensify
Copy link
Contributor

Paid you both! Closing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor
Projects
No open projects
Development

No branches or pull requests

9 participants