Skip to content

fix: members: use new *All objects for *AllManager managers #1827

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
Merged
merged 1 commit into from
Jan 13, 2022
Merged

fix: members: use new *All objects for *AllManager managers #1827

merged 1 commit into from
Jan 13, 2022

Conversation

JohnVillalovos
Copy link
Member

@JohnVillalovos JohnVillalovos commented Jan 12, 2022 •
edited
Loading

Change it so that:

GroupMemberAllManager uses GroupMemberAll object
ProjectMemberAllManager uses ProjectMemberAll object

Create GroupMemberAll and ProjectMemberAll objects that do not support
any Mixin type methods. Previously we were using GroupMember and
ProjectMember which support the save() and delete() methods but
those methods will not work with objects retrieved using the
/members/all/ API calls.

list() API calls:
GET /groups/:id/members/all
GET /projects/:id/members/all

get() API calls:
GET /groups/:id/members/all/:user_id
GET /projects/:id/members/all/:user_id

Closes: #1825
Closes: #848

[1] https://docs.gitlab.com/ee/api/members.html#list-all-members-of-a-group-or-project-including-inherited-and-invited-members
[2] https://docs.gitlab.com/ee/api/members.html#get-a-member-of-a-group-or-project-including-inherited-and-invited-members

@JohnVillalovos JohnVillalovos force-pushed the jlvillal/all_objects branch 10 times, most recently from 24b28b6 to 88ac96d Compare January 12, 2022 02:38
@codecov-commenter
Copy link

codecov-commenter commented Jan 12, 2022 •
edited
Loading

Codecov Report

Merging #1827 (755e0a3) into main (4a000b6) will increase coverage by 0.00%.
The diff coverage is 75.00%.

@@           Coverage Diff           @@
##             main    #1827   +/-   ##
=======================================
  Coverage   92.19%   92.20%           
=======================================
  Files          77       77           
  Lines        4819     4823    +4     
=======================================
+ Hits         4443     4447    +4     
  Misses        376      376
Flag Coverage Δ
cli_func_v4 81.36% <75.00%> (+0.01%) ⬆️
py_func_v4 80.17% <75.00%> (+0.01%) ⬆️
unit 83.18% <75.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
gitlab/v4/objects/members.py 94.73% <75.00%> (+0.39%) ⬆️
gitlab/v4/objects/services.py 100.00% <0.00%> (ø)

nejch
nejch reviewed Jan 13, 2022
View reviewed changes
Copy link
Member

@nejch nejch left a comment

Choose a reason for hiding this comment

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

Thanks @JohnVillalovos! At the beginning I was happy about the initial implementation because it wasn't duplicating things, but looks like it was kind of broken that way.

A good side-effect of this is that it will also work on the CLI now I'm pretty sure.

@JohnVillalovos
Copy link
Member Author

JohnVillalovos commented Jan 13, 2022 •
edited
Loading

Yeah, because of that members/all we have no idea where they are a member from. It could be anywhere in the path above them. So we can't delete them in the members/all as have to go up the path to figure out where they are actually getting their access from.

@nejch
Copy link
Member

nejch commented Jan 13, 2022 •
edited
Loading

@JohnVillalovos I'm pretty sure this also closes #848 by adding a new CLI command:

(.venv) nejc@zbook-fury:~/repos/python-gitlab$ gitlab project-member-all list --help
usage: gitlab project-member-all list [-h] [--sudo SUDO] --project-id PROJECT_ID [--page PAGE] [--per-page PER_PAGE] [--all]

optional arguments:
  -h, --help            show this help message and exit
  --sudo SUDO
  --project-id PROJECT_ID
  --page PAGE
  --per-page PER_PAGE
  --all

would you mind adding a quick test for that?

Edit: and just one more nit, fix(members): 🙇

@JohnVillalovos
fix(members): use new *All objects for *AllManager managers
755e0a3 
Change it so that:

  GroupMemberAllManager uses GroupMemberAll object
  ProjectMemberAllManager uses ProjectMemberAll object

Create GroupMemberAll and ProjectMemberAll objects that do not support
any Mixin type methods. Previously we were using GroupMember and
ProjectMember which support the `save()` and `delete()` methods but
those methods will not work with objects retrieved using the
`/members/all/` API calls.

`list()` API calls: [1]
  GET /groups/:id/members/all
  GET /projects/:id/members/all

`get()` API calls: [2]
  GET /groups/:id/members/all/:user_id
  GET /projects/:id/members/all/:user_id

Closes: #1825
Closes: #848

[1] https://docs.gitlab.com/ee/api/members.html#list-all-members-of-a-group-or-project-including-inherited-and-invited-members
[2] https://docs.gitlab.com/ee/api/members.html#get-a-member-of-a-group-or-project-including-inherited-and-invited-members
@JohnVillalovos
Copy link
Member Author

@JohnVillalovos I'm pretty sure this also closes #848 by adding a new CLI command:

(.venv) nejc@zbook-fury:~/repos/python-gitlab$ gitlab project-member-all list --help
usage: gitlab project-member-all list [-h] [--sudo SUDO] --project-id PROJECT_ID [--page PAGE] [--per-page PER_PAGE] [--all]

optional arguments:
  -h, --help            show this help message and exit
  --sudo SUDO
  --project-id PROJECT_ID
  --page PAGE
  --per-page PER_PAGE
  --all

would you mind adding a quick test for that?

Edit: and just one more nit, fix(members): 🙇

Done. And found bug with this test. Thanks!

@JohnVillalovos JohnVillalovos requested a review from nejch January 13, 2022 18:28
nejch
nejch approved these changes Jan 13, 2022
View reviewed changes
@nejch nejch merged commit 58e5b25 into main Jan 13, 2022
@nejch nejch deleted the jlvillal/all_objects branch January 13, 2022 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nejch nejch nejch approved these changes

@max-wittig max-wittig Awaiting requested review from max-wittig

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@JohnVillalovos @codecov-commenter @nejch