From a3829c4093b40c6152cf1285dfa9506efcb28b8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dawid=20Ka=C5=82=C4=99dkowski?= Date: Thu, 17 Oct 2024 14:59:22 +0200 Subject: [PATCH] forbid "all" in teal_transform_module (#1381) Closes #1347 Additionally, changed `transforms` to `transformers` in the `ui/srv_transform_data()` documentation as there was a param-name mismatch with `module()` --- R/module_nested_tabs.R | 6 +- R/module_transform_data.R | 30 ++++---- R/modules.R | 57 ++++++++------- R/teal_data_module.R | 14 ++-- man/example_module.Rd | 19 ++--- man/module_transform_data.Rd | 8 ++- man/teal_modules.Rd | 58 +++++++++------- man/teal_transform_module.Rd | 7 +- tests/testthat/test-modules.R | 20 ------ tests/testthat/test-teal_data_module.R | 8 +++ vignettes/data-transform-as-shiny-module.Rmd | 73 -------------------- 11 files changed, 119 insertions(+), 181 deletions(-) diff --git a/R/module_nested_tabs.R b/R/module_nested_tabs.R index c51e6138ec..deb83cc59f 100644 --- a/R/module_nested_tabs.R +++ b/R/module_nested_tabs.R @@ -125,9 +125,7 @@ ui_teal_module.teal_module <- function(id, modules, depth = 0L) { width = 3, ui_data_summary(ns("data_summary")), ui_filter_data(ns("filter_panel")), - if (length(modules$transformers) > 0 && !isTRUE(attr(modules$transformers, "custom_ui"))) { - ui_transform_data(ns("data_transform"), transforms = modules$transformers, class = "well") - }, + ui_transform_data(ns("data_transform"), transformers = modules$transformers, class = "well"), class = "teal_secondary_col" ) ) @@ -272,7 +270,7 @@ srv_teal_module.teal_module <- function(id, transformed_teal_data <- srv_transform_data( "data_transform", data = filtered_teal_data, - transforms = modules$transformers, + transformers = modules$transformers, modules = modules, is_transformer_failed = is_transformer_failed ) diff --git a/R/module_transform_data.R b/R/module_transform_data.R index 418c81d06d..1d3607b4f3 100644 --- a/R/module_transform_data.R +++ b/R/module_transform_data.R @@ -13,18 +13,19 @@ NULL #' @rdname module_transform_data -ui_transform_data <- function(id, transforms, class = "well") { +ui_transform_data <- function(id, transformers = list(), class = "well") { checkmate::assert_string(id) - checkmate::assert_list(transforms, "teal_transform_module", null.ok = TRUE) + checkmate::assert_list(transformers, "teal_transform_module") + ns <- NS(id) - labels <- lapply(transforms, function(x) attr(x, "label")) + labels <- lapply(transformers, function(x) attr(x, "label")) ids <- get_unique_labels(labels) - names(transforms) <- ids + names(transformers) <- ids lapply( - names(transforms), + names(transformers), function(name) { - data_mod <- transforms[[name]] + data_mod <- transformers[[name]] wrapper_id <- ns(sprintf("wrapper_%s", name)) div( # todo: accordion? # class .teal_validated changes the color of the boarder on error in ui_validate_reactive_teal_data @@ -44,7 +45,7 @@ ui_transform_data <- function(id, transforms, class = "well") { ), div( id = wrapper_id, - ui_teal_data(id = ns(name), data_module = transforms[[name]]$ui) + ui_teal_data(id = ns(name), data_module = transformers[[name]]$ui) ) ) } @@ -52,29 +53,26 @@ ui_transform_data <- function(id, transforms, class = "well") { } #' @rdname module_transform_data -srv_transform_data <- function(id, data, transforms, modules, is_transformer_failed = reactiveValues()) { +srv_transform_data <- function(id, data, transformers = list(), modules, is_transformer_failed = reactiveValues()) { checkmate::assert_string(id) assert_reactive(data) - checkmate::assert_list(transforms, "teal_transform_module", null.ok = TRUE) + checkmate::assert_list(transformers, "teal_transform_module") checkmate::assert_class(modules, "teal_module") - if (length(transforms) == 0L) { - return(data) - } - labels <- lapply(transforms, function(x) attr(x, "label")) + labels <- lapply(transformers, function(x) attr(x, "label")) ids <- get_unique_labels(labels) - names(transforms) <- ids + names(transformers) <- ids moduleServer(id, function(input, output, session) { logger::log_debug("srv_teal_data_modules initializing.") Reduce( function(previous_result, name) { srv_teal_data( id = name, - data_module = function(id) transforms[[name]]$server(id, previous_result), + data_module = function(id) transformers[[name]]$server(id, previous_result), modules = modules, is_transformer_failed = is_transformer_failed ) }, - x = names(transforms), + x = names(transformers), init = data ) }) diff --git a/R/modules.R b/R/modules.R index 76dea53d68..59ad3e5d97 100644 --- a/R/modules.R +++ b/R/modules.R @@ -20,6 +20,29 @@ setOldClass("teal_modules") #' because they are used by the `mapping` argument of [teal_slices()] #' and the report previewer module [reporter_previewer_module()], respectively. #' +#' # Restricting datasets used by `teal_module`: +#' The `datanames` argument controls which datasets are used by the module’s server. These datasets, +#' passed via server's `data` argument, are the only ones shown in the module's tab. +#' +#' When `datanames` is set to `"all"`, all datasets in the data object are treated as relevant. +#' However, this may include unnecessary datasets, such as: +#' - Proxy variables for column modifications +#' - Temporary datasets used to create final versions +#' - Connection objects +#' +#' To exclude irrelevant datasets, use the [set_datanames()] function to change `datanames` from +#' `"all"` to specific names. Trying to modify non-`"all"` values with [set_datanames()] will result +#' in a warning. Datasets with names starting with . are ignored globally unless explicitly listed +#' in `datanames`. +#' +#' # `datanames` with `transformers` +#' When transformers are specified, their `datanames` are added to the module’s `datanames`, which +#' changes the behavior as follows: +#' - If `module(datanames)` is `NULL` and the `transformers` have defined `datanames`, the sidebar +#' will appear showing the `transformers`' datasets, instead of being hidden. +#' - If `module(datanames)` is set to specific values and any `transformer` has `datanames = "all"`, +#' the module may receive extra datasets that could be unnecessary +#' #' @param label (`character(1)`) Label shown in the navigation item for the module or module group. #' For `modules()` defaults to `"root"`. See `Details`. #' @param server (`function`) `shiny` module with following arguments: @@ -42,40 +65,24 @@ setOldClass("teal_modules") #' - `...` (optional) When provided, `ui_args` elements will be passed to the module named argument #' or to the `...`. #' @param filters (`character`) Deprecated. Use `datanames` instead. -#' @param datanames (`character`) Names of the datasets that are relevant for the item. -#' The keyword `"all"` provides all datasets available in `data` passed to `teal` application. -#' `NULL` will hide the filter panel. +#' @param datanames (`character`) Names of the datasets relevant to the item. +#' There are 2 reserved values that have specific behaviors: +#' - The keyword `"all"` includes all datasets available in the data passed to the teal application. +#' - `NULL` hides the sidebar panel completely. +#' - If `transformers` are specified, their `datanames` are automatically added to this `datanames` +#' argument. #' @param server_args (named `list`) with additional arguments passed on to the server function. #' @param ui_args (named `list`) with additional arguments passed on to the UI function. #' @param x (`teal_module` or `teal_modules`) Object to format/print. #' @param indent (`integer(1)`) Indention level; each nested element is indented one level more. #' @param transformers (`list` of `teal_data_module`) that will be applied to transform the data. -#' Each transform module UI will appear in the `teal` application, unless the `custom_ui` attribute is set on the list. -#' If so, the module developer is responsible to display the UI in the module itself. `datanames` of the `transformers` -#' will be added to the `datanames`. +#' Each transform module UI will appear in the `teal`'s sidebar panel. +#' Transformers' `datanames` are added to the `datanames`. See [teal_transform_module()]. #' -#' When the transformation does not have sufficient input data, the resulting data will fallback -#' to the last successful transform or, in case there are none, to the filtered data. #' @param ... #' - For `modules()`: (`teal_module` or `teal_modules`) Objects to wrap into a tab. #' - For `format()` and `print()`: Arguments passed to other methods. #' -#' @section `datanames`: -#' The module's `datanames` argument determines a subset of datasets from the `data` object, as specified in the -#' server function argument, to be presented in the module. Datasets displayed in the filter panel will be limited -#' to this subset. -#' When `datanames` is set to `"all"`, all available datasets in the `data` object are considered relevant for the -#' module. However, setting `datanames` argument to `"all"` might include datasets that are irrelevant for the module, -#' for example: -#' - Proxy variables used for modifying columns. -#' - Modified copies of datasets used to create a final dataset. -#' - Connection objects. -#' To prevent these irrelevant datasets from appearing in the module, use the [set_datanames()] function on the -#' [module] or [modules()] to change the `datanames` from `"all"` to specific dataset names. Attempting to change -#' `datanames` values that was not set to `"all"` using [set_datanames()] will be ignored with a warning. -#' -#' Additionally, datasets with names starting with `.` are ignored when `datanames` is set to `"all"`. -#' #' @return #' `module()` returns an object of class `teal_module`. #' @@ -264,7 +271,7 @@ module <- function(label = "module", } checkmate::assert_list(transformers, types = "teal_transform_module") transformer_datanames <- unlist(lapply(transformers, attr, "datanames")) - combined_datanames <- if (identical(datanames, "all") || any(sapply(transformer_datanames, identical, "all"))) { + combined_datanames <- if (identical(datanames, "all")) { "all" } else { union(datanames, transformer_datanames) diff --git a/R/teal_data_module.R b/R/teal_data_module.R index 5017be1432..99dec6bc01 100644 --- a/R/teal_data_module.R +++ b/R/teal_data_module.R @@ -109,9 +109,8 @@ teal_data_module <- function(ui, server, label = "data module", once = TRUE) { #' unexpected behavior. #' See `vignettes("data-transform-as-shiny-module")` for more information. #' @param datanames (`character`) -#' Names of the datasets that are relevant for the module. The -#' filter panel will only display filters for specified `datanames`. The keyword `"all"` will show -#' filters of all datasets. `datanames` will be automatically appended to the [modules()] `datanames`. +#' Names of the datasets that are relevant for this module to evaluate. If set to `character(0)` +#' then module would receive [modules()] `datanames`. #' @examples #' my_transformers <- list( #' teal_transform_module( @@ -144,10 +143,17 @@ teal_data_module <- function(ui, server, label = "data module", once = TRUE) { teal_transform_module <- function(ui = function(id) NULL, server = function(id, data) data, label = "transform module", - datanames = "all") { + datanames = character(0)) { checkmate::assert_function(ui, args = "id", nargs = 1) checkmate::assert_function(server, args = c("id", "data"), nargs = 2) checkmate::assert_string(label) + checkmate::assert_character(datanames) + if (identical(datanames, "all")) { + stop( + "teal_transform_module can't have datanames property equal to 'all'. Set `datanames = character(0)` instead.", + call. = FALSE + ) + } structure( list( ui = ui, diff --git a/man/example_module.Rd b/man/example_module.Rd index 26558f8650..7f59bc01b6 100644 --- a/man/example_module.Rd +++ b/man/example_module.Rd @@ -14,17 +14,18 @@ example_module( \item{label}{(\code{character(1)}) Label shown in the navigation item for the module or module group. For \code{modules()} defaults to \code{"root"}. See \code{Details}.} -\item{datanames}{(\code{character}) Names of the datasets that are relevant for the item. -The keyword \code{"all"} provides all datasets available in \code{data} passed to \code{teal} application. -\code{NULL} will hide the filter panel.} +\item{datanames}{(\code{character}) Names of the datasets relevant to the item. +There are 2 reserved values that have specific behaviors: +\itemize{ +\item The keyword \code{"all"} includes all datasets available in the data passed to the teal application. +\item \code{NULL} hides the sidebar panel completely. +\item If \code{transformers} are specified, their \code{datanames} are automatically added to this \code{datanames} +argument. +}} \item{transformers}{(\code{list} of \code{teal_data_module}) that will be applied to transform the data. -Each transform module UI will appear in the \code{teal} application, unless the \code{custom_ui} attribute is set on the list. -If so, the module developer is responsible to display the UI in the module itself. \code{datanames} of the \code{transformers} -will be added to the \code{datanames}. - -When the transformation does not have sufficient input data, the resulting data will fallback -to the last successful transform or, in case there are none, to the filtered data.} +Each transform module UI will appear in the \code{teal}'s sidebar panel. +Transformers' \code{datanames} are added to the \code{datanames}. See \code{\link[=teal_transform_module]{teal_transform_module()}}.} } \value{ A \code{teal} module which can be included in the \code{modules} argument to \code{\link[=init]{init()}}. diff --git a/man/module_transform_data.Rd b/man/module_transform_data.Rd index 2a4a351062..5ae155632e 100644 --- a/man/module_transform_data.Rd +++ b/man/module_transform_data.Rd @@ -6,12 +6,12 @@ \alias{srv_transform_data} \title{Module to transform \code{reactive} \code{teal_data}} \usage{ -ui_transform_data(id, transforms, class = "well") +ui_transform_data(id, transformers = list(), class = "well") srv_transform_data( id, data, - transforms, + transformers = list(), modules, is_transformer_failed = reactiveValues() ) @@ -19,6 +19,10 @@ srv_transform_data( \arguments{ \item{id}{(\code{character(1)}) Module id} +\item{transformers}{(\code{list} of \code{teal_data_module}) that will be applied to transform the data. +Each transform module UI will appear in the \code{teal}'s sidebar panel. +Transformers' \code{datanames} are added to the \code{datanames}. See \code{\link[=teal_transform_module]{teal_transform_module()}}.} + \item{data}{(\verb{reactive teal_data})} \item{modules}{(\code{teal_modules} or \code{teal_module}) For \code{datanames} validation purpose} diff --git a/man/teal_modules.Rd b/man/teal_modules.Rd index 33865d976c..b763c01513 100644 --- a/man/teal_modules.Rd +++ b/man/teal_modules.Rd @@ -67,21 +67,22 @@ or to the \code{...}. \item{filters}{(\code{character}) Deprecated. Use \code{datanames} instead.} -\item{datanames}{(\code{character}) Names of the datasets that are relevant for the item. -The keyword \code{"all"} provides all datasets available in \code{data} passed to \code{teal} application. -\code{NULL} will hide the filter panel.} +\item{datanames}{(\code{character}) Names of the datasets relevant to the item. +There are 2 reserved values that have specific behaviors: +\itemize{ +\item The keyword \code{"all"} includes all datasets available in the data passed to the teal application. +\item \code{NULL} hides the sidebar panel completely. +\item If \code{transformers} are specified, their \code{datanames} are automatically added to this \code{datanames} +argument. +}} \item{server_args}{(named \code{list}) with additional arguments passed on to the server function.} \item{ui_args}{(named \code{list}) with additional arguments passed on to the UI function.} \item{transformers}{(\code{list} of \code{teal_data_module}) that will be applied to transform the data. -Each transform module UI will appear in the \code{teal} application, unless the \code{custom_ui} attribute is set on the list. -If so, the module developer is responsible to display the UI in the module itself. \code{datanames} of the \code{transformers} -will be added to the \code{datanames}. - -When the transformation does not have sufficient input data, the resulting data will fallback -to the last successful transform or, in case there are none, to the filtered data.} +Each transform module UI will appear in the \code{teal}'s sidebar panel. +Transformers' \code{datanames} are added to the \code{datanames}. See \code{\link[=teal_transform_module]{teal_transform_module()}}.} \item{...}{\itemize{ \item For \code{modules()}: (\code{teal_module} or \code{teal_modules}) Objects to wrap into a tab. @@ -121,24 +122,33 @@ The labels \code{"global_filters"} and \code{"Report previewer"} are reserved because they are used by the \code{mapping} argument of \code{\link[=teal_slices]{teal_slices()}} and the report previewer module \code{\link[=reporter_previewer_module]{reporter_previewer_module()}}, respectively. } -\section{\code{datanames}}{ - -The module's \code{datanames} argument determines a subset of datasets from the \code{data} object, as specified in the -server function argument, to be presented in the module. Datasets displayed in the filter panel will be limited -to this subset. -When \code{datanames} is set to \code{"all"}, all available datasets in the \code{data} object are considered relevant for the -module. However, setting \code{datanames} argument to \code{"all"} might include datasets that are irrelevant for the module, -for example: +\section{Restricting datasets used by \code{teal_module}:}{ +The \code{datanames} argument controls which datasets are used by the module’s server. These datasets, +passed via server's \code{data} argument, are the only ones shown in the module's tab. + +When \code{datanames} is set to \code{"all"}, all datasets in the data object are treated as relevant. +However, this may include unnecessary datasets, such as: \itemize{ -\item Proxy variables used for modifying columns. -\item Modified copies of datasets used to create a final dataset. -\item Connection objects. -To prevent these irrelevant datasets from appearing in the module, use the \code{\link[=set_datanames]{set_datanames()}} function on the -\link{module} or \code{\link[=modules]{modules()}} to change the \code{datanames} from \code{"all"} to specific dataset names. Attempting to change -\code{datanames} values that was not set to \code{"all"} using \code{\link[=set_datanames]{set_datanames()}} will be ignored with a warning. +\item Proxy variables for column modifications +\item Temporary datasets used to create final versions +\item Connection objects } -Additionally, datasets with names starting with \code{.} are ignored when \code{datanames} is set to \code{"all"}. +To exclude irrelevant datasets, use the \code{\link[=set_datanames]{set_datanames()}} function to change \code{datanames} from +\code{"all"} to specific names. Trying to modify non-\code{"all"} values with \code{\link[=set_datanames]{set_datanames()}} will result +in a warning. Datasets with names starting with . are ignored globally unless explicitly listed +in \code{datanames}. +} + +\section{\code{datanames} with \code{transformers}}{ +When transformers are specified, their \code{datanames} are added to the module’s \code{datanames}, which +changes the behavior as follows: +\itemize{ +\item If \code{module(datanames)} is \code{NULL} and the \code{transformers} have defined \code{datanames}, the sidebar +will appear showing the \code{transformers}' datasets, instead of being hidden. +\item If \code{module(datanames)} is set to specific values and any \code{transformer} has \code{datanames = "all"}, +the module may receive extra datasets that could be unnecessary +} } \examples{ diff --git a/man/teal_transform_module.Rd b/man/teal_transform_module.Rd index e64e0ca25f..931329ab6c 100644 --- a/man/teal_transform_module.Rd +++ b/man/teal_transform_module.Rd @@ -8,7 +8,7 @@ teal_transform_module( ui = function(id) NULL, server = function(id, data) data, label = "transform module", - datanames = "all" + datanames = character(0) ) } \arguments{ @@ -27,9 +27,8 @@ See \code{vignettes("data-transform-as-shiny-module")} for more information.} \item{label}{(\code{character(1)}) Label of the module.} \item{datanames}{(\code{character}) -Names of the datasets that are relevant for the module. The -filter panel will only display filters for specified \code{datanames}. The keyword \code{"all"} will show -filters of all datasets. \code{datanames} will be automatically appended to the \code{\link[=modules]{modules()}} \code{datanames}.} +Names of the datasets that are relevant for this module to evaluate. If set to \code{character(0)} +then module would receive \code{\link[=modules]{modules()}} \code{datanames}.} } \description{ \ifelse{html}{\href{https://lifecycle.r-lib.org/articles/stages.html#experimental}{\figure{lifecycle-experimental.svg}{options: alt='[Experimental]'}}}{\strong{[Experimental]}} diff --git a/tests/testthat/test-modules.R b/tests/testthat/test-modules.R index 594e801d45..9085f76462 100644 --- a/tests/testthat/test-modules.R +++ b/tests/testthat/test-modules.R @@ -534,26 +534,6 @@ testthat::test_that("module datanames is appended by its transformers datanames" testthat::expect_identical(out$datanames, c("c", "a", "b")) }) -testthat::test_that("module datanames is set to 'all' if any transformer $datanames is 'all'", { - transformer_w_datanames <- teal_transform_module( - ui = function(id) NULL, - server = function(id, data) { - moduleServer(id, function(input, output, session) { - reactive({ - new_data <- within(data(), { - new_dataset <- data.frame(a = 1:3, b = 4:6) - }) - new_data - }) - }) - }, - datanames = "all" - ) - - out <- module(datanames = "c", transformers = list(transformer_w_datanames)) - testthat::expect_identical(out$datanames, "all") -}) - testthat::test_that("module datanames stays 'all' regardless of transformers", { transformer_w_datanames <- teal_transform_module( ui = function(id) NULL, diff --git a/tests/testthat/test-teal_data_module.R b/tests/testthat/test-teal_data_module.R index d751cdc000..0361ea4ea4 100644 --- a/tests/testthat/test-teal_data_module.R +++ b/tests/testthat/test-teal_data_module.R @@ -22,3 +22,11 @@ testthat::test_that("teal_data_module throws when server has other formals than ".*formal arguments.*" ) }) + + +testthat::test_that("teal_transform_module doesn't accept datanames = 'all'", { + testthat::expect_error( + teal_transform_module(datanames = "all"), + "can't have datanames property equal to 'all'" + ) +}) diff --git a/vignettes/data-transform-as-shiny-module.Rmd b/vignettes/data-transform-as-shiny-module.Rmd index 854b507b66..94e3289a0b 100644 --- a/vignettes/data-transform-as-shiny-module.Rmd +++ b/vignettes/data-transform-as-shiny-module.Rmd @@ -164,76 +164,3 @@ if (interactive()) { shinyApp(app$ui, app$server) } ``` - -## Custom placement of the transform UI - -When a custom transformation is used, the UI for the transformation is placed below the filter panel. -However, there is a way to customize the placement of the UI inside the module content. - -In order to place the transformation UI inside the module there are few things one has to do: -1. Create a custom module wrapper function. -2. Call the desired module in the module wrapper function and store it in a variable so it's UI can be modified. -3. Modify the UI of the module with the transform UI at the desired location by calling the `ui_transform_data`. Note that in order for the transform to work you need to change the namespace of the `id` by passing `NS(gsub("-module$", "", id), "data_transform")`. -4. Set the `custom_ui` attribute of the `module$transformers` to `TRUE`. - -Now the custom module should embed the transformation UI inside the module content. - -Here is an example of a custom module wrapper function that modifies the `example_module` module. -```{r} -example_module_encoding <- function(label = "example module (on encoding)", datanames = "all", transformers = list()) { - mod <- example_module(label, datanames, transformers) - mod$ui <- function(id) { - ns <- NS(id) - teal.widgets::standard_layout( - output = verbatimTextOutput(ns("text")), - encoding = tags$div( - ui_transform_data(NS(gsub("-module$", "", id), "data_transform"), transformers), - selectInput(ns("dataname"), "Choose a dataset", choices = NULL), - teal.widgets::verbatim_popup_ui(ns("rcode"), "Show R code") - ) - ) - } - attr(mod$transformers, "custom_ui") <- TRUE - mod -} - -data <- within(teal_data(), { - iris <- iris - mtcars <- mtcars -}) -datanames(data) <- c("iris", "mtcars") - -my_transformers <- list( - teal_transform_module( - label = "Custom transform for iris", - ui = function(id) { - ns <- NS(id) - tags$div( - numericInput(ns("n_rows"), "Number of rows to subset", value = 6, min = 1, max = 150, step = 1) - ) - }, - server = function(id, data) { - moduleServer(id, function(input, output, session) { - reactive({ - within(data(), - { - iris <- head(iris, num_rows) - }, - num_rows = input$n_rows - ) - }) - }) - } - ) -) - -app <- init( - data = data, - modules = example_module_encoding(transformers = my_transformers) -) - -if (interactive()) { - shinyApp(app$ui, app$server) -} -``` -