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

[$250] Taxes settings – The old tax name reappears for a moment if you change it offline. #41300

Closed
3 of 6 tasks
izarutskaya opened this issue Apr 30, 2024 · 21 comments
Closed
3 of 6 tasks
Assignees
Labels
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 Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@izarutskaya
Copy link

izarutskaya commented Apr 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: v1.4.68-0
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4524643
Email or phone of affected tester (no customers): applausetester+jp_e_category@applause.expensifail.com
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-internal team

Action Performed:

  1. Go to https://staging.new.expensify.com/
  2. Log in
  3. Create a workspace
  4. Click on More features and enable Taxes
  5. Add new tax rate
  6. Go offline
  7. Edit tax rate – change name and rate
  8. Go back to taxes setting page
  9. Go online and observe edited tax rate

Expected Result:

The new tax name presents if you change it offline

Actual Result:

The old tax name reappears for a moment if you change it offline.
The same with Tags and Categories

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

Bug6466174_1714448684274.Tax.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01e83b2f5493f8acd6
  • Upwork Job ID: 1785239218990989313
  • Last Price Increase: 2024-05-07
Issue OwnerCurrent Issue Owner: @alitoshmatov
@izarutskaya izarutskaya added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 30, 2024
Copy link

melvin-bot bot commented Apr 30, 2024

Triggered auto assignment to @grgia (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

Copy link

melvin-bot bot commented Apr 30, 2024

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

@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Apr 30, 2024
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.

@izarutskaya
Copy link
Author

@zanyrenney 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.

@izarutskaya
Copy link
Author

We think this issue might be related to the #collect project.

@izarutskaya
Copy link
Author

Production

Recording.2410.mp4

@grgia grgia removed the DeployBlockerCash This issue or pull request should block deployment label Apr 30, 2024
@grgia
Copy link
Contributor

grgia commented Apr 30, 2024

Not a deploy blocker

@grgia grgia added Daily KSv2 External Added to denote the issue can be worked on by a contributor and removed Hourly KSv2 labels Apr 30, 2024
@melvin-bot melvin-bot bot changed the title Taxes settings – The old tax name reappears for a moment if you change it offline. [$250] Taxes settings – The old tax name reappears for a moment if you change it offline. Apr 30, 2024
Copy link

melvin-bot bot commented Apr 30, 2024

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

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

melvin-bot bot commented Apr 30, 2024

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

@ph17
Copy link

ph17 commented May 2, 2024

Proposal

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

When you change the tax offline and goes back online, the old name reappears for a few seconds until is substituted by the new one

What is the root cause of that problem?

This is an example of the replay effect, as stated in this comment:

function applyHTTPSOnyxUpdates(request: Request, response: Response) {
console.debug('[OnyxUpdateManager] Applying https update');
// For most requests we can immediately update Onyx. For write requests we queue the updates and apply them after the sequential queue has flushed to prevent a replay effect in
// the UI. See https://github.com/Expensify/App/issues/12775 for more info.
const updateHandler: (updates: OnyxUpdate[]) => Promise<unknown> = request?.data?.apiRequestType === CONST.API_REQUEST_TYPE.WRITE ? QueuedOnyxUpdates.queueOnyxUpdates : Onyx.update;

The problem is that the first request that applyHTTPSOnyxUpdates receives when the app goes back online is of type read. updateHandler receives Onyx.update which causes the replay effect.

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

We should use the same process both for read and write request types. Making udpateHandler receive QueuedOnyxUpdates.queueOnyxUpdates for both cases. For this, we should change the applyHTTPSOnyxUpdates function:

    const isWriteOrReadRequest = request?.data?.apiRequestType === CONST.API_REQUEST_TYPE.WRITE || request?.data?.apiRequestType === CONST.API_REQUEST_TYPE.READ;
    const updateHandler: (updates: OnyxUpdate[]) => Promise<unknown> = isWriteOrReadRequest ? QueuedOnyxUpdates.queueOnyxUpdates : Onyx.update;

@melvin-bot melvin-bot bot added the Overdue label May 2, 2024
Copy link

melvin-bot bot commented May 2, 2024

📣 @ph17! 📣
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>

@ph17
Copy link

ph17 commented May 2, 2024

Contributor details
Your Expensify account email: pedrohenriquelemos17@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01000c5b7e9dbec144

Copy link

melvin-bot bot commented May 2, 2024

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

@zanyrenney
Copy link
Contributor

@alitoshmatov please can you review the proposal above and let us know if we can move forward with it?

@zanyrenney
Copy link
Contributor

bump @alitoshmatov please can you prioritise this or let us know if we should be finding another C+ to work on this?

Copy link

melvin-bot bot commented May 3, 2024

@grgia, @zanyrenney, @alitoshmatov Whoops! This issue is 2 days overdue. Let's get this updated quick!

@alitoshmatov
Copy link
Contributor

I couldn't reproduce this issue

Screen.Recording.2024-05-05.at.10.40.59.PM.mov

@melvin-bot melvin-bot bot removed the Overdue label May 5, 2024
Copy link

melvin-bot bot commented May 7, 2024

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

Copy link

melvin-bot bot commented May 8, 2024

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

@melvin-bot melvin-bot bot added the Overdue label May 8, 2024
@zanyrenney
Copy link
Contributor

Let me try and test too.

@zanyrenney
Copy link
Contributor

I can't reproduce this on taxes anymore, its mimicking the expected behaviour offline pattern C.

@melvin-bot melvin-bot bot removed the Overdue label May 9, 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 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
None yet
Development

No branches or pull requests

5 participants