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

Feat: Add Billable Toggle UI #27172

Merged
merged 16 commits into from
Sep 18, 2023
Merged
15 changes: 15 additions & 0 deletions src/components/MoneyRequestConfirmationList.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {format} from 'date-fns';
import _ from 'underscore';
import {View} from 'react-native';
import lodashGet from 'lodash/get';
import Text from './Text';
import styles from '../styles/styles';
import * as ReportUtils from '../libs/ReportUtils';
import * as OptionsListUtils from '../libs/OptionsListUtils';
Expand All @@ -30,6 +31,7 @@ import Image from './Image';
import useLocalize from '../hooks/useLocalize';
import * as ReceiptUtils from '../libs/ReceiptUtils';
import categoryPropTypes from './categoryPropTypes';
import Switch from './Switch';
import tagPropTypes from './tagPropTypes';
import ConfirmedRoute from './ConfirmedRoute';
import transactionPropTypes from './transactionPropTypes';
Expand Down Expand Up @@ -530,6 +532,16 @@ function MoneyRequestConfirmationList(props) {
disabled={didConfirm || props.isReadOnly}
/>
)}
{canUseTags && !lodashGet(props.policy, 'disabledFields.defaultBillable', true) && (
Copy link
Contributor

Choose a reason for hiding this comment

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

To confirm: billable toggle is only for expense request (workspace) right? Not applicable to IOU request (DM, group)?

Copy link
Contributor Author

@waterim waterim Sep 14, 2023

Choose a reason for hiding this comment

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

As I understand yes
@amyevans to be sure

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, correct. An IOU request doesn't have an associated policy ID so the field is never going to show for one, which is intended.

Copy link
Contributor

Choose a reason for hiding this comment

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

Whoops I was too slow. Thanks Puneet 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Also to confirm that this uses same newDotTags beta permission?

<View style={[styles.flexRow, styles.mb4, styles.justifyContentBetween, styles.alignItemsCenter, styles.ml5, styles.mr8]}>
<Text color={!props.iouIsBillable ? themeColors.textSupporting : undefined}>{translate('common.billable')}</Text>
<Switch
accessibilityLabel={translate('common.billable')}
isOn={props.iouIsBillable}
onToggle={props.onToggleBillable}
Copy link
Contributor

Choose a reason for hiding this comment

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

It appears this was never added to the propType definitions. Can you please add it?

Copy link
Contributor

Choose a reason for hiding this comment

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

In progress here: #29327

Thanks for flagging!

Comment on lines +538 to +541
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we missed to disable switch its readonly, caused #43756

/>
</View>
)}
</>
)}
</OptionsSelector>
Expand Down Expand Up @@ -561,5 +573,8 @@ export default compose(
transaction: {
key: ({transactionID}) => `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`,
},
policy: {
key: ({policyID}) => `${ONYXKEYS.COLLECTION.POLICY}${policyID}`,
},
}),
)(MoneyRequestConfirmationList);
1 change: 1 addition & 0 deletions src/languages/en.ts
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,7 @@ export default {
showMore: 'Show more',
merchant: 'Merchant',
category: 'Category',
billable: 'Billable',
tag: 'Tag',
receipt: 'Receipt',
replace: 'Replace',
Expand Down
1 change: 1 addition & 0 deletions src/languages/es.ts
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,7 @@ export default {
showMore: 'Mostrar más',
merchant: 'Comerciante',
category: 'Categoría',
billable: 'Facturable',
tag: 'Etiqueta',
receipt: 'Recibo',
replace: 'Sustituir',
Expand Down
3 changes: 3 additions & 0 deletions src/libs/TransactionUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ Onyx.connect({
* @param {String} [filename]
* @param {String} [existingTransactionID] When creating a distance request, an empty transaction has already been created with a transactionID. In that case, the transaction here needs to have it's transactionID match what was already generated.
* @param {String} [category]
* @param {Boolean} [defaultBillable]
* @returns {Object}
*/
function buildOptimisticTransaction(
Expand All @@ -49,6 +50,7 @@ function buildOptimisticTransaction(
filename = '',
existingTransactionID = null,
category = '',
defaultBillable = false,
waterim marked this conversation as resolved.
Show resolved Hide resolved
) {
// transactionIDs are random, positive, 64-bit numeric strings.
// Because JS can only handle 53-bit numbers, transactionIDs are strings in the front-end (just like reportActionID)
Expand Down Expand Up @@ -77,6 +79,7 @@ function buildOptimisticTransaction(
receipt,
filename,
category,
defaultBillable,
};
}

Expand Down
17 changes: 16 additions & 1 deletion src/libs/actions/IOU.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ function resetMoneyRequestInfo(id = '') {
receiptPath: '',
receiptSource: '',
transactionID: '',
defaultBillable: '',
waterim marked this conversation as resolved.
Show resolved Hide resolved
});
}

Expand Down Expand Up @@ -338,6 +339,7 @@ function buildOnyxDataForMoneyRequest(
* @param {Object} [receipt]
* @param {String} [existingTransactionID]
* @param {String} [category]
* @param {Boolean} defaultBillable
* @returns {Object} data
* @returns {String} data.payerEmail
* @returns {Object} data.iouReport
Expand Down Expand Up @@ -365,6 +367,7 @@ function getMoneyRequestInformation(
receipt = undefined,
existingTransactionID = undefined,
category = undefined,
defaultBillable = undefined,
) {
const payerEmail = OptionsListUtils.addSMSDomainIfPhoneNumber(participant.login);
const payerAccountID = Number(participant.accountID);
Expand Down Expand Up @@ -430,6 +433,7 @@ function getMoneyRequestInformation(
filename,
existingTransactionID,
category,
defaultBillable,
);

const uniquePolicyRecentlyUsedCategories = allRecentlyUsedCategories
Expand Down Expand Up @@ -597,8 +601,9 @@ function createDistanceRequest(report, participant, comment, created, transactio
* @param {String} comment
* @param {Object} [receipt]
* @param {String} [category]
* @param {Boolean} [defaultBillable]
*/
function requestMoney(report, amount, currency, created, merchant, payeeEmail, payeeAccountID, participant, comment, receipt = undefined, category = undefined) {
function requestMoney(report, amount, currency, created, merchant, payeeEmail, payeeAccountID, participant, comment, receipt = undefined, category = undefined, defaultBillable = undefined) {
// If the report is iou or expense report, we should get the linked chat report to be passed to the getMoneyRequestInformation function
const isMoneyRequestReport = ReportUtils.isMoneyRequestReport(report);
const currentChatReport = isMoneyRequestReport ? ReportUtils.getReport(report.chatReportID) : report;
Expand All @@ -615,6 +620,7 @@ function requestMoney(report, amount, currency, created, merchant, payeeEmail, p
receipt,
undefined,
category,
defaultBillable,
);

API.write(
Expand All @@ -635,6 +641,7 @@ function requestMoney(report, amount, currency, created, merchant, payeeEmail, p
reportPreviewReportActionID: reportPreviewAction.reportActionID,
receipt,
category,
billable: defaultBillable,
},
onyxData,
);
Expand Down Expand Up @@ -1945,6 +1952,13 @@ function resetMoneyRequestCategory() {
Onyx.merge(ONYXKEYS.IOU, {category: ''});
}

/**
* @param {Boolean} defaultBillable
*/
function setMoneyRequestDefaultBillable(defaultBillable) {
Onyx.merge(ONYXKEYS.IOU, {defaultBillable});
}

/**
* @param {Object[]} participants
*/
Expand Down Expand Up @@ -2027,6 +2041,7 @@ export {
setMoneyRequestMerchant,
setMoneyRequestCategory,
resetMoneyRequestCategory,
setMoneyRequestDefaultBillable,
setMoneyRequestParticipants,
setMoneyRequestReceipt,
createEmptyTransaction,
Expand Down
10 changes: 10 additions & 0 deletions src/pages/iou/steps/MoneyRequestConfirmPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ function MoneyRequestConfirmPage(props) {
if (policyExpenseChat) {
Policy.openDraftWorkspaceRequest(policyExpenseChat.policyID);
}
if (typeof props.iou.defaultBillable !== 'boolean') IOU.setMoneyRequestDefaultBillable(lodashGet(props.policy, 'defaultBillable', false));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (typeof props.iou.defaultBillable !== 'boolean') IOU.setMoneyRequestDefaultBillable(lodashGet(props.policy, 'defaultBillable', false));
if (typeof props.iou.defaultBillable !== 'boolean') {
IOU.setMoneyRequestDefaultBillable(lodashGet(props.policy, 'defaultBillable', false));
}

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to the code style request, and also please add a comment explaining why we're doing this

// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

Expand Down Expand Up @@ -135,6 +136,7 @@ function MoneyRequestConfirmPage(props) {
trimmedComment,
receipt,
props.iou.category,
props.iou.defaultBillable,
);
},
[
Expand All @@ -146,6 +148,7 @@ function MoneyRequestConfirmPage(props) {
props.currentUserPersonalDetails.login,
props.currentUserPersonalDetails.accountID,
props.iou.category,
props.iou.defaultBillable,
],
);

Expand Down Expand Up @@ -283,6 +286,8 @@ function MoneyRequestConfirmPage(props) {
iouAmount={props.iou.amount}
iouComment={props.iou.comment}
iouCurrencyCode={props.iou.currency}
iouIsBillable={props.iou.defaultBillable}
onToggleBillable={IOU.setMoneyRequestDefaultBillable}
iouCategory={props.iou.category}
iouTag={props.iou.tag}
onConfirm={createTransaction}
Expand Down Expand Up @@ -352,4 +357,9 @@ export default compose(
key: `${ONYXKEYS.COLLECTION.SELECTED_TAB}${CONST.TAB.RECEIPT_TAB_ID}`,
},
}),
withOnyx({
policy: {
key: ({report}) => `${ONYXKEYS.COLLECTION.POLICY}${report.policyID}`,
},
}),
Comment on lines +363 to +367
Copy link
Contributor

Choose a reason for hiding this comment

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

This caused a regression #28182. The report object may be undefined and accessing the policyID on an undefined crashes the app.

)(MoneyRequestConfirmPage);
Loading