[HOLD for payment 2022-11-09] [$250] App crashes when navigating to Room details, participants, and settings pages with account that doesn't have access

[Beamanator]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Navigate to a room, open the details page
2. Copy the URL
3. Log out
4. Log in as an account that doesn't have access to that room
5. Go to the URL from step 2

Note: Navigating to the room participants page & settings page also cause the same error

Expected Result:

User is redirected to their home page

Actual Result:

"Uh-oh, something went wrong" message is shown

Workaround:

Yes they can refresh the page

Platform:

Where is this issue occurring?

• Web
• Mobile Web

Version Number: Current
Reproducible in staging?: Yes
Reproducible in production?: Unknown
Notes/Photos/Videos: #11849 (comment)
Issue reported by: @sobitneupane
Slack conversation: None

View all open jobs on GitHub

[melvin-bot]

Triggered auto assignment to @puneetlath (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav (External)

[melvin-bot]

Current assignee @Beamanator is eligible for the External assigner, not assigning anyone new.

[mollfpr]

Proposal

The issue occurred when the report from Onyx return undefined on ReportDetailsPage, ReportParticipantsPageandReportSettingsPage`.

Solution
Creating a HOC to get the report data from Onyx will navigate to the home page if the report data is not available. Then updating of the above 3 pages to use the HOC.

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,
    };

    const defaultProps = {
        forwardedRef: () => {},
        report: {},
    };

    const WithReport = (props) => {
        if (_.isEmpty(props.report)) {
            Navigation.dismissModal();
            return;
        }

        const rest = _.omit(props, ['forwardedRef']);

        return (
            <WrappedComponent
                // eslint-disable-next-line react/jsx-props-no-spreading
                {...rest}
                ref={props.forwardedRef}
            />
        );
    };

    WithReport.propTypes = propTypes;
    WithReport.defaultProps = defaultProps;
    WithReport.displayName = `withReport(${getComponentDisplayName(WrappedComponent)})`;
    const withReport = React.forwardRef((props, ref) => (
        // eslint-disable-next-line react/jsx-props-no-spreading
        <WithReport {...props} forwardedRef={ref} />
    ));

    return withOnyx({
        report: {
            key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${route.params.reportID}`,
        },
    })(withReport);
}

diff --git a/src/pages/ReportDetailsPage.js b/src/pages/ReportDetailsPage.js
index e76c29c72..43faf41eb 100644
--- a/src/pages/ReportDetailsPage.js
+++ b/src/pages/ReportDetailsPage.js
@@ -22,6 +22,7 @@ import MenuItem from '../components/MenuItem';
 import Text from '../components/Text';
 import CONST from '../CONST';
 import reportPropTypes from './reportPropTypes';
+import withReport from './home/report/withReport';

 const propTypes = {
     ...withLocalizePropTypes,
@@ -177,10 +178,8 @@ ReportDetailsPage.propTypes = propTypes;

 export default compose(
     withLocalize,
+    withReport,
     withOnyx({
-        report: {
-            key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${route.params.reportID}`,
-        },
         personalDetails: {
             key: ONYXKEYS.PERSONAL_DETAILS,
         },
diff --git a/src/pages/ReportParticipantsPage.js b/src/pages/ReportParticipantsPage.js
index f66c0cbfd..8f8a27323 100755
--- a/src/pages/ReportParticipantsPage.js
+++ b/src/pages/ReportParticipantsPage.js
@@ -19,6 +19,7 @@ import withLocalize, {withLocalizePropTypes} from '../components/withLocalize';
 import compose from '../libs/compose';
 import * as ReportUtils from '../libs/ReportUtils';
 import reportPropTypes from './reportPropTypes';
+import withReport from './home/report/withReport';

 const propTypes = {
     /* Onyx Props */
@@ -113,12 +114,10 @@ ReportParticipantsPage.displayName = 'ReportParticipantsPage';

 export default compose(
     withLocalize,
+    withReport,
     withOnyx({
         personalDetails: {
             key: ONYXKEYS.PERSONAL_DETAILS,
         },
-        report: {
-            key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${route.params.reportID}`,
-        },
     }),
 )(ReportParticipantsPage);
diff --git a/src/pages/ReportSettingsPage.js b/src/pages/ReportSettingsPage.js
index 52c1ec527..58df7da72 100644
--- a/src/pages/ReportSettingsPage.js
+++ b/src/pages/ReportSettingsPage.js
@@ -21,6 +21,7 @@ import Picker from '../components/Picker';
 import * as ValidationUtils from '../libs/ValidationUtils';
 import OfflineWithFeedback from '../components/OfflineWithFeedback';
 import reportPropTypes from './reportPropTypes';
+import withReport from './home/report/withReport';

 const propTypes = {
     /** Route params */
@@ -243,10 +244,8 @@ ReportSettingsPage.propTypes = propTypes;

 export default compose(
     withLocalize,
+    withReport,
     withOnyx({
-        report: {
-            key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${route.params.reportID}`,
-        },
         policies: {
             key: ONYXKEYS.COLLECTION.POLICY,
         },

Result

Screen.Recording.2022-10-19.at.21.57.05.mov

[melvin-bot]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[mananjadhav]

I am okay with @mollfpr's proposal here. I am just not sure if:

• we should do an HoC like the proposal sugggested
• A parent component like WorkspacePageWithSections
• or just add the if(!report) checks in three pages.

What do you think @Beamanator ?

🎀 👀 🎀 
C+ reviewed

[mollfpr]

A parent component like WorkspacePageWithSections

Honestly, I like this approach but we still need to update the code that throws an error because of the render call before we dismiss the modal.

or just add the if(!report) checks in three pages.

I guess we can't do this because we want to make all code DRY.
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)

[melvin-bot]

@puneetlath, @mananjadhav, @Beamanator Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

@puneetlath, @mananjadhav, @Beamanator Whoops! This issue is 2 days overdue. Let's get this updated quick!

[Beamanator]

Interesting, honestly I like @mollfpr 's proposal, making an HOC and using that to check for a Report & pass it down, or navigate home if it doesn't exist. What are y'all's concerns with that approach?

Also, what do we think about naming it something more useful? I kinda feel like withReport is good but maybe needs to mention something about navigating home if Report doesn't exist? Maybe withReportOrNavigateHome but that doesn't sound super great

[mollfpr]

@Beamanator Yeah, I kinda got a hard time naming stuff 😅

withReportOrNavigateHome sounds good to me!

[Beamanator]

@mollfpr LOL yeah sometimes me too 😅

Cool I like your proposal, and @mananjadhav approved here so hiring!

[melvin-bot]

📣 @mollfpr You have been assigned to this job by @Beamanator!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.22-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Create HOC for get report data or navigate to home #12089
• Create HOC for get report data or navigate to home #12089

If no regressions arise, payment will be issued on 2022-11-09. 🎊

[puneetlath]

@sobitneupane @mananjadhav @mollfpr can you please apply to the Upwork job here: https://www.upwork.com/jobs/~018e89dfcc7471e60c

[sobitneupane]

@puneetlath Applied.

[mollfpr]

Applied too

[puneetlath]

@mananjadhav can you please help with the step of identifying the PR where this bug was introduced?

[mananjadhav]

@puneetlath I would say this bug existed since the inception of the creation of the components. For example, ReportDetailsPage was created in this PR #3742.

We're directly fetching the report from Onyx and then accessing it's properties, this.props.report.reportName, etc. which crashed the App. These undefined checks were added in these PRs.

Do you need something specific here?

[puneetlath]

That's perfect, thanks @mananjadhav!

[aimane-chnaif]

I think #12428 is a regression from this PR

[mollfpr]

Thanks for pointing it out @aimane-chnaif

I just did some investigation and we probably miss the fact that we should fetch the report data on the withReportOrNavigateHome when the report is not available because we directly go to the detail page.

App/src/pages/home/ReportScreen.js

Lines 165 to 176 in 599f1b9

	fetchReportIfNeeded() {
	const reportIDFromPath = getReportID(this.props.route);
	// It possible that we may not have the report object yet in Onyx yet e.g. we navigated to a URL for an accessible report that
	// is not stored locally yet. If props.report.reportID exists, then the report has been stored locally and nothing more needs to be done.
	// If it doesn't exist, then we fetch the report from the API.
	if (this.props.report.reportID) {
	return;
	}
	Report.openReport(reportIDFromPath);
	}

App/src/pages/home/ReportScreen.js

Lines 122 to 135 in 599f1b9

	componentDidMount() {
	this.fetchReportIfNeeded();
	toggleReportActionComposeView(true);
	this.removeViewportResizeListener = addViewportResizeListener(this.updateViewportOffsetTop);
	}
	componentDidUpdate(prevProps) {
	if (this.props.route.params.reportID === prevProps.route.params.reportID) {
	return;
	}
	this.fetchReportIfNeeded();
	toggleReportActionComposeView(true);
	}

And also we need to update the menuItems on ReportDetailsPage to set the menu items according to the report either from fetching or the Onyx.

App/src/pages/ReportDetailsPage.js

Lines 60 to 96 in 599f1b9

	this.menuItems = [];
	if (ReportUtils.isArchivedRoom(this.props.report)) {
	return;
	}
	// All nonarchived chats should let you see their members
	this.menuItems.push({
	key: CONST.REPORT_DETAILS_MENU_ITEM.MEMBERS,
	translationKey: 'common.members',
	icon: Expensicons.Users,
	subtitle: lodashGet(props.report, 'participants', []).length,
	action: () => { Navigation.navigate(ROUTES.getReportParticipantsRoute(props.report.reportID)); },
	});
	if (ReportUtils.isPolicyExpenseChat(this.props.report) || ReportUtils.isChatRoom(this.props.report)) {
	this.menuItems.push({
	key: CONST.REPORT_DETAILS_MENU_ITEM.SETTINGS,
	translationKey: 'common.settings',
	icon: Expensicons.Gear,
	action: () => { Navigation.navigate(ROUTES.getReportSettingsRoute(props.report.reportID)); },
	});
	}
	if (ReportUtils.isUserCreatedPolicyRoom(this.props.report)) {
	this.menuItems.push({
	key: CONST.REPORT_DETAILS_MENU_ITEM.INVITE,
	translationKey: 'common.invite',
	icon: Expensicons.Plus,
	action: () => { /* Placeholder for when inviting other users is built in */ },
	},
	{
	key: CONST.REPORT_DETAILS_MENU_ITEM.LEAVE_ROOM,
	translationKey: 'common.leaveRoom',
	icon: Expensicons.Exit,
	action: () => { /* Placeholder for when leaving rooms is built in */ },
	});
	}

@mananjadhav @puneetlath @Beamanator Let me know what action we need to do here since the expected result is done and the #12428 need more work.

[puneetlath]

@mollfpr can you post that solution over on #12428 since the PR is already done for this issue?

[mollfpr]

@puneetlath Yup will do it shortly, thanks!

[parasharrajat]

Let's hold the payment for this issue until I have reviewed the suspected regression. If this really caused #12428 then payment will be made after that is fixed.

[melvin-bot]

@puneetlath, @mananjadhav, @Beamanator, @mollfpr Whoops! This issue is 2 days overdue. Let's get this updated quick!

[Beamanator]

Seems like payment is still on hold, see related discussions in #12676

[puneetlath]

Ok, so based on the discussion in #12676 I'm going to go ahead and close this out. Everyone has already been paid and any further reverts or fixes can happen in #12676.