button for membership discussion part reviving membership#1353
button for membership discussion part reviving membership#1353fenekku wants to merge 4 commits intoinveniosoftware:masterfrom
Conversation
| @property | ||
| def member_policy_is_open(self): | ||
| """Return true when member policy is open.""" | ||
| return self.member_policy == MemberPolicyEnum.OPEN.value |
There was a problem hiding this comment.
so that clients of this code don't need to know about MemberPolicyEnum.Open.value
| if self._errors: | ||
| res["errors"] = self._errors | ||
| return res | ||
| Kept for legacy reasons and availability in the future. Would be happy |
There was a problem hiding this comment.
Would honestly be happy to remove CommunityItem in its entirety (we are already in a breaking change and non-adopted version).
To be clear the removed code was removed because completely redundant. The parent class already implements the functionality.
|
|
||
| class ArchivedInvitation(Record, MemberMixin): | ||
| """An archived invitation record. | ||
| """An archived invitation or membership request record. |
There was a problem hiding this comment.
may look into ramifications of changing that name in future PRs.
| {% if request_id_of_pending_member %} | ||
| <a href="{{ url_for('invenio_app_rdm_requests.user_dashboard_request_view', request_pid_value=request_id_of_pending_member) }}" | ||
| class="ui button labeled icon rel-mt-1 theme-secondary"> | ||
| <i class="sign-in icon" aria-hidden="true"></i> | ||
| {{ _("Membership discussion") }} | ||
| </a> |
There was a problem hiding this comment.
To note: this is creating a button for invitation discussion even if membership feature itself is not enabled.
There was a problem hiding this comment.
The button should appear only if the membership feature is enabled.
I wonder if we should have a feature flag too, which disables entirely this feature, based on my comment above.
There was a problem hiding this comment.
I changed it to only display if the feature was enabled at site + community levels. If those are enabled, that button would show up if there is a membership request or an invitation pending. It's a better UX in my opinion to include invitation because someone can't request to join a community if they have already been invited to it.
| @blueprint.app_context_processor | ||
| def processor_for_member_button(): |
There was a problem hiding this comment.
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)
| else_value="open", | ||
| then_=then_, | ||
| else_=else_, | ||
| ) |
There was a problem hiding this comment.
Replaced by below. This was unreadable when I returned to it.
| needs = [CommunityRoleNeed(community_id, role) for role in roles] | ||
| return needs | ||
| return [] | ||
| return [CommunityRoleNeed(community_id, r) for r in self.roles(**kwargs) or []] |
There was a problem hiding this comment.
just making more readable in passing.
| else_=[SystemProcess()], | ||
| ), | ||
| IfCommunityAllowsMembershipRequests( | ||
| then_=[AuthenticatedUserButNotCommunityMember()], else_=[Disable()] |
There was a problem hiding this comment.
@ntarocco This used to be Disable() but was set to SystemProcess() in d3daea2 . What was the context for that change? I am setting it to Disable() again because of the explanation above. To add to that explanation: it's both surprising for a superuser to encounter the membership button and other related functionality when the feature is off and surprising for a non-superuser manager of a community to then receive a membership request when their community doesn't allow it (a superuser or systemprocess sending one).
There was a problem hiding this comment.
It was done because otherwise no CLI/programmatic script can use it, which they normally act with a system user/process.
I agree with you that superusers will see it and it will be confusing. At the same time, we need to allow a system user/process to perform changes. This is useful when running scripts to update things, for example. It might not be needed in this context.
My change was a bulk update Disable -> SystemProcess.
There was a problem hiding this comment.
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 Disable().
| 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"] |
There was a problem hiding this comment.
just more accurate test (don't really care about the title)
…ture enabled [+] - also optimizes for reduced chance of hitting the DB
2 participants
I am reviving the feature for a user to be able to request to become part of a community.
The (draft) RFC describing this feature is here: inveniosoftware/rfcs#111
The rotting corpse of the former PR is here: #1175 .
This PR is just the first in a series — I will submit smaller PRs than a 49 files one, but more of them.
This PR covers the part of the feature that shows the "Request membership" button or "Membership discussion" button on the community header ribbon when feature is enabled.