Skip to content

Commit

Permalink
Merge pull request #37930 from Expensify/arosiclair-offline-request-fix
Browse files Browse the repository at this point in the history
[No QA] Fix optimistic reports getting cleared by unnecessary OpenReport calls
  • Loading branch information
amyevans authored Mar 21, 2024
2 parents 657b02b + 9e0304e commit 956d14f
Show file tree
Hide file tree
Showing 6 changed files with 44 additions and 8 deletions.
4 changes: 4 additions & 0 deletions src/libs/ReportUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4212,6 +4212,10 @@ function canAccessReport(report: OnyxEntry<Report>, policies: OnyxCollection<Pol
return false;
}

if (report?.errorFields?.notFound) {
return false;
}

return true;
}

Expand Down
1 change: 1 addition & 0 deletions src/libs/actions/IOU.ts
Original file line number Diff line number Diff line change
Expand Up @@ -631,6 +631,7 @@ function buildOnyxDataForMoneyRequest(
value: {
pendingFields: null,
errorFields: null,
isOptimisticReport: false,
},
});
}
Expand Down
9 changes: 9 additions & 0 deletions src/libs/actions/Report.ts
Original file line number Diff line number Diff line change
Expand Up @@ -578,6 +578,15 @@ function openReport(
];

const successData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
value: {
errorFields: {
notFound: null,
},
},
},
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportID}`,
Expand Down
7 changes: 7 additions & 0 deletions src/libs/shouldFetchReport.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import type Report from '@src/types/onyx/Report';

export default function shouldFetchReport(report: Report) {
// If the report is optimistic, there's no need to fetch it. The original action should create it.
// If there is an error for creating the chat, there's no need to fetch it since it doesn't exist
return !report?.isOptimisticReport && !report?.errorFields?.createChat;
}
13 changes: 11 additions & 2 deletions src/pages/home/ReportScreen.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import Performance from '@libs/Performance';
import * as PersonalDetailsUtils from '@libs/PersonalDetailsUtils';
import * as ReportActionsUtils from '@libs/ReportActionsUtils';
import * as ReportUtils from '@libs/ReportUtils';
import shouldFetchReport from '@libs/shouldFetchReport';
import type {CentralPaneNavigatorParamList} from '@navigation/types';
import variables from '@styles/variables';
import * as ComposerActions from '@userActions/Composer';
Expand Down Expand Up @@ -263,7 +264,11 @@ function ReportScreen({
// There are no reportActions at all to display and we are still in the process of loading the next set of actions.
const isLoadingInitialReportActions = reportActions.length === 0 && !!reportMetadata?.isLoadingInitialReportActions;
const isOptimisticDelete = report.statusNum === CONST.REPORT.STATUS_NUM.CLOSED;
const shouldHideReport = !ReportUtils.canAccessReport(report, policies, betas);

// If there's a non-404 error for the report we should show it instead of blocking the screen
const hasHelpfulErrors = Object.keys(report?.errorFields ?? {}).some((key) => key !== 'notFound');
const shouldHideReport = !hasHelpfulErrors && !ReportUtils.canAccessReport(report, policies, betas);

const isLoading = !reportIDFromRoute || !isSidebarLoaded || PersonalDetailsUtils.isPersonalDetailsEmpty();
const lastReportAction: OnyxEntry<OnyxTypes.ReportAction> = useMemo(
() =>
Expand Down Expand Up @@ -356,8 +361,12 @@ function ReportScreen({
return;
}

if (!shouldFetchReport(report)) {
return;
}

fetchReport();
}, [report.reportID, reportMetadata?.isLoadingInitialReportActions, fetchReport, reportIDFromRoute]);
}, [report, reportMetadata?.isLoadingInitialReportActions, fetchReport, reportIDFromRoute]);

const dismissBanner = useCallback(() => {
setIsBannerVisible(false);
Expand Down
18 changes: 12 additions & 6 deletions src/pages/home/report/ReportActionsView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,14 @@ import Performance from '@libs/Performance';
import * as ReportActionsUtils from '@libs/ReportActionsUtils';
import {isUserCreatedPolicyRoom} from '@libs/ReportUtils';
import {didUserLogInDuringSession} from '@libs/SessionUtils';
import shouldFetchReport from '@libs/shouldFetchReport';
import {ReactionListContext} from '@pages/home/ReportScreenContext';
import * as Report from '@userActions/Report';
import Timing from '@userActions/Timing';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import type SCREENS from '@src/SCREENS';
import type * as OnyxTypes from '@src/types/onyx';
import {isEmptyObject} from '@src/types/utils/EmptyObject';
import getInitialPaginationSize from './getInitialPaginationSize';
import PopoverReactionList from './ReactionList/PopoverReactionList';
import ReportActionsList from './ReportActionsList';
Expand Down Expand Up @@ -113,15 +113,21 @@ function ReportActionsView({
const isReportFullyVisible = useMemo((): boolean => getIsReportFullyVisible(isFocused), [isFocused]);

const openReportIfNecessary = () => {
const createChatError = report.errorFields?.createChat;
// If the report is optimistic (AKA not yet created) we don't need to call openReport again
if (!!report.isOptimisticReport || !isEmptyObject(createChatError)) {
if (!shouldFetchReport(report)) {
return;
}

Report.openReport(reportID, reportActionID);
};

const reconnectReportIfNecessary = () => {
if (!shouldFetchReport(report)) {
return;
}

Report.reconnect(reportID);
};

useLayoutEffect(() => {
setCurrentReportActionID('');
}, [route]);
Expand Down Expand Up @@ -223,7 +229,7 @@ function ReportActionsView({
if (isReportFullyVisible) {
openReportIfNecessary();
} else {
Report.reconnect(reportID);
reconnectReportIfNecessary();
}
}
// update ref with current network state
Expand All @@ -237,7 +243,7 @@ function ReportActionsView({
if (isReportFullyVisible) {
openReportIfNecessary();
} else {
Report.reconnect(reportID);
reconnectReportIfNecessary();
}
}
// eslint-disable-next-line react-hooks/exhaustive-deps
Expand Down

0 comments on commit 956d14f

Please sign in to comment.