-
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 error props being incorrectly accessed in components #23394
Conversation
Hey @mollfpr @marcaaron, here's the behaviour of emoji picker after applying the fix. Follow the test steps in PR description to verify yourself. It is working as expected.Before fix - before_fix.movAfter fix - after_fix.mov
Here's what happens for the other two pages -
IOUCurrencySelection and MoneyRequestAmountPage ), when there is an error the modal is closed but we are also redirected to the concierge chat because of this PR that was merged two weeks ago and is now on production - |
// navigate to concierge when the room removed from another device (e.g. user leaving a room) | |
// the report will not really null when removed, it will have defaultProps properties and values | |
if ( | |
prevOnyxReportID && | |
prevOnyxReportID === routeReportID && | |
!onyxReportID && | |
// non-optimistic case | |
(_.isEqual(this.props.report, defaultProps.report) || | |
// optimistic case | |
(prevProps.report.statusNum === CONST.REPORT.STATUS.OPEN && this.props.report.statusNum === CONST.REPORT.STATUS.CLOSED)) | |
) { | |
Navigation.goBack(); | |
Report.navigateToConciergeChat(); | |
// isReportRemoved will prevent <FullPageNotFoundView> showing when navigating | |
this.setState({isReportRemoved: true}); | |
return; | |
} |
The behaviour of the app (prod/staging/local) right now -
Screen.Recording.2023-07-24.at.1.34.12.AM.mov
When I undo the changes in that PR and apply the fix in this PR -
Screen.Recording.2023-07-24.at.1.34.47.AM.mov
@marcaaron I just wanted to confirm the expected behaviour here before I proceed. If there is an error and the request money/split bill/assign task modal is opened, is it okay that we are redirected to concierge chat?
Or should we stay on the same report because as you can see in the video below the chat still exists even after we are redirected to concierge.
Screen.Recording.2023-07-24.at.1.57.00.AM.mov
I'm okay with that. |
Hey @marcaaron, what do you think of the behaviour mentioned above in case of |
That sounds like a bug to me caused by the PR you mentioned. I would expect to see some error on the page itself and not to get redirected. |
Apologies, I was sick these last 2 days so couldn't dedicate much time to this. I am better now so will update the PR tomorrow morning. I will try to find the solution for the issue caused by the referenced PR so that we stay on the same page after an error. |
Updated the branch to resolve merge conflicts. I also noticed that the regression from the other PR was identified and there is a new PR to fix it, I added a comment there since they are also trying to fix an issue with the same root cause - @marcaaron what should be done now? Should we put this PR on hold until that new PR is merged or something else? |
Mind directing that question to @mollfpr? I'll jump in here when this is looking ready to merge. Thanks! |
@mollfpr what do you think? I think we have two options -
|
@Nikhil-Vats We can do option two since they will add it back with the fix for the regression. But let's notify them about this and wait for their feedback, then apply the necessary change here. |
@Nikhil-Vats Great! Let's do it here. |
@@ -71,15 +60,6 @@ function IOUCurrencySelection(props) { | |||
const iouType = lodashGet(props.route, 'params.iouType', CONST.IOU.MONEY_REQUEST_TYPE.REQUEST); | |||
const reportID = lodashGet(props.route, 'params.reportID', ''); | |||
|
|||
const shouldDismissModal = ReportUtils.shouldHideComposer(props.report, props.errors); |
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.
@mollfpr I removed this from IOUCurrencySelection page because we don't need to add it here. We check this in MoneyRequestAmountPage
page and they both belong to the same stack of navigators so it works, I tested.
I even tried by pressing browser forward button but it did not let me go to that screen.
I did see your comment here and wondered why it was not working before. Anyway, please let me know if it doesn't work for you.
@mollfpr PR is ready for review. |
const hideComposer = ReportUtils.shouldHideComposer(props.report, errors); | ||
const shouldOmitBottomSpace = hideComposer || isArchivedRoom; | ||
// If composer is hidden, omit the bottom space. | ||
const shouldOmitBottomSpace = ReportUtils.shouldHideComposer(props.report); |
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.
Archived report is already checked in shouldHideComposer
so removed the duplicate condition.
@@ -158,36 +156,17 @@ class ReportScreen extends React.Component { | |||
|
|||
componentDidUpdate(prevProps) { | |||
// If composer should be hidden, hide emoji picker as well | |||
if (ReportUtils.shouldHideComposer(this.props.report, this.props.errors)) { |
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.
except this line and line 389 where errors are no longer passed to ReportFooter
, the rest code in this file is reverting the PR that was blocking us - https://github.com/Expensify/App/pull/22787/files
Reviewer Checklist
Screenshots/VideosWeb23394.Web.movMobile Web - ChromeScreen_Recording_20230809_213949_Chrome.mp4Mobile Web - Safari23394.mWeb.Safari.mp4Desktop23394.Desktop.moviOS23394.iOS.mp4AndroidScreen_Recording_20230809_215256_New.Expensify.mp4 |
@Nikhil-Vats The behavior on the IOU pages is different; sometimes, the RHN is closed and shows the reported error, and sometimes keep, the RHN and showing the not found page. Screen.Recording.2023-08-02.at.18.33.12.movScreen.Recording.2023-08-02.at.18.34.48.mov |
@mollfpr This was reported here in the regression we were discussing about. It is a backend bug where sometimes the report is cleared from onyx before the error is sent by backend due to which we are redirected to not found page. I believe they will handle it in the new PR as they already reported it. I will paste your video there to make sure they test this as I never faced this. |
@Nikhil-Vats I constantly got the not found page on the Desktop while testing the IOU flow. I'm afraid the QA team will bump into this issue, and the QA will fail. Screen.Recording.2023-08-02.at.21.23.37.mov |
What should be done here then @mollfpr? It is a backend issue and they are still discussing the approach over there so it will take some time for it to be fixed. |
Ideally, we should get the same expected result on all platforms. So if the attached PR needed to be deployed first, we should wait. |
Sure @mollfpr, should we put a comment on the main issue and put it on hold then? I commented on the other issue asking their confirmation on if they are going to fix this redirect to 404 page error in their issue. |
@Nikhil-Vats I agree with Marc's explanation here. We can skip the test case on the IOU flow for the platform constantly showing the issue. What do you think? |
@mollfpr Sure, I will open a separate issue for that. Let me know if it happens constantly on any other platform for you as well so I can put that in the test steps. |
@Nikhil-Vats Could you resolve the conflicts? I'll test all the platforms and let you know. Thanks! |
Done @mollfpr |
There are conflicts again, I'll resolve them today. |
Hey @mollfpr, conflicts resolved. You can retest. |
Sorry for the delay, I'll give you an update shortly regarding the test. |
@Nikhil-Vats The test on IOU flow only works for Web and mWeb. On Desktop, iOS, and Android, it redirects to the not found page after online. We can skip the test on IOU flow and just put the test case for the emoji picker to be closed after being back online. |
@mollfpr Okay, I will update the PR description to reflect that and create a separate issue for the IOU flow because for that report is also removed from LHN when user is redirected to 404 page. |
PR description updated. |
@mollfpr Please let me know if there's anything else left for me to do. |
@Nikhil-Vats Looks good to me. I need to finish the test for Android. |
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.
LGTM and tests well 👍
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/marcaaron in version: 1.3.53-0 🚀
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.3.53-2 🚀
|
Details
errors
props were being incorrectly accessed inIOUCurrencySelection
,ReportScreen
, andMoneyRequestAmountPage
. This fixes that by getting theerrors
from thereport
object using a new function to keep code DRY.Fixed Issues
$ #23373
PROPOSAL: #23373 (comment)
Tests
a@_.cd
or2f34d2be9bce77da59b2e91f1a2bd25e0b85f2142f34d2be9bce77da59b2e91f1a2bd25e0b85f2142f34d2be9bce77da59b2e91f1a2bd25e0b85f214@gmail.com
.Since we can't open multiple tabs in apps (android/ios/desktop), we can test the same by switching off wifi in our device in step 2 and then switching on wifi in step 5 again.
Offline tests
Same as above.
QA Steps
Same as above.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
23394_web1.mov
23394_web2.mov
Mobile Web - Chrome
23394_mchrome1.mov
23394_mchrome2.mov
Mobile Web - Safari
23394_safari1.mov
23394_safari2.mov
23394_safari3.mov
Desktop
23394_desktop1.mov
iOS
23394_ios1.mov
23394_ios2.mov
Android
23394_android1.mov
23394_android2.mov