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

Handle tax for split requests #40240

Merged
merged 19 commits into from
May 24, 2024
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
c611187
Pass taxCode and taxAmount params for split bill request
MonilBhavsar Apr 15, 2024
c7ec5f2
Merge branch 'main' into monil-splitBillTaxUpdate
MonilBhavsar Apr 17, 2024
8cb3a49
Merge branch 'main' into monil-splitBillTaxUpdate
MonilBhavsar May 13, 2024
bb6d480
Merge branch 'monil-splitBillTaxUpdate' of github.com:Expensify/App i…
MonilBhavsar May 13, 2024
98e0e40
Merge branch 'main' into monil-splitBillTaxUpdate
MonilBhavsar May 15, 2024
477b381
Split amount based on new logic and update optimistic transaction
MonilBhavsar May 15, 2024
ec48476
Fix lint
MonilBhavsar May 15, 2024
bd20eed
Merge branch 'main' into monil-splitBillTaxUpdate
MonilBhavsar May 16, 2024
de50156
Fix bug when selectedReportID and hence policy is undefined on confir…
MonilBhavsar May 16, 2024
c3db0c8
Merge branch 'main' into monil-splitBillTaxUpdate
MonilBhavsar May 17, 2024
c667482
Fix taxAmount not being calculated
MonilBhavsar May 21, 2024
cdfb3ad
Handle editing split bill case
MonilBhavsar May 21, 2024
78bf98a
Handle scan split requests optimistically
MonilBhavsar May 21, 2024
776856c
Fix lint
MonilBhavsar May 21, 2024
6dd21cb
Check for ownerAccountID in case of split with policy
MonilBhavsar May 22, 2024
c7d8b42
Merge branch 'main' into monil-splitBillTaxUpdate
MonilBhavsar May 23, 2024
787c520
Fix taxAmount is negative optimistically
MonilBhavsar May 23, 2024
d136b4a
Fix lint and typecheck
MonilBhavsar May 23, 2024
c671c7a
Fix bug when updating taxAmount and refactor
MonilBhavsar May 23, 2024
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
11 changes: 3 additions & 8 deletions src/components/MoneyRequestConfirmationList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -315,8 +315,8 @@
const formattedTaxAmount = CurrencyUtils.convertToDisplayString(transaction?.taxAmount, iouCurrencyCode);
const taxRateTitle = TransactionUtils.getTaxName(policy, transaction);

const previousTransactionAmount = usePrevious(transaction?.amount);

Check failure on line 318 in src/components/MoneyRequestConfirmationList.tsx

View workflow job for this annotation

GitHub Actions / Run ESLint

'previousTransactionAmount' is assigned a value but never used
const previousTransactionCurrency = usePrevious(transaction?.currency);

Check failure on line 319 in src/components/MoneyRequestConfirmationList.tsx

View workflow job for this annotation

GitHub Actions / Run ESLint

'previousTransactionCurrency' is assigned a value but never used

const isFocused = useIsFocused();
const [formError, debouncedFormError, setFormError] = useDebouncedState('');
Expand Down Expand Up @@ -372,14 +372,9 @@
// 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);
}
Comment on lines -377 to -379
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mostly think this was pre optimization. When the user updates transactionAmount, previousTransactionAmount was not being changed, and we we were not setting updated taxAmount in Onyx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this introduced a bug where taxAmount is not being updated in draft transaction. 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.

This is fixed now with latest commit - c671c7a


IOU.setMoneyRequestTaxAmount(transactionID, amountInSmallestCurrencyUnits, true);
}, [policy, transaction, transactionID, previousTransactionAmount, previousTransactionCurrency]);
const taxAmountInSmallestCurrencyUnits = CurrencyUtils.convertToBackendAmount(Number.parseFloat(taxAmount));
IOU.setMoneyRequestTaxAmount(transactionID, taxAmountInSmallestCurrencyUnits, true);
}, [policy, transaction, transactionID]);

// If completing a split expense fails, set didConfirm to false to allow the user to edit the fields again
if (isEditingSplitBill && didConfirm) {
Expand Down
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;
29 changes: 24 additions & 5 deletions src/libs/actions/IOU.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3726,6 +3726,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 @@ -3747,8 +3749,8 @@ function createSplitsAndOnyxData(
undefined,
category,
tag,
undefined,
undefined,
taxCode,
taxAmount,
billable,
);

Expand Down Expand Up @@ -3903,14 +3905,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 @@ -3980,8 +3984,8 @@ function createSplitsAndOnyxData(
undefined,
category,
tag,
undefined,
undefined,
taxCode,
splitTaxAmount,
billable,
);

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

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

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

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

API.write(WRITE_COMMANDS.SPLIT_BILL, parameters, onyxData);
Expand All @@ -4210,6 +4223,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 @@ -4227,6 +4242,8 @@ function splitBillAndOpenReport({
'',
billable,
iouRequestType,
taxCode,
taxAmount,
);

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

API.write(WRITE_COMMANDS.SPLIT_BILL_AND_OPEN_REPORT, parameters, onyxData);
Expand Down
36 changes: 14 additions & 22 deletions src/pages/iou/request/step/IOURequestStepAmount.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import withFullTransactionOrNotFound from './withFullTransactionOrNotFound';
import type {WithWritableReportOrNotFoundProps} from './withWritableReportOrNotFound';
import withWritableReportOrNotFound from './withWritableReportOrNotFound';
import lodashIsEmpty from "lodash/isEmpty";

Check failure on line 29 in src/pages/iou/request/step/IOURequestStepAmount.tsx

View workflow job for this annotation

GitHub Actions / Run ESLint

`lodash/isEmpty` import should occur before import of `./StepScreenWrapper`

Check failure on line 29 in src/pages/iou/request/step/IOURequestStepAmount.tsx

View workflow job for this annotation

GitHub Actions / Run ESLint

'lodashIsEmpty' is defined but never used

type AmountParams = {
amount: string;
Expand All @@ -39,9 +40,6 @@
/** Whether the confirmation step should be skipped */
skipConfirmation: OnyxEntry<boolean>;

/** The draft transaction object being modified in Onyx */
draftTransaction: OnyxEntry<OnyxTypes.Transaction>;

/** Personal details of all users */
personalDetails: OnyxEntry<OnyxTypes.PersonalDetailsList>;

Expand All @@ -67,7 +65,6 @@
currentUserPersonalDetails,
splitDraftTransaction,
skipConfirmation,
draftTransaction,
}: IOURequestStepAmountProps) {
const {translate} = useLocalize();
const textInput = useRef<BaseTextInputRef | null>(null);
Expand All @@ -78,8 +75,9 @@
const isEditing = action === CONST.IOU.ACTION.EDIT;
const isSplitBill = iouType === CONST.IOU.TYPE.SPLIT;
const isEditingSplitBill = isEditing && isSplitBill;
const {amount: transactionAmount} = ReportUtils.getTransactionDetails(isEditingSplitBill && !isEmptyObject(splitDraftTransaction) ? splitDraftTransaction : transaction) ?? {amount: 0};
const {currency: originalCurrency} = ReportUtils.getTransactionDetails(isEditing ? draftTransaction : transaction) ?? {currency: CONST.CURRENCY.USD};
const currentTransaction = isEditingSplitBill && !isEmptyObject(splitDraftTransaction) ? splitDraftTransaction : transaction;
const {amount: transactionAmount} = ReportUtils.getTransactionDetails(currentTransaction) ?? {amount: 0};
const {currency: originalCurrency} = ReportUtils.getTransactionDetails(currentTransaction) ?? {currency: CONST.CURRENCY.USD};
const currency = CurrencyUtils.isValidCurrencyCode(selectedCurrency) ? selectedCurrency : originalCurrency;

// For quick button actions, we'll skip the confirmation page unless the report is archived or this is a workspace request, as
Expand Down Expand Up @@ -269,25 +267,25 @@
}

// If the value hasn't changed, don't request to save changes on the server and just close the modal
const transactionCurrency = TransactionUtils.getCurrency(transaction);
if (newAmount === TransactionUtils.getAmount(transaction) && currency === transactionCurrency) {
const transactionCurrency = TransactionUtils.getCurrency(currentTransaction);
if (newAmount === TransactionUtils.getAmount(currentTransaction) && currency === transactionCurrency) {
Navigation.dismissModal();
return;
}

// If currency has changed, then we get the default tax rate based on currency, otherwise we use the current tax rate selected in transaction, if we have it.
const transactionTaxCode = ReportUtils.getTransactionDetails(currentTransaction)?.taxCode;
const defaultTaxCode = TransactionUtils.getDefaultTaxCode(policy, currentTransaction, currency) ?? '';
const taxCode = (currency !== transactionCurrency ? defaultTaxCode : transactionTaxCode) ?? defaultTaxCode;
const taxPercentage = TransactionUtils.getTaxValue(policy, currentTransaction, taxCode) ?? '';
const taxAmount = CurrencyUtils.convertToBackendAmount(TransactionUtils.calculateTaxAmount(taxPercentage, newAmount));

if (isSplitBill) {
IOU.setDraftSplitTransaction(transactionID, {amount: newAmount, currency});
IOU.setDraftSplitTransaction(transactionID, {amount: newAmount, currency, taxCode, taxAmount});
Navigation.goBack(backTo);
return;
}

// If currency has changed, then we get the default tax rate based on currency, otherwise we use the current tax rate selected in transaction, if we have it.
const transactionTaxCode = transaction?.taxCode ?? '';
const defaultTaxCode = TransactionUtils.getDefaultTaxCode(policy, transaction, currency) ?? '';
const taxCode = (currency !== transactionCurrency ? defaultTaxCode : transactionTaxCode) ?? defaultTaxCode;
const taxPercentage = TransactionUtils.getTaxValue(policy, transaction, taxCode) ?? '';
const taxAmount = CurrencyUtils.convertToBackendAmount(TransactionUtils.calculateTaxAmount(taxPercentage, newAmount));

IOU.updateMoneyRequestAmountAndCurrency({transactionID, transactionThreadReportID: reportID, currency, amount: newAmount, taxAmount, policy, taxCode});
Navigation.dismissModal();
};
Expand Down Expand Up @@ -326,12 +324,6 @@
return `${ONYXKEYS.COLLECTION.SPLIT_TRANSACTION_DRAFT}${transactionID}`;
},
},
draftTransaction: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

withFullTransactionOrNotFound() returns draftTransaction or transaction depending upon condition. So I think we can remove this

key: ({route}) => {
const transactionID = route.params.transactionID ?? 0;
return `${ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}${transactionID}`;
},
},
skipConfirmation: {
key: ({route}) => {
const transactionID = route.params.transactionID ?? 0;
Expand Down
6 changes: 6 additions & 0 deletions src/pages/iou/request/step/IOURequestStepConfirmation.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,8 @@ function IOURequestStepConfirmation({
iouRequestType: transaction.iouRequestType,
splitShares: transaction.splitShares,
splitPayerAccountIDs: transaction.splitPayerAccountIDs ?? [],
taxCode: transactionTaxCode,
taxAmount: transactionTaxAmount,
});
}
return;
Expand All @@ -398,6 +400,8 @@ function IOURequestStepConfirmation({
iouRequestType: transaction.iouRequestType,
splitShares: transaction.splitShares,
splitPayerAccountIDs: transaction.splitPayerAccountIDs,
taxCode: transactionTaxCode,
taxAmount: transactionTaxAmount,
});
}
return;
Expand Down Expand Up @@ -493,6 +497,8 @@ function IOURequestStepConfirmation({
policy,
policyTags,
policyCategories,
transactionTaxAmount,
transactionTaxCode,
],
);

Expand Down
4 changes: 3 additions & 1 deletion src/pages/iou/request/step/IOURequestStepParticipants.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ function IOURequestStepParticipants({
const participants = transaction?.participants;
const {translate} = useLocalize();
const styles = useThemeStyles();
const selectedReportID = useRef<string>(reportID);

// We need to set selectedReportID if user has navigated back from confirmation page and navigates to confirmation page with already selected participant
const selectedReportID = useRef<string>(participants?.length === 1 ? participants[0]?.reportID ?? reportID : reportID);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bug was

  1. User enters amount
  2. Selects participants and clicks "Next"
  3. Goes to confirmation page and see's Category, Tags and Tax fields
  4. User goes back to 1 and edit's amount(doesn't matter)
  5. User without selecting a participant clicks "Next" because participant was already selected(This is root cause)
  6. Goes to confirmation page and doesn't see Category, Tags and Tax fields because reportID and policyID is not properly set from 5 above.

Let me know if you think this is not the correct way and should use separate hook or may be something else. Open to other solutions.

const numberOfParticipants = useRef(participants?.length ?? 0);
const iouRequestType = TransactionUtils.getRequestType(transaction);
const isSplitRequest = iouType === CONST.IOU.TYPE.SPLIT;
Expand Down
28 changes: 23 additions & 5 deletions src/pages/iou/request/step/IOURequestStepTaxAmountPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,17 @@
import withFullTransactionOrNotFound from './withFullTransactionOrNotFound';
import type {WithWritableReportOrNotFoundProps} from './withWritableReportOrNotFound';
import withWritableReportOrNotFound from './withWritableReportOrNotFound';
import {isEmptyObject} from "@src/types/utils/EmptyObject";

Check failure on line 23 in src/pages/iou/request/step/IOURequestStepTaxAmountPage.tsx

View workflow job for this annotation

GitHub Actions / Run ESLint

`@src/types/utils/EmptyObject` import should occur before import of `./StepScreenWrapper`

type IOURequestStepTaxAmountPageOnyxProps = {
policy: OnyxEntry<Policy>;
policyCategories: OnyxEntry<PolicyCategories>;

/** Collection of tag list on a policy */
policyTags: OnyxEntry<PolicyTagList>;

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

type IOURequestStepTaxAmountPageProps = IOURequestStepTaxAmountPageOnyxProps &
Expand Down Expand Up @@ -58,14 +62,16 @@
policy,
policyTags,
policyCategories,
splitDraftTransaction,
}: IOURequestStepTaxAmountPageProps) {
const {translate} = useLocalize();
const textInput = useRef<BaseTextInputRef | null>();
const isEditing = action === CONST.IOU.ACTION.EDIT;
const isEditingSplitBill = isEditing && iouType === CONST.IOU.TYPE.SPLIT;

const focusTimeoutRef = useRef<NodeJS.Timeout>();

const transactionDetails = ReportUtils.getTransactionDetails(transaction);
const transactionDetails = ReportUtils.getTransactionDetails(isEditingSplitBill && !isEmptyObject(splitDraftTransaction) ? splitDraftTransaction : transaction);
const currency = CurrencyUtils.isValidCurrencyCode(selectedCurrency) ? selectedCurrency : transactionDetails?.currency;

useFocusEffect(
Expand Down Expand Up @@ -102,19 +108,25 @@
};

const updateTaxAmount = (currentAmount: CurrentMoney) => {
const amountInSmallestCurrencyUnits = CurrencyUtils.convertToBackendAmount(Number.parseFloat(currentAmount.amount));
const taxAmountInSmallestCurrencyUnits = CurrencyUtils.convertToBackendAmount(Number.parseFloat(currentAmount.amount));

if (isEditingSplitBill) {
IOU.setDraftSplitTransaction(transactionID, {taxAmount: taxAmountInSmallestCurrencyUnits});
navigateBack();
return;
}

if (isEditing) {
if (amountInSmallestCurrencyUnits === TransactionUtils.getTaxAmount(transaction, false)) {
if (taxAmountInSmallestCurrencyUnits === TransactionUtils.getTaxAmount(transaction, false)) {
navigateBack();
return;
}
IOU.updateMoneyRequestTaxAmount(transactionID, report?.reportID ?? '', amountInSmallestCurrencyUnits, policy, policyTags, policyCategories);
IOU.updateMoneyRequestTaxAmount(transactionID, report?.reportID ?? '', taxAmountInSmallestCurrencyUnits, policy, policyTags, policyCategories);
navigateBack();
return;
}

IOU.setMoneyRequestTaxAmount(transactionID, amountInSmallestCurrencyUnits, true);
IOU.setMoneyRequestTaxAmount(transactionID, taxAmountInSmallestCurrencyUnits, true);

// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
IOU.setMoneyRequestCurrency(transactionID, currency || CONST.CURRENCY.USD);
Expand Down Expand Up @@ -164,6 +176,12 @@
IOURequestStepTaxAmountPage.displayName = 'IOURequestStepTaxAmountPage';

const IOURequestStepTaxAmountPageWithOnyx = withOnyx<IOURequestStepTaxAmountPageProps, IOURequestStepTaxAmountPageOnyxProps>({
splitDraftTransaction: {
key: ({route}) => {
const transactionID = route?.params.transactionID ?? 0;
return `${ONYXKEYS.COLLECTION.SPLIT_TRANSACTION_DRAFT}${transactionID}`;
},
},
policy: {
key: ({report}) => `${ONYXKEYS.COLLECTION.POLICY}${report ? report.policyID : '0'}`,
},
Expand Down
Loading
Loading