SafetyGraphics / safetyGraphics

Clinical Trial Safety Graphics with R
https://safetygraphics.github.io/safetyGraphics/
Other
97 stars 24 forks source link

prepareChart() sets an invalid `name` parameter if none is provided #636

Closed xni7 closed 2 years ago

xni7 commented 2 years ago

@jwildfire updateSelectizeInput() not updating in Example 3 of ChartConfiguration Vignette

See repex below with similar nature

# Shiny module example
library(safetyData) 
library(safetyGraphics)
library(dplyr)
library(yaml)

moduleTest_ui <- function(id) {
  ns <- NS(id) 
  wellPanel(
    actionButton(ns("b"), "update"),
    verbatimTextOutput(ns("bo")),
    numericInput(ns("test"), "Test", value = -1)
  )
}

moduleTest_server <- function(input, output, session, params) {

  observe({
    print(input$b)
    updateNumericInput(session = session,
                       inputId = "test",
                       value = input$b )  # or value = 99)
  })

  output[["bo"]] <- renderPrint({
    input$b
  })

}

testMod_yaml <- '
env: safetyGraphics
label: test Module
type: module
package: safetyCharts
domain: 
  - labs
workflow:
  ui: moduleTest_ui
  server: moduleTest_server
'

charts <- makeChartConfig() %>% purrr::keep(~.x$type == "NONE")
charts$testMod<-prepareChart(read_yaml(text=testMod_yaml))
safetyGraphicsApp(charts=charts)
jwildfire commented 2 years ago

The example in the vignette is modeled off of this function in safetyCharts which seems to run ok still. Will look at your reprex a bit later and see if i can advise ...

jwildfire commented 2 years ago

Definitely seeing the bug in the reprex. Not sure what the fix is honestly ... Also strange that the code download button isn't showing up for the chart in the app. Not sure if it's related or a separate bug ...

jwildfire commented 2 years ago

OK, found the (very random!) bug and updated the issue name accordingly. Issue is that prepareChart() assigns chart$name<-"safetyGraphics chart" by default. The space in that string does not work well when we call ns(chart$name) later on and was causing extra issues in the custom module you added. For now the fix is just to specify a name parameter (without spaces) in the YAML. Will file a PR with a fix for prepareChart() in a sec.

Will go update the vignette to make sure name is included in a second. Here's the fixed reprex:

# Shiny module example
library(safetyData) 
library(safetyGraphics)
library(dplyr)
library(yaml)

moduleTest_ui <- function(id) {
  ns <- NS(id) 
  wellPanel(
    actionButton(ns("b"), "update"),
    verbatimTextOutput(ns("bo")),
    numericInput(ns("test"), "Test", value = -1)
  )
}

moduleTest_server <- function(input, output, session, params) {

  observe({
    print(input$b)
    updateNumericInput(session = session,
                       inputId = "test",
                       value = input$b )  # or value = 99)
  })

  output[["bo"]] <- renderPrint({
    input$b
  })

}

##### NOTE that name parameter was added below ####
testMod_yaml <- '
env: safetyGraphics
name:testMod
label: test Module
type: module
package: safetyCharts
domain: 
  - labs
workflow:
  ui: moduleTest_ui
  server: moduleTest_server
'

charts <- makeChartConfig() %>% purrr::keep(~.x$type == "NONE")
charts$testMod<-prepareChart(read_yaml(text=testMod_yaml))
safetyGraphicsApp(charts=charts)
jwildfire commented 2 years ago

Ooops - pushed the solution directly to dev - need to set a protection to avoid that happening again. Here's the commit so you can see the changes.

@xni7 Can you just confirm that the updated version of prepareChart() in dev works for you. Try a YAML with no name and a YAML where the name parameter has spaces - both should work as expected now.

xni7 commented 2 years ago

@jwildfire the dev commit e3b0edb couldn't install...

Error: object ‘space’ is not exported by 'namespace:stringr'
Execution halted
ERROR: lazy loading failed for package ‘safetyGraphics’
jwildfire commented 2 years ago

@xni7 Sorry about that. Should be fixed now I think.

xni7 commented 2 years ago

pkg installed ok, but got an error in the makeChartConfig() call...

> charts <- makeChartConfig()

Error in stringr::str_replace_all(x, "[[:space:]]", "") : 
  object 'x' not found 
6.
stri_replace_all_regex(string, pattern, fix_replacement(replacement), 
                       vectorize_all = vec, opts_regex = opts(pattern)) 
5.
stringr::str_replace_all(x, "[[:space:]]", "") at prepareChart.R#21
4.
.f(.x[[i]], ...) 
3.
map(., prepareChart) 
2.
charts_raw %>% map(prepareChart) at makeChartConfig.R#90
1.
makeChartConfig() 
jwildfire commented 2 years ago

@xni7 sorry - give it one more try now ...

xni7 commented 2 years ago

Works well now!!

image

jwildfire commented 2 years ago

good good :) Closing out this issue.

@samussiah @bzkrouse - Note that there is a bug in the prepareChart() function in v2.0 that can mess up custom modules a bit (details above). It's resolved in dev now.