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-07-25] [$250] Split- Unknown user profile loads infinitely when opening from split confirmation page offline #42841

Closed
6 tasks done
m-natarajan opened this issue May 30, 2024 · 49 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@m-natarajan
Copy link

m-natarajan commented May 30, 2024

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: 1.4.77-3
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 to staging.new.expensify.com
  2. Go offline.
  3. Go to FAB > Split expense > Scan.
  4. Upload a receipt.
  5. Select an unknown account (non-existing account or never interacted with before account).
  6. Proceed to confirmation page.
  7. Click on the unknown account.

Expected Result:

The user profile of the unknown account will not load infinitely.

Actual Result:

The user profile of the unknown account loads infinitely.

Workaround:

unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6496029_1717063180701.20240530_175403.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01a2f8d719d411a342
  • Upwork Job ID: 1796338202260049920
  • Last Price Increase: 2024-05-31
  • Automatic offers:
    • rojiphil | Reviewer | 103030367
    • abzokhattab | Contributor | 103030368
Issue OwnerCurrent Issue Owner: @jliexpensify
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels May 30, 2024
@m-natarajan
Copy link
Author

m-natarajan commented May 30, 2024

@jliexpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

@m-natarajan
Copy link
Author

We think that this bug might be related to #vip-vsb

Copy link

melvin-bot bot commented May 30, 2024

Triggered auto assignment to @jliexpensify (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.

@abzokhattab
Copy link
Contributor

abzokhattab commented May 30, 2024

Proposal

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

An unknown user profile loads infinitely when accessed from the split confirmation page while offline.

What is the root cause of that problem?

This issue occurs because, when selecting a participant, the system attempts to load the profile data. Since the network is offline, it keeps loading until the network is back online.

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

We should disable clicking on a participant when the data is optimistic, similar to how it is handled in the "submit expense".

To achieve this, we can make the participant field disabled if the personal data is optimistic, as shown here:

isDisabled: !participant.isInvoiceRoom && !participant.isPolicyExpenseChat && !participant.isSelfDM && ReportUtils.isOptimisticPersonalDetail(participant.accountID ?? -1),

We can make it consistent and share the same condition in both the split and submit expenses:

To do this, we can define a shared function isParticipantDisabled:

    const isParticipantDisabled = (participant: MoneyRequestConfirmationListItem) =>
        !participant.isInvoiceRoom && !participant.isPolicyExpenseChat && !participant.isSelfDM && ReportUtils.isOptimisticPersonalDetail(participant.accountID ?? -1);

Then, use it here and add isDisabled: isParticipantDisabled(participantOption) to the participants sections here.

Alternative solution

Alternatively, to simplify, we can just add isDisabled: ReportUtils.isOptimisticPersonalDetail(participantOption.accountID ?? -1), here.

POC:

Screen.Recording.2024-05-30.at.4.23.29.PM.mov

@jliexpensify jliexpensify added the External Added to denote the issue can be worked on by a contributor label May 31, 2024
Copy link

melvin-bot bot commented May 31, 2024

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

@melvin-bot melvin-bot bot changed the title Split- Unknown user profile loads infinitely when opening from split confirmation page offline [$250] Split- Unknown user profile loads infinitely when opening from split confirmation page offline May 31, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label May 31, 2024
Copy link

melvin-bot bot commented May 31, 2024

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

@jliexpensify
Copy link
Contributor

Hmm yeah, can repro this one, moving it to #vip-split

@dominictb
Copy link
Contributor

dominictb commented May 31, 2024

Proposal

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

Unknown user profile loads infinitely when opening from split confirmation page offline

What is the root cause of that problem?

When the user opens a profile that doesn't yet exist in personalDetails, the app will attempt to load the profile data, but the user is currently offline so the loading will show forever.

In this non-split participants page

isDisabled: !participant.isInvoiceRoom && !participant.isPolicyExpenseChat && !participant.isSelfDM && ReportUtils.isOptimisticPersonalDetail(participant.accountID ?? -1),
, the app will prevent clicking on user profile if the profile is optimistic, we can do the same for the split case.

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

According to this requirement for the workspace participant, we should not make the participant look disabled

(but please do not make it look disabled, because it's not a disabled UI component, its just read-only text)

So, we should not use the isDisabled prop, instead there will likely be a new prop introduced for the non-interactive state, let's say the prop name is isNonInteractive.

We should hold this issue for that one so we have the prop ready, then in

and
return [payeeOption, ...selectedParticipants].map((participantOption: Participant) => ({
, set isNonInteractive with the identical condition as
isDisabled: !participant.isInvoiceRoom && !participant.isPolicyExpenseChat && !participant.isSelfDM && ReportUtils.isOptimisticPersonalDetail(participant.accountID ?? -1),
, that condition can be a common method and will be used for those cases. Some of the sub-conditions might not be relevant for the split case (ie. isSelfDM), so the method can be improved to check slightly differently for the split case, maybe we just need to check ReportUtils.isOptimisticPersonalDetail for splits.

What alternative solutions did you explore? (Optional)

Optimistically generate the participant data and save to personalDetails when the user clicks to view the profile of the user. If it exists in personalDetails, the user will still be able to see the profile page. It looks like there's a problem in the profile page that even though the user is offline, and the optimistic data of the profile is there, it still shows loading state. This can be improved by not showing loading state in such a case.

PrinceMakavana added a commit to PrinceMakavana/Expensify that referenced this issue May 31, 2024
@PrinceMakavana
Copy link

Proposal


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

Unknown user profile loads infinitely when opening from split confirmation page offline

### What is the root cause of that problem?

When the user opens a profile that doesn't yet exist in personalDetails also Not on Main Database , the app will attempt to load the profile data, but the user is currently offline so the loading will show forever.

{!hasAvatar && isLoading && <FullScreenLoadingIndicator style={styles.flex1} />}

Here hasAvatar & isLoading Both True Because user Data not valid or you can say that no data avaliable.

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

According to #42552 (comment) for the workspace participant, we should make the participant look disabled if it's not an expencify user , otherwise if we show then it cause problem for other steps like chat section *(from profile section use can able to navigate to 1:1 chat section).

So that Right Solution Just Disable Participant look disabled if it's not an expencify user.

Solution :

const details: PersonalDetails | EmptyObject = personalDetails?.[accountID] ?? (ValidationUtils.isValidAccountRoute(accountID) ? {} : {accountID: 0});

convert This details variable in useState and Add useEffect for that details variable.

Copy link

melvin-bot bot commented Jun 1, 2024

📣 @PrinceMakavana! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@PrinceMakavana
Copy link

Contributor details
Your Expensify account email: princemakavana61@gmail.com
Upwork Profile Link: Upwork Link

Copy link

melvin-bot bot commented Jun 1, 2024

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 Weekly KSv2 labels Jun 1, 2024
@jliexpensify
Copy link
Contributor

Bump @youssef-lr for a review please!

@jliexpensify jliexpensify added Weekly KSv2 and removed Monthly KSv2 labels Jul 8, 2024
@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 8, 2024
Copy link

melvin-bot bot commented Jul 8, 2024

📣 @rojiphil 🎉 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

Copy link

melvin-bot bot commented Jul 8, 2024

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

@youssef-lr
Copy link
Contributor

@abzokhattab's solution LGTM as well

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jul 13, 2024
@melvin-bot melvin-bot bot changed the title [$250] Split- Unknown user profile loads infinitely when opening from split confirmation page offline [HOLD for payment 2024-07-25] [$250] Split- Unknown user profile loads infinitely when opening from split confirmation page offline Jul 18, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jul 18, 2024
Copy link

melvin-bot bot commented Jul 18, 2024

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

Copy link

melvin-bot bot commented Jul 18, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.8-6 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-07-25. 🎊

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

Copy link

melvin-bot bot commented Jul 18, 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:

  • [@rojiphil] The PR that introduced the bug has been identified. Link to the PR:
  • [@rojiphil] 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:
  • [@rojiphil] 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:
  • [@rojiphil] Determine if we should create a regression test for this bug.
  • [@rojiphil] 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.
  • [@jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jul 24, 2024
@rojiphil
Copy link
Contributor

  • [@rojiphil] The PR that introduced the bug has been identified. Link to the PR: Offending PR
  • [@rojiphil] 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: Added comment
  • [@rojiphil] 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 Required. Existing checklist is good enough to capture such issues.
  • [@rojiphil] Determine if we should create a regression test for this bug. : Since this is an ui improvement, it is better to include regression tests
  • [@rojiphil] 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.

Test 1:
1 Go to FAB > Split expense > Manual.
2 Select an unknown account (non-existing account or never interacted with before account).
3 Proceed to the confirmation page.
4 Click on the unknown account.
5 ensure that the participant row is not clickable and thus the user's profile page is not opened
Test 2:
1 Go to FAB > Split expense > Scan.
2 Upload a receipt.
3 Select an unknown account (non-existing account or never interacted with before account).
4 Proceed to the confirmation page.
5 Click on the unknown account.
6 ensure that the participant row is not clickable and thus the user's profile page is not opened

@jliexpensify
Copy link
Contributor

I'll create a new upworks job, current one has expired.

@jliexpensify
Copy link
Contributor

Payment Summary

Have invited you both to the new Upworks job.

@rojiphil
Copy link
Contributor

@jliexpensify Accepted the offer. Thanks.

@abzokhattab
Copy link
Contributor

Thank @jliexpensify .. accepted the offer

@jliexpensify
Copy link
Contributor

Thanks, paying now!

@jliexpensify
Copy link
Contributor

Paid and job closed!

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 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

8 participants