Handle validation in d-e-s #113

nikolas-burkoff · 2022-12-01T14:59:50Z

This would close #112

See examples in the issue

Tests for new functionality still required

R/data_extract_module.R

asbates · 2022-12-01T22:42:13Z

A couple of questions. I think I know 1. and 2. because of insightsengineering/teal.osprey/pull/199 but just want to double check.

Will error messages display in the main UI?
If there are multiple error messages, with they all be displayed in the main UI?
I think this PR only updates validation for data extract spec? So we would probably open another issue for other places here that have validation?

nikolas-burkoff · 2022-12-02T08:31:38Z

Will error messages display in the main UI?

If there are multiple error messages, with they all be displayed in the main UI?

They can be yes - it's up to what the module developer chooses to do with these messages

I think this PR only updates validation for data extract spec? So we would probably open another issue for other places here that have validation?

Correct - the only places d-e-s are used are tmg and tmc so the following issues would then make use of this functionality:

R/data_extract_module.R

R/utils.R

gogonzo · 2022-12-08T12:23:32Z

Hi All, I don't think that validation of the filters are urgent right now. I have an opinion that we can remove data_extract in the future or simplify this to the form which can be:

easy to validate
easy to interpret

@mhallal1 if you conclude that validation of the d-e-s filter is too complicated please just skip this. Validating select still makes sense.

Adds input validation for `filter_spec` similar to `select_spec`. One thing I want to note is that within `data_extract_read_srv` is the variable [`filter_idx`](https://github.com/insightsengineering/teal.transform/blob/7b0e91ef4cafa856fd5b496c60b02d17b42fe96f/R/data_extract_read_module.R#L23) which presumably can have length > 1. Because it can have length > 1, I would think we need to have multiple `iv$add_rule` calls, 1 for each element of `filter_idx`. However, it seems to be working with a [single call](https://github.com/insightsengineering/teal.transform/blob/7b0e91ef4cafa856fd5b496c60b02d17b42fe96f/R/data_extract_read_module.R#L78). I'm thinking I'm misunderstanding how to create a `filter_spec` that would result in `length(filter_idx) > 1`. It would be great if some can test this out. My attempt was the following ``` library(shiny) library(teal.transform) library(shinyvalidate) ADSL <- data.frame( STUDYID = c("B", rep(c("A", "B", "B"), 3)), USUBJID = LETTERS[1:10], SEX = rep(c("F", "M"), 5), AGE = rpois(10, 30), BMRKR1 = rlnorm(10) ) adsl_extract <- data_extract_spec( dataname = "ADSL", filter = filter_spec( vars = c("SEX", "STUDYID"), choices = c("F - A", "M - A", "F - B", "M - B"), selected = c("F - A") ), select = select_spec( label = "Select variable:", choices = variable_choices(ADSL, c("AGE", "BMRKR1")), selected = "AGE", multiple = TRUE, fixed = FALSE ) ) mtcars_extract <- data_extract_spec( dataname = "mtcars", filter = filter_spec( vars = c("cyl", "am"), choices = c("4 - 1", "8 - 1", "4 - 0"), sep = " - ", multiple = TRUE ), select = select_spec( label = "Select variable: ", choices = variable_choices(mtcars, c("mpg", "hp")), multiple = TRUE ) ) data_list <- list(ADSL = reactive(ADSL), mtcars = reactive(mtcars)) join_keys <- teal.data::join_keys(teal.data::join_key("ADSL", "ADSL", c("STUDYID", "USUBJID"))) app <- shinyApp( ui = fluidPage( shinyjs::useShinyjs(), teal.widgets::standard_layout( output = verbatimTextOutput("out1"), encoding = tagList( data_extract_ui( id = "des_var", label = "Selection", data_extract_spec = list(adsl_extract, mtcars_extract) ) ) ) ), server = function(input, output, session) { des_reactive_input <- data_extract_srv( id = "des_var", datasets = data_list, data_extract_spec = list(adsl_extract, mtcars_extract), join_keys = join_keys, select_validation_rule = sv_required(message = "Please please select a column from a dataset"), filter_validation_rule = sv_required(message = "filter test") #optional add this to add a nice message #dataset_validation_rule = sv_required(message = "Please select a dataset!") ) iv_r <- reactive({ iv <- InputValidator$new() des_reactive_input()$iv$enable() iv$add_validator(des_reactive_input()$iv) iv$enable() iv iv }) output$out1 <- renderPrint({ if (iv_r()$is_valid()) des_reactive_input() else "Check that you have made a selection" }) } ) runApp(app) ``` Signed-off-by: Mahmoud Hallal <86970066+mhallal1@users.noreply.github.com> Co-authored-by: Mahmoud Hallal <mahmoud.hallal@roche.com> Co-authored-by: Mahmoud Hallal <86970066+mhallal1@users.noreply.github.com> Co-authored-by: Nikolas Burkoff <nikolas.burkoff@roche.com>

gogonzo · 2022-12-12T13:16:42Z

After reading the vignette in shinyvalidate I no longer think it's weird or too complicated. It adds the complexity to the data_extract but doesn't change the default functionality which remains the same.

Please update the value returned from the data_extract_srv to include iv information.
Update all tests and checks to allow returning a list(iv = iv) instead of NULL from data_extract_srv.
data_merge_module should also accept these rules and pass it directly to the data_extract_multiple_srv.
Watch out, data_merge_srv has own validate for selected datasets "at least one dataset needs to be selected" - i think this rule has to be added to data_extract when called through data_merge_srv

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

So I agree with the above @gogonzo except:

data_merge_module should also accept these rules and pass it directly to the data_extract_multiple_srv.

This would then require data_merge output to include iv and I don't think we should do that - as it's wrapping everything up so tightly would prefer to make module developers (i.e. us) split up the call to extract and merge? Thoughts?

gogonzo · 2022-12-13T11:23:37Z

So I agree with the above @gogonzo except:

data_merge_module should also accept these rules and pass it directly to the data_extract_multiple_srv.

This would then require data_merge output to include iv and I don't think we should do that - as it's wrapping everything up so tightly would prefer to make module developers (i.e. us) split up the call to extract and merge? Thoughts?

Yup, let's not add iv to the output of data_merge. Yes, this would require splitting merge_expression_module to data_extract_multiple_srv and merge_expression_srv everywhere we want inputs to be validated.

chlebowa · 2022-12-13T15:47:42Z

So how do I add a conditional validation rule?

EDIT: To clarify, normally I would do

if (condition) iv$add_rule()

nikolas-burkoff · 2022-12-13T16:16:40Z

So how do I add a conditional validation rule?
EDIT: To clarify, normally I would do if (condition) iv$add_rule()

There are two ways to do it - if you need reactivity put the condition inside the rule (write your own ~ if () or function(value) rule), if you don't then you can create the selection_validation_rule list with the rule there or not

chlebowa · 2022-12-13T16:25:39Z

So how do I add a conditional validation rule?
EDIT: To clarify, normally I would do if (condition) iv$add_rule()

There are two ways to do it - if you need reactivity put the condition inside the rule (write your own ~ if () or function(value) rule), if you don't then you can create the selection_validation_rule list with the rule there or not

Will selection_validation_rule handle a condition on another input value?

R/data_extract_module.R

Signed-off-by: Nikolas Burkoff <nikolas.burkoff@capgemini.com>

gogonzo

I went through the code and everything looks good. I've tried to break the modules but didn't "succeed". Extra exported function still bothers me but I guess we have no choice as we need to include iv from multiple extracts into parent iv.

asbates

LGTM!

…teal.transform into validator_test@main

github-actions · 2022-12-30T16:29:15Z

Code Coverage Summary

Filename                          Stmts    Miss  Cover    Missing
------------------------------  -------  ------  -------  ------------------------------------------------------------------------
R/all_choices.R                       1       0  100.00%
R/call_utils.R                      156     124  20.51%   14-23, 64, 66, 68, 107-431
R/check_selector.R                   31       0  100.00%
R/choices_labeled.R                 200      61  69.50%   60, 66, 71, 78, 94, 218-222, 226-231, 261-274, 388-389, 391, 423-471
R/choices_selected.R                 81      11  86.42%   201-229, 260
R/column_functions.R                  3       3  0.00%    13-16
R/data_extract_datanames.R           32       8  75.00%   9-13, 64-66
R/data_extract_filter_module.R       95      16  83.16%   75-88, 90-91, 145
R/data_extract_module.R             298      66  77.85%   3, 121, 126, 143, 146-151, 153, 172-175, 203-249, 483, 488, 667, 733-738
R/data_extract_read_module.R        135       7  94.81%   29, 33-36, 131, 148
R/data_extract_select_module.R       32      18  43.75%   31-48
R/data_extract_single_module.R       53       2  96.23%   29, 42
R/data_extract_spec.R                32       0  100.00%
R/data_merge_module.R                61      15  75.41%   101-118
R/filter_spec.R                     186       1  99.46%   373
R/format_data_extract.R              16       1  93.75%   49
R/get_dplyr_call.R                  299       0  100.00%
R/get_merge_call.R                  280      29  89.64%   28-34, 45, 206-215, 368, 384-396
R/include_css_js.R                    5       0  100.00%
R/input_checks.R                     11       2  81.82%   18-19
R/merge_data_utils.R                  2       0  100.00%
R/merge_datasets.R                  135       6  95.56%   77, 225-229
R/merge_expression_module.R          49       1  97.96%   147
R/resolve_delayed.R                  16       0  100.00%
R/resolve.R                         114      44  61.40%   229-312
R/select_spec.R                      49       8  83.67%   149, 213-220
R/utils.R                            24      11  54.17%   26-39
R/zzz.R                               2       2  0.00%    2-3
TOTAL                              2398     436  81.82%

Diff against main

Filename                        Stmts    Miss  Cover
----------------------------  -------  ------  -------
R/data_extract_module.R           +51      +5  +2.55%
R/data_extract_read_module.R      +12      -6  +5.38%
R/data_merge_module.R              +3       0  +1.27%
R/merge_expression_module.R        +3       0  +0.13%
R/utils.R                          +9       0  +27.50%
TOTAL                             +78      -1  +0.65%

Results for commit: 3b8d9a3

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

Nikolas Burkoff added 3 commits December 1, 2022 10:30

simple validation

c0f7095

Handle case of multiple datasets

2f86d13

handle multiple_srv

f470fc5

nikolas-burkoff requested review from mhallal1 and chlebowa December 1, 2022 14:59

nikolas-burkoff added the core label Dec 1, 2022

nikolas-burkoff commented Dec 1, 2022

View reviewed changes

R/data_extract_module.R Outdated Show resolved Hide resolved

mhallal1 self-assigned this Dec 2, 2022

chlebowa reviewed Dec 5, 2022

View reviewed changes

R/data_extract_module.R Show resolved Hide resolved

add helper function

0192a24

nikolas-burkoff commented Dec 6, 2022

View reviewed changes

R/utils.R Outdated Show resolved Hide resolved

asbates mentioned this pull request Dec 6, 2022

tmc shinyvalidate insightsengineering/teal.modules.clinical#699

Merged

gogonzo self-assigned this Dec 8, 2022

dataset iv fix£

df8aeb7

Nikolas Burkoff and others added 2 commits December 8, 2022 15:14

fix

a77c02c

Merge branch 'main' into validator_test@main

bf2214a

Nikolas Burkoff added 4 commits December 13, 2022 11:57

default selector list

6bfecc3

roxygen, news etc.

4c1c7c7

docs

1d95423

roxygen

b9b9965

Nikolas Burkoff and others added 4 commits December 14, 2022 20:23

small tweak

349af55

remove extra commas

16c21d1

fix validation

104c782

Add tests for data extract input validation (#118)

9c43d86

gogonzo reviewed Dec 16, 2022

View reviewed changes

R/data_extract_module.R Outdated Show resolved Hide resolved

nikolas-burkoff commented Dec 16, 2022

View reviewed changes

R/data_extract_module.R Outdated Show resolved Hide resolved

Apply suggestions from code review

c77391f

Signed-off-by: Nikolas Burkoff <nikolas.burkoff@capgemini.com>

gogonzo approved these changes Dec 16, 2022

View reviewed changes

Nikolas Burkoff and others added 11 commits December 16, 2022 11:03

lintr

e56df00

change validation

36b7319

edge case

3fdf845

edge edge case

eeb33aa

Merge eeb33aa into 1d68830

b6bddf6

[skip actions] Restyle files

0f20fd9

trigger

b6ceaa3

test edge case

38db304

Merge 38db304 into 1d68830

c5f7115

[skip actions] Restyle files

fb1a5a8

trigger

a3df0ba

asbates approved these changes Dec 22, 2022

View reviewed changes

nikolas-burkoff mentioned this pull request Dec 23, 2022

add shinyvalidate to tmg insightsengineering/teal.modules.general#498

Merged

Aleksander Chlebowski and others added 5 commits December 30, 2022 16:45

handle optional validations

dcea888

bug fix

5053af4

[skip actions] Roxygen Man Pages Auto Update

369c721

another bug fix

a90089b

Merge branch 'validator_test@main' of github.com:insightsengineering/…

8eb225e

…teal.transform into validator_test@main

Merge branch 'main' into validator_test@main

bbdc19d

nikolas-burkoff merged commit 4c467e4 into main Jan 4, 2023

nikolas-burkoff deleted the validator_test@main branch January 4, 2023 12:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle validation in d-e-s #113

Handle validation in d-e-s #113

nikolas-burkoff commented Dec 1, 2022 •

edited

Loading

asbates commented Dec 1, 2022

nikolas-burkoff commented Dec 2, 2022

gogonzo commented Dec 8, 2022

gogonzo commented Dec 12, 2022

nikolas-burkoff commented Dec 13, 2022

gogonzo commented Dec 13, 2022

chlebowa commented Dec 13, 2022 •

edited

Loading

nikolas-burkoff commented Dec 13, 2022

chlebowa commented Dec 13, 2022

gogonzo left a comment

asbates left a comment

github-actions bot commented Dec 30, 2022 •

edited

Loading

Handle validation in d-e-s #113

Handle validation in d-e-s #113

Conversation

nikolas-burkoff commented Dec 1, 2022 • edited Loading

asbates commented Dec 1, 2022

nikolas-burkoff commented Dec 2, 2022

gogonzo commented Dec 8, 2022

gogonzo commented Dec 12, 2022

nikolas-burkoff commented Dec 13, 2022

gogonzo commented Dec 13, 2022

chlebowa commented Dec 13, 2022 • edited Loading

nikolas-burkoff commented Dec 13, 2022

chlebowa commented Dec 13, 2022

gogonzo left a comment

Choose a reason for hiding this comment

asbates left a comment

Choose a reason for hiding this comment

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

Code Coverage Summary

Diff against main

nikolas-burkoff commented Dec 1, 2022 •

edited

Loading

chlebowa commented Dec 13, 2022 •

edited

Loading

github-actions bot commented Dec 30, 2022 •

edited

Loading