Add GET membership API #9197
Merged
Add GET membership API #9197
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
seanh
force-pushed
the
add-get-membership-api
branch
from
bb360fd to
0617567
Compare
seanh
force-pushed
the
add-get-membership-api
branch
from
0617567 to
792c35e
Compare
seanh
commented
Dec 9, 2024
Comment on lines
+74
to
+84
| @api_config( | ||
| versions=["v1", "v2"], | ||
| route_name="api.group_member", | ||
| request_method="GET", | ||
| link_name="group.member.read", | ||
| description="Fetch a group membership", | ||
| permission=Permission.Group.READ, | ||
| ) | ||
| def get_member(context: GroupMembershipContext, request): | ||
| return GroupMembershipJSONPresenter(request, context.membership).asdict() |
There was a problem hiding this comment.
🤷♂️
If some of the old nonsense in h was factored out so that Pyramid could be used properly this could be even simpler:
@view_defaults(...)
class MembershipAPIViews:
...
@view_config(request_method="GET", permission=Permission.Group.READ, renderer=MembershipJSONRenderer)
def get(context, request):
return context.membership
...
seanh
added a commit
that referenced
this pull request
Dec 11, 2024
Problem ------- `group_membership_api_factory()` is the context factory for all views that use the `"api.group_member"` route: * `remove_member()` (`request_method="DELETE"`) * `add_member()` (`request_method="POST"`) * `edit_member()` (`request_method="PATCH"`) * Add in #9197 also `get_member()` (`request_method="GET"`) `group_membership_api_factory()` returns a `GroupMembershipContext` object but: * `GroupMembershipContext` isn't the right context for `edit_member()`: it lacks the `context.new_roles` attribute for the new roles that the request wants to change `context.membership.roles` to. * `GroupMembershipContext` isn't the right context for `add_member()`: it has an inappropriate `context.membership` attribute (when adding a new membership there shouldn't be an existing membership in the context) and it lacks the `context.new_roles` attribute for the roles that the request wants to create a membership with. The context for the `edit_member()` view should be an `EditGroupMembershipContext` object, and for `add_member()` it should be an `AddGroupMembershipContext`. As a result the `edit_member()` view has to create its own context object to pass to `request.has_permission()`: def edit_member(context: GroupMembershipContext, request): appstruct = EditGroupMembershipAPISchema().validate(json_payload(request)) new_roles = appstruct["roles"] if not request.has_permission( Permission.Group.MEMBER_EDIT, EditGroupMembershipContext( context.group, context.user, context.membership, new_roles ), ): raise HTTPNotFound() When a future PR enables users (not just auth clients) to call the add-membership API the `add_member()` view will have to do something similar: constructing its own `AddGroupMembershipContext` object and passing it to `request.has_permission()`. This means there are two different context objects in play for the `edit_member()` and `add_member()` views: the `context` that is passed to the view is a `GroupMembershipContext`, but the `context` that is passed to `request.has_permission()` is an `EditGroupMembershipContext` or `AddGroupMembershipContext` constructed by the view itself. Solution ------- This commit changes `group_membership_api_factory()` to return a `GroupMembershipContext` for `GET` and `DELETE` requests but return an `EditGroupMembershipContext` for `PATCH` requests and an `AddGroupMembershipContext` for `POST`s. It's not possible for `group_membership_api_factory()` to set the context's `new_roles` attribute: the value for `new_roles` isn't available until later in the request processing cycle after the view has parsed and validated the request's JSON body. So the factory returns `context` objects with `context.new_roles=None` and the `edit_member()` view has to set `new_roles` before calling `has_permission()`: appstruct = EditGroupMembershipAPISchema().validate(json_payload(request)) context.new_roles = appstruct["roles"] if not request.has_permission(Permission.Group.MEMBER_EDIT, context): raise HTTPNotFound() In future the `add_member()` view will have to do the same. So this is still a little weird, but I think it's better than having two different context objects for a single request.
seanh
added a commit
that referenced
this pull request
Dec 11, 2024
Problem ------- `group_membership_api_factory()` is the context factory for all views that use the `"api.group_member"` route: * `remove_member()` (`request_method="DELETE"`) * `add_member()` (`request_method="POST"`) * `edit_member()` (`request_method="PATCH"`) * Add in #9197 also `get_member()` (`request_method="GET"`) `group_membership_api_factory()` returns a `GroupMembershipContext` object but: * `GroupMembershipContext` isn't the right context for `edit_member()`: it lacks the `context.new_roles` attribute for the new roles that the request wants to change `context.membership.roles` to. * `GroupMembershipContext` isn't the right context for `add_member()`: it has an inappropriate `context.membership` attribute (when adding a new membership there shouldn't be an existing membership in the context) and it lacks the `context.new_roles` attribute for the roles that the request wants to create a membership with. The context for the `edit_member()` view should be an `EditGroupMembershipContext` object, and for `add_member()` it should be an `AddGroupMembershipContext`. As a result the `edit_member()` view has to create its own context object to pass to `request.has_permission()`: def edit_member(context: GroupMembershipContext, request): appstruct = EditGroupMembershipAPISchema().validate(json_payload(request)) new_roles = appstruct["roles"] if not request.has_permission( Permission.Group.MEMBER_EDIT, EditGroupMembershipContext( context.group, context.user, context.membership, new_roles ), ): raise HTTPNotFound() When a future PR enables users (not just auth clients) to call the add-membership API the `add_member()` view will have to do something similar: constructing its own `AddGroupMembershipContext` object and passing it to `request.has_permission()`. This means there are two different context objects in play for the `edit_member()` and `add_member()` views: the `context` that is passed to the view is a `GroupMembershipContext`, but the `context` that is passed to `request.has_permission()` is an `EditGroupMembershipContext` or `AddGroupMembershipContext` constructed by the view itself. Solution ------- This commit changes `group_membership_api_factory()` to return a `GroupMembershipContext` for `GET` and `DELETE` requests but return an `EditGroupMembershipContext` for `PATCH` requests and an `AddGroupMembershipContext` for `POST`s. It's not possible for `group_membership_api_factory()` to set the context's `new_roles` attribute: the value for `new_roles` isn't available until later in the request processing cycle after the view has parsed and validated the request's JSON body. So the factory returns `context` objects with `context.new_roles=None` and the `edit_member()` view has to set `new_roles` before calling `has_permission()`: appstruct = EditGroupMembershipAPISchema().validate(json_payload(request)) context.new_roles = appstruct["roles"] if not request.has_permission(Permission.Group.MEMBER_EDIT, context): raise HTTPNotFound() In future the `add_member()` view will have to do the same. So this is still a little weird, but I think it's better than having two different context objects for a single request.
seanh
added a commit
that referenced
this pull request
Dec 11, 2024
Problem ------- `group_membership_api_factory()` is the context factory for all views that use the `"api.group_member"` route: * `remove_member()` (`request_method="DELETE"`) * `add_member()` (`request_method="POST"`) * `edit_member()` (`request_method="PATCH"`) * Add in #9197 also `get_member()` (`request_method="GET"`) `group_membership_api_factory()` returns a `GroupMembershipContext` object but: * `GroupMembershipContext` isn't the right context for `edit_member()`: it lacks the `context.new_roles` attribute for the new roles that the request wants to change `context.membership.roles` to. * `GroupMembershipContext` isn't the right context for `add_member()`: it has an inappropriate `context.membership` attribute (when adding a new membership there shouldn't be an existing membership in the context) and it lacks the `context.new_roles` attribute for the roles that the request wants to create a membership with. The context for the `edit_member()` view should be an `EditGroupMembershipContext` object, and for `add_member()` it should be an `AddGroupMembershipContext`. As a result the `edit_member()` view has to create its own context object to pass to `request.has_permission()`: def edit_member(context: GroupMembershipContext, request): appstruct = EditGroupMembershipAPISchema().validate(json_payload(request)) new_roles = appstruct["roles"] if not request.has_permission( Permission.Group.MEMBER_EDIT, EditGroupMembershipContext( context.group, context.user, context.membership, new_roles ), ): raise HTTPNotFound() When a future PR enables users (not just auth clients) to call the add-membership API the `add_member()` view will have to do something similar: constructing its own `AddGroupMembershipContext` object and passing it to `request.has_permission()`. This means there are two different context objects in play for the `edit_member()` and `add_member()` views: the `context` that is passed to the view is a `GroupMembershipContext`, but the `context` that is passed to `request.has_permission()` is an `EditGroupMembershipContext` or `AddGroupMembershipContext` constructed by the view itself. Solution ------- This commit changes `group_membership_api_factory()` to return a `GroupMembershipContext` for `GET` and `DELETE` requests but return an `EditGroupMembershipContext` for `PATCH` requests and an `AddGroupMembershipContext` for `POST`s. It's not possible for `group_membership_api_factory()` to set the context's `new_roles` attribute: the value for `new_roles` isn't available until later in the request processing cycle after the view has parsed and validated the request's JSON body. So the factory returns `context` objects with `context.new_roles=None` and the `edit_member()` view has to set `new_roles` before calling `has_permission()`: appstruct = EditGroupMembershipAPISchema().validate(json_payload(request)) context.new_roles = appstruct["roles"] if not request.has_permission(Permission.Group.MEMBER_EDIT, context): raise HTTPNotFound() In future the `add_member()` view will have to do the same. So this is still a little weird, but I think it's better than having two different context objects for a single request.
seanh
force-pushed
the
add-membership-response-body
branch
from
2ebfb06 to
c649dc9
Compare
seanh
force-pushed
the
add-get-membership-api
branch
from
792c35e to
6cbcdcb
Compare
seanh
added a commit
that referenced
this pull request
Dec 12, 2024
Problem ------- `group_membership_api_factory()` is the context factory for all views that use the `"api.group_member"` route: * `remove_member()` (`request_method="DELETE"`) * `add_member()` (`request_method="POST"`) * `edit_member()` (`request_method="PATCH"`) * Add in #9197 also `get_member()` (`request_method="GET"`) `group_membership_api_factory()` returns a `GroupMembershipContext` object but: * `GroupMembershipContext` isn't the right context for `edit_member()`: it lacks the `context.new_roles` attribute for the new roles that the request wants to change `context.membership.roles` to. * `GroupMembershipContext` isn't the right context for `add_member()`: it has an inappropriate `context.membership` attribute (when adding a new membership there shouldn't be an existing membership in the context) and it lacks the `context.new_roles` attribute for the roles that the request wants to create a membership with. The context for the `edit_member()` view should be an `EditGroupMembershipContext` object, and for `add_member()` it should be an `AddGroupMembershipContext`. As a result the `edit_member()` view has to create its own context object to pass to `request.has_permission()`: def edit_member(context: GroupMembershipContext, request): appstruct = EditGroupMembershipAPISchema().validate(json_payload(request)) new_roles = appstruct["roles"] if not request.has_permission( Permission.Group.MEMBER_EDIT, EditGroupMembershipContext( context.group, context.user, context.membership, new_roles ), ): raise HTTPNotFound() When a future PR enables users (not just auth clients) to call the add-membership API the `add_member()` view will have to do something similar: constructing its own `AddGroupMembershipContext` object and passing it to `request.has_permission()`. This means there are two different context objects in play for the `edit_member()` and `add_member()` views: the `context` that is passed to the view is a `GroupMembershipContext`, but the `context` that is passed to `request.has_permission()` is an `EditGroupMembershipContext` or `AddGroupMembershipContext` constructed by the view itself. Solution ------- This commit changes `group_membership_api_factory()` to return a `GroupMembershipContext` for `GET` and `DELETE` requests but return an `EditGroupMembershipContext` for `PATCH` requests and an `AddGroupMembershipContext` for `POST`s. It's not possible for `group_membership_api_factory()` to set the context's `new_roles` attribute: the value for `new_roles` isn't available until later in the request processing cycle after the view has parsed and validated the request's JSON body. So the factory returns `context` objects with `context.new_roles=None` and the `edit_member()` view has to set `new_roles` before calling `has_permission()`: appstruct = EditGroupMembershipAPISchema().validate(json_payload(request)) context.new_roles = appstruct["roles"] if not request.has_permission(Permission.Group.MEMBER_EDIT, context): raise HTTPNotFound() In future the `add_member()` view will have to do the same. So this is still a little weird, but I think it's better than having two different context objects for a single request.
There was a problem hiding this comment.
Copilot reviewed 3 out of 3 changed files in this pull request and generated no suggestions.
Comments skipped due to low confidence (2)
tests/unit/h/views/api/group_members_test.py:126
- Add a test case to handle the scenario where the membership does not exist and ensure the function returns an appropriate error response.
def test_it(self, context, pyramid_request, GroupMembershipJSONPresenter):
tests/functional/api/groups/members_test.py:365
- [nitpick] The
do_requestfunction is defined twice with different parameters. This could be confusing and should be reviewed for consistency.
def do_request(
seanh
force-pushed
the
add-get-membership-api
branch
from
6cbcdcb to
f9a2edb
Compare
seanh
added a commit
that referenced
this pull request
Jan 7, 2025
Problem ------- `group_membership_api_factory()` is the context factory for all views that use the `"api.group_member"` route: * `remove_member()` (`request_method="DELETE"`) * `add_member()` (`request_method="POST"`) * `edit_member()` (`request_method="PATCH"`) * Add in #9197 also `get_member()` (`request_method="GET"`) `group_membership_api_factory()` returns a `GroupMembershipContext` object but: * `GroupMembershipContext` isn't the right context for `edit_member()`: it lacks the `context.new_roles` attribute for the new roles that the request wants to change `context.membership.roles` to. * `GroupMembershipContext` isn't the right context for `add_member()`: it has an inappropriate `context.membership` attribute (when adding a new membership there shouldn't be an existing membership in the context) and it lacks the `context.new_roles` attribute for the roles that the request wants to create a membership with. The context for the `edit_member()` view should be an `EditGroupMembershipContext` object, and for `add_member()` it should be an `AddGroupMembershipContext`. As a result the `edit_member()` view has to create its own context object to pass to `request.has_permission()`: def edit_member(context: GroupMembershipContext, request): appstruct = EditGroupMembershipAPISchema().validate(json_payload(request)) new_roles = appstruct["roles"] if not request.has_permission( Permission.Group.MEMBER_EDIT, EditGroupMembershipContext( context.group, context.user, context.membership, new_roles ), ): raise HTTPNotFound() When a future PR enables users (not just auth clients) to call the add-membership API the `add_member()` view will have to do something similar: constructing its own `AddGroupMembershipContext` object and passing it to `request.has_permission()`. This means there are two different context objects in play for the `edit_member()` and `add_member()` views: the `context` that is passed to the view is a `GroupMembershipContext`, but the `context` that is passed to `request.has_permission()` is an `EditGroupMembershipContext` or `AddGroupMembershipContext` constructed by the view itself. Solution ------- This commit changes `group_membership_api_factory()` to return a `GroupMembershipContext` for `GET` and `DELETE` requests but return an `EditGroupMembershipContext` for `PATCH` requests and an `AddGroupMembershipContext` for `POST`s. It's not possible for `group_membership_api_factory()` to set the context's `new_roles` attribute: the value for `new_roles` isn't available until later in the request processing cycle after the view has parsed and validated the request's JSON body. So the factory returns `context` objects with `context.new_roles=None` and the `edit_member()` view has to set `new_roles` before calling `has_permission()`: appstruct = EditGroupMembershipAPISchema().validate(json_payload(request)) context.new_roles = appstruct["roles"] if not request.has_permission(Permission.Group.MEMBER_EDIT, context): raise HTTPNotFound() In future the `add_member()` view will have to do the same. So this is still a little weird, but I think it's better than having two different context objects for a single request.
seanh
added a commit
that referenced
this pull request
Jan 7, 2025
Problem ------- `group_membership_api_factory()` is the context factory for all views that use the `"api.group_member"` route: * `remove_member()` (`request_method="DELETE"`) * `add_member()` (`request_method="POST"`) * `edit_member()` (`request_method="PATCH"`) * Add in #9197 also `get_member()` (`request_method="GET"`) `group_membership_api_factory()` returns a `GroupMembershipContext` object but: * `GroupMembershipContext` isn't the right context for `edit_member()`: it lacks the `context.new_roles` attribute for the new roles that the request wants to change `context.membership.roles` to. * `GroupMembershipContext` isn't the right context for `add_member()`: it has an inappropriate `context.membership` attribute (when adding a new membership there shouldn't be an existing membership in the context) and it lacks the `context.new_roles` attribute for the roles that the request wants to create a membership with. The context for the `edit_member()` view should be an `EditGroupMembershipContext` object, and for `add_member()` it should be an `AddGroupMembershipContext`. As a result the `edit_member()` view has to create its own context object to pass to `request.has_permission()`: def edit_member(context: GroupMembershipContext, request): appstruct = EditGroupMembershipAPISchema().validate(json_payload(request)) new_roles = appstruct["roles"] if not request.has_permission( Permission.Group.MEMBER_EDIT, EditGroupMembershipContext( context.group, context.user, context.membership, new_roles ), ): raise HTTPNotFound() When a future PR enables users (not just auth clients) to call the add-membership API the `add_member()` view will have to do something similar: constructing its own `AddGroupMembershipContext` object and passing it to `request.has_permission()`. This means there are two different context objects in play for the `edit_member()` and `add_member()` views: the `context` that is passed to the view is a `GroupMembershipContext`, but the `context` that is passed to `request.has_permission()` is an `EditGroupMembershipContext` or `AddGroupMembershipContext` constructed by the view itself. Solution ------- This commit changes `group_membership_api_factory()` to return a `GroupMembershipContext` for `GET` and `DELETE` requests but return an `EditGroupMembershipContext` for `PATCH` requests and an `AddGroupMembershipContext` for `POST`s. It's not possible for `group_membership_api_factory()` to set the context's `new_roles` attribute: the value for `new_roles` isn't available until later in the request processing cycle after the view has parsed and validated the request's JSON body. So the factory returns `context` objects with `context.new_roles=None` and the `edit_member()` view has to set `new_roles` before calling `has_permission()`: appstruct = EditGroupMembershipAPISchema().validate(json_payload(request)) context.new_roles = appstruct["roles"] if not request.has_permission(Permission.Group.MEMBER_EDIT, context): raise HTTPNotFound() In future the `add_member()` view will have to do the same. So this is still a little weird, but I think it's better than having two different context objects for a single request.
seanh
added a commit
that referenced
this pull request
Jan 7, 2025
This adds docs for the API that was added in #9197
seanh
added a commit
that referenced
this pull request
Jan 7, 2025
This adds docs for the API that was added in #9197
seanh
added a commit
that referenced
this pull request
Jan 7, 2025
This adds docs for the API that was added in #9197
seanh
added a commit
that referenced
this pull request
Jan 7, 2025
This adds docs for the API that was added in #9197
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add an API for getting a single membership. This isn't needed by any of our own frontend code, it's just to round out the public memberships API.