Support settings requirements for charts

[jwildfire]

Summary

Version 1 of the app documented chart-level settings requirements in this file (e.g. outlier-explorer required id_col, measure_col and value_col). This allowed us to enable or disable a chart based on whether all of the required mappings were present. We should consider whether reimplementing this functionality is worthwhile in v2 (or as a later release). Currently, v2 just tries to draw the chart no matter what. If needed settings are missing, the user generally just sees a blank screen for the chart.

Details

I'm thinking we'd capture the chart requirements by expanding the domain section in the YAML. For example, the YAML for outlier explorer would look like:

env: safetyGraphics
label: Outlier Explorer - widget
type: htmlwidget
domain: 
  - labs
    - id_col
    - measure_col
    - value_col
    - ... 
package: safetyCharts
export: true
...

Then we'd need to add some basic validation to the charting modules. Basically we'd check to see whether all of the required settings are present before deciding to render the chart. We could also show ✅ or ❌ in the charts dropdown to indicate whether the chart is available (like we did in v1).

I definitely think this would be nice to have, but am on the fence about whether it's needed for v2.0. Curious about what others think - @xni7 @elimillera @samussiah @JimBuchanan @mli1 (and everyone) any thoughts?

[JimBuchanan]

Yes, definitely nice to have as I discovered with my data. But maybe not a requirement for the 2.0 release. For the first release I would suggest we inform users about data structure requirements in lieu of a settings check. I didn’t realize that there were trailing spaces after my lab data, for example, or that the subjid was in a numerical rather than character format. The spreadsheet I was using originally came from a laboratory service, so it’s entirely possible that other users will encounter similar oddities in their data before they have it converted to STDM.

[elimillera]

I would definitely agree we would want a type for variables. I would suggest attempting to coerce them if possible before erroring out. I think this could be left for v2.1 but I would be happy to help out to add it

[jwildfire]

Feel free to start a PR if you have time @elimillera. Typing is also related to this #563. I think we'd start by adding a new type column in safetyGraphics::meta. Not exactly sure the next steps from there, but I put a few thoughts in the other issue.

[samussiah]

At the very least it would be super useful to hint at missing variables on the client side. I'm assuming the mapping module is metadata-driven but unconnected to the chart config files. As a (possibly) quick stopgap, we could dump R output to the UI on error.

[jwildfire]

As a (possibly) quick stopgap, we could dump R output to the UI on error.

This already happens for static R charts and modules to an extent. I think the confusion is (mostly) when an htmlwidget fails to render, then any errors are logged to the web console, which is not at all obvious for most R users ...

[elimillera]

@jwildfire Could we add an that defaults to printing out to the R console but can output to the web console? I think its important for production deployments because most people won't have access to the R console if its deployed on a shiny server or something like that.

[jwildfire]

I've been looking at the {validate} package and the awesome associated cookbook, and think it might be a good option for this issue and/or #563. I especially like that it's already easy to define validation rules as YAML as shown here. With that already in place, I think we could just add a rules parameter in each charts YAML and then show validation status in the app.

[jwildfire]

Removing this from v2.1, but likely to revisit in a future release.

[jwildfire]

Thinking that putting a simplified (and optional) version of this in for v2.2 would be good. Could even hold v2.1 a week if people like the proposal below.

In short, I'm thinking we could add an optional requiredSettings parameter to the chart config:

env: safetyGraphics
label: Outlier Explorer - widget
type: htmlwidget
requiredSettings: 
  - labs
    - id_col
    - measure_col
    - value_col
    - ... 
package: safetyCharts
export: true
...

Then have a checkRequiredSettings() function like this trigger just before a chart is rendered:

if(hasProperty(chart, "requiredSettings)){
    missingSettings<-c()

    for(domain in chart$requiredSettings){
        for(setting in chart$requiredSettings[[domain]]){
              if(is.null(meta[[domain]][[setting]]) missingSettings <- c(missingSettings, meta[[domain]][[setting]])
        }
     }
    # Being lazy with loops here - can probably refactor to 1 or 2 lines using `purrr::map()` and `any`
     if(length(missingSettings) > 0) {
         # print a message listing the missing settings
      } else { 
          # render the chart
      }
}

Seems pretty straightforward, and I think it would be helpful as people start developing charts in new domains.

@xni7 @elimillera @bzkrouse @samussiah Any thoughts? Unintended consequences I'm missing?

[jwildfire]

I suppose we could also support types here ...

env: safetyGraphics
label: Outlier Explorer - widget
type: htmlwidget
requiredSettings: 
  - labs
    - id_col: character
    - measure_col: character
    - value_col: numeric
    - ... 
package: safetyCharts
export: true
...

Though I'd probably lean towards doing the simple version first and then adding that later.

[xni7]

Thinking that putting a simplified (and optional) version of this in for v2.2 would be good. Could even hold v2.1 a week if people like the proposal below.

In short, I'm thinking we could add an optional requiredSettings parameter to the chart config:

env: safetyGraphics
label: Outlier Explorer - widget
type: htmlwidget
requiredSettings: 
  - labs
    - id_col
    - measure_col
    - value_col
    - ... 
package: safetyCharts
export: true
...

Then have a checkRequiredSettings() function like this trigger just before a chart is rendered:

if(hasProperty(chart, "requiredSettings)){
    missingSettings<-c()

    for(domain in chart$requiredSettings){
        for(setting in chart$requiredSettings[[domain]]){
              if(is.null(meta[[domain]][[setting]]) missingSettings <- c(missingSettings, meta[[domain]][[setting]])
        }
     }
    # Being lazy with loops here - can probably refactor to 1 or 2 lines using `purrr::map()` and `any`
     if(length(missingSettings) > 0) {
         # print a message listing the missing settings
      } else { 
          # render the chart
      }
}

Seems pretty straightforward, and I think it would be helpful as people start developing charts in new domains.

@xni7 @elimillera @bzkrouse @samussiah Any thoughts? Unintended consequences I'm missing?

@jwildfire I like this idea and suggest keeping it simple, by first requiring existence of mandatory mappings. It will get trickier if we need to also validate other attributes such as type. Jim had an example with trailing white space in the characters. Then we would need to handle the err msgs. https://developer.mozilla.org/en-US/docs/Learn/Forms/Form_validation#using_built-in_form_validation