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: IOU-Error message remains on IOU report even same error message is dismissed from 1:1 #37875

Merged
merged 15 commits into from
Mar 25, 2024
8 changes: 4 additions & 4 deletions src/libs/ErrorUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,16 +40,16 @@ function getAuthenticateErrorMessage(response: Response): keyof TranslationFlatO
* Method used to get an error object with microsecond as the key.
* @param error - error key or message to be saved
*/
function getMicroSecondOnyxError(error: string | null, isTranslated = false): Errors {
return {[DateUtils.getMicroseconds()]: error && [error, {isTranslated}]};
function getMicroSecondOnyxError(error: string | null, isTranslated = false, errorKey?: number): Errors {
return {[errorKey ?? DateUtils.getMicroseconds()]: error && [error, {isTranslated}]};
}

/**
* Method used to get an error object with microsecond as the key and an object as the value.
* @param error - error key or message to be saved
*/
function getMicroSecondOnyxErrorObject(error: Errors): ErrorFields {
return {[DateUtils.getMicroseconds()]: error};
function getMicroSecondOnyxErrorObject(error: Errors, errorKey?: number): ErrorFields {
return {[errorKey ?? DateUtils.getMicroseconds()]: error};
}

// We can assume that if error is a string, it has already been translated because it is server error
Expand Down
24 changes: 16 additions & 8 deletions src/libs/actions/IOU.ts
Original file line number Diff line number Diff line change
Expand Up @@ -417,10 +417,10 @@ function resetMoneyRequestInfo(id = '') {
}

/** Helper function to get the receipt error for money requests, or the generic error if there's no receipt */
function getReceiptError(receipt?: Receipt, filename?: string, isScanRequest = true): Errors | ErrorFields {
function getReceiptError(receipt?: Receipt, filename?: string, isScanRequest = true, errorKey?: number): Errors | ErrorFields {
return isEmptyObject(receipt) || !isScanRequest
? ErrorUtils.getMicroSecondOnyxError('iou.error.genericCreateFailureMessage')
: ErrorUtils.getMicroSecondOnyxErrorObject({error: CONST.IOU.RECEIPT_ERROR, source: receipt.source?.toString() ?? '', filename: filename ?? ''});
? ErrorUtils.getMicroSecondOnyxError('iou.error.genericCreateFailureMessage', false, errorKey)
: ErrorUtils.getMicroSecondOnyxErrorObject({error: CONST.IOU.RECEIPT_ERROR, source: receipt.source?.toString() ?? '', filename: filename ?? ''}, errorKey);
}

function needsToBeManuallySubmitted(iouReport: OnyxTypes.Report) {
Expand Down Expand Up @@ -708,6 +708,8 @@ function buildOnyxDataForMoneyRequest(
},
);

const errorKey = DateUtils.getMicroseconds();

const failureData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
Expand Down Expand Up @@ -763,7 +765,7 @@ function buildOnyxDataForMoneyRequest(
[iouCreatedAction.reportActionID]: {
// Disabling this line since transaction.filename can be an empty string
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
errors: getReceiptError(transaction.receipt, transaction.filename || transaction.receipt?.filename, isScanRequest),
errors: getReceiptError(transaction.receipt, transaction.filename || transaction.receipt?.filename, isScanRequest, errorKey),
},
[iouAction.reportActionID]: {
errors: ErrorUtils.getMicroSecondOnyxError(null),
Expand All @@ -773,7 +775,7 @@ function buildOnyxDataForMoneyRequest(
[iouAction.reportActionID]: {
// Disabling this line since transaction.filename can be an empty string
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
errors: getReceiptError(transaction.receipt, transaction.filename || transaction.receipt?.filename, isScanRequest),
errors: getReceiptError(transaction.receipt, transaction.filename || transaction.receipt?.filename, isScanRequest, errorKey),
},
}),
},
Expand All @@ -783,7 +785,7 @@ function buildOnyxDataForMoneyRequest(
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${transactionThreadReport.reportID}`,
value: {
[transactionThreadCreatedReportAction.reportActionID]: {
errors: ErrorUtils.getMicroSecondOnyxError('iou.error.genericCreateFailureMessage'),
errors: ErrorUtils.getMicroSecondOnyxError('iou.error.genericCreateFailureMessage', false, errorKey),
},
},
},
Expand Down Expand Up @@ -3771,6 +3773,8 @@ function deleteMoneyRequest(transactionID: string, reportAction: OnyxTypes.Repor
});
}

const errorKey = DateUtils.getMicroseconds();

failureData.push(
{
onyxMethod: Onyx.METHOD.MERGE,
Expand All @@ -3779,7 +3783,9 @@ function deleteMoneyRequest(transactionID: string, reportAction: OnyxTypes.Repor
[reportAction.reportActionID]: {
...reportAction,
pendingAction: null,
errors: ErrorUtils.getMicroSecondOnyxError('iou.error.genericDeleteFailureMessage'),
errors: {
[errorKey]: ['iou.error.genericDeleteFailureMessage', {isTranslated: false}],
},
},
},
},
Expand All @@ -3801,7 +3807,9 @@ function deleteMoneyRequest(transactionID: string, reportAction: OnyxTypes.Repor
[reportPreviewAction?.reportActionID ?? '']: {
...reportPreviewAction,
pendingAction: null,
errors: ErrorUtils.getMicroSecondOnyxError('iou.error.genericDeleteFailureMessage'),
errors: {
[errorKey]: ['iou.error.genericDeleteFailureMessage', {isTranslated: false}],
},
},
},
},
Expand Down
51 changes: 48 additions & 3 deletions src/libs/actions/ReportActions.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import Onyx from 'react-native-onyx';
import type {OnyxEntry} from 'react-native-onyx';
import * as ReportActionUtils from '@libs/ReportActionsUtils';
import * as ReportUtils from '@libs/ReportUtils';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import type ReportAction from '@src/types/onyx/ReportAction';
import * as Report from './Report';

function clearReportActionErrors(reportID: string, reportAction: OnyxEntry<ReportAction>) {
type IgnoreDirection = 'parent' | 'child';

function clearReportActionErrors(reportID: string, reportAction: ReportAction, keys?: string[]) {
const originalReportID = ReportUtils.getOriginalReportID(reportID, reportAction);

if (!reportAction?.reportActionID) {
Expand All @@ -34,14 +35,58 @@ function clearReportActionErrors(reportID: string, reportAction: OnyxEntry<Repor
return;
}

if (keys) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is to handle cases with multiple errors in the same report action, right?

It may be a good improvement but It is not mentioned in the issue. Why do you think It is our expectation? Do you confirm with the design team or any internal engineer about that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to handle cases with multiple errors in the same report action, right?

Yes

Why do you think It is our expectation?

When users click x button, we'll clear all errors of that action, so the related action can share some same errors and we need to clear all of them. I believe it's the expectation in the OP

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that's the expectation

const errors: Record<string, null> = {};

keys.forEach((key) => {
errors[key] = null;
});

Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${originalReportID}`, {
[reportAction.reportActionID]: {
errors,
},
});
return;
}
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${originalReportID}`, {
[reportAction.reportActionID]: {
errors: null,
},
});
}

/**
*
ignore: `undefined` means we want to check both parent and children report actions
ignore: `parent` or `child` means we want to ignore checking parent or child report actions because they've been previously checked
*/
function clearAllRelatedReportActionErrors(reportID: string, reportAction: ReportAction | null, ignore?: IgnoreDirection, keys?: string[]) {
const errorKeys = keys ?? Object.keys(reportAction?.errors ?? {});
if (!reportAction || errorKeys.length === 0) {
return;
}

clearReportActionErrors(reportID, reportAction, keys);

const report = ReportUtils.getReport(reportID);
if (report?.parentReportID && report?.parentReportActionID && ignore !== 'parent') {
const parentReportAction = ReportActionUtils.getReportAction(report.parentReportID, report.parentReportActionID);
const parentErrorKeys = Object.keys(parentReportAction?.errors ?? {}).filter((err) => errorKeys.includes(err));

clearAllRelatedReportActionErrors(report.parentReportID, parentReportAction, 'child', parentErrorKeys);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please point out 1 case where we need to call clearAllRelatedReportActionErrors instead of clearReportActionErrors.
The current logic is fine but I am not sure if we need it or not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}

if (reportAction.childReportID && ignore !== 'child') {
const childActions = ReportActionUtils.getAllReportActions(reportAction.childReportID);
Object.values(childActions).forEach((action) => {
const childErrorKeys = Object.keys(action.errors ?? {}).filter((err) => errorKeys.includes(err));
clearAllRelatedReportActionErrors(reportAction.childReportID ?? '', action, 'parent', childErrorKeys);
Copy link
Contributor

@DylanDylann DylanDylann Mar 18, 2024

Choose a reason for hiding this comment

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

Please point out 1 case where we need to call clearAllRelatedReportActionErrors instead of clearReportActionErrors.
The current logic is fine but I am not sure if we need it or not

@tienifr For this point, I mean that in here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, we need to use clearAllRelatedReportActionError here since we need to clear the errors of the grandchildren/grandparent report actions. Currently we do not have any such cases but in the future there may be. clearAllRelatedReportActionErrors is more general than clearReportActionErrors so I believe it's better

});
}
}

export {
// eslint-disable-next-line import/prefer-default-export
clearReportActionErrors,
clearAllRelatedReportActionErrors,
};
2 changes: 1 addition & 1 deletion src/pages/home/report/ReportActionItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -829,7 +829,7 @@ function ReportActionItem({
/>
<View style={StyleUtils.getReportActionItemStyle(hovered || isWhisper || isContextMenuActive || !!isEmojiPickerActive || draftMessage !== undefined)}>
<OfflineWithFeedback
onClose={() => ReportActions.clearReportActionErrors(report.reportID, action)}
onClose={() => ReportActions.clearAllRelatedReportActionErrors(report.reportID, action)}
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
pendingAction={
draftMessage !== undefined ? undefined : action.pendingAction ?? (action.isOptimisticAction ? CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD : undefined)
Expand Down
38 changes: 35 additions & 3 deletions tests/actions/IOUTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -786,13 +786,12 @@ describe('actions/IOU', () => {
.then(
() =>
new Promise<void>((resolve) => {
ReportActions.clearReportActionErrors(iouReportID ?? '', iouAction);
ReportActions.clearReportActionErrors(transactionThreadReport?.reportID ?? '', transactionThreadAction);
ReportActions.clearAllRelatedReportActionErrors(iouReportID ?? '', iouAction);
resolve();
}),
)

// Then the reportAction should be removed from Onyx
// Then the reportAction from chat report should be removed from Onyx
.then(
() =>
new Promise<void>((resolve) => {
Expand All @@ -809,6 +808,39 @@ describe('actions/IOU', () => {
}),
)

// Then the reportAction from iou report should be removed from Onyx
.then(
() =>
new Promise<void>((resolve) => {
const connectionID = Onyx.connect({
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${iouReportID}`,
waitForCollectionCallback: false,
callback: (reportActionsForReport) => {
Onyx.disconnect(connectionID);
iouAction = Object.values(reportActionsForReport ?? {}).find((reportAction) => reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.IOU) ?? null;
expect(iouAction).toBeFalsy();
resolve();
},
});
}),
)

// Then the reportAction from transaction report should be removed from Onyx
.then(
() =>
new Promise<void>((resolve) => {
const connectionID = Onyx.connect({
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${transactionThreadReport?.reportID}`,
waitForCollectionCallback: false,
callback: (reportActionsForReport) => {
Onyx.disconnect(connectionID);
expect(reportActionsForReport).toMatchObject({});
resolve();
},
});
}),
)

// Along with the associated transaction
.then(
() =>
Expand Down
Loading