-
Notifications
You must be signed in to change notification settings - Fork 81
button for membership discussion part reviving membership #1353
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
7676b0b
7bf88a6
3e0edf8
6584f15
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so that clients of this code don't need to know about |
||
|
|
||
| @property | ||
| def record_submission_policy(self): | ||
| """Get the record submission policy level.""" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would honestly be happy to remove To be clear the removed code was removed because completely redundant. The parent class already implements the functionality. |
||
| to remove though. | ||
| """ | ||
|
|
||
|
|
||
| class FeaturedCommunityItem(CommunityItem): | ||
| """Single request result.""" | ||
| """Single Featured Community result.""" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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_, | ||
| ) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replaced by below. This was unreadable when I returned to it. |
||
|
|
||
|
|
||
| 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 []] | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just making more readable in passing. |
||
|
|
||
| def query_filter(self, identity=None, **kwargs): | ||
| """Filters for current identity as owner.""" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. may look into ramifications of changing that name in future PRs. |
||
|
|
||
| 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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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()] | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ntarocco This used to be
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was done because otherwise no CLI/programmatic script can use it, which they normally act with a system user/process. My change was a bulk update
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, this is a case of not needed in this context (programmatic request to join only makes sense if the feature is enabled and scripts can always workaround anyway). I've kept the |
||
| ) | ||
| ] | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -259,4 +259,17 @@ def resolve_community_logo(logo_link, community_id): | |
|
|
||
| return logo_link | ||
|
|
||
| @blueprint.app_context_processor | ||
| def processor_for_member_button(): | ||
|
Comment on lines
+262
to
+263
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is so that the community records page also gets the request id without changing its view (and having breakage in between change). Consequences of having the community's records page (aka detail page) defined in invenio-app-rdm. (case of "polluting" the global processor context for something that could have been scoped) |
||
| """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 | ||
Uh oh!
There was an error while loading. Please reload this page.