From d8a7f6233ac619ade072037741b186d496d0fde2 Mon Sep 17 00:00:00 2001 From: Guillaume Viger Date: Thu, 18 Jul 2024 14:55:54 -0400 Subject: [PATCH] membership-request [#855]: implement decision flow - update role - addressed TODOs --- .../api/membershipRequests/api.js | 12 +- .../MembershipRequestsEmptyResults.js | 2 +- .../MembershipRequestsResultItem.js | 33 +- .../members/membership_requests/index.js | 2 - invenio_communities/members/records/api.py | 5 +- .../members/records/dumpers.py | 2 +- .../members/resources/config.py | 4 +- .../members/resources/resource.py | 14 + .../members/services/components.py | 4 +- .../members/services/config.py | 1 + .../members/services/request.py | 31 +- .../members/services/schemas.py | 23 +- .../members/services/service.py | 589 +++++++++--------- invenio_communities/views/communities.py | 3 +- tests/members/test_members_resource.py | 62 +- tests/members/test_members_services.py | 178 ++++-- 16 files changed, 558 insertions(+), 407 deletions(-) diff --git a/invenio_communities/assets/semantic-ui/js/invenio_communities/api/membershipRequests/api.js b/invenio_communities/assets/semantic-ui/js/invenio_communities/api/membershipRequests/api.js index c9954d29f..6a57611ff 100644 --- a/invenio_communities/assets/semantic-ui/js/invenio_communities/api/membershipRequests/api.js +++ b/invenio_communities/assets/semantic-ui/js/invenio_communities/api/membershipRequests/api.js @@ -5,9 +5,11 @@ // Invenio-communities is free software; you can redistribute it and/or modify it // under the terms of the MIT License; see LICENSE file for more details. -import { CommunityLinksExtractor } from "../CommunityLinksExtractor"; import { http } from "react-invenio-forms"; +import { CommunityLinksExtractor } from "../CommunityLinksExtractor"; +import { bulkMembersSerializer } from "../serializers"; + /** * API Client for community membership requests. * @@ -21,6 +23,14 @@ export class CommunityMembershipRequestsApi { } requestMembership = async (payload) => { + // assigned rather than defiend for ease of passing as callback return await http.post(this.linksExtractor.url("membership_requests"), payload); }; + + updateRole = async (membershipRequest, role) => { + // assigned rather than defiend for ease of passing as callback + const memberSerialized = bulkMembersSerializer([membershipRequest]); + const payload = { members: memberSerialized, role: role }; + return await http.put(this.linksExtractor.url("membership_requests"), payload); + }; } diff --git a/invenio_communities/assets/semantic-ui/js/invenio_communities/members/membership_requests/MembershipRequestsEmptyResults.js b/invenio_communities/assets/semantic-ui/js/invenio_communities/members/membership_requests/MembershipRequestsEmptyResults.js index 0b5e7e6f8..a658b042a 100644 --- a/invenio_communities/assets/semantic-ui/js/invenio_communities/members/membership_requests/MembershipRequestsEmptyResults.js +++ b/invenio_communities/assets/semantic-ui/js/invenio_communities/members/membership_requests/MembershipRequestsEmptyResults.js @@ -13,7 +13,7 @@ class MembershipRequestsEmptyResultsCmp extends Component {
- {i18next.t("No matching members found.")} + {i18next.t("No matching membership requests found.")}
{queryString && (

diff --git a/invenio_communities/assets/semantic-ui/js/invenio_communities/members/membership_requests/MembershipRequestsResultItem.js b/invenio_communities/assets/semantic-ui/js/invenio_communities/members/membership_requests/MembershipRequestsResultItem.js index 79ec348ad..eca887838 100644 --- a/invenio_communities/assets/semantic-ui/js/invenio_communities/members/membership_requests/MembershipRequestsResultItem.js +++ b/invenio_communities/assets/semantic-ui/js/invenio_communities/members/membership_requests/MembershipRequestsResultItem.js @@ -15,8 +15,9 @@ import React, { Component } from "react"; import { Image } from "react-invenio-forms"; import { Grid, Item, Table } from "semantic-ui-react"; +import { MembershipRequestsContext } from "../../api/membershipRequests/MembershipRequestsContextProvider"; import { RoleDropdown } from "../components/dropdowns"; -import { formattedTime } from "../utils"; +import { buildRequest, formattedTime } from "../utils"; export class MembershipRequestsResultItem extends Component { constructor(props) { @@ -25,6 +26,8 @@ export class MembershipRequestsResultItem extends Component { this.state = { membershipRequest: result }; } + static contextType = MembershipRequestsContext; + update = (data, value) => { const { membershipRequest } = this.state; this.setState({ membershipRequest: { ...membershipRequest, ...{ role: value } } }); @@ -39,13 +42,15 @@ export class MembershipRequestsResultItem extends Component { } = this.props; const { - membershipRequest: { member, request }, + membershipRequest: { member }, membershipRequest, } = this.state; - // TODO: Decision flow - // const { api: membershipRequestsApi } = this.context; - const rolesCanAssignByType = rolesCanAssign[member.type]; - const membershipRequestExpiration = formattedTime(request.expires_at); + + const request = buildRequest(membershipRequest, ["accept", "decline"]); + const { api: membershipRequestsApi } = this.context; + const roles = rolesCanAssign[member.type]; + const expiration = formattedTime(request.expires_at); + return ( @@ -73,17 +78,16 @@ export class MembershipRequestsResultItem extends Component { - {membershipRequestExpiration} + {expiration} console.log("actionSuccessCallback called")} + request={request} + actionSuccessCallback={() => { + window.location.reload(); + }} /> diff --git a/invenio_communities/assets/semantic-ui/js/invenio_communities/members/membership_requests/index.js b/invenio_communities/assets/semantic-ui/js/invenio_communities/members/membership_requests/index.js index 7eefec3e0..8986ba6de 100644 --- a/invenio_communities/assets/semantic-ui/js/invenio_communities/members/membership_requests/index.js +++ b/invenio_communities/assets/semantic-ui/js/invenio_communities/members/membership_requests/index.js @@ -39,8 +39,6 @@ const dataAttr = document.getElementById( const community = JSON.parse(dataAttr.community); const communitiesAllRoles = JSON.parse(dataAttr.communitiesAllRoles); const communitiesRolesCanAssign = JSON.parse(dataAttr.communitiesRolesCanAssign); -// TODO: Decision flow: do we need? -// const permissions = JSON.parse(dataAttr.permissions); const appName = "InvenioCommunities.MembershipRequestsSearch"; diff --git a/invenio_communities/members/records/api.py b/invenio_communities/members/records/api.py index 41edcb429..1fb4649b3 100644 --- a/invenio_communities/members/records/api.py +++ b/invenio_communities/members/records/api.py @@ -204,7 +204,10 @@ class Member(Record, MemberMixin): class ArchivedInvitation(Record, MemberMixin): - """An archived invitation record. + """An archived invitation or membership request record. + + The name is a historical legacy. It could be renamed in the future, but we don't + want to rename the index. We are using a record without using the actual JSON document and schema validation normally used in a record. The reason for using a record diff --git a/invenio_communities/members/records/dumpers.py b/invenio_communities/members/records/dumpers.py index 5d797e4b2..8fa476b72 100644 --- a/invenio_communities/members/records/dumpers.py +++ b/invenio_communities/members/records/dumpers.py @@ -30,6 +30,6 @@ def dump(self, record, data): def load(self, data, record_cls): """Load relations.request.type. - TODO: Works without it for now. Potentially revisit? + Works without implementation for now. """ pass diff --git a/invenio_communities/members/resources/config.py b/invenio_communities/members/resources/config.py index 29eb164fa..3ccaac9ad 100644 --- a/invenio_communities/members/resources/config.py +++ b/invenio_communities/members/resources/config.py @@ -46,7 +46,9 @@ class MemberResourceConfig(RecordResourceConfig): AlreadyMemberError: create_error_handler( HTTPJSONException( code=400, - description="A member was already added or invited.", + description=_( + "A membership already exists or is already being assessed." + ), ) ), CommunityDeletedError: create_error_handler( diff --git a/invenio_communities/members/resources/resource.py b/invenio_communities/members/resources/resource.py index f4e342d92..4cf1205dd 100644 --- a/invenio_communities/members/resources/resource.py +++ b/invenio_communities/members/resources/resource.py @@ -39,6 +39,9 @@ def create_url_rules(self): route( "GET", routes["membership_requests"], self.search_membership_requests ), + route( + "PUT", routes["membership_requests"], self.update_membership_requests + ), ] @request_view_args @@ -160,3 +163,14 @@ def search_membership_requests(self): search_preference=search_preference(), ) return hits.to_dict(), 200 + + @request_view_args + @request_extra_args + @request_data + def update_membership_requests(self): + """Update membership request. + + From the outside, a membership request is its own resource. + From the inside, it's just a member when it comes to update. + """ + return self.update() diff --git a/invenio_communities/members/services/components.py b/invenio_communities/members/services/components.py index f4c133b4d..74d713851 100644 --- a/invenio_communities/members/services/components.py +++ b/invenio_communities/members/services/components.py @@ -52,8 +52,8 @@ def _member_changed(self, member, community=None): for user_id in user_ids: on_user_membership_change(Identity(user_id)) - def accept_invite(self, identity, record=None, data=None, **kwargs): - """On accept invite.""" + def accept_member_request(self, identity, record=None, data=None, **kwargs): + """Upon acceptance of a member request (invitation or membership request).""" self._member_changed(record) def members_add(self, identity, record=None, community=None, data=None, **kwargs): diff --git a/invenio_communities/members/services/config.py b/invenio_communities/members/services/config.py index e7d573b0e..7a62abff0 100644 --- a/invenio_communities/members/services/config.py +++ b/invenio_communities/members/services/config.py @@ -182,6 +182,7 @@ class MemberServiceConfig(RecordServiceConfig, ConfiguratorMixin): search = MemberSearchOptions search_public = PublicSearchOptions search_invitations = InvitationsSearchOptions + search_membership_requests = InvitationsSearchOptions # Use same as invitations links_item = { "actions": LinksForActionsOfMember( diff --git a/invenio_communities/members/services/request.py b/invenio_communities/members/services/request.py index 2eac4ef56..0f73b5650 100644 --- a/invenio_communities/members/services/request.py +++ b/invenio_communities/members/services/request.py @@ -47,7 +47,7 @@ class AcceptAction(actions.AcceptAction): def execute(self, identity, uow): """Execute action.""" - service().accept_invite(system_identity, self.request.id, uow=uow) + service().accept_member_request(system_identity, self.request.id, uow=uow) uow.register( NotificationOp( CommunityInvitationAcceptNotificationBuilder.build(self.request) @@ -61,7 +61,7 @@ class DeclineAction(actions.DeclineAction): def execute(self, identity, uow): """Execute action.""" - service().decline_invite(system_identity, self.request.id, uow=uow) + service().close_member_request(system_identity, self.request.id, uow=uow) uow.register( NotificationOp( CommunityInvitationDeclineNotificationBuilder.build(self.request) @@ -75,7 +75,7 @@ class CancelAction(actions.CancelAction): def execute(self, identity, uow): """Execute action.""" - service().decline_invite(system_identity, self.request.id, uow=uow) + service().close_member_request(system_identity, self.request.id, uow=uow) uow.register( NotificationOp( CommunityInvitationCancelNotificationBuilder.build(self.request) @@ -89,7 +89,7 @@ class ExpireAction(actions.ExpireAction): def execute(self, identity, uow): """Execute action.""" - service().decline_invite(system_identity, self.request.id, uow=uow) + service().close_member_request(system_identity, self.request.id, uow=uow) uow.register( NotificationOp( CommunityInvitationExpireNotificationBuilder.build(self.request) @@ -141,27 +141,32 @@ class CancelMembershipRequestAction(actions.CancelAction): def execute(self, identity, uow): """Execute action.""" - service().close_membership_request(system_identity, self.request.id, uow=uow) + service().close_member_request(system_identity, self.request.id, uow=uow) # TODO: Notification flow: Investigate notifications super().execute(identity, uow) -class AcceptMembershipRequestAction(actions.AcceptAction): - """Accept membership request action.""" +class DeclineMembershipRequestAction(actions.DeclineAction): + """Decline membership request action.""" def execute(self, identity, uow): """Execute action.""" - # TODO: Decision flow: Implement me - pass + service().close_member_request(system_identity, self.request.id, uow=uow) + # TODO: Notification flow: Investigate notifications + super().execute(identity, uow) -class DeclineMembershipRequestAction(actions.DeclineAction): - """Decline membership request action.""" +# TODO: Expiration flow: ExpireAction + + +class AcceptMembershipRequestAction(actions.AcceptAction): + """Accept membership request action.""" def execute(self, identity, uow): """Execute action.""" - # TODO: Decision flow: Implement me - pass + service().accept_member_request(system_identity, self.request.id, uow=uow) + # TODO: Notification flow: Investigate notifications + super().execute(identity, uow) class MembershipRequestRequestType(RequestType): diff --git a/invenio_communities/members/services/schemas.py b/invenio_communities/members/services/schemas.py index 471018072..bca6e795c 100644 --- a/invenio_communities/members/services/schemas.py +++ b/invenio_communities/members/services/schemas.py @@ -229,9 +229,24 @@ def get_permissions(self, obj): class MembershipRequestDumpSchema(MemberDumpSchema): - """Schema for dumping membership requests. - - TODO: Decision flow: Investigate if can be merged with InvitationDumpSchema - """ + """Schema for dumping membership requests.""" request = fields.Nested(RequestSchema) + + def get_permissions(self, obj): + """Get permission. + + Only permission to see if current identity can_update role is needed. + + :param obj: api.Member + """ + is_open = obj["request"]["is_open"] + permission_check = self.context["field_permission_check"] + can_update = permission_check( + "members_update", + community_id=obj.community_id, + member=obj, + ) + return { + "can_update_role": is_open and can_update + } diff --git a/invenio_communities/members/services/service.py b/invenio_communities/members/services/service.py index 5c031036d..8f00b5bdd 100644 --- a/invenio_communities/members/services/service.py +++ b/invenio_communities/members/services/service.py @@ -87,7 +87,7 @@ def invitation_dump_schema(self): @property def membership_request_dump_schema(self): - """Schema for dumping a membership request to JSON.""" + """Schema for dumping a membership request to REST API JSON.""" return ServiceSchemaWrapper(self, schema=MembershipRequestDumpSchema) # Loading schemas @@ -141,50 +141,8 @@ def archive_indexer(self): ) # - # Add and invite + # General # - @unit_of_work() - def add(self, identity, community_id, data, uow=None): - """Add group members. - - The default permission policy only allow groups to be added. Users must - be invited. - """ - ret = self._create( - identity, - community_id, - data, - self.add_schema, - "members_add", - self._add_factory, - uow, - ) - # ensure index is refreshed to search for newly added members - uow.register(IndexRefreshOp(indexer=self.indexer)) - return ret - - @unit_of_work() - def invite(self, identity, community_id, data, uow=None): - """Invite group members. - - Only users and email member types can be invited, and a member can only - have one invitation per community - - Email member type is not yet supported. - """ - ret = self._create( - identity, - community_id, - data, - self.invite_schema, - "members_invite", - self._invite_factory, - uow, - ) - - # ensure index is refreshed to search for newly added members - uow.register(IndexRefreshOp(indexer=self.indexer)) - return ret def _create(self, identity, community_id, data, schema, action, factory, uow): """Internal method used for both adding and inviting users/groups. @@ -279,182 +237,6 @@ def _add_factory( # instead of N single indexing requests. uow.register(RecordCommitOp(member, indexer=self.indexer)) - def _invite_factory(self, identity, community, role, visible, member, message, uow): - """Invite a member to the community.""" - if member["type"] == "group": - # Groups cannot be invited, because groups have no one who can - # accept an invitation. - raise InvalidMemberError(member) - - # Add member entry - if member["type"] == "user": - # Create request - title = _('Invitation to join "{community}"').format( - community=community.metadata["title"], - ) - description = _('You will join as "{role}".').format(role=role.title) - - request_item = current_requests_service.create( - identity, - { - "title": title, - "description": description, - }, - CommunityInvitation, - receiver={"user": member["id"]}, - creator=community, - # TODO: perhaps topic should be the actual membership record - # instead - topic=community, - expires_at=invite_expires_at(), - uow=uow, - ) - - # message was provided. - if message: - data = {"payload": {"content": message}} - current_events_service.create( - identity, - request_item.id, - data, - CommentEventType, - uow=uow, - notify=False, - ) - - uow.register( - NotificationOp( - CommunityInvitationSubmittedNotificationBuilder.build( - request=request_item._request, - # explicit string conversion to get the value of LazyText - role=str(role.title), - message=message, - ) - ) - ) - - # Create an inactive member entry linked to the request. - self._add_factory( - identity, - community, - role, - visible, - member, - message, - uow, - active=False, - request_id=request_item.id, - ) - - def search( - self, - identity, - community_id, - params=None, - search_preference=None, - extra_filter=None, - **kwargs - ): - """Search.""" - # Apply extra filters - filter_ = dsl.Q("term", **{"active": True}) - if extra_filter: - filter_ &= extra_filter - - return self._members_search( - identity, - community_id, - "members_search", - self.member_dump_schema, - self.config.search, - extra_filter=filter_, - params=params, - search_preference=search_preference, - endpoint="members", - links_item_tpl=LinksTemplate(self.config.links_item), - **kwargs - ) - - def scan( - self, - identity, - community_id, - params=None, - search_preference=None, - extra_filter=None, - **kwargs - ): - """Scan community members to retrieve all matching the query.""" - # Apply extra filters - filter_ = dsl.Q("term", **{"active": True}) - if extra_filter: - filter_ &= extra_filter - - return self._members_search( - identity, - community_id, - "members_search", - self.member_dump_schema, - self.config.search, - extra_filter=filter_, - scan_params=params, - search_preference=search_preference, - scan=True, - # just in case scan becomes accessible through a resource URL - endpoint="members", - links_item_tpl=LinksTemplate(self.config.links_item), - **kwargs - ) - - def search_public( - self, identity, community_id, params=None, search_preference=None, **kwargs - ): - """Search public members matching the querystring.""" - # The search for members is split two methods (public, members) to - # prevent leaking of information. E.g. the public serialization - # must not have all fields present. - # TODO: limit fields on which the query works on to avoid leaking information - return self._members_search( - identity, - community_id, - "members_search_public", - self.public_dump_schema, - self.config.search_public, - extra_filter=( - dsl.Q("term", **{"visible": True}) & dsl.Q("term", **{"active": True}) - ), - params=params, - search_preference=search_preference, - endpoint="members", - links_item_tpl=LinksTemplate(self.config.links_item), - **kwargs - ) - - def search_invitations( - self, identity, community_id, params=None, search_preference=None, **kwargs - ): - """Search invitations.""" - # The search for invitations used the ArchivedInvitation record class - # which will search over the "communitymembers" alias which include - # both current invitations and archived invitations. - return self._members_search( - identity, - community_id, - "search_invites", - self.invitation_dump_schema, - self.config.search_invitations, - record_cls=ArchivedInvitation, - extra_filter=( - dsl.Q("term", **{"active": False}) - & dsl.Q("term", **{"request.type": CommunityInvitation.type_id}) - ), - params=params, - search_preference=search_preference, - endpoint="invitations", - links_item_tpl=LinksTemplate(self.config.links_item), - **kwargs - ) - def _members_search( self, identity, @@ -471,7 +253,7 @@ def _members_search( links_item_tpl=None, **kwargs ): - """Members search.""" + """General members search.""" community = self.community_cls.get_record(community_id) self.require_permission(identity, permission_action, record=community) @@ -522,16 +304,12 @@ def _members_search( schema=schema, ) - def read_memberships(self, identity): - """Searches the memberships of a specific user/identity.""" - return {"memberships": self.config.record_cls.get_memberships(identity)} - @unit_of_work() def update(self, identity, community_id, data, uow=None, refresh=False): """Bulk update. - Used to update both active members and active invitations. Archived - invitations cannot be updated. + Used to update members, invitations and membership requests. Archived + requests cannot be updated. """ community = self.community_cls.get_record(community_id) @@ -693,80 +471,316 @@ def delete(self, identity, community_id, data, uow=None): return True + def read_many(self, *args, **kwargs): + """Not implemented.""" + raise NotImplementedError("Use search() or search_public()") + + def read_all(self, *args, **kwargs): + """Not implemented.""" + raise NotImplementedError("Use search() or search_public()") + + def read(self, *args, **kwargs): + """Not implemented.""" + raise NotImplementedError("Use search() or search_public()") + + def create(self, *args, **kwargs): + """Not implemented.""" + raise NotImplementedError("Use add() or invite()") + + def rebuild_index(self, identity, uow=None): + """Reindex all records managed by this service. + + Note: Skips (soft) deleted records. + """ + members = self.record_cls.model_cls.query.filter_by(is_deleted=False).all() + self.indexer.bulk_index([member.id for member in members]) + + archived_invitations = ArchivedInvitation.model_cls.query.filter_by( + is_deleted=False + ).all() + self.archive_indexer.bulk_index([inv.id for inv in archived_invitations]) + + return True + @unit_of_work() - def accept_invite(self, identity, request_id, uow=None): - """Accept an invitation.""" + def close_member_request(self, identity, request_id, uow=None): + """Close member request (invitation or membership request). + + Used when cancelling, declining, or expiring. + """ # Permissions are checked on the request action assert identity == system_identity member = self.record_cls.get_member_by_request(request_id) assert member.active is False archived_invitation = ArchivedInvitation.create_from_member(member) - member.active = True + uow.register(RecordDeleteOp(member, indexer=self.indexer, force=True)) + uow.register(RecordCommitOp(archived_invitation, indexer=self.archive_indexer)) + uow.register( + IndexRefreshOp( + # need to use an indexer with a diff index + # no access to invitations indexer + indexer=self.indexer, + index=ArchivedInvitation.index, + ) + ) + + @unit_of_work() + def accept_member_request(self, identity, request_id, uow=None): + """Accept a member request (invitation or membership request).""" + # Permissions are checked on the request action + assert identity == system_identity + member = self.record_cls.get_member_by_request(request_id) + assert member.active is False + archived_request = ArchivedInvitation.create_from_member(member) + member.active = True # TODO: recompute permissions for member. # Run components self.run_components( - "accept_invite", + "accept_member_request", identity, record=member, errors=None, uow=uow, ) - uow.register(RecordCommitOp(member, indexer=self.indexer)) - uow.register(RecordCommitOp(archived_invitation, indexer=self.archive_indexer)) + uow.register(RecordCommitOp(archived_request, indexer=self.archive_indexer)) uow.register(IndexRefreshOp(indexer=self.indexer)) + # + # Members + # + @unit_of_work() - def decline_invite(self, identity, request_id, uow=None): - """Decline an invitation.""" - # Permissions are checked on the request action - assert identity == system_identity - member = self.record_cls.get_member_by_request(request_id) - assert member.active is False - archived_invitation = ArchivedInvitation.create_from_member(member) - uow.register(RecordDeleteOp(member, indexer=self.indexer, force=True)) - uow.register(RecordCommitOp(archived_invitation, indexer=self.archive_indexer)) - uow.register( - IndexRefreshOp( - # need to use an indexer with a diff index - # no access to invitations indexer - indexer=self.indexer, - index=ArchivedInvitation.index, - ) + def add(self, identity, community_id, data, uow=None): + """Add group members. + + The default permission policy only allow groups to be added. Users must + be invited. + """ + ret = self._create( + identity, + community_id, + data, + self.add_schema, + "members_add", + self._add_factory, + uow, ) + # ensure index is refreshed to search for newly added members + uow.register(IndexRefreshOp(indexer=self.indexer)) + return ret - def read_many(self, *args, **kwargs): - """Not implemented.""" - raise NotImplementedError("Use search() or search_public()") + def search( + self, + identity, + community_id, + params=None, + search_preference=None, + extra_filter=None, + **kwargs + ): + """Search.""" + # Apply extra filters + filter_ = dsl.Q("term", **{"active": True}) + if extra_filter: + filter_ &= extra_filter - def read_all(self, *args, **kwargs): - """Not implemented.""" - raise NotImplementedError("Use search() or search_public()") + return self._members_search( + identity, + community_id, + "members_search", + self.member_dump_schema, + self.config.search, + extra_filter=filter_, + params=params, + search_preference=search_preference, + endpoint="members", + links_item_tpl=LinksTemplate(self.config.links_item), + **kwargs + ) - def read(self, *args, **kwargs): - """Not implemented.""" - raise NotImplementedError("Use search() or search_public()") + def scan( + self, + identity, + community_id, + params=None, + search_preference=None, + extra_filter=None, + **kwargs + ): + """Scan community members to retrieve all matching the query.""" + # Apply extra filters + filter_ = dsl.Q("term", **{"active": True}) + if extra_filter: + filter_ &= extra_filter - def create(self, *args, **kwargs): - """Not implemented.""" - raise NotImplementedError("Use add() or invite()") + return self._members_search( + identity, + community_id, + "members_search", + self.member_dump_schema, + self.config.search, + extra_filter=filter_, + scan_params=params, + search_preference=search_preference, + scan=True, + # just in case scan becomes accessible through a resource URL + endpoint="members", + links_item_tpl=LinksTemplate(self.config.links_item), + **kwargs + ) - def rebuild_index(self, identity, uow=None): - """Reindex all records managed by this service. + def search_public( + self, identity, community_id, params=None, search_preference=None, **kwargs + ): + """Search public members matching the querystring.""" + # The search for members is split two methods (public, members) to + # prevent leaking of information. E.g. the public serialization + # must not have all fields present. + # TODO: limit fields on which the query works on to avoid leaking information + return self._members_search( + identity, + community_id, + "members_search_public", + self.public_dump_schema, + self.config.search_public, + extra_filter=( + dsl.Q("term", **{"visible": True}) & dsl.Q("term", **{"active": True}) + ), + params=params, + search_preference=search_preference, + endpoint="members", + links_item_tpl=LinksTemplate(self.config.links_item), + **kwargs + ) - Note: Skips (soft) deleted records. + def read_memberships(self, identity): + """Searches the memberships of a specific user/identity.""" + return {"memberships": self.config.record_cls.get_memberships(identity)} + + # + # Invitations + # + + @unit_of_work() + def invite(self, identity, community_id, data, uow=None): + """Invite group members. + + Only users and email member types can be invited, and a member can only + have one invitation per community + + Email member type is not yet supported. """ - members = self.record_cls.model_cls.query.filter_by(is_deleted=False).all() - self.indexer.bulk_index([member.id for member in members]) + ret = self._create( + identity, + community_id, + data, + self.invite_schema, + "members_invite", + self._invite_factory, + uow, + ) - archived_invitations = ArchivedInvitation.model_cls.query.filter_by( - is_deleted=False - ).all() - self.archive_indexer.bulk_index([inv.id for inv in archived_invitations]) + # ensure index is refreshed to search for newly added members + uow.register(IndexRefreshOp(indexer=self.indexer)) + return ret - return True + def _invite_factory(self, identity, community, role, visible, member, message, uow): + """Invite a member to the community.""" + if member["type"] == "group": + # Groups cannot be invited, because groups have no one who can + # accept an invitation. + raise InvalidMemberError(member) + + # Add member entry + if member["type"] == "user": + # Create request + title = _('Invitation to join "{community}"').format( + community=community.metadata["title"], + ) + description = _('You will join as "{role}".').format(role=role.title) + + request_item = current_requests_service.create( + identity, + { + "title": title, + "description": description, + }, + CommunityInvitation, + receiver={"user": member["id"]}, + creator=community, + # TODO: perhaps topic should be the actual membership record + # instead + topic=community, + expires_at=invite_expires_at(), + uow=uow, + ) + # message was provided. + if message: + data = {"payload": {"content": message}} + current_events_service.create( + identity, + request_item.id, + data, + CommentEventType, + uow=uow, + notify=False, + ) + + uow.register( + NotificationOp( + CommunityInvitationSubmittedNotificationBuilder.build( + request=request_item._request, + # explicit string conversion to get the value of LazyText + role=str(role.title), + message=message, + ) + ) + ) + + # Create an inactive member entry linked to the request. + self._add_factory( + identity, + community, + role, + visible, + member, + message, + uow, + active=False, + request_id=request_item.id, + ) + + def search_invitations( + self, identity, community_id, params=None, search_preference=None, **kwargs + ): + """Search invitations.""" + # The search for invitations used the ArchivedInvitation record class + # which will search over the "communitymembers" alias which include + # both current invitations and archived invitations. + return self._members_search( + identity, + community_id, + "search_invites", + self.invitation_dump_schema, + self.config.search_invitations, + record_cls=ArchivedInvitation, + extra_filter=( + dsl.Q("term", **{"active": False}) + & dsl.Q("term", **{"request.type": CommunityInvitation.type_id}) + ), + params=params, + search_preference=search_preference, + endpoint="invitations", + links_item_tpl=LinksTemplate(self.config.links_item), + **kwargs + ) + + # # Request membership + # + @unit_of_work() def request_membership(self, identity, community_id, data, uow=None): """Request membership to the community. @@ -850,12 +864,6 @@ def request_membership(self, identity, community_id, data, uow=None): # Has to return the request so that frontend can redirect to it return request_item - @unit_of_work() - def update_membership_request(self, identity, community_id, data, uow=None): - """Update membership request.""" - # TODO: Implement me - pass - def search_membership_requests( self, identity, community_id, params=None, search_preference=None, **kwargs ): @@ -865,9 +873,8 @@ def search_membership_requests( community_id, "search_membership_requests", self.membership_request_dump_schema, - # Use same as invitations - self.config.search_invitations, # TODO: Decision flow: Rename/merge ? - record_cls=ArchivedInvitation, # TODO: Decision flow: merge or new? + self.config.search_membership_requests, + record_cls=ArchivedInvitation, extra_filter=( dsl.Q("term", **{"active": False}) & dsl.Q( @@ -881,28 +888,6 @@ def search_membership_requests( **kwargs ) - @unit_of_work() - def accept_membership_request(self, identity, request_id, uow=None): - """Accept membership request.""" - # TODO: Implement me - pass - - @unit_of_work() - def close_membership_request(self, identity, request_id, uow=None): - """Close membership request. - - Used for cancelling, declining, or expiring a membership request. - - For now we just delete the "fake" member that was created in - request_membership. TODO: explore alternatives/ramifications at a - later point. - """ - # Permissions are checked on the request action - assert identity == system_identity - member = self.record_cls.get_member_by_request(request_id) - assert member.active is False - uow.register(RecordDeleteOp(member, indexer=self.indexer, force=True)) - def get_pending_request_id_if_any(self, user_id, community_id): """Utility function to get associated active request id. diff --git a/invenio_communities/views/communities.py b/invenio_communities/views/communities.py index e0effcdd4..2f2f909f5 100644 --- a/invenio_communities/views/communities.py +++ b/invenio_communities/views/communities.py @@ -466,7 +466,8 @@ def membership_requests(pid_value, community, community_ui): "invenio_communities/details/members/membership_requests.html", theme=community_ui.get("theme", {}), community=community_ui, - roles_can_assign=_get_roles_can_invite(community.id), # TODO: Decision flow + # We use the same roles as the ones that can be invited + roles_can_assign=_get_roles_can_invite(community.id), permissions=permissions, ) diff --git a/tests/members/test_members_resource.py b/tests/members/test_members_resource.py index 3042e38e4..3b89610fc 100644 --- a/tests/members/test_members_resource.py +++ b/tests/members/test_members_resource.py @@ -238,6 +238,10 @@ def test_search( assert r.status_code == 200 data = r.json assert data["sortBy"] == "name" + expected_links = { + "self": f"https://127.0.0.1:5000/api/communities/{community_id}/members?page=1&size=25&sort=name" # noqa + } + assert expected_links == data["links"] assert data["hits"]["total"] == 3 assert "role" in data["aggregations"] assert "visibility" in data["aggregations"] @@ -300,6 +304,10 @@ def test_search_public( assert r.status_code == 200 data = r.json assert data["sortBy"] == "name" + expected_links = { + "self": f"https://127.0.0.1:5000/api/communities/{community_id}/members?page=1&size=25&sort=name" # noqa + } + assert expected_links == data["links"] assert data["hits"]["total"] == 1 hit = data["hits"]["hits"][0] # Public view has no facets (because that would leak information on @@ -350,9 +358,15 @@ def test_search_invitation( assert hit["permissions"]["can_update_role"] is True assert hit["permissions"]["can_cancel"] is True + request_id = hit["request"]["id"] + expected_links = { + "actions": { + "cancel": f"https://127.0.0.1:5000/api/requests/{request_id}/actions/cancel", + }, + } + assert expected_links == hit["links"] + -# TODO: member serialization/links -# TODO: request serialization/links # TODO: community member can see info # TODO: community non-member can't see info # TODO: facet by role, facet by visibility, define sorts. @@ -375,9 +389,8 @@ def test_post_membership_requests(app, client, headers, community_id, create_use headers=headers, json={"message": "Can I join the club?"}, ) - assert 201 == r.status_code - RequestEvent.index.refresh() + assert 201 == r.status_code # Get links to check url_of_request = r.json["links"]["self"].replace(app.config["SITE_API_URL"], "") @@ -399,22 +412,25 @@ def test_post_membership_requests(app, client, headers, community_id, create_use assert "Can I join the club?" == msg -def test_put_membership_requests( - client, headers, community_id, owner, new_user_data, db -): - # update membership request - # TODO: Implement me! - assert True - - def test_error_handling_for_membership_requests( - client, headers, community_id, owner, new_user_data, db + client, create_user, headers, community_id, owner, new_user_data, db ): - # TODO: Implement me! - # error handling registered - # - permission handling registered - # - duplicate handling registered - assert True + user = create_user({"email": "user_foo@example.org", "username": "user_foo"}) + client = user.login(client) + r = client.post( + f"/communities/{community_id}/membership-requests", + headers=headers, + json={"message": "Can I join the club?"}, + ) + assert 201 == r.status_code + + # Error if duplicate posting + r = client.post( + f"/communities/{community_id}/membership-requests", + headers=headers, + json={"message": "Can I join the club again?"}, + ) + assert 400 == r.status_code def test_get_membership_requests( @@ -451,6 +467,7 @@ def test_get_membership_requests( assert "id" in hit["request"] assert "status" in hit["request"] assert "expires_at" in hit["request"] + # hits > hit > links request_id = hit["request"]["id"] expected_links = { "actions": { @@ -459,11 +476,8 @@ def test_get_membership_requests( }, } assert expected_links == hit["links"] + # hits > hit > permissions + assert hit["permissions"]["can_update_role"] is True + # TODO: Expiration flow : assess if expiration makes sense for membership requests. # assert hit["request"]["expires_at"] is not None - # TODO: Decision flow - # hits > hit > permissions - # assert "permissions" in hit - # assert hit["permissions"]["can_update_role"] is True - # assert hit["permissions"]["can_cancel"] is True - # hits > hit > links diff --git a/tests/members/test_members_services.py b/tests/members/test_members_services.py index 5d03e2558..b3e5eef2c 100644 --- a/tests/members/test_members_services.py +++ b/tests/members/test_members_services.py @@ -666,7 +666,7 @@ def test_leave_owner_allowed(member_service, community, owner, group, db): member_service.add(owner.identity, community._record.id, data) # Leave - data = {"members": [{"type": "user", "id": str(owner.id)}]} + data = {"members": [{"type": "group", "id": group.name}]} assert member_service.delete(owner.identity, community._record.id, data) @@ -1166,43 +1166,6 @@ def test_update_invalid_data(member_service, community, group): # -def test_request_cancel_request_flow( - member_service, - community, - create_user, - requests_service, - db, - # search_clear, - clean_index, -): - """Check creation of membership request after first creation closed. - - This tests a temporary business rule that should be revisited later. - """ - # Create membership request - user = create_user() - data = { - "message": "Can I join the club?", - } - membership_request = member_service.request_membership( - user.identity, - community._record.id, - data, - ) - - # Close request - here via cancel - request = requests_service.execute_action( - user.identity, membership_request.id, "cancel" - ).to_dict() - - # Should be possible to create a new one again - membership_request_2 = member_service.request_membership( - user.identity, - community._record.id, - {"message": "Oops didn't mean to cancel. Oh well, I will request again."}, - ) - - def test_get_pending_request_id_if_any( member_service, community, @@ -1210,7 +1173,6 @@ def test_get_pending_request_id_if_any( create_user, requests_service, db, - # search_clear, clean_index, ): user = create_user() @@ -1261,7 +1223,143 @@ def test_get_pending_request_id_if_any( assert request_id is None -# TODO: Decision flow: test "archived" membership requests +def test_request_membership_cancel_request_flow( + member_service, + community, + owner, + create_user, + requests_service, + db, + clean_index, +): + """Check creation of membership request after first creation closed. + + This tests a temporary business rule that should be revisited later. + """ + # Create membership request + user = create_user() + data = { + "message": "Can I join the club?", + } + membership_request = member_service.request_membership( + user.identity, + community._record.id, + data, + ) + + # Cancel request + request = requests_service.execute_action( + user.identity, membership_request.id, "cancel" + ).to_dict() + ArchivedInvitation.index.refresh() # switch name? needed? + + # 1 "request", the cancelled membership request + res = member_service.search_membership_requests( + owner.identity, community._record.id + ) + hits = res.to_dict()["hits"] + assert 1 == hits["total"] + hit = hits["hits"][0] + assert "cancelled" == hit["request"]["status"] + assert hit["request"]["is_open"] is False + + # Should be possible to create a new one again + member_service.request_membership( + user.identity, + community._record.id, + {"message": "Oops didn't mean to cancel. Oh well, I will request again."}, + ) + + +def test_request_membership_decline_flow( + create_user, + community, + member_service, + requests_service, + owner, + db, + clean_index, +): + # Create membership request + user = create_user() + data = {"message": "Can I join the club?"} + community_uuid = community._record.id + membership_request = member_service.request_membership( + user.identity, + community_uuid, + data, + ) + Member.index.refresh() + + # Preconditions + # 1 member, the owner + res = member_service.search(owner.identity, community_uuid) + assert 1 == res.to_dict()["hits"]["total"] + # 1 "request", the membership request + res = member_service.search_membership_requests(owner.identity, community_uuid) + assert 1 == res.to_dict()["hits"]["total"] + + # Decline request + request = requests_service.execute_action( + owner.identity, membership_request.id, "decline" + ).to_dict() + ArchivedInvitation.index.refresh() # switch name? + Member.index.refresh() + + # Postconditions + # 1 member, the owner + res = member_service.search(owner.identity, community_uuid) + assert 1 == res.to_dict()["hits"]["total"] + # 1 "request", the declined membership_request + res = member_service.search_membership_requests(owner.identity, community_uuid) + hits = res.to_dict()["hits"] + assert 1 == hits["total"] + hit = hits["hits"][0] + assert "declined" == hit["request"]["status"] + assert hit["request"]["is_open"] is False + + +def test_request_membership_accept_flow( + create_user, + community, + member_service, + requests_service, + owner, + invite_user, + invite_request_id, + db, + clean_index, +): + # Create membership request + user = create_user() + data = {"message": "Can I join the club?"} + community_uuid = community._record.id + membership_request = member_service.request_membership( + user.identity, + community_uuid, + data, + ) + Member.index.refresh() + + # Accept request + request = requests_service.execute_action( + owner.identity, membership_request.id, "accept" + ).to_dict() + ArchivedInvitation.index.refresh() # switch name? + Member.index.refresh() + + # Postconditions + # 2 members, the owner + requester + res = member_service.search(owner.identity, community_uuid) + assert 2 == res.to_dict()["hits"]["total"] + + # 1 "request", the accepted one + res = member_service.search_membership_requests(owner.identity, community_uuid) + hits = res.to_dict()["hits"] + assert 1 == hits["total"] + hit = hits["hits"][0] + assert "accepted" == hit["request"]["status"] + assert hit["request"]["is_open"] is False #