capture-app icon indicating copy to clipboard operation
capture-app copied to clipboard

refactor: [TECH-1283] default values for program rule variables

Open superskip opened this issue 3 years ago • 10 comments

This is a refactor of VariableService, the module which takes care of populating program rule variables inside the rules engine. The goal in this patch is to introduce a standardized way for representing "empty" variables, and subsequently make sure such variables gets assigned a default value.

The new population process goes like this: When creating a new program rule variable, the source value first gets processed by inputConverter (which in VariableService goes by the name onProcessValue). The input converters have been modified so that they return null whenever the source value is undefined. This enables VariableService to treat a variable as not having a value whenever the processed value equals null. These processed values are then passed as arguments to buildVariable. The variable built by buildVariable gets assigned a default value if the value given as input equals null. Also the property hasValue is set based on the same condition.

superskip avatar Nov 03 '22 15:11 superskip

This pull request introduces 1 alert and fixes 1 when merging d6dcdb8d8844eda698effd6f6b7dfce6763482fc into 6fd52be2f40eb4f344bebfda310bcb2683c8e90c - view on LGTM.com

new alerts:

  • 1 for Incomplete string escaping or encoding

fixed alerts:

  • 1 for Incomplete string escaping or encoding

lgtm-com[bot] avatar Nov 03 '22 15:11 lgtm-com[bot]

This pull request introduces 1 alert and fixes 1 when merging 222bbd8206dac861299b68b8ef88d6574b613465 into 6fd52be2f40eb4f344bebfda310bcb2683c8e90c - view on LGTM.com

new alerts:

  • 1 for Incomplete string escaping or encoding

fixed alerts:

  • 1 for Incomplete string escaping or encoding

lgtm-com[bot] avatar Nov 03 '22 16:11 lgtm-com[bot]

This pull request fixes 1 alert when merging 173c0d838b372cd9bb80c2de988eaad8a73b59bd into 6fd52be2f40eb4f344bebfda310bcb2683c8e90c - view on LGTM.com

fixed alerts:

  • 1 for Incomplete string escaping or encoding

lgtm-com[bot] avatar Nov 03 '22 17:11 lgtm-com[bot]

Thanks for the review @simonadomnisoru!

It got me thinking a bit more about the true-only field, and now I got a question myself (for myself): Should an unticked "true-only"-field always make d2:hasValue return false, or should that happen only before the user touches it?

Same for text fields that the user chooses to blank out.

If it such fields are also considered to not have a value, then I need to rewrite the input-converters so that they return null also when the input value is equal to the default value...

superskip avatar Nov 10 '22 17:11 superskip

This pull request fixes 1 alert when merging 70acfe9b9e850c5851aa0b1647a887e90a1aef87 into c6dc6cd170279a80b90ee5fd6c5a5574e4e678a4 - view on LGTM.com

fixed alerts:

  • 1 for Incomplete string escaping or encoding

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine :gear: that powers LGTM.com. For more information, please check out our post on the GitHub blog.

lgtm-com[bot] avatar Nov 15 '22 21:11 lgtm-com[bot]

This pull request fixes 1 alert when merging 607db1360a4fd5e3913a33e3a8bcb26ea7890fa2 into 98b6af14ee68053c735660300ff3fdc6677b1a2d - view on LGTM.com

fixed alerts:

  • 1 for Incomplete string escaping or encoding

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine :gear: that powers LGTM.com. For more information, please check out our post on the GitHub blog.

lgtm-com[bot] avatar Nov 16 '22 12:11 lgtm-com[bot]

I made a thorough comparison with Tracker Capture, and found that blanking out a field should make an associated program rule variable count as empty. Hence the input converters should transform empty strings to null. However, I generalised a bit too far in my last comment when I said that source value === default value implies empty variable, because a numeric source field containing a zero should not be treated as empty. In the end only the string value converters turned out to need adjustment. Or in a sense not really those either, as apparently empty strings get converted to null in this function before arriving the rules engine. I modified them anyway, as a safety measure.

superskip avatar Nov 16 '22 12:11 superskip

I made a thorough comparison with Tracker Capture, and found that blanking out a field should make an associated program rule variable count as empty. Hence the input converters should transform empty strings to null. However, I generalised a bit too far in my last comment when I said that source value === default value implies empty variable, because a numeric source field containing a zero should not be treated as empty. In the end only the string value converters turned out to need adjustment. Or in a sense not really those either, as apparently empty strings get converted to null in this function before arriving the rules engine. I modified them anyway, as a safety measure.

Good conclusion!

JoakimSM avatar Nov 16 '22 14:11 JoakimSM

This pull request fixes 1 alert when merging fa7655bb49da7eed0602f0405f966f3e57ef94b9 into 34670239521d1e9525251e2f53967d3d207a45b8 - view on LGTM.com

fixed alerts:

  • 1 for Incomplete string escaping or encoding

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine :gear: that powers LGTM.com. For more information, please check out our post on the GitHub blog.

lgtm-com[bot] avatar Nov 17 '22 12:11 lgtm-com[bot]

🚀 Deployed on https://deploy-preview-3089--dhis2-capture.netlify.app

github-actions[bot] avatar Nov 18 '22 08:11 github-actions[bot]

:tada: This PR is included in version 100.23.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

dhis2-bot avatar Jan 10 '23 10:01 dhis2-bot