Skip to content

Conversation

@tomzemp
Copy link
Member

@tomzemp tomzemp commented Dec 11, 2025

Background

This adds an update ExpressionBuilder (with list of variables for insertion) into validation rules form. See ticket description https://dhis2.atlassian.net/browse/DHIS2-20474. Note that I have updated the acceptance criteria after some discussion with David and Joe about behaviour.

After:
image

Overall notes

  • there is bug with app-runtime which prevents us from using normal mutation here (fix: text payload for validationRules expression mutation app-runtime#1420). Once that is merged, we can update app-runtime and then clean up to not use a direct invocation of fetch api [Now fixed in app-runtime. I'm leaving this note here because this is why there is now a resolution for app-runtime: the version in app-shell does not match the app-runtime version we need, so need to resolve]
  • I've used/modified some of the code from the existing src/components/metadataFormControls/ExpressionBuilder, but I've decided to keep this implementation separate, so we can first review this implementation for validationRules and then can do some additional generalisation clean up when applying to indicators/program indicators [to address in separate PR]
  • the general idea for handling the different elements available for validation rules, indicators, program indicators can be seen in the ValidationRuleVariables.tsx file. E.g. the definition of validationRuleElementTypes. The idea is that we define custom components for the various element types and then define an array of the the element types that we want to use for the paritcular implementation of the expression builder. For now, I am just import validationRuleElementTypes in the VariableSelectionBox.tsx file, but when adapting for indicators/program indicators this will be updated to have logic to pull the correct array of element definitions.

UX notes

  • the initial accpentance criteria caused for validation on blur of the expression text area. Since validation is done via an api call, and since clicking on an element caused a blur of the text area, we ended up with a race condition if you then forced a blur/validation of the textarea (e.g. either the originally blurred or the updated value could resolve later and then the validation was not necessarily correct). We decided to implement manual validation for now, with the option to click "Apply" even if not validated (in this case, clicking "Apply" first validates the expression before actually applying). This might be improved in the future to use blur and manage race conditions, but we didn't think it was worth it for now.

Other potential improvements

David, Joe, and I discussed some potential ideas for improvement that we will not address at the moment:

  • validation on blur (see above)
  • filtering of data elements by value type / adding indicators (we weren't sure if certain data elements might not be relevant, or if indicators should be allowed). For now, we just replicate the behaviour of the current app, but we might want someone who is familiar with validation rules to review if there are improvements we could make here.
  • old app uses double click for insertion. Here we use single click, which means that double click inserts twice. We may want to add a debounce to stop a second click adding element twice (if there is a standard, minimal effort, way to do this)
  • there are a couple of points I've accepted from sonarqube, but I think they are minor points relating to accessibility. I've tried to make the navigation generally keyboard accessible.

@netlify
Copy link

netlify bot commented Dec 11, 2025

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

Name Link
🔨 Latest commit d12c755
🔍 Latest deploy log https://app.netlify.com/projects/dhis2-maintenance-app-beta/deploys/694ab4b809d9720008a4fb94
😎 Deploy Preview https://deploy-preview-719.maintenance-app-beta.netlify.dhis2.org
📱 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 project configuration.

@tomzemp tomzemp marked this pull request as ready for review December 17, 2025 14:32
@tomzemp tomzemp requested a review from a team December 17, 2025 14:33
>
{descriptionToShow || i18n.t('(No Value)')}
</div>
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

In the old expression builder we decided to incluse here an hidden field:

                <TextAreaFieldFF
                    dataTest={`formfields-${fieldName}`}
                    input={input}
                    meta={meta}
                    inputWidth="400px"
                    disabled
                    className={styles.expressionHiddenInput}
                />

This was to make sure the field would be present on form submission, so the behavior would act consistently to other fields (e.g. the field would be validated and validation text would show, for example if expression is required).
I think it might useful to add here as well?

}, [setIsEmpty, setValidationResponse])

useEffect(() => {
const performInitialValidation = async (s: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

would it not make sense to pull this put of tehe useEffect?

}

return (
<div className={styles.expressionListContainer}>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need here some sort of onClick={e => {e.preventDefault(); e.stopPropagation();}} because otherwise the parent onClick gets called and setSelectedElementType gets invoked

}

.elementListContainer {
background-color: white;
Copy link
Contributor

@flaminic flaminic Dec 23, 2025

Choose a reason for hiding this comment

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

Maybe a bit of padding bottom here as well?

padding-inline: var(--spacers-dp4);
padding-block-start: var(--spacers-dp4);
align-items: center;
font-size: 16px;
Copy link
Contributor

@flaminic flaminic Dec 23, 2025

Choose a reason for hiding this comment

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

Maybe a bit of padding top/bottom here?


return (
<>
<div className={styles.preliminarySelect}>
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe worth adding a box shadow here if ProgramJunction is not rendered, so it does not appear after a selection. but it's always there?

Copy link
Contributor

@flaminic flaminic left a comment

Choose a reason for hiding this comment

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

Thank you Tom! looks great. Left a few minor comments

@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
2 New issues
B Reliability Rating on New Code (required ≥ A)
1 New Bugs (required ≤ 0)
1 New Code Smells (required ≤ 0)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@tomzemp
Copy link
Member Author

tomzemp commented Dec 23, 2025

Thank you Tom! looks great. Left a few minor comments

Thanks! Also while I was implementing your comments, I noticed that the saved expression could be invalid, and in that case the form said "No expression", so I've updated that to say "Invalid expression" if validation of existing value fails. If nothing is actually there, it will say "No expression"; if something valid is there, it will show the actual description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants