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

Revert "Revert "Handle tax for split requests"" and fix reported bugs #42737

Merged
merged 13 commits into from
Jun 6, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 35 additions & 28 deletions src/components/MoneyRequestConfirmationList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -178,11 +178,15 @@ type MoneyRequestConfirmationListProps = MoneyRequestConfirmationListOnyxProps &

type MoneyRequestConfirmationListItem = Participant | ReportUtils.OptionData;

const getTaxAmount = (transaction: OnyxEntry<OnyxTypes.Transaction>, policy: OnyxEntry<OnyxTypes.Policy>) => {
/**
* Calculate and set tax amount in transaction draft
*/
const setTaxAmountInDraft = (transaction: OnyxEntry<OnyxTypes.Transaction>, policy: OnyxEntry<OnyxTypes.Policy>) => {
const defaultTaxCode = TransactionUtils.getDefaultTaxCode(policy, transaction) ?? '';

const taxPercentage = TransactionUtils.getTaxValue(policy, transaction, transaction?.taxCode ?? defaultTaxCode) ?? '';
return TransactionUtils.calculateTaxAmount(taxPercentage, transaction?.amount ?? 0);
const taxAmount = TransactionUtils.calculateTaxAmount(taxPercentage, transaction?.amount ?? 0);
const taxAmountInSmallestCurrencyUnits = CurrencyUtils.convertToBackendAmount(Number.parseFloat(taxAmount.toString()));
IOU.setMoneyRequestTaxAmount(transaction?.transactionID ?? '', taxAmountInSmallestCurrencyUnits, true);
};

function MoneyRequestConfirmationList({
Expand Down Expand Up @@ -244,16 +248,6 @@ function MoneyRequestConfirmationList({
const transactionID = transaction?.transactionID ?? '';
const customUnitRateID = TransactionUtils.getRateID(transaction) ?? '';

useEffect(() => {
if (customUnitRateID || !canUseP2PDistanceRequests) {
return;
}
if (!customUnitRateID) {
const rateID = lastSelectedDistanceRates?.[policy?.id ?? ''] ?? defaultMileageRate?.customUnitRateID ?? '';
IOU.setCustomUnitRateID(transactionID, rateID);
}
}, [defaultMileageRate, customUnitRateID, lastSelectedDistanceRates, policy?.id, canUseP2PDistanceRequests, transactionID]);

const policyCurrency = policy?.outputCurrency ?? PolicyUtils.getPersonalPolicy()?.outputCurrency ?? CONST.CURRENCY.USD;

const mileageRate = TransactionUtils.isCustomUnitRateIDForP2P(transaction)
Expand Down Expand Up @@ -314,9 +308,9 @@ function MoneyRequestConfirmationList({
);
const formattedTaxAmount = CurrencyUtils.convertToDisplayString(transaction?.taxAmount, iouCurrencyCode);
const taxRateTitle = TransactionUtils.getTaxName(policy, transaction);

const previousTransactionAmount = usePrevious(transaction?.amount);
const previousTransactionCurrency = usePrevious(transaction?.currency);
const prevTaxAmount = usePrevious(transaction?.taxAmount);
const prevAmount = usePrevious(transaction?.amount);
const prevCurrency = usePrevious(transaction?.currency);

const isFocused = useIsFocused();
const [formError, debouncedFormError, setFormError] = useDebouncedState('');
Expand Down Expand Up @@ -345,6 +339,31 @@ function MoneyRequestConfirmationList({

const isCategoryRequired = !!policy?.requiresCategory;

useEffect(() => {
if (!shouldShowTax) {
return;
}
setTaxAmountInDraft(transaction, policy);
// eslint-disable-next-line react-hooks/exhaustive-deps -- we want to call this function when component is mounted
}, []);

useEffect(() => {
if (!shouldShowTax || prevTaxAmount !== transaction?.taxAmount || (prevAmount === transaction?.amount && prevCurrency === transaction?.currency)) {
Copy link
Contributor

@dukenv0307 dukenv0307 May 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MonilBhavsar Is this expectation? This is inconsistency with the below flow where the new amount is saved

  1. Create a split scan.
  2. When the receipt is scanning, click on the split preview.
  3. Edit tax amount
  4. Save it
  5. Reload page
  6. Observe the new amount is saved
web-resize.mp4

Copy link
Contributor Author

@MonilBhavsar MonilBhavsar May 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think should be expected behavior here?
The expense amount is also saved in a similar way, so looks good to me

Copy link
Contributor

@dukenv0307 dukenv0307 Jun 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The expense amount is also saved in a similar way

@MonilBhavsar Sorry, I don't understand your mean. Pls check the video below, the expense amount is saved but tax amount.

web-resize.mp4

I'm not sure, if it's the expectation we can go ahead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay, I got you're saying now. Looking

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dukenv0307 I have fixed this bug. Hopefully, I didn't break anything in fixing it 🙏

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll re-check again

return;
}
setTaxAmountInDraft(transaction, policy);
}, [policy, shouldShowTax, prevTaxAmount, prevAmount, prevCurrency, transaction]);

useEffect(() => {
if (customUnitRateID || !canUseP2PDistanceRequests) {
return;
}
if (!customUnitRateID) {
const rateID = lastSelectedDistanceRates?.[policy?.id ?? ''] ?? defaultMileageRate?.customUnitRateID ?? '';
IOU.setCustomUnitRateID(transactionID, rateID);
}
}, [defaultMileageRate, customUnitRateID, lastSelectedDistanceRates, policy?.id, canUseP2PDistanceRequests, transactionID]);

useEffect(() => {
if (shouldDisplayFieldError && hasSmartScanFailed) {
setFormError('iou.receiptScanningFailed');
Expand All @@ -369,18 +388,6 @@ function MoneyRequestConfirmationList({
IOU.setMoneyRequestAmount(transactionID, amount, currency ?? '');
}, [shouldCalculateDistanceAmount, distance, rate, unit, transactionID, currency]);

// Calculate and set tax amount in transaction draft
useEffect(() => {
const taxAmount = getTaxAmount(transaction, policy).toString();
const amountInSmallestCurrencyUnits = CurrencyUtils.convertToBackendAmount(Number.parseFloat(taxAmount));

if (transaction?.taxAmount && previousTransactionAmount === transaction?.amount && previousTransactionCurrency === transaction?.currency) {
return IOU.setMoneyRequestTaxAmount(transactionID, transaction?.taxAmount ?? 0, true);
}

IOU.setMoneyRequestTaxAmount(transactionID, amountInSmallestCurrencyUnits, true);
}, [policy, transaction, transactionID, previousTransactionAmount, previousTransactionCurrency]);

// If completing a split expense fails, set didConfirm to false to allow the user to edit the fields again
if (isEditingSplitBill && didConfirm) {
setDidConfirm(false);
Expand Down
21 changes: 18 additions & 3 deletions src/components/TaxPicker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React, {useMemo, useState} from 'react';
import {withOnyx} from 'react-native-onyx';
import type {OnyxEntry} from 'react-native-onyx';
import type {EdgeInsets} from 'react-native-safe-area-context';
import type {ValueOf} from 'type-fest';
import useLocalize from '@hooks/useLocalize';
import useStyleUtils from '@hooks/useStyleUtils';
import * as IOUUtils from '@libs/IOUUtils';
Expand All @@ -11,6 +12,7 @@ import CONST from '@src/CONST';
import type {IOUAction} from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import type {Policy, Transaction} from '@src/types/onyx';
import {isEmptyObject} from '@src/types/utils/EmptyObject';
import SelectionList from './SelectionList';
import RadioListItem from './SelectionList/RadioListItem';

Expand All @@ -20,6 +22,9 @@ type TaxPickerOnyxProps = {

/** All the data for the transaction */
transaction: OnyxEntry<Transaction>;

/** The draft transaction that holds data to be persisted on the current split transaction */
splitDraftTransaction: OnyxEntry<Transaction>;
};

type TaxPickerProps = TaxPickerOnyxProps & {
Expand All @@ -46,13 +51,20 @@ type TaxPickerProps = TaxPickerOnyxProps & {
/** The action to take */
// eslint-disable-next-line react/no-unused-prop-types
action?: IOUAction;

/** The type of IOU */
iouType?: ValueOf<typeof CONST.IOU.TYPE>;
};

function TaxPicker({selectedTaxRate = '', policy, transaction, insets, onSubmit}: TaxPickerProps) {
function TaxPicker({selectedTaxRate = '', policy, transaction, insets, onSubmit, action, splitDraftTransaction, iouType}: TaxPickerProps) {
const StyleUtils = useStyleUtils();
const {translate} = useLocalize();
const [searchValue, setSearchValue] = useState('');

const isEditing = action === CONST.IOU.ACTION.EDIT;
const isEditingSplitBill = isEditing && iouType === CONST.IOU.TYPE.SPLIT;
const currentTransaction = isEditingSplitBill && !isEmptyObject(splitDraftTransaction) ? splitDraftTransaction : transaction;

const taxRates = policy?.taxRates;
const taxRatesCount = TransactionUtils.getEnabledTaxRateCount(taxRates?.taxes ?? {});
const isTaxRatesCountBelowThreshold = taxRatesCount < CONST.TAX_RATES_LIST_THRESHOLD;
Expand All @@ -74,8 +86,8 @@ function TaxPicker({selectedTaxRate = '', policy, transaction, insets, onSubmit}
}, [selectedTaxRate]);

const sections = useMemo(
() => OptionsListUtils.getTaxRatesSection(policy, selectedOptions as OptionsListUtils.Tax[], searchValue, transaction),
[searchValue, selectedOptions, policy, transaction],
() => OptionsListUtils.getTaxRatesSection(policy, selectedOptions as OptionsListUtils.Tax[], searchValue, currentTransaction),
[searchValue, selectedOptions, policy, currentTransaction],
);

const headerMessage = OptionsListUtils.getHeaderMessageForNonUserList(sections[0].data.length > 0, searchValue);
Expand Down Expand Up @@ -112,4 +124,7 @@ export default withOnyx<TaxPickerProps, TaxPickerOnyxProps>({
return `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`;
},
},
splitDraftTransaction: {
key: ({transactionID}) => `${ONYXKEYS.COLLECTION.SPLIT_TRANSACTION_DRAFT}${transactionID}`,
},
})(TaxPicker);
2 changes: 2 additions & 0 deletions src/libs/API/parameters/CompleteSplitBillParams.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ type CompleteSplitBillParams = {
category?: string;
tag?: string;
splits: string;
taxCode?: string;
taxAmount?: number;
};

export default CompleteSplitBillParams;
2 changes: 2 additions & 0 deletions src/libs/API/parameters/SplitBillParams.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ type SplitBillParams = {
policyID: string | undefined;
chatType: string | undefined;
splitPayerAccountIDs: number[];
taxCode: string;
taxAmount: number;
};

export default SplitBillParams;
2 changes: 2 additions & 0 deletions src/libs/API/parameters/StartSplitBillParams.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ type StartSplitBillParams = {
createdReportActionID?: string;
billable: boolean;
chatType?: string;
taxCode?: string;
taxAmount?: number;
};

export default StartSplitBillParams;
49 changes: 40 additions & 9 deletions src/libs/actions/IOU.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3753,6 +3753,8 @@ function createSplitsAndOnyxData(
existingSplitChatReportID = '',
billable = false,
iouRequestType: IOURequestType = CONST.IOU.REQUEST_TYPE.MANUAL,
taxCode = '',
taxAmount = 0,
): SplitsAndOnyxData {
const currentUserEmailForIOUSplit = PhoneNumber.addSMSDomainIfPhoneNumber(currentUserLogin);
const participantAccountIDs = participants.map((participant) => Number(participant.accountID));
Expand All @@ -3774,8 +3776,8 @@ function createSplitsAndOnyxData(
undefined,
category,
tag,
undefined,
undefined,
taxCode,
taxAmount,
billable,
);

Expand Down Expand Up @@ -3930,14 +3932,16 @@ function createSplitsAndOnyxData(

// Loop through participants creating individual chats, iouReports and reportActionIDs as needed
const currentUserAmount = splitShares?.[currentUserAccountID]?.amount ?? IOUUtils.calculateAmount(participants.length, amount, currency, true);
const currentUserTaxAmount = IOUUtils.calculateAmount(participants.length, taxAmount, currency, true);

const splits: Split[] = [{email: currentUserEmailForIOUSplit, accountID: currentUserAccountID, amount: currentUserAmount}];
const splits: Split[] = [{email: currentUserEmailForIOUSplit, accountID: currentUserAccountID, amount: currentUserAmount, taxAmount: currentUserTaxAmount}];

const hasMultipleParticipants = participants.length > 1;
participants.forEach((participant) => {
// In a case when a participant is a workspace, even when a current user is not an owner of the workspace
const isPolicyExpenseChat = ReportUtils.isPolicyExpenseChat(participant);
const splitAmount = splitShares?.[participant.accountID ?? -1]?.amount ?? IOUUtils.calculateAmount(participants.length, amount, currency, false);
const splitTaxAmount = IOUUtils.calculateAmount(participants.length, taxAmount, currency, false);

// To exclude someone from a split, the amount can be 0. The scenario for this is when creating a split from a group chat, we have remove the option to deselect users to exclude them.
// We can input '0' next to someone we want to exclude.
Expand Down Expand Up @@ -4007,8 +4011,8 @@ function createSplitsAndOnyxData(
undefined,
category,
tag,
undefined,
undefined,
taxCode,
ReportUtils.isExpenseReport(oneOnOneIOUReport) ? -splitTaxAmount : splitTaxAmount,
billable,
);

Expand Down Expand Up @@ -4099,6 +4103,7 @@ function createSplitsAndOnyxData(
reportPreviewReportActionID: oneOnOneReportPreviewAction.reportActionID,
transactionThreadReportID: optimisticTransactionThread.reportID,
createdReportActionIDForThread: optimisticCreatedActionForTransactionThread.reportActionID,
taxAmount: splitTaxAmount,
};

splits.push(individualSplit);
Expand Down Expand Up @@ -4152,6 +4157,8 @@ type SplitBillActionsParams = {
existingSplitChatReportID?: string;
splitShares?: SplitShares;
splitPayerAccountIDs?: number[];
taxCode?: string;
taxAmount?: number;
};

/**
Expand All @@ -4174,6 +4181,8 @@ function splitBill({
existingSplitChatReportID = '',
splitShares = {},
splitPayerAccountIDs = [],
taxCode = '',
taxAmount = 0,
}: SplitBillActionsParams) {
const currentCreated = DateUtils.enrichMoneyRequestTimestamp(created);
const {splitData, splits, onyxData} = createSplitsAndOnyxData(
Expand All @@ -4191,6 +4200,8 @@ function splitBill({
existingSplitChatReportID,
billable,
iouRequestType,
taxCode,
taxAmount,
);

const parameters: SplitBillParams = {
Expand All @@ -4210,6 +4221,8 @@ function splitBill({
policyID: splitData.policyID,
chatType: splitData.chatType,
splitPayerAccountIDs,
taxCode,
taxAmount,
};

API.write(WRITE_COMMANDS.SPLIT_BILL, parameters, onyxData);
Expand All @@ -4236,6 +4249,8 @@ function splitBillAndOpenReport({
iouRequestType = CONST.IOU.REQUEST_TYPE.MANUAL,
splitShares = {},
splitPayerAccountIDs = [],
taxCode = '',
taxAmount = 0,
}: SplitBillActionsParams) {
const currentCreated = DateUtils.enrichMoneyRequestTimestamp(created);
const {splitData, splits, onyxData} = createSplitsAndOnyxData(
Expand All @@ -4253,6 +4268,8 @@ function splitBillAndOpenReport({
'',
billable,
iouRequestType,
taxCode,
taxAmount,
);

const parameters: SplitBillParams = {
Expand All @@ -4272,6 +4289,8 @@ function splitBillAndOpenReport({
policyID: splitData.policyID,
chatType: splitData.chatType,
splitPayerAccountIDs,
taxCode,
taxAmount,
};

API.write(WRITE_COMMANDS.SPLIT_BILL_AND_OPEN_REPORT, parameters, onyxData);
Expand All @@ -4291,6 +4310,8 @@ type StartSplitBilActionParams = {
category: string | undefined;
tag: string | undefined;
currency: string;
taxCode: string;
taxAmount: number;
};

/** Used exclusively for starting a split expense request that contains a receipt, the split request will be completed once the receipt is scanned
Expand All @@ -4309,6 +4330,8 @@ function startSplitBill({
category = '',
tag = '',
currency,
taxCode = '',
taxAmount = 0,
}: StartSplitBilActionParams) {
const currentUserEmailForIOUSplit = PhoneNumber.addSMSDomainIfPhoneNumber(currentUserLogin);
const participantAccountIDs = participants.map((participant) => Number(participant.accountID));
Expand All @@ -4333,8 +4356,8 @@ function startSplitBill({
undefined,
category,
tag,
undefined,
undefined,
taxCode,
taxAmount,
billable,
);

Expand Down Expand Up @@ -4576,6 +4599,8 @@ function startSplitBill({
billable,
...(existingSplitChatReport ? {} : {createdReportActionID: splitChatCreatedReportAction.reportActionID}),
chatType: splitChatReport?.chatType,
taxCode,
taxAmount,
};

API.write(WRITE_COMMANDS.START_SPLIT_BILL, parameters, {optimisticData, successData, failureData});
Expand Down Expand Up @@ -4660,9 +4685,11 @@ function completeSplitBill(chatReportID: string, reportAction: OnyxTypes.ReportA
const splitParticipants: Split[] = updatedTransaction?.comment.splits ?? [];
const amount = updatedTransaction?.modifiedAmount;
const currency = updatedTransaction?.modifiedCurrency;
console.debug(updatedTransaction);

// Exclude the current user when calculating the split amount, `calculateAmount` takes it into account
const splitAmount = IOUUtils.calculateAmount(splitParticipants.length - 1, amount ?? 0, currency ?? '', false);
const splitTaxAmount = IOUUtils.calculateAmount(splitParticipants.length - 1, updatedTransaction?.taxAmount ?? 0, currency ?? '', false);

const splits: Split[] = [{email: currentUserEmailForIOUSplit}];
splitParticipants.forEach((participant) => {
Expand Down Expand Up @@ -4726,8 +4753,8 @@ function completeSplitBill(chatReportID: string, reportAction: OnyxTypes.ReportA
undefined,
updatedTransaction?.category,
updatedTransaction?.tag,
undefined,
undefined,
updatedTransaction?.taxCode,
isPolicyExpenseChat ? -splitTaxAmount : splitAmount,
updatedTransaction?.billable,
);

Expand Down Expand Up @@ -4801,6 +4828,8 @@ function completeSplitBill(chatReportID: string, reportAction: OnyxTypes.ReportA
comment: transactionComment,
category: transactionCategory,
tag: transactionTag,
taxCode: transactionTaxCode,
taxAmount: transactionTaxAmount,
} = ReportUtils.getTransactionDetails(updatedTransaction) ?? {};

const parameters: CompleteSplitBillParams = {
Expand All @@ -4813,6 +4842,8 @@ function completeSplitBill(chatReportID: string, reportAction: OnyxTypes.ReportA
category: transactionCategory,
tag: transactionTag,
splits: JSON.stringify(splits),
taxCode: transactionTaxCode,
taxAmount: transactionTaxAmount,
};

API.write(WRITE_COMMANDS.COMPLETE_SPLIT_BILL, parameters, {optimisticData, successData, failureData});
Expand Down
Loading
Loading