-
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
Add possibility to leave from workspaces and workspace expense chats #35671
Add possibility to leave from workspaces and workspace expense chats #35671
Conversation
…ture/35286-leave-workspace
This reverts commit 3775ad0.
…ture/35286-leave-workspace
…ture/35286-leave-workspace
…ture/35286-leave-workspace
@rezkiy37 can we please resolve this conflicts 🙏 |
…ture/35286-leave-workspace
@MonilBhavsar, you can try to use this PR for the testing. |
…ture/35286-leave-workspace
…ture/35286-leave-workspace
…ture/35286-leave-workspace
…ture/35286-leave-workspace
This reverts commit 5ca68a6.
Update - #35286 (comment). |
…ture/35286-leave-workspace
There are some conflicts to resolve and @rayane-djouah looks like last bug is not reproducible. Can you please take a look? |
…ture/35286-leave-workspace
I'm not able to reproduce. |
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 🚀
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.
Thanks! Looks good overall
src/libs/ReportActionsUtils.ts
Outdated
|
||
// Currently, we only render messages when members are invited | ||
const verb = isInviteAction ? Localize.translateLocal('workspace.invite.invited') : Localize.translateLocal('workspace.invite.removed'); | ||
let verb = Localize.translateLocal('workspace.invite.removed'); | ||
|
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.
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.
Done - 7321ad2.
src/libs/ReportActionsUtils.ts
Outdated
); | ||
} | ||
|
||
function isInviteMemberAction(reportAction: OnyxEntry<ReportAction>) { | ||
return reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.ROOMCHANGELOG.INVITE_TO_ROOM || reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.POLICYCHANGELOG.INVITE_TO_ROOM; | ||
} | ||
|
||
function isLeaveMemberAction(reportAction: OnyxEntry<ReportAction>) { |
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.
function isLeaveMemberAction(reportAction: OnyxEntry<ReportAction>) { | |
function isLeavePolicyAction(reportAction: OnyxEntry<ReportAction>) { |
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.
Done - 7321ad2.
src/libs/actions/Policy.ts
Outdated
@@ -960,6 +974,101 @@ function removeMembers(accountIDs: number[], policyID: string) { | |||
API.write(WRITE_COMMANDS.DELETE_MEMBERS_FROM_WORKSPACE, params, {optimisticData, successData, failureData}); | |||
} | |||
|
|||
/** Leave a workspace */ |
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.
We can remove this. name implies the reason
/** Leave a workspace */ |
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.
Done - 7321ad2.
…ture/35286-leave-workspace
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.
Thank you! Let's ship it
✋ 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/MonilBhavsar in version: 1.4.63-0 🚀
|
@rayane-djouah @rezkiy37 there were just this two backend bugs to fix #35671 (comment), right? |
@MonilBhavsar, Yes! |
Thank you for confirming! |
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.63-21 🚀
|
@getusha @rezkiy37 this is the exact issue we discussed here, right? |
and as per the security guidelines, a non workspace member can join the workspace chat https://github.com/Expensify/App?tab=readme-ov-file#security. So I think we should update both frontend and backend to allow anyone to rejoin the chat. What do you think? |
Can we create an issue for this or do you want to fix it in a scope of the current issue? |
Hey! |
} | ||
|
||
if (report?.errorFields?.notFound) { | ||
Report.clearReportNotFoundErrors(report.reportID); |
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.
This doesn't seem right to me. Why is this needed? @rezkiy37
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.
@arosiclair, please check the condition above. The app checks if a report has reportID
, if yes it means that the notFound
error is not relevant anymore.
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.
That's not necessarily true. If you lose access to a report,report.errorFields.notFound
will be set along with the rest of the report data including the reportID
.
So why is this needed? errorFields.notFound
should already be cleared in the successData for OpenReport.
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.
There was a bug - #35671 (comment). I fixed it - #35671 (comment), #35671 (comment).
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.
Okay it sounds like the notFound
error is not cleared when we push reports to users. I think we can just fix that on the backend. Do you know which API command you were testing that caused that bug? cc @MonilBhavsar
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 like InviteToRoom
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.
Alright looks like we just need to update Report::structureReportForOnyx to clear errorFields.notFound
. I opened an issue to clean this up here.
@@ -165,6 +170,14 @@ function WorkspacesListPage({policies, allPolicyMembers, reimbursementAccount, r | |||
}); | |||
} | |||
|
|||
if (!(isAdmin || isOwner)) { | |||
threeDotsMenuItems.push({ | |||
icon: Expensicons.ChatBubbles, |
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.
Hi 👋 coming from #46248.
Pretty minor issue and part of the polish issue. We should be using the Exit
icon.
Details
The PR introduces a couple flows for leaving a workspace and a workspace chat. It is based on rules that described here for these scenarios.
Fixed Issues
$ #35286
PROPOSAL: N/A
Tests
Leaving a workspace
1.1. So invite the user from another account to a workspace. You can do it via the workspace settings members page.
Leaving a workspace chat
Offline tests
Same as "Tests".
QA Steps
Same as "Tests".
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)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.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
Android: Native
Android.Leave.Workspace.mp4
Android: mWeb Chrome
Android.Chrome.Leave.Workspace.mp4
iOS: Native
IOS.Leave.Workspace.mp4
iOS: mWeb Safari
IOS.Safari.Leave.Workspace.mp4
MacOS: Chrome / Safari
Chrome.Leave.Workspace.mp4
Leave.mp4
MacOS: Desktop
Desktop.Leave.Workspace.mp4