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.

Sign up for GitHub

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

Jump to bottom

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

Merged

Beamanator merged 3 commits into Expensify:main from mollfpr:fix-report-undefined-crash

Oct 28, 2022

Contributor

mollfpr commented Oct 24, 2022

Details

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

Fixed Issues

$ #11982
$ #11982 (comment)

Tests

Navigate to a room, open the details page or participants page, or settings page
Copy the URL
Open another account that doesn't have access to that room
Go to the URL from step 2
Verified that the page (details, participants, or settings page) modal dismisses and shows the current account chat

Verify that no errors appear in the JS console

QA Steps

Navigate to a room, open the details page or participants page, or settings page
Copy the URL
Open another account that doesn't have access to that room
Go to the URL from step 2
Verified that the page (details, participants, or settings page) modal dismisses and shows the current account chat

Verify that no errors appear in the JS console

PR Review Checklist

PR Author Checklist

PR Reviewer Checklist

The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed

Screenshots

Web

Fix.Report.Undefined.Crash.Web.mov

Mobile Web - Chrome

Fix.Report.Undefined.Crash.mWeb-Chrome.mov

Mobile Web - Safari

Fix.Report.Undefined.Crash.mWeb-Safari.mov

Desktop

Fix.Report.Undefined.Crash.Desktop.mov

iOS

Fix.Report.Undefined.Crash.iOS.mov

Android

Fix.Report.Undefined.Crash.Android.mov


          create HOC for get report data or navigate to home

mollfpr requested a review from a team as a code owner

October 24, 2022 15:26

melvin-bot bot requested review from Beamanator and mananjadhav and removed request for a team

October 24, 2022 15:26

Collaborator

mananjadhav commented Oct 25, 2022 •

edited

Loading

@mollfpr I am getting a console error when the I hit the URL. Can you please check this?

--

Please note I'll be available for limited hours this week, and expect delayed responses.

mollfpr added 2 commits

October 26, 2022 10:13


          convert the function component to class component

c31fdd2


          fix the error if report.participants not provided on redirect

d671ade

Contributor Author

mollfpr commented Oct 26, 2022

@mananjadhav I just update the HOC component to use the class component and move the navigation redirect to componentDidMount. I also add a fix after redirection if the report.participants not yet loaded and throwing error page.

Collaborator

mananjadhav commented Oct 27, 2022

Thanks for the changes @mollfpr I am going to review and test this by tomorrow.

Contributor

Beamanator commented Oct 28, 2022 •

edited

Loading

FYI @mananjadhav it looks like your checklist is a bit outdated, I see these changed lines:

// Required in the checklist:
- [ ] I verified any copy / text that was added to the app is correct English and approved by marketing by adding the `Waiting for Copy` label for a copy review on the original GH to get the correct copy.

// In your checklist:
- [ ] I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.

Beamanator approved these changes

View reviewed changes

Contributor

Beamanator left a comment

Contributor

Beamanator commented Oct 28, 2022

Code looks good to me! @mananjadhav can you try fixing your PR review checklist then we'll see if the final test passes? 👍

Collaborator

mananjadhav commented Oct 28, 2022

@Beamanator Thanks for pointing that out. I've updated the checklist and all checks are passing now.

Beamanator merged commit 7f70c1d into Expensify:main

melvin-bot bot added the Emergency label

melvin-bot bot commented Oct 28, 2022

@Beamanator looks like this was merged without the checklist test passing. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

Beamanator removed the Emergency label

Contributor

OSBotify commented Oct 28, 2022

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify mentioned this pull request

Deploy Checklist: New Expensify 2022-10-31 #12305

Closed

22 tasks

Contributor

OSBotify commented Oct 31, 2022

🚀 Deployed to staging by @Beamanator in version: 1.2.22-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

melvin-bot bot mentioned this pull request

[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 #11982

Closed

Contributor

OSBotify commented Nov 2, 2022

🚀 Deployed to production by @Julesssss in version: 1.2.22-3 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

puneetlath mentioned this pull request

[HOLD #11768] [$250] Report detail page is opened and closed immediately after login while trying to open that deeplink url reported by @0xmiroslav #12428

Closed

mollfpr mentioned this pull request

[HOLD] Restricted access - Redirection does not work #12676

Closed

marcaaron reviewed

View reviewed changes

src/pages/home/report/withReportOrNavigateHome.js

+                      report: {},
+                  };
+                  class WithReportOrNavigateHome extends Component {

Contributor

marcaaron Nov 17, 2022

Ouch, did we really need an HOC for this? Very confused about what purpose this even serves. @Beamanator would you mind summarizing why we need this?

Contributor

Beamanator Nov 21, 2022

Is your question "Why a HOC for this functionality" or "What is the point of the code in this HOC"? Will happily respond to either one tomorrow (end of my day & brain is foggy from timezone shifting)

Contributor

marcaaron Nov 21, 2022

Yes to both.

Contributor

Beamanator Nov 22, 2022

"What is the point of the code in this HOC" should be pretty straightforward: If the user is on a page that needs report data (from Onyx), we check if this.props.report is empty or not. If it's empty, we dismiss the open modal (this is what we were calling "navigate home" though that's not really accurate since we don't have a "home" screen...). If it's not empty, we show that page.

"Why a HOC for this functionality": We wanted to use this functionality across multiple pages, so thought it was clean to put it in an HOC. This lets us not have to subscribe to ${ONYXKEYS.COLLECTION.REPORT}${route.params.reportID}, on multiple pages, and instead we just wrap the page with this new HOC. We could do the same thing with a new action or lib that we call whenever these pages are mounted, but in my brain an HOC seemed more useful.

Contributor

marcaaron Nov 22, 2022

Cool, thanks and sorry for my initial reaction. I think we got the main solution for this wrong which is the important thing. I tend to only pull out the HOC when we the code reuse is expected to be very high for a solution. But if you thought we might re-use this then the HOC is certainly one way to do that.

Contributor

Beamanator Nov 23, 2022

I think we got the main solution for this wrong which is the important thing

I think you're referring to the fact that we aren't actually sure if we want to navigate at this point, right? Like we probably want to see if the user has access to that report or not before navigating away? (this is what our other discussion is about in a different thread, I believe)

I tend to only pull out the HOC when we the code reuse is expected to be very high for a solution. But if you thought we might re-use this then the HOC is certainly one way to do that.

Yeah exactly that was my thought process too, BUT maybe you would have waited till we have a few more use cases for this kind of code before making it a HOC - I'll happily defer to you on this, though I guess the "next steps" probably depend on what the "correct solution" is (again referring to our other discussion - showing a "you don't have accesss" page or navigating somewhere)

Contributor

marcaaron Nov 23, 2022

we aren't actually sure if we want to navigate at this point, right

Yes. I think there are different possible states of report access and different app states depending on if those reports were previously accessible or not.

Using the existence of the report in storage doesn't tell us whether you have "access" or not especially when you are offline...

Offline and never attempted to fetch a report: No way to really tell if the user has access or not but that's fine the same error message works in this case.
Offline and previous attempt returned 404: Tell them they can't access it i.e. "Hmm... it's not here"
Online and attempt returns 404: Tell them they can't access it i.e. "Hmm... it's not here"

I don't think there is any situation where we'd want to redirect without any explanation (but this deserves a wider discussion to get more input than just me 😅).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet