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

[HOLD for payment 2025-01-13] [HOLD for payment 2025-01-02] [$250] Apply tax expense rules associated with category #53220

Closed
8 tasks done
MonilBhavsar opened this issue Nov 27, 2024 · 39 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item. Weekly KSv2

Comments

@MonilBhavsar
Copy link
Contributor

MonilBhavsar commented Nov 27, 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!


Linked to internal issue https://github.com/Expensify/Expensify/issues/338213
Version Number:
Reproducible in staging?:
Reproducible in production?:
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?:
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:
Slack conversation (hyperlinked to channel name):

Now that we have tax expense rules for categories, we do want to apply them when user creates an expense with that category or updates any existing expense with a specific category.

The expense rules are stored in policy_ collection inside rules > expenseRules key

When user creates or updates an expense associated with a policy and we have expenseRules with tax, we want to check and apply them.

We do have a isMatch() function in expensify-common repo and we could utilise it https://github.com/Expensify/expensify-common/blob/302e37e40d9d7e718e7c1c3d98faaa644b51da9d/lib/ExpenseRule.jsx#L40

Action Performed:

Break down in numbered steps

  1. Create a control workspace
  2. Enable Rules
  3. Enable Taxes and create couple of tax rates
  4. Go to categories, click on it and update the tax rate associated with category
  5. Go to this workspace chat, create a money request and on confirmation page, change the category

Expected Result:

Describe what you think should've happened
Tax rate should change to the tax rate associated with category in step 4 above

Actual Result:

Describe what actually happened
Tax rate does not change

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?
None

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

View all open jobs on GitHub

Issue OwnerCurrent Issue Owner: @JmillsExpensify
@MonilBhavsar MonilBhavsar added Daily KSv2 NewFeature Something to build that is a new item. labels Nov 27, 2024
Copy link

melvin-bot bot commented Nov 27, 2024

Triggered auto assignment to @JmillsExpensify (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Nov 27, 2024
Copy link

melvin-bot bot commented Nov 27, 2024

⚠️ It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time ⚠️

Copy link

melvin-bot bot commented Nov 27, 2024

Triggered auto assignment to Design team member for new feature review - @dannymcclain (NewFeature)

@MonilBhavsar
Copy link
Contributor Author

@dannymcclain sorry for the ping. We are all good here with the design

@MonilBhavsar MonilBhavsar added External Added to denote the issue can be worked on by a contributor and removed Weekly KSv2 labels Nov 27, 2024
@melvin-bot melvin-bot bot changed the title Apply tax expense rules associated with category [$250] Apply tax expense rules associated with category Nov 27, 2024
Copy link

melvin-bot bot commented Nov 27, 2024

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

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

melvin-bot bot commented Nov 27, 2024

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

@cretadn22
Copy link
Contributor

cretadn22 commented Nov 27, 2024

Edited by proposal-police: This proposal was edited at 2024-11-27 16:12:00 UTC.

Proposal

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

Apply tax expense rules associated with category

What is the root cause of that problem?

New feature

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

When creating a money request or editing the category in a request, we need to verify if the selected category is associated with any tax using the isMatch function

https://github.com/Expensify/expensify-common/blob/302e37e40d9d7e718e7c1c3d98faaa644b51da9d/lib/ExpenseRule.jsx#L40

The we can use getCategoryDefaultTaxRate function to get the tax value

function getCategoryDefaultTaxRate(expenseRules: ExpenseRule[], categoryName: string, defaultTaxRate?: string) {

If getCategoryDefaultTaxRate returns a tax, we will set this value to the tax field in the optimistic data.

Additionally, this needs to be updated on the backend as well.

What alternative solutions did you explore? (Optional)

@cretadn22
Copy link
Contributor

@MonilBhavsar I believe a backend update is necessary to fully resolve this issue.

@MonilBhavsar
Copy link
Contributor Author

Yes, I am working on it

@bernhardoj
Copy link
Contributor

bernhardoj commented Nov 27, 2024

Proposal

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

We want to apply the tax expense rules for category.

What is the root cause of that problem?

Feature request.

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

When selecting the category, we need to handle 3 cases.

if (transaction) {
// In the split flow, when editing we use SPLIT_TRANSACTION_DRAFT to save draft value
if (isEditingSplitBill) {
IOU.setDraftSplitTransaction(transaction.transactionID, {category: updatedCategory});
navigateBack();
return;
}
if (isEditing && report) {
IOU.updateMoneyRequestCategory(transaction.transactionID, report.reportID, updatedCategory, policy, policyTags, policyCategories);
navigateBack();
return;
}
}
IOU.setMoneyRequestCategory(transactionID, updatedCategory);
if (action === CONST.IOU.ACTION.CATEGORIZE) {
Navigation.closeAndNavigate(ROUTES.MONEY_REQUEST_STEP_CONFIRMATION.getRoute(action, iouType, transactionID, report?.reportID ?? '-1'));
return;
}
navigateBack();

First, set the money request draft tax if the category has the tax associated with it.
Second, update the money request tax optimistically if the category has the tax associated with it.
Last, set the money request split draft tax if the category has the tax associated with it.

To do that, first we need to get the associated tax with the category.

const categoryTaxCode = CategoryUtils.getCategoryDefaultTaxRate(policy?.rules?.expenseRules ?? [], updatedCategory);
// or we can set the default text code to policy?.taxRates?.defaultExternalID
// const categoryTaxCode = getCategoryDefaultTaxRate(policy?.rules?.expenseRules ?? [], updatedCategory, policy?.taxRates?.defaultExternalID);
// we did this in https://github.com/Expensify/App/blob/9ea0f51cba69f6183386eeb39f5f751e77968271/src/pages/workspace/categories/CategoryDefaultTaxRatePage.tsx#L32
let categoryTaxPercentage;
let categoryTaxAmount;

if (categoryTaxCode) {
    categoryTaxPercentage = TransactionUtils.getTaxValue(policy, currentTransaction, categoryTaxCode);

    if (categoryTaxPercentage) {
        const isFromExpenseReport = ReportUtils.isExpenseReport(report) || ReportUtils.isPolicyExpenseChat(report);
        categoryTaxAmount = CurrencyUtils.convertToBackendAmount(TransactionUtils.calculateTaxAmount(
            categoryTaxPercentage,
            TransactionUtils.getAmount(currentTransaction, isFromExpenseReport),
            TransactionUtils.getCurrency(transaction)
        ));
    }
}

The logic above is similar to the logic we have on the amount page.

const taxPercentage = TransactionUtils.getTaxValue(policy, currentTransaction, taxCode) ?? '';
const taxAmount = CurrencyUtils.convertToBackendAmount(TransactionUtils.calculateTaxAmount(taxPercentage, newAmount, currency ?? CONST.CURRENCY.USD));

For the first case, set the tax here.

IOU.setMoneyRequestCategory(transactionID, updatedCategory);

if (categoryTaxCode && categoryTaxAmount !== undefined) {
    IOU.setMoneyRequestTaxRate(transactionID, categoryTaxCode);
    IOU.setMoneyRequestTaxAmount(transactionID, categoryTaxAmount);
}

For the second case, accept 2 new params called categoryTaxCode and categoryTaxAmount and pass to updateMoneyRequestCategory.

if (isEditing && report) {
IOU.updateMoneyRequestCategory(transaction.transactionID, report.reportID, updatedCategory, policy, policyTags, policyCategories);
navigateBack();
return;

IOU.updateMoneyRequestCategory(transaction.transactionID, report.reportID, updatedCategory, categoryTaxCode, categoryTaxAmount, policy, policyTags, policyCategories);

Then, add it to the transactionChanges object.

App/src/libs/actions/IOU.ts

Lines 3202 to 3212 in 9ea0f51

function updateMoneyRequestCategory(
transactionID: string,
transactionThreadReportID: string,
category: string,
policy: OnyxEntry<OnyxTypes.Policy>,
policyTagList: OnyxEntry<OnyxTypes.PolicyTagLists>,
policyCategories: OnyxEntry<OnyxTypes.PolicyCategories>,
) {
const transactionChanges: TransactionChanges = {
category,
};

const transactionChanges: TransactionChanges = {
    category,
};

if (categoryTaxCode && categoryTaxAmount !== undefined) {
    transactionChanges["taxCode"] = categoryTaxCode;
    transactionChanges["taxAmount"] = categoryTaxAmount;
}

For the last case, it's similar to the 2nd one. The setDraftSplitTransaction accept a transactionChanges object, so we need to apply the same logic above before passing the object.

if (isEditingSplitBill) {
IOU.setDraftSplitTransaction(transaction.transactionID, {category: updatedCategory});
navigateBack();

const transactionChanges: TransactionUtils.TransactionChanges = {category: updatedCategory};
if (categoryTaxCode && categoryTaxAmount !== undefined) {
    transactionChanges.taxCode = categoryTaxCode;
    transactionChanges.taxAmount = categoryTaxAmount;
}
IOU.setDraftSplitTransaction(transaction.transactionID, transactionChanges);

@MonilBhavsar
Copy link
Contributor Author

Thanks for quick proposals!

@cretadn22 would be great to get some more details

@bernhardoj looks good mostly, instead of getting categoryTaxCode first, can we first see if the category has any matching expense rule?
Like -

  1. iterate through expense rules
  2. See if we have tax rule, by checking expenseRule.tax field
  3. See if the rule matches the category; Utilise or replicate this function https://github.com/Expensify/expensify-common/blob/302e37e40d9d7e718e7c1c3d98faaa644b51da9d/lib/ExpenseRule.jsx#L40
  4. Finally get the taxID and taxRate and update the transaction taxCode and taxAmount

@bernhardoj
Copy link
Contributor

I think that's already handled by getCategoryDefaultTaxRate.

function getCategoryDefaultTaxRate(expenseRules: ExpenseRule[], categoryName: string, defaultTaxRate?: string) {
const categoryDefaultTaxRate = expenseRules?.find((rule) => rule.applyWhen.some((when) => when.value === categoryName))?.tax?.field_id_TAX?.externalID;
// If the default taxRate is not found in expenseRules, use the default value for policy
if (!categoryDefaultTaxRate) {
return defaultTaxRate;
}
return categoryDefaultTaxRate;
}

@MonilBhavsar
Copy link
Contributor Author

Got it, thanks!

@MonilBhavsar
Copy link
Contributor Author

@mollfpr mind taking a look, please

@melvin-bot melvin-bot bot added the Weekly KSv2 label Dec 26, 2024
Copy link

melvin-bot bot commented Dec 26, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Dec 26, 2024
@melvin-bot melvin-bot bot changed the title [$250] Apply tax expense rules associated with category [HOLD for payment 2025-01-02] [$250] Apply tax expense rules associated with category Dec 26, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Dec 26, 2024
Copy link

melvin-bot bot commented Dec 26, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Dec 26, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.78-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2025-01-02. 🎊

For reference, here are some details about the assignees on this issue:

  • @mollfpr requires payment through NewDot Manual Requests
  • @bernhardoj requires payment through NewDot Manual Requests

Copy link

melvin-bot bot commented Dec 26, 2024

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@mollfpr] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
  • [@JmillsExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jan 2, 2025
Copy link

melvin-bot bot commented Jan 2, 2025

Payment Summary

Upwork Job

BugZero Checklist (@JmillsExpensify)

  • I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@melvin-bot melvin-bot bot added Overdue Weekly KSv2 and removed Daily KSv2 labels Jan 6, 2025
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2025-01-02] [$250] Apply tax expense rules associated with category [HOLD for payment 2025-01-13] [HOLD for payment 2025-01-02] [$250] Apply tax expense rules associated with category Jan 6, 2025
@melvin-bot melvin-bot bot removed the Overdue label Jan 6, 2025
Copy link

melvin-bot bot commented Jan 6, 2025

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.80-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2025-01-13. 🎊

For reference, here are some details about the assignees on this issue:

  • @mollfpr requires payment through NewDot Manual Requests
  • @bernhardoj requires payment through NewDot Manual Requests

Copy link

melvin-bot bot commented Jan 6, 2025

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@mollfpr] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
  • [@JmillsExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

Copy link

melvin-bot bot commented Jan 13, 2025

Payment Summary

Upwork Job

BugZero Checklist (@JmillsExpensify)

  • I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@JmillsExpensify
Copy link

Confirming the payment summary above is correct.

@JmillsExpensify
Copy link

Both are paid out via NewDot, so I'm closing, though @mollfpr please complete the BZ checklist before requesting payment.

@bernhardoj
Copy link
Contributor

Requested in ND.

@garrettmknight
Copy link
Contributor

Payment Summary

@JmillsExpensify
Copy link

$250 approved for @bernhardoj

@mollfpr
Copy link
Contributor

mollfpr commented Jan 15, 2025

I think it's no need a BZ checklist since it's a new feature but I suggesting we add a regression test case.

Regression Test Proposal

A

Precondition:

  • Rules, Distance rates, Categories, and Taxes are enabled.
  • There are two non-zero tax rates - Tax rate A and Tax rate B.
  • Distance Track tax is enabled.
  • A distance rate is assigned to tax rate A.
  • A category default tax rate is assigned to tax rate B

Test

  1. Open the workspace chat
  2. Open a submit expense flow and select Distance
  3. Proceed to the confirmation flow
  4. Change the distance rate to the distance rate from precondition (if not selected)
  5. Verify the tax rate is the tax rate A (the distance rate tax)
  6. Change the category to the category from precondition
  7. Verify the tax rate isn't updated
  8. Submit
  9. Open the expense/transaction report
  10. Change the category
  11. Verify the tax rate isn't updated

B

Precondition:

  • Rules, Categories, and Taxes are enabled.
  • There are 3 different tax rates - Tax rate A, B, C.
  • Tax Rate A is set as workspace default currency
  • Tax Rate B is set as foreign default currency
  • A category default tax rate is assigned to tax rate C

Test

  1. Open the workspace
  2. Submit a new manual request with the category from precondition
  3. Open the expense report
  4. Verify the tax rate is tax rate C
  5. Unselect the category
  6. Verify the tax rate is updated to tax rate B
  7. Select a different category
  8. Verify the tax rate is still tax rate B
  9. Verify the system message only mentions about category

Do we agree 👍 or 👎

@garrettmknight
Copy link
Contributor

$250 approved for @mollfpr

@garrettmknight
Copy link
Contributor

Also, tests added.

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

No branches or pull requests

7 participants