Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(data-integrity): add checks-selector #348
feat(data-integrity): add checks-selector #348
Changes from 7 commits
172cc7d
9343cf2
18dfc25
48fa4b8
8f81118
45e0d92
e23c4c1
dc5f213
4f51dae
a4d8d68
30921ad
d9078b4
0e04e4d
931385d
4e4e066
77ddb80
12b149f
05fa734
a699792
38df2b4
41d5f23
d7349fa
f2608ad
6915b18
559bebd
f41a728
0671703
27a80e2
63105e7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 initializerunSelected
tofalse
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 toRun 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 tofalse
, would make the user need to select therun selected checks
?This is not the reason for the logic; see above. It's for when editing a job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 inreact-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.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding this, I decided to fix this by having empty array meaning "run selected only", and
null
orundefined
, meaning "run all". Should be a simple enough fix for the situation above.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!