-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Disable renaming public rooms if the current user is not an admin or the creator of the linked workspace #15688
Conversation
@eVoloshchak @bondydaa One of you needs to 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] |
@jasperhuangg, where can I find Do I need my account to be added to the policy rooms beta on dev for this to work? |
@eVoloshchak I've updated the testing steps, you don't need that script anymore.
You can just sign in with the testing account that I've provided in the QA steps, that's already been added to all the necessary betas, and you can just follow the QA steps if you're using |
Gonna ask for more reviews since we want to get this deployed before Friday |
Reviewer Checklist
Screenshots/Videos |
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.
Looks good and tests well with the updated instructions, thanks
🎯 @eVoloshchak, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #15756. |
After you correct the function name here: https://github.com/Expensify/App/pull/15688/files#r1130755483, I'll approve. Sorry for the slowness, just got to test it today. |
Added a commit to this while Jasper is OOO, please give this a review (we want this out before ECX) |
Okay tested locally, good to go cc @MonilBhavsar or @aldo-expensify |
@bondydaa @aldo-expensify @thienlnam Thank you guys for keeping this moving while I've been out sick. I have literally been in bed all day 😢 fixed the conditions and consolidated the logic as stated in your review comments, good for another look! |
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.
Just one comment about the naming of the linked workspace (can be addressed in a seperate PR)
Otherwise this looks good
I know this is urgent so going to approve
* @param {Object|null} linkedWorkspace - the workspace the report is on, null if the user isn't a member of the workspace | ||
* @returns {Boolean} | ||
*/ | ||
shouldDisableRename(linkedWorkspace) { |
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.
Can we just change this to currentWorkspace? I am not sure what linked workspace implies
✋ 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/thienlnam in version: 1.2.83-0 🚀
|
🚀 Deployed to staging by https://github.com/thienlnam in version: 1.2.83-0 🚀
|
🚀 Deployed to production by https://github.com/roryabraham in version: 1.2.83-3 🚀
|
Details
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/266085
Tests
(if you're using the web proxy, just follow the QA steps instead)
localhost:8080/r/<reportID
) > Settings.Offline tests
QA Steps
asdfASDF00
)PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)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
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android