From b86968fe01e1673ebe38914dd29ebf1a9be33ce4 Mon Sep 17 00:00:00 2001 From: dukenv0307 Date: Tue, 1 Aug 2023 11:24:10 +0700 Subject: [PATCH 1/6] fix crash app when opening split bill page by loading --- .../withReportAndReportActionOrNotFound.js | 125 ++++++++++++++++++ src/pages/iou/SplitBillDetailsPage.js | 20 +-- 2 files changed, 127 insertions(+), 18 deletions(-) create mode 100644 src/pages/home/report/withReportAndReportActionOrNotFound.js diff --git a/src/pages/home/report/withReportAndReportActionOrNotFound.js b/src/pages/home/report/withReportAndReportActionOrNotFound.js new file mode 100644 index 00000000000..5dbfe6c7467 --- /dev/null +++ b/src/pages/home/report/withReportAndReportActionOrNotFound.js @@ -0,0 +1,125 @@ +import PropTypes from 'prop-types'; +import React from 'react'; +import {withOnyx} from 'react-native-onyx'; +import _ from 'underscore'; +import getComponentDisplayName from '../../../libs/getComponentDisplayName'; +import NotFoundPage from '../../ErrorPage/NotFoundPage'; +import ONYXKEYS from '../../../ONYXKEYS'; +import reportPropTypes from '../../reportPropTypes'; +import reportActionPropTypes from './reportActionPropTypes'; +import FullscreenLoadingIndicator from '../../../components/FullscreenLoadingIndicator'; +import * as ReportUtils from '../../../libs/ReportUtils'; +import * as ReportActionsUtils from '../../../libs/ReportActionsUtils'; + +export default function (WrappedComponent) { + const propTypes = { + /** The HOC takes an optional ref as a prop and passes it as a ref to the wrapped component. + * That way, if a ref is passed to a component wrapped in the HOC, the ref is a reference to the wrapped component, not the HOC. */ + forwardedRef: PropTypes.func, + + /** The report currently being looked at */ + report: reportPropTypes, + + /** Array of report actions for this report */ + reportActions: PropTypes.shape(reportActionPropTypes), + + /** The policies which the user has access to */ + policies: PropTypes.objectOf( + PropTypes.shape({ + /** The policy name */ + name: PropTypes.string, + + /** The type of the policy */ + type: PropTypes.string, + }), + ), + + /** Route params */ + route: PropTypes.shape({ + params: PropTypes.shape({ + /** Report ID passed via route */ + reportID: PropTypes.string, + + /** ReportActionID passed via route */ + reportActionID: PropTypes.string, + }), + }).isRequired, + + /** Beta features list */ + betas: PropTypes.arrayOf(PropTypes.string), + + /** Indicated whether the report data is loading */ + isLoadingReportData: PropTypes.bool, + }; + + const defaultProps = { + forwardedRef: () => {}, + reportActions: {}, + report: { + isLoadingReportActions: true, + }, + policies: {}, + betas: [], + isLoadingReportData: true, + }; + + // eslint-disable-next-line rulesdir/no-negated-variables + function WithReportAndReportActionOrNotFound(props) { + const isLoadingReport = props.isLoadingReportData && (_.isEmpty(props.report) || !props.report.reportID); + const shouldHideReport = !isLoadingReport && (_.isEmpty(props.report) || !props.report.reportID || !ReportUtils.canAccessReport(props.report, props.policies, props.betas)); + + let reportAction = props.reportActions[`${props.route.params.reportActionID.toString()}`]; + // Handle threads if needed + if (reportAction === undefined || reportAction.reportActionID === undefined) { + reportAction = ReportActionsUtils.getParentReportAction(props.report); + } + const isLoadingReportAction = _.isEmpty(props.reportActions) || (props.report.isLoadingReportActions && _.isEmpty(reportAction)); + + if ((isLoadingReport || isLoadingReportAction) && !shouldHideReport) { + return ; + } + if (shouldHideReport || _.isEmpty(reportAction)) { + return ; + } + const rest = _.omit(props, ['forwardedRef']); + return ( + + ); + } + + WithReportAndReportActionOrNotFound.propTypes = propTypes; + WithReportAndReportActionOrNotFound.defaultProps = defaultProps; + WithReportAndReportActionOrNotFound.displayName = `withReportAndReportActionOrNotFound(${getComponentDisplayName(WrappedComponent)})`; + + // eslint-disable-next-line rulesdir/no-negated-variables + const withReportAndReportActionOrNotFound = React.forwardRef((props, ref) => ( + + )); + + return withOnyx({ + report: { + key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${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, + }, + })(withReportAndReportActionOrNotFound); +} diff --git a/src/pages/iou/SplitBillDetailsPage.js b/src/pages/iou/SplitBillDetailsPage.js index 92edb90256b..b638da09187 100644 --- a/src/pages/iou/SplitBillDetailsPage.js +++ b/src/pages/iou/SplitBillDetailsPage.js @@ -14,7 +14,7 @@ import withLocalize, {withLocalizePropTypes} from '../../components/withLocalize import compose from '../../libs/compose'; import reportActionPropTypes from '../home/report/reportActionPropTypes'; import reportPropTypes from '../reportPropTypes'; -import withReportOrNotFound from '../home/report/withReportOrNotFound'; +import withReportAndReportActionOrNotFound from '../home/report/withReportAndReportActionOrNotFound'; import FullPageNotFoundView from '../../components/BlockingViews/FullPageNotFoundView'; import CONST from '../../CONST'; import HeaderWithBackButton from '../../components/HeaderWithBackButton'; @@ -50,18 +50,6 @@ const defaultProps = { reportActions: {}, }; -/** - * Get the reportID for the associated chatReport - * - * @param {Object} route - * @param {Object} route.params - * @param {String} route.params.reportID - * @returns {String} - */ -function getReportID(route) { - return route.params.reportID.toString(); -} - function SplitBillDetailsPage(props) { const reportAction = props.reportActions[`${props.route.params.reportActionID.toString()}`]; const participantAccountIDs = reportAction.originalMessage.participantAccountIDs; @@ -108,14 +96,10 @@ SplitBillDetailsPage.displayName = 'SplitBillDetailsPage'; export default compose( withLocalize, - withReportOrNotFound, + withReportAndReportActionOrNotFound, withOnyx({ personalDetails: { key: ONYXKEYS.PERSONAL_DETAILS_LIST, }, - reportActions: { - key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${getReportID(route)}`, - canEvict: false, - }, }), )(SplitBillDetailsPage); From a809aa3e52c3acae2e08a7e72e1e2a7c765e783c Mon Sep 17 00:00:00 2001 From: dukenv0307 Date: Tue, 1 Aug 2023 14:46:04 +0700 Subject: [PATCH 2/6] update HOC to work for small screen --- .../withReportAndReportActionOrNotFound.js | 60 +++++++++++++------ 1 file changed, 41 insertions(+), 19 deletions(-) diff --git a/src/pages/home/report/withReportAndReportActionOrNotFound.js b/src/pages/home/report/withReportAndReportActionOrNotFound.js index 5dbfe6c7467..8cda31c219c 100644 --- a/src/pages/home/report/withReportAndReportActionOrNotFound.js +++ b/src/pages/home/report/withReportAndReportActionOrNotFound.js @@ -1,5 +1,5 @@ import PropTypes from 'prop-types'; -import React from 'react'; +import React, {useEffect, useRef} from 'react'; import {withOnyx} from 'react-native-onyx'; import _ from 'underscore'; import getComponentDisplayName from '../../../libs/getComponentDisplayName'; @@ -10,6 +10,9 @@ import reportActionPropTypes from './reportActionPropTypes'; import FullscreenLoadingIndicator from '../../../components/FullscreenLoadingIndicator'; import * as ReportUtils from '../../../libs/ReportUtils'; import * as ReportActionsUtils from '../../../libs/ReportActionsUtils'; +import * as Report from '../../../libs/actions/Report'; +import compose from '../../../libs/compose'; +import withWindowDimensions from '../../../components/withWindowDimensions'; export default function (WrappedComponent) { const propTypes = { @@ -50,6 +53,9 @@ export default function (WrappedComponent) { /** Indicated whether the report data is loading */ isLoadingReportData: PropTypes.bool, + + /** Is the window width narrow, like on a mobile device? */ + isSmallScreenWidth: PropTypes.bool.isRequired, }; const defaultProps = { @@ -65,6 +71,19 @@ export default function (WrappedComponent) { // eslint-disable-next-line rulesdir/no-negated-variables function WithReportAndReportActionOrNotFound(props) { + // For small screen, we don't call openReport API when we go to a sub report page by deeplinnk + // So we need to call openReport here for small screen + const firstRef = useRef(true); + useEffect(() => { + if (!firstRef.current) { + return; + } + firstRef.current = false; + if (props.isSmallScreenWidth) { + Report.openReport(props.route.params.reportID); + } + }, [props.isSmallScreenWidth, props.route.params.reportID]); + const isLoadingReport = props.isLoadingReportData && (_.isEmpty(props.report) || !props.report.reportID); const shouldHideReport = !isLoadingReport && (_.isEmpty(props.report) || !props.report.reportID || !ReportUtils.canAccessReport(props.report, props.policies, props.betas)); @@ -104,22 +123,25 @@ export default function (WrappedComponent) { /> )); - return withOnyx({ - report: { - key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${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, - }, - })(withReportAndReportActionOrNotFound); + return compose( + withWindowDimensions, + withOnyx({ + report: { + key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${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, + }, + }), + )(withReportAndReportActionOrNotFound); } From 60a7ea3b37ad7ecb88ceab5b8f48c1fbccd3fc0f Mon Sep 17 00:00:00 2001 From: dukenv0307 Date: Wed, 2 Aug 2023 14:35:01 +0700 Subject: [PATCH 3/6] refactor HOC --- .../withReportAndReportActionOrNotFound.js | 42 +++++++++++-------- 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/src/pages/home/report/withReportAndReportActionOrNotFound.js b/src/pages/home/report/withReportAndReportActionOrNotFound.js index 8cda31c219c..cb8564e621f 100644 --- a/src/pages/home/report/withReportAndReportActionOrNotFound.js +++ b/src/pages/home/report/withReportAndReportActionOrNotFound.js @@ -1,5 +1,5 @@ import PropTypes from 'prop-types'; -import React, {useEffect, useRef} from 'react'; +import React, {useEffect, useCallback} from 'react'; import {withOnyx} from 'react-native-onyx'; import _ from 'underscore'; import getComponentDisplayName from '../../../libs/getComponentDisplayName'; @@ -61,9 +61,7 @@ export default function (WrappedComponent) { const defaultProps = { forwardedRef: () => {}, reportActions: {}, - report: { - isLoadingReportActions: true, - }, + report: {}, policies: {}, betas: [], isLoadingReportData: true, @@ -71,35 +69,43 @@ export default function (WrappedComponent) { // eslint-disable-next-line rulesdir/no-negated-variables function WithReportAndReportActionOrNotFound(props) { - // For small screen, we don't call openReport API when we go to a sub report page by deeplinnk + // 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 - const firstRef = useRef(true); useEffect(() => { - if (!firstRef.current) { + if (!props.isSmallScreenWidth || !_.isEmpty(props.report)) { return; } - firstRef.current = false; - if (props.isSmallScreenWidth) { - Report.openReport(props.route.params.reportID); + Report.openReport(props.route.params.reportID); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); + + const getReportAction = useCallback(() => { + let reportAction = props.reportActions[`${props.route.params.reportActionID}`]; + + // Handle threads if needed + if (reportAction === undefined || reportAction.reportActionID === undefined) { + reportAction = ReportActionsUtils.getParentReportAction(props.report); } - }, [props.isSmallScreenWidth, props.route.params.reportID]); + return reportAction; + }, [props.report, props.reportActions, props.route.params.reportActionID]); + + const reportAction = getReportAction(); + + // Perform all the loading checks const isLoadingReport = props.isLoadingReportData && (_.isEmpty(props.report) || !props.report.reportID); + const isLoadingReportAction = _.isEmpty(props.reportActions) || (props.report.isLoadingReportActions && _.isEmpty(getReportAction())); const shouldHideReport = !isLoadingReport && (_.isEmpty(props.report) || !props.report.reportID || !ReportUtils.canAccessReport(props.report, props.policies, props.betas)); - let reportAction = props.reportActions[`${props.route.params.reportActionID.toString()}`]; - // Handle threads if needed - if (reportAction === undefined || reportAction.reportActionID === undefined) { - reportAction = ReportActionsUtils.getParentReportAction(props.report); - } - const isLoadingReportAction = _.isEmpty(props.reportActions) || (props.report.isLoadingReportActions && _.isEmpty(reportAction)); - if ((isLoadingReport || isLoadingReportAction) && !shouldHideReport) { return ; } + + // Perform the access/not found checks if (shouldHideReport || _.isEmpty(reportAction)) { return ; } + const rest = _.omit(props, ['forwardedRef']); return ( Date: Wed, 2 Aug 2023 19:14:10 +0700 Subject: [PATCH 4/6] call API if reportAction is empty --- .../withReportAndReportActionOrNotFound.js | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/pages/home/report/withReportAndReportActionOrNotFound.js b/src/pages/home/report/withReportAndReportActionOrNotFound.js index cb8564e621f..96d348175e0 100644 --- a/src/pages/home/report/withReportAndReportActionOrNotFound.js +++ b/src/pages/home/report/withReportAndReportActionOrNotFound.js @@ -69,16 +69,6 @@ export default function (WrappedComponent) { // eslint-disable-next-line rulesdir/no-negated-variables function WithReportAndReportActionOrNotFound(props) { - // 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 (!props.isSmallScreenWidth || !_.isEmpty(props.report)) { - return; - } - Report.openReport(props.route.params.reportID); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); - const getReportAction = useCallback(() => { let reportAction = props.reportActions[`${props.route.params.reportActionID}`]; @@ -92,6 +82,16 @@ export default function (WrappedComponent) { const reportAction = getReportAction(); + // 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 (!props.isSmallScreenWidth || (!_.isEmpty(props.report) && !_.isEmpty(reportAction))) { + return; + } + Report.openReport(props.route.params.reportID); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); + // Perform all the loading checks const isLoadingReport = props.isLoadingReportData && (_.isEmpty(props.report) || !props.report.reportID); const isLoadingReportAction = _.isEmpty(props.reportActions) || (props.report.isLoadingReportActions && _.isEmpty(getReportAction())); From ead3a3d2877d3a63bcc1cece1211b27cacf8480e Mon Sep 17 00:00:00 2001 From: dukenv0307 Date: Wed, 2 Aug 2023 19:40:50 +0700 Subject: [PATCH 5/6] add dependencies for useEffect --- src/pages/home/report/withReportAndReportActionOrNotFound.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/pages/home/report/withReportAndReportActionOrNotFound.js b/src/pages/home/report/withReportAndReportActionOrNotFound.js index 96d348175e0..5f6b116ddbf 100644 --- a/src/pages/home/report/withReportAndReportActionOrNotFound.js +++ b/src/pages/home/report/withReportAndReportActionOrNotFound.js @@ -89,8 +89,7 @@ export default function (WrappedComponent) { return; } Report.openReport(props.route.params.reportID); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + }, [props.report, props.isSmallScreenWidth, props.route.params.reportID, reportAction]); // Perform all the loading checks const isLoadingReport = props.isLoadingReportData && (_.isEmpty(props.report) || !props.report.reportID); From 70e497f2a7066e1dc7a5e0e3fb9530560b0e478c Mon Sep 17 00:00:00 2001 From: dukenv0307 Date: Wed, 2 Aug 2023 20:49:25 +0700 Subject: [PATCH 6/6] optimize dependencies --- src/pages/home/report/withReportAndReportActionOrNotFound.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/pages/home/report/withReportAndReportActionOrNotFound.js b/src/pages/home/report/withReportAndReportActionOrNotFound.js index 5f6b116ddbf..9bf3e73e761 100644 --- a/src/pages/home/report/withReportAndReportActionOrNotFound.js +++ b/src/pages/home/report/withReportAndReportActionOrNotFound.js @@ -89,7 +89,8 @@ export default function (WrappedComponent) { return; } Report.openReport(props.route.params.reportID); - }, [props.report, props.isSmallScreenWidth, props.route.params.reportID, reportAction]); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [props.isSmallScreenWidth, props.route.params.reportID]); // Perform all the loading checks const isLoadingReport = props.isLoadingReportData && (_.isEmpty(props.report) || !props.report.reportID);