OBPIH-6108 Default Preference Type section (fixes after QA) by drodzewicz · Pull Request #4536 · openboxes/openboxes

drodzewicz · 2024-03-08T09:45:43Z

There is a bit of a refactor that I did on the validation schema that allowed me to add a custom refine() validation on the whole section.
Additionally, I believe that separating our validations schema into smaller subsections will be a lot more readable and maintainable. I believe we already talked about this and since there aren't any tickets that will have conflicts with these changes Id decided to proceed with this refactor.

So what has changed?

I have split all of the fields in the validations schema into smaller subsections just like we have in the UI and imported them in the main schema

 z
      .object({
        id: z
          .string()
          .optional(),
        basicDetails: basicDetailsSchema,
        additionalDetails: additionalDetailsSchema,
        defaultPreferenceType: defaultPreferenceTypeSchema,
        productSupplierPreferences: z.array(productSupplierPreferenceSchema),
        packageSpecification: packageSpecificationSchema,
        fixedPrice: fixedPriceSchema,
        attributes: z.object(attributesValidationSchema),
      });

Because of these changes I have modified getProductSource, submitted payload and field name attributes accordingly.
I have created a generic FormErrorPropType which is repeated in all of our error props.
I have also decided to export the whole errors propType object from subsections and import it in the sections, to avoid any propType inconsistencies and code repetition when defining prop types.
Modified onClear function for the date - it looks like we have to assign undefined instead of null because we have .coerce.date() defined on validation schema for date which parses null to some date value like 1970
I have also moved out logic for calculating each price into a separate hook useCalculateEachPrice which I believe will declutter the main sources form hook

…preference sections has changed

alannadolny · 2024-03-08T10:24:11Z

src/js/components/productSupplier/create/subsections/AdditionalDetails.jsx

 export default AdditionalDetails;

+export const additionalDetailsFormErrors = PropTypes.shape({


Eslint doesn't shout at it? I remember there was a restriction about using default export and normal export in one file

I had no such lint errors? Do we have this rule in our eslint config?

alannadolny · 2024-03-08T10:30:08Z

src/js/hooks/productSupplier/form/useProductSupplierForm.js

+      ...omitEmptyValues(values.basicDetails),
+      ...omitEmptyValues(values.additionalDetails),
+      ...omitEmptyValues(values.defaultPreferenceType),
+      ...omitEmptyValues(values.packageSpecification),
+      ...omitEmptyValues(values.fixedPrice),


It could be better to use object restructuring. The value is repeated many times... but as I know the onSubmit will be refactored, so I don't know if this change is needed at this point.

I agree with @alannadolny and was about to point out the same thing, but anyway this method will be refactored, so there is no need to spend time on that now imho.

alannadolny · 2024-03-08T10:35:01Z

src/js/utils/propTypes.js

+import PropTypes from 'prop-types';
+
+// eslint-disable-next-line import/prefer-default-export
+export const FormErrorPropType = PropTypes.shape({
+  message: PropTypes.string,
+});


so if this file will be used for defining our custom propTypes, you can use default export like that:

Suggested change

import PropTypes from 'prop-types';

// eslint-disable-next-line import/prefer-default-export

export const FormErrorPropType = PropTypes.shape({

message: PropTypes.string,

});

import PropTypes from 'prop-types';

const FormErrorPropType = PropTypes.shape({

message: PropTypes.string,

});

export default propTypes = {

// Add here newly created custom propTypes

FormErrorPropType,

};

this was intentional from my side.
The above example with default export will not get optimized by webpack treeshaking algorithm which should omit the unused code from being included in the bundle.
So in such cases, it is best to export it individually.

src/js/hooks/productSupplier/form/useProductSupplierForm.js

kchelstowski · 2024-03-08T10:42:46Z

src/js/hooks/productSupplier/form/useProductSupplierForm.js

+  const defaultPreferenceType = useWatch({ control, name: 'defaultPreferenceType' });
+  useEffect(() => {
+    trigger('defaultPreferenceType.preferenceType');
+  }, [defaultPreferenceType]);


I don't know if it's expected, but if defaultPreferenceType is an object, it would trigger the useEffect whenever the component re-renders, not when the defaultPreferenceType changes. It is because objects are compared by reference in JS, not by its values.
I think currently it might run more times than we would expect it to run.

this was the point, we want to trigger this validation when any of the values in the default preference type subsection changes.
I tested dit with console logs on this useEffect and everything seems to work as expected.

I know, but I meant that even if none values change, and something else triggers the re-render, this useEffect would anyway be triggered, because a reference of the object would change.

kchelstowski · 2024-03-08T10:43:29Z

src/js/hooks/productSupplier/form/useProductSupplierForm.js

+      ...omitEmptyValues(values.basicDetails),
+      ...omitEmptyValues(values.additionalDetails),
+      ...omitEmptyValues(values.defaultPreferenceType),
+      ...omitEmptyValues(values.packageSpecification),
+      ...omitEmptyValues(values.fixedPrice),


I agree with @alannadolny and was about to point out the same thing, but anyway this method will be refactored, so there is no need to spend time on that now imho.

src/js/hooks/productSupplier/form/useProductSupplierValidation.js

kchelstowski · 2024-03-08T10:47:48Z

src/js/hooks/productSupplier/form/useProductSupplierValidation.js

+      ) {
+        return !!subsectionData.preferenceType;
+      }
+      return true;


I kinda don't get what it does. I thought that it should return true (so that it requires the preferenceType to be chosen) when any of other values are specified.
So wouldn't this line mean, that if none of other values are specified, we are still requiring the preference type to be chosen?

if any other field is filled in then preferenceType is required.

the boolean values return true if field is valid and false if it is invalid, which means that

true - field is not required (everything is alright) false - field should be required (validation failed)

could you add this comment above the method? I think the name requirePreferenceType suggests that if it returns true, then it means that we require a preference type.

added comments and changed the name, I hope there is no more confusion

src/js/hooks/productSupplier/form/useProductSupplierValidation.js

kchelstowski · 2024-03-08T10:51:02Z

A general comment is that I feel uncomfortable with such refactors at the end of the feature development. There were many edge cases where something in schema was/was not needed or it was intentionally done as null or '' instead of undefined etc.
I hope no regression issues will come up, but yeah...

- added comments and renamed validation function for Preference type - moved validation trigger to onBlur on other fields rather than useEffect

* OBPIH-6108 Refactor validation schema sections into separate schemas * OBPIH-6108 export error types from subsections * OBPIH-6108 Move Each price calculation into separate hook * OBPIH-6108 trigger validation on preference type fields when default preference sections has changed * OBPIH-6108 refactor default values for product sources create form * OBPIH-6108 Fix issue with clearing default prefference type select * OBPIH-6108 Fixes after review - added comments and renamed validation function for Preference type - moved validation trigger to onBlur on other fields rather than useEffect

drodzewicz added 6 commits March 7, 2024 17:10

OBPIH-6108 Refactor validation schema sections into separate schemas

54668d8

OBPIH-6108 export error types from subsections

5541892

OBPIH-6108 Move Each price calculation into separate hook

9b0ab7f

OBPIH-6108 trigger validation on preference type fields when default …

07bada0

…preference sections has changed

OBPIH-6108 refactor default values for product sources create form

4c8a967

OBPIH-6108 Fix issue with clearing default prefference type select

2dabff0

drodzewicz requested review from alannadolny, awalkowiak and kchelstowski March 8, 2024 09:45

drodzewicz self-assigned this Mar 8, 2024

awalkowiak approved these changes Mar 8, 2024

View reviewed changes

alannadolny reviewed Mar 8, 2024

View reviewed changes

kchelstowski reviewed Mar 8, 2024

View reviewed changes

alannadolny approved these changes Mar 8, 2024

View reviewed changes

OBPIH-6108 Fixes after review

cdc6bc4

- added comments and renamed validation function for Preference type - moved validation trigger to onBlur on other fields rather than useEffect

drodzewicz force-pushed the OBPIH-6108-add-preference-type-section-with-inputs branch from 5ed5d6c to cdc6bc4 Compare March 8, 2024 13:46

drodzewicz requested a review from kchelstowski March 8, 2024 13:46

kchelstowski approved these changes Mar 8, 2024

View reviewed changes

kchelstowski merged commit 2f932bc into feature/product-supplier-list-redesign Mar 8, 2024

kchelstowski deleted the OBPIH-6108-add-preference-type-section-with-inputs branch March 8, 2024 13:54

		export default AdditionalDetails;

		export const additionalDetailsFormErrors = PropTypes.shape({

Uh oh!

Conversation

drodzewicz commented Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kchelstowski commented Mar 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

drodzewicz commented Mar 8, 2024 •

edited

Loading