Repository: inveniosoftware/invenio-rdm-records (language: python)

Issue:
Add UserRecipientFilter to community inclusion notification builder
              Nice! I think this can be re-used also for the community record inclusion/review notifications for the cases where the request/record creator is also a community curator (to avoid sending a notification to themselves).

_Originally posted by @slint in https://github.com/inveniosoftware/invenio-requests/pull/322#discussion_r1272338507_
            

We are currently adding the request creator as a recipient for the notification ([see here](https://github.com/inveniosoftware/invenio-rdm-records/blob/1e29b995d3df0481c502945894c45be07cd6f8f3/invenio_rdm_records/notifications/builders.py#L44)). As the request creator will already know about the request, they can be removed as a recipient. Following this, we can also add a  [UserRecipientFilter](https://github.com/inveniosoftware/invenio-requests/blob/85421d554aeb28d419688703e2ccb85fbec1fc4c/invenio_requests/notifications/builders.py#L51) to the builder, so we filter the user, if they are a curator/owner of the community.


Discussion / hints:

Fix patch:
diff --git a/invenio_rdm_records/notifications/builders.py b/invenio_rdm_records/notifications/builders.py
index 50f9d2372..64624276f 100644
--- a/invenio_rdm_records/notifications/builders.py
+++ b/invenio_rdm_records/notifications/builders.py
@@ -13,8 +13,8 @@
 from invenio_notifications.registry import EntityResolverRegistry
 from invenio_notifications.services.builders import NotificationBuilder
 from invenio_notifications.services.generators import EntityResolve, UserEmailBackend
+from invenio_requests.notifications.filters import UserRecipientFilter
 from invenio_users_resources.notifications.filters import UserPreferencesRecipientFilter
-from invenio_users_resources.notifications.generators import UserRecipient
 
 
 class CommunityInclusionNotificationBuilder(NotificationBuilder):
@@ -41,11 +41,11 @@ def build(cls, request):
 
     recipients = [
         CommunityMembersRecipient(key="request.receiver", roles=["curator", "owner"]),
-        UserRecipient(key="request.created_by"),
     ]
 
     recipient_filters = [
         UserPreferencesRecipientFilter(),
+        UserRecipientFilter(key="request.created_by"),
     ]
 
     recipient_backends = [
diff --git a/invenio_rdm_records/templates/semantic-ui/invenio_notifications/community-submission.submit.jinja b/invenio_rdm_records/templates/semantic-ui/invenio_notifications/community-submission.submit.jinja
index abad9a61f..26762fb98 100644
--- a/invenio_rdm_records/templates/semantic-ui/invenio_notifications/community-submission.submit.jinja
+++ b/invenio_rdm_records/templates/semantic-ui/invenio_notifications/community-submission.submit.jinja
@@ -15,24 +15,26 @@
 %}
 
 {%- block subject -%}
-    New record submission for your community {{ community_title }} submitted by {{ creator_name }}
+   {{ _("New record submission for your community '{community_title}' submitted by '{creator_name}'").format(community_title=community_title, creator_name=creator_name) }}
 {%- endblock subject -%}
 
 {%- block html_body -%}
-<p>The record "{{ record_title }}" was submitted to your community "{{ community_title }}" by "{{ creator_name }}".</p>
+<p>
+    {{ _("The record '{record_title}' was submitted to your community '{community_title}' by '{creator_name}'.").format(record_title=record_title, community_title=community_title, creator_name=creator_name) }}
+</p>
 
-<a href="{{ request_link }}" class="button">Review the submission_request</a>
+<a href="{{ request_link }}" class="button"> {{ _("Review the submission request") }}</a>
 {%- endblock html_body -%}
 
 {%- block plain_body -%}
-The record "{{ record_title }}" was submitted to your community "{{ community_title }}" by "{{ creator_name }}".
+    {{ _("The record '{record_title}' was submitted to your community '{community_title}' by '{creator_name}'.").format(record_title=record_title, community_title=community_title, creator_name=creator_name) }}
 
-Review the submission request: {{ request_link }}
+    {{ _("Review the submission request: {request_link}").format(request_link=request_link) }}
 {%- endblock plain_body -%}
 
 {# Markdown for Slack/Mattermost/chat #}
 {%- block md_body -%}
-The record *{{ record_title }}* was submitted to your community *{{ community_title }}* by *{{ creator_name }}*.
+    {{ _("The record *{record_title}* was submitted to your community *{community_title}* by *{creator_name}*.").format(record_title=record_title, community_title=community_title, creator_name=creator_name) }}
 
-[Review the submission request]({{ request_link }})
+    [{{ _("Review the submission request") }}]({{ request_link }})
 {%- endblock md_body -%}


Test patch:
diff --git a/tests/conftest.py b/tests/conftest.py
index 6f00aa58c..fb17bcf89 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -3,7 +3,7 @@
 # Copyright (C) 2019-2022 CERN.
 # Copyright (C) 2019-2022 Northwestern University.
 # Copyright (C) 2021 TU Wien.
-# Copyright (C) 2022 Graz University of Technology.
+# Copyright (C) 2022-2023 Graz University of Technology.
 #
 # 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 more details.
@@ -42,7 +42,7 @@
 from dateutil import tz
 from flask import g
 from flask_principal import Identity, Need, UserNeed
-from flask_security import login_user, logout_user
+from flask_security import login_user
 from flask_security.utils import hash_password
 from invenio_access.models import ActionRoles
 from invenio_access.permissions import superuser_access, system_identity
@@ -53,6 +53,9 @@
 from invenio_cache import current_cache
 from invenio_communities import current_communities
 from invenio_communities.communities.records.api import Community
+from invenio_communities.notifications.builders import (
+    CommunityInvitationSubmittedNotificationBuilder,
+)
 from invenio_notifications.backends import EmailNotificationBackend
 from invenio_notifications.services.builders import NotificationBuilder
 from invenio_oauth2server.models import Client
@@ -60,6 +63,10 @@
 from invenio_records_resources.proxies import current_service_registry
 from invenio_records_resources.references.entity_resolvers import ServiceResultResolver
 from invenio_records_resources.services.custom_fields import TextCF
+from invenio_requests.notifications.builders import (
+    CommentRequestEventCreateNotificationBuilder,
+)
+from invenio_users_resources.proxies import current_users_service
 from invenio_users_resources.services.schemas import (
     NotificationPreferences,
     UserPreferencesSchema,
@@ -89,129 +96,6 @@
 from .fake_datacite_client import FakeDataCiteClient
 
 
-#
-# User fixture helper - move to pytest-invenio
-#
-#
-# Helper
-#
-class UserFixture_:
-    """A user fixture for easy test user creation."""
-
-    def __init__(self, email=None, password=None, active=True):
-        """Constructor."""
-        self._email = email
-        self._active = active
-        self._password = password
-        self._identity = None
-        self._user = None
-        self._client = None
-
-    #
-    # Creation
-    #
-    def create(self, app, db):
-        """Create the user."""
-        with db.session.begin_nested():
-            datastore = app.extensions["security"].datastore
-            user = datastore.create_user(
-                email=self.email,
-                password=hash_password(self.password),
-                active=self._active,
-            )
-        db.session.commit()
-        self._user = user
-        return self
-
-    #
-    # Properties
-    #
-    @property
-    def user(self):
-        """Get the user."""
-        return self._user
-
-    @property
-    def id(self):
-        """Get the user id as a string."""
-        return str(self._user.id)
-
-    @property
-    def email(self):
-        """Get the user."""
-        return self._email
-
-    @property
-    def password(self):
-        """Get the user."""
-        return self._password
-
-    #
-    # App context helpers
-    #
-    @property
-    def identity(self):
-        """Create identity for the user."""
-        if self._identity is None:
-            # Simulate a full login
-            assert login_user(self.user)
-            self._identity = deepcopy(g.identity)
-            # Clean up - we just want the identity object.
-            logout_user()
-        return self._identity
-
-    @identity.deleter
-    def identity(self):
-        """Delete the user."""
-        self._identity = None
-
-    def app_login(self):
-        """Create identity for the user."""
-        assert login_user(self.user)
-
-    def app_logout(self):
-        """Create identity for the user."""
-        assert logout_user()
-
-    #
-    # Test client helpers
-    #
-    def login(self, client, logout_first=False):
-        """Login the given client."""
-        return self._login(client, "/", logout_first)
-
-    def api_login(self, client, logout_first=False):
-        """Login the given client."""
-        return self._login(client, "/api/", logout_first)
-
-    def logout(self, client):
-        """Logout the given client."""
-        return self._logout(client, "/")
-
-    def api_logout(self, client):
-        """Logout the given client."""
-        return self._logout(client, "/api/")
-
-    def _login(self, client, base_path, logout):
-        """Login the given client."""
-        if logout:
-            self._logout(client, base_path)
-        res = client.post(
-            f"{base_path}login",
-            data=dict(email=self.email, password=self.password),
-            environ_base={"REMOTE_ADDR": "127.0.0.1"},
-            follow_redirects=True,
-        )
-        assert res.status_code == 200
-        return client
-
-    def _logout(self, client, base_path):
-        """Logout the client."""
-        res = client.get(f"{base_path}logout")
-        assert res.status_code < 400
-        return client
-
-
 class UserPreferencesNotificationsSchema(UserPreferencesSchema):
     """Schema extending preferences with notification preferences for model validation."""
 
@@ -236,12 +120,6 @@ def build(cls, **kwargs):
         return {}
 
 
-@pytest.fixture(scope="session")
-def UserFixture():
-    """Class to create user fixtures from."""
-    return UserFixture_
-
-
 @pytest.fixture(scope="module")
 def celery_config():
     """Override pytest-invenio fixture."""
@@ -395,7 +273,9 @@ def app_config(app_config, mock_datacite_client):
 
     # Specifying dummy builders to avoid raising errors for most tests. Extend as needed.
     app_config["NOTIFICATIONS_BUILDERS"] = {
+        CommentRequestEventCreateNotificationBuilder.type: DummyNotificationBuilder,
         CommunityInclusionSubmittedNotificationBuilder.type: DummyNotificationBuilder,
+        CommunityInvitationSubmittedNotificationBuilder.type: DummyNotificationBuilder,
     }
 
     # Specifying default resolvers. Will only be used in specific test cases.
@@ -471,7 +351,10 @@ def full_record(users):
                 "provider": "datacite",
                 "client": "inveniordm",
             },
-            "oai": {"identifier": "oai:vvv.com:abcde-fghij", "provider": "oai"},
+            "oai": {
+                "identifier": "oai:vvv.com:abcde-fghij",
+                "provider": "oai",
+            },
         },
         "uuid": "445aaacd-9de1-41ab-af52-25ab6cb93df7",
         "version_id": "1",
@@ -487,7 +370,10 @@ def full_record(users):
                         "given_name": "Lars Holm",
                         "family_name": "Nielsen",
                         "identifiers": [
-                            {"scheme": "orcid", "identifier": "0000-0001-8135-3489"}
+                            {
+                                "scheme": "orcid",
+                                "identifier": "0000-0001-8135-3489",
+                            }
                         ],
                     },
                     "affiliations": [{"id": "cern"}, {"name": "free-text"}],
@@ -515,7 +401,10 @@ def full_record(users):
                         "given_name": "Lars Holm",
                         "family_name": "Nielsen",
                         "identifiers": [
-                            {"scheme": "orcid", "identifier": "0000-0001-8135-3489"}
+                            {
+                                "scheme": "orcid",
+                                "identifier": "0000-0001-8135-3489",
+                            }
                         ],
                     },
                     "role": {"id": "other"},
@@ -523,7 +412,11 @@ def full_record(users):
                 }
             ],
             "dates": [
-                {"date": "1939/1945", "type": {"id": "other"}, "description": "A date"}
+                {
+                    "date": "1939/1945",
+                    "type": {"id": "other"},
+                    "description": "A date",
+                }
             ],
             "languages": [{"id": "dan"}, {"id": "eng"}],
             "identifiers": [{"identifier": "1924MNRAS..84..308E", "scheme": "bibcode"}],
@@ -658,7 +551,10 @@ def enhanced_full_record(users):
                         "given_name": "Lars Holm",
                         "family_name": "Nielsen",
                         "identifiers": [
-                            {"scheme": "orcid", "identifier": "0000-0001-8135-3489"}
+                            {
+                                "scheme": "orcid",
+                                "identifier": "0000-0001-8135-3489",
+                            }
                         ],
                     },
                     "affiliations": [{"id": "cern"}, {"name": "free-text"}],
@@ -709,7 +605,10 @@ def enhanced_full_record(users):
                         "given_name": "Lars Holm",
                         "family_name": "Nielsen",
                         "identifiers": [
-                            {"scheme": "orcid", "identifier": "0000-0001-8135-3489"}
+                            {
+                                "scheme": "orcid",
+                                "identifier": "0000-0001-8135-3489",
+                            }
                         ],
                     },
                     "role": {
@@ -972,7 +871,10 @@ def minimal_community2():
         "access": {
             "visibility": "public",
         },
-        "metadata": {"title": "Research Data Management", "type": {"id": "topic"}},
+        "metadata": {
+            "title": "Research Data Management",
+            "type": {"id": "topic"},
+        },
     }
 
 
@@ -1645,40 +1547,62 @@ def embargoed_record(running_app, minimal_record, superuser_identity):
 
 
 @pytest.fixture()
-def test_user(UserFixture, app, db):
+def test_user(UserFixture, app, db, index_users):
     """User meant to test permissions."""
     u = UserFixture(
         email="testuser@inveniosoftware.org",
         password="testuser",
     )
     u.create(app, db)
+    index_users()
     return u
 
 
 @pytest.fixture()
-def uploader(UserFixture, app, db):
+def uploader(UserFixture, app, db, index_users):
     """Uploader."""
     u = UserFixture(
         email="uploader@inveniosoftware.org",
         password="uploader",
+        preferences={
+            "visibility": "public",
+            "email_visibility": "restricted",
+            "notifications": {
+                "enabled": True,
+            },
+        },
+        active=True,
+        confirmed=True,
     )
     u.create(app, db)
+    index_users()
+
     return u
 
 
 @pytest.fixture()
-def community_owner(UserFixture, app, db):
+def community_owner(UserFixture, app, db, index_users):
     """Community owner."""
     u = UserFixture(
         email="community_owner@inveniosoftware.org",
         password="community_owner",
+        preferences={
+            "visibility": "public",
+            "email_visibility": "restricted",
+            "notifications": {
+                "enabled": True,
+            },
+        },
+        active=True,
+        confirmed=True,
     )
     u.create(app, db)
+    index_users()
     return u
 
 
 @pytest.fixture()
-def inviter():
+def inviter(index_users):
     """Add/invite a user to a community with a specific role."""
 
     def invite(user_id, community_id, role):
@@ -1697,6 +1621,7 @@ def invite(user_id, community_id, role):
         current_communities.service.members.add(
             system_identity, community_id, invitation_data
         )
+        index_users()
 
     return invite
 
@@ -1707,6 +1632,15 @@ def curator(UserFixture, community, inviter, app, db):
     curator = UserFixture(
         email="curatoruser@inveniosoftware.org",
         password="curatoruser",
+        preferences={
+            "visibility": "public",
+            "email_visibility": "restricted",
+            "notifications": {
+                "enabled": True,
+            },
+        },
+        active=True,
+        confirmed=True,
     )
     curator.create(app, db)
     inviter(curator.id, community.id, "curator")
@@ -1764,7 +1698,10 @@ def community2(running_app, community_type_record, community_owner, minimal_comm
 
 @pytest.fixture()
 def restricted_community(
-    running_app, community_type_record, community_owner, restricted_minimal_community
+    running_app,
+    community_type_record,
+    community_owner,
+    restricted_minimal_community,
 ):
     """Get the current RDM records service."""
     return _community_get_or_create(
@@ -1774,7 +1711,10 @@ def restricted_community(
 
 @pytest.fixture()
 def open_review_community(
-    running_app, community_type_record, community_owner, open_review_minimal_community
+    running_app,
+    community_type_record,
+    community_owner,
+    open_review_minimal_community,
 ):
     """Create community with open review policy i.e allow direct publishes."""
     return _community_get_or_create(
@@ -1784,7 +1724,10 @@ def open_review_community(
 
 @pytest.fixture()
 def closed_review_community(
-    running_app, community_type_record, community_owner, closed_review_minimal_community
+    running_app,
+    community_type_record,
+    community_owner,
+    closed_review_minimal_community,
 ):
     """Create community with close review policy i.e allow direct publishes."""
     return _community_get_or_create(
@@ -1800,7 +1743,10 @@ class Record:
         """Test record class."""
 
         def create_record(
-            self, record_dict=minimal_record, uploader=uploader, community=community
+            self,
+            record_dict=minimal_record,
+            uploader=uploader,
+            community=community,
         ):
             """Creates new record that belongs to the same community."""
             # create draft
@@ -1833,7 +1779,7 @@ def headers():
 
 
 @pytest.fixture()
-def admin(UserFixture, app, db, admin_role_need):
+def admin(UserFixture, app, db, admin_role_need, index_users):
     """Admin user for requests."""
     u = UserFixture(
         email="admin@inveniosoftware.org",
@@ -1846,6 +1792,7 @@ def admin(UserFixture, app, db, admin_role_need):
 
     datastore.add_role_to_user(u.user, role)
     db.session.commit()
+    index_users()
     return u
 
 
@@ -1877,3 +1824,14 @@ def oauth2_client(db, uploader):
         db.session.add(client_)
     db.session.commit()
     return client_.client_id
+
+
+@pytest.fixture()
+def index_users():
+    """Index users for an up-to-date user service."""
+
+    def _index():
+        current_users_service.indexer.process_bulk_queue()
+        current_users_service.record_cls.index.refresh()
+
+    return _index
diff --git a/tests/services/test_service_review.py b/tests/services/test_service_review.py
index 0f5d432d1..9603f8bf2 100644
--- a/tests/services/test_service_review.py
+++ b/tests/services/test_service_review.py
@@ -609,7 +609,14 @@ def test_review_gives_access_to_curator(running_app, draft, service, requests_se
 
 
 def test_review_notification(
-    draft, running_app, community, service, requests_service, monkeypatch
+    draft_for_open_review,
+    running_app,
+    open_review_community,
+    curator,
+    community_owner,
+    service,
+    inviter,
+    monkeypatch,
 ):
     """Test notifcation being built on review submit."""
 
@@ -629,8 +636,13 @@ def test_review_notification(
         },
     )
 
+    inviter(curator.id, open_review_community.id, "curator")
+
     # check draft status
-    assert draft["status"] == DraftStatus.review_to_draft_statuses["created"]
+    assert (
+        draft_for_open_review["status"]
+        == DraftStatus.review_to_draft_statuses["created"]
+    )
     assert not mock_build.called
 
     mail = running_app.app.extensions.get("mail")
@@ -638,13 +650,18 @@ def test_review_notification(
 
     with mail.record_messages() as outbox:
         # Validate that email was sent
-        req = service.review.submit(running_app.superuser_identity, draft.id).to_dict()
+        req = service.review.submit(
+            community_owner.identity, draft_for_open_review.id
+        ).to_dict()
         assert req["status"] == "submitted"
         # check notification is build on submit
         assert mock_build.called
         assert len(outbox) == 1
+        sent_mail = outbox[0]
         # TODO: update to `req["links"]["self_html"]` when addressing https://github.com/inveniosoftware/invenio-rdm-records/issues/1327
-        assert "/me/requests/{}".format(req["id"]) in outbox[0].html
+        assert "/me/requests/{}".format(req["id"]) in sent_mail.html
+        assert community_owner.email not in sent_mail.recipients
+        assert curator.email in sent_mail.recipients
 
 
 # TODO tests: