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.

Sign up for GitHub

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

Jump to bottom

fix: fix validation for split bill #40868

Merged
merged 4 commits into from
Apr 26, 2024
Merged

fix: fix validation for split bill #40868

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
24aac4e
fix: fix validation for split bill
koko57 Apr 24, 2024
47a0fc4
fix: fix validation for submitting a split
koko57 Apr 24, 2024
171c0d4
fix: remove unnecessary condition
koko57 Apr 25, 2024
96817be
fix: remove unnecessary condition
koko57 Apr 25, 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
31 changes: 8 additions & 23 deletions src/components/MoneyRequestConfirmationList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,8 +303,6 @@ function MoneyRequestConfirmationList({
const [didConfirm, setDidConfirm] = useState(false);
const [didConfirmSplit, setDidConfirmSplit] = useState(false);

const [merchantError, setMerchantError] = useState(false);

const [isAttachmentInvalid, setIsAttachmentInvalid] = useState(false);

const navigateBack = () => {
Expand All @@ -322,19 +320,10 @@ function MoneyRequestConfirmationList({
const isMerchantEmpty = !iouMerchant || iouMerchant === CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT;
const isMerchantRequired = isPolicyExpenseChat && !isScanRequest && shouldShowMerchant;

const isCategoryRequired = canUseViolations && !!policy?.requiresCategory;
const shouldDisplayMerchantError =
(isMerchantRequired || (isEditingSplitBill && isScanRequest)) && (shouldDisplayFieldError || formError === 'iou.error.invalidMerchant') && isMerchantEmpty;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little confused about the (isEditingSplitBill && isScanRequest) condition here, I can't see any instance in which it is true due to isScanRequest always being false, but if isScanRequest was true we would be seeing the merchant error in places where the merchant isn't required.

Maybe I'm missing something but could you clarify this for me? Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ollyws ok, you're right - removing this from the condition

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks but now we are getting a field required error on a non-workspace split:

Screen.Recording.2024-04-25.at.15.39.13.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ollyws fixed


useEffect(() => {
if ((!isMerchantRequired && isMerchantEmpty) || !merchantError) {
return;
}
if (!isMerchantEmpty && merchantError) {
setMerchantError(false);
if (formError === 'iou.error.invalidMerchant') {
setFormError('');
}
}
}, [formError, isMerchantEmpty, merchantError, isMerchantRequired]);
const isCategoryRequired = canUseViolations && !!policy?.requiresCategory;

useEffect(() => {
if (shouldDisplayFieldError && hasSmartScanFailed) {
Expand All @@ -345,13 +334,9 @@ function MoneyRequestConfirmationList({
setFormError('iou.error.genericSmartscanFailureMessage');
return;
}
if (merchantError) {
setFormError('iou.error.invalidMerchant');
return;
}
// reset the form error whenever the screen gains or loses focus
setFormError('');
}, [isFocused, transaction, shouldDisplayFieldError, hasSmartScanFailed, didConfirmSplit, isMerchantRequired, merchantError]);
}, [isFocused, transaction, shouldDisplayFieldError, hasSmartScanFailed, didConfirmSplit]);

useEffect(() => {
if (!shouldCalculateDistanceAmount) {
Expand Down Expand Up @@ -577,8 +562,8 @@ function MoneyRequestConfirmationList({
if (selectedParticipants.length === 0) {
return;
}
if ((isMerchantRequired && isMerchantEmpty) || (shouldDisplayFieldError && TransactionUtils.isMerchantMissing(transaction ?? null))) {
setMerchantError(true);
if (!isEditingSplitBill && isMerchantRequired && (isMerchantEmpty || (shouldDisplayFieldError && TransactionUtils.isMerchantMissing(transaction ?? null)))) {
setFormError('iou.error.invalidMerchant');
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Without the early return the report will submit even with the merchant error:

Screen.Recording.2024-04-24.at.21.27.02.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

}
if (iouCategory.length > CONST.API_TRANSACTION_CATEGORY_MAX_LENGTH) {
Expand Down Expand Up @@ -805,8 +790,8 @@ function MoneyRequestConfirmationList({
}}
disabled={didConfirm}
interactive={!isReadOnly}
brickRoadIndicator={merchantError ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : undefined}
error={merchantError ? translate('common.error.fieldRequired') : ''}
brickRoadIndicator={shouldDisplayMerchantError ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : undefined}
error={shouldDisplayMerchantError ? translate('common.error.fieldRequired') : ''}
rightLabel={isMerchantRequired ? translate('common.required') : ''}
/>
),
Expand Down
Loading