Enum custom field UI #2210

niklasva82 · 2024-09-29T10:59:46Z

Description

This PR adds UI for enum_text type custom fields.

Screenshots

Changes

Maps key value to label value in person details
Select box to select option in edit person modal
Adds mapping between values and options in import configuration
Adds possibility of choosing enum field option in smart search
Adds view column for enum fields, mapping key to option label

Notes to reviewer

This requires backend PR: zetkin/zetkin-platform#848

…m type.

richardolsson

I would love to try this but the preview build is failing. 😞

Please fix all of the linting and formatting errors before a more thorough review.

richardolsson · 2024-10-03T02:52:20Z

Please install the pre-commit hook, as suggested in the CONTRIBUTING.md file, to avoid committing code with formatting and type issues.

niklasva82 · 2024-10-03T22:29:53Z

Please install the pre-commit hook, as suggested in the CONTRIBUTING.md file, to avoid committing code with formatting and type issues.

Yes done now

…n renamed to PersonFieldColumn, it was only used by PersonField.

richardolsson

I've taken an initial look, mostly focusing on the functional side (using the UI and seeing how it works). In that process I did find some problems that I've highlighted below. I will move on to do a full code review tomorrow but didn't want to hold you up if you do keep on working today.

src/features/views/components/ViewDataTable/columnTypes/PersonFieldColumnType.tsx

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

src/features/smartSearch/components/filters/PersonField/DisplayPersonField.tsx

src/utils/types/zetkin.ts

richardolsson

Awesome work on this major new feature! I have tried a local build (with some changes to match recent changes to the backend) and I have reviewed all the code. Most of it works really well! 💯

I did find some bugs, and some things that I think can be improved in the code, as well as a bunch of things where I don't feel entirely comfortable but can definitely be persuaded if I hear your reasoning. Please have a read and reach out if you want to discuss.

src/features/import/l10n/messageIds.ts

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

src/features/import/hooks/useColumn.ts

richardolsson · 2024-10-17T06:39:48Z

src/features/import/utils/createPreviewData.ts

+      if (column.kind === ColumnKind.ENUM) {
+        column.mapping.forEach((mappedColumn) => {
+          if (mappedColumn.value === row[colIdx]) {
+            personPreviewOp.data = {
+              ...personPreviewOp.data,
+              [`${column.field}`]: row[colIdx],
+            };
+          }
+        });
+      }


Please add a test case to createPreviewData.spec.ts that encompasses this. Maybe this code is related to the fact that the preview doesn't seem to be working (see separate comment) and you can try to reproduce that in a test and TDD a solution?

The test should verify that mapping empty values work as expected. Right now it does not seem to be working. The store looks ok after mapping an empty value, but the preview does not render anything. I'm not sure if it's here, or if it's the UI.

src/features/smartSearch/components/filters/PersonField/DisplayPersonField.tsx

src/features/smartSearch/l10n/messageIds.ts

src/features/views/components/ViewDataTable/columnTypes/PersonFieldColumnType.tsx

richardolsson

I commented in two places where "" is used and need to be translated to null. Not sure if there are more? How about Smart Search?

richardolsson · 2024-10-18T04:20:42Z

src/features/import/utils/prepareImportOperations.ts

+            if (mappedColumn.value === row.data[colIdx] && mappedColumn.key) {
+              personImportOps[rowIndex].data = {
+                ...personImportOps[rowIndex].data,
+                [`${column.field}`]: mappedColumn.key,


This should be translated to null if "" (could also be solved elsewhere, e.g. closer to the UI, but doing it here should make it testable).

richardolsson · 2024-10-18T04:22:18Z

src/features/profile/components/EditPersonDialog/EditPersonFields.tsx

+                  fullWidth
+                  label={field.title}
+                  onChange={(ev) => {
+                    onChange(field.slug, ev.target.value);


Here we need to translate from "" to null.

…EnumPreview.

richardolsson · 2024-10-18T14:22:06Z

src/features/views/components/ViewDataTable/columnTypes/PersonFieldColumnType.tsx

+      valueGetter: (params) => {
+        const cell = params.row[params.field];
+        if (column.config.enum_choices) {
+          this.enumChoices = column.config.enum_choices;


Setting this.enumChoices here and then relying on it in several other places feels risky. This instance is not for one specific column, it's for all columns of this type. So there is nothing guaranteeing (at least not type/syntax-wise) that this method will be called before, say, cellToString() or that this method can't be called for another method before calling cellToString().

Most likely this will fail for views that have more than one column of enum type.

richardolsson · 2024-10-18T14:28:36Z

src/features/views/components/ViewDataTable/columnTypes/PersonFieldColumnType.tsx

+    if (this.enumChoices) {
+      const choice = this.enumChoices.find((c) => c.key == cell);
+      return choice ? choice.label : '';


As far as I can tell, this never happens when exporting a CSV, because this code is executed on the backend where getColDef will have never been called, and that's where the this.enumChoices state is (riskily) set.

I'm thinking it makes sense to pass in column to these functions instead.

src/features/views/components/ViewDataTable/columnTypes/PersonFieldColumnType.tsx

richardolsson

I've gone over this again and I did find some additional bugs. Details below.

richardolsson · 2024-10-18T14:44:28Z

src/features/import/utils/prepareImportOperations.ts

+        if (column.kind === ColumnKind.ENUM) {
+          column.mapping.forEach((mappedColumn) => {
+            if (mappedColumn.value === row.data[colIdx] && mappedColumn.key) {
+              personImportOps[rowIndex].data = {
+                ...personImportOps[rowIndex].data,
+                [`${column.field}`]: mappedColumn.key,
+              };
+            }
+          });
+        }


A test should make sure that mapping empty values work as expected. Right now, it does not seem to be working. Configuring empty values using the UI yields the expected configuration in the store, but when submitted to the backend the mappings for "empty values" seem lost.

richardolsson · 2024-10-18T14:45:14Z

src/features/import/utils/createPreviewData.ts

+      if (column.kind === ColumnKind.ENUM) {
+        column.mapping.forEach((mappedColumn) => {
+          if (mappedColumn.value === row[colIdx]) {
+            personPreviewOp.data = {
+              ...personPreviewOp.data,
+              [`${column.field}`]: row[colIdx],
+            };
+          }
+        });
+      }


The test should verify that mapping empty values work as expected. Right now it does not seem to be working. The store looks ok after mapping an empty value, but the preview does not render anything. I'm not sure if it's here, or if it's the UI.

niklasva82 added 3 commits September 28, 2024 12:57

Update ZetkinCustomField type and associated types to reflect new enu…

26beb6c

…m type.

Map enum value (key) to proper label in PersonDetailsCard

04b859b

Edit person enum field select box

b0be7ed

niklasva82 changed the title ~~Undocumented/enum UI~~ Enum custom field UI Sep 29, 2024

richardolsson added the CCInt2024 label Sep 29, 2024

vercel bot had a problem deploying to Preview – app-zetkin-org September 29, 2024 11:25 Failure

vercel bot deployed to Preview – gen3-storybook September 29, 2024 11:26 View deployment

richardolsson requested changes Sep 29, 2024

View reviewed changes

Import with enum fields

43d4e6b

vercel bot had a problem deploying to Preview – app-zetkin-org October 2, 2024 21:08 Failure

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

Fix code formatting.

a2ffcdb

vercel bot deployed to Preview – gen3-storybook October 2, 2024 21:19 View deployment

vercel bot had a problem deploying to Preview – app-zetkin-org October 2, 2024 21:20 Failure

Add enum_text smart search

d326a04

vercel bot deployed to Preview – gen3-storybook October 3, 2024 22:25 View deployment

vercel bot had a problem deploying to Preview – app-zetkin-org October 3, 2024 22:27 Failure

Add missing type fixes

0a1d389

vercel bot deployed to Preview – gen3-storybook October 3, 2024 22:31 View deployment

vercel bot deployed to Preview – app-zetkin-org October 3, 2024 22:36 View deployment

Add mapping of enum choice keys to labels in view column. SimpleColum…

4367150

…n renamed to PersonFieldColumn, it was only used by PersonField.

richardolsson removed the CCInt2024 label Oct 15, 2024

vercel bot deployed to Preview – app-zetkin-org October 15, 2024 14:36 View deployment

vercel bot deployed to Preview – gen3-storybook October 15, 2024 14:38 View deployment

richardolsson requested changes Oct 16, 2024

View reviewed changes

richardolsson requested changes Oct 17, 2024

View reviewed changes

niklasva82 added 2 commits October 17, 2024 16:19

Update enum_text to enum

1d0dffb

Submit null instead of '' for empty enum value

e3e5469

niklasva82 added 4 commits October 17, 2024 21:08

Implement cellToString for PersonFeildColumnType

31d681a

Fix enum preview

9faecf9

Fix message

2f136fe

Use CUSTOM_FIELD_TYPE

c6f88b5

richardolsson requested changes Oct 18, 2024

View reviewed changes

niklasva82 added 3 commits October 18, 2024 12:55

Separate DATE and ENUM conditionals in useColumn

56adf2d

Add mapping button in EnumConfigRow

660e657

Remove enumChoices from Option and instead load the custom fields in …

d641dd6

…EnumPreview.

vercel bot deployed to Preview – gen3-storybook October 18, 2024 13:55 View deployment

vercel bot deployed to Preview – app-zetkin-org October 18, 2024 14:01 View deployment

richardolsson reviewed Oct 18, 2024

View reviewed changes

richardolsson requested changes Oct 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enum custom field UI #2210

Enum custom field UI #2210

niklasva82 commented Sep 29, 2024 •

edited by richardolsson

Loading

richardolsson left a comment

richardolsson commented Oct 3, 2024

niklasva82 commented Oct 3, 2024

richardolsson left a comment •

edited

Loading

richardolsson left a comment

richardolsson Oct 17, 2024

richardolsson Oct 18, 2024

richardolsson left a comment

richardolsson Oct 18, 2024

richardolsson Oct 18, 2024

richardolsson Oct 18, 2024

richardolsson Oct 18, 2024 •

edited

Loading

richardolsson left a comment

richardolsson Oct 18, 2024

richardolsson Oct 18, 2024

Enum custom field UI #2210

Are you sure you want to change the base?

Enum custom field UI #2210

Conversation

niklasva82 commented Sep 29, 2024 • edited by richardolsson Loading

Description

Screenshots

Changes

Notes to reviewer

richardolsson left a comment

Choose a reason for hiding this comment

richardolsson commented Oct 3, 2024

niklasva82 commented Oct 3, 2024

richardolsson left a comment • edited Loading

Choose a reason for hiding this comment

richardolsson left a comment

Choose a reason for hiding this comment

richardolsson Oct 17, 2024

Choose a reason for hiding this comment

richardolsson Oct 18, 2024

Choose a reason for hiding this comment

richardolsson left a comment

Choose a reason for hiding this comment

richardolsson Oct 18, 2024

Choose a reason for hiding this comment

richardolsson Oct 18, 2024

Choose a reason for hiding this comment

richardolsson Oct 18, 2024

Choose a reason for hiding this comment

richardolsson Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

richardolsson left a comment

Choose a reason for hiding this comment

richardolsson Oct 18, 2024

Choose a reason for hiding this comment

richardolsson Oct 18, 2024

Choose a reason for hiding this comment

niklasva82 commented Sep 29, 2024 •

edited by richardolsson

Loading

richardolsson left a comment •

edited

Loading

richardolsson Oct 18, 2024 •

edited

Loading