Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.34k stars 2.77k forks source link

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

Closed IuliiaHerets closed 1 month ago

IuliiaHerets commented 1 month ago

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:

  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?

Screenshots/Videos

https://github.com/user-attachments/assets/0fa301d4-2df0-4510-98bc-c85cc46a91df

View all open jobs on GitHub

Issue OwnerCurrent Issue Owner: @Christinadobrzyn
melvin-bot[bot] commented 1 month ago

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.

melvin-bot[bot] commented 1 month ago

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

IuliiaHerets commented 1 month ago

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

github-actions[bot] commented 1 month ago

:wave: 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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

trjExpensify commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

But wait. Why this created as blocker though 🤔

mountiny commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

MonilBhavsar commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

Christinadobrzyn commented 1 month ago

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

cristipaval commented 1 month ago

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

Christinadobrzyn commented 1 month ago

Following https://github.com/Expensify/Auth/pull/12007 and https://github.com/Expensify/Expensify/issues/418428

MonilBhavsar commented 1 month ago

PR was deployed and issue is off hold now @Christinadobrzyn

Christinadobrzyn commented 1 month ago

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

m-natarajan commented 1 month ago

Bug is fixed

https://github.com/user-attachments/assets/8ac6e356-034c-483b-b0a7-47a21fb9f579

Christinadobrzyn commented 1 month ago

Awesome! closing!