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

[P2P Distance] - Error shows up after selecting distance rate without tax rate and tax reclaimable #46957

Closed
6 tasks done
IuliiaHerets opened this issue Aug 7, 2024 · 23 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Aug 7, 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: v9.0.17-1

Reproducible in staging?: Y
Reproducible in production?: N
Email or phone of affected tester (no customers): applausetester+kh050806@applause.expensifail.com
Issue reported by: Applause Internal Team

Action Performed:

Precondition:

  • Track tax is enabled on Workspace settings > Distance rates > Settings.
  1. Go to staging.new.expensify.com
  2. Go to workspace settings > Distance rates.
  3. Create a distance rate.
  4. Do not assign a tax rate and tax reclaimable to the distance rate.
  5. Go to workspace chat.
  6. Submit a distance expense with distance rate different from the rate created in Step 4.
  7. Go to transaction thread.
  8. Click Rate.
  9. Select the distance rate without tax rate from Step 4.

Expected Result:

User will be able to select the distance rate without tax rate and tax reclaimable.

Actual Result:

Error shows up after selecting the distance rate without tax rate and tax reclaimable.

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

Bug6564262_1723025936008.20240807_181156.mp4

View all open jobs on GitHub

Issue OwnerCurrent Issue Owner: @Christinadobrzyn
@IuliiaHerets IuliiaHerets added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. DeployBlocker Indicates it should block deploying the API labels Aug 7, 2024
Copy link

melvin-bot bot commented Aug 7, 2024

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

Copy link

melvin-bot bot commented Aug 7, 2024

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

@IuliiaHerets
Copy link
Author

We think that this bug might be related to #wave-collect - Release 2

Copy link
Contributor

github-actions bot commented Aug 7, 2024

👋 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.

@trjExpensify
Copy link
Contributor

trjExpensify commented Aug 7, 2024

The Rate field is not on staging for me in my tom@trj.chat account, because it's a feature behind the p2pDistanceRequests beta and that account isn't on the beta.

image

So I think this can be demoted as a deploy blocker.

@trjExpensify
Copy link
Contributor

Testing on my expensify.com account which is on the beta, I'm getting this when the UpdateMoneyRequestDistanceRate command is called:

jsonCode: 501, message: "501 Invalid tax percentage", onyxData: [], requestID: "8af762156c4bbefd-LHR"}
"501 Invalid tax percentage"
8af762156c4bbefd-LHR
image

Internal logs for that requestID are here.

Could not calculate taxAmount, taxPercentage: , and amount: 0
Throwing exception with message: '501 Invalid tax percentage' from auth/lib/Transaction.cpp:3065

That's here in Auth.

@MonilBhavsar any insight as you added this in the blame? 🤔 (CC: @nkdengineer as well for context on building this feature a few months ago!)

@MonilBhavsar
Copy link
Contributor

I was assigned couple of server errors too. Something looks off with the command. Looking into it

@trjExpensify
Copy link
Contributor

Amazing! Do you agree this isn't an /app blocker though, given you can't access this Rate field on the expense to change if you aren't on the p2pDistanceRequests beta?

@MonilBhavsar
Copy link
Contributor

agree, shouldn't be an app blocker, neither web. The logic lies in Auth.

But I wonder, in this case if user does not select tax rate and tax claimable percentage. Should we fallback to workspace default tax percentage?

@MonilBhavsar
Copy link
Contributor

But wait. Why this created as blocker though 🤔

@mountiny
Copy link
Contributor

mountiny commented Aug 7, 2024

@cristipaval @MonilBhavsar I agree this does not have to be a blocker cc @neil-marcellini as its related to the p2p distance

Feel free to remove the labels

@trjExpensify trjExpensify added Daily KSv2 and removed Hourly KSv2 DeployBlockerCash This issue or pull request should block deployment DeployBlocker Indicates it should block deploying the API labels Aug 7, 2024
@trjExpensify
Copy link
Contributor

But wait. Why this created as blocker though 🤔

You hit this error if you change the Rate field on the distance expense, under the conditions in the steps in the OP.

But I wonder, in this case if user does not select tax rate and tax claimable percentage. Should we fallback to workspace default tax percentage?

No, I don't think we do that on OldDot do we? Just the output is $0.00. Let me check.

@trjExpensify
Copy link
Contributor

Yeah, we just leave the tax amount at $0.00 and uneditable - we don't default to the workspace default tax rate or anything as far as I can tell.

image image

@MonilBhavsar
Copy link
Contributor

Okay, thanks for looking! 🙌

Here, we need to skip calculating tax percentage if taxCode is empty(not set by user), and add automated tests
https://github.com/Expensify/Auth/blob/c15ed2282c02386eb3d2239ff33263994c4ec798/auth/command/UpdateMoneyRequestDistanceRate.cpp#L90

I'm prioritising fire cleanup task currently, so not picking up

@melvin-bot melvin-bot bot added the Overdue label Aug 9, 2024
@MonilBhavsar
Copy link
Contributor

I'm working on https://github.com/Expensify/Expensify/issues/418428, and that should fix this issue

Copy link

melvin-bot bot commented Aug 12, 2024

@cristipaval, @Christinadobrzyn Huh... This is 4 days overdue. Who can take care of this?

@Christinadobrzyn Christinadobrzyn changed the title Distance - Error shows up after selecting distance rate without tax rate and tax reclaimable [HOLD FOR #418428] Distance - Error shows up after selecting distance rate without tax rate and tax reclaimable Aug 12, 2024
@Christinadobrzyn
Copy link
Contributor

Thanks @MonilBhavsar! I'll add a hold label to retest after https://github.com/Expensify/Expensify/issues/418428 is deployed

@melvin-bot melvin-bot bot removed the Overdue label Aug 12, 2024
@cristipaval
Copy link
Contributor

I'm handing this over to you, @MonilBhavsar, as I get closer to my parental leave. Thanks for fixing 🙏

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Aug 13, 2024

@neil-marcellini neil-marcellini changed the title [HOLD FOR #418428] Distance - Error shows up after selecting distance rate without tax rate and tax reclaimable [HOLD FOR #418428][P2P Distance] - Error shows up after selecting distance rate without tax rate and tax reclaimable Aug 13, 2024
@MonilBhavsar MonilBhavsar changed the title [HOLD FOR #418428][P2P Distance] - Error shows up after selecting distance rate without tax rate and tax reclaimable [P2P Distance] - Error shows up after selecting distance rate without tax rate and tax reclaimable Aug 14, 2024
@MonilBhavsar
Copy link
Contributor

PR was deployed and issue is off hold now @Christinadobrzyn

@Christinadobrzyn
Copy link
Contributor

Testing again - this appears to be resolved! Asking QA to test. https://expensify.slack.com/archives/C9YU7BX5M/p1723649337946129

@m-natarajan
Copy link

Bug is fixed

20240815_140710.mp4

@melvin-bot melvin-bot bot added the Overdue label Aug 16, 2024
@Christinadobrzyn
Copy link
Contributor

Awesome! closing!

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
Projects
Status: Done
Development

No branches or pull requests

7 participants