From 56cd00883904c52e2a0ba02664e6b76b7500320f Mon Sep 17 00:00:00 2001
From: Martin Varga <martin.varga@lutraconsulting.co.uk>
Date: Mon, 9 Dec 2024 14:14:44 +0100
Subject: [PATCH] Address review comments

---
 server/mergin/auth/app.py                      |  4 ++++
 server/mergin/auth/models.py                   | 10 +++++-----
 server/mergin/sync/private_api_controller.py   |  2 +-
 server/mergin/sync/public_api_v2.yaml          |  2 +-
 server/mergin/sync/public_api_v2_controller.py |  4 ++--
 server/mergin/sync/schemas.py                  |  3 ++-
 6 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/server/mergin/auth/app.py b/server/mergin/auth/app.py
index 10f9270c..cfba8d98 100644
--- a/server/mergin/auth/app.py
+++ b/server/mergin/auth/app.py
@@ -97,6 +97,10 @@ def confirm_token(token, expiration=3600 * 24 * 3):
 
 
 def send_confirmation_email(app, user, url, template, header, **kwargs):
+    """
+    Send confirmation email from selected template with customizable email subject and confirmation URL.
+    Optional kwargs are passed to render_template method if needed for particular template.
+    """
     from ..celery import send_email_async
 
     token = generate_confirmation_token(app, user.email)
diff --git a/server/mergin/auth/models.py b/server/mergin/auth/models.py
index 991698c0..c16c21f6 100644
--- a/server/mergin/auth/models.py
+++ b/server/mergin/auth/models.py
@@ -180,13 +180,13 @@ def anonymize(self):
         db.session.commit()
 
     @classmethod
-    def get_by_login(cls, username: str) -> Optional[User]:
-        """Find user by its username or email"""
-        username = username.strip().lower()
+    def get_by_login(cls, login: str) -> Optional[User]:
+        """Find user by its login which can be either username or email"""
+        login = login.strip().lower()
         return cls.query.filter(
             or_(
-                func.lower(User.email) == username,
-                func.lower(User.username) == username,
+                func.lower(User.email) == login,
+                func.lower(User.username) == login,
             )
         ).first()
 
diff --git a/server/mergin/sync/private_api_controller.py b/server/mergin/sync/private_api_controller.py
index d4b66c0d..616ac69d 100644
--- a/server/mergin/sync/private_api_controller.py
+++ b/server/mergin/sync/private_api_controller.py
@@ -135,10 +135,10 @@ def accept_project_access_request(request_id):
     project = access_request.project
     project_role = ProjectPermissions.get_user_project_role(project, current_user)
     if project_role == ProjectRole.OWNER:
+        access_request.accept(permission)
         project_access_granted.send(
             access_request.project, user_id=access_request.requested_by
         )
-        access_request.accept(permission)
         return "", 200
     abort(403, "You don't have permissions to accept project access request")
 
diff --git a/server/mergin/sync/public_api_v2.yaml b/server/mergin/sync/public_api_v2.yaml
index dcb9a3ee..16c71155 100644
--- a/server/mergin/sync/public_api_v2.yaml
+++ b/server/mergin/sync/public_api_v2.yaml
@@ -206,7 +206,7 @@ paths:
       tags:
         - project
       summary: Remove project collaborator
-      operationId: remove_project_member
+      operationId: remove_project_collaborator
       parameters:
         - $ref: "#/components/parameters/ProjectId"
       responses:
diff --git a/server/mergin/sync/public_api_v2_controller.py b/server/mergin/sync/public_api_v2_controller.py
index be1eb8ee..c4e3cd77 100644
--- a/server/mergin/sync/public_api_v2_controller.py
+++ b/server/mergin/sync/public_api_v2_controller.py
@@ -100,8 +100,8 @@ def add_project_member(id):
         abort(409, "User is already a project member")
 
     project.set_role(user.id, ProjectRole(request.json["role"]))
-    project_access_granted.send(project, user_id=user.id)
     db.session.commit()
+    project_access_granted.send(project, user_id=user.id)
     data = ProjectMemberSchema().dump(project.get_member(user.id))
     return data, 201
 
@@ -121,7 +121,7 @@ def update_project_member(id, user_id):
 
 
 @auth_required
-def remove_project_member(id, user_id):
+def remove_project_collaborator(id, user_id):
     """Remove project collaborator"""
     project = require_project_by_uuid(id, ProjectPermissions.Update)
     if not project.get_role(user_id):
diff --git a/server/mergin/sync/schemas.py b/server/mergin/sync/schemas.py
index 1225c4d5..c782c5ca 100644
--- a/server/mergin/sync/schemas.py
+++ b/server/mergin/sync/schemas.py
@@ -337,7 +337,8 @@ class UserWorkspaceSchema(ma.SQLAlchemyAutoSchema):
     def _user_role(self, obj):
         if not self.context.get("user"):
             return
-        return obj.get_user_role(self.context.get("user")).value
+        role = obj.get_user_role(self.context.get("user"))
+        return role and role.value
 
 
 class ProjectInvitationAccessSchema(Schema):