-
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
Open report for anonymous users #18293
Conversation
…r-component feat: add AnonymouseReportFooter component
I'll review this too since I jumped in to review the Web-E PR. First, let's update the test steps to create a public room first because the Expensiconx room won't exist in the local environment obviously. Let's also test opening a non-public room while signed out, for completeness. |
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.
The online test works. I don't understand how the offline test would work since you can't navigate to a new URL when offline. I'm able to see the report as the anonymous user, but I can't comment, and when I clicked on my avatar I was signed out. When are we planning to fix those pieces?
Screen.Recording.2023-05-26.at.1.53.24.PM.mov
The PR is ready for a final review |
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.
I agree there is a few things to properly research and make sure they dont have any bad influence on performance, or in general the patter of chekcing if an action can be taken when user is anonymous (add reactions) is quite ugly, however there is probably not many ways to make it nicer.
Great job on this one @marcochavezf and thanks everyone for reviews and testing, I will now to staging
@@ -81,6 +89,7 @@ const defaultProps = { | |||
updateAvailable: false, | |||
isSidebarLoaded: false, | |||
screenShareRequest: null, | |||
isCheckingPublicRoom: true, |
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.
@marcochavezf It would be good to follow up on this and confirm there is no detriment to app performance and how long the splashcreen shows with this change
…ounts Open report for anonymous users (cherry picked from commit c27575b)
…-18293 🍒 Cherry pick PR #18293 to staging 🍒
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 1.3.20-2 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
@marcochavezf thanks, now I am able to test locally against staging and find bugs. |
The root cause and solution explained in slack |
This PR also seems to have introduced this regression. Confirmed locally |
🚀 Deployed to production by https://github.com/Julesssss in version: 1.3.20-5 🚀
|
🚀 Cherry-picked to staging by https://github.com/AndrewGable in version: 1.3.28-2 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.3.28-5 🚀
|
This PR causes this regression(s). We allowed to access public rooms here but didn't allow any actions that can be performed like, copy message, url, etc. |
The anonymous users' functionality requires these Web-E changes to be live on staging/prod. But to speed things up a bit, this PR will work even without those changes. So we can merge and deploy this PR in parallel with the Web-E changes.The Web-E changes are live on the staging server.Details
This PR checks if the user is trying to open a chat report when they click on a link. If the URL to open contains a reportID, the app is online and the user is unauthenticated, we check if the chat report is public. If does, we create an anonymous account for the user and let them open the public room.
Fixed Issues
$ #18318
Tests
Sign in
or any action that should not be allowed to an unauthenticated userUpdate: We're not covering Desktop atm in this PR. We'll cover it in the upcoming changes.
Offline tests
Update: I noticed the offline scenario is not working, but I will address it in a follow-up PR to avoid blocking this PR for RenderATL, so I will cross it out atm.
1. As unauthenticated user, open a public room (like the ExpensiConX #announce room)2. Verify it's redirected to the login page
3. Enable the internet connection
5. Verify it opens the public room automatically
QA Steps
Same as test steps
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
Screen.Recording.2023-05-26.at.17.08.45.mov
Mobile Web - Chrome
Screen.Recording.2023-05-26.at.17.17.40.mov
Mobile Web - Safari
Screen.Recording.2023-05-26.at.17.20.40.mov
Desktop
N/A
iOS
Screen.Recording.2023-05-26.at.19.22.42.mov
Android
Screen.Recording.2023-05-26.at.19.19.55.mov