Skip to content
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

Fetch policy chats after deleting a member so that the room updates correctly #8437

Merged
merged 1 commit into from
Apr 11, 2022

Conversation

Gonals
Copy link
Contributor

@Gonals Gonals commented Apr 1, 2022

Details

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/204497

Tests/QA Steps

  1. Create a new workspace from an account that's on the policyExpenseChat beta (e.g @expensifail.com)
  2. Add a new member to the workspace via Manage Members > Invite
  3. Log-in as the member and send a couple of messages in the member's workspace chat
  4. Log back in again to the admin account and remove the member from the workspace via Manage Members > Remove
  5. Navigate to the removed member's workspace chat, the archived reason textbox should appear and the chat in the LHN should have the trashcan icon:

Screen Shot 2022-04-01 at 2 45 36 PM

Screen Shot 2022-04-01 at 2 45 42 PM

@Gonals Gonals requested a review from a team as a code owner April 1, 2022 12:43
@Gonals Gonals self-assigned this Apr 1, 2022
@melvin-bot melvin-bot bot requested review from Luke9389 and removed request for a team April 1, 2022 12:43
@Luke9389
Copy link
Contributor

Luke9389 commented Apr 1, 2022

Not merging bc this is on HOLD.

@Gonals Gonals changed the title [HOLD] Fetch policy chats after deleting a member so that the room updates correctly Fetch policy chats after deleting a member so that the room updates correctly Apr 11, 2022
@Gonals
Copy link
Contributor Author

Gonals commented Apr 11, 2022

Off HOLD!

@Luke9389 Luke9389 merged commit f9987f9 into main Apr 11, 2022
@Luke9389 Luke9389 deleted the alberto-archiveRoom branch April 11, 2022 21:32
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @Luke9389 in version: 1.1.55-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @Luke9389 in version: 1.1.55-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@yuwenmemon
Copy link
Contributor

@Gonals @TomatoToaster Does this work when we try to fetch workspaceChats that don't belong to the policy member? Since we send all workspace chats here... when we end up sending that list of IDs via GetReportSummaryList, we're going to get a 404 returned to us.

@TomatoToaster
Copy link
Contributor

It should be fine because the only ones calling modifyEmployees should be admins. They should have access to all workspace chats that exist on the policy.

@yuwenmemon
Copy link
Contributor

Right but what about the employee that's getting removed? The state of their EChat client won't update to reflect that the workspace room should now be archived.

@TomatoToaster
Copy link
Contributor

Yes, 100% agree their clients need to be notified. The pusher event for when a policy member removed is the right solution.

I also think the same should happen for other admins when a member gets added to the policy. Even though right now, we don't support multiple admins on a policy. There was some talk about doing this here: https://github.com/Expensify/Expensify/issues/201621#issuecomment-1074225114 but because of the offline doc I didn't end up adding a pusher event for that. We could add that pusher event too, or we could wait till N08 to add it because right now there isn't really a way to test it (without manually making someone an admin on a free workspace).

@yuwenmemon
Copy link
Contributor

Okay, @TomatoToaster - should I make a follow-up issue?

@TomatoToaster
Copy link
Contributor

@yuwenmemon yeah let's make a follow up issue. I have a feeling this will be forgotten about otherwise 😅. We can test multiple admin workspaces on dev.

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @yuwenmemon in version: 1.1.55-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@Beamanator
Copy link
Contributor

Beamanator commented Apr 25, 2022

Just to confirm y'all - the room name also has "(archived)" now due to #8447

This should happen right? Confirming b/c this was reported as a bug here: #8643

@TomatoToaster
Copy link
Contributor

Good question @Beamanator, this is not a bug! That is intended behavior.

@Beamanator
Copy link
Contributor

Thanks for confirming @TomatoToaster 👍 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants