`shinyvalidate` improvements #786

chlebowa · 2022-12-01T11:32:51Z

Adds the validate_inputs functions to teal so they can be called in modules.

github-actions · 2022-12-01T16:10:27Z

Code Coverage Summary

Filename                         Stmts    Miss  Cover    Missing
-----------------------------  -------  ------  -------  ------------------------------
R/dummy_functions.R                 74      61  17.57%   12-95
R/example_module.R                  18      18  0.00%    19-36
R/get_rcode_utils.R                 52       2  96.15%   94, 99
R/get_rcode.R                      137      54  60.58%   74, 81, 86, 211-277
R/include_css_js.R                  24       0  100.00%
R/init.R                            21       3  85.71%   173, 184-185
R/module_nested_tabs.R             130       7  94.62%   57, 96, 101-102, 148, 198, 227
R/module_tabs_with_filters.R        68       1  98.53%   160
R/module_teal_with_splash.R         33       2  93.94%   62, 74
R/module_teal.R                    111       5  95.50%   49, 52, 155-156, 180
R/modules_debugging.R               18      18  0.00%    39-58
R/modules.R                        101       8  92.08%   341-366
R/reporter_previewer_module.R       12       2  83.33%   18, 22
R/show_rcode_modal.R                20      20  0.00%    17-38
R/tdata.R                           41       2  95.12%   146, 172
R/utils.R                           13       0  100.00%
R/validate_inputs.R                 21       0  100.00%
R/validations.R                     62      39  37.10%   103-355
R/zzz.R                             11       7  36.36%   3-14
TOTAL                              967     249  74.25%

Diff against main

Filename               Stmts  Miss    Cover
-------------------  -------  ------  --------
R/validate_inputs.R      +21  -       +100.00%
TOTAL                    +21  -       +0.57%

Results for commit: 37fc943

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

github-actions · 2022-12-01T16:48:20Z

Unit Tests Summary

    1 files   13 suites 17s ⏱️
142 tests 142 ✔️ 0 💤 0 ❌
284 runs 284 ✔️ 0 💤 0 ❌

Results for commit e469ba1.

♻️ This comment has been updated with latest results.

nikolas-burkoff

As we're using shinyvalidate in the examples we need to add it to SUGGESTS in the DESC file
I'm not sure about the name of the function - as we are not just gathering any validation failures but also calling shiny::validate as well - not sure what to call it instead though but making it clear there's "validation" going on in the function is probably a good idea?
I wonder if there's a way to use S3 dispatching here so that users of these functions only need to know one rather than having to look up which of the three they need to use in their case?
I don't think there's any case where we want to call these functions but not call shiny::validate (i.e. just get the messages and then do something with them) - the reason I ask is by coupling the gathering to the validation we can't get the messages out if we need them. We could decouple completely and only have the gathering messages here and let the module developer add the shiny::validate themselves?

tests/testthat/test-gather_fails.R

chlebowa · 2022-12-02T09:54:03Z

I wonder if there's a way to use S3 dispatching here so that users of these functions only need to know one rather than having to look up which of the three they need to use in their case?

I thought about it.
One solution I see would be to get rid of gather_fails_com and have one method for InputValidator and another for a list, the latter with an extra argument for one of multiple blocks of messages.
Or we could go with just gather_fails_grp and always pass a list, with an argument to control grouping. But then there is no good way to have a default header.
I don't particularly like any of these but that's probably a matter of opinion.

I'm not sure about the name of the function - as we are not just gathering any validation failures but also calling shiny::validate as well - not sure what to call it instead though but making it clear there's "validation" going on in the function is probably a good idea?

I'm open to suggestions. The name is derived from the original design, where it would only collate messages and put into a validate call.

I don't think there's any case where we want to call these functions but not call shiny::validate (i.e. just get the messages and then do something with them) - the reason I ask is by coupling the gathering to the validation we can't get the messages out if we need them. We could decouple completely and only have the gathering messages here and let the module developer add the shiny::validate themselves?

That was the original design, the function would only put messages together and then it would be called with validate(need(is.null(gather_fails(iv)), gather_fails(iv)). I wanted to wrap is.null inside the function.

tests/testthat/test-gather_fails.R

chlebowa · 2022-12-04T23:16:38Z

I'm not sure about the name of the function - as we are not just gathering any validation failures but also calling shiny::validate as well - not sure what to call it instead though but making it clear there's "validation" going on in the function is probably a good idea?

I'm open to suggestions. The name is derived from the original design, where it would only collate messages and put into a validate call.

Changed to validate_inputs and validate_inputs_segregated.

nikolas-burkoff

Example unhappy (also I may change "hierarchical" to "sequential")

Otherwise lots of minor tweaks to the roxygen etc. but all looking good - once example works I'll approve and we can merge

Also I don't know why but we still have roxygen 7.2.1 in the image 🤷 so we should revert the DESC file change

nikolas-burkoff · 2022-12-06T08:52:53Z

NEWS.md

@@ -1,5 +1,9 @@
 # teal 0.12.0.9011

+### New features
+
+* Added the `validate_inputs` function that produces informative error messages in app output.


Suggested change

* Added the `validate_inputs` function that produces informative error messages in app output.

* Added `validate_inputs` and `validate_inputs_segregated` functions that produce informative error messages in app outputs.

I may also move this below the "Major breaking changes" section but since we're going to tidy up the NEWS before the next release i think it's fine

I may also move this below the "Major breaking changes" section but since we're going to tidy up the NEWS before the next release i think it's fine

Is it not a "new feature"?

it is yes - I would move the whole New feature section below the "we have changed how teal modules work after 5 years" breaking change mention. But this can all be done later

nikolas-burkoff · 2022-12-06T08:54:12Z

_pkgdown.yml

@@ -52,6 +52,7 @@ reference:
  - title: Validation functions
    contents:
      - starts_with("validate_")
+      - starts_with("gather_fails")


Suggested change

- starts_with("gather_fails")

As those functions use keywords internal they don't appear in pkgdown

Only at the bottom of the file use @keywords internal, pkgdown breaks if the main ones are not added here (but I did forget to change the name here).

yup though the line above starts_with("validate_") should cover them?

Yes, but I hear they are to deprecated ("are we still using those?") so maybe it makes sense to add an extra line for these two?

Yes, but I hear they are to deprecated ("are we still using those?")

They are used quite a bit downstream so I doubt they will be going.. but happy for you to add these extra two explicitly - as long as pkgdown doesn't then output them twice :)

R/validate_inputs.R

nikolas-burkoff · 2022-12-06T08:59:36Z

R/validate_inputs.R

+#' into one message passed to `validate`.
+#'
+#' `shiny::validate` is used to withhold rendering of an output element until
+#' certain conditions are met and a print a validation message in place


Suggested change

#' certain conditions are met and a print a validation message in place

#' certain conditions are met and prints a validation message in place

We are both wrong here. It should read "is used to withhold (...) and (to?) print".

R/validate_inputs.R

nikolas-burkoff · 2022-12-06T09:02:39Z

R/validate_inputs.R

+#' `shinyvalidate::InputValidator` allows to validate input elements
+#' and display specific messages in their respective input widgets.


Suggested change

#' `shinyvalidate::InputValidator` allows to validate input elements

#' and display specific messages in their respective input widgets.

#' `shinyvalidate::InputValidator` allows the validation of input elements

#' and to display specific messages in their respective input widgets.

nikolas-burkoff · 2022-12-06T09:05:04Z

R/validate_inputs.R

+#' @return
+#' Returns NULL if the final validation call passes and a `shiny.silent.error` if it fails.
+#'
+#' @seealso \code{[shinyvalidate::InputValidator]} \code{[shiny::validate]}


Suggested change

#' @seealso \code{[shinyvalidate::InputValidator]} \code{[shiny::validate]}

#' @seealso [`shinyvalidate::InputValidator`], [`shiny::validate`]

I think we can do this?

Yes! Thank you! I could NOT get the links to work 😠

nikolas-burkoff · 2022-12-06T09:06:31Z

R/validate_inputs.R

+#' @seealso \code{[shinyvalidate::InputValidator]} \code{[shiny::validate]}
+#'
+#' @examples
+#' library(shiny)


teal depends on shiny so we don't actually need this line?

Just so the example is self-contained.

nikolas-burkoff

👍

asbates · 2022-12-06T19:02:32Z

Copied over from tmc:
I'm wondering if the messages in the main panel should be highlighted more. I know the inputs themselves have red. I'm not entirely sure it would be obvious to all users that all the errors are in the main panel. Except maybe if they are already familiar with the modules.

I can work on this tomorrow if no one else has gotten to it.

mhallal1 · 2022-12-07T07:01:22Z

NEWS.md

@@ -8,6 +8,9 @@

 * Due to deprecation of `chunks` in `teal.code`, the `teal` framework now uses their replacement (`qenv`) instead. The documentation in `teal` has been updated to reflect this and custom modules written with `chunks` should be updated to use `qenv`.

+### New features
+
+* Added the `validate_inputs` and `validate_inputs_segregated` functions that produce informative error messages in app output.


Suggested change

* Added the `validate_inputs` and `validate_inputs_segregated` functions that produce informative error messages in app output.

* Added the `validate_inputs` and `validate_inputs_segregated` functions that produce informative `shinyvalidate` error messages in the app output.

mhallal1 · 2022-12-07T07:02:35Z

R/validate_inputs.R

@@ -0,0 +1,164 @@
+
+#' send input validation messages to output


Suggested change

#' send input validation messages to output

#' Send `shinyvalidate` input validation messages to output

mhallal1 · 2022-12-07T07:03:15Z

R/validate_inputs.R

+
+#' send input validation messages to output
+#'
+#' Captures messages from \code{InputValidator} objects and collates them


Suggested change

#' Captures messages from \code{InputValidator} objects and collates them

#' Captures messages from \code{shinyvalidate::InputValidator} objects and collates them

I think it would be a good idea to link to shinyvalidate functions as they are external to teal, what do you think @chlebowa ? Maybe only in the description?
For this we can use the format [shinyvalidate::InputValidator()]

and is there a reason to use \code{} here? I do see any instance of this usage in teal

I kept prefixes from Description on purpose, they appear in Details.

Links are in @seealso

\code{} is the markdown way to highlight text as code, I'm still not used to Rmarkdown in Rd files.

@chlebowa \code{} is the old way of assigning code, we now use ``, hence there is no occurence of \code{} in `teal`.
Could you please re-open the PR and correct them to conserve the good practice?

mhallal1 · 2022-12-07T07:03:39Z

R/validate_inputs.R

+#' send input validation messages to output
+#'
+#' Captures messages from \code{InputValidator} objects and collates them
+#' into one message passed to \code{validate}.


Suggested change

#' into one message passed to \code{validate}.

#' into one message passed to \code{shiny::validate}.

R/validate_inputs.R

chlebowa · 2022-12-07T09:14:45Z

Copied over from tmc: I'm wondering if the messages in the main panel should be highlighted more. I know the inputs themselves have red. I'm not entirely sure it would be obvious to all users that all the errors are in the main panel. Except maybe if they are already familiar with the modules.

I can work on this tomorrow if no one else has gotten to it.

Highlighted how?

Validation messages come with a CSS class and validate_inputs_segregated can further modify it.

asbates · 2022-12-07T17:22:04Z

Copied over from tmc: I'm wondering if the messages in the main panel should be highlighted more. I know the inputs themselves have red. I'm not entirely sure it would be obvious to all users that all the errors are in the main panel. Except maybe if they are already familiar with the modules.
I can work on this tomorrow if no one else has gotten to it.

Highlighted how?

Validation messages come with a CSS class and validate_inputs_segregated can further modify it.

Maybe something similar to highlighting of inputs: red text and red box. And/or larger font size. The fact that validation messages have a class would make this fairly easy. I'll open another issue and we can address this later.

Closes [this issue](#185) Following the introduction of `validate_inputs` to `teal` by [#199](insightsengineering/teal#786), this PR: + changes all possible input validations from `validate` calls to `shinyvalidate` input validators + passes all validators to `validate_input` funcitons Signed-off-by: Aleksander Chlebowski <[email protected]> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Nikolas Burkoff <[email protected]> Co-authored-by: Dawid Kałędkowski <[email protected]> Co-authored-by: 27856297+dependabot-preview[bot]@users.noreply.github.com <27856297+dependabot-preview[bot]@users.noreply.github.com>

Aleksander Chlebowski and others added 6 commits December 1, 2022 12:11

add gather_fails functions

bad88da

amend documentation

851f9db

update NEWS

4320c68

add dependency

55cc6e7

add unit tests

444af9a

Merge 444af9a into e35e91a

3b50b9e

chlebowa marked this pull request as ready for review December 1, 2022 16:01

chlebowa added enhancement New feature or request core labels Dec 1, 2022

[skip actions] Restyle files

4e92db3

chlebowa mentioned this pull request Dec 1, 2022

shinyvalidate improvements insightsengineering/teal.osprey#199

Merged

Aleksander Chlebowski added 4 commits December 1, 2022 17:20

update pkgdown

30bdddb

fix links in docs

183fca6

lintr

2d7d43a

spellcheck

5ea79ce

chlebowa requested a review from a team December 1, 2022 17:00

nikolas-burkoff reviewed Dec 2, 2022

View reviewed changes

tests/testthat/test-gather_fails.R Outdated Show resolved Hide resolved

move shinyvalidate to Suggests

0d93eec

nikolas-burkoff reviewed Dec 2, 2022

View reviewed changes

tests/testthat/test-gather_fails.R Outdated Show resolved Hide resolved

nikolas-burkoff reviewed Dec 2, 2022

View reviewed changes

tests/testthat/test-gather_fails.R Outdated Show resolved Hide resolved

nikolas-burkoff reviewed Dec 2, 2022

View reviewed changes

tests/testthat/test-gather_fails.R Outdated Show resolved Hide resolved

Aleksander Chlebowski and others added 2 commits December 5, 2022 00:15

reduce and rename functions

794777f

Merge 794777f into e35e91a

8657c58

[skip actions] Restyle files

2ce23b9

nikolas-burkoff self-assigned this Dec 6, 2022

trigger

b96e7cb

trigger

f5a87ff

nikolas-burkoff reviewed Dec 6, 2022

View reviewed changes

Aleksander Chlebowski added 4 commits December 6, 2022 10:31

fix example

7ffefe7

fix pkgdown

8d8c3ac

update documentation

bf0b063

update NEWS

1048640

nikolas-burkoff mentioned this pull request Dec 6, 2022

tmc shinyvalidate insightsengineering/teal.modules.clinical#699

Merged

fix links in docs

b899785

nikolas-burkoff approved these changes Dec 6, 2022

View reviewed changes

mhallal1 reviewed Dec 7, 2022

View reviewed changes

R/validate_inputs.R Outdated Show resolved Hide resolved

final review

e469ba1

chlebowa enabled auto-merge (squash) December 7, 2022 11:39

chlebowa merged commit 7a6ba9c into main Dec 7, 2022

chlebowa deleted the 185_gather_fails@main branch December 7, 2022 11:51

chlebowa restored the 185_gather_fails@main branch December 7, 2022 13:39

	* Added the `validate_inputs` function that produces informative error messages in app output.
	* Added `validate_inputs` and `validate_inputs_segregated` functions that produce informative error messages in app outputs.

	#' certain conditions are met and a print a validation message in place
	#' certain conditions are met and prints a validation message in place

		#' `shinyvalidate::InputValidator` allows to validate input elements
		#' and display specific messages in their respective input widgets.

	#' @seealso \code{[shinyvalidate::InputValidator]} \code{[shiny::validate]}
	#' @seealso [`shinyvalidate::InputValidator`], [`shiny::validate`]

	* Added the `validate_inputs` and `validate_inputs_segregated` functions that produce informative error messages in app output.
	* Added the `validate_inputs` and `validate_inputs_segregated` functions that produce informative `shinyvalidate` error messages in the app output.

		@@ -0,0 +1,164 @@

		#' send input validation messages to output

	#' send input validation messages to output
	#' Send `shinyvalidate` input validation messages to output

	#' Captures messages from \code{InputValidator} objects and collates them
	#' Captures messages from \code{shinyvalidate::InputValidator} objects and collates them

	#' into one message passed to \code{validate}.
	#' into one message passed to \code{shiny::validate}.

shinyvalidate improvements #786

shinyvalidate improvements #786

Conversation

chlebowa commented Dec 1, 2022 • edited Loading

github-actions bot commented Dec 1, 2022 • edited Loading

Code Coverage Summary

Diff against main

github-actions bot commented Dec 1, 2022 • edited Loading

Unit Tests Summary

nikolas-burkoff left a comment

Choose a reason for hiding this comment

chlebowa commented Dec 2, 2022

chlebowa commented Dec 4, 2022

nikolas-burkoff left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikolas-burkoff Dec 6, 2022 • edited by chlebowa Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chlebowa Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chlebowa Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikolas-burkoff left a comment

Choose a reason for hiding this comment

asbates commented Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhallal1 Dec 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chlebowa commented Dec 7, 2022

asbates commented Dec 7, 2022

`shinyvalidate` improvements #786

`shinyvalidate` improvements #786

chlebowa commented Dec 1, 2022 •

edited

Loading

github-actions bot commented Dec 1, 2022 •

edited

Loading

github-actions bot commented Dec 1, 2022 •

edited

Loading

nikolas-burkoff left a comment •

edited

Loading

nikolas-burkoff Dec 6, 2022 •

edited by chlebowa

Loading

chlebowa Dec 6, 2022 •

edited

Loading

chlebowa Dec 6, 2022 •

edited

Loading

asbates commented Dec 6, 2022 •

edited

Loading

mhallal1 Dec 7, 2022 •

edited

Loading