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

[$500] [HOLD for payment 2023-09-27] Android - Infinite loading after opening personal details page #27560

Closed
1 of 6 tasks
kavimuru opened this issue Sep 15, 2023 · 34 comments
Closed
1 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

@kavimuru
Copy link

kavimuru commented Sep 15, 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. Click on your avatar to open settings
  2. Click on profile
  3. Click on personal details

Expected Result:

Personal details page should be opened.

Actual Result:

Can't open personal details page and infinite loading spinner

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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.70-5
Reproducible in staging?: y
Reproducible in production?: n
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

XRecorder_15092023_165615.mp4
az_recorder_20230915_151359.1.mp4

Expensify/Expensify Issue URL:
Issue reported by: @Ahmed-Abdella
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1694786719105809

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01dbd77bfba87b38dc
  • Upwork Job ID: 1707718640343793664
  • Last Price Increase: 2023-09-29
@kavimuru kavimuru added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 15, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 15, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 15, 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

@OSBotify
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@melvin-bot
Copy link

melvin-bot bot commented Sep 15, 2023

Triggered auto assignment to @Li357 (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@mkhutornyi
Copy link
Contributor

Proposal

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

Infinite loading when first open personal details page after updating personal details on another device

What is the root cause of that problem?

Clear repro step:

  1. Login any account on device A
  2. On device B, logout if already logged in
  3. On device B, login same account as in device A
  4. On device A, update any personal details. i.e. Date of birth
  5. On device B, go to Settings > Profile > Personal details page

Let's find the root cause from above repro step.
On first login on B, private_personalDetails has no data in Onyx.
But after updating dob in A, B receives pusher event and Onyx data is updated to {dob: 'xxxx-xx-xx}
PersonalDetailsInitialPage uses usePrivatePersonalDetails hook to get personal details data from api.
The issue is in that hook.

useEffect(() => {
if (isOffline || Boolean(PersonalDetails.getPrivatePersonalDetails())) {
return;
}
PersonalDetails.openPersonalDetailsPage();
}, [isOffline]);

{dob: 'xxxx-xx-xx} meets Boolean(PersonalDetails.getPrivatePersonalDetails()) so early returned.

So even though local data is not complete (not yet fetched from api), api is never called, so isLoading key never exists which makes this condition always true:

const isLoadingPersonalDetails = lodashGet(props.privatePersonalDetails, 'isLoading', true);

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

One api is called, isLoading value is added to private_personalDetails, so I think this key existence should be enough to check if personal details is fetched or not

if (isOffline || Boolean(PersonalDetails.getPrivatePersonalDetails())) {

So update this condition to:

const data = PersonalDetails.getPrivatePersonalDetails();
 if (isOffline || Boolean(data) && data.isLoading !== undefined) { 

What alternative solutions did you explore? (Optional)

We may use another key instead of isLoading

@Li357
Copy link
Contributor

Li357 commented Sep 18, 2023

Able to reproduce on Web as well via @mkhutornyi's repro steps. RCA is good and proposal looks good. Since this is a deploy blocker, unless we can raise a PR within the hour we might handle this internally.

@mkhutornyi
Copy link
Contributor

@Li357 I can raise PR right now

@thienlnam
Copy link
Contributor

@mkhutornyi Please raise a PR so we can resolve this deploy blocker

@thienlnam
Copy link
Contributor

Going to assign you since @Li357 gave an initial approval

@mkhutornyi
Copy link
Contributor

PR is ready for review

@CortneyOfstad
Copy link
Contributor

@Li357 ^^^ TIA!

@thienlnam
Copy link
Contributor

This has been merged / CPED

@Ahmed-Abdella
Copy link
Contributor

Ahmed-Abdella commented Sep 19, 2023

@thienlnam @CortneyOfstad Sorry, but why no reporting bonus paid here?

@thienlnam
Copy link
Contributor

Ah yeah this should be paid a reporting bonus

@melvin-bot
Copy link

melvin-bot bot commented Sep 29, 2023

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

@melvin-bot melvin-bot bot added Overdue Help Wanted Apply this label when an issue is open to proposals by contributors labels Sep 29, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 29, 2023

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

@CortneyOfstad CortneyOfstad removed Help Wanted Apply this label when an issue is open to proposals by contributors Overdue labels Sep 29, 2023
@CortneyOfstad
Copy link
Contributor

Sorry @eVoloshchak It shows that the external label was never applied to create the Upwork job. Removing your assignment as a C+ has already worked on this issue.

Sorry for any hassle/confusion!

@CortneyOfstad
Copy link
Contributor

@Li357 I just want to confirm here that @mkhutornyi should get both the Contributor and C+ payment since they both proposed the fix and drafted the PR? Also, they should get the bonus for being within 3 days, correct?

If so, that would be $500 for being the contributor + 50% bonus ($750); $500 for being C+ plus the 50% bonus ($750) — totaling $1,500. Does that appear to be correct?

@Ahmed-Abdella I have you down for the reporting bonus, so you are all set! Will be sending out the proposals in Upwork, once I have that above clarified 👍

@CortneyOfstad
Copy link
Contributor

@Li357 bump on the above ^^^^

@CortneyOfstad
Copy link
Contributor

@Li357 bump again

@Li357
Copy link
Contributor

Li357 commented Oct 6, 2023

Sorry about the delay! If I'm interpreting this SO right, that should be correct.

@CortneyOfstad
Copy link
Contributor

Thanks @Li357!

@Ahmed-Abdella — can you link me your Upwork profile? I'm having trouble finding it and I want to make sure that I am sending the proposal to the correct individual. Thank you!

@mkhutornyi — same as above. I want to make sure I am sending the proposal to the correct individual as there are two "Mykhailo K." entries in Upwork.

Thanks!

@mkhutornyi
Copy link
Contributor

@CortneyOfstad I am the one whom you just paid in #26243 🙂

@Ahmed-Abdella
Copy link
Contributor

@CortneyOfstad
Copy link
Contributor

I am the one whom you just paid in #26243 🙂

For some reason that one had your hiring profile listed automatically, but not this one 🤔 So weird! Thanks @mkhutornyi and send over the proposal!

@Ahmed-Abdella Also sent over your proposal! Let me know when these are accepted and I'll get them paid ASAP 👍

@Ahmed-Abdella
Copy link
Contributor

@CortneyOfstad I accepted the offer, Thanks!

@CortneyOfstad
Copy link
Contributor

Both have been paid!

@Li357 and @mkhutornyi where are we at with the checklist above? Thanks!

@mkhutornyi
Copy link
Contributor

I can help filling checklist.

Regression Test Proposal

  1. Login any account on device A
  2. On device B, logout if already logged in
  3. On device B, login same account as in device A
  4. On device A, update any personal details. i.e. Date of birth
  5. On device B, go to Settings > Profile > Personal details page
  6. Verify that page fully shows after full screen loading indicator, not stuck at loading

@melvin-bot melvin-bot bot added the Overdue label Oct 9, 2023
@CortneyOfstad
Copy link
Contributor

Regression Test GH created here — https://github.com/Expensify/Expensify/issues/324340

@melvin-bot melvin-bot bot removed the Overdue label Oct 9, 2023
@CortneyOfstad
Copy link
Contributor

Thanks all! 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
None yet
Development

No branches or pull requests

9 participants