-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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: Migrate withReportAndReportActionOrNotFound from withOnyx to useOnyx #49460
Changes from 19 commits
07b78ce
b882c6d
8398950
7e3931c
a65f34f
242103d
ca1e4e5
77cbc79
1649082
dc936ba
cc065e8
84472ba
ddb8c9e
aa98328
0329631
5a63090
5ca151b
f70dfaf
26f1d07
0398b3f
ae366bb
2a2c70a
3702c9a
93dd702
b656710
94f75b5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,9 @@ | ||
/* eslint-disable rulesdir/no-negated-variables */ | ||
import type {StackScreenProps} from '@react-navigation/stack'; | ||
import type {ComponentType, ForwardedRef, RefAttributes} from 'react'; | ||
import React, {useCallback, useEffect} from 'react'; | ||
import type {OnyxCollection, OnyxEntry, WithOnyxState} from 'react-native-onyx'; | ||
import {withOnyx} from 'react-native-onyx'; | ||
import React, {useEffect, useMemo} from 'react'; | ||
import type {OnyxEntry} from 'react-native-onyx'; | ||
import {useOnyx} from 'react-native-onyx'; | ||
import FullscreenLoadingIndicator from '@components/FullscreenLoadingIndicator'; | ||
import useResponsiveLayout from '@hooks/useResponsiveLayout'; | ||
import getComponentDisplayName from '@libs/getComponentDisplayName'; | ||
|
@@ -16,68 +16,71 @@ import type SCREENS from '@src/SCREENS'; | |
import type * as OnyxTypes from '@src/types/onyx'; | ||
import {isEmptyObject} from '@src/types/utils/EmptyObject'; | ||
|
||
type OnyxProps = { | ||
type WithReportAndReportActionOrNotFoundProps = StackScreenProps< | ||
FlagCommentNavigatorParamList & SplitDetailsNavigatorParamList, | ||
typeof SCREENS.FLAG_COMMENT_ROOT | typeof SCREENS.SPLIT_DETAILS.ROOT | ||
> & { | ||
/** The report currently being looked at */ | ||
report: OnyxEntry<OnyxTypes.Report>; | ||
report: OnyxTypes.Report; | ||
|
||
/** The reportAction from the current route */ | ||
reportAction: OnyxTypes.ReportAction; | ||
|
||
/** The parent report if the current report is a thread and it has a parent */ | ||
parentReport: OnyxEntry<OnyxTypes.Report>; | ||
|
||
/** The report metadata */ | ||
reportMetadata: OnyxEntry<OnyxTypes.ReportMetadata>; | ||
|
||
/** Array of report actions for this report */ | ||
reportActions: OnyxEntry<OnyxTypes.ReportActions>; | ||
|
||
/** The report's parentReportAction */ | ||
parentReportAction: NonNullable<OnyxEntry<OnyxTypes.ReportAction>> | null; | ||
|
||
/** The policies which the user has access to */ | ||
policies: OnyxCollection<OnyxTypes.Policy>; | ||
|
||
/** Beta features list */ | ||
betas: OnyxEntry<OnyxTypes.Beta[]>; | ||
|
||
/** Indicated whether the report data is loading */ | ||
isLoadingReportData: OnyxEntry<boolean>; | ||
}; | ||
|
||
type WithReportAndReportActionOrNotFoundProps = OnyxProps & | ||
StackScreenProps<FlagCommentNavigatorParamList & SplitDetailsNavigatorParamList, typeof SCREENS.FLAG_COMMENT_ROOT | typeof SCREENS.SPLIT_DETAILS.ROOT>; | ||
|
||
export default function <TProps extends WithReportAndReportActionOrNotFoundProps, TRef>( | ||
WrappedComponent: ComponentType<TProps & RefAttributes<TRef>>, | ||
): ComponentType<Omit<TProps & RefAttributes<TRef>, keyof OnyxProps>> { | ||
): ComponentType<TProps & RefAttributes<TRef>> { | ||
function WithReportOrNotFound(props: TProps, ref: ForwardedRef<TRef>) { | ||
const getReportAction = useCallback(() => { | ||
let reportAction: OnyxEntry<OnyxTypes.ReportAction> = props.reportActions?.[`${props.route.params.reportActionID}`]; | ||
const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${props.route.params.reportID}`); | ||
const [parentReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${report ? report.parentReportID : '-1'}`); | ||
const [reportMetadata] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_METADATA}${props.route.params.reportID}`); | ||
const [isLoadingReportData] = useOnyx(ONYXKEYS.IS_LOADING_REPORT_DATA); | ||
const [betas] = useOnyx(ONYXKEYS.BETAS); | ||
const [policies] = useOnyx(ONYXKEYS.COLLECTION.POLICY); | ||
const [reportActions] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${props.route.params.reportID}`, {canEvict: false}); | ||
const [parentReportAction] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report ? report.parentReportID : '-1'}`, { | ||
selector: (parentReportActions) => { | ||
const parentReportActionID = report?.parentReportActionID; | ||
if (!parentReportActionID) { | ||
return null; | ||
} | ||
return parentReportActions?.[parentReportActionID] ?? null; | ||
}, | ||
canEvict: false, | ||
}); | ||
const linkedReportAction = useMemo(() => { | ||
let reportAction: OnyxEntry<OnyxTypes.ReportAction> = reportActions?.[`${props.route.params.reportActionID}`]; | ||
|
||
// Handle threads if needed | ||
if (!reportAction?.reportActionID) { | ||
reportAction = props?.parentReportAction ?? undefined; | ||
reportAction = parentReportAction ?? undefined; | ||
} | ||
|
||
return reportAction; | ||
}, [props.reportActions, props.route.params.reportActionID, props.parentReportAction]); | ||
|
||
const reportAction = getReportAction(); | ||
}, [reportActions, props.route.params.reportActionID, parentReportAction]); | ||
|
||
const {shouldUseNarrowLayout} = useResponsiveLayout(); | ||
|
||
// For small screen, we don't call openReport API when we go to a sub report page by deeplink | ||
// So we need to call openReport here for small screen | ||
useEffect(() => { | ||
if (!shouldUseNarrowLayout || (!isEmptyObject(props.report) && !isEmptyObject(reportAction))) { | ||
if (!shouldUseNarrowLayout || (!isEmptyObject(report) && !isEmptyObject(linkedReportAction))) { | ||
return; | ||
} | ||
Report.openReport(props.route.params.reportID); | ||
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps | ||
}, [shouldUseNarrowLayout, props.route.params.reportID]); | ||
|
||
// Perform all the loading checks | ||
const isLoadingReport = props.isLoadingReportData && !props.report?.reportID; | ||
const isLoadingReportAction = isEmptyObject(props.reportActions) || (props.reportMetadata?.isLoadingInitialReportActions && isEmptyObject(getReportAction())); | ||
const shouldHideReport = !isLoadingReport && (!props.report?.reportID || !ReportUtils.canAccessReport(props.report, props.policies, props.betas)); | ||
const isLoadingReport = isLoadingReportData && !report?.reportID; | ||
const isLoadingReportAction = isEmptyObject(reportActions) || (reportMetadata?.isLoadingInitialReportActions && isEmptyObject(linkedReportAction)); | ||
const shouldHideReport = !isLoadingReport && (!report?.reportID || !ReportUtils.canAccessReport(report, policies, betas)); | ||
|
||
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
if ((isLoadingReport || isLoadingReportAction) && !shouldHideReport) { | ||
|
@@ -86,57 +89,28 @@ export default function <TProps extends WithReportAndReportActionOrNotFoundProps | |
|
||
// Perform the access/not found checks | ||
// Be sure to avoid showing the not-found page while the parent report actions are still being read from Onyx. The parentReportAction will be undefined while it's being read from Onyx | ||
// and then reportAction will either be a valid parentReportAction or an empty object. In the case of an empty object, then it's OK to show the not-found page. | ||
if (shouldHideReport || (props?.parentReportAction !== undefined && isEmptyObject(reportAction))) { | ||
// and then linkedReportAction will either be a valid parentReportAction or an empty object. In the case of an empty object, then it's OK to show the not-found page. | ||
if (shouldHideReport || (parentReportAction !== undefined && isEmptyObject(linkedReportAction))) { | ||
return <NotFoundPage />; | ||
} | ||
|
||
return ( | ||
<WrappedComponent | ||
// eslint-disable-next-line react/jsx-props-no-spreading | ||
{...props} | ||
report={report} | ||
parentReport={parentReport} | ||
reportActions={reportActions} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we still need to pass There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I forgot to remove |
||
reportAction={linkedReportAction} | ||
parentReportAction={parentReportAction} | ||
ref={ref} | ||
/> | ||
); | ||
} | ||
|
||
WithReportOrNotFound.displayName = `withReportOrNotFound(${getComponentDisplayName(WrappedComponent)})`; | ||
|
||
return withOnyx<TProps & RefAttributes<TRef>, OnyxProps>({ | ||
report: { | ||
key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${route.params.reportID}`, | ||
}, | ||
parentReport: { | ||
key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT}${report ? report.parentReportID : '-1'}`, | ||
}, | ||
reportMetadata: { | ||
key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_METADATA}${route.params.reportID}`, | ||
}, | ||
isLoadingReportData: { | ||
key: ONYXKEYS.IS_LOADING_REPORT_DATA, | ||
}, | ||
betas: { | ||
key: ONYXKEYS.BETAS, | ||
}, | ||
policies: { | ||
key: ONYXKEYS.COLLECTION.POLICY, | ||
}, | ||
reportActions: { | ||
key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${route.params.reportID}`, | ||
canEvict: false, | ||
}, | ||
parentReportAction: { | ||
key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report ? report.parentReportID : 0}`, | ||
selector: (parentReportActions: OnyxEntry<OnyxTypes.ReportActions>, props?: WithOnyxState<OnyxProps>): NonNullable<OnyxEntry<OnyxTypes.ReportAction>> | null => { | ||
const parentReportActionID = props?.report?.parentReportActionID; | ||
if (!parentReportActionID) { | ||
return null; | ||
} | ||
return parentReportActions?.[parentReportActionID] ?? null; | ||
}, | ||
canEvict: false, | ||
}, | ||
})(React.forwardRef(WithReportOrNotFound)); | ||
return React.forwardRef(WithReportOrNotFound); | ||
} | ||
|
||
export type {WithReportAndReportActionOrNotFoundProps}; |
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,9 +1,7 @@ | ||||||||||||||
import type {StackScreenProps} from '@react-navigation/stack'; | ||||||||||||||
import type {ComponentType} from 'react'; | ||||||||||||||
import React, {useCallback, useMemo, useState} from 'react'; | ||||||||||||||
import React, {useCallback, useState} from 'react'; | ||||||||||||||
import {View} from 'react-native'; | ||||||||||||||
import {withOnyx} from 'react-native-onyx'; | ||||||||||||||
import type {OnyxEntry} from 'react-native-onyx'; | ||||||||||||||
import {useOnyx} from 'react-native-onyx'; | ||||||||||||||
import FullPageNotFoundView from '@components/BlockingViews/FullPageNotFoundView'; | ||||||||||||||
import HeaderWithBackButton from '@components/HeaderWithBackButton'; | ||||||||||||||
import Icon from '@components/Icon'; | ||||||||||||||
|
@@ -26,46 +24,25 @@ import * as IOU from '@userActions/IOU'; | |||||||||||||
import CONST from '@src/CONST'; | ||||||||||||||
import ONYXKEYS from '@src/ONYXKEYS'; | ||||||||||||||
import type SCREENS from '@src/SCREENS'; | ||||||||||||||
import type {PersonalDetailsList, Report, ReportAction, Session, Transaction} from '@src/types/onyx'; | ||||||||||||||
import type {Participant} from '@src/types/onyx/IOU'; | ||||||||||||||
import type {ReportActions} from '@src/types/onyx/ReportAction'; | ||||||||||||||
import {isEmptyObject} from '@src/types/utils/EmptyObject'; | ||||||||||||||
|
||||||||||||||
type SplitBillDetailsPageTransactionOnyxProps = { | ||||||||||||||
/** The current transaction */ | ||||||||||||||
transaction: OnyxEntry<Transaction>; | ||||||||||||||
type SplitBillDetailsPageProps = WithReportAndReportActionOrNotFoundProps & StackScreenProps<SplitDetailsNavigatorParamList, typeof SCREENS.SPLIT_DETAILS.ROOT>; | ||||||||||||||
|
||||||||||||||
/** The draft transaction that holds data to be persisited on the current transaction */ | ||||||||||||||
draftTransaction: OnyxEntry<Transaction>; | ||||||||||||||
}; | ||||||||||||||
|
||||||||||||||
type SplitBillDetailsPageOnyxPropsWithoutTransaction = { | ||||||||||||||
/** The personal details of the person who is logged in */ | ||||||||||||||
personalDetails: OnyxEntry<PersonalDetailsList>; | ||||||||||||||
|
||||||||||||||
/** The active report */ | ||||||||||||||
report: OnyxEntry<Report>; | ||||||||||||||
|
||||||||||||||
/** Array of report actions for this report */ | ||||||||||||||
reportActions: OnyxEntry<ReportActions>; | ||||||||||||||
|
||||||||||||||
/** Session info for the currently logged in user. */ | ||||||||||||||
session: OnyxEntry<Session>; | ||||||||||||||
}; | ||||||||||||||
|
||||||||||||||
type SplitBillDetailsPageOnyxProps = SplitBillDetailsPageTransactionOnyxProps & SplitBillDetailsPageOnyxPropsWithoutTransaction; | ||||||||||||||
|
||||||||||||||
type SplitBillDetailsPageProps = WithReportAndReportActionOrNotFoundProps & | ||||||||||||||
SplitBillDetailsPageOnyxProps & | ||||||||||||||
StackScreenProps<SplitDetailsNavigatorParamList, typeof SCREENS.SPLIT_DETAILS.ROOT>; | ||||||||||||||
|
||||||||||||||
function SplitBillDetailsPage({personalDetails, report, route, reportActions, transaction, draftTransaction, session}: SplitBillDetailsPageProps) { | ||||||||||||||
function SplitBillDetailsPage({report, reportAction}: SplitBillDetailsPageProps) { | ||||||||||||||
const styles = useThemeStyles(); | ||||||||||||||
const reportID = report?.reportID ?? '-1'; | ||||||||||||||
const {translate} = useLocalize(); | ||||||||||||||
const theme = useTheme(); | ||||||||||||||
const reportAction = useMemo(() => reportActions?.[route.params.reportActionID] ?? ({} as ReportAction), [reportActions, route.params.reportActionID]); | ||||||||||||||
const participantAccountIDs = ReportActionsUtils.isMoneyRequestAction(reportAction) ? ReportActionsUtils.getOriginalMessage(reportAction)?.participantAccountIDs ?? [] : []; | ||||||||||||||
|
||||||||||||||
const reportID = report?.reportID ?? '-1'; | ||||||||||||||
const originalMessage = reportAction && ReportActionsUtils.isMoneyRequestAction(reportAction) ? ReportActionsUtils.getOriginalMessage(reportAction) : undefined; | ||||||||||||||
const IOUTransactionID = originalMessage?.IOUTransactionID ?? '-1'; | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The original setter logic here is const IOUTransactionID = reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.IOU && originalMessage?.IOUTransactionID ? originalMessage.IOUTransactionID : 0; Can you elaborate on why it's only Moreover, be careful when using "??" operator, because if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changes on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that what we have here is correct. We still check for the reportAction being of type IOU on the previous line when we get the originalMessage There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Though @hoangzinh could be right that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the past, I had an issue that was because of changing from "||" to "??" here. It's safer to use "||" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sounds good to me |
||||||||||||||
const participantAccountIDs = originalMessage?.participantAccountIDs ?? []; | ||||||||||||||
|
||||||||||||||
const [transaction] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION}${IOUTransactionID}`); | ||||||||||||||
const [draftTransaction] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}${IOUTransactionID}`); | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc @etCoderDysto just in case you missed this comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @hoangzinh, I thought Rory has addressed your comments since he has worked on the changes on SplitBill pages. Sorry if I missed anything. Is there any changes I have to make? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's a typo here, the current Onyx key is App/src/pages/iou/SplitBillDetailsPage.tsx Line 183 in 9627f5f
Also the feedback there #49460 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change is causing lint failure. const IOUTransactionID = originalMessage?.IOUTransactionID || '-1';
Should I go back to using nullish calescing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can disable @typescript-eslint/prefer-nullish-coalescing, same here App/src/components/StateSelector.tsx Lines 86 to 88 in f83b69e
What do you guys think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or we can handle nil case here. For example: const IOUTransactionID = originalMessage?.IOUTransactionID ? originalMessage.IOUTransactionID : '-1'; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sounds good to me 👍. I have made the changes. |
||||||||||||||
const [personalDetails] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST); | ||||||||||||||
const [session] = useOnyx(ONYXKEYS.SESSION); | ||||||||||||||
|
||||||||||||||
// In case this is workspace split expense, we manually add the workspace as the second participant of the split expense | ||||||||||||||
// because we don't save any accountID in the report action's originalMessage other than the payee's accountID | ||||||||||||||
|
@@ -162,37 +139,4 @@ function SplitBillDetailsPage({personalDetails, report, route, reportActions, tr | |||||||||||||
|
||||||||||||||
SplitBillDetailsPage.displayName = 'SplitBillDetailsPage'; | ||||||||||||||
|
||||||||||||||
const WrappedComponent = withOnyx<SplitBillDetailsPageProps, SplitBillDetailsPageTransactionOnyxProps>({ | ||||||||||||||
transaction: { | ||||||||||||||
key: ({route, reportActions}) => { | ||||||||||||||
const reportAction = reportActions?.[route.params.reportActionID]; | ||||||||||||||
const originalMessage = reportAction && ReportActionsUtils.isMoneyRequestAction(reportAction) ? ReportActionsUtils.getOriginalMessage(reportAction) : undefined; | ||||||||||||||
const IOUTransactionID = reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.IOU && originalMessage?.IOUTransactionID ? originalMessage.IOUTransactionID : 0; | ||||||||||||||
return `${ONYXKEYS.COLLECTION.TRANSACTION}${IOUTransactionID}`; | ||||||||||||||
}, | ||||||||||||||
}, | ||||||||||||||
draftTransaction: { | ||||||||||||||
key: ({route, reportActions}) => { | ||||||||||||||
const reportAction = reportActions?.[route.params.reportActionID]; | ||||||||||||||
const originalMessage = reportAction && ReportActionsUtils.isMoneyRequestAction(reportAction) ? ReportActionsUtils.getOriginalMessage(reportAction) : undefined; | ||||||||||||||
const IOUTransactionID = reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.IOU && originalMessage?.IOUTransactionID ? originalMessage.IOUTransactionID : 0; | ||||||||||||||
return `${ONYXKEYS.COLLECTION.SPLIT_TRANSACTION_DRAFT}${IOUTransactionID}`; | ||||||||||||||
}, | ||||||||||||||
}, | ||||||||||||||
})(withReportAndReportActionOrNotFound(SplitBillDetailsPage) as ComponentType<SplitBillDetailsPageProps>); | ||||||||||||||
|
||||||||||||||
export default withOnyx<Omit<SplitBillDetailsPageProps, keyof SplitBillDetailsPageTransactionOnyxProps>, SplitBillDetailsPageOnyxPropsWithoutTransaction>({ | ||||||||||||||
report: { | ||||||||||||||
key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${route.params.reportID}`, | ||||||||||||||
}, | ||||||||||||||
reportActions: { | ||||||||||||||
key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${route.params.reportID}`, | ||||||||||||||
canEvict: false, | ||||||||||||||
}, | ||||||||||||||
personalDetails: { | ||||||||||||||
key: ONYXKEYS.PERSONAL_DETAILS_LIST, | ||||||||||||||
}, | ||||||||||||||
session: { | ||||||||||||||
key: ONYXKEYS.SESSION, | ||||||||||||||
}, | ||||||||||||||
})(WrappedComponent); | ||||||||||||||
export default withReportAndReportActionOrNotFound(SplitBillDetailsPage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong condition is set here, If
parentReportID
is undefined it is still concatenating to onyx key, resulting inreport_undefined
key.#50786