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

[Awaiting Payment] [$250] Distance - Tax amount shows value A then changes to value B after changing distance rate #50641

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

Comments

@lanitochka17
Copy link

lanitochka17 commented Oct 11, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 9.0.48-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

Precondition:

  • Track tax is enabled
  • Workspace has at least two distance rates that have tax rate and tax reclaimable on.
  1. Go to staging.new.expensify.com
  2. Go to workspace chat
  3. Submit a distance expense
  4. Go to transaction thread
  5. Click Rate
  6. Select another distance rate that has tax rate and tax reclaimable on

Expected Result:

Tax amount will be consistent after changing distance rate

Actual Result:

Tax amount shows value A then changes to value B after changing distance rate

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
Bug6631708_1728640172607.20241011_174609.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021844824122118463977
  • Upwork Job ID: 1844824122118463977
  • Last Price Increase: 2024-10-11
  • Automatic offers:
    • FitseTLT | Contributor | 104444772
Issue OwnerCurrent Issue Owner: @ntdiary
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 11, 2024
Copy link

melvin-bot bot commented Oct 11, 2024

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

@lanitochka17
Copy link
Author

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

@lanitochka17
Copy link
Author

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

@garrettmknight garrettmknight added the External Added to denote the issue can be worked on by a contributor label Oct 11, 2024
Copy link

melvin-bot bot commented Oct 11, 2024

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

@melvin-bot melvin-bot bot changed the title Distance - Tax amount shows value A then changes to value B after changing distance rate [$250] Distance - Tax amount shows value A then changes to value B after changing distance rate Oct 11, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 11, 2024
Copy link

melvin-bot bot commented Oct 11, 2024

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

@nkdengineer
Copy link
Contributor

@neil-marcellini Can you please confirm it's bug or not since you worked on distance request? Thanks!

Copy link

melvin-bot bot commented Oct 15, 2024

@garrettmknight, @ntdiary 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 Oct 15, 2024
@FitseTLT
Copy link
Contributor

FitseTLT commented Oct 15, 2024

Proposal

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

Distance - Tax amount shows value A then changes to value B after changing distance rate
The problem occurs when the workspace unit is kilometer.

What is the root cause of that problem?

We are using the long (kilometer for km) unit version to getDistanceInMeters which expects the short form

const taxableAmount = DistanceRequestUtils.getTaxableAmount(policy, customUnitRateID, TransactionUtils.getDistanceInMeters(transaction, unit));

So it is falling back to mile, when converting to meter, as this condition here will be false even if the unit is km
if (unit === CONST.CUSTOM_UNITS.DISTANCE_UNIT_KILOMETERS) {

hence we are setting the tax amount to wrong value optimistically which changes when the BE sets the correct value. 👍

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

We should separately define the unitToDisplay here

const unit = (Object.values(rates).at(0)?.unit === CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES ? translate('common.mile') : translate('common.kilometer')) as Unit;

 const unit = Object.values(rates).at(0)?.unit;
    const unitToDisplay = (unit === CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES ? translate('common.mile') : translate('common.kilometer')) as Unit;

Then use it here

<Text style={[styles.mh5, styles.mv4]}>{translate('iou.chooseARate', {unit})}</Text>

            <Text style={[styles.mh5, styles.mv4]}>{translate('iou.chooseARate', {unit: unitToDisplay})}</Text>

What alternative solutions did you explore? (Optional)

@garrettmknight
Copy link
Contributor

garrettmknight commented Oct 15, 2024

Can you please confirm it's bug or not since you worked on distance request? Thanks!

The tax amount should change based the rate change, but it looks like it's calculating it incorrectly, then correcting, instead of calculating the correct rate immediately.

@ntdiary
Copy link
Contributor

ntdiary commented Oct 16, 2024

@FitseTLT's proposal looks good. Addtionally, it's just a small thing, maybe we should change the type of unit (in ReimbursementRateParams) to string, so we wouldn't need to use as to cast unitToDisplay to Unit, as this type doesn't actually match its real value. :)

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Oct 16, 2024

Triggered auto assignment to @mjasikowski, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@mjasikowski
Copy link
Contributor

@FitseTLT's proposal looks good indeed, let's go with the PR!

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

melvin-bot bot commented Oct 16, 2024

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

@FitseTLT
Copy link
Contributor

PR was raised but a conflicting PR solved the issue so I had to close the PR.
We can close this issue after issuing the necessary payments according to the rules 👍

@garrettmknight
Copy link
Contributor

garrettmknight commented Oct 22, 2024

Thanks for the update, I'll pay 50% for getting the PR up.

Payment summary:

@ntdiary
Copy link
Contributor

ntdiary commented Oct 22, 2024

Hi, @garrettmknight, curious, is there any compensation for C+ in this case? 😂

@FitseTLT
Copy link
Contributor

Hi, @garrettmknight, curious, is there any compensation for C+ in this case? 😂

Yes I think the rule in this case 100% for the contributor and 50% for C+ for reviewing the proposal

If a contributor has been hired for a job and we decide to close the job before it is successfully completed, full payment is due for the hired contributor. If the C+ only reviewed proposals and not a PR, they are eligible for 50%. If they started reviewing a PR it's 100%. One caveat here might be if the hired contributor wasn't working on their PR with urgency.

@garrettmknight read #47712 (comment) for context

@garrettmknight
Copy link
Contributor

Thanks for citing that - I've just paid the other 50% to @FitseTLT.

@ntdiary did you review the PR beyond asking to resolve the conflicts? Trying to figure out if we should be paying 50% or 100%.

@garrettmknight garrettmknight added Daily KSv2 Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 Reviewing Has a PR in review Daily KSv2 labels Oct 22, 2024
@ntdiary
Copy link
Contributor

ntdiary commented Oct 23, 2024

Thanks for citing that - I've just paid the other 50% to @FitseTLT.

@ntdiary did you review the PR beyond asking to resolve the conflicts? Trying to figure out if we should be paying 50% or 100%.

@garrettmknight, yeah, I reviewed the PR locally and then tried resolving the conflicts myself to record test videos. Usually, resolving conflicts is easy and doesn’t affect test, but here I found that it had already been fixed by another PR, so just left a reminder comment. 😂

@garrettmknight
Copy link
Contributor

Sounds good, updated the payment summary.

@garrettmknight garrettmknight changed the title [$250] Distance - Tax amount shows value A then changes to value B after changing distance rate [Awaiting Payment] [$250] Distance - Tax amount shows value A then changes to value B after changing distance rate Oct 23, 2024
@garrettmknight
Copy link
Contributor

Please request when you're ready.

@ntdiary
Copy link
Contributor

ntdiary commented Oct 24, 2024

Thx, have requested. 😄

@JmillsExpensify
Copy link

$250 approved for @ntdiary

Copy link

melvin-bot bot commented Oct 25, 2024

@garrettmknight @mjasikowski @ntdiary @FitseTLT 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!

@ntdiary
Copy link
Contributor

ntdiary commented Oct 28, 2024

@garrettmknight @mjasikowski @ntdiary @FitseTLT 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!

It looks like all the payments have been completed, so we can close this issue? :)

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

No branches or pull requests

7 participants