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.

Sign up for GitHub

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

Jump to bottom

[WPB-4547] Attach the reason for a member to leave a conversation to the leave event #3640

Merged
merged 8 commits into from
Oct 13, 2023
Merged

[WPB-4547] Attach the reason for a member to leave a conversation to the leave event #3640

merged 8 commits into from
Oct 13, 2023

Conversation

MangoIV
Copy link
Contributor

@MangoIV MangoIV commented Oct 10, 2023 •
edited
Loading

https://wearezeta.atlassian.net/browse/WPB-4547

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@MangoIV MangoIV changed the title Aattach the reason for a member to leave a conversation to the leave event [WPB-4547] Attach the reason for a member to leave a conversation to the leave event Oct 10, 2023
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Oct 10, 2023
@MangoIV MangoIV force-pushed the mangoiv/conversation-member-leave-reason branch from 0184727 to 3d33ba7 Compare October 11, 2023 09:06
@MangoIV MangoIV force-pushed the mangoiv/conversation-member-leave-reason branch from 4535d7e to ba9d75b Compare October 12, 2023 12:20
@MangoIV MangoIV force-pushed the mangoiv/conversation-member-leave-reason branch from 557e8b1 to 013d3a4 Compare October 12, 2023 14:54
@MangoIV MangoIV marked this pull request as ready for review October 12, 2023 15:47
@MangoIV MangoIV requested a review from mdimjasevic October 13, 2023 09:36
pcapriotti
pcapriotti approved these changes Oct 13, 2023
View reviewed changes
Copy link
Contributor

@pcapriotti pcapriotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. However, it seems the "user deleted" and "user removed from team" reasons are conflated into one. That's probably fine, but maybe they should both be called "user deleted" rather than removed from team.

Minor comments follow.

.envrc Outdated Show resolved Hide resolved
services/galley/src/Galley/API/Teams.hs Outdated Show resolved Hide resolved
services/galley/src/Galley/API/Update.hs Show resolved Hide resolved
@MangoIV
Copy link
Contributor Author

MangoIV commented Oct 13, 2023

Looks good to me. However, it seems the "user deleted" and "user removed from team" reasons are conflated into one. That's probably fine, but maybe they should both be called "user deleted" rather than removed from team.

Minor comments follow.

So I will change it to "user deleted"? Or should I add another reason? I tried to tightly cohere with the ticket.

@MangoIV
[fix] apply fixed asked for by Paolo
3e2ac0f 
- remove leftovers
- s/EdReasonRemovedFromTeam/EdReasonDeleted
- s/removed-from-team/user-deleted
@MangoIV
Copy link
Contributor Author

MangoIV commented Oct 13, 2023

For transparency: it was internally decide to rename to field to something more appropriate.

@MangoIV MangoIV merged commit 80c7f7f into develop Oct 13, 2023
2 checks passed
@MangoIV MangoIV deleted the mangoiv/conversation-member-leave-reason branch October 13, 2023 17:18
This was referenced Oct 19, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pcapriotti pcapriotti pcapriotti approved these changes

@mdimjasevic mdimjasevic Awaiting requested review from mdimjasevic

Assignees
No one assigned
Labels
ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MangoIV @pcapriotti @zebot