From 20869df9968065b51636334ff2df06922f7d69d4 Mon Sep 17 00:00:00 2001 From: "marcel.kocisek" Date: Thu, 20 Mar 2025 15:47:25 +0100 Subject: [PATCH 1/4] Fix access request emails: - introduce get_email_receivers method which will get users able to get mails about project --- server/mergin/sync/interfaces.py | 6 +++ server/mergin/sync/private_api_controller.py | 13 +------ server/mergin/sync/project_handler.py | 22 +++++++++++ server/mergin/tests/test_project_handler.py | 41 +++++++++++++++++++- server/mergin/tests/utils.py | 2 +- 5 files changed, 70 insertions(+), 14 deletions(-) diff --git a/server/mergin/sync/interfaces.py b/server/mergin/sync/interfaces.py index aa916b38..f57f103c 100644 --- a/server/mergin/sync/interfaces.py +++ b/server/mergin/sync/interfaces.py @@ -188,6 +188,12 @@ def get_push_permission(self, changes: dict): """ pass + def get_email_receivers(self, project_id: str): + """ + Return list of members who should receive email notifications about project changes + """ + pass + class WorkspaceRole(Enum): GUEST = "guest" diff --git a/server/mergin/sync/private_api_controller.py b/server/mergin/sync/private_api_controller.py index 00d0a13d..24b30596 100644 --- a/server/mergin/sync/private_api_controller.py +++ b/server/mergin/sync/private_api_controller.py @@ -17,8 +17,6 @@ AccessRequest, ProjectRole, RequestStatus, - ProjectVersion, - ProjectUser, ) from .schemas import ( ProjectListSchema, @@ -64,16 +62,7 @@ def create_project_access_request(namespace, project_name): # noqa: E501 db.session.add(access_request) db.session.commit() # notify project owners - owners = ( - User.query.join(UserProfile, ProjectUser) - .filter( - ProjectUser.project_id == project.id, - ProjectUser.role == ProjectRole.OWNER.value, - User.verified_email, - UserProfile.receive_notifications, - ) - .all() - ) + owners = current_app.project_handler.get_email_receivers(project.id) for owner in owners: email_data = { "subject": "Project access requested", diff --git a/server/mergin/sync/project_handler.py b/server/mergin/sync/project_handler.py index 8bf17af8..0e6eada5 100644 --- a/server/mergin/sync/project_handler.py +++ b/server/mergin/sync/project_handler.py @@ -1,7 +1,29 @@ +from .models import ProjectRole, ProjectUser from .interfaces import AbstractProjectHandler from .permissions import ProjectPermissions +from sqlalchemy import or_, and_ +from typing import List +from ..auth.models import User, UserProfile class ProjectHandler(AbstractProjectHandler): def get_push_permission(self, changes: dict): return ProjectPermissions.Upload + + def get_email_receivers(self, project_id: str) -> List[User]: + return ( + User.query.join(UserProfile) + .outerjoin(ProjectUser, ProjectUser.user_id == User.id) + .filter( + or_( + and_( + ProjectUser.project_id == project_id, + ProjectUser.role == ProjectRole.OWNER.value, + ), + User.is_admin, + ), + User.verified_email, + UserProfile.receive_notifications, + ) + .all() + ) diff --git a/server/mergin/tests/test_project_handler.py b/server/mergin/tests/test_project_handler.py index d44bc5f5..0b2ebe81 100644 --- a/server/mergin/tests/test_project_handler.py +++ b/server/mergin/tests/test_project_handler.py @@ -1,8 +1,47 @@ +from ..sync.models import Project, ProjectRole +from .utils import add_user, create_project, create_workspace from ..sync.project_handler import ProjectHandler from ..sync.permissions import ProjectPermissions +from ..auth.models import User +from ..app import db -def test_project_handler(): + +def test_project_permissions(client): project_handler = ProjectHandler() project_permission = project_handler.get_push_permission(None) assert project_permission == ProjectPermissions.Upload + + +def test_email_receivers(client): + project_handler = ProjectHandler() + # test project email receivers (owners and super admins) + workspace = create_workspace() + user = add_user() + project = create_project("test_project", workspace, user) + project.set_role(user.id, ProjectRole.READER) + db.session.commit() + receivers = project_handler.get_email_receivers(project.id) + assert len(receivers) == 1 + + project.set_role(user.id, ProjectRole.OWNER) + db.session.commit() + receivers = project_handler.get_email_receivers(project.id) + assert len(receivers) == 2 + + user.verified_email = False + db.session.commit() + receivers = project_handler.get_email_receivers(project.id) + assert len(receivers) == 1 + + user.verified_email = True + user.profile.receive_notifications = False + db.session.commit() + receivers = project_handler.get_email_receivers(project.id) + assert len(receivers) == 1 + + admin = User.query.filter(User.username == "mergin").first() + admin.is_admin = False + db.session.commit() + receivers = project_handler.get_email_receivers(project.id) + assert len(receivers) == 0 diff --git a/server/mergin/tests/utils.py b/server/mergin/tests/utils.py index 0d4d4f9c..0f768c6e 100644 --- a/server/mergin/tests/utils.py +++ b/server/mergin/tests/utils.py @@ -28,7 +28,7 @@ CHUNK_SIZE = 1024 -def add_user(username="random", password="random", is_admin=False): +def add_user(username="random", password="random", is_admin=False) -> User: """Helper function to create not-privileged user. Associated user workspace is created with db hook. From 8d3b7d3faed79badf62228ad6501dfaccfd1e770 Mon Sep 17 00:00:00 2001 From: "marcel.kocisek" Date: Thu, 20 Mar 2025 17:40:40 +0100 Subject: [PATCH 2/4] Handle project instead of project_id --- server/mergin/sync/interfaces.py | 2 +- server/mergin/sync/private_api_controller.py | 2 +- server/mergin/sync/project_handler.py | 6 +++--- server/mergin/tests/test_project_handler.py | 10 +++++----- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/server/mergin/sync/interfaces.py b/server/mergin/sync/interfaces.py index f57f103c..bb2e9843 100644 --- a/server/mergin/sync/interfaces.py +++ b/server/mergin/sync/interfaces.py @@ -188,7 +188,7 @@ def get_push_permission(self, changes: dict): """ pass - def get_email_receivers(self, project_id: str): + def get_email_receivers(self, project): """ Return list of members who should receive email notifications about project changes """ diff --git a/server/mergin/sync/private_api_controller.py b/server/mergin/sync/private_api_controller.py index 24b30596..643c274c 100644 --- a/server/mergin/sync/private_api_controller.py +++ b/server/mergin/sync/private_api_controller.py @@ -62,7 +62,7 @@ def create_project_access_request(namespace, project_name): # noqa: E501 db.session.add(access_request) db.session.commit() # notify project owners - owners = current_app.project_handler.get_email_receivers(project.id) + owners = current_app.project_handler.get_email_receivers(project) for owner in owners: email_data = { "subject": "Project access requested", diff --git a/server/mergin/sync/project_handler.py b/server/mergin/sync/project_handler.py index 0e6eada5..b4f1b09f 100644 --- a/server/mergin/sync/project_handler.py +++ b/server/mergin/sync/project_handler.py @@ -1,4 +1,4 @@ -from .models import ProjectRole, ProjectUser +from .models import ProjectRole, ProjectUser, Project from .interfaces import AbstractProjectHandler from .permissions import ProjectPermissions from sqlalchemy import or_, and_ @@ -10,14 +10,14 @@ class ProjectHandler(AbstractProjectHandler): def get_push_permission(self, changes: dict): return ProjectPermissions.Upload - def get_email_receivers(self, project_id: str) -> List[User]: + def get_email_receivers(self, project: Project) -> List[User]: return ( User.query.join(UserProfile) .outerjoin(ProjectUser, ProjectUser.user_id == User.id) .filter( or_( and_( - ProjectUser.project_id == project_id, + ProjectUser.project_id == project.id, ProjectUser.role == ProjectRole.OWNER.value, ), User.is_admin, diff --git a/server/mergin/tests/test_project_handler.py b/server/mergin/tests/test_project_handler.py index 0b2ebe81..e49cb119 100644 --- a/server/mergin/tests/test_project_handler.py +++ b/server/mergin/tests/test_project_handler.py @@ -21,27 +21,27 @@ def test_email_receivers(client): project = create_project("test_project", workspace, user) project.set_role(user.id, ProjectRole.READER) db.session.commit() - receivers = project_handler.get_email_receivers(project.id) + receivers = project_handler.get_email_receivers(project) assert len(receivers) == 1 project.set_role(user.id, ProjectRole.OWNER) db.session.commit() - receivers = project_handler.get_email_receivers(project.id) + receivers = project_handler.get_email_receivers(project) assert len(receivers) == 2 user.verified_email = False db.session.commit() - receivers = project_handler.get_email_receivers(project.id) + receivers = project_handler.get_email_receivers(project) assert len(receivers) == 1 user.verified_email = True user.profile.receive_notifications = False db.session.commit() - receivers = project_handler.get_email_receivers(project.id) + receivers = project_handler.get_email_receivers(project) assert len(receivers) == 1 admin = User.query.filter(User.username == "mergin").first() admin.is_admin = False db.session.commit() - receivers = project_handler.get_email_receivers(project.id) + receivers = project_handler.get_email_receivers(project) assert len(receivers) == 0 From 11fb9dc040aec4b4ee204756621bbe35c33a73ce Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Fri, 21 Mar 2025 10:00:20 +0100 Subject: [PATCH 3/4] Add User.active condition --- server/mergin/sync/project_handler.py | 1 + 1 file changed, 1 insertion(+) diff --git a/server/mergin/sync/project_handler.py b/server/mergin/sync/project_handler.py index b4f1b09f..8299935a 100644 --- a/server/mergin/sync/project_handler.py +++ b/server/mergin/sync/project_handler.py @@ -22,6 +22,7 @@ def get_email_receivers(self, project: Project) -> List[User]: ), User.is_admin, ), + User.active, User.verified_email, UserProfile.receive_notifications, ) From 208c3f2794f944cc8f4ffe3297889373ed7ba5ff Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Fri, 21 Mar 2025 10:43:13 +0100 Subject: [PATCH 4/4] Add test for inactive user --- server/mergin/tests/test_project_handler.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/server/mergin/tests/test_project_handler.py b/server/mergin/tests/test_project_handler.py index e49cb119..76040ca8 100644 --- a/server/mergin/tests/test_project_handler.py +++ b/server/mergin/tests/test_project_handler.py @@ -40,6 +40,12 @@ def test_email_receivers(client): receivers = project_handler.get_email_receivers(project) assert len(receivers) == 1 + user.profile.receive_notifications = True + user.active = False + db.session.commit() + receivers = project_handler.get_email_receivers(project) + assert len(receivers) == 1 + admin = User.query.filter(User.username == "mergin").first() admin.is_admin = False db.session.commit()