Prevent It's not here message when logging in #55721#56228
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
|
@rushatgabhane Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
| } | ||
|
|
||
| // Check if the report exists in the collection | ||
| const report = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]; |
There was a problem hiding this comment.
maybe a noobie questions:
@OmarKoueifi will onyx always have the report at this point in code? Can there be a case where the report is being fetched, so it isn't in onyx. But the report actually exists.
There was a problem hiding this comment.
@rushatgabhane
To my understanding and based on my testing, the report should always be in Onyx at this point. However, I’m still new to this project and haven’t explored all edge cases. If there's a scenario where it might not be available yet, I’d love to understand how to reproduce that.
I initially tried another approach using onyx.connect with the key ${ONYXKEYS.COLLECTION.REPORT}${thread.reportID} and checking inside the callback. That seemed to work, but I ultimately went with the current solution since it's being used in many place in code allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`];.
I also wanted to add a unit test for this, but I couldn’t find any existing test cases for openReportFromDeepLink. I’m still getting familiar with how tests are structured in this repo, if there’s a recommended place to add it or an example of a similar function with tests, I’d appreciate any guidance!
Reviewer Checklist
Screenshots/VideosAndroid: mWeb ChromeiOS: NativeScreen.Recording.2025-02-05.at.07.28.25.mov |
rushatgabhane
left a comment
There was a problem hiding this comment.
LGTM!
@OmarKoueifi to test it on iOS and Android, you can run these commands -
npx uri-scheme open 'new-expensify://r/1234' --ios
npx uri-scheme open 'new-expensify://r/1234' --android
Beamanator
left a comment
There was a problem hiding this comment.
Looking great to me! Very interesting, the question about "will the reportID exist - cuz it looks like we do openReport a few lines above (line 2888)... Does that actually load the report into Onyx BEFORE our new code runs? (probably yes b/c our new code runs after waitForUserSignIn)
I have decent confidence this should work perfectly 🚀 Let's shipittt
Hmmmmmmm @rushatgabhane is that something we need to fix in this PR? 😅 @OmarKoueifi mind pulling |
|
Done! |
|
Thanks! Rerunning github actions |
|
|
|
@Beamanator looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
Not emergency, as mentioned above, the 1 failing check was ESlint, and it failed on a file that wasn't modified in this PR |
|
✋ 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/Beamanator in version: 9.0.95-0 🚀
|
|
@OmarKoueifi Found issue on apps #56521 |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.0.95-6 🚀
|
|
@OmarKoueifi @rushatgabhane This PR causes an issue where the user cannot deep link to a route like /settings/profile:
I'm unsure if this is new behavior or a bug caused by this PR. Please correct me if I’m mistaken. Thanks! |
|
@truph01 I'll take a look and test later today |
|
Hi! I'm coming from #65297 Did you guys tested public rooms in this PR? If yes what is the valid flow of handling a public room that is not in a collection? Currently I'm struggling with finding a code that handles that for authenticated non-anonymous users |







Explanation of Change
Fixed Issues
$ #55721
PROPOSAL: #55721 (comment)
Tests
A. Test Access When Logged Out and Switching Accounts
B. Test Access When Already Signed In (Unauthorized Report)
C. Test Access When Already Signed In (Authorized Report)
1. Log in with an account that has access to a known report.
2. Navigate to the report using its link.
Expected Result: The report opens normally without any redirection or error messages.
Verify that no errors appear in the JS console
Offline tests
N/A
Can't sign in when offline
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: mWeb Chrome
no.access.android.mov
iOS: mWeb Safari
no.access.ios.mov
MacOS: Chrome / Safari
** Chrome **
report.with.no.access.mov
** Safari **
report.with.no.access.mov