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

fix: [DHIS2-17854] validate the assigned values from rules engine #3783

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

simonadomnisoru
Copy link
Contributor

@simonadomnisoru simonadomnisoru commented Sep 3, 2024

DHIS2-17854

Tech summary

  • extracted the validateField logic from the FormBuilder.component into an external file.
  • added a function, validateAssignEffects, that also calls validateField. The common function runs every time the rules engine finishes executing and before the assigned values are stored in Redux. To improve performance, only the last assigned value is validated, since it is the one stored in the redux formsValues key.
  • switchMap, from() and of() were used in the epics to handle the async validation checks
  • to prevent the user from clicking save/complete while an async validation is running:
    • In forms that rely on withSaveHandler.js, depending on the form logic, the actions startLoadDataEntry, startRunRulesPostUpdateField, and startRunRulesPostLoadDataEntry will be dispatched before running the validations. These actions will add an entry in dataEntriesInProgressList that will show the waitForPromisesDialogOpen modal if necessary.
    • In the WidgetProfile that does not rely on withSaveHandler.js, the Save button will be disabled

@simonadomnisoru simonadomnisoru marked this pull request as ready for review September 3, 2024 09:02
@simonadomnisoru simonadomnisoru requested a review from a team as a code owner September 3, 2024 09:02
@simonadomnisoru simonadomnisoru marked this pull request as draft September 3, 2024 12:50
@simonadomnisoru simonadomnisoru marked this pull request as ready for review September 4, 2024 08:13
@simonadomnisoru simonadomnisoru marked this pull request as draft October 2, 2024 13:50
@simonadomnisoru simonadomnisoru marked this pull request as ready for review October 3, 2024 08:25
Co-authored-by: Tony Valle <79843014+superskip@users.noreply.github.com>
Copy link

github-actions bot commented Oct 3, 2024

Copy link
Member

@JoakimSM JoakimSM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have made a few structural suggestions that I hope is reasonable. Open to discuss if you disagree of course.

I'm also wondering, since we are dealing with potential async validations here, what are we doing to prevent the user from clicking save/complete while an async validation is running? I couldn't see this being handled properly, but I very much might have missed something.

Copy link
Member

@JoakimSM JoakimSM Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this file to capture-core/utils/validation (new directory) and rename to validateValue.js (and update the function to validateValue). You will also have to move getValidators.js etc.

In addition, move capture-core/utils/validators into the folder created above (capture-core/utils/validation/validators...)

};

export const validateField = async (
{ validators }: { validators?: Array<ValidatorContainer> },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change the input here to only be the validators and not use object destructering.

{ validators }: { validators?: Array<ValidatorContainer> },
value: any,
validationContext: ?Object,
onIsValidatingInternal: ?Function,
Copy link
Member

@JoakimSM JoakimSM Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hard to find a good name for this one (onIsValidatingInternal). Can we change the name of this callback to postProcessAsyncValidatonInitiation? Quite a mouthful, I know, but would like to have something a bit more describing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this file to capture-core/rules?

@simonadomnisoru simonadomnisoru marked this pull request as draft October 10, 2024 13:37
@simonadomnisoru
Copy link
Contributor Author

I have made a few structural suggestions that I hope is reasonable. Open to discuss if you disagree of course.

I'm also wondering, since we are dealing with potential async validations here, what are we doing to prevent the user from > clicking save/complete while an async validation is running? I couldn't see this being handled properly, but I very much > might have missed something.

Hi @JoakimSM, I implemented the suggested structural changes.
Regarding preventing the user from clicking save/complete while an async validation is running, in withSaveHandler.js there is already logic for this handled by the waitForPromisesDialogOpen modal. There were only a few forms where the actions to enable/disable the waitForPromisesDialogOpen modal were missing and I added them. For WdigetProfile, the only form that does not rely on withSaveHandler.js, I added logic to disable/enable the Save button.

Can you have a look? Thank you!

@simonadomnisoru simonadomnisoru marked this pull request as ready for review October 24, 2024 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants