Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(de form): add field rules #363

Merged
merged 5 commits into from
Jan 16, 2024
Merged

feat(de form): add field rules #363

merged 5 commits into from
Jan 16, 2024

Conversation

Mohammer5
Copy link
Contributor

@Mohammer5 Mohammer5 commented Nov 15, 2023

This is a draft PR as I want to discuss moving validation from the form-level to the field-level (see last paragraph below).

  • Implements the field rules for the data element form
  • Tests
  • Docs

The old app uses a configuration to define the rules. I tried to follow a similar approach but aligned with how React works and realized that it adds a lot of complexity:

  • The Zod schema has to be created on the view-component level.
  • The category combo is optional when domainType === 'TRACKER'.
  • The disabled state would have to live in the views.
  • The Zod schema would have to be dynamic (category combo optional / required).
  • The disabled state would have to be passed down to the actual category combo field (either directly, with context, or React-external state like Zustand).

Instead, I've co-located everything with the actual field, which makes it somewhat simple because everything related to a particular field is in one place only. It doesn't require too much state (e.g., whether the category combo is disabled or not is simply values.domainType === 'TRACKER'). This approach won't yield a list of rules, which requires us to document which rules exist. I've done that in comments above the fields that have rules in fields.tsx.

I'm tempted to remove Zod and move validation to the field level. This will further untangle/simplify the architecture as field-level validator functions will be co-located with the fields in the code. The biggest advantage that Zod provides is that we don't have to duplicate types, but this doesn't apply to us as we already have generated types. Having the validators on the field level allows us to have very simple validation functions where necessary (see the new validateCategoryCombo function in fields.tsx) which reduces cognitive overhead. With the current approach (with Zod), you'll have to remember the schema itself, where the schema is located (dateElementsSchema.ts), how it's being applied (on RFF's <Form /> component), and where the form component is located (in the views). This will all be reduced to it's where the field is (in fields.tsx).

@Mohammer5 Mohammer5 requested a review from Birkbjo November 15, 2023 09:50
Copy link

netlify bot commented Nov 15, 2023

Deploy Preview for dhis2-maintenance-app-beta ready!

Name Link
🔨 Latest commit 4a034ba
🔍 Latest deploy log https://app.netlify.com/sites/dhis2-maintenance-app-beta/deploys/65a63ddba47cb7000840d7e0
😎 Deploy Preview https://deploy-preview-363--dhis2-maintenance-app-beta.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Mohammer5 Mohammer5 changed the base branch from master to DHIS2-14656-validation November 15, 2023 09:50
@Mohammer5 Mohammer5 force-pushed the fieldRules branch 2 times, most recently from 23555fc to c518123 Compare November 15, 2023 10:32
@Birkbjo
Copy link
Member

Birkbjo commented Nov 15, 2023

I'm tempted to remove Zod and move validation to the field level. This will further untangle/simplify the architecture as field-level validator functions will be co-located with the fields in the code. The biggest advantage that Zod provides is that we don't have to duplicate types, but this doesn't apply to us as we already have generated types. Having the validators on the field level allows us to have very simple validation functions where necessary

I see what you mean, but I do think there can be some value in having zod. The field-level rules could even just refer to those fields - so every validation of a form is co-located. We might also not need field-level validations, if the form-level is run regardless?

@Mohammer5 Mohammer5 force-pushed the DHIS2-14656-validation branch from 266ecd0 to aa62a70 Compare November 16, 2023 09:20
@Mohammer5 Mohammer5 force-pushed the fieldRules branch 2 times, most recently from ac4865f to d1091ae Compare November 22, 2023 10:17
@codecov-commenter
Copy link

codecov-commenter commented Nov 22, 2023

Codecov Report

Attention: 47 lines in your changes are missing coverage. Please review.

Comparison is base (0fecea2) 72.44% compared to head (4a034ba) 74.50%.

Files Patch % Lines
src/lib/optionSet/useOptionSetQuery.ts 30.00% 7 Missing ⚠️
...ges/dataElements/fields/AggregationLevelsField.tsx 56.25% 7 Missing ⚠️
...ages/dataElements/fields/OptionSetCommentField.tsx 60.00% 6 Missing ⚠️
src/pages/dataElements/fields/OptionSetField.tsx 60.00% 6 Missing ⚠️
src/pages/dataElements/fields/LegendSetField.tsx 64.28% 5 Missing ⚠️
...c/pages/dataElements/fields/CategoryComboField.tsx 86.20% 4 Missing ⚠️
...rc/pages/dataElements/fields/ColorAndIconField.tsx 50.00% 4 Missing ⚠️
src/pages/dataElements/form/validate.ts 76.92% 3 Missing ⚠️
src/lib/models/useIsFieldValueUnique.ts 88.88% 2 Missing ⚠️
...mControls/OptionSetSelect/useInitialOptionQuery.ts 66.66% 1 Missing ⚠️
... and 2 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #363      +/-   ##
==========================================
+ Coverage   72.44%   74.50%   +2.06%     
==========================================
  Files         107      127      +20     
  Lines        1691     1781      +90     
  Branches      318      342      +24     
==========================================
+ Hits         1225     1327     +102     
+ Misses        463      451      -12     
  Partials        3        3              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Mohammer5 Mohammer5 marked this pull request as ready for review November 22, 2023 10:30
@Mohammer5 Mohammer5 changed the title Field rules feat(de form): add field rules Nov 22, 2023
Base automatically changed from DHIS2-14656-validation to master November 22, 2023 12:21
@Mohammer5
Copy link
Contributor Author

We've decided to remove validation / require-checks from the zod schema and move that to the field level.
Zod is now being used to verify the shape as well as do post-submit transformations like trimming.

@Mohammer5 Mohammer5 force-pushed the fieldRules branch 2 times, most recently from 561fa30 to a3195b4 Compare November 23, 2023 14:10
@Mohammer5
Copy link
Contributor Author

@Birkbjo nice finds! I've implement all change requests!

@Mohammer5 Mohammer5 requested a review from Birkbjo January 8, 2024 09:26
@Mohammer5 Mohammer5 merged commit 30bd07a into master Jan 16, 2024
9 checks passed
@Mohammer5 Mohammer5 deleted the fieldRules branch January 16, 2024 08:48
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants