membership-request [inveniosoftware#855]: address TODOs

Author: fenekku

Diff for: invenio_communities/assets/semantic-ui/js/invenio_communities/members/membership_requests/MembershipRequestsEmptyResults.js

@@ -13,7 +13,7 @@ class MembershipRequestsEmptyResultsCmp extends Component {
         <Segment placeholder textAlign="center">
           <Header icon>
             <Icon name="search" />
-            {i18next.t("No matching members found.")}
+            {i18next.t("No matching membership requests found.")}
           </Header>
           {queryString && (
             <p>

Diff for: invenio_communities/assets/semantic-ui/js/invenio_communities/members/membership_requests/MembershipRequestsResultItem.js

@@ -45,20 +45,12 @@ export class MembershipRequestsResultItem extends Component {
       membershipRequest: { member },
       membershipRequest,
     } = this.state;
-    console.log("********************");
-    console.log("membershipRequest");
-    console.dir(membershipRequest);
-    console.log("********************");
 
     const request = buildRequest(membershipRequest, ["accept", "decline"]);
-    console.log("********************");
-    console.log("request");
-    console.dir(request);
-    console.log("********************");
-
     const { api: membershipRequestsApi } = this.context;
     const roles = rolesCanAssign[member.type];
     const expiration = formattedTime(request.expires_at);
+
     return (
       <Table.Row className="community-member-item">
         <Table.Cell>
@@ -104,9 +96,10 @@ export class MembershipRequestsResultItem extends Component {
         </Table.Cell>
         <Table.Cell data-label={i18next.t("Actions")}>
           <RequestActionController
-            request={membershipRequest}
-            // TODO: Decision flow
-            actionSuccessCallback={() => console.log("actionSuccessCallback called")}
+            request={request}
+            actionSuccessCallback={() => {
+              window.location.reload();
+            }}
           />
         </Table.Cell>
       </Table.Row>

Diff for: 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 for: 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 for: 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 for: 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 for: 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 for: 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 for: 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
@@ -873,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(

Diff for: 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 for: 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": "[email protected]", "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