align args asserts; align args defaults; logger in test (#1008)

ready for review Unfortunately it grew more than I expected initially and I should have been split it into few for the review convenience... - align argument assertions Currently on main we have two approaches for assert and convert. Please find below examples (note the order) - `modules`: - assert all classes: https://github.com/insightsengineering/teal/blob/e98eab0a2465a00e05957cefc408ee10185440aa/R/init.R#L121 - convert to the desired: https://github.com/insightsengineering/teal/blob/e98eab0a2465a00e05957cefc408ee10185440aa/R/init.R#L133-L138 - `data`: - convert to the desired: https://github.com/insightsengineering/teal/blob/e98eab0a2465a00e05957cefc408ee10185440aa/R/init.R#L105-L108 - assert classes: https://github.com/insightsengineering/teal/blob/e98eab0a2465a00e05957cefc408ee10185440aa/R/init.R#L120 I decided to follow the latter one. - align argument defaults for `header`, `footer`, etc. - silent logger in tests to avoid the following: ``` ✔ | 7 | rcode_utils ⠏ | 0 | report_previewer_module [INFO] 2023-12-13 16:14:23.5635 pid:41602 token:[] teal Initializing reporter_previewer_module [INFO] 2023-12-13 16:14:23.5663 pid:41602 token:[] teal Initializing reporter_previewer_module ✔ | 5 | report_previewer_module ``` - change error message from "The teal_data_module must ..." into "The data module must ..." as end users don't have to know the name of the class. They are interacting with function arguments like `data` or `modules`. - I noticed that a few tests expects a string with error / warning message that comes from logger. This is wrong as this actually test logger functionality and assumes certain value of logger threshold, appender, layout etc. I changed this to the base warning that is independent on all of those things. Will create a separate task to discuss handler via logger - https://github.com/insightsengineering/teal/issues/1028 --------- Signed-off-by: Pawel Rucki <12943682+pawelru@users.noreply.github.com> Co-authored-by: Aleksander Chlebowski <114988527+chlebowa@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
insightsengineering · Jan 15, 2024 · f20f29f · f20f29f
1 parent 7538ecb
commit f20f29f
Show file tree

Hide file tree

Showing 16 changed files with 270 additions and 160 deletions.
diff --git a/R/init.R b/R/init.R
@@ -23,20 +23,18 @@
 #'   `teal_modules` or `teal_module` object. These are the specific output modules which
 #'   will be displayed in the teal application. See [modules()] and [module()] for
 #'   more details.
-#' @param title (`shiny.tag` or `character`)\cr
-#'   The browser window title. Defaults to a title "Teal app" with the icon of NEST.
+#' @param title (`shiny.tag` or `character(1)`)\cr
+#'   The browser window title. Defaults to a title "teal app" with the icon of NEST.
 #'   Can be created using the `build_app_title()` or
 #'   by passing a valid `shiny.tag` which is a head tag with title and link tag.
 #' @param filter (`teal_slices`)\cr
 #'   Specification of initial filter. Filters can be specified using [teal::teal_slices()].
 #'   Old way of specifying filters through a list is deprecated and will be removed in the
 #'   next release. Please fix your applications to use [teal::teal_slices()].
-#' @param header (`shiny.tag` or `character`) \cr
-#'   the header of the app. Note shiny code placed here (and in the footer
-#'   argument) will be placed in the app's `ui` function so code which needs to be placed in the `ui` function
-#'   (such as loading `CSS` via [htmltools::htmlDependency()]) should be included here.
-#' @param footer (`shiny.tag` or `character`)\cr
-#'   the footer of the app
+#' @param header (`shiny.tag` or `character(1)`) \cr
+#'   The header of the app.
+#' @param footer (`shiny.tag` or `character(1)`)\cr
+#'   The footer of the app.
 #' @param id (`character`)\cr
 #'   module id to embed it, if provided,
 #'   the server function must be called with [shiny::moduleServer()];
@@ -107,11 +105,10 @@ init <- function(data,
                  header = tags$p(),
                  footer = tags$p(),
                  id = character(0)) {
-  logger::log_trace("init initializing teal app with: data ({ class(data)[1] }).")
-  if (is.list(data) && !inherits(data, "teal_data_module")) {
-    checkmate::assert_list(data, names = "named")
-    data <- do.call(teal.data::teal_data, data)
-  }
+  logger::log_trace("init initializing teal app with: data ('{ class(data) }').")
+
+  # argument checking (independent)
+  ## `data`
   if (inherits(data, "TealData")) {
     lifecycle::deprecate_stop(
       when = "0.99.0",
@@ -122,52 +119,75 @@ init <- function(data,
       )
     )
   }
-
-  checkmate::assert_multi_class(data, c("teal_data", "teal_data_module"))
-  checkmate::assert_multi_class(modules, c("teal_module", "list", "teal_modules"))
   checkmate::assert(
-    checkmate::check_class(filter, "teal_slices"),
-    checkmate::check_list(filter, names = "named")
+    .var.name = "data",
+    checkmate::check_multi_class(data, c("teal_data", "teal_data_module")),
+    checkmate::check_list(data, names = "named")
   )
-  checkmate::assert_multi_class(title, c("shiny.tag", "character"))
-  checkmate::assert_multi_class(header, c("shiny.tag", "character"))
-  checkmate::assert_multi_class(footer, c("shiny.tag", "character"))
-  checkmate::assert_character(id, max.len = 1, any.missing = FALSE)
-
-  teal.logger::log_system_info()
-
-  if (is.character(title)) {
-    title <- build_app_title(title)
-  } else {
-    validate_app_title_tag(title)
+  if (is.list(data) && !inherits(data, "teal_data_module")) {
+    data <- do.call(teal.data::teal_data, data)
   }
 
+  ## `modules`
+  checkmate::assert(
+    .var.name = "modules",
+    checkmate::check_multi_class(modules, c("teal_modules", "teal_module")),
+    checkmate::check_list(modules, min.len = 1, any.missing = FALSE, types = c("teal_module", "teal_modules"))
+  )
   if (inherits(modules, "teal_module")) {
     modules <- list(modules)
   }
-  if (inherits(modules, "list")) {
+  if (checkmate::test_list(modules, min.len = 1, any.missing = FALSE, types = c("teal_module", "teal_modules"))) {
     modules <- do.call(teal::modules, modules)
   }
 
+  ## `filter`
+  checkmate::assert(
+    .var.name = "filter",
+    checkmate::check_class(filter, "teal_slices"),
+    checkmate::check_list(filter, names = "named")
+  )
+
+  ## all other arguments
+  checkmate::assert(
+    .var.name = "title",
+    checkmate::check_string(title),
+    checkmate::check_multi_class(title, c("shiny.tag", "shiny.tag.list", "html"))
+  )
+  checkmate::assert(
+    .var.name = "header",
+    checkmate::check_string(header),
+    checkmate::check_multi_class(header, c("shiny.tag", "shiny.tag.list", "html"))
+  )
+  checkmate::assert(
+    .var.name = "footer",
+    checkmate::check_string(footer),
+    checkmate::check_multi_class(footer, c("shiny.tag", "shiny.tag.list", "html"))
+  )
+  checkmate::assert_character(id, max.len = 1, any.missing = FALSE)
+
+  # log
+  teal.logger::log_system_info()
+
+  # argument transformations
+  ## `modules` - landing module
   landing <- extract_module(modules, "teal_module_landing")
-  if (length(landing) > 1L) stop("Only one `landing_popup_module` can be used.")
-  modules <- drop_module(modules, "teal_module_landing")
-
-  # Calculate app id that will be used to stamp filter state snapshots.
-  # App id is a hash of the app's data and modules.
-  # See "transferring snapshots" section in ?snapshot.
-  hashables <- mget(c("data", "modules"))
-  hashables$data <- if (inherits(hashables$data, "teal_data")) {
-    as.list(hashables$data@env)
-  } else if (inherits(data, "teal_data_module")) {
-    body(data$server)
+  landing_module <- NULL
+  if (length(landing) == 1L) {
+    landing_module <- landing[[1L]]
+    modules <- drop_module(modules, "teal_module_landing")
+  } else if (length(landing) > 1L) {
+    stop("Only one `landing_popup_module` can be used.")
   }
 
-  attr(filter, "app_id") <- rlang::hash(hashables)
+  ## `filter` - app_id attribute
+  attr(filter, "app_id") <- create_app_id(data, modules)
 
-  # convert teal.slice::teal_slices to teal::teal_slices
+  ## `filter` - convert teal.slice::teal_slices to teal::teal_slices
   filter <- as.teal_slices(as.list(filter))
 
+  # argument checking (interdependent)
+  ## `filter` - `modules`
   if (isTRUE(attr(filter, "module_specific"))) {
     module_names <- unlist(c(module_labels(modules), "global_filters"))
     failed_mod_names <- setdiff(names(attr(filter, "mapping")), module_names)
@@ -194,6 +214,7 @@ init <- function(data,
     }
   }
 
+  ## `data` - `modules`
   if (inherits(data, "teal_data")) {
     if (length(teal_data_datanames(data)) == 0) {
       stop("`data` object has no datanames and its environment is empty. Specify `datanames(data)` and try again.")
@@ -207,7 +228,7 @@ init <- function(data,
 
     is_filter_ok <- check_filter_datanames(filter, teal_data_datanames(data))
     if (!isTRUE(is_filter_ok)) {
-      logger::log_warn(is_filter_ok)
+      warning(is_filter_ok)
       # we allow app to continue if applied filters are outside
       # of possible data range
     }
@@ -220,14 +241,14 @@ init <- function(data,
   res <- list(
     ui = ui_teal_with_splash(id = id, data = data, title = title, header = header, footer = footer),
     server = function(input, output, session) {
-      if (length(landing) == 1L) {
-        landing_module <- landing[[1L]]
+      if (!is.null(landing_module)) {
         do.call(landing_module$server, c(list(id = "landing_module_shiny_id"), landing_module$server_args))
       }
-      filter <- deep_copy_filter(filter)
-      srv_teal_with_splash(id = id, data = data, modules = modules, filter = filter)
+      srv_teal_with_splash(id = id, data = data, modules = modules, filter = deep_copy_filter(filter))
     }
   )
+
   logger::log_trace("init teal app has been initialized.")
+
   return(res)
 }
diff --git a/R/module_teal.R b/R/module_teal.R
@@ -61,32 +61,39 @@ NULL
 #' @rdname module_teal
 ui_teal <- function(id,
                     splash_ui = tags$h2("Starting the Teal App"),
-                    title = NULL,
-                    header = tags$p(""),
-                    footer = tags$p("")) {
-  if (checkmate::test_string(header)) {
-    header <- tags$h1(header)
-  }
-  if (checkmate::test_string(footer)) {
-    footer <- tags$p(footer)
+                    title = build_app_title(),
+                    header = tags$p(),
+                    footer = tags$p()) {
+  checkmate::assert_character(id, max.len = 1, any.missing = FALSE)
+
+  checkmate::assert_multi_class(splash_ui, c("shiny.tag", "shiny.tag.list", "html"))
+
+  if (is.character(title)) {
+    title <- build_app_title(title)
+  } else {
+    validate_app_title_tag(title)
   }
+
   checkmate::assert(
-    checkmate::check_class(splash_ui, "shiny.tag"),
-    checkmate::check_class(splash_ui, "shiny.tag.list"),
-    checkmate::check_class(splash_ui, "html")
-  )
-  checkmate::assert(
-    checkmate::check_class(header, "shiny.tag"),
-    checkmate::check_class(header, "shiny.tag.list"),
-    checkmate::check_class(header, "html")
+    .var.name = "header",
+    checkmate::check_string(header),
+    checkmate::check_multi_class(header, c("shiny.tag", "shiny.tag.list", "html"))
   )
+  if (checkmate::test_string(header)) {
+    header <- tags$p(header)
+  }
+
   checkmate::assert(
-    checkmate::check_class(footer, "shiny.tag"),
-    checkmate::check_class(footer, "shiny.tag.list"),
-    checkmate::check_class(footer, "html")
+    .var.name = "footer",
+    checkmate::check_string(footer),
+    checkmate::check_multi_class(footer, c("shiny.tag", "shiny.tag.list", "html"))
   )
+  if (checkmate::test_string(footer)) {
+    footer <- tags$p(footer)
+  }
 
   ns <- NS(id)
+
   # Once the data is loaded, we will remove this element and add the real teal UI instead
   splash_ui <- div(
     # id so we can remove the splash screen once ready, which is the first child of this container
@@ -146,7 +153,8 @@ srv_teal <- function(id, modules, teal_data_rv, filter = teal_slices()) {
     )
 
     # `JavaScript` code
-    run_js_files(files = "init.js") # `JavaScript` code to make the clipboard accessible
+    run_js_files(files = "init.js")
+
     # set timezone in shiny app
     # timezone is set in the early beginning so it will be available also
     # for `DDL` and all shiny modules

diff --git a/R/module_teal_with_splash.R b/R/module_teal_with_splash.R
@@ -19,10 +19,27 @@
 #' @export
 ui_teal_with_splash <- function(id,
                                 data,
-                                title,
-                                header = tags$p("Add Title Here"),
-                                footer = tags$p("Add Footer Here")) {
+                                title = build_app_title(),
+                                header = tags$p(),
+                                footer = tags$p()) {
+  checkmate::assert_character(id, max.len = 1, any.missing = FALSE)
   checkmate::assert_multi_class(data, c("teal_data", "teal_data_module"))
+  checkmate::assert(
+    .var.name = "title",
+    checkmate::check_string(title),
+    checkmate::check_multi_class(title, c("shiny.tag", "shiny.tag.list", "html"))
+  )
+  checkmate::assert(
+    .var.name = "header",
+    checkmate::check_string(header),
+    checkmate::check_multi_class(header, c("shiny.tag", "shiny.tag.list", "html"))
+  )
+  checkmate::assert(
+    .var.name = "footer",
+    checkmate::check_string(footer),
+    checkmate::check_multi_class(footer, c("shiny.tag", "shiny.tag.list", "html"))
+  )
+
   ns <- NS(id)
 
   # Startup splash screen for delayed loading
@@ -58,7 +75,10 @@ ui_teal_with_splash <- function(id,
 #' If data is not loaded yet, `reactive` returns `NULL`.
 #' @export
 srv_teal_with_splash <- function(id, data, modules, filter = teal_slices()) {
+  checkmate::assert_character(id, max.len = 1, any.missing = FALSE)
   checkmate::check_multi_class(data, c("teal_data", "teal_data_module"))
+  checkmate::assert_class(modules, "teal_modules")
+  checkmate::assert_class(filter, "teal_slices")
 
   moduleServer(id, function(input, output, session) {
     logger::log_trace("srv_teal_with_splash initializing module with data.")
@@ -72,7 +92,7 @@ srv_teal_with_splash <- function(id, data, modules, filter = teal_slices()) {
     teal_data_rv <- if (inherits(data, "teal_data_module")) {
       data <- data$server(id = "teal_data_module")
       if (!is.reactive(data)) {
-        stop("The `teal_data_module` must return a reactive expression.", call. = FALSE)
+        stop("The `teal_data_module` passed to `data` must return a reactive expression.", call. = FALSE)
       }
       data
     } else if (inherits(data, "teal_data")) {
@@ -94,7 +114,7 @@ srv_teal_with_splash <- function(id, data, modules, filter = teal_slices()) {
           need(
             FALSE,
             paste(
-              "Error when executing `teal_data_module`:\n ",
+              "Error when executing `teal_data_module` passed to `data`:\n ",
               paste(data$message, collapse = "\n"),
               "\n Check your inputs or contact app developer if error persists."
             )
@@ -108,7 +128,7 @@ srv_teal_with_splash <- function(id, data, modules, filter = teal_slices()) {
           need(
             FALSE,
             paste(
-              "Error when executing `teal_data_module`:\n ",
+              "Error when executing `teal_data_module` passed to `data`:\n ",
               paste(data$message, collpase = "\n"),
               "\n Check your inputs or contact app developer if error persists."
             )
@@ -120,8 +140,10 @@ srv_teal_with_splash <- function(id, data, modules, filter = teal_slices()) {
         need(
           inherits(data, "teal_data"),
           paste(
-            "Error: `teal_data_module` did not return `teal_data` object",
-            "\n Check your inputs or contact app developer if error persists"
+            "Error: `teal_data_module` passed to `data` failed to return `teal_data` object, returned",
+            toString(sQuote(class(data))),
+            "instead.",
+            "\n Check your inputs or contact app developer if error persists."
           )
         )
       )
@@ -132,7 +154,6 @@ srv_teal_with_splash <- function(id, data, modules, filter = teal_slices()) {
 
       is_modules_ok <- check_modules_datanames(modules, teal_data_datanames(data))
       if (!isTRUE(is_modules_ok)) {
-        logger::log_warn(is_modules_ok)
         validate(need(isTRUE(is_modules_ok), sprintf("%s. Contact app developer.", is_modules_ok)))
       }
 
@@ -143,7 +164,7 @@ srv_teal_with_splash <- function(id, data, modules, filter = teal_slices()) {
           type = "warning",
           duration = 10
         )
-        logger::log_warn(is_filter_ok)
+        warning(is_filter_ok)
       }
 
       teal_data_rv()