Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Include users membership in group in summary API #2382

Merged
merged 5 commits into from Jul 24, 2017

Conversation

erikjohnston
Copy link
Member

No description provided.

},
retcols=("is_admin", "is_public"),
allow_none=True,
desc="is_user_adim_in_group",
Copy link
Member

@dbkr dbkr Jul 24, 2017

Choose a reason for hiding this comment

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

I think you meant 'admin'


if row:
return {
"memebrship": "joined",
Copy link
Contributor

Choose a reason for hiding this comment

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

"meme br ship"?


if row:
return {
"membership": "joined",
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I thought we generally used "join" rather than "joined", "invite" rather than "invited" and "leave" rather than "left" etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

probs

return {
"membership": "joined",
"is_public": row["is_public"],
"is_privileged": row["is_admin"],
Copy link
Contributor

Choose a reason for hiding this comment

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

I've somewhat lost track of whether we call this "admin" or "privileged" in the rest of the API. Is it intentional that we are calling this "admin" internally and "privileged" externally?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, basically its a bit handy wavey the exact semantics for the admin/visibility stuff. is_privileged is a basically something that goes down the read APIs to tell the clients that "this person has some power, so just let them try and do stuff and see if they get a 403"

@NegativeMjark
Copy link
Contributor

Codewise LGTM. I've left some quibbles about the API for you though.

Example if joined:

{
"membership": "joined",
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe update the example as well?

Copy link
Contributor

@NegativeMjark NegativeMjark left a comment

Choose a reason for hiding this comment

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

LGTM otherwise

@erikjohnston erikjohnston merged commit ebbaae5 into erikj/groups_merged Jul 24, 2017
@erikjohnston erikjohnston deleted the erikj/group_privilege branch October 26, 2017 11:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants