shiny validate improvements

[nikolas-burkoff]

From #183

We should give better error messages in i.e. shinyvalidate::sv_required() takes a message argument so we can have:

And the advantage of this is that the helpful error message is then available to be displayed in the main output so you can see what's wrong directly rather than only being told "please observe red flags in the encoding"

We won't have capacity to do this before the release @donyunardi but it should be considered before we roll shinyvalidate out to other packages

[donyunardi]

Thanks Nik.
I added this issue to shinyvalidate roadmap.

[chlebowa]

Here's an idea:
shinyvalidate messages are collated in the main output and preceded by an optional generic message.

This is done by a function that returns either NULL or the messages, so one would call

shiny::validate(need(foo(), message = foo()))

This function would be placed in teal.

Now, there are more shinyvalidate::InputValidator objects in this particular module and currently I am only capturing one, but I would aim to intercept them all.

If nobody objects, I will go on with this design.,

[gogonzo]

Now, there are more shinyvalidate::InputValidator objects in this particular module and currently I am only capturing one, but I would aim to intercept them all.

@chlebowa Do you mean by this that you will create a hierarchy of validators using InputValidator$add_validator? Could you show an example how would it be organized?

---

Unrelated side note:
Please remove all validate statements from observeEvent or observer if there are any (I saw couple somewhere). They are redundant as validate propagates error to the output, and observers have no "connection" with the output. This is one of the reasons why we implement shinyvalidate - to validate inputs which are needed to update other inputs (through observe)

[nikolas-burkoff]

Please remove all validate statements from observeEvent or observer if there are any (I saw couple somewhere)

I guess you could use a req() if you want the observerEvent to stop

[chlebowa]

@gogonzo In the examples all of the messages were captured from one InputValidator:

iv <- shinyvalidate::InputValidator$new()
(...)
foo(iv, "Some inputs require attention")

I thought extending the function to be able to accept multiple validators

foo(iv1, iv2, "Some inputs require attention")

This way there would be no hierarchy, only that messages from iv2 would appear below those from iv1.
But maybe that's overkill as one can call

foo(iv1, "Some inputs require attention")
foo(iv2)

To be clear, I don't want to change the way validators work, only to pass their messages to the validate call that blocks output display, which is there already.

[gogonzo]

@chlebowa What's the advantage of introducing a new validator over $add_rule? Do I get it correctly that you'd like to have one validator per input? Whatever you decide, we should be consistent in other modules then. If number of validators will be purely subjective choice then in the future there will be a problem of convincing each other on the subjective things, which we'd like to avoid.

[chlebowa]

Adding validators is not within the scope of this task. However, in some cases multiple validators must be created for whatever reason. An example from tm_g_ae_oview (pictured above):
One validator, vi, checks that at least one Arm Variable and Flag are selected.
Another validator, vi_comp, checks that Control and Treatment are set to different values.
Now, since the inputs for Control and Treatment are updated every time the Arm Variable selection changes. Somehow (perhaps @BLAZEWIM or @mhallal1 know the details) the former must be instantiated in the module server and the latter in the output_q reactive.

[chlebowa]

PS. My understanding is this:

1. There used to be many validates that printed validation errors to the main onput.
2. shinyvalidate was introduced to localize validation errors to their respective widgets.
3. This resulted in the main output being empty, so a generic validation message was added.
4. Now we want the main output to also display validation errors.

[nikolas-burkoff]

There used to be many validates that printed validation errors to the main onput.

Of course only the first validation failure was shown, not all of them

[chlebowa]

Right.

Now there would be many. Is that what we want?

[nikolas-burkoff]

Now there would be many. Is that what we want?

Yes please!!!!

[chlebowa]

Update re multiple validators:

So the first validator is defined here and the second one is there. Rules from there cannot be moved to the validator here BUT if one validator is created there, (in the quenv), it can hold and test all rules. I think that would be an improvement.

EDIT:
Actually, if the validator is created "here", rules can be added from "there".
Which solution is better may be personal: everything in one place, but buried vs. validator clearly defined at the top of the module, but with additional parts equally burried.

[chlebowa]

Related: #198

[chlebowa]

See this PR for my proposed mechanism.

[chlebowa]

I implemented gather_fails in all modules of teal.osprey as per #199 .

If everyone is ok with this, I will:

• add gather_fails to teal
• open shinyvalidate improvements #199 for formal review
• begin implementing in teal.modules.general

[nikolas-burkoff]

begin implementing in teal.modules.general

tmc please :)

[chlebowa]

begin implementing in teal.modules.general

tmc please :)

My mistake.