Merge pull request #1564 from ScilifelabDataCentre/DDS-2152-Update-MO…

…TD-endpoint-Ina-s-review Update MOTD endpoint
ScilifelabDataCentre · Oct 21, 2024 · fc40670 · fc40670
2 parents 5fbe91a + 9fb4bf3
commit fc40670
Show file tree

Hide file tree

Showing 6 changed files with 22 additions and 30 deletions.
diff --git a/SPRINTLOG.md b/SPRINTLOG.md
@@ -442,3 +442,4 @@ _Nothing merged during this sprint_
 - Workflow bug fixed: PDFs (Technical Overview and Troubleshooting) were downloaded to incorrect directory([#1559](https://github.com/ScilifelabDataCentre/dds_web/pull/1559))
 - Update trivy action and add a second mirror repository to reduce TOO MANY REQUEST issue([#1560](https://github.com/ScilifelabDataCentre/dds_web/pull/1560))
 - Modify the invoicing commands to send the instance name in the emails([#1561](https://github.com/ScilifelabDataCentre/dds_web/pull/1561))
+- Fix the MOTD endpoint according to post merge review([#1564](https://github.com/ScilifelabDataCentre/dds_web/pull/1564))
diff --git a/dds_web/api/superadmin_only.py b/dds_web/api/superadmin_only.py
@@ -163,17 +163,12 @@ def post(self):
         if not motd_obj or not motd_obj.active:
             raise ddserr.DDSArgumentError(message=f"There is no active MOTD with ID '{motd_id}'.")
 
-        # check if sent to unit personnel only or all users
-        unit_personnel_only: bool = request_json.get("unit_personnel_only", False)
-        if not isinstance(unit_personnel_only, bool):
-            raise ddserr.DDSArgumentError(
-                message="The 'unit_personnel_only' argument must be a boolean."
-            )
-        if unit_personnel_only:
-            unit_usernames = db.session.query(models.UnitUser.username)
-            users_to_send = db.session.query(models.User).filter(
-                models.User.username.in_(unit_usernames)
-            )
+        # check if sent to unit users only or all users
+        unit_only: bool = request_json.get("unit_only", False)
+        if not isinstance(unit_only, bool):
+            raise ddserr.DDSArgumentError(message="The 'unit_only' argument must be a boolean.")
+        if unit_only:
+            users_to_send = db.session.query(models.UnitUser)
         else:
             users_to_send = db.session.query(models.User)
 
@@ -212,7 +207,7 @@ def post(self):
                 utils.send_email_with_retry(msg=msg, obj=conn)
 
         return_msg = f"MOTD '{motd_id}' has been "
-        if unit_personnel_only:
+        if unit_only:
             return_msg += "sent to unit personnel only."
         else:
             return_msg += "sent to all users."

diff --git a/dds_web/static/swagger.yaml b/dds_web/static/swagger.yaml
@@ -1447,7 +1447,7 @@ paths:
                 motd_id:
                   type: integer
                   example: 1
-                unit_personnel_only:
+                unit_only:
                   type: boolean
                   example: false
   /user/find:

diff --git a/dds_web/static/swaggerv3.yaml b/dds_web/static/swaggerv3.yaml
@@ -1409,7 +1409,7 @@ paths:
                 motd_id:
                   type: integer
                   example: 1
-                unit_personnel_only:
+                unit_only:
                   type: boolean
                   example: false
   /user/find:

diff --git a/tests/api/test_superadmin_only.py b/tests/api/test_superadmin_only.py
@@ -588,8 +588,8 @@ def test_send_motd_no_primary_email(client: flask.testing.FlaskClient) -> None:
         assert "incorrect subject" not in outbox[-1].subject
 
 
-def test_send_motd_incorrect_type_unit_personnel_only(client: flask.testing.FlaskClient) -> None:
-    """The parameter unit_personnel_only should be a boolean"""
+def test_send_motd_incorrect_type_unit_user_only(client: flask.testing.FlaskClient) -> None:
+    """The parameter unit_only should be a boolean"""
     # Authenticate
     token: typing.Dict = get_token(username=users["Super Admin"], client=client)
 
@@ -608,12 +608,10 @@ def test_send_motd_incorrect_type_unit_personnel_only(client: flask.testing.Flas
         response: werkzeug.test.WrapperTestResponse = client.post(
             tests.DDSEndpoint.MOTD_SEND,
             headers=token,
-            json={"motd_id": created_motd.id, "unit_personnel_only": "some_string"},
+            json={"motd_id": created_motd.id, "unit_only": "some_string"},
         )
         assert response.status_code == http.HTTPStatus.BAD_REQUEST
-        assert "The 'unit_personnel_only' argument must be a boolean." in response.json.get(
-            "message"
-        )
+        assert "The 'unit_only' argument must be a boolean." in response.json.get("message")
         assert mock_mail_send.call_count == 0
 
 
@@ -640,7 +638,7 @@ def test_send_motd_ok_all(client: flask.testing.FlaskClient) -> None:
         response: werkzeug.test.WrapperTestResponse = client.post(
             tests.DDSEndpoint.MOTD_SEND,
             headers=token,
-            json={"motd_id": created_motd.id, "unit_personnel_only": False},
+            json={"motd_id": created_motd.id, "unit_only": False},
         )
         assert response.status_code == http.HTTPStatus.OK
         assert len(outbox) == num_users
@@ -670,7 +668,7 @@ def test_send_motd_ok_unitusers(client: flask.testing.FlaskClient) -> None:
         response: werkzeug.test.WrapperTestResponse = client.post(
             tests.DDSEndpoint.MOTD_SEND,
             headers=token,
-            json={"motd_id": created_motd.id, "unit_personnel_only": True},
+            json={"motd_id": created_motd.id, "unit_only": True},
         )
         assert response.status_code == http.HTTPStatus.OK
         assert len(outbox) == num_users

diff --git a/tests/tests_v3/api/test_superadmin_only.py b/tests/tests_v3/api/test_superadmin_only.py
@@ -575,8 +575,8 @@ def test_send_motd_no_primary_email(client: flask.testing.FlaskClient) -> None:
         assert "incorrect subject" not in outbox[-1].subject
 
 
-def test_send_motd_incorrect_type_unit_personnel_only(client: flask.testing.FlaskClient) -> None:
-    """The parameter unit_personnel_only should be a boolean"""
+def test_send_motd_incorrect_type_unit_only(client: flask.testing.FlaskClient) -> None:
+    """The parameter unit_only should be a boolean"""
     # Authenticate
     token: typing.Dict = get_token(username=users["Super Admin"], client=client)
 
@@ -595,12 +595,10 @@ def test_send_motd_incorrect_type_unit_personnel_only(client: flask.testing.Flas
         response: werkzeug.test.WrapperTestResponse = client.post(
             tests.DDSEndpoint.MOTD_SEND,
             headers=token,
-            json={"motd_id": created_motd.id, "unit_personnel_only": "some_string"},
+            json={"motd_id": created_motd.id, "unit_only": "some_string"},
         )
         assert response.status_code == http.HTTPStatus.BAD_REQUEST
-        assert "The 'unit_personnel_only' argument must be a boolean." in response.json.get(
-            "message"
-        )
+        assert "The 'unit_only' argument must be a boolean." in response.json.get("message")
         assert mock_mail_send.call_count == 0
 
 
@@ -627,7 +625,7 @@ def test_send_motd_ok_all(client: flask.testing.FlaskClient) -> None:
         response: werkzeug.test.WrapperTestResponse = client.post(
             tests.DDSEndpoint.MOTD_SEND,
             headers=token,
-            json={"motd_id": created_motd.id, "unit_personnel_only": False},
+            json={"motd_id": created_motd.id, "unit_only": False},
         )
         assert response.status_code == http.HTTPStatus.OK
         assert len(outbox) == num_users
@@ -657,7 +655,7 @@ def test_send_motd_ok_unitusers(client: flask.testing.FlaskClient) -> None:
         response: werkzeug.test.WrapperTestResponse = client.post(
             tests.DDSEndpoint.MOTD_SEND,
             headers=token,
-            json={"motd_id": created_motd.id, "unit_personnel_only": True},
+            json={"motd_id": created_motd.id, "unit_only": True},
         )
         assert response.status_code == http.HTTPStatus.OK
         assert len(outbox) == num_users