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 38 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
9 changes: 9 additions & 0 deletions src/CONST.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3905,6 +3905,15 @@ const CONST = {
WARNING: 'warning',
},

/**
* Constants with different types for the modifiedAmount violation
*/
MODIFIED_AMOUNT_VIOLATION_DATA: {
DISTANCE: 'distance',
CARD: 'card',
SMARTSCAN: 'smartscan',
},

/**
* Constants for types of violation names.
* Defined here because they need to be referenced by the type system to generate the
Expand Down
26 changes: 22 additions & 4 deletions src/components/ReceiptAudit.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,34 @@ import Icon from './Icon';
import * as Expensicons from './Icon/Expensicons';
import Text from './Text';

function ReceiptAuditHeader({notes, shouldShowAuditMessage}: {notes: string[]; shouldShowAuditMessage: boolean}) {
type ReceiptAuditProps = {
/** List of audit notes */
notes: string[];

/** Weather to show success message (e.g. When receipt is scanning, we don't want to show `Verified` message) */
shouldShowAuditSuccess: boolean;

/** Weather to show failure message (e.g. When receipt isn't required, we don't want to show `Issues Found` message) */
shouldShowAuditFailure: boolean;
};

function ReceiptAudit({notes, shouldShowAuditSuccess, shouldShowAuditFailure}: ReceiptAuditProps) {
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');
}

return (
<View style={[styles.ph5, styles.mbn1]}>
<View style={[styles.flexRow, styles.alignItemsCenter]}>
<Text style={[styles.textLabelSupporting]}>{translate('common.receipt')}</Text>
{shouldShowAuditMessage && (
{!!auditText && (
<>
<Text style={[styles.textLabelSupporting]}>{` • ${auditText}`}</Text>
<Icon
Expand All @@ -39,4 +56,5 @@ function ReceiptAuditMessages({notes = []}: {notes?: string[]}) {
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 {ReceiptAuditMessages};
export default ReceiptAudit;
19 changes: 9 additions & 10 deletions src/components/ReportActionItem/MoneyRequestView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import MenuItem from '@components/MenuItem';
import MenuItemWithTopDescription from '@components/MenuItemWithTopDescription';
import OfflineWithFeedback from '@components/OfflineWithFeedback';
import {useSession} from '@components/OnyxProvider';
import {ReceiptAuditHeader, ReceiptAuditMessages} from '@components/ReceiptAudit';
import ReceiptAudit, {ReceiptAuditMessages} from '@components/ReceiptAudit';
import ReceiptEmptyState from '@components/ReceiptEmptyState';
import Switch from '@components/Switch';
import Text from '@components/Text';
Expand Down Expand Up @@ -189,7 +189,7 @@ function MoneyRequestView({
const tripID = ReportUtils.getTripIDFromTransactionParentReport(parentReport);
const shouldShowViewTripDetails = TransactionUtils.hasReservationList(transaction) && !!tripID;

const {getViolationsForField} = useViolations(transactionViolations ?? []);
const {getViolationsForField} = useViolations(transactionViolations ?? [], !isReceiptBeingScanned || !ReportUtils.isPaidGroupPolicy(report));
const hasViolations = useCallback(
(field: ViolationField, data?: OnyxTypes.TransactionViolation['data'], policyHasDependentTags = false, tagValue?: string): boolean =>
!!canUseViolations && getViolationsForField(field, data, policyHasDependentTags, tagValue).length > 0,
Expand Down Expand Up @@ -340,14 +340,13 @@ function MoneyRequestView({
const receiptViolationNames: OnyxTypes.ViolationName[] = [
CONST.VIOLATIONS.RECEIPT_REQUIRED,
CONST.VIOLATIONS.RECEIPT_NOT_SMART_SCANNED,
CONST.VIOLATIONS.MODIFIED_DATE,
Copy link
Contributor Author

@Krishna2323 Krishna2323 Jul 8, 2024

Choose a reason for hiding this comment

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

@sobitneupane @cead22, I mistakenly added modified amount violation here while resolving conflict, thats why the violation was shown under receipt. I have removed that and also CONST.VIOLATIONS.MODIFIED_DATE because I believe the date violation should also be shown under date field.

CONST.VIOLATIONS.CASH_EXPENSE_WITH_NO_RECEIPT,
CONST.VIOLATIONS.SMARTSCAN_FAILED,
];
const receiptViolations =
transactionViolations?.filter((violation) => receiptViolationNames.includes(violation.name)).map((violation) => ViolationsUtils.getViolationTranslation(violation, translate)) ?? [];
const shouldShowNotesViolations = !isReceiptBeingScanned && canUseViolations && ReportUtils.isPaidGroupPolicy(report);
const shouldShowReceiptHeader = isReceiptAllowed && (shouldShowReceiptEmptyState || hasReceipt);
const shouldShowAuditMessage = !isReceiptBeingScanned && !!canUseViolations && ReportUtils.isPaidGroupPolicy(report);
const shouldShowReceiptAudit = isReceiptAllowed && (shouldShowReceiptEmptyState || hasReceipt);

const errors = {
...(transaction?.errorFields?.route ?? transaction?.errors),
Expand Down Expand Up @@ -389,10 +388,11 @@ function MoneyRequestView({
<View style={styles.pRelative}>
{shouldShowAnimatedBackground && <AnimatedEmptyStateBackground />}
<>
{shouldShowReceiptHeader && (
<ReceiptAuditHeader
{shouldShowReceiptAudit && (
<ReceiptAudit
notes={receiptViolations}
shouldShowAuditMessage={!!(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.

/>
)}
{(hasReceipt || errors) && (
Expand Down Expand Up @@ -448,7 +448,7 @@ function MoneyRequestView({
/>
)}
{!shouldShowReceiptEmptyState && !hasReceipt && <View style={{marginVertical: 6}} />}
{shouldShowNotesViolations && <ReceiptAuditMessages notes={receiptViolations} />}
{shouldShowAuditMessage && <ReceiptAuditMessages notes={receiptViolations} />}
<OfflineWithFeedback pendingAction={getPendingFieldAction('amount')}>
<MenuItemWithTopDescription
title={amountTitle}
Expand Down Expand Up @@ -555,7 +555,6 @@ function MoneyRequestView({
/>
</OfflineWithFeedback>
)}

{shouldShowTax && (
<OfflineWithFeedback pendingAction={getPendingFieldAction('taxAmount')}>
<MenuItemWithTopDescription
Expand Down
24 changes: 20 additions & 4 deletions src/hooks/useViolations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,26 +40,42 @@ const violationFields: Record<ViolationName, ViolationField> = {
smartscanFailed: 'receipt',
someTagLevelsRequired: 'tag',
tagOutOfPolicy: 'tag',
taxRateChanged: 'tax',
taxAmountChanged: 'tax',
taxOutOfPolicy: 'tax',
taxRateChanged: 'tax',
taxRequired: 'tax',
hold: 'none',
};

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

function useViolations(violations: TransactionViolation[]) {
// We don't want to show these violations on NewDot
const excludedViolationsName = ['taxAmountChanged', 'taxRateChanged'];

/**
* @param violations – List of transaction violations
* @param shouldShowOnlyViolations – Whether we should only show violations of type 'violation'
*/
function useViolations(violations: TransactionViolation[], shouldShowOnlyViolations: boolean) {
const violationsByField = useMemo((): ViolationsMap => {
const filteredViolations = violations.filter((violation) => violation.type === CONST.VIOLATION_TYPES.VIOLATION);
const filteredViolations = violations.filter((violation) => {
if (excludedViolationsName.includes(violation.name)) {
return false;
}
if (shouldShowOnlyViolations) {
return violation.type === CONST.VIOLATION_TYPES.VIOLATION;
}
return true;
});

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, shouldShowOnlyViolations]);

const getViolationsForField = useCallback(
(field: ViolationField, data?: TransactionViolation['data'], policyHasDependentTags = false, tagValue?: string) => {
Expand Down
15 changes: 14 additions & 1 deletion src/languages/en.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ import type {
ViolationsInvoiceMarkupParams,
ViolationsMaxAgeParams,
ViolationsMissingTagParams,
ViolationsModifiedAmountParams,
ViolationsOverCategoryLimitParams,
ViolationsOverLimitParams,
ViolationsPerDayLimitParams,
Expand Down Expand Up @@ -3633,7 +3634,19 @@ export default {
missingCategory: 'Missing category',
missingComment: 'Description required for selected category',
missingTag: ({tagName}: ViolationsMissingTagParams) => `Missing ${tagName ?? 'tag'}`,
modifiedAmount: 'Amount greater than scanned receipt',
modifiedAmount: ({type, displayPercentVariance}: ViolationsModifiedAmountParams): string => {
switch (type) {
case 'distance':
return 'Amount differs from calculated distance';
case 'card':
return 'Amount greater than card transaction';
default:
if (displayPercentVariance) {
return `Amount ${displayPercentVariance}% greater than scanned receipt`;
}
return 'Amount greater than scanned receipt';
}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

#41401 (comment)

  • {data: {type: 'distance'}}
  • {data: {type: 'card'}}
  • {data: {type: 'smartscan'}}
  • {data: {type: 'smartscan', displayPercentVariance: $displayPercentVariance}}

displayPercentVariance is present in type 'smartscan' not 'card'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated 👀

modifiedDate: 'Date differs from scanned receipt',
nonExpensiworksExpense: 'Non-Expensiworks expense',
overAutoApprovalLimit: ({formattedLimit}: ViolationsOverLimitParams) => `Expense exceeds auto approval limit of ${formattedLimit}`,
Expand Down
15 changes: 14 additions & 1 deletion src/languages/es.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ import type {
ViolationsInvoiceMarkupParams,
ViolationsMaxAgeParams,
ViolationsMissingTagParams,
ViolationsModifiedAmountParams,
ViolationsOverAutoApprovalLimitParams,
ViolationsOverCategoryLimitParams,
ViolationsOverLimitParams,
Expand Down Expand Up @@ -4141,7 +4142,19 @@ export default {
missingCategory: 'Falta categoría',
missingComment: 'Descripción obligatoria para la categoría seleccionada',
missingTag: ({tagName}: ViolationsMissingTagParams) => `Falta ${tagName ?? 'etiqueta'}`,
modifiedAmount: 'Importe superior al del recibo escaneado',
modifiedAmount: ({type, displayPercentVariance}: ViolationsModifiedAmountParams) => {
switch (type) {
case 'distance':
return 'Importe difiere del calculado basado en distancia';
case 'card':
return 'Importe mayor al de la transacción de la tarjeta';
default:
if (displayPercentVariance) {
return `Importe ${displayPercentVariance}% mayor al del recibo escaneado`;
}
return 'Importe mayor al del recibo escaneado';
}
},
modifiedDate: 'Fecha difiere del recibo escaneado',
nonExpensiworksExpense: 'Gasto no proviene de Expensiworks',
overAutoApprovalLimit: ({formattedLimit}: ViolationsOverAutoApprovalLimitParams) =>
Expand Down
4 changes: 4 additions & 0 deletions src/languages/types.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import type {OnyxInputOrEntry, ReportAction} from '@src/types/onyx';
import type {Unit} from '@src/types/onyx/Policy';
import type {ViolationDataType} from '@src/types/onyx/TransactionViolation';
import type en from './en';

type AddressLineParams = {
Expand Down Expand Up @@ -222,6 +223,8 @@ type ViolationsMaxAgeParams = {maxAge: number};

type ViolationsMissingTagParams = {tagName?: string};

type ViolationsModifiedAmountParams = {type?: ViolationDataType; displayPercentVariance?: number};

type ViolationsOverAutoApprovalLimitParams = {formattedLimit?: string};

type ViolationsOverCategoryLimitParams = {formattedLimit?: string};
Expand Down Expand Up @@ -431,6 +434,7 @@ export type {
ViolationsInvoiceMarkupParams,
ViolationsMaxAgeParams,
ViolationsMissingTagParams,
ViolationsModifiedAmountParams,
ViolationsOverAutoApprovalLimitParams,
ViolationsOverCategoryLimitParams,
ViolationsOverLimitParams,
Expand Down
3 changes: 2 additions & 1 deletion src/libs/Violations/ViolationsUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ const ViolationsUtils = {
maxAge = 0,
tagName,
taxName,
type,
} = violation.data ?? {};

switch (violation.name) {
Expand Down Expand Up @@ -288,7 +289,7 @@ const ViolationsUtils = {
case 'missingTag':
return translate('violations.missingTag', {tagName});
case 'modifiedAmount':
return translate('violations.modifiedAmount');
return translate('violations.modifiedAmount', {type, displayPercentVariance: violation.data?.displayPercentVariance});
case 'modifiedDate':
return translate('violations.modifiedDate');
case 'nonExpensiworksExpense':
Expand Down
14 changes: 13 additions & 1 deletion src/types/onyx/TransactionViolation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@ import type CONST from '@src/CONST';
*/
type ViolationName = ValueOf<typeof CONST.VIOLATIONS>;

/**
* Types for the data in the modifiedAmount violation
* Derived from CONST.VIOLATION_DATA_TYPES to maintain a single source of truth.
*/
type ViolationDataType = ValueOf<typeof CONST.MODIFIED_AMOUNT_VIOLATION_DATA>;

/** Model of transaction violation data */
type TransactionViolationData = {
/** Who rejected the transaction */
Expand Down Expand Up @@ -63,6 +69,12 @@ type TransactionViolationData = {
/** Whether the current violation is `pending RTER` */
pendingPattern?: boolean;

/** modifiedAmount violation type (eg, 'distance', 'card') */
type?: ViolationDataType;

/** Percent Variance for modified amount violations */
displayPercentVariance?: number;

/** List of duplicate transactions */
duplicates?: string[];
};
Expand All @@ -82,5 +94,5 @@ type TransactionViolation = {
/** Collection of transaction violations */
type TransactionViolations = TransactionViolation[];

export type {TransactionViolation, ViolationName};
export type {TransactionViolation, ViolationName, ViolationDataType};
export default TransactionViolations;
Loading