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] IOU - Category is displayed in delay in employee workspace #37768

Closed
3 of 6 tasks
lanitochka17 opened this issue Mar 5, 2024 · 31 comments
Closed
3 of 6 tasks

[$500] IOU - Category is displayed in delay in employee workspace #37768

lanitochka17 opened this issue Mar 5, 2024 · 31 comments
Assignees
Labels
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

@lanitochka17
Copy link

lanitochka17 commented Mar 5, 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.47
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. Launch app
  2. Login as employee ( collect policy)
  3. Tap fab --- request money
  4. Enter an amount
  5. Tap next
  6. Select employee workspace
  7. Tap category
  8. Select a category
  9. Tap category
  10. Select another category

Expected Result:

Category must be displayed without delay in employee workspace

Actual Result:

Category is displayed in delay in employee workspace

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

Add any screenshot/video evidence

Bug6402940_1709652465108.empf.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01bb370ed42a61f34d
  • Upwork Job ID: 1765537922395549696
  • Last Price Increase: 2024-03-28
  • Automatic offers:
    • fedirjh | Reviewer | 0
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Mar 5, 2024
Copy link

melvin-bot bot commented Mar 5, 2024

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

@lanitochka17
Copy link
Author

@lschurr 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

@lschurr
Copy link
Contributor

lschurr commented Mar 6, 2024

This is part of wave-collect but I'll file it under wave 6 for now. Tagging @greg-schroeder who is sorting through these.

@greg-schroeder
Copy link
Contributor

Sorry, looks like have now agreed to close out the Wave 6 project, so we'll get it into the new one soon

@lschurr lschurr added the External Added to denote the issue can be worked on by a contributor label Mar 7, 2024
Copy link

melvin-bot bot commented Mar 7, 2024

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

@melvin-bot melvin-bot bot changed the title IOU - Category is displayed in delay in employee workspace [$500] IOU - Category is displayed in delay in employee workspace Mar 7, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 7, 2024
Copy link

melvin-bot bot commented Mar 7, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Mar 11, 2024
@jeremy-croff
Copy link
Contributor

Proposal

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

We have two screens here.

  1. Request confirmation screen
  2. Request Category screen

When we change the value of the category, it shows up delayed in the confirmation screen we immediately navigate back to.

What is the root cause of that problem?

When we go to the Category screen, we save the category AND navigate back to the confirmation screen in the same step. This causes a race condition between the Modal Stack Navigator and the time it takes to update the Onyx store and refetch the newly updated category to display.

Important note: The delay time is not very noticeable on faster laptops, but on slower devices, it can be pretty noticeable, as in the original post.

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

In order to immediately reflect the category, we should not use Navigate.goBack like we are doing here But instead use Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_CONFIRMATION.getRoute(iouType, transactionID, reportID, category));to go back to the previous screen.

  1. I copied the existing route and params for the screen we intend to go to
  2. I added a new category param to pass in the newly selected category value

We would update IOURequestStepConfirmation ( the parent here with the outdated category value) to check if the category is in its route params and use that value; otherwise, use the transaction category as a fallback for all existing flows.

These parameters from navigation allow us to immediately utilize the updated value rather than waiting for it! And it doesn't impact the user flow.

What alternative solutions did you explore? (Optional)

@lschurr
Copy link
Contributor

lschurr commented Mar 11, 2024

@fedirjh could you review the proposal?

@melvin-bot melvin-bot bot removed the Overdue label Mar 11, 2024
@fedirjh
Copy link
Contributor

fedirjh commented Mar 11, 2024

This causes a race condition between the Modal Stack Navigator and the time it takes to update the Onyx store and refetch the newly updated category to display.

@jeremy-croff Could you please elaborate on this? can you explain this race condition?

@jeremy-croff
Copy link
Contributor

jeremy-croff commented Mar 11, 2024

This causes a race condition between the Modal Stack Navigator and the time it takes to update the Onyx store and refetch the newly updated category to display.

@jeremy-croff Could you please elaborate on this? can you explain this race condition?

It's the animation transition between the two screens, how the card transitions from left to right as you click save category.
If the PC can update the category fast enough through props, before that navigation animation completes, then it won't show the outdated one. But in majority of cases there's a slight delay.

If the animation for navigation was super slow. i.e. 1 second, most pc's would have already updated the prop of the selected category.

If you slow down your CPU in the browser, you extend the time at which the stale category is shown.

Check the video timestamp at 27 seconds.

@fedirjh
Copy link
Contributor

fedirjh commented Mar 11, 2024

I think this is due to Onyx and not the animation. I recall that we had a similar bug where the animation was not smooth after changing the merchant, it was reported here #31193.

@jeremy-croff
Copy link
Contributor

jeremy-croff commented Mar 11, 2024

I think this is due to Onyx and not the animation. I recall that we had a similar bug where the animation was not smooth after changing the merchant, it was reported here #31193.

I think we are on the same page, I just called it out as a race condition between both in regards of the user experience.
Because if the animation(navigation) took longer it would hide the time it takes onyx to update the category.
Or if onyx merge was instant then it would also not be noticeable.

But my solution works around this by allowing the updated data to be passed as params instead of having to wait for onyx. I don't think we can realistically expect to write and retrieve to a cache outside of the components' lifecycle to be reflected instantly ever. And I did not want to impact the customer experience by adding in a middle step or something that would delay the navigation to mask the onyx persistence.

Copy link

melvin-bot bot commented Mar 14, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Mar 14, 2024
@lschurr
Copy link
Contributor

lschurr commented Mar 14, 2024

Any update @fedirjh?

@melvin-bot melvin-bot bot removed the Overdue label Mar 14, 2024
@fedirjh
Copy link
Contributor

fedirjh commented Mar 15, 2024

But my solution works around this by allowing the updated data to be passed as params instead of having to wait for onyx. I don't think we can realistically expect to write and retrieve to a cache outside of the components' lifecycle to be reflected instantly ever.

@jeremy-croff I appreciate your effort to address the issue, but I have concerns about the suggested workaround. While passing updated data as parameters may temporarily fix the issue, it doesn't address the root cause. In the past, we used a similar approach in this flow. However, we've since moved to using Onyx directly, signifying a shift towards more efficient and modern practices. Reverting to a workaround now feels regressive and could reintroduce complexity and maintenance challenges that we've worked hard to overcome.

I believe it's crucial to investigate and address the root cause, which appears to be related to Onyx. By tackling the underlying problem, we can ensure a more robust and maintainable solution in the long run.

I suggest we focus our efforts on identifying and resolving the Onyx-related issues to improve the overall stability and performance of our App.


Any update?

@lschurr We are still awaiting for proposals, We could probably pass this issue to Agency expert.

@melvin-bot melvin-bot bot added the Overdue label Mar 18, 2024
@lschurr
Copy link
Contributor

lschurr commented Mar 18, 2024

Thanks! I'll post in an agency room today.

@melvin-bot melvin-bot bot removed the Overdue label Mar 18, 2024
Copy link

melvin-bot bot commented Mar 19, 2024

@lschurr @fedirjh 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!

@jeremy-croff
Copy link
Contributor

@fedirjh
A new pattern has emerged for currency, which faced this same issue. It's now showing a loading indicator when navigating to the screen. Would implementing a loading iindicator for category be a reasonable solution? If so, I can update my proposal.

2024-03-19_21-53-13.mp4

@fedirjh
Copy link
Contributor

fedirjh commented Mar 21, 2024

A new pattern has emerged for currency, which faced this same issue.

@jeremy-croff I think this pattern was implemented in this PR to fix another issue, The loading indicator prevents the component from rendering with the wrong currency, until the correct currency is set.

@jeremy-croff
Copy link
Contributor

A new pattern has emerged for currency, which faced this same issue.

@jeremy-croff I think this pattern was implemented in this PR to fix another issue, The loading indicator prevents the component from rendering with the wrong currency, until the correct currency is set.

Is that not the same problem for a different field?
This ticket faces the issue of wrong category until the correct currency is set

Copy link

melvin-bot bot commented Mar 21, 2024

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

@fedirjh
Copy link
Contributor

fedirjh commented Mar 25, 2024

@jeremy-croff It seems that the loading view will be removed in this PR:

Copy link

melvin-bot bot commented Mar 28, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Mar 28, 2024
@fedirjh
Copy link
Contributor

fedirjh commented Mar 29, 2024

We are still awaiting new proposals.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Mar 29, 2024
@lschurr
Copy link
Contributor

lschurr commented Apr 1, 2024

I asked for help from Callstack here - https://expensify.slack.com/archives/C03UK30EA1Z/p1711987304872579

@melvin-bot melvin-bot bot removed the Overdue label Apr 1, 2024
@pac-guerreiro
Copy link
Contributor

Hi! I’m Pedro Guerreiro from Callstack - expert contributor group. I’d like to work on this task!

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 2, 2024
Copy link

melvin-bot bot commented Apr 2, 2024

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

@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@pac-guerreiro
Copy link
Contributor

Hi, I wasn't able to reproduce this issue 😅 Could this happen in slow internet connections?

@fedirjh
Copy link
Contributor

fedirjh commented Apr 4, 2024

I am unable to reproduce as well. It seems like the recent Onyx changes have fixed this issue.

@lschurr
Copy link
Contributor

lschurr commented Apr 4, 2024

Great! Going to close since this is no longer an issue.

@lschurr lschurr closed this as completed Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
No open projects
Archived in project
Development

No branches or pull requests

7 participants