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

Remove group members #3011

Merged
merged 3 commits into from
Dec 10, 2022
Merged

Remove group members #3011

merged 3 commits into from
Dec 10, 2022

Conversation

OmarBasem
Copy link
Contributor

@OmarBasem OmarBasem commented Dec 9, 2022

Fixes #3009

This PR adds a method to remove a list of members from a group at once.
Needed by: status-im/status-mobile#14494

@ghost
Copy link

ghost commented Dec 9, 2022

Pull Request Checklist

  • Have you updated the documentation, if impacted (e.g. docs.status.im)?
  • Have you tested changes with mobile?
  • Have you tested changes with desktop?

@status-im-auto
Copy link
Member

status-im-auto commented Dec 9, 2022

Jenkins Builds

Click to see older builds (3)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 3745ec0 #1 2022-12-09 05:05:05 ~1 min linux 📦zip
✔️ 3745ec0 #1 2022-12-09 05:08:36 ~4 min android 📦aar
✔️ 3745ec0 #1 2022-12-09 05:08:49 ~5 min ios 📦zip
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 53ccf53 #2 2022-12-09 13:52:26 ~2 min linux 📦zip
✔️ 53ccf53 #2 2022-12-09 13:54:54 ~4 min android 📦aar
✔️ 53ccf53 #2 2022-12-09 14:02:53 ~12 min ios 📦zip
✔️ 3a8aa4c #3 2022-12-10 04:13:12 ~1 min linux 📦zip
✔️ 3a8aa4c #3 2022-12-10 04:16:06 ~4 min android 📦aar
✔️ 3a8aa4c #3 2022-12-10 04:17:32 ~5 min ios 📦zip

@OmarBasem OmarBasem marked this pull request as ready for review December 9, 2022 06:18
Copy link
Member

@0x-r4bbit 0x-r4bbit left a comment

Choose a reason for hiding this comment

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

This LGTM however there are some details/APIs in this PR that I'm not familiar with, so I'll defer to the other reviewers to double check the correctness here.

Also, bonus points if there was a test for this new API

Copy link
Contributor

@cammellos cammellos left a comment

Choose a reason for hiding this comment

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

Great job @OmarBasem! looks good, only thing, could you please add some tests for the new functionality?

Copy link
Contributor

@osmaczko osmaczko left a comment

Choose a reason for hiding this comment

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

The logic of the new endpoint is almost the same as for RemoveMemberFromGroupChat. Could you please remove the duplication by making RemoveMemberFromGroupChat use RemoveMembersFromGroupChat ?

func (m *Messenger) RemoveMemberFromGroupChat(ctx context.Context, chatID string, member string) (*MessengerResponse, error) {
    return m.RemoveMembersFromGroupChat(ctx, chatID, []string{member})
}

@OmarBasem
Copy link
Contributor Author

OmarBasem commented Dec 9, 2022

The logic of the new endpoint is almost the same as for RemoveMemberFromGroupChat. Could you please remove the duplication by making RemoveMemberFromGroupChat use RemoveMembersFromGroupChat ?

func (m *Messenger) RemoveMemberFromGroupChat(ctx context.Context, chatID string, member string) (*MessengerResponse, error) {
    return m.RemoveMembersFromGroupChat(ctx, chatID, []string{member})
}

Sure! Just to confirm, do you mean RemoveMemberFromGroupChat to use RemoveMembersFromGroupChat or the opposite?

@OmarBasem
Copy link
Contributor Author

OmarBasem commented Dec 9, 2022

Great job @OmarBasem! looks good, only thing, could you please add some tests for the new functionality?

Sure, I will!

@OmarBasem OmarBasem self-assigned this Dec 9, 2022
Copy link
Contributor

@osmaczko osmaczko left a comment

Choose a reason for hiding this comment

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

LGTM :)

Copy link
Contributor

@cammellos cammellos left a comment

Choose a reason for hiding this comment

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

It'd be good to have a test that removes multiple members, but it's ok I guess, also don't forget to bump the version (this is breaking change, so you can bump the y)
Great work!

@OmarBasem OmarBasem merged commit d1a4b53 into develop Dec 10, 2022
@OmarBasem OmarBasem deleted the remove-group-members branch December 10, 2022 04:26
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.

An endpoint for removing a list of users from a group
5 participants