Map gender fields on CSV import #2401

k-nut · 2024-12-02T12:06:54Z

Description

This PR allows users to properly map the gender fields form their CSV data to the gender definition that Zetkin understands

Screenshots

[Add screenshots here]

Changes

Inspired by the enum and org mappings, this adds config and preview components for the gender field.

Notes to reviewer

The null handling is pretty ugly right now (just replacing null with "null" so that it can also be used in translations). I'm also unsure how it should generally be handled and how much translations should be duplicated. Any input is appreciated.

Related issues

Resolves #2348

richardolsson · 2024-12-02T12:54:18Z

@ziggabyte can you have a look at this and provide some feedback to @k-nut? Especially around the handling of null, which is an allowed value for gender but difficult to use as value in components.

ziggabyte · 2024-12-02T17:50:07Z

Hi @k-nut ! :D So glad you have taken on this task, it's been very needed!

I've played with the deployed version of the code, and also experimented around a bit in your code to give some feedback.

I agree with you that the handling of null as "null" needs a bit of work. My thought is that basically, there are three gender options in the Zetkin universe, OR null, which is not a gender option but a value the gender field can have when you don't know the gender of the person. So in that line of thought I think the experimental and interesting null-as-a-string version, could be replaced by making the type Gender contain only the strings f, m and o and then typing things to be Gender | null:

This means the Select in GenderMappingRow would look like this (adding an option with the value unknown which represents setting the value to null)

Does this approach make sense to you?

k-nut · 2024-12-05T21:26:26Z

Thank you for your thoughts. I pushed a version inspired by your proposal, let me know what you think!

richardolsson · 2024-12-07T10:23:20Z

This PR is still in draft mode, but I think it's ready for review so I'm going to mark it as such. Let me know if that was a mistake and we can change it back.

k-nut · 2024-12-07T11:14:30Z

Oh yes, I forgot. Thank you!

ziggabyte

Hi! :D Looking nice with some updates! I have given a bit of a more detailed reading of the code and some feedback, if something does not seem right please write your thoughts and we can discuss it!

I also noticed a little bit of logic is missing, that you need to "hook up to" by including GENDER as a configurable column in this array in MappingRow.tsx. This will show a button to open the config if you wanna get back to it after going to different fields, and also have a warning message if the mapping is unfinished:

ziggabyte · 2024-12-09T09:09:26Z

src/features/import/components/ImportDialog/Configure/Configuration/GenderConfigRow.tsx

+          {!showSelect && (
+            <Button onClick={() => setMapping(true)}>
+              <Msg
+                id={
+                  messageIds.configuration.configure.genders
+                    .showGenderSelectButton
+                }
+              />
+            </Button>
+          )}
+          {showSelect && (


This logic, to have a button in place of the Select, was created in the organization mapping because loading and rendering each Select with organisations took a really really long time. So this button was placed there to only load/render each select "on demand". With genders, since no loading of data is happening and it's a small set of values, i think it would be nice to remove the button and just show the select!

Nice, I thought this was for a nicer UI. Removed 🧹

ziggabyte · 2024-12-09T09:23:14Z

src/features/import/components/ImportDialog/Configure/Configuration/GenderConfigRow.tsx

+                      // selecting `unknown` is like deselecting
+                      onDeselectGender();
+                    }


I am not sure I agree (and I might very well be wrong!) that it should be the same to deselect as to select to import a value as unknown (null).

What I mean is, that when selecting unknown, I want there to be information added to my object of mapping that says "value x should be mapped to null". But deselecting means removing the information related to "value x" in my object of mapping. This comes with the undesired side effect that if I open the gender mapping, and the first value i want to map i select should map to "unknown" - this is not recognised as me having done any mapping at all. Our validation for the mapping will believe no mapping for gender has happened, and it will be forbidden to move on to the validation step.

What do you think?

I might be misunderstanding, so please ignore if does not make sense.

I believe that another aspect of this decision is what happens to people for where there is no mapping in the import, but do have gender data in the database.

Not mapping anything doesn't change them in the database (the data object will not include gender), while mapping to "unknown" will overwrite whatever is already in the database (setting data.gender to null).

So IMO not mapping, and mapping to unknown is two separate actions where the former is a no-op and the latter is destructive.

I changed the implementation to something that should now work for any of the "regular" genders, a deliberate "we want to set this to null" and a "ignore this value in the import"

ziggabyte · 2024-12-09T09:25:24Z

src/features/import/components/ImportDialog/Configure/Preview/GenderPreview.tsx

+      <PreviewGrid columnHeader={'Gender'} rowValue={null} unmappedRow={true} />
+    );
+  }
+
+  const value = fields?.[fieldKey];
+  if (value !== 'm' && value !== 'f' && value !== 'o' && value !== 'null') {
+    return (
+      <PreviewGrid
+        columnHeader={messages.configuration.preview.columnHeader.gender()}
+        emptyLabel={messages.configuration.preview.noValue()}
+        rowValue={null}
+        unmappedRow={false}
+      />
+    );
+  }
+  return (
+    <PreviewGrid
+      columnHeader={'Gender'}


I think columnHeader needs to be localised in all the places :)

Good catch. Added 👍

ziggabyte · 2024-12-09T09:27:13Z

src/features/import/hooks/useGenderMapping.ts

+    // Check if there is already a map for this row value to an org ID
+    const map = column.mapping.find((map) => map.value == value);
+    // If no map for that value
+    if (!map) {
+      const newMap = { gender, value: value };
+      dispatch(
+        // Add value to mapping for the column
+        columnUpdate([
+          columnIndex,
+          {
+            ...column,
+            mapping: [...column.mapping, newMap],
+          },
+        ])
+      );
+    } else {
+      // If there is already a map, replace it
+      // Find mappings that are not for this row value
+      const filteredMapping = column.mapping.filter((m) => m.value != value);
+      // New orgId for that row value
+      const updatedMap = { ...map, gender };


Some little copy-paste mistakes here in the comments, do you wanna updated them to relate to gender mapping and not org mapping? :)

I think this is a perfect example of the reasons why I try to avoid comments. They exist to make the code easier to read, but it's so easy to make mistakes like this where the code actually becomes more confusing because of the comment.

Generally, if I feel like a comment is needed, I will try to rewrite the code (and especially name things) so that the code itself becomes easier to read and the comment isn't needed.

I think the code was actually way too complicated for what it was doing. I changed the implementation. Let me know what you think!

ziggabyte · 2024-12-09T09:35:34Z

src/features/import/l10n/messageIds.ts

+      genders: {
+        gender: m('Gender'),
+        genders: {
+          f: m('female'),
+          m: m('male'),
+          null: m('unspecified'),
+          o: m('other'),
+        },
+        header: m('Gender'),


Here is both a key called header, and gender on the same level translated to the same thing, "Gender". I'm thinking that both could be using the same one - where I prefer header since it communicates better what the transaltion is needed for?

Also an object strucutre thing - genders.genders does not feel like the optimal naming, could the inner object maybe be called "genderSelectLabels" or something like that, to more communicate what they are used for?

I also think the genders need to have uppercase in the first letter to look nice in the Select

In other parts of the code we use the word "unknown" instead of "unspecified", I think this could be good for consistency.

Good call, I've changed them to be more descriptive I hope.

ziggabyte

Noticed only a small thing when playing around with it right now - that the values in the select are still all lower case :)

ziggabyte · 2024-12-09T14:34:01Z

src/features/import/l10n/messageIds.ts

+          f: m('female'),
+          m: m('male'),
+          o: m('other'),
+          unknown: m('unspecified'),


Tiny thing here - still in need of uppercase on the options, and I think "unspecified" should be changed to "unknown" as the value! :D

Oops, it did feel like I was forgetting something 🙈

ziggabyte · 2024-12-09T14:34:16Z

src/features/import/l10n/messageIds.ts

+        f: m('female'),
+        m: m('male'),
+        o: m('other'),
+        unknown: m('unspecified'),


Also changed ✅

ziggabyte

Playing with this feels like it behaves the way i expect it to! I'm very happy that you added some very neat simplifications to the code that I wrote for org mapping, it's super fun to have you on board contributing :D

Giving you the seal of approval: 🦭 !

Map gender fields on CSV import

a68de34

vercel bot deployed to Preview – gen3-storybook December 2, 2024 12:08 View deployment

richardolsson requested a review from ziggabyte December 2, 2024 12:49

vercel bot deployed to Preview – app-zetkin-org December 2, 2024 12:57 View deployment

Use "unknown" for null gender

61945de

vercel bot deployed to Preview – gen3-storybook December 5, 2024 21:10 View deployment

vercel bot deployed to Preview – app-zetkin-org December 5, 2024 21:16 View deployment

Add actual gender mapping for import

298066a

vercel bot deployed to Preview – gen3-storybook December 5, 2024 21:27 View deployment

vercel bot deployed to Preview – app-zetkin-org December 5, 2024 21:33 View deployment

richardolsson marked this pull request as ready for review December 7, 2024 10:23

ziggabyte requested changes Dec 9, 2024

View reviewed changes

k-nut added 3 commits December 9, 2024 14:45

Add gender as a mapping row

0a99f28

Improve how null values are handled

63a1471

Improve translation keys

2c6e82d

k-nut requested a review from ziggabyte December 9, 2024 13:56

vercel bot deployed to Preview – app-zetkin-org December 9, 2024 14:04 View deployment

vercel bot deployed to Preview – gen3-storybook December 9, 2024 14:07 View deployment

ziggabyte requested changes Dec 9, 2024

View reviewed changes

Update translations

066cb5b

k-nut requested a review from ziggabyte December 9, 2024 16:05

vercel bot deployed to Preview – gen3-storybook December 9, 2024 16:06 View deployment

vercel bot deployed to Preview – app-zetkin-org December 9, 2024 16:10 View deployment

ziggabyte approved these changes Dec 10, 2024

View reviewed changes

ziggabyte merged commit 2c0177a into zetkin:main Dec 10, 2024
6 checks passed

richardolsson mentioned this pull request Dec 20, 2024

Gender can be mapped from multiple columns #2437

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Map gender fields on CSV import #2401

Map gender fields on CSV import #2401

k-nut commented Dec 2, 2024

richardolsson commented Dec 2, 2024

ziggabyte commented Dec 2, 2024 •

edited

Loading

k-nut commented Dec 5, 2024

richardolsson commented Dec 7, 2024

k-nut commented Dec 7, 2024

ziggabyte left a comment

ziggabyte Dec 9, 2024

k-nut Dec 9, 2024

ziggabyte Dec 9, 2024

richardolsson Dec 9, 2024

k-nut Dec 9, 2024

ziggabyte Dec 9, 2024

k-nut Dec 9, 2024

ziggabyte Dec 9, 2024

richardolsson Dec 9, 2024

k-nut Dec 9, 2024

ziggabyte Dec 9, 2024

k-nut Dec 9, 2024

ziggabyte left a comment

ziggabyte Dec 9, 2024

k-nut Dec 9, 2024

ziggabyte Dec 9, 2024

k-nut Dec 9, 2024

ziggabyte left a comment

Map gender fields on CSV import #2401

Map gender fields on CSV import #2401

Conversation

k-nut commented Dec 2, 2024

Description

Screenshots

Changes

Notes to reviewer

Related issues

richardolsson commented Dec 2, 2024

ziggabyte commented Dec 2, 2024 • edited Loading

k-nut commented Dec 5, 2024

richardolsson commented Dec 7, 2024

k-nut commented Dec 7, 2024

ziggabyte left a 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

ziggabyte left a 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

ziggabyte left a comment

Choose a reason for hiding this comment

ziggabyte commented Dec 2, 2024 •

edited

Loading