Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Include users membership in group in summary API #2382

Merged
merged 5 commits into from Jul 24, 2017

Conversation

Projects
None yet
3 participants
Owner

erikjohnston commented Jul 24, 2017

No description provided.

erikjohnston added some commits Jul 24, 2017

synapse/storage/group_server.py
+ },
+ retcols=("is_admin", "is_public"),
+ allow_none=True,
+ desc="is_user_adim_in_group",
@dbkr

dbkr Jul 24, 2017 edited

Member

I think you meant 'admin'

synapse/storage/group_server.py
+
+ if row:
+ return {
+ "memebrship": "joined",
@NegativeMjark

NegativeMjark Jul 24, 2017

Contributor

"meme br ship"?

synapse/storage/group_server.py
+
+ if row:
+ return {
+ "membership": "joined",
@NegativeMjark

NegativeMjark Jul 24, 2017

Contributor

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

synapse/storage/group_server.py
+ return {
+ "membership": "joined",
+ "is_public": row["is_public"],
+ "is_privileged": row["is_admin"],
@NegativeMjark

NegativeMjark Jul 24, 2017

Contributor

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?

@erikjohnston

erikjohnston Jul 24, 2017

Owner

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"

Contributor

NegativeMjark commented Jul 24, 2017

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

synapse/storage/group_server.py
+ Example if joined:
+
+ {
+ "membership": "joined",
@NegativeMjark

NegativeMjark Jul 24, 2017

Contributor

Maybe update the example as well?

LGTM otherwise

@erikjohnston erikjohnston merged commit ebbaae5 into erikj/groups_merged Jul 24, 2017

7 of 8 checks passed

Sytest Dendron (Commit) Build #2502 origin/erikj/group_privilege failed in 10 min
Details
Sytest Dendron (Merged PR) Build finished.
Details
Sytest Postgres (Commit) Build #3336 origin/erikj/group_privilege succeeded in 8 min 48 sec
Details
Sytest Postgres (Merged PR) Build finished.
Details
Sytest SQLite (Commit) Build #3431 origin/erikj/group_privilege succeeded in 1 min 38 sec
Details
Sytest SQLite (Merged PR) Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@erikjohnston erikjohnston deleted the erikj/group_privilege branch Oct 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment