From d445cd8ea34b6c7fc5db06e4cd6a0ee0a4b4333e Mon Sep 17 00:00:00 2001 From: Jakub Kosmydel <104823336+kosmydel@users.noreply.github.com> Date: Tue, 4 Jul 2023 14:17:33 +0200 Subject: [PATCH 1/8] Refactor ReportScreenWrapper to functional component --- .../AppNavigator/ReportScreenWrapper.js | 69 +++++++------------ 1 file changed, 24 insertions(+), 45 deletions(-) diff --git a/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js b/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js index 37ecc92a8f55..e96db4735ccb 100644 --- a/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js +++ b/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js @@ -1,4 +1,4 @@ -import React, {Component} from 'react'; +import React, {useEffect} from 'react'; import PropTypes from 'prop-types'; import lodashGet from 'lodash/get'; import {withOnyx} from 'react-native-onyx'; @@ -72,62 +72,41 @@ const getLastAccessedReportID = (reports, ignoreDefaultRooms, policies, isFirstT }; // This wrapper is reponsible for opening the last accessed report if there is no reportID specified in the route params -class ReportScreenWrapper extends Component { - constructor(props) { - super(props); - - // If there is no reportID in route, try to find last accessed and use it for setParams - if (!lodashGet(this.props.route, 'params.reportID', null)) { - const reportID = getLastAccessedReportID( - this.props.reports, - !Permissions.canUseDefaultRooms(this.props.betas), - this.props.policies, - this.props.isFirstTimeNewExpensifyUser, - this.props.route.params.openOnAdminRoom, - ); - - // It's possible that props.reports aren't fully loaded yet - // in that case the reportID is undefined - if (reportID) { - this.props.navigation.setParams({reportID: String(reportID)}); - } else { - App.confirmReadyToOpenApp(); - } - } - } - - shouldComponentUpdate(nextProps) { +function ReportScreenWrapper(props) { + useEffect(() => { // Don't update if there is a reportID in the params already - if (lodashGet(this.props.route, 'params.reportID', null)) { + if (lodashGet(props.route, 'params.reportID', null)) { App.confirmReadyToOpenApp(); - return false; + return; } - // If the reports weren't fully loaded in the constructor, - // try to get and set reportID again + // If there is no reportID in route, try to find last accessed and use it for setParams const reportID = getLastAccessedReportID( - nextProps.reports, - !Permissions.canUseDefaultRooms(nextProps.betas), - nextProps.policies, - lodashGet(nextProps, 'route.params.openOnAdminRoom', false), + props.reports, + !Permissions.canUseDefaultRooms(props.betas), + props.policies, + props.isFirstTimeNewExpensifyUser, + lodashGet(props.route, 'params.openOnAdminRoom', false), ); + // It's possible that props.reports aren't fully loaded yet + // in that case the reportID is undefined if (reportID) { - this.props.navigation.setParams({reportID: String(reportID)}); - return true; + props.navigation.setParams({reportID: String(reportID)}); + } else { + App.confirmReadyToOpenApp(); } - return false; + }, [props.route, props.navigation, props.reports, props.betas, props.policies, props.isFirstTimeNewExpensifyUser]); + + // Wait until there is reportID in the route params + if (lodashGet(props.route, 'params.reportID', null)) { + return ; } - render() { - // Wait until there is reportID in the route params - if (lodashGet(this.props.route, 'params.reportID', null)) { - return ; - } + return ; +}; + - return ; - } -} ReportScreenWrapper.propTypes = propTypes; ReportScreenWrapper.defaultProps = defaultProps; From 86f4578b4985b9ea865dd637d473e4064af70ec9 Mon Sep 17 00:00:00 2001 From: Jakub Kosmydel <104823336+kosmydel@users.noreply.github.com> Date: Tue, 11 Jul 2023 10:43:27 +0200 Subject: [PATCH 2/8] Prettier --- src/libs/Navigation/AppNavigator/ReportScreenWrapper.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js b/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js index e96db4735ccb..6fb08d839aa4 100644 --- a/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js +++ b/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js @@ -104,9 +104,7 @@ function ReportScreenWrapper(props) { } return ; -}; - - +} ReportScreenWrapper.propTypes = propTypes; ReportScreenWrapper.defaultProps = defaultProps; From 2cf2ca0dc34648c17eca7300e72822d294e1aec7 Mon Sep 17 00:00:00 2001 From: Jakub Kosmydel <104823336+kosmydel@users.noreply.github.com> Date: Tue, 11 Jul 2023 12:22:44 +0200 Subject: [PATCH 3/8] Use usePermissions hook --- .../Navigation/AppNavigator/ReportScreenWrapper.js | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js b/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js index 6fb08d839aa4..42e512ade0a4 100644 --- a/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js +++ b/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js @@ -4,7 +4,6 @@ import lodashGet from 'lodash/get'; import {withOnyx} from 'react-native-onyx'; import ONYXKEYS from '../../../ONYXKEYS'; -import Permissions from '../../Permissions'; import ReportScreen from '../../../pages/home/ReportScreen'; import * as ReportUtils from '../../ReportUtils'; @@ -12,14 +11,12 @@ import reportPropTypes from '../../../pages/reportPropTypes'; import FullScreenLoadingIndicator from '../../../components/FullscreenLoadingIndicator'; import {withNavigationPropTypes} from '../../../components/withNavigation'; import * as App from '../../actions/App'; +import usePermissions from '../../../hooks/usePermissions'; const propTypes = { /** Available reports that would be displayed in this navigator */ reports: PropTypes.objectOf(reportPropTypes), - /** Beta features list */ - betas: PropTypes.arrayOf(PropTypes.string), - /** The policies which the user has access to */ policies: PropTypes.objectOf( PropTypes.shape({ @@ -50,7 +47,6 @@ const propTypes = { const defaultProps = { reports: {}, - betas: [], policies: {}, isFirstTimeNewExpensifyUser: false, }; @@ -73,6 +69,8 @@ const getLastAccessedReportID = (reports, ignoreDefaultRooms, policies, isFirstT // This wrapper is reponsible for opening the last accessed report if there is no reportID specified in the route params function ReportScreenWrapper(props) { + const {canUseDefaultRooms} = usePermissions(); + useEffect(() => { // Don't update if there is a reportID in the params already if (lodashGet(props.route, 'params.reportID', null)) { @@ -83,7 +81,7 @@ function ReportScreenWrapper(props) { // If there is no reportID in route, try to find last accessed and use it for setParams const reportID = getLastAccessedReportID( props.reports, - !Permissions.canUseDefaultRooms(props.betas), + !canUseDefaultRooms, props.policies, props.isFirstTimeNewExpensifyUser, lodashGet(props.route, 'params.openOnAdminRoom', false), @@ -114,9 +112,6 @@ export default withOnyx({ reports: { key: ONYXKEYS.COLLECTION.REPORT, }, - betas: { - key: ONYXKEYS.BETAS, - }, policies: { key: ONYXKEYS.COLLECTION.POLICY, }, From e1197423e8d42645e44f922e4fd23157d17d83fb Mon Sep 17 00:00:00 2001 From: Jakub Kosmydel <104823336+kosmydel@users.noreply.github.com> Date: Tue, 11 Jul 2023 13:10:47 +0200 Subject: [PATCH 4/8] Fix useEffect dependencies --- src/libs/Navigation/AppNavigator/ReportScreenWrapper.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js b/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js index 42e512ade0a4..3ffc7d54784f 100644 --- a/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js +++ b/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js @@ -94,7 +94,7 @@ function ReportScreenWrapper(props) { } else { App.confirmReadyToOpenApp(); } - }, [props.route, props.navigation, props.reports, props.betas, props.policies, props.isFirstTimeNewExpensifyUser]); + }, [props.route, props.navigation, props.reports, canUseDefaultRooms, props.policies, props.isFirstTimeNewExpensifyUser]); // Wait until there is reportID in the route params if (lodashGet(props.route, 'params.reportID', null)) { From 60dd0ef89252840a823f0b4911233f38d7bccbf0 Mon Sep 17 00:00:00 2001 From: Jakub Kosmydel <104823336+kosmydel@users.noreply.github.com> Date: Tue, 11 Jul 2023 14:37:43 +0200 Subject: [PATCH 5/8] Fix skeleton --- src/libs/Navigation/AppNavigator/ReportScreenWrapper.js | 8 +------- src/pages/home/ReportScreen.js | 2 +- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js b/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js index 3ffc7d54784f..fd525c0057ea 100644 --- a/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js +++ b/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js @@ -8,7 +8,6 @@ import ONYXKEYS from '../../../ONYXKEYS'; import ReportScreen from '../../../pages/home/ReportScreen'; import * as ReportUtils from '../../ReportUtils'; import reportPropTypes from '../../../pages/reportPropTypes'; -import FullScreenLoadingIndicator from '../../../components/FullscreenLoadingIndicator'; import {withNavigationPropTypes} from '../../../components/withNavigation'; import * as App from '../../actions/App'; import usePermissions from '../../../hooks/usePermissions'; @@ -96,12 +95,7 @@ function ReportScreenWrapper(props) { } }, [props.route, props.navigation, props.reports, canUseDefaultRooms, props.policies, props.isFirstTimeNewExpensifyUser]); - // Wait until there is reportID in the route params - if (lodashGet(props.route, 'params.reportID', null)) { - return ; - } - - return ; + return ; } ReportScreenWrapper.propTypes = propTypes; diff --git a/src/pages/home/ReportScreen.js b/src/pages/home/ReportScreen.js index 4571e5318a8b..e1391685eb36 100644 --- a/src/pages/home/ReportScreen.js +++ b/src/pages/home/ReportScreen.js @@ -113,7 +113,7 @@ const defaultProps = { * @returns {String} */ function getReportID(route) { - return route.params.reportID.toString(); + return lodashGet(route, 'params.reportID', null); } // Keep a reference to the list view height so we can use it when a new ReportScreen component mounts From f6e5f2a8b5e5c46de381446f86f31d03a41bedc0 Mon Sep 17 00:00:00 2001 From: Jakub Kosmydel <104823336+kosmydel@users.noreply.github.com> Date: Wed, 12 Jul 2023 10:35:19 +0200 Subject: [PATCH 6/8] Don't use Onyx when there is no reportID --- .../AppNavigator/ReportScreenWrapper.js | 10 ++- src/pages/home/ReportScreen.js | 77 +++++++++---------- 2 files changed, 46 insertions(+), 41 deletions(-) diff --git a/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js b/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js index fd525c0057ea..8723cb8f4aad 100644 --- a/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js +++ b/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js @@ -5,7 +5,7 @@ import {withOnyx} from 'react-native-onyx'; import ONYXKEYS from '../../../ONYXKEYS'; -import ReportScreen from '../../../pages/home/ReportScreen'; +import ReportScreen, {BaseReportScreen} from '../../../pages/home/ReportScreen'; import * as ReportUtils from '../../ReportUtils'; import reportPropTypes from '../../../pages/reportPropTypes'; import {withNavigationPropTypes} from '../../../components/withNavigation'; @@ -95,7 +95,13 @@ function ReportScreenWrapper(props) { } }, [props.route, props.navigation, props.reports, canUseDefaultRooms, props.policies, props.isFirstTimeNewExpensifyUser]); - return ; + // Wait until there is reportID in the route params + if (lodashGet(props.route, 'params.reportID', null)) { + return ; + } + + // If there is no reportID in route, display stripped down version of the report screen + return ; } ReportScreenWrapper.propTypes = propTypes; diff --git a/src/pages/home/ReportScreen.js b/src/pages/home/ReportScreen.js index 17abfa8dd1e4..152907217312 100644 --- a/src/pages/home/ReportScreen.js +++ b/src/pages/home/ReportScreen.js @@ -47,8 +47,8 @@ const propTypes = { params: PropTypes.shape({ /** The ID of the report this screen should display */ reportID: PropTypes.string, - }).isRequired, - }).isRequired, + }), + }), /** Tells us if the sidebar has rendered */ isSidebarLoaded: PropTypes.bool, @@ -91,8 +91,9 @@ const propTypes = { }; const defaultProps = { + route: {}, isSidebarLoaded: false, - reportActions: {}, + reportActions: [], report: { hasOutstandingIOU: false, isLoadingReportActions: false, @@ -383,39 +384,37 @@ class ReportScreen extends React.Component { ReportScreen.propTypes = propTypes; ReportScreen.defaultProps = defaultProps; -export default compose( - withViewportOffsetTop, - withLocalize, - withWindowDimensions, - withNavigationFocus, - withNavigation, - withNetwork(), - withOnyx({ - isSidebarLoaded: { - key: ONYXKEYS.IS_SIDEBAR_LOADED, - }, - reportActions: { - key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${getReportID(route)}`, - canEvict: false, - selector: ReportActionsUtils.getSortedReportActionsForDisplay, - }, - report: { - key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${getReportID(route)}`, - }, - isComposerFullSize: { - key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_IS_COMPOSER_FULL_SIZE}${getReportID(route)}`, - }, - betas: { - key: ONYXKEYS.BETAS, - }, - policies: { - key: ONYXKEYS.COLLECTION.POLICY, - }, - accountManagerReportID: { - key: ONYXKEYS.ACCOUNT_MANAGER_REPORT_ID, - }, - personalDetails: { - key: ONYXKEYS.PERSONAL_DETAILS_LIST, - }, - }), -)(ReportScreen); +// A stripped-down version of the ReportScreen without an Onyx connection. Used to display +// skeleton views while the reportID is still loading in the ReportScreenWrapper. +const BaseReportScreen = compose(withViewportOffsetTop, withLocalize, withWindowDimensions, withNavigationFocus, withNavigation, withNetwork())(ReportScreen); + +export default withOnyx({ + isSidebarLoaded: { + key: ONYXKEYS.IS_SIDEBAR_LOADED, + }, + reportActions: { + key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${getReportID(route)}`, + canEvict: false, + selector: ReportActionsUtils.getSortedReportActionsForDisplay, + }, + report: { + key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${getReportID(route)}`, + }, + isComposerFullSize: { + key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_IS_COMPOSER_FULL_SIZE}${getReportID(route)}`, + }, + betas: { + key: ONYXKEYS.BETAS, + }, + policies: { + key: ONYXKEYS.COLLECTION.POLICY, + }, + accountManagerReportID: { + key: ONYXKEYS.ACCOUNT_MANAGER_REPORT_ID, + }, + personalDetails: { + key: ONYXKEYS.PERSONAL_DETAILS_LIST, + }, +})(BaseReportScreen); + +export {BaseReportScreen}; From c56caf42eeb8e540ecf9e141a3ba9a9b77449ec3 Mon Sep 17 00:00:00 2001 From: Jakub Kosmydel <104823336+kosmydel@users.noreply.github.com> Date: Thu, 13 Jul 2023 14:23:06 +0200 Subject: [PATCH 7/8] Revert changes, add explanation --- .../AppNavigator/ReportScreenWrapper.js | 12 +-- src/pages/home/ReportScreen.js | 75 ++++++++++--------- 2 files changed, 42 insertions(+), 45 deletions(-) diff --git a/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js b/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js index 8723cb8f4aad..b5bbaf91c152 100644 --- a/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js +++ b/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js @@ -5,7 +5,7 @@ import {withOnyx} from 'react-native-onyx'; import ONYXKEYS from '../../../ONYXKEYS'; -import ReportScreen, {BaseReportScreen} from '../../../pages/home/ReportScreen'; +import ReportScreen from '../../../pages/home/ReportScreen'; import * as ReportUtils from '../../ReportUtils'; import reportPropTypes from '../../../pages/reportPropTypes'; import {withNavigationPropTypes} from '../../../components/withNavigation'; @@ -95,13 +95,9 @@ function ReportScreenWrapper(props) { } }, [props.route, props.navigation, props.reports, canUseDefaultRooms, props.policies, props.isFirstTimeNewExpensifyUser]); - // Wait until there is reportID in the route params - if (lodashGet(props.route, 'params.reportID', null)) { - return ; - } - - // If there is no reportID in route, display stripped down version of the report screen - return ; + // The ReportScreen without the reportID set will display a skeleton + // until the reportID is loaded and set in the route param + return ; } ReportScreenWrapper.propTypes = propTypes; diff --git a/src/pages/home/ReportScreen.js b/src/pages/home/ReportScreen.js index 152907217312..06754db27831 100644 --- a/src/pages/home/ReportScreen.js +++ b/src/pages/home/ReportScreen.js @@ -47,8 +47,8 @@ const propTypes = { params: PropTypes.shape({ /** The ID of the report this screen should display */ reportID: PropTypes.string, - }), - }), + }).isRequired, + }).isRequired, /** Tells us if the sidebar has rendered */ isSidebarLoaded: PropTypes.bool, @@ -91,7 +91,6 @@ const propTypes = { }; const defaultProps = { - route: {}, isSidebarLoaded: false, reportActions: [], report: { @@ -384,37 +383,39 @@ class ReportScreen extends React.Component { ReportScreen.propTypes = propTypes; ReportScreen.defaultProps = defaultProps; -// A stripped-down version of the ReportScreen without an Onyx connection. Used to display -// skeleton views while the reportID is still loading in the ReportScreenWrapper. -const BaseReportScreen = compose(withViewportOffsetTop, withLocalize, withWindowDimensions, withNavigationFocus, withNavigation, withNetwork())(ReportScreen); - -export default withOnyx({ - isSidebarLoaded: { - key: ONYXKEYS.IS_SIDEBAR_LOADED, - }, - reportActions: { - key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${getReportID(route)}`, - canEvict: false, - selector: ReportActionsUtils.getSortedReportActionsForDisplay, - }, - report: { - key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${getReportID(route)}`, - }, - isComposerFullSize: { - key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_IS_COMPOSER_FULL_SIZE}${getReportID(route)}`, - }, - betas: { - key: ONYXKEYS.BETAS, - }, - policies: { - key: ONYXKEYS.COLLECTION.POLICY, - }, - accountManagerReportID: { - key: ONYXKEYS.ACCOUNT_MANAGER_REPORT_ID, - }, - personalDetails: { - key: ONYXKEYS.PERSONAL_DETAILS_LIST, - }, -})(BaseReportScreen); - -export {BaseReportScreen}; +export default compose( + withViewportOffsetTop, + withLocalize, + withWindowDimensions, + withNavigationFocus, + withNavigation, + withNetwork(), + withOnyx({ + isSidebarLoaded: { + key: ONYXKEYS.IS_SIDEBAR_LOADED, + }, + reportActions: { + key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${getReportID(route)}`, + canEvict: false, + selector: ReportActionsUtils.getSortedReportActionsForDisplay, + }, + report: { + key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${getReportID(route)}`, + }, + isComposerFullSize: { + key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_IS_COMPOSER_FULL_SIZE}${getReportID(route)}`, + }, + betas: { + key: ONYXKEYS.BETAS, + }, + policies: { + key: ONYXKEYS.COLLECTION.POLICY, + }, + accountManagerReportID: { + key: ONYXKEYS.ACCOUNT_MANAGER_REPORT_ID, + }, + personalDetails: { + key: ONYXKEYS.PERSONAL_DETAILS_LIST, + }, + }), +)(ReportScreen); From 9f1586979f1266ad24dc1cea9fe780de26d8c872 Mon Sep 17 00:00:00 2001 From: Jakub Kosmydel <104823336+kosmydel@users.noreply.github.com> Date: Thu, 13 Jul 2023 15:59:30 +0200 Subject: [PATCH 8/8] Convert returned reportID to string --- src/pages/home/ReportScreen.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pages/home/ReportScreen.js b/src/pages/home/ReportScreen.js index 06754db27831..f489cd3f0374 100644 --- a/src/pages/home/ReportScreen.js +++ b/src/pages/home/ReportScreen.js @@ -113,7 +113,7 @@ const defaultProps = { * @returns {String} */ function getReportID(route) { - return lodashGet(route, 'params.reportID', null); + return String(lodashGet(route, 'params.reportID', null)); } // Keep a reference to the list view height so we can use it when a new ReportScreen component mounts