2170 introduce gender model #2485

hjanott · 2024-06-21T13:18:36Z

Themes to check:

meeting export: export only the gender string per user.
meeting import: import the gender only for users that need to be created.
save_saml_account-Action: saml_attr_mapping field continue to use gender:gender mapping. In the action we have to transform the gender-string to an existing or new gender_id and update the user. Always create not existing gender-instances. An action-payload without gender-field may change nothing, a gender-field with an empty string should be interpreted as "no gender value wanted" and the user's gender_id should be set to NULL. @rrenkert @Elblinator hope you agree
user.create/update: now uses gender_id in payload instead of gender as string. Must be also changed in client.
set null in user and organization on deletion of gender-instances

…rganiziation

r-peschke

See comments in migration code.
With help of the openslides-datastore-service pull request 368 it will be easier to write the migration.

docs/actions/organization.update.md

openslides_backend/migrations/migrations/0054_introduce_flexible_gender_model.py

Elblinator

Please change the permission from can_manage_organization to can_manage_accounts to edit/delete or update a gender

hjanott · 2024-06-27T11:18:07Z

can_manage_accounts

I will use can_manage_users instead. ;)

openslides_backend/migrations/migrations/0054_introduce_flexible_gender_model.py

r-peschke · 2024-07-04T08:02:26Z

openslides_backend/migrations/migrations/0054_introduce_flexible_gender_model.py

+            # update users
+            for user_id, user in users.items():
+                if user.get("gender"):
+                    if gender_id := gender_strings.index(user.get("gender")) + 1:


The index-method on a list will raise an exception, if no string matches. Use it with a check before (user.get("gender") in gender_strings) or with a try/except-construct.

r-peschke · 2024-07-04T08:03:54Z

openslides_backend/migrations/migrations/0054_introduce_flexible_gender_model.py

+                        events.append(
+                            RequestUpdateEvent(
+                                fqid_from_collection_and_id("user", user_id),
+                                {"gender_id": gender_id, "gender": None},


Even if there was no gender_id found for the gender you must create an event to set gender to None.

r-peschke · 2024-07-04T08:15:06Z

openslides_backend/action/actions/meeting/import_.py

@@ -216,6 +216,7 @@ def update_instance(self, instance: dict[str, Any]) -> dict[str, Any]:
                    "last_email_sent",
                    "last_login",
                    "committee_ids",
+                    "gender",


Removing is a concern of the migration. Please remove the deletion here and integrate it in the migration.

We can do that but then we would have to ensure, that the client will not export the gender in future migration indices. Also we would have to rewrite the tests for the meeting import to not include the gender per default since the majority of tests uses the backend migration index, which I then did.

r-peschke · 2024-07-04T09:12:40Z

tests/system/action/meeting/test_import.py

+                "gender/1": {"name": "male"},
+                "gender/2": {"name": "female"},
+                "gender/3": {"name": "diverse"},
+                "gender/4": {"name": "non-binary"},


Amend an existing test: explicit test for the field gender being empty after importing a new user

I used test_all_migrations for this.

r-peschke · 2024-07-04T16:15:04Z

openslides_backend/action/actions/user/user_mixins.py

+) -> dict[str, int] | None:
+    """returns gender as a dict of "name" and "id" if it exists, otherwise raises ActionException"""
+    if gender_id := instance.get("gender_id"):
+        gender_dict = datastore.get_all("gender", ["id", "name"], lock_result=False)


use a simple self.datastore.get("gender", gender_id,...) to read the gender instance.

I use the fqid to get the gender now.

r-peschke · 2024-07-04T16:22:19Z

openslides_backend/action/actions/user/user_mixins.py

-                )
+def check_gender_exists(
+    datastore: DatastoreService, instance: dict[str, Any]
+) -> dict[str, int] | None:


don't return anything, it is never used

r-peschke · 2024-07-05T13:13:42Z

tests/system/action/user/test_save_saml_account.py

@@ -29,7 +29,7 @@ def setUp(self) -> None:
                        "first_name": "firstName",
                        "last_name": "lastName",
                        "email": "email",
-                        "gender": "gender",
+                        "gender_id": "gender_id",


revert to gender:gender

r-peschke · 2024-07-05T13:20:25Z

openslides_backend/action/actions/user/merge_together.py

@@ -28,7 +28,7 @@ class UserMergeTogether(CreateAction, CheckForArchivedMeetingMixin):
            "is_active",
            "is_physical_person",
            "default_password",
-            "gender",
+            "gender_id",


revert to gender

As discussed with @Elblinator only the backend and client will use this and we can expect the client to use the id.

Please change the documentation or the implementation

r-peschke · 2024-07-05T13:21:08Z

tests/system/action/organization/test_update.py

@@ -12,7 +12,7 @@ class OrganizationUpdateActionTest(BaseActionTestCase):
        "first_name": "firstName",
        "last_name": "lastName",
        "email": "email",
-        "gender": "gender",
+        "gender_id": "gender_id",


revert to gender:gender

r-peschke · 2024-08-13T12:32:59Z

docs/actions/meeting.import.md

@@ -16,10 +16,11 @@ Import one meeting from a file. The file must only contain exactly one meeting.
 - The request user is assigned to the admin group.
 - meeting.is_active_in_organization_id is set.
 - It has to be checked, whether the organization.limit_of_meetings is unlimited(=0) or lower than the active meetings in organization.active_meeting_ids, if the new meeting is not archived (`is_active_in_organization_id` is set)
- Search for users and if username, first-name, last-name and email are identical use this user instead creating a duplicate. Keep the data, including password, of the exiating user.
+- Search for users and if username, first-name, last-name and email are identical use this existing user instead of creating a duplicate. Keep the data, including password, of the existing user. Relavent relations such as to the meeting will be updated though.


Suggested change

- Search for users and if username, first-name, last-name and email are identical use this existing user instead of creating a duplicate. Keep the data, including password, of the existing user. Relavent relations such as to the meeting will be updated though.

- Search for users and if username, first-name, last-name and email are identical use this existing user instead of creating a duplicate. Keep the data, including password, of the existing user. Relevant relations such as to the meeting will be updated though.

r-peschke · 2024-08-13T13:48:01Z

openslides_backend/action/actions/user/merge_together.py

@@ -28,7 +28,7 @@ class UserMergeTogether(CreateAction, CheckForArchivedMeetingMixin):
            "is_active",
            "is_physical_person",
            "default_password",
-            "gender",
+            "gender_id",


Please change the documentation or the implementation

r-peschke · 2024-08-13T15:08:32Z

tests/system/action/gender/test_create.py

+
+    def test_create(self) -> None:
+        self.set_models(self.test_models)
+        self.set_models({"gender/1": {"organization_id": 1, "name": "male"}})


You are using the test_models only once. Then please join organization and user setting with the gender/1 setting in line 18 and remove it from setUp-method.
Additionally you should add the opposite relation (field organization/gender_ids) to the organization/1-record and assert the gender/1 and gender/2 instances in organization.

r-peschke · 2024-08-13T15:16:20Z

tests/system/action/gender/test_delete.py

+    def create_data(self) -> None:
+        self.set_models(
+            {
+                ONE_ORGANIZATION_FQID: {"gender_ids": [self.gender_id]},


Also add gender/1 to the gender_ids.

r-peschke · 2024-08-13T15:19:43Z

tests/system/action/gender/test_delete.py

+                ONE_ORGANIZATION_FQID: {"gender_ids": [1, self.gender_id, 6]},
+            }
+        )
+        # user = self.request("user.update", {"id": 21, "gender_id": self.gender_id}) #error?


Please remove the comment.

r-peschke · 2024-08-14T07:22:48Z

tests/system/action/meeting/test_import.py

@@ -2140,6 +2139,7 @@ def test_with_listfields_from_migration(self) -> None:
        data["meeting"]["meeting"]["1"]["motion_ids"] = [5, 6]
        data["meeting"]["meeting"]["1"]["list_of_speakers_ids"] = [1, 2]
        data["meeting"]["motion_state"]["1"]["motion_ids"] = [5, 6]
+        data["meeting"]["user"]["1"]["gender"] = "male"


Concerning gender:

set a different gender in the user that will become user/1 from import (match an existing user)

Write an assert for the imported user/1 gender/gender_id field after the request, showing that the gender not changed

In contrast to the other changed test make one with matching user and the other with new user

I hope I understood this correctly. I created another user to check both: creating a user with a new gender and not updating a matching user.

let's talk about it

r-peschke · 2024-08-14T07:29:43Z

tests/system/action/meeting/test_import.py

@@ -2225,6 +2224,10 @@ def test_all_migrations(self) -> None:
            "organization/1", {"user_ids": [1, 2], "active_meeting_ids": [1, 2]}
        )

+        self._assert_fields(


Don't use the private- method _assert_fields, but instead use self.assert_model_assists

tests/system/action/user/test_save_saml_account.py

r-peschke · 2024-08-14T09:09:25Z

openslides_backend/action/actions/user/save_saml_account.py

@@ -109,7 +124,16 @@ def validate_fields(self, instance_old: dict[str, Any]) -> dict[str, Any]:
                    if isinstance((tx := instance_old[payload_field]), list) and len(tx)
                    else tx
                )
-                if value not in (None, []):
+                if payload_field == "gender":


The requirement for the save_saml_account action is to create a gender-instance, if the value of the gender is not in the collection.
IMO this is the most convenient way to add new gender-entries to the collection and the same behavior of accepting all gender-values in save_saml_account as before.

Please update the action documentation and implement these feature as described.

…ate gender for saml accounts

…er_model

…e tests.

docs/actions/gender.update.md

r-peschke · 2024-08-21T10:10:46Z

openslides_backend/action/actions/meeting/import_.py

+        for user in users.values():
+            if gender := user.get("gender"):
+                del user["gender"]
+                # if user is to be created, convert to gender_id


IMO the comment at this place is misleading, because you don't convert gender to gender_id at this place

r-peschke · 2024-08-21T10:12:25Z

openslides_backend/action/actions/meeting/import_.py

+            if gender := user.get("gender"):
+                del user["gender"]
+                # if user is to be created, convert to gender_id
+                if gender != "":


No need for this if-statement, because the if in line 169 results in False, if gender is an empty string

Done. Also considered that an empty string should be deleted, plus writing a test for it.

r-peschke · 2024-08-21T10:17:05Z

openslides_backend/action/actions/meeting/import_.py

+        users = instance["meeting"].get("user", {})
+        for user_id, gender in self.user_id_to_gender.items():
+            if user_id not in self.merge_user_map:
+                gender_dict = self.datastore.filter(


IMO you should read all existing gender-names to a dict before the loop. Then you don't need to ask the database for each user.

r-peschke · 2024-08-21T10:20:57Z

openslides_backend/action/actions/meeting/import_.py

+                        GenderCreate, [{"name": gender}]
+                    )
+                    gender_id = action_result[0].get("id", 0)  # type: ignore
+                self.execute_other_action(


IMO you shouldn't update one field with an action, but update it in the users data. I hope I remember correctly, that finally user's will be update or created.

openslides_backend/shared/export_helper.py

tests/system/action/user/test_save_saml_account.py

r-peschke · 2024-08-21T10:56:39Z

tests/system/action/meeting/test_import.py

@@ -2140,6 +2139,7 @@ def test_with_listfields_from_migration(self) -> None:
        data["meeting"]["meeting"]["1"]["motion_ids"] = [5, 6]
        data["meeting"]["meeting"]["1"]["list_of_speakers_ids"] = [1, 2]
        data["meeting"]["motion_state"]["1"]["motion_ids"] = [5, 6]
+        data["meeting"]["user"]["1"]["gender"] = "male"


let's talk about it

r-peschke · 2024-08-21T11:03:35Z

openslides_backend/migrations/migrations/0054_introduce_flexible_gender_model.py

+
+    def migrate_models(self) -> list[BaseRequestEvent] | None:
+        events: list[BaseRequestEvent] = []
+        users = self.reader.get_all("user", ["gender"])


You don't need to read the users, if it is a in-memory-migration. Therefore you read them only, if it is no in-memory-migration

openslides_backend/migrations/migrations/0054_introduce_flexible_gender_model.py

… in saml, export, import and migration plus additional test for empty string.

r-peschke · 2024-08-22T15:56:41Z

docs/actions/gender.update.md

@@ -0,0 +1,15 @@
+## Payload
+```js


Why do you put the js-tag to the page?

I used it to see the highlighting changes as it is being done in some other md files. Since this is mostly not used in our docs I agreed with @rrenkert to delete these tags from the files I'm editing.

r-peschke · 2024-08-22T15:59:35Z

docs/actions/user.create.md

@@ -57,7 +57,7 @@ Creates a user.
 * If `username` is given, it has to be unique within all users. If there already exists a user with the same username, an error must be returned. If the `username` is not given, 1. the saml_id will be used or 2. it has to be generated (see [user.create#generate-a-username](user.create.md#generate-a-username) below). Also the username may not contain spaces.
 * The `organization_management_level` as restring can be taken from the enum of this user field.
 * Remove starting and trailing spaces from `username`, `first_name` and `last_name`
-* The given `gender` must be present in `organization/genders`
+* The given `gender_id` must be present in `organization/gender_ids`


Better: ... must be present in database. The gender lived only in organization/genders-field, the gender_id is presented by an gender-instance in the database.

r-peschke · 2024-08-26T08:36:54Z

openslides_backend/shared/export_helper.py

@@ -157,11 +157,16 @@ def add_users(
    )

    for user in users.values():
+        # convert to gender string


Please remove the comment. It is just the description of the following variable, but that would be to much to maintain

r-peschke · 2024-08-26T09:00:29Z

tests/system/action/meeting/test_import.py

+                "organization_id": 1,
+            }
+        }
+        other_user_request_data = {


Before this line you have to update your dict meeting["meeting"]["user"] with the data for user/2. Write something like
data["meeting"]["user"].update(other_user_request_data)
or put the user/2-data directly into the update-bracket.

I prefer to use both in one curly brace instead.

r-peschke · 2024-08-26T09:12:45Z

tests/system/action/meeting/test_import.py

+                "organization_id": 1,
+            }
+        }
+        data["meeting"]["user"]["1"]["gender"] = "needs_to_be_created"


Please put the change of user/1 in front of the changes of user/2. IMO it is easier to understand, that user/1 in data is not the request user user/1.

r-peschke · 2024-08-26T09:14:55Z

tests/system/action/meeting/test_import.py

+            }
+        }
+        data["meeting"]["user"]["1"]["gender"] = "needs_to_be_created"
+        data["meeting"]["user"] = data["meeting"]["user"] | other_user_request_data


This part is missing for user/2 data, see above.
I would prefer the dict.update-notation for easier understanding. Use the same way as you will for user/2

r-peschke · 2024-08-26T09:47:48Z

tests/system/action/user/test_save_saml_account.py

@@ -12,7 +12,7 @@ def setUp(self) -> None:
            "first_name": "Max",
            "last_name": "Mustermann",
            "email": "[email protected]",
-            "gender": "male",
+            "gender_id": 1,


Seem that the whole structure self.result isn't used anymore. Than you could delete it.

r-peschke · 2024-08-26T10:00:48Z

openslides_backend/action/actions/user/save_saml_account.py

@@ -83,11 +84,26 @@ def validate_instance(self, instance: dict[str, Any]) -> None:
                    ]
                }
                for model_field, payload_field in self.saml_attr_mapping.items()
-                if model_field in allowed_user_fields
+                # handle only allowed fields. handle gender seperatly since it needs conversion to id


typo: separately

r-peschke · 2024-08-26T10:51:33Z

openslides_backend/action/actions/meeting/import_.py

+        self.user_id_to_gender = {}
+        users = instance["meeting"].get("user", {})
+        for user in users.values():
+            if gender := user.get("gender"):


What do you think about following code:

if "gender" in user: if gender := user["gender"]: self.user_id_to_gender[user["id"]] = gender del user["gender"]

Your way you could get None if gender was set to it or if gender doesn't exist in dict

I like the new way even better, because it's leaner.

r-peschke · 2024-08-26T11:44:45Z

openslides_backend/action/actions/meeting/import_.py

+                if gender in gender_dict:
+                    gender_id = gender_dict[gender]
+                else:
+                    action_result = self.execute_other_action(


You have to add a new key to your gender_dict, otherwise you'll execute the create_gender action for all users with the same gender

r-peschke · 2024-08-28T07:12:24Z

Please fix the error and merge the main branch into the pull request

r-peschke

Just 2 little things in documentation.
I'll try to solve the user update problem.

r-peschke · 2024-09-02T13:46:21Z

docs/actions/gender.delete.md

+```
+
+## Action
+Deletes the gender.


Add some text like The first 4 gender names are fixed and cannot be changed or delete.

r-peschke · 2024-09-02T13:46:49Z

docs/actions/gender.update.md

+```
+
+## Action
+Updates the gender if a gender with that name doesn't exist.


Also add some text like The first 4 gender names are fixed and cannot be changed or delete

…e import without any new genders would result in exception.

hjanott · 2024-09-05T16:00:57Z

I'll try to solve the user update problem.

Thank you. That was really helpful. I didn't think of adding it to the org like this.

reiterl · 2024-09-11T06:31:05Z

Notice: Needs to be updated, to work with my pr in client.

hjanott added 2 commits June 21, 2024 14:55

draft create update delete + tests + testupdates + migration

3e4b85d

update meta rep

36f1d70

hjanott added feature migration Introduces a new migration labels Jun 21, 2024

hjanott added this to the 4.2 milestone Jun 21, 2024

hjanott requested a review from r-peschke June 21, 2024 13:18

hjanott assigned hjanott and r-peschke Jun 21, 2024

hjanott marked this pull request as ready for review June 21, 2024 13:57

hjanott marked this pull request as draft June 21, 2024 14:17

hjanott added 7 commits June 21, 2024 16:35

general error removal and improvement for draft

a025ba2

remove meeting import error

275aed2

update meeting import so that checker removes gender_id

9a79a5e

general code improvements delete gender checks for back relation in o…

42b63b6

…rganiziation

md documentation + pleasing mypy

b4f9e8f

merge upstream main

9162ee6

Merge commit 'b1d1e6fd' into 2170_introduce_gender_model

6e74b9c

r-peschke requested changes Jun 26, 2024

View reviewed changes

hjanott added 3 commits June 26, 2024 17:56

use defaultdict

8dfbd80

use new in memory flag of datastore

c356c42

fix mypy error by upping datastore version

468f9eb

Elblinator reviewed Jun 27, 2024

View reviewed changes

hjanott added 3 commits June 27, 2024 13:20

change the permission from can_manage_organization to can_manage_users

87dfb9e

improve documentation

3aec96d

lock result false + no orga check

21addc2

r-peschke mentioned this pull request Jul 5, 2024

remove statutes (amendments) #2506

Open

add test for gender import

443cb58

r-peschke requested changes Jul 5, 2024

View reviewed changes

make name and org id required

773582b

hjanott added 3 commits August 9, 2024 17:59

merge main by adding gender stuff (and locked_out in merge_mixin)

8be71fe

update from upstream/main

abfbefd

add test update gender on user merge.

35f739f

r-peschke requested changes Aug 13, 2024

View reviewed changes

r-peschke requested changes Aug 14, 2024

View reviewed changes

hjanott added 4 commits August 16, 2024 12:51

update docs test for meeting import and saml account (+ new test) cre…

fe3e77e

…ate gender for saml accounts

Merge remote-tracking branch 'upstream/main' into 2170_introduce_gend…

8ac6300

…er_model

updated meta

6b9d772

meeting import create user with gender. export gender strings. improv…

a9341b0

…e tests.

hjanott requested a review from r-peschke August 21, 2024 07:31

r-peschke requested changes Aug 21, 2024

View reviewed changes

hjanott added 2 commits August 21, 2024 18:44

Separate test for gender on meeting import. General code improvements…

4633eed

… in saml, export, import and migration plus additional test for empty string.

merge upstream main and generate models

2641a8e

hjanott requested a review from r-peschke August 22, 2024 08:48

r-peschke requested changes Aug 26, 2024

View reviewed changes

extend test for gender import, plus fixes. docs update

7054aed

hjanott requested a review from r-peschke August 27, 2024 12:40

r-peschke removed their assignment Aug 28, 2024

update meta repo

92fa165

r-peschke requested changes Sep 3, 2024

View reviewed changes

r-peschke and others added 2 commits September 4, 2024 14:01

improve user and gender updates/creation

5b26bf1

Gender will not be created without being used anymore. Fix error wher…

20b6d5c

…e import without any new genders would result in exception.

hjanott requested a review from r-peschke September 5, 2024 15:55

Improve gender action documentation

ee2cff4

hjanott assigned r-peschke Sep 6, 2024

fixed typo in migration gender female

033ebde

reiterl mentioned this pull request Sep 11, 2024

Customizable gender field OpenSlides/openslides-client#4027

Open

	- Search for users and if username, first-name, last-name and email are identical use this existing user instead of creating a duplicate. Keep the data, including password, of the existing user. Relavent relations such as to the meeting will be updated though.
	- Search for users and if username, first-name, last-name and email are identical use this existing user instead of creating a duplicate. Keep the data, including password, of the existing user. Relevant relations such as to the meeting will be updated though.

2170 introduce gender model #2485

Are you sure you want to change the base?

2170 introduce gender model #2485

Conversation

hjanott commented Jun 21, 2024 • edited Loading

r-peschke left a comment

Choose a reason for hiding this comment

Elblinator left a comment

Choose a reason for hiding this comment

hjanott commented Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hjanott Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hjanott Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-peschke commented Aug 28, 2024

r-peschke left a comment

Choose a reason for hiding this comment

hjanott commented Jun 21, 2024 •

edited

Loading

hjanott commented Jun 27, 2024 •

edited

Loading

hjanott Jul 31, 2024 •

edited

Loading

hjanott Jul 30, 2024 •

edited

Loading