diff --git a/invenio_communities/communities/records/systemfields/access.py b/invenio_communities/communities/records/systemfields/access.py index 14a5315fb..f00de36d2 100644 --- a/invenio_communities/communities/records/systemfields/access.py +++ b/invenio_communities/communities/records/systemfields/access.py @@ -91,7 +91,7 @@ def __init__( """ self.visibility = visibility or VisibilityEnum.PUBLIC self.members_visibility = members_visibility or MembersVisibilityEnum.PUBLIC - self.member_policy = member_policy or MemberPolicyEnum.OPEN + self.member_policy = member_policy or MemberPolicyEnum.CLOSED self.record_submission_policy = ( record_submission_policy or current_app.config["COMMUNITIES_DEFAULT_RECORD_SUBMISSION_POLICY"] @@ -170,6 +170,11 @@ def member_policy(self, value): raise ValueError(f"Unknown member policy level: {value}") self._member_policy = value + @property + def member_policy_is_open(self): + """Return true when member policy is open.""" + return self.member_policy == MemberPolicyEnum.OPEN.value + @property def record_submission_policy(self): """Get the record submission policy level.""" diff --git a/invenio_communities/communities/services/results.py b/invenio_communities/communities/services/results.py index c0cfd8e01..475a4e393 100644 --- a/invenio_communities/communities/services/results.py +++ b/invenio_communities/communities/services/results.py @@ -105,20 +105,12 @@ def hits(self): class CommunityItem(RecordItem): - """Single request result.""" + """Single Community result. - @property - def links(self): - """Get links for this result item.""" - return self._links_tpl.expand(self._identity, self._record) - - def to_dict(self): - """Get a dictionary for the community.""" - res = self.data - if self._errors: - res["errors"] = self._errors - return res + Kept for legacy reasons and availability in the future. Would be happy + to remove though. + """ class FeaturedCommunityItem(CommunityItem): - """Single request result.""" + """Single Featured Community result.""" diff --git a/invenio_communities/generators.py b/invenio_communities/generators.py index 6d5cb9031..bfca09cd9 100644 --- a/invenio_communities/generators.py +++ b/invenio_communities/generators.py @@ -4,7 +4,7 @@ # Copyright (C) 2016-2024 CERN. # Copyright (C) 2021 Graz University of Technology. # Copyright (C) 2021 TU Wien. -# Copyright (C) 2022 Northwestern University. +# Copyright (C) 2022-2026 Northwestern University. # # Invenio is free software; you can redistribute it and/or modify it # under the terms of the MIT License; see LICENSE file for more details. @@ -16,10 +16,11 @@ from functools import partial, reduce from itertools import chain +from flask import current_app from flask_principal import UserNeed from invenio_access.permissions import any_user, authenticated_user, system_process from invenio_records.dictutils import dict_lookup -from invenio_records_permissions.generators import Generator +from invenio_records_permissions.generators import ConditionalGenerator, Generator from invenio_search.engine import dsl from .communities.records.systemfields.deletion_status import ( @@ -186,26 +187,6 @@ def __init__(self, then_, else_): ) -class IfMemberPolicyClosed(IfRestrictedBase): - """If member policy is closed.""" - - def __init__(self, then_, else_): - """Initialize.""" - field = "member_policy" - super().__init__( - field_getter=lambda r: ( - getattr(r.access, field, None) - if hasattr(r, "access") - else r.get("access", {}).get(field) - ), # needed for running permission check at serialization time and avoid db query - field_name=f"access.{field}", - then_value="closed", - else_value="open", - then_=then_, - else_=else_, - ) - - class IfCommunityDeleted(Generator): """Conditional generator for deleted communities.""" @@ -263,11 +244,29 @@ def query_filter(self, **kwargs): # -class AuthenticatedButNotCommunityMembers(Generator): +class IfCommunityAllowsMembershipRequests(ConditionalGenerator): + """If community allows membership requests.""" + + def _condition(self, record=None, **kwargs): + """Condition to choose generators. + + :param record: api.Community + """ + config_enabled = bool( + current_app.config.get("COMMUNITIES_ALLOW_MEMBERSHIP_REQUESTS") + ) + setting_enabled = record and record.access.member_policy_is_open + return config_enabled and setting_enabled + + +class AuthenticatedUserButNotCommunityMember(Generator): """Authenticated user not part of community.""" def needs(self, record=None, **kwargs): - """Required needs.""" + """Required needs. + + :param record: api.Community + """ return [authenticated_user] def excludes(self, record=None, **kwargs): @@ -276,6 +275,9 @@ def excludes(self, record=None, **kwargs): Excludes identities with a role in the community. This assumes all roles at this point mean valid memberships. This is the same assumption as `CommunityMembers` below. + + :param record: api.Community + """ if not record: return [] @@ -302,11 +304,7 @@ def needs(self, record=None, community_id=None, **kwargs): assert community_id, "No community id provided." community_id = str(community_id) - roles = self.roles(**kwargs) - if roles: - needs = [CommunityRoleNeed(community_id, role) for role in roles] - return needs - return [] + return [CommunityRoleNeed(community_id, r) for r in self.roles(**kwargs) or []] def query_filter(self, identity=None, **kwargs): """Filters for current identity as owner.""" diff --git a/invenio_communities/members/records/api.py b/invenio_communities/members/records/api.py index 5bf3ac28b..b6a4d1212 100644 --- a/invenio_communities/members/records/api.py +++ b/invenio_communities/members/records/api.py @@ -19,7 +19,7 @@ from invenio_records_resources.records.systemfields import IndexField from invenio_requests.records.api import Request from invenio_users_resources.records.api import GroupAggregate, UserAggregate -from sqlalchemy import or_ +from sqlalchemy import or_, select from ..errors import InvalidMemberError from .models import ArchivedInvitationModel, MemberModel @@ -43,10 +43,10 @@ class MemberMixin: """The data-layer id of the user (or None).""" group_id = ModelField("group_id") - """The data-layer id of the user (or None).""" + """The data-layer id of the group (or None).""" request_id = ModelField("request_id") - """The data-layer id of the user (or None).""" + """The data-layer id of the request (or None).""" role = ModelField("role") """The role of the entity.""" @@ -160,6 +160,25 @@ def has_members(cls, community_id, role=None): """Get members of a community.""" return cls.model_cls.count_members(community_id, role=role) + @classmethod + def get_pending_request_id(cls, user_id, community_id): + """ + Return request id of invitation/membership request pending for user+community. + + :param cls: Description + :param user_id: Description + :param community_id: Description + :return: UUID: id of active request associated with user and community + """ + stmt = ( + select(cls.model_cls.request_id) + .where(cls.model_cls.user_id == user_id) + .where(cls.model_cls.community_id == community_id) + .where(cls.model_cls.active == False) + ) + request_id = db.session.scalars(stmt).one_or_none() + return request_id + class Member(Record, MemberMixin): """A member/invitation record. @@ -187,7 +206,10 @@ class Member(Record, MemberMixin): class ArchivedInvitation(Record, MemberMixin): - """An archived invitation record. + """An archived invitation or membership request record. + + The name is a legacy prior to membership request. Since we don't want to rename the + DB model or the document engine index, it is kept. 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/services/service.py b/invenio_communities/members/services/service.py index a2a664615..7b608943a 100644 --- a/invenio_communities/members/services/service.py +++ b/invenio_communities/members/services/service.py @@ -853,3 +853,14 @@ def close_membership_request(self, identity, request_id, uow=None): 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_request_id_of_pending_member(self, identity, community_id): + """ + Return request id of invitation/membership request pending for user+community. + + :param cls: Description + :param user_id: Description + :param community_id: Description + :return: UUID: id of active request associated with user and community + """ + return self.record_cls.get_pending_request_id(identity.id, community_id) diff --git a/invenio_communities/permissions.py b/invenio_communities/permissions.py index b4f71b409..de2fd95f7 100644 --- a/invenio_communities/permissions.py +++ b/invenio_communities/permissions.py @@ -25,15 +25,15 @@ from .generators import ( AllowedMemberTypes, - AuthenticatedButNotCommunityMembers, + AuthenticatedUserButNotCommunityMember, CommunityCurators, CommunityManagers, CommunityManagersForRole, CommunityMembers, CommunityOwners, CommunitySelfMember, + IfCommunityAllowsMembershipRequests, IfCommunityDeleted, - IfMemberPolicyClosed, IfRecordSubmissionPolicyClosed, IfRestricted, ReviewPolicy, @@ -187,19 +187,13 @@ class CommunityPermissionPolicy(BasePermissionPolicy): can_manage_parent = [Administration(), SystemProcess()] # request_membership permission is based on configuration, community settings and - # identity. Other factors (e.g., previous membership requests) are not under - # its purview and are dealt with elsewhere. + # identity. At end of the day, it's convenient for this permission to answer + # if allowed by also checking if possible at all. In other words, even a superuser + # is disallowed if the app config or community setting turned the feature off. can_request_membership = [ - IfConfig( - "COMMUNITIES_ALLOW_MEMBERSHIP_REQUESTS", - then_=[ - IfMemberPolicyClosed( - then_=[SystemProcess()], - else_=[AuthenticatedButNotCommunityMembers()], - ), - ], - else_=[SystemProcess()], - ), + IfCommunityAllowsMembershipRequests( + then_=[AuthenticatedUserButNotCommunityMember()], else_=[Disable()] + ) ] diff --git a/invenio_communities/templates/semantic-ui/invenio_communities/details/header.html b/invenio_communities/templates/semantic-ui/invenio_communities/details/header.html index 3c3ef3478..a719a31f9 100644 --- a/invenio_communities/templates/semantic-ui/invenio_communities/details/header.html +++ b/invenio_communities/templates/semantic-ui/invenio_communities/details/header.html @@ -2,7 +2,7 @@ This file is part of Invenio. Copyright (C) 2016-2024 CERN. - Copyright (C) 2024 Northwestern University. + Copyright (C) 2024-2026 Northwestern University. Copyright (C) 2025 Graz University of Technology. Invenio is free software; you can redistribute it and/or modify it @@ -12,16 +12,38 @@ {%- from "invenio_theme/macros/truncate.html" import truncate_text %} {%- from "invenio_communities/details/macros/access-status-label.html" import access_status_label -%} -{% macro button_to_request_membership(community_ui) %} - {% if permissions.can_request_membership %} - {# TODO: Add relation_to_community for other flows #} -
-
+ +{% macro button_for_membership(community_ui) %} + {# Macro to display the button related to membership (request or discussion) #} + + {# Ths conditionals try hard to reduce the need for a DB hit #} + {% if config.COMMUNITIES_ALLOW_MEMBERSHIP_REQUESTS %} + {% if community_ui["access"]["member_policy"] == "open" %} + {# The below permission check includes the above checks among other checks, so + it was not enough on its own to prevent a large swath of cases where the DB + could be hit. + #} + {% if permissions.can_request_membership %} +
+
+ {% else %} + {% set request_id_of_pending_member = get_request_id_of_pending_member(g.identity, community_ui["id"]) %} + {# None if no pending membership or invitation request #} + {% if request_id_of_pending_member %} + + + {{ _("Membership discussion") }} + + {% endif %} + {% endif %} + {% endif %} {% endif %} + {% endmacro %} {% macro community_title(community_ui) %} @@ -113,8 +135,7 @@
- {# Button to request membership is fully disabled until feature completely merged in v14. #} - {# {{ button_to_request_membership(community) }} #} + {{ button_for_membership(community_ui) }} {%- if not community_use_jinja_header %} {%- if not permissions.can_submit_record %}
diff --git a/invenio_communities/views/ui.py b/invenio_communities/views/ui.py index b9d57403d..2cc8e183a 100644 --- a/invenio_communities/views/ui.py +++ b/invenio_communities/views/ui.py @@ -259,4 +259,17 @@ def resolve_community_logo(logo_link, community_id): return logo_link + @blueprint.app_context_processor + def processor_for_member_button(): + """Processor for member button related function. + + It unfortunately has to be an app_context_processor simply because a community's + records page is not under this blueprint. + """ + return { + "get_request_id_of_pending_member": ( + current_communities.service.members.get_request_id_of_pending_member + ) + } + return blueprint diff --git a/tests/communities/test_permissions.py b/tests/communities/test_permissions.py index 9bc96d177..b4f446053 100644 --- a/tests/communities/test_permissions.py +++ b/tests/communities/test_permissions.py @@ -1,6 +1,6 @@ # -*- coding: utf-8 -*- # -# Copyright (C) 2024 Northwestern University. +# Copyright (C) 2024-2026 Northwestern University. # # Invenio-RDM-Records is free software; you can redistribute it # and/or modify it under the terms of the MIT License; see LICENSE file for @@ -12,61 +12,63 @@ def test_can_request_membership( - app, community, owner, anon_identity, any_user, superuser_identity + app, + community, + community_open_to_membership_requests, + owner, + anon_identity, + any_user, + superuser_identity, + set_app_config_fn_scoped, ): policy = CommunityPermissionPolicy - community_record = community._record - authenticated_identity = any_user.identity + community_closed_record = community._record + community_open_record = community_open_to_membership_requests._record + identity_authenticated = any_user.identity + identity_superuser = superuser_identity # just to parallel naming + identity_anonymous = anon_identity # just to parallel naming + set_app_config_fn_scoped({"COMMUNITIES_ALLOW_MEMBERSHIP_REQUESTS": False}) + # subsequent config changes can be done directly, knowing they will all be + # roll-backed correctly - allow_membership_requests_orig = app.config["COMMUNITIES_ALLOW_MEMBERSHIP_REQUESTS"] - - # Case - feature disabled - app.config["COMMUNITIES_ALLOW_MEMBERSHIP_REQUESTS"] = False - # superuser can - assert policy(action="request_membership", record=community_record).allows( - superuser_identity + # Case: config disabled - nobody can request membership + assert not ( + policy(action="request_membership", record=community_open_record).allows( + identity_authenticated + ) ) - # authenticated cannot assert not ( - policy(action="request_membership", record=community_record).allows( - authenticated_identity + policy(action="request_membership", record=community_open_record).allows( + identity_superuser ) ) + # Case: config enabled but setting disabled - nobody can request membership app.config["COMMUNITIES_ALLOW_MEMBERSHIP_REQUESTS"] = True - - # Case - setting disabled - community_record.access.member_policy = "closed" - # superuser can - assert policy(action="request_membership", record=community_record).allows( - superuser_identity - ) - # authenticated cannot assert not ( - policy(action="request_membership", record=community_record).allows( - authenticated_identity + policy(action="request_membership", record=community_closed_record).allows( + identity_authenticated ) ) - - community_record.access.member_policy = "open" - - # Case - unlogged user assert not ( - policy(action="request_membership", record=community_record).allows( - anon_identity + policy(action="request_membership", record=community_closed_record).allows( + identity_superuser ) ) - # Case - logged user not part of community - assert policy(action="request_membership", record=community_record).allows( - authenticated_identity + # Case - config enabled, setting enabled - unlogged user can't request + assert not ( + policy(action="request_membership", record=community_open_record).allows( + identity_anonymous + ) ) - - # Case - member of community + # Case - config enabled, setting enabled - already member of community can't request assert not ( - policy(action="request_membership", record=community_record).allows( + policy(action="request_membership", record=community_open_record).allows( owner.identity ) ) - - app.config["COMMUNITIES_ALLOW_MEMBERSHIP_REQUESTS"] = allow_membership_requests_orig + # Case - config enabled, setting enabled - authenticated but not member of community + assert policy(action="request_membership", record=community_open_record).allows( + identity_authenticated + ) diff --git a/tests/conftest.py b/tests/conftest.py index f78066f3d..5661e5f93 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -530,6 +530,21 @@ def restricted_members_community(community_service, owner, minimal_community, lo return c +@pytest.fixture(scope="module") +def community_open_to_membership_requests( + community_service, owner, minimal_community, location +): + """A community open to membership requests.""" + data = deepcopy(minimal_community) + data["access"]["member_policy"] = "open" + data["metadata"]["title"] = "Community open to membership requests" + data["slug"] = "community-open-to-membership-requests" + c = community_service.create(owner.identity, data) + Community.index.refresh() + owner.refresh() + return c + + @pytest.fixture(scope="function") def fake_communities( community_service, diff --git a/tests/members/test_members_resource.py b/tests/members/test_members_resource.py index c3094d2ae..a64178352 100644 --- a/tests/members/test_members_resource.py +++ b/tests/members/test_members_resource.py @@ -364,9 +364,12 @@ def test_search_invitation( # -def test_post_membership_requests(app, client, headers, community_id, create_user, db): +def test_post_membership_requests( + app, client, headers, community_open_to_membership_requests, create_user, db +): user = create_user({"email": "user_foo@example.org", "username": "user_foo"}) client = user.login(client) + community_id = str(community_open_to_membership_requests._record.id) # Post membership request r = client.post( @@ -388,7 +391,7 @@ def test_post_membership_requests(app, client, headers, community_id, create_use # Check the request r = client.get(url_of_request, headers=headers) assert 200 == r.status_code - assert 'Request to join "My Community"' in r.json["title"] + assert community_id == r.json["receiver"]["community"] # Check the timeline r = client.get(url_of_timeline, headers=headers) diff --git a/tests/members/test_members_services.py b/tests/members/test_members_services.py index 044a80b75..417e3832a 100644 --- a/tests/members/test_members_services.py +++ b/tests/members/test_members_services.py @@ -14,7 +14,7 @@ from invenio_accounts.proxies import current_datastore from invenio_cache import current_cache from invenio_records_resources.services.errors import PermissionDeniedError -from invenio_requests.records.api import RequestEvent +from invenio_requests.records.api import Request, RequestEvent from marshmallow import ValidationError from invenio_communities.members.errors import AlreadyMemberError, InvalidMemberError @@ -1162,9 +1162,74 @@ def test_update_invalid_data(member_service, community, group): # +def test_get_request_id_of_pending_member( + member_service, + community_open_to_membership_requests, + anon_identity, + owner, + create_user, + requests_service, + db, + clean_index, +): + user = create_user() + community = community_open_to_membership_requests + + # Case: anonymous identity - return None + request_id = member_service.get_request_id_of_pending_member( + anon_identity, community._record.id + ) + assert request_id is None + + # Case: no pending request - return None + request_id = member_service.get_request_id_of_pending_member( + user.identity, community._record.id + ) + assert request_id is None + + # Case: pending membership request - return request id + membership_request = member_service.request_membership( + user.identity, + community._record.id, + {"message": "Can I join the club?"}, + ) + request_id = member_service.get_request_id_of_pending_member( + user.identity, community._record.id + ) + assert membership_request.id == str(request_id) + + # Case (sanity check): pending invitation request - return request id + requests_service.execute_action(user.identity, membership_request.id, "cancel") + data = { + "members": [{"type": "user", "id": str(user.id)}], + "role": "reader", + "message": "Welcome to the club!", + } + member_service.invite(owner.identity, community._record.id, data) + # get invitation_request_id + Request.index.refresh() + results = requests_service.search( + user.identity, + receiver={"user": user.id}, + type="community-invitation", + ).to_dict() + invitation_request_id = results["hits"]["hits"][0]["id"] + request_id = member_service.get_request_id_of_pending_member( + user.identity, community._record.id + ) + assert invitation_request_id == str(request_id) + + # Case: membership established (associated request id but not pending) - return None + requests_service.execute_action(user.identity, invitation_request_id, "accept") + request_id = member_service.get_request_id_of_pending_member( + user.identity, community._record.id + ) + assert request_id is None + + def test_request_cancel_request_flow( member_service, - community, + community_open_to_membership_requests, create_user, requests_service, db, @@ -1176,6 +1241,7 @@ def test_request_cancel_request_flow( """ # Create membership request user = create_user() + community = community_open_to_membership_requests data = { "message": "Can I join the club?", }