Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: data as reactive passed to srv_teal is not validated #1308

Closed
gogonzo opened this issue Aug 12, 2024 · 1 comment · Fixed by #1341
Closed

[Bug]: data as reactive passed to srv_teal is not validated #1308

gogonzo opened this issue Aug 12, 2024 · 1 comment · Fixed by #1341
Assignees
Labels
bug Something isn't working core

Comments

@gogonzo
Copy link
Contributor

gogonzo commented Aug 12, 2024

What happened?

data as reactive passed to srv_teal is not validated (only teal_data_module is)

This happens - no error messages, only log entry that Reactive data failed

app
options(
  teal.log_level = "TRACE",
  teal.show_js_log = TRUE,
  # teal.bs_theme = bslib::bs_theme(version = 5),
  shiny.bookmarkStore = "server"
)
library(scda)
pkgload::load_all("teal")
# pkgload::load_all("teal.slice")

ui_data <- function(id) {
  ns <- NS(id)
  tagList(
    actionButton(ns("submit"), label = "Submit to run")
  )
}

srv_data <- function(id, ...) {
  moduleServer(id, function(input, output, session) {
    eventReactive(input$submit, {
      data <- teal_data() |>
        within({
          logger::log_trace("Loading data")
          stop("error")
        })

      join_keys(data) <- default_cdisc_join_keys[c("ADSL", "ADTTE")]

      data
    })
  })
}

modules <- modules(
  teal.modules.general::tm_data_table("Data Table"),
  example_module("Example Module", datanames = "ADTTE"),
  module(
    ui = function(id) {
      ns <- NS(id)
      tagList(
        tableOutput(ns("filter_summary"))
      )
    },
    server = function(id, datasets) {
      moduleServer(id, function(input, output, session) {
        output$filter_summary <- renderTable({
          datasets$get_filter_overview(datanames = datasets$datanames())
        })
      })
    }
  )
)

shinyApp(
  ui = function(request) {
    fluidPage(
      ui_data("data"),
      ui_teal(id = "teal", modules = modules)
    )
  },
  server = function(input, output, session) {
    data_rv <- srv_data("data", data = data, modules = modules)
    srv_teal(id = "teal", data = data_rv, modules = modules)
  }
)
image
@gogonzo gogonzo added bug Something isn't working core labels Aug 12, 2024
@gogonzo gogonzo added this to the teal transform milestone Aug 12, 2024
@donyunardi
Copy link
Contributor

donyunardi commented Aug 12, 2024

Related to #1307

Acceptance Criteria

  • The validation warning of reactive teal_data should be passed to the teal module level, in addition to teal.
  • The validation should be shown in the teal application and console from module level.

@vedhav vedhav self-assigned this Aug 20, 2024
@vedhav vedhav linked a pull request Aug 22, 2024 that will close this issue
@vedhav vedhav linked a pull request Aug 28, 2024 that will close this issue
@gogonzo gogonzo linked a pull request Sep 17, 2024 that will close this issue
vedhav added a commit that referenced this issue Oct 3, 2024
Alternative solution to #1330. Closes #1304, #1307, and #1308

1. When teal_data_module fails, then teal-module-tabs are disabled. When
teal_data_module returns teal_data again teal-module-tabs are enabled
2. When reactive data passed directly to srv_teal fails, then the whole
tab-panel is hidden and error message is shown. Warning messages are
displayed over tab-panel.
3. when teal_transform_module fails then following
teal_transform_module(s) show generic message that something was wrong.
Reason for this is the same as (3).
4. when teal_transform_module fails then teal-module output is disabled
and generic failure message is shown in the main panel. We decided to
show a generic failure message as "real failure message" should be only
shown in the place where error occurred to no cause confusion.
5. failing teal_data_module/teal_transform_module fallbacks to previous
valid data (see exaplanation below)

The most important part of the implementation is that when
teal_data_module fails then it return the previous valid data (i.e. it
return unchanged data). This means that failure doesn't trigger
downstream reactivity and we don't need to deal with `data` input as
error. In other words, this implementation halts reactivity when
something goes wrong.
When something goes wrong, teal-module-output is hidden and instead
error message is displayed.

Also, I've moved `data` completely away from `ui` and now if there is
`teal_data_module` then data-tab is added dynamically.

<details>
<summary>app w/ teal_data_module</summary>

```r
options(
  teal.log_level = "TRACE",
  teal.show_js_log = TRUE,
  # teal.bs_theme = bslib::bs_theme(version = 5),
  shiny.bookmarkStore = "server"
)

# pkgload::load_all("teal.data")
pkgload::load_all("teal")

make_data <- function(datanames = c("ADSL", "ADTTE")) {
  data_obj <- teal.data::teal_data()
  if ("ADSL" %in% datanames) {
    data_obj <- within(data_obj, ADSL <- teal.data::rADSL)
  }
  if ("ADTTE" %in% datanames) {
    data_obj <- within(data_obj, ADTTE <- teal.data::rADTTE)
  }
  join_keys(data_obj) <- default_cdisc_join_keys[datanames]
  teal.data::datanames(data_obj) <- datanames
  data_obj
}

trans <- list(
  teal_transform_module(
    ui = function(id) {
      ns <- NS(id)
      tagList(
        selectizeInput(
          ns("errortype"),
          label = "Error Type",
          choices = c(
            "ok", "insufficient datasets", "no data",
            "qenv.error", "error in reactive", "validate error", "silent.shiny.error", "not a reactive"
          )
        )
      )
    },
    server = function(id, data) {
      moduleServer(id, function(input, output, session) {
        logger::log_trace("example_module_transform2 initializing.")
        reactive({
          switch(input$errortype,
            ok = data(),
            `insufficient datasets` = teal:::.subset_teal_data(data(), "ADSL"),
            `no data` = teal_data(),
            qenv.error = within(teal_data(), stop("\nthis is qenv.error in teal_transform_module\n")),
            `error in reactive` = stop("\nerror in a reactive in teal_transform_module\n"),
            `validate error` = validate(need(FALSE, "\nvalidate error in teal_transform_module\n")),
            `silent.shiny.error` = req(FALSE)
          )
        })
      })
    }
  )
)

data <- teal_data_module(
  once = FALSE,
  ui = function(id) {
    ns <- NS(id)
    tagList(
      selectizeInput(
        ns("errortype"),
        label = "Error Type",
        choices = c(
          "ok", "insufficient datasets", "no data",
          "qenv.error", "error in reactive", "validate error", "silent.shiny.error", "not a reactive"
        )
      ),
      actionButton(ns("submit"), "Go!")
    )
  },
  server = function(id, ...) {
    moduleServer(id, function(input, output, session) {
      logger::log_trace("example_module_transform2 initializing.")
      eventReactive(input$submit, {
        switch(input$errortype,
          ok = make_data(),
          `insufficient datasets` = make_data(datanames = "ADSL"),
          `no data` = teal_data(),
          qenv.error = within(data(), stop("\nthis is qenv.error in teal_data_module\n")),
          `error in reactive` = stop("\nerror in a reactive in teal_data_module\n"),
          `validate error` = validate(need(FALSE, "\nvalidate error in teal_data_module\n")),
          `silent.shiny.error` = req(FALSE)
        )
      })
    })
  }
)

app <- teal::init(
  data = data,
  modules = list(
    example_module("mod-1", transformers = c(trans, trans, trans), datanames = c("ADSL", "ADTTE")),
    example_module("mod-2", transformers = trans, datanames = c("ADSL", "ADTTE")),
    module(
      label = "I was made to annoy you",
      ui = function(id) NULL,
      server = function(id, data) {
        moduleServer(id, function(input, output, session) {
          observe({
            teal.data::datanames(data())
            ADSL <- data()[["ADSL"]]
            ADSL$AGE
          })

          observeEvent(data(), {
            print(data()[["ADSL"]]$SEX)
          })
        })
      },
      datanames = "ADSL"
    )
  ),
  filter = teal_slices(
    teal_slice("ADSL", "SEX"),
    teal_slice("ADSL", "AGE", selected = c(18L, 65L)),
    teal_slice("ADTTE", "PARAMCD", selected = "CRSD"),
    include_varnames = list(
      ADSL = c("SEX", "AGE")
    )
  )
)

runApp(app)

```

</details>

<details>
<summary>app wrapped</summary>

```r
options(
  teal.log_level = "TRACE",
  teal.show_js_log = TRUE,
  # teal.bs_theme = bslib::bs_theme(version = 5),
  shiny.bookmarkStore = "server"
)
library(scda)
pkgload::load_all("teal")

make_data <- function(datanames = c("ADSL", "ADTTE")) {
  data_obj <- teal.data::teal_data()
  if ("ADSL" %in% datanames) {
    data_obj <- within(data_obj, ADSL <- teal.data::rADSL)
  }
  if ("ADTTE" %in% datanames) {
    data_obj <- within(data_obj, ADTTE <- teal.data::rADTTE)
  }
  join_keys(data_obj) <- default_cdisc_join_keys[datanames]
  teal.data::datanames(data_obj) <- datanames
  data_obj
}

ui_data <- function(id) {
  ns <- NS(id)
  tagList(
    selectizeInput(
      ns("errortype"),
      label = "Error Type",
      choices = c(
        "ok", "insufficient datasets", "no data",
        "qenv.error", "error in reactive", "validate error", "silent.shiny.error", "not a reactive"
      )
    ),
    actionButton(ns("submit"), "Go!")
  )
}

srv_data <- function(id, ...) {
  moduleServer(id, function(input, output, session) {
    logger::log_trace("example_module_transform2 initializing.")
    eventReactive(input$submit, {
      switch(input$errortype,
        ok = make_data(),
        `insufficient datasets` = make_data(datanames = "ADSL"),
        `no data` = teal_data(),
        qenv.error = within(data(), stop("\nthis is qenv.error in teal_data_module\n")),
        `error in reactive` = stop("\nerror in a reactive in teal_data_module\n"),
        `validate error` = validate(need(FALSE, "\nvalidate error in teal_data_module\n")),
        `silent.shiny.error` = req(FALSE)
      )
    })
  })
}

modules <- modules(
  teal.modules.general::tm_data_table("Data Table"),
  example_module("Example Module", datanames = "ADTTE"),
  module(
    ui = function(id) {
      ns <- NS(id)
      tagList(
        tableOutput(ns("filter_summary"))
      )
    },
    server = function(id, datasets) {
      moduleServer(id, function(input, output, session) {
        output$filter_summary <- renderTable({
          datasets$get_filter_overview(datanames = datasets$datanames())
        })
      })
    }
  )
)

shinyApp(
  ui = function(request) {
    fluidPage(
      ui_data("data"),
      ui_teal(id = "teal", modules = modules)
    )
  },
  server = function(input, output, session) {
    data_rv <- srv_data("data", data = data, modules = modules)
    srv_teal(id = "teal", data = data_rv, modules = modules)
  }
)

```

</details>

---------

Signed-off-by: Vedha Viyash <[email protected]>
Signed-off-by: Marcin <[email protected]>
Co-authored-by: vedhav <[email protected]>
Co-authored-by: Vedha Viyash <[email protected]>
Co-authored-by: m7pr <[email protected]>
Co-authored-by: Marcin <[email protected]>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: 27856297+dependabot-preview[bot]@users.noreply.github.com <27856297+dependabot-preview[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment