feat(data-integrity): add checks-selector #348
Conversation
Birkbjo
force-pushed
the
DHIS2-12286/feat/data-integrity-config
branch
from
cfe9042 to
9343cf2
Compare
|
🚀 Deployed on https://pr-348--dhis2-scheduler.netlify.app |
cypress.json
Outdated
| "env": { | ||
| "dhis2DataTestPrefix": "dhis2-scheduler", | ||
| "networkMode": "live", | ||
| "dhis2ApiVersion": "37" |
There was a problem hiding this comment.
Yeah, I would think so as well... Hendrik added this, but I guess the param from the command takes precedence, but will fix.
| const checked = value === 'true' | ||
|
|
||
| if (!checked) { | ||
| // clear checks when "Run all" is selected |
There was a problem hiding this comment.
We have destroyOnUnregister enabled, see here, which should take care of removing the value if the field is unregistered. But I see you linked an open issue about conditional rendering causing issues. We're rendering the parameter fields conditionally, could what you encountered be solved in a similar manner?
There was a problem hiding this comment.
Well, I got errors that crashed the app, so I added the hidden-prop to fix this. It wasn't just because of conditional rendering, but conditional rendering in combination with updating the state of the component.
It does not seem like the value is cleared when conditionally rendering the field either. As you can toggle back and forth, and the selected checks will be there. We also need to mark the form as dirty when toggling back and forth (which onChange([]) does) - since we need to be able to switch from "some selected checks" to "run all", and be able to save the form (which is disabled when it's not dirty).
There was a problem hiding this comment.
I see. It seems like you're initializing the state for runSelected based on the previous value of the field. Which will be there if the user toggles back and forth between "run all" and "run only". It'll be lost anyway as soon as the user switches to another job type. If you lose that bit of logic and initialize runSelected to false there's no longer a need to clear the onChange (provided you conditionally render the transfer field in the field group), or use useField. That means you shouldn't run into the linked bug either (at least I didn't during testing, but let me know if I missed it).
I'd go with that. We don't save state for parameter settings between jobs anyway, and if not saving it for this section doesn't require workarounds and simplifies things I would personally go in that direction.
There was a problem hiding this comment.
If you lose that bit of logic and initialize runSelected to false
I'm not sure I follow, this would make it so that when you edit a Data integrity check with "selected checks", it would change it to Run all? That does not work.
The reason for checking for previous value of the field is so that you select the correct "toggle" based on the value of checks. Simple use case is to add a new check for a previous job; always defaulting to false, would make the user need to select the run selected checks?
I'd go with that. We don't save state for parameter settings between jobs anyway, and if not saving it for this section doesn't require workarounds and simplifies things I would personally go in that direction.
This is not the reason for the logic; see above. It's for when editing a job.
There was a problem hiding this comment.
Haha, oh yeah of course you're right. Minor oversight on my part 😅. Wait, let me check one thing that might still satisfy that requirement.
There was a problem hiding this comment.
Thanks for looking into this! Actually makes sense for useField to prevent unregistering of the field. However, I don't really think this is much better than the current solution? And may have unintended side effect - see below. This is a bug in react-final-form anyway, and it's referenced in the code - so once it's fixed we could move the "hidden"-prop and conditionally render instead.
It might be cleaner to unmount the field, however we still need some logic to actually clear the data, if you edit an already existing job, and toggle from "selected checks" to "run all" in your example above, you won't be able to save the form. I think this is because the field is not mounted, and thus does not count as dirty? So the form would then be "pristine".
Other than that, your solution would potentially fix an annoying situation by not unmounting the field; we need to "change" the validation-function when Run all is selected, since we don't want to validate the checks in that case. Not sure if the current "complexity" is worth it, or if we should just drop validation entirely, as it's not actually that bad to not select any checks.
I do wonder if our usage of initialValues in the app is a little flawed. They'll be lost as soon as final form unregisters a field and clears the value, like when a user switches jobTypes for an existing job.
Yeah, haven't looked too much into that, but seems like we would need to change the destroyOnUnmount or something if we would like to keep the initialValues when unmounting fields?
There was a problem hiding this comment.
Other than that, your solution would potentially fix an annoying situation by not unmounting the field; we need to "change" the validation-function when Run all is selected, since we don't want to validate the checks in that case. Not sure if the current "complexity" is worth it, or if we should just drop validation entirely, as it's not actually that bad to not select any checks.
Regarding this, I decided to fix this by having empty array meaning "run selected only", and null or undefined, meaning "run all". Should be a simple enough fix for the situation above.
There was a problem hiding this comment.
Yeah, haven't looked too much into that, but seems like we would need to change the destroyOnUnmount or something if we would like to keep the initialValues when unmounting fields?
Yeah, yeah the reason that that's there is so that if you switch a jobType, the existing parameters don't persist. Otherwise those parameters will be sent to the backend if the user submits, even if they don't match the current job. I suppose the backend would probably drop unrecognized job parameters, but it seemed more correct to me at the time.
That's also why I was thinking of a way to automatically remove the transfer values if it unregisters. My assumption was that selecting run all would mean you don't need the checks job parameter at all. But of course, run all is identical to selecting all checks (I assume?). Which is obvious of course, but I'd not even stopped to consider that.
So, first off, I'll approve the PR since I don't want to block you. Since I rewrote a lot of this app I tend to get a little product-managery about it, but that's not actually my position.
That being said. I can't help myself thinking that the design could be simplified. Instead of a "run all" radio button I'd just render the transfer at all times and forego the radios. After all, the transfer already has a convenient "select all" control. That would simplify our code and the UI if you ask me. Do you know if there's a technical reason why we didn't go with that approach?
(Also, let me know if I'm misunderstanding anything here. We're getting a little into the weeds technically, so maybe I'm overlooking sth here)
There was a problem hiding this comment.
So, first off, I'll approve the PR since I don't want to block you. Since I rewrote a lot of this app I tend to get a little product-managery about it, but that's not actually my position.
I absolutely understand this, no worries! There's a lot of decision that's taken here, and I don't know the full background. I've just tried to implement this feature without too drastic changes.
That being said. I can't help myself thinking that the design could be simplified. Instead of a "run all" radio button I'd just render the transfer at all times and forego the radios. After all, the transfer already has a convenient "select all" control. That would simplify our code and the UI if you ask me. Do you know if there's a technical reason why we didn't go with that approach?
I do agree with this point, and this was my first implementation, with a "If no checks are selected, all checks will be run"-message in the "SelectedEmptyComponent". But I talked to Joe, and he proposed the implemented solution. The reason being that it might be a bit confusing that actually selecting nothing selects all, and the reason below:
Manually having to select all would also probably not be the best (even with the "select all"-button), since if new checks are added in the backend, you would have to manually select them again. In the current solution, all checks will run regardless of user interaction. You might argue that the user have more "control" of not adding new checks - but this is how it always has been and we don't want this change to be too disruptive. It would potentially be confusing to users why some checks are not running.
There was a problem hiding this comment.
I see. Since we already talked a bit on slack, I'll just add here what I said there: seems tricky to really simplify the UI then on our end. The requirements kind of dictate the current solution. Would be nice if eventually we could get rid of the two types of "all checks", because now effectively we have "all current and future checks" and "all explicitly selected checks". To me that wasn't really clear from the UI. Anyway, something for the future I guess. Thanks for explaining!
|
Ok, I see some of my questions were already answered in the PR description. Sorry about that. I'll go through them and close what you already explained. |
# [100.1.0](v100.0.4...v100.1.0) (2022-03-16) ### Features * **data-integrity:** add checks-selector ([#348](#348)) ([aa7baf4](aa7baf4))
|
🎉 This PR is included in version 100.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
https://jira.dhis2.org/browse/DHIS2-12286
Don't be too intimidated of all the file changes, most are updated fixtures for tests.
Should probably create a generic field component for 'java.lang.Set', however we still need to override the
TransferOptiondue to the label not supporting a react-component. I plan to update this in UI though.Note that the https://debug.dhis2.org/dev/api/dataIntegrity.json. endpoint contains properties like
description,recommendationandintroduction.These are very inconsistent, and most jobs do not have all these properties - and
descriptionseem to just be a human readablename. After a brief discussion with Joe, we decided a simple MVP is good enough for now. So we opted to just show thenameandseverity.This endpoint is "semi-dynamic", in the sense that it's described by a
yaml-file, but still compiled at build-time, it also does not have support for translations. I've included translations for all the current jobs, but also have a simple fallback for any other jobs with a simple "snake"-case converter. This should all ideally be done on the backend, but we do what we have to work with what've got.