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

fix: [Violations] Distance - Incorrect error message when distance amount is changed to smaller amount. #41649

Merged
merged 43 commits into from
Jul 15, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
2f4d561
fix: [Violations] Distance - Incorrect error message when distance am…
Krishna2323 May 4, 2024
bac54f1
remove tax rate changed violation.
Krishna2323 May 4, 2024
d399453
remove ReceiptAuditHeader.
Krishna2323 May 4, 2024
c7ec1df
Merge branch 'Expensify:main' into krishna2323/issue/41401
Krishna2323 May 10, 2024
34d5e2e
updated amountModified translation.
Krishna2323 May 10, 2024
0549056
minor update.
Krishna2323 May 10, 2024
ef8e655
Merge branch 'Expensify:main' into krishna2323/issue/41401
Krishna2323 May 17, 2024
f039038
remove taxAmountChanged & taxRateChanged notice violations.
Krishna2323 May 17, 2024
be4f9be
fix conflicts.
Krishna2323 Jun 2, 2024
c9fd9aa
fix conflicts.
Krishna2323 Jun 2, 2024
b801997
Merge branch 'Expensify:main' into krishna2323/issue/41401
Krishna2323 Jun 2, 2024
4240c2f
Merge branch 'krishna2323/issue/41401' of https://github.com/Krishna2…
Krishna2323 Jun 2, 2024
1043d16
Merge branch 'Expensify:main' into krishna2323/issue/41401
Krishna2323 Jun 6, 2024
3c71e67
use violation data type from backend.
Krishna2323 Jun 6, 2024
12f0433
add ViolationMessages for receipt.
Krishna2323 Jun 7, 2024
407e552
return all type of violations for paid policies.
Krishna2323 Jun 7, 2024
7611122
fix: merge conflicts.
Krishna2323 Jun 12, 2024
4ebb250
Merge branch 'Expensify:main' into krishna2323/issue/41401
Krishna2323 Jun 12, 2024
03a9345
add changes back in MoneyRequestView.
Krishna2323 Jun 12, 2024
d693489
Merge branch 'krishna2323/issue/41401' of https://github.com/Krishna2…
Krishna2323 Jun 12, 2024
245d667
updated receiptViolationNames.
Krishna2323 Jun 12, 2024
efd639c
Merge branch 'Expensify:main' into krishna2323/issue/41401
Krishna2323 Jun 13, 2024
4e8f365
fix type check.
Krishna2323 Jun 13, 2024
9e452c4
minor fixes.
Krishna2323 Jun 14, 2024
49954fb
Merge branch 'Expensify:main' into krishna2323/issue/41401
Krishna2323 Jun 18, 2024
d671670
Merge branch 'main' into krishna2323/issue/41401
Krishna2323 Jun 23, 2024
e2851c4
fix lint issue.
Krishna2323 Jun 23, 2024
7ae2d6b
get displayPercentVariance from violation data.
Krishna2323 Jun 23, 2024
e5420c1
Merge branch 'Expensify:main' into krishna2323/issue/41401
Krishna2323 Jun 26, 2024
7c38e5c
Merge branch 'Expensify:main' into krishna2323/issue/41401
Krishna2323 Jul 5, 2024
1595f33
minor fixes.
Krishna2323 Jul 5, 2024
2589ba6
update isPaidGroupPolicy param to shouldShowOnlyViolations.
Krishna2323 Jul 5, 2024
6e63f57
add ReceiptAuditMessages back.
Krishna2323 Jul 5, 2024
9fec7f9
add comments.
Krishna2323 Jul 5, 2024
4fe1913
Merge branch 'Expensify:main' into krishna2323/issue/41401
Krishna2323 Jul 8, 2024
68ebaa7
update comments.
Krishna2323 Jul 8, 2024
2a76377
minor update.
Krishna2323 Jul 8, 2024
c5d5a3c
fix: amount violation shows under receipt.
Krishna2323 Jul 8, 2024
995eaed
fix amount violation not showing.
Krishna2323 Jul 8, 2024
e3b692e
remove shouldShowAuditSuccess & shouldShowAuditFailure prop.
Krishna2323 Jul 10, 2024
0a92c99
add shouldShowAuditResult prop in ReceiptAudit.
Krishna2323 Jul 12, 2024
1c4d2a1
refactor.
Krishna2323 Jul 12, 2024
7cdc36c
add comment.
Krishna2323 Jul 12, 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
1 change: 0 additions & 1 deletion src/CONST.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3621,7 +3621,6 @@ const CONST = {
TAG_OUT_OF_POLICY: 'tagOutOfPolicy',
TAX_AMOUNT_CHANGED: 'taxAmountChanged',
TAX_OUT_OF_POLICY: 'taxOutOfPolicy',
TAX_RATE_CHANGED: 'taxRateChanged',
TAX_REQUIRED: 'taxRequired',
},

Expand Down
20 changes: 11 additions & 9 deletions src/components/ReceiptAudit.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,24 @@ import Icon from './Icon';
import * as Expensicons from './Icon/Expensicons';
import Text from './Text';

function ReceiptAuditHeader({notes, shouldShowAuditMessage}: {notes: string[]; shouldShowAuditMessage: boolean}) {
function ReceiptAudit({notes, shouldShowAuditSuccess, shouldShowAuditFailure}: {notes: string[]; shouldShowAuditSuccess?: boolean; shouldShowAuditFailure?: boolean}) {
Krishna2323 marked this conversation as resolved.
Show resolved Hide resolved
const styles = useThemeStyles();
const theme = useTheme();
const {translate} = useLocalize();

const auditText = notes.length > 0 ? translate('iou.receiptIssuesFound', notes.length) : translate('common.verified');
let auditText = '';

if (notes.length > 0 && shouldShowAuditFailure) {
auditText = translate('iou.receiptIssuesFound', notes.length);
} else if (!notes.length && shouldShowAuditSuccess) {
auditText = translate('common.verified');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This block needs a comment explaining why we're doing what we're doing here, or as I mentioned in another comment, we need to document shouldShowAuditSuccess and shouldShowAuditFailure to make this clear

Suggested change
let auditText = '';
if (notes.length > 0 && shouldShowAuditFailure) {
auditText = translate('iou.receiptIssuesFound', notes.length);
} else if (!notes.length && shouldShowAuditSuccess) {
auditText = translate('common.verified');
}
let auditText = '';
if (notes.length > 0 && shouldShowAuditFailure) {
auditText = translate('iou.receiptIssuesFound', notes.length);
} else if (!notes.length && shouldShowAuditSuccess) {
auditText = translate('common.verified');
}


return (
<View style={[styles.ph5, styles.mbn1]}>
<View style={[styles.flexRow, styles.alignItemsCenter]}>
<Text style={[styles.textLabelSupporting]}>{translate('common.receipt')}</Text>
{shouldShowAuditMessage && (
{auditText && (
Krishna2323 marked this conversation as resolved.
Show resolved Hide resolved
<>
<Text style={[styles.textLabelSupporting]}>{` • ${auditText}`}</Text>
<Icon
Expand All @@ -34,9 +41,4 @@ function ReceiptAuditHeader({notes, shouldShowAuditMessage}: {notes: string[]; s
);
}

function ReceiptAuditMessages({notes = []}: {notes?: string[]}) {
const styles = useThemeStyles();
return <View style={[styles.mtn1, styles.mb2, styles.ph5, styles.gap1]}>{notes.length > 0 && notes.map((message) => <Text style={[styles.textLabelError]}>{message}</Text>)}</View>;
}

export {ReceiptAuditHeader, ReceiptAuditMessages};
export default ReceiptAudit;
12 changes: 6 additions & 6 deletions src/components/ReportActionItem/MoneyRequestView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import * as Expensicons from '@components/Icon/Expensicons';
import MenuItemWithTopDescription from '@components/MenuItemWithTopDescription';
import OfflineWithFeedback from '@components/OfflineWithFeedback';
import {useSession} from '@components/OnyxProvider';
import {ReceiptAuditHeader, ReceiptAuditMessages} from '@components/ReceiptAudit';
import ReceiptAudit from '@components/ReceiptAudit';
import ReceiptEmptyState from '@components/ReceiptEmptyState';
import Switch from '@components/Switch';
import Text from '@components/Text';
Expand Down Expand Up @@ -178,7 +178,7 @@ function MoneyRequestView({
// A flag for showing tax rate
const shouldShowTax = isTaxTrackingEnabled(isPolicyExpenseChat, policy);

const {getViolationsForField} = useViolations(transactionViolations ?? []);
const {getViolationsForField} = useViolations(transactionViolations ?? [], !isReceiptBeingScanned && ReportUtils.isPaidGroupPolicy(report));
const hasViolations = useCallback(
(field: ViolationField, data?: OnyxTypes.TransactionViolation['data']): boolean => !!canUseViolations && getViolationsForField(field, data).length > 0,
[canUseViolations, getViolationsForField],
Expand Down Expand Up @@ -330,16 +330,17 @@ function MoneyRequestView({
const shouldShowMapOrReceipt = showMapAsImage || hasReceipt;
const shouldShowReceiptEmptyState = !hasReceipt && !isInvoice && (canEditReceipt || isAdmin || isApprover);
const noticeTypeViolations = transactionViolations?.filter((violation) => violation.type === 'notice').map((v) => ViolationsUtils.getViolationTranslation(v, translate)) ?? [];
const shouldShowNotesViolations = !isReceiptBeingScanned && canUseViolations && ReportUtils.isPaidGroupPolicy(report);
const shouldShowAuditMessage = !isReceiptBeingScanned && canUseViolations && ReportUtils.isPaidGroupPolicy(report);

return (
<View style={[StyleUtils.getReportWelcomeContainerStyle(isSmallScreenWidth, true, shouldShowAnimatedBackground)]}>
{shouldShowAnimatedBackground && <AnimatedEmptyStateBackground />}
<View style={shouldShowAnimatedBackground && [StyleUtils.getReportWelcomeTopMarginStyle(isSmallScreenWidth, true)]}>
{!isInvoice && (
<ReceiptAuditHeader
<ReceiptAudit
notes={noticeTypeViolations}
shouldShowAuditMessage={Boolean(shouldShowNotesViolations && didRceiptScanSucceed)}
shouldShowAuditSuccess={shouldShowAuditMessage && didRceiptScanSucceed}
shouldShowAuditFailure={shouldShowAuditMessage && hasReceipt}
Copy link
Contributor

Choose a reason for hiding this comment

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

I've been looking at this for a minute, and it's not obvious to me why we now need shouldShowAuditSuccess and shouldShowAuditFailure, instead of using notes.length to determine what to show.

What cases would we miss if we went back to relying only on notes.length? If you can update the docs for these params to make it clear, please do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

previously we were relying only on notes.length but if I recall correctly there were cases where success/failure text was shown before the receipt is scanned and even if there were no receipt.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you confirm, and if you do, add a manual tests that would reproduce the issue, and confirm your change solves it. If you can't confirm, let's revert to what we had previously

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cead22, pls check the 2nd issue here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Krishna2323 sorry if this is obvious, but can you explain why we can't use a single prop? Both success and failure need to be shown only if there's a receipt, and the issue you linked shows an error when there's no receipt. What am I missing? Can you share test steps to reproduce the issue when we use only 1 prop, that doesn't happen with this solution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, this is bit confusing. Above condition won't work correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cead22, I think we need shouldShowAuditSuccess and shouldShowAuditFailure or a single prop to check if the receipt is being scanned. Relying on the notes length will show Verified while the receipt is being scanned, as initially, notes will be an empty array.

We can remove shouldShowAuditSuccess and shouldShowAuditFailure and introduce a new prop, isScanning. This way, we only display the failure/verified message when the receipt is not being scanned.

Copy link
Contributor

Choose a reason for hiding this comment

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

Love it, sorry for the late reply

Copy link
Contributor

Choose a reason for hiding this comment

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

@Krishna2323 will you be able to update this today and get the PR ready for review?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cead22, on it, will be ready in few minutes.

/>
)}
{shouldShowMapOrReceipt && (
Expand Down Expand Up @@ -393,7 +394,6 @@ function MoneyRequestView({
/>
)}
{!shouldShowReceiptEmptyState && !shouldShowMapOrReceipt && <View style={{marginVertical: 6}} />}
{shouldShowNotesViolations && <ReceiptAuditMessages notes={noticeTypeViolations} />}
{canUseViolations && <ViolationMessages violations={getViolationsForField('receipt')} />}
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, why did this condition change? Why don't we need to check !isReceiptBeingScanned and ReportUtils.isPaidGroupPolicy(report); anymore?

Copy link
Contributor Author

@Krishna2323 Krishna2323 Jul 5, 2024

Choose a reason for hiding this comment

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

hmm, maybe I removed it in confusion, sorry for that, added that back.

Copy link
Contributor

Choose a reason for hiding this comment

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

That change must have broken something that isn't being tested in the manual tests of this PR (otherwise we would've noticed it broke something). Given that, consider adding a manual test and confirming your changes/code solve for the case in question

<OfflineWithFeedback pendingAction={getPendingFieldAction('amount')}>
<MenuItemWithTopDescription
Expand Down
13 changes: 9 additions & 4 deletions src/hooks/useViolations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,23 +42,28 @@ const violationFields: Record<ViolationName, ViolationField> = {
tagOutOfPolicy: 'tag',
taxAmountChanged: 'tax',
taxOutOfPolicy: 'tax',
taxRateChanged: 'tax',
taxRequired: 'tax',
};

type ViolationsMap = Map<ViolationField, TransactionViolation[]>;

function useViolations(violations: TransactionViolation[]) {
function useViolations(violations: TransactionViolation[], shouldIncludeNoticeViolations?: boolean) {
const violationsByField = useMemo((): ViolationsMap => {
const filteredViolations = violations.filter((violation) => violation.type === CONST.VIOLATION_TYPES.VIOLATION);
const filteredViolations = violations.filter((violation) => {
if (!shouldIncludeNoticeViolations) {
return violation.type === CONST.VIOLATION_TYPES.VIOLATION;
}
return violation.type === CONST.VIOLATION_TYPES.VIOLATION || violation.type === CONST.VIOLATION_TYPES.NOTICE;
});

const violationGroups = new Map<ViolationField, TransactionViolation[]>();
for (const violation of filteredViolations) {
const field = violationFields[violation.name];
const existingViolations = violationGroups.get(field) ?? [];
violationGroups.set(field, [...existingViolations, violation]);
}
return violationGroups ?? new Map();
}, [violations]);
}, [violations, shouldIncludeNoticeViolations]);

const getViolationsForField = useCallback(
(field: ViolationField, data?: TransactionViolation['data']) => {
Expand Down
2 changes: 0 additions & 2 deletions src/libs/Violations/ViolationsUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -276,8 +276,6 @@ const ViolationsUtils = {
return translate('violations.taxAmountChanged');
case 'taxOutOfPolicy':
return translate('violations.taxOutOfPolicy', {taxName});
case 'taxRateChanged':
return translate('violations.taxRateChanged');
case 'taxRequired':
return translate('violations.taxRequired');
default:
Expand Down
Loading