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

Cleanup completing a split bill #29568

Merged
merged 25 commits into from
Oct 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
d940b33
Fix proptypes
youssef-lr Oct 13, 2023
ce96b01
Add configurable option to OptionsSelector and make split details scr…
youssef-lr Oct 13, 2023
ae0a3b6
Show receipt scanning message for splits as well
youssef-lr Oct 13, 2023
8cf9b1e
Do not display last message from backend if report has no actions
youssef-lr Oct 13, 2023
e2a5657
Only make BaseOptionsSelector scrollable when inputs are below the op…
youssef-lr Oct 13, 2023
a36590f
Merge branch 'main' into youssef_cleanup_completeSplitBill
youssef-lr Oct 13, 2023
1127b20
Fix proptype name
youssef-lr Oct 13, 2023
c35cbc6
Address comments
youssef-lr Oct 13, 2023
4983055
Add partial merchant when creating transaction
youssef-lr Oct 14, 2023
e622ab6
CLeanup checking for missing fields
youssef-lr Oct 14, 2023
d52729d
Remove extra component
youssef-lr Oct 14, 2023
9e2e935
Make isEditingSplitBill depend on only the fields being empty
youssef-lr Oct 14, 2023
87851c7
Merge branch 'main' into youssef_cleanup_completeSplitBill
youssef-lr Oct 14, 2023
fc1e3ff
Fix missing money request status details
youssef-lr Oct 14, 2023
696de5d
Disable nested scroll on OptionsList used in MoneyRequestConfirmation…
youssef-lr Oct 15, 2023
072c633
tidy up
youssef-lr Oct 15, 2023
249d904
Use scrollViewEnabled as well to disable scroll on iOS
youssef-lr Oct 15, 2023
984e5e8
Fix error translation key
youssef-lr Oct 15, 2023
630cfd3
Translation fix
youssef-lr Oct 15, 2023
cbfa239
Allow options to expand to full height
youssef-lr Oct 15, 2023
2a406ca
Disable bounce effect on money request options to fix scroll issues
youssef-lr Oct 15, 2023
43d8dcb
Fix lint
youssef-lr Oct 15, 2023
6f47cf8
Another lint fix
youssef-lr Oct 15, 2023
18769cf
Remove redundant prop
youssef-lr Oct 15, 2023
71064ed
Update src/components/MoneyRequestConfirmationList.js
youssef-lr Oct 16, 2023
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
24 changes: 10 additions & 14 deletions src/components/MoneyRequestConfirmationList.js
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,9 @@ const propTypes = {
/** Whether the money request is a scan request */
isScanRequest: PropTypes.bool,

/** Whether we're editing a split bill */
isEditingSplitBill: PropTypes.bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

For the future, please name the props for the effect they should have inside the component. When only reviewing code inside this component, if you saw the prop isEditingSplitBill, you would have no idea what that prop should do or how it should be applied. For example, the prop below is called shouldShowSmartScanFields which is much more explicit and its purpose is understood immediately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @tgolen - sorry for the late reply! Will keep this in mind, however, I think it's a bit difficult in this component as it handles different IOUs. What would be a better naming here?


/** Whether we should show the amount, date, and merchant fields. */
shouldShowSmartScanFields: PropTypes.bool,

Expand Down Expand Up @@ -554,6 +557,7 @@ function MoneyRequestConfirmationList(props) {
optionHoveredStyle={canModifyParticipants ? styles.hoveredComponentBG : {}}
footerContent={footerContent}
listStyles={props.listStyles}
shouldAllowScrollingChildren
>
{props.isDistanceRequest && (
<View style={styles.confirmationListMapItem}>
Expand Down Expand Up @@ -589,8 +593,8 @@ function MoneyRequestConfirmationList(props) {
style={[styles.moneyRequestMenuItem, styles.mt2]}
titleStyle={styles.moneyRequestConfirmationAmount}
disabled={didConfirm}
brickRoadIndicator={shouldDisplayFieldError && !transaction.modifiedAmount ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''}
error={shouldDisplayFieldError && !transaction.modifiedAmount ? translate('common.error.enterAmount') : ''}
brickRoadIndicator={shouldDisplayFieldError && TransactionUtils.isAmountMissing(transaction) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''}
error={shouldDisplayFieldError && TransactionUtils.isAmountMissing(transaction) ? translate('common.error.enterAmount') : ''}
/>
)}
<MenuItemWithTopDescription
Expand Down Expand Up @@ -644,8 +648,8 @@ function MoneyRequestConfirmationList(props) {
}}
disabled={didConfirm}
interactive={!props.isReadOnly}
brickRoadIndicator={shouldDisplayFieldError && _.isEmpty(transaction.modifiedCreated) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''}
error={shouldDisplayFieldError && _.isEmpty(transaction.modifiedCreated) ? translate('common.error.enterDate') : ''}
brickRoadIndicator={shouldDisplayFieldError && TransactionUtils.isCreatedMissing(transaction) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''}
error={shouldDisplayFieldError && TransactionUtils.isCreatedMissing(transaction) ? translate('common.error.enterDate') : ''}
/>
)}
{props.isDistanceRequest && (
Expand Down Expand Up @@ -676,16 +680,8 @@ function MoneyRequestConfirmationList(props) {
}}
disabled={didConfirm}
interactive={!props.isReadOnly}
brickRoadIndicator={
shouldDisplayFieldError && (transaction.modifiedMerchant === '' || transaction.modifiedMerchant === CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT)
? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR
: ''
}
error={
shouldDisplayFieldError && (transaction.modifiedMerchant === '' || transaction.modifiedMerchant === CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT)
? translate('common.error.enterMerchant')
: ''
}
brickRoadIndicator={shouldDisplayFieldError && TransactionUtils.isMerchantMissing(transaction) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''}
error={shouldDisplayFieldError && TransactionUtils.isMerchantMissing(transaction) ? translate('common.error.enterMerchant') : ''}
/>
)}
{shouldShowCategories && (
Expand Down
6 changes: 5 additions & 1 deletion src/components/OptionsList/BaseOptionsList.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ function BaseOptionsList({
innerRef,
isRowMultilineSupported,
isLoadingNewOptions,
nestedScrollEnabled,
bounces,
}) {
const flattenedData = useRef();
const previousSections = usePrevious(sections);
Expand Down Expand Up @@ -255,11 +257,12 @@ function BaseOptionsList({
) : null}
<SectionList
ref={innerRef}
nestedScrollEnabled
style={listStyles}
indicatorStyle="white"
keyboardShouldPersistTaps="always"
keyboardDismissMode={keyboardDismissMode}
nestedScrollEnabled={nestedScrollEnabled}
scrollEnabled={nestedScrollEnabled}
onScrollBeginDrag={onScrollBeginDrag}
onScroll={onScroll}
contentContainerStyle={contentContainerStyles}
Expand All @@ -276,6 +279,7 @@ function BaseOptionsList({
windowSize={5}
viewabilityConfig={{viewAreaCoveragePercentThreshold: 95}}
onViewableItemsChanged={onViewableItemsChanged}
bounces={bounces}
/>
</>
)}
Expand Down
8 changes: 8 additions & 0 deletions src/components/OptionsList/optionsListPropTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,12 @@ const propTypes = {

/** Whether we are loading new options */
isLoadingNewOptions: PropTypes.bool,

/** Whether nested scroll of options is enabled, true by default */
nestedScrollEnabled: PropTypes.bool,

/** Whether the list should have a bounce effect on iOS */
bounces: PropTypes.bool,
};

const defaultProps = {
Expand Down Expand Up @@ -117,6 +123,8 @@ const defaultProps = {
showScrollIndicator: false,
isRowMultilineSupported: false,
isLoadingNewOptions: false,
nestedScrollEnabled: true,
bounces: true,
};

export {propTypes, defaultProps};
44 changes: 34 additions & 10 deletions src/components/OptionsSelector/BaseOptionsSelector.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import _ from 'underscore';
import lodashGet from 'lodash/get';
import React, {Component} from 'react';
import PropTypes from 'prop-types';
import {View} from 'react-native';
import {ScrollView, View} from 'react-native';
import Button from '../Button';
import FixedFooter from '../FixedFooter';
import OptionsList from '../OptionsList';
Expand Down Expand Up @@ -432,8 +432,21 @@ class BaseOptionsSelector extends Component {
isRowMultilineSupported={this.props.isRowMultilineSupported}
isLoadingNewOptions={this.props.isLoadingNewOptions}
shouldPreventDefaultFocusOnSelectRow={this.props.shouldPreventDefaultFocusOnSelectRow}
nestedScrollEnabled={this.props.nestedScrollEnabled}
bounces={!this.props.shouldTextInputAppearBelowOptions || !this.props.shouldAllowScrollingChildren}
/>
);

const optionsAndInputsBelowThem = (
<>
<View style={[styles.flexGrow0, styles.flexShrink1, styles.flexBasisAuto, styles.w100, styles.flexRow]}>{optionsList}</View>
<View style={this.props.shouldUseStyleForChildren ? [styles.ph5, styles.pv5, styles.flexGrow1, styles.flexShrink0] : []}>
{this.props.children}
{this.props.shouldShowTextInput && textInput}
</View>
</>
);

return (
<ArrowKeyFocusManager
disabledIndexes={this.disabledOptionsIndexes}
Expand All @@ -443,15 +456,26 @@ class BaseOptionsSelector extends Component {
shouldResetIndexOnEndReached={false}
>
<View style={[styles.flexGrow1, styles.flexShrink1, styles.flexBasisAuto]}>
{this.props.shouldTextInputAppearBelowOptions ? (
<>
<View style={[styles.flexGrow0, styles.flexShrink1, styles.flexBasisAuto, styles.w100, styles.flexRow]}>{optionsList}</View>
<View style={this.props.shouldUseStyleForChildren ? [styles.ph5, styles.pv5, styles.flexGrow1, styles.flexShrink0] : []}>
{this.props.children}
{this.props.shouldShowTextInput && textInput}
</View>
</>
) : (
{/*
* The OptionsList component uses a SectionList which uses a VirtualizedList internally.
* VirtualizedList cannot be directly nested within ScrollViews of the same orientation.
* To work around this, we wrap the OptionsList component with a horizontal ScrollView.
*/}
{this.props.shouldTextInputAppearBelowOptions && this.props.shouldAllowScrollingChildren && (
<ScrollView contentContainerStyle={[styles.flexGrow1]}>
<ScrollView
horizontal
bounces={false}
contentContainerStyle={[styles.flex1, styles.flexColumn]}
>
{optionsAndInputsBelowThem}
</ScrollView>
</ScrollView>
)}

{this.props.shouldTextInputAppearBelowOptions && !this.props.shouldAllowScrollingChildren && optionsAndInputsBelowThem}

{!this.props.shouldTextInputAppearBelowOptions && (
<>
<View style={this.props.shouldUseStyleForChildren ? [styles.ph5, styles.pb3] : []}>
{this.props.children}
Expand Down
8 changes: 8 additions & 0 deletions src/components/OptionsSelector/optionsSelectorPropTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,12 @@ const propTypes = {

/** Whether the text input should intercept swipes or not */
shouldTextInputInterceptSwipe: PropTypes.bool,

/** Whether we should allow the view wrapping the nested children to be scrollable */
shouldAllowScrollingChildren: PropTypes.bool,

/** Whether nested scroll of options is enabled, true by default */
nestedScrollEnabled: PropTypes.bool,
};

const defaultProps = {
Expand Down Expand Up @@ -165,6 +171,8 @@ const defaultProps = {
isRowMultilineSupported: false,
initialFocusedIndex: undefined,
shouldTextInputInterceptSwipe: false,
shouldAllowScrollingChildren: false,
nestedScrollEnabled: true,
};

export {propTypes, defaultProps};
3 changes: 3 additions & 0 deletions src/libs/ReportUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -1542,6 +1542,9 @@ function getReportPreviewMessage(report, reportAction = {}, shouldConsiderReceip
if (_.isEmpty(linkedTransaction)) {
return reportActionMessage;
}
if (TransactionUtils.isReceiptBeingScanned(linkedTransaction)) {
return Localize.translateLocal('iou.receiptScanning');
}
const {amount, currency, comment} = getTransactionDetails(linkedTransaction);
const formattedAmount = CurrencyUtils.convertToDisplayString(amount, currency);
return Localize.translateLocal('iou.didSplitAmount', {formattedAmount, comment});
Expand Down
10 changes: 5 additions & 5 deletions src/libs/SidebarUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -347,17 +347,17 @@ function getOptionData(report, reportActions, personalDetails, preferredLocale,

if ((result.isChatRoom || result.isPolicyExpenseChat || result.isThread || result.isTaskReport) && !result.isArchivedRoom) {
const lastAction = visibleReportActionItems[report.reportID];
if (lodashGet(lastAction, 'actionName', '') === CONST.REPORT.ACTIONS.TYPE.RENAMED) {
if (lastAction && lastAction.actionName === CONST.REPORT.ACTIONS.TYPE.RENAMED) {
const newName = lodashGet(lastAction, 'originalMessage.newName', '');
result.alternateText = Localize.translate(preferredLocale, 'newRoomPage.roomRenamedTo', {newName});
} else if (lodashGet(lastAction, 'actionName', '') === CONST.REPORT.ACTIONS.TYPE.TASKREOPENED) {
} else if (lastAction && lastAction.actionName === CONST.REPORT.ACTIONS.TYPE.TASKREOPENED) {
result.alternateText = `${Localize.translate(preferredLocale, 'task.messages.reopened')}`;
} else if (lodashGet(lastAction, 'actionName', '') === CONST.REPORT.ACTIONS.TYPE.TASKCOMPLETED) {
} else if (lastAction && lastAction.actionName === CONST.REPORT.ACTIONS.TYPE.TASKCOMPLETED) {
result.alternateText = `${Localize.translate(preferredLocale, 'task.messages.completed')}`;
} else if (lodashGet(lastAction, 'actionName', '') !== CONST.REPORT.ACTIONS.TYPE.REPORTPREVIEW && lastActorDisplayName && lastMessageTextFromReport) {
} else if (lastAction && lastAction.actionName !== CONST.REPORT.ACTIONS.TYPE.REPORTPREVIEW && lastActorDisplayName && lastMessageTextFromReport) {
result.alternateText = `${lastActorDisplayName}: ${lastMessageText}`;
} else {
result.alternateText = lastMessageTextFromReport.length > 0 ? lastMessageText : Localize.translate(preferredLocale, 'report.noActivityYet');
result.alternateText = lastAction && lastMessageTextFromReport.length > 0 ? lastMessageText : Localize.translate(preferredLocale, 'report.noActivityYet');
Copy link
Contributor

@s77rt s77rt Oct 26, 2023

Choose a reason for hiding this comment

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

This seems unintended? The lastAction check is not needed for this case

}
} else {
if (!lastMessageText) {
Expand Down
20 changes: 16 additions & 4 deletions src/libs/TransactionUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ function hasReceipt(transaction: Transaction | undefined | null): boolean {
return !!transaction?.receipt?.state;
}

function areRequiredFieldsEmpty(transaction: Transaction): boolean {
function isMerchantMissing(transaction: Transaction) {
const isMerchantEmpty =
transaction.merchant === CONST.TRANSACTION.UNKNOWN_MERCHANT || transaction.merchant === CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT || transaction.merchant === '';

Expand All @@ -90,10 +90,19 @@ function areRequiredFieldsEmpty(transaction: Transaction): boolean {
transaction.modifiedMerchant === CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT ||
transaction.modifiedMerchant === '';

const isModifiedAmountEmpty = !transaction.modifiedAmount || transaction.modifiedAmount === 0;
const isModifiedCreatedEmpty = !transaction.modifiedCreated || transaction.modifiedCreated === '';
return isMerchantEmpty && isModifiedMerchantEmpty;
}

function isAmountMissing(transaction: Transaction) {
return transaction.amount === 0 && (!transaction.modifiedAmount || transaction.modifiedAmount === 0);
}

return (isModifiedMerchantEmpty && isMerchantEmpty) || (isModifiedAmountEmpty && transaction.amount === 0) || (isModifiedCreatedEmpty && transaction.created === '');
function isCreatedMissing(transaction: Transaction) {
return transaction.created === '' && (!transaction.created || transaction.modifiedCreated === '');
}

function areRequiredFieldsEmpty(transaction: Transaction): boolean {
return isMerchantMissing(transaction) || isAmountMissing(transaction) || isCreatedMissing(transaction);
}

/**
Expand Down Expand Up @@ -472,6 +481,9 @@ export {
isPending,
isPosted,
getWaypoints,
isAmountMissing,
isMerchantMissing,
isCreatedMissing,
areRequiredFieldsEmpty,
hasMissingSmartscanFields,
getWaypointIndex,
Expand Down
15 changes: 13 additions & 2 deletions src/libs/actions/IOU.js
Original file line number Diff line number Diff line change
Expand Up @@ -1303,7 +1303,18 @@ function startSplitBill(participants, currentUserLogin, currentUserAccountID, co
const receiptObject = {state, source};

// ReportID is -2 (aka "deleted") on the group transaction
const splitTransaction = TransactionUtils.buildOptimisticTransaction(0, CONST.CURRENCY.USD, CONST.REPORT.SPLIT_REPORTID, comment, '', '', '', '', receiptObject, filename);
const splitTransaction = TransactionUtils.buildOptimisticTransaction(
0,
CONST.CURRENCY.USD,
CONST.REPORT.SPLIT_REPORTID,
comment,
'',
'',
'',
CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT,
receiptObject,
filename,
);

// Note: The created action must be optimistically generated before the IOU action so there's no chance that the created action appears after the IOU action in the chat
const splitChatCreatedReportAction = ReportUtils.buildOptimisticCreatedReportAction(currentUserEmailForIOUSplit);
Expand Down Expand Up @@ -1419,7 +1430,7 @@ function startSplitBill(participants, currentUserLogin, currentUserAccountID, co
errors: ErrorUtils.getMicroSecondOnyxError('report.genericCreateReportFailureMessage'),
},
[splitIOUReportAction.reportActionID]: {
errors: ErrorUtils.getMicroSecondOnyxError('report.genericCreateFailureMessage'),
errors: ErrorUtils.getMicroSecondOnyxError('iou.error.genericCreateFailureMessage'),
},
},
},
Expand Down
4 changes: 2 additions & 2 deletions src/pages/EditSplitBillPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,11 @@ const propTypes = {
transaction: transactionPropTypes.isRequired,

/** The draft transaction that holds data to be persisted on the current transaction */
draftTransaction: PropTypes.shape(transactionPropTypes),
draftTransaction: transactionPropTypes,
};

const defaultProps = {
draftTransaction: {},
draftTransaction: undefined,
};

function EditSplitBillPage({route, transaction, draftTransaction}) {
Expand Down
Loading
Loading