diff --git a/NEWS.md b/NEWS.md index 771c9cfce6..830555bdaa 100644 --- a/NEWS.md +++ b/NEWS.md @@ -8,6 +8,7 @@ * Possibility to download lockfile to restore app session for reproducibility. #479 * Introduced a function `set_datanames()` to change a `datanames` of the `teal_module`. * Datasets which name starts with `.` are ignored when `module`'s `datanames` is set as `"all"`. +* Added warning when reserved `datanames`, such as `all` and `.raw_data` are being used. ### Breaking changes diff --git a/R/utils.R b/R/utils.R index 6c3f8de083..5bd81b7a28 100644 --- a/R/utils.R +++ b/R/utils.R @@ -134,31 +134,58 @@ check_modules_datanames <- function(modules, datanames) { } #' @rdname check_modules_datanames -check_modules_datanames_html <- function(modules, - datanames) { +check_reserved_datanames <- function(datanames) { + reserved_datanames <- datanames[datanames %in% c("all", ".raw_data")] + if (length(reserved_datanames) == 0L) { + return(NULL) + } + + tags$span( + to_html_code_list(reserved_datanames), + sprintf( + "%s reserved for internal use. Please avoid using %s as %s.", + pluralize(reserved_datanames, "is", "are"), + pluralize(reserved_datanames, "it", "them"), + pluralize(reserved_datanames, "a dataset name", "dataset names") + ) + ) +} + +#' @rdname check_modules_datanames +check_modules_datanames_html <- function(modules, datanames) { check_datanames <- check_modules_datanames_recursive(modules, datanames) show_module_info <- inherits(modules, "teal_modules") # used in two contexts - module and app + + reserved_datanames <- check_reserved_datanames(datanames) + if (!length(check_datanames)) { - return(TRUE) + out <- if (is.null(reserved_datanames)) { + TRUE + } else { + shiny::tagList(reserved_datanames) + } + return(out) } shiny::tagList( + reserved_datanames, lapply( check_datanames, function(mod) { tagList( tags$span( - tags$span(if (length(mod$missing_datanames) == 1) "Dataset" else "Datasets"), + tags$span(pluralize(mod$missing_datanames, "Dataset")), to_html_code_list(mod$missing_datanames), tags$span( - paste0( - if (length(mod$missing_datanames) > 1) "are missing" else "is missing", - if (show_module_info) sprintf(" for module '%s'.", mod$label) else "." + sprintf( + "%s missing%s.", + pluralize(mod$missing_datanames, "is", "are"), + if (show_module_info) sprintf(" for module '%s'", mod$label) else "" ) ) ), if (length(datanames) >= 1) { tagList( - tags$span(if (length(datanames) == 1) "Dataset" else "Datasets"), + tags$span(pluralize(datanames, "Dataset")), tags$span("available in data:"), tagList( tags$span( @@ -382,7 +409,7 @@ paste_datanames_character <- function(x, tagList( tags$code(x[.ix]), if (.ix != length(x)) { - tags$span(ifelse(.ix == length(x) - 1, " and ", ", ")) + tags$span(if (.ix == length(x) - 1) " and " else ", ") } ) }) @@ -400,17 +427,18 @@ build_datanames_error_message <- function(label = NULL, tags = list(span = shiny::tags$span, code = shiny::tags$code), tagList = shiny::tagList) { # nolint: object_name. tags$span( - tags$span(ifelse(length(extra_datanames) > 1, "Datasets", "Dataset")), + tags$span(pluralize(extra_datanames, "Dataset")), paste_datanames_character(extra_datanames, tags, tagList), tags$span( - paste0( - ifelse(length(extra_datanames) > 1, "are missing", "is missing"), - ifelse(is.null(label), ".", sprintf(" for tab '%s'.", label)) + sprintf( + "%s missing%s", + pluralize(extra_datanames, "is", "are"), + if (is.null(label)) "" else sprintf(" for tab '%s'", label) ) ), if (length(datanames) >= 1) { tagList( - tags$span(ifelse(length(datanames) > 1, "Datasets", "Dataset")), + tags$span(pluralize(datanames, "Dataset")), tags$span("available in data:"), tagList( tags$span( @@ -445,3 +473,26 @@ build_datanames_error_message <- function(label = NULL, } ) } + +#' Pluralize a word depending on the size of the input +#' +#' @param x (`object`) to check length for plural. +#' @param singular (`character`) singular form of the word. +#' @param plural (optional `character`) plural form of the word. If not given an "s" +#' is added to the singular form. +#' +#' @return A `character` that correctly represents the size of the `x` argument. +#' @keywords internal +pluralize <- function(x, singular, plural = NULL) { + checkmate::assert_string(singular) + checkmate::assert_string(plural, null.ok = TRUE) + if (length(x) == 1L) { # Zero length object should use plural form. + singular + } else { + if (is.null(plural)) { + sprintf("%ss", singular) + } else { + plural + } + } +} diff --git a/man/check_modules_datanames.Rd b/man/check_modules_datanames.Rd index b01270eae2..f1df42ef2a 100644 --- a/man/check_modules_datanames.Rd +++ b/man/check_modules_datanames.Rd @@ -2,11 +2,14 @@ % Please edit documentation in R/utils.R \name{check_modules_datanames} \alias{check_modules_datanames} +\alias{check_reserved_datanames} \alias{check_modules_datanames_html} \title{Check \code{datanames} in modules} \usage{ check_modules_datanames(modules, datanames) +check_reserved_datanames(datanames) + check_modules_datanames_html(modules, datanames) } \arguments{ diff --git a/man/pluralize.Rd b/man/pluralize.Rd new file mode 100644 index 0000000000..eac6b67321 --- /dev/null +++ b/man/pluralize.Rd @@ -0,0 +1,23 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/utils.R +\name{pluralize} +\alias{pluralize} +\title{Pluralize a word depending on the size of the input} +\usage{ +pluralize(x, singular, plural = NULL) +} +\arguments{ +\item{x}{(\code{object}) to check length for plural.} + +\item{singular}{(\code{character}) singular form of the word.} + +\item{plural}{(optional \code{character}) plural form of the word. If not given an "s" +is added to the singular form.} +} +\value{ +A \code{character} that correctly represents the size of the \code{x} argument. +} +\description{ +Pluralize a word depending on the size of the input +} +\keyword{internal} diff --git a/tests/testthat/test-init.R b/tests/testthat/test-init.R index 1ebca65fa2..62284d1d9b 100644 --- a/tests/testthat/test-init.R +++ b/tests/testthat/test-init.R @@ -108,6 +108,33 @@ testthat::test_that( } ) +testthat::describe("init throws warning when datanames in modules has reserved name", { + testthat::it("`all`", { + testthat::expect_warning( + init( + data = teal.data::teal_data(all = mtcars), + modules = list(example_module()) + ), + "`all` is reserved for internal use\\. Please avoid using it as a dataset name\\." + ) + }) + + testthat::it("`.raw_data` and `all`", { + td <- + testthat::expect_warning( + init( + data = teal.data::teal_data( + all = mtcars, + .raw_data = iris, + join_keys = teal.data::join_keys(teal.data::join_key(".raw_data", "all", "a_key")) + ), + modules = list(example_module()) + ), + "`.raw_data` and `all` are reserved for internal use\\. Please avoid using them as dataset names\\." + ) + }) +}) + testthat::test_that("init throws when dataname in filter incompatible w/ datanames in data", { testthat::expect_warning( init( diff --git a/tests/testthat/test-module_teal.R b/tests/testthat/test-module_teal.R index 2e91a13f57..849b0ecd7f 100644 --- a/tests/testthat/test-module_teal.R +++ b/tests/testthat/test-module_teal.R @@ -545,6 +545,78 @@ testthat::describe("srv_teal teal_modules", { ) }) + testthat::describe("reserved dataname is being used:", { + testthat::it("multiple datanames with `all` and `.raw_data`", { + testthat::skip_if_not_installed("rvest") + + # Shared common code for tests + td <- within(teal.data::teal_data(), { + all <- mtcars + iris <- iris + .raw_data <- data.frame( + Species = c("Setosa", "Virginica", "Versicolor"), + New.Column = c("Setosas are cool", "Virginicas are also cool", "Versicolors are cool too") + ) + }) + teal.data::join_keys(td) <- teal.data::join_keys(join_key(".raw_data", "iris", "Species")) + + shiny::testServer( + app = srv_teal, + args = list( + id = "test", + data = td, + modules = modules(module("module_1", server = function(id, data) data)) + ), + expr = { + session$setInputs("teal_modules-active_tab" = "module_1") + testthat::expect_equal( + trimws( + rvest::html_text2( + rvest::read_html( + output[["teal_modules-module_1-validate_datanames-shiny_warnings-message"]]$html + ) + ) + ), + "all and .raw_data are reserved for internal use. Please avoid using them as dataset names." + ) + } + ) + }) + + testthat::it("single dataname with `all`", { + testthat::skip_if_not_installed("rvest") + + td <- within(teal.data::teal_data(), { + all <- mtcars + iris <- iris + }) + + shiny::testServer( + app = srv_teal, + args = list( + id = "test", + data = td, + modules = modules( + module("module_1", server = function(id, data) data) + ) + ), + expr = { + session$setInputs("teal_modules-active_tab" = "module_1") + testthat::expect_equal( + trimws( + rvest::html_text2( + rvest::read_html( + output[["teal_modules-module_1-validate_datanames-shiny_warnings-message"]]$html + ) + ) + ), + "all is reserved for internal use. Please avoid using it as a dataset name." + ) + } + ) + }) + }) + testthat::describe("warnings on missing datanames", { testthat::it("warns when dataname is not available", { testthat::skip_if_not_installed("rvest")