From 8889d7c99a73becbe205e89aec28821bca7a2143 Mon Sep 17 00:00:00 2001 From: Hannes Janott Date: Wed, 16 Oct 2024 19:41:36 +0200 Subject: [PATCH] participant import error with gender and meeting admin (#2689) --- .../action/actions/user/participant_import.py | 6 +- .../actions/user/participant_json_upload.py | 12 ++-- .../action/user/test_participant_import.py | 67 +++++++++++-------- .../user/test_participant_json_upload.py | 35 +++++++--- 4 files changed, 72 insertions(+), 48 deletions(-) diff --git a/openslides_backend/action/actions/user/participant_import.py b/openslides_backend/action/actions/user/participant_import.py index fb4e1c1fc..fec1cfd77 100644 --- a/openslides_backend/action/actions/user/participant_import.py +++ b/openslides_backend/action/actions/user/participant_import.py @@ -120,9 +120,7 @@ def validate_entry(self, row: ImportRow) -> None: entry["meeting_id"] = self.meeting_id if isinstance(entry.get("gender"), dict): - if entry["gender"].get("info") != ImportState.WARNING: - entry["gender_id"] = entry["gender"]["id"] - entry.pop("gender") + entry["gender_id"] = entry.pop("gender") if "groups" not in entry: raise ActionException( @@ -199,6 +197,8 @@ def validate_entry(self, row: ImportRow) -> None: entry, row["messages"], entry.get("groups", []), row ) + if entry.get("gender_id"): + entry["gender"] = entry.pop("gender_id") entry.pop("meeting_id") if row["state"] == ImportState.ERROR and self.import_state == ImportState.DONE: self.import_state = ImportState.ERROR diff --git a/openslides_backend/action/actions/user/participant_json_upload.py b/openslides_backend/action/actions/user/participant_json_upload.py index 4dc108b6b..b6dd27ed1 100644 --- a/openslides_backend/action/actions/user/participant_json_upload.py +++ b/openslides_backend/action/actions/user/participant_json_upload.py @@ -103,12 +103,10 @@ def validate_entry(self, entry: dict[str, Any]) -> dict[str, Any]: ) payload_index = entry.pop("payload_index", None) - # only needed for get_failing_fields not to fail - if gender := entry.pop("gender", None): - entry["gender_id"] = {} + # swapping needed for get_failing_fields and setting import states not to fail + if entry.get("gender"): + entry["gender_id"] = entry.pop("gender") failing_fields = self.permission_check.get_failing_fields(entry) - if gender: - entry.pop("gender_id") entry.pop("group_ids") entry.pop("structure_level_ids") entry.pop("meeting_id") @@ -157,8 +155,8 @@ def validate_entry(self, entry: dict[str, Any]) -> dict[str, Any]: if payload_index: entry["payload_index"] = payload_index - if gender: - entry["gender"] = gender + if entry.get("gender_id"): + entry["gender"] = entry.pop("gender_id") return results diff --git a/tests/system/action/user/test_participant_import.py b/tests/system/action/user/test_participant_import.py index c676b6f04..4cf97c97a 100644 --- a/tests/system/action/user/test_participant_import.py +++ b/tests/system/action/user/test_participant_import.py @@ -42,6 +42,13 @@ def setUp(self) -> None: "value": "male", "info": ImportState.DONE, }, + "groups": [ + { + "id": 1, + "value": "group1", + "info": ImportState.DONE, + } + ], }, }, ], @@ -75,6 +82,8 @@ def setUp(self) -> None: ) def test_import_without_any_group_in_import_data(self) -> None: + del self.import_preview1_data["result"]["rows"][0]["data"]["groups"] + self.update_model("import_preview/1", self.import_preview1_data) response = self.request("participant.import", {"id": 1, "import": True}) self.assert_status_code(response, 400) assert ( @@ -100,10 +109,6 @@ def test_import_wrong_invalid_name_in_preview(self) -> None: self.assert_model_exists("import_preview/1", {"name": "account"}) def test_import_names_and_email_and_create(self) -> None: - self.import_preview1_data["result"]["rows"][0]["data"]["groups"] = [ - {"info": ImportState.DONE, "value": "group1", "id": 1} - ] - self.update_model("import_preview/1", self.import_preview1_data) response = self.request("participant.import", {"id": 1, "import": True}) self.assert_status_code(response, 200) self.assert_model_exists( @@ -135,10 +140,6 @@ def test_import_names_and_email_and_create(self) -> None: ) def test_import_with_group_created_in_between(self) -> None: - self.import_preview1_data["result"]["rows"][0]["data"]["groups"] = [ - {"info": ImportState.DONE, "value": "group1", "id": 1} - ] - self.update_model("import_preview/1", self.import_preview1_data) self.set_models( { "group/123": {"meeting_id": 1, "name": "2Bcreated"}, @@ -195,9 +196,6 @@ def test_import_saml_id_error_new_and_saml_id_exists(self) -> None: "value": "testsaml", "info": ImportState.NEW, } - self.import_preview1_data["result"]["rows"][0]["data"]["groups"] = [ - {"info": ImportState.DONE, "value": "group1", "id": 1} - ] self.set_models( { "user/1": {"saml_id": "testsaml"}, @@ -219,9 +217,6 @@ def test_import_gender_warning(self) -> None: "value": "notAGender", "info": ImportState.WARNING, } - self.import_preview1_data["result"]["rows"][0]["data"]["groups"] = [ - {"info": ImportState.DONE, "value": "group1", "id": 1} - ] self.import_preview1_data["result"]["rows"][0]["messages"] = [ "Gender 'notAGender' is not in the allowed gender list." ] @@ -255,9 +250,6 @@ def test_import_error_state_done_missing_user_in_db(self) -> None: "id": 111, } self.import_preview1_data["result"]["rows"][0]["data"]["id"] = 111 - self.import_preview1_data["result"]["rows"][0]["data"]["groups"] = [ - {"info": ImportState.DONE, "value": "group1", "id": 1} - ] self.update_model("import_preview/1", self.import_preview1_data) response = self.request("participant.import", {"id": 1, "import": True}) self.assert_status_code(response, 200) @@ -278,10 +270,6 @@ def test_import_no_permission(self) -> None: self.base_permission_test({}, "participant.import", {"id": 1, "import": True}) def test_import_permission(self) -> None: - self.import_preview1_data["result"]["rows"][0]["data"]["groups"] = [ - {"info": ImportState.DONE, "value": "group1", "id": 1} - ] - self.update_model("import_preview/1", self.import_preview1_data) self.base_permission_test( {}, "participant.import", @@ -290,10 +278,6 @@ def test_import_permission(self) -> None: ) def test_import_permission_2(self) -> None: - self.import_preview1_data["result"]["rows"][0]["data"]["groups"] = [ - {"info": ImportState.DONE, "value": "group1", "id": 1} - ] - self.update_model("import_preview/1", self.import_preview1_data) self.base_permission_test( {}, "participant.import", @@ -302,6 +286,33 @@ def test_import_permission_2(self) -> None: True, ) + def test_import_permission_meeting_admin(self) -> None: + self.import_preview1_data["result"]["rows"][0]["data"]["id"] = 1 + self.import_preview1_data["result"]["rows"][0]["state"] = ImportState.DONE + self.import_preview1_data["result"]["rows"][0]["data"]["gender"][ + "info" + ] = ImportState.REMOVE + self.update_model("import_preview/1", self.import_preview1_data) + + self.create_meeting() + self.create_meeting(4) + user_id = self.create_user_for_meeting(1) + other_user_id = 3 + self.set_models( + { + f"user/{other_user_id}": self._get_user_data("jonny", {1: [], 4: []}), + } + ) + self.set_user_groups(user_id, [2]) + self.set_user_groups(other_user_id, [1, 4]) + self.login(user_id) + + response = self.request("participant.import", {"id": 1, "import": True}) + + self.assert_status_code(response, 200) + self.assert_model_exists("meeting_user/3", {"user_id": 3, "meeting_id": 4}) + self.assert_model_not_exists("meeting_user/4") + def test_import_locked_meeting(self) -> None: self.base_locked_out_superadmin_permission_test( {}, "participant.import", {"id": 1, "import": True} @@ -734,7 +745,7 @@ def test_json_upload_one_structure_level_newly_created(self) -> None: {"id": created_groups["group4"], "info": "new", "value": "group4"}, ], "structure_level": [{"info": "new", "value": "level up", "id": 2}], - "gender_id": 3, + "gender": {"id": 3, "info": "done", "value": "diverse"}, } row = result["rows"][1] @@ -780,6 +791,7 @@ def test_json_upload_one_structure_level_newly_created(self) -> None: {"info": ImportState.NEW, "value": "level up", "id": 2}, {"info": ImportState.DONE, "value": "no. 5", "id": 1}, ], + "gender": {"info": "warning", "value": "unknown"}, } self.assert_model_exists("structure_level/2", {"name": "level up"}) @@ -847,7 +859,7 @@ def test_json_upload_update_multiple_users_all_error(self) -> None: {"info": "new", "value": "group4"}, ], "structure_level": [{"info": "new", "value": "level up"}], - "gender_id": 3, + "gender": {"id": 3, "info": "done", "value": "diverse"}, } row = result["rows"][1] @@ -898,6 +910,7 @@ def test_json_upload_update_multiple_users_all_error(self) -> None: {"info": ImportState.NEW, "value": "level up"}, {"info": ImportState.NEW, "value": "no. 5"}, ], + "gender": {"info": "warning", "value": "unknown"}, } row = result["rows"][4] diff --git a/tests/system/action/user/test_participant_json_upload.py b/tests/system/action/user/test_participant_json_upload.py index 4f2a873e3..6005ffc1a 100644 --- a/tests/system/action/user/test_participant_json_upload.py +++ b/tests/system/action/user/test_participant_json_upload.py @@ -9,6 +9,7 @@ class ParticipantJsonUpload(BaseActionTestCase): def setUp(self) -> None: + self.maxDiff = None super().setUp() self.set_models( { @@ -365,10 +366,18 @@ def test_json_upload_permission_2(self) -> None: True, ) - def test_json_upload_permission_meeting_admin(self) -> None: + def test_json_upload_no_permission_meeting_admin(self) -> None: self.create_meeting() + self.create_meeting(4) user_id = self.create_user_for_meeting(1) + other_user_id = 3 + self.set_models( + { + f"user/{other_user_id}": self._get_user_data("test", {1: [], 4: []}), + } + ) self.set_user_groups(user_id, [2]) + self.set_user_groups(other_user_id, [1, 4]) self.login(user_id) response = self.request( "participant.json_upload", @@ -389,28 +398,32 @@ def test_json_upload_permission_meeting_admin(self) -> None: "meeting_id": 1, "rows": [ { - "state": ImportState.NEW, - "messages": [], + "state": ImportState.DONE, + "messages": [ + "Following fields were removed from payload, because the user has no permissions to change them: username, gender_id, default_password" + ], "data": { "username": { "value": "test", - "info": ImportState.DONE, + "info": ImportState.REMOVE, + "id": 3, + }, + "default_password": { + "value": "secret", + "info": ImportState.REMOVE, }, + "id": 3, "groups": [ { - "id": 1, "info": ImportState.GENERATED, "value": "group1", + "id": 1, } ], "gender": { - "id": 1, - "info": ImportState.DONE, + "info": ImportState.REMOVE, "value": "male", - }, - "default_password": { - "value": "secret", - "info": ImportState.DONE, + "id": 1, }, }, }