928 upload snapshot file

[chlebowa]

Closes #928

Added possibility to upload a snapshot file to the snapshot manager module.
The snapshot manager control bar (top row in table) receives a third button. Clicking it opens a modal dialog that allows for selecting a .json file to upload and naming the snapshot.
The new snapshot is added to the snapshot list and the filter manager dialog is reopened so that the user may immediately apply the new snapshot.

To ensure that the uploaded snapshot matches the app, teal_slices class receives the app_id attribute, which can be set with the app_id argument to teal::teal_slices. The attribute will be set by init to stamp a teal_slices. The attribute will be stored along with other attributes. Upon uploading a snapshot, the app id of the upload will be compared to that of the app.

slices_restore had to be re-defined in the teal namespace because it calls teal_slices and that must be the one from teal.

TESTING

options(teal.log_level = "WARN", teal.show_js_log = FALSE)

library(teal.modules.general)
pkgload::load_all("../teal.slice")
pkgload::load_all("../teal")

rm(list = ls())

funny_module <- function (label = "Filter states", datanames = "all") {
  checkmate::assert_string(label)
  module(
    label = label,
    datanames = datanames,
    ui = function(id, ...) {
      ns <- NS(id)
      div(
        h2("The following filter calls are generated:"),
        verbatimTextOutput(ns("filter_states")),
        verbatimTextOutput(ns("filter_calls")),
        actionButton(ns("reset"), "reset_to_default")
      )
    },
    server = function(input, output, session, data, filter_panel_api) {
      checkmate::assert_class(data, "tdata")
      observeEvent(input$reset, set_filter_state(filter_panel_api, default_filters))
      output$filter_states <-  renderPrint({
        logger::log_trace("rendering text1")
        filter_panel_api %>% get_filter_state()
      })
      output$filter_calls <- renderText({
        logger::log_trace("rendering text2")
        attr(data, "code")()
      })
    }
  )
}

dead_module <- function(label = "empty module", datanames = NULL) {
  module(label = label,
         datanames = datanames,
         ui = function(id) {
           ns <- NS(id)
           tagList(
             h4("this is just text")
           )
         },
         server = function(id, filter_panel_api, reporter) {
           message("hello")
         }
  )
}

default_filters <- teal::teal_slices(
  teal_slice("iris", "Sepal.Length"),
  teal_slice("iris", "Sepal.Width"),
  teal_slice("iris", "Species", fixed = TRUE),
  teal_slice("mtcars", "mpg"),
  exclude_varnames = list(
    iris = c("Petal.Length"),
    mtcars = c("qsec", "drat")
  ),
  module_specific = TRUE,
  mapping = list(
    table = "iris Species",
    funny1 = c("iris Sepal.Length", "iris Sepal.Width", "iris Species"),
    funny2 = "iris Species",
    global_filters = "mtcars mpg"
  )
)

app <- init(
  data = teal_data(
    dataset("iris", iris),
    dataset("mtcars", mtcars)
  ),
  modules = modules(
    tm_data_table(
      "table",
      variables_selected = list(),
      dt_args = list()
    ),
    modules(
      label = "tab1",
      funny_module("funny1", datanames = NULL),
      funny_module("funny2", datanames = "iris"),
      dead_module("empty", datanames = NULL)
    )
  ),
  filter = default_filters
)

shinyApp(app$ui, app$server, options = list(launch.browser = TRUE))

[github-actions]

Code Coverage Summary

Filename                         Stmts    Miss  Cover    Missing
-----------------------------  -------  ------  -------  -------------------------------------------------------------------------------------------------------------------------------
R/dummy_functions.R                 88      63  28.41%   9-76, 101-104, 107-111
R/get_rcode_utils.R                 46       1  97.83%   49
R/include_css_js.R                  24       0  100.00%
R/init.R                            86      28  67.44%   182-189, 194-215, 227-229
R/module_filter_manager.R          107      29  72.90%   62-70, 79-84, 228, 233-246
R/module_nested_tabs.R             170      16  90.59%   72, 119, 123-124, 138-145, 163, 216, 238, 271
R/module_snapshot_manager.R        209     157  24.88%   86-98, 126-135, 139-151, 153-160, 167-181, 185-187, 189-194, 197-207, 210-226, 235-250, 264-287, 290-301, 304-310, 324, 345-368
R/module_tabs_with_filters.R        67       1  98.51%   95
R/module_teal_with_splash.R         33       2  93.94%   65, 77
R/module_teal.R                    164      12  92.68%   68, 71, 158-159, 209-210, 230-233, 235, 239
R/modules_debugging.R               18      18  0.00%    25-44
R/modules.R                        128      22  82.81%   206-211, 222-226, 341-384
R/reporter_previewer_module.R       17       2  88.24%   23, 27
R/show_rcode_modal.R                20      20  0.00%    16-37
R/tdata.R                           41       2  95.12%   146, 172
R/teal_reporter.R                   60       5  91.67%   65, 116-117, 120, 137
R/teal_slices-store.R               25       0  100.00%
R/teal_slices.R                     59      12  79.66%   125-138
R/utils.R                           33       0  100.00%
R/validate_inputs.R                 32       0  100.00%
R/validations.R                     60      37  38.33%   111-373
R/zzz.R                             11       7  36.36%   3-14
TOTAL                             1498     434  71.03%

Diff against main

Filename                       Stmts    Miss  Cover
---------------------------  -------  ------  -------
R/init.R                          +9       0  +3.81%
R/module_snapshot_manager.R      +61     +52  -4.17%
R/teal_slices.R                   +2       0  +0.71%
TOTAL                            +72     +52  -2.18%

Results for commit: 8250dfb

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

[chlebowa]

I have two questions:

1. Should the user be able to upload several files at once? Currently they are not. Naming multiple snapshots can be bothersome and I think naming is important.
2. Should the newly loaded snapshot be automatically applied? Currently it is not.

Note that (1) and (2) are mutually exclusive.

[github-actions]

Unit Tests Summary

    1 files    17 suites   16s ⏱️
178 tests 178 ✔️ 0 💤 0 ❌
359 runs  359 ✔️ 0 💤 0 ❌

Results for commit 8250dfb.

♻️ This comment has been updated with latest results.

[donyunardi]

Thanks for working on this so quickly, Aleks.

1. Should the user be able to upload several files at once? Currently they are not. Naming multiple snapshots can be bothersome and I think naming is important.

I agree that naming is important. Let's stick with uploading one file at a time for now and show this to the user. If a valid business case arises for loading multiple files, then we can consider it.

2. Should the newly loaded snapshot be automatically applied? Currently it is not.

I did expect the newly loaded snapshot to be automatically uploaded, especially since we're uploading one file at a time. Is it a simple fix to enable this? If so, I would prefer/vote for the snapshot to be automatically uploaded.

[donyunardi]

Code looks good to me.
Maybe adding NEWS.md and test cases?

[chlebowa]

2. Should the newly loaded snapshot be automatically applied? Currently it is not.

I did expect the newly loaded snapshot to be automatically uploaded, especially since we're uploading one file at a time. Is it a simple fix to enable this?

It is. Will do.

[m7pr]

@chlebowa should we remove from teal.slice?

[chlebowa]

I believe teal.slice should contain everything that can be done with a teal_slice or a teal_slices. The fact that we need a copy in teal is an unfortunate consequence of the teal extension of the teal_slices class.

[m7pr]

ok, good to know. slices_restore will be extended by insightsengineering/teal.slice#432 so it's good to keep that in mind, which I know you had!

[chlebowa]

Yes, I will copy the changes as soon as they are merged into teal.slice.

[averissimo]

Could we add an argument to the function in teal.slices::slices_restore() where the function is passed. This way the method on {teal} would only have to call teal.slices::slices_restore with teal::teal_slices?

I'm not sure if this is less convoluted than having duplicated code

[chlebowa]

What do you mean by adding an argument? Can you give some example code?

[averissimo]

{teal.slices}

slices_restore <- function(file, teal_slices_fun = teal_slices) {
 ....
}

{teal}

slices_restore <- function(file) {
  teal.slices::slices_restore(file, teal_slices)
}

edit: or keep the same parameters to maintain API consistency for the function across teal and teal.slices

[chlebowa]

hmmm 🤔

[chlebowa]

As neat as it is, it does not resolve the underlying problem (which I had missed). Consider this:
There are two versions of teal_slices and two respective versions slices_restore. Start a fresh session.

A teal_slices object created under teal.slice can be restored by both restore funcitons.

x <- teal.slice::teal_slices(
  teal.slice::teal_slice("iris", "Species")
)
teal.slice::slices_store(x, "testfile.json")
y <- teal.slice::slices_restore("testfile.json")
z <- teal:::slices_restore("testfile.json")

However, a similar object created under teal can only be loaded by the function from teal.

X <- teal::teal_slices(
  teal.slice::teal_slice("iris", "Species")
)
teal.slice::slices_store(X, "testfile.json")
Y <- teal.slice::slices_restore("testfile.json")   # FAILS
Z <- teal:::slices_restore("testfile.json")

After some discussion with @gogonzo and @m7pr we decided to move slices_store and slices_restore to teal. Only then will there be no opportunity for error.

Thank you for raising this @averissimo 👍

[chlebowa]

@donyunardi
I'm not sure I can write tests for this.
NEWS is amended.

What is missing is ensuring that the uploaded teal_slices can be applied to the app where it was uploaded. We need an app ID of a sort.

[chlebowa]

One way to provide an app id (from @gogonzo) is to have init hash all of its arguments.

I imagine something like

rlang::hash(c(mget(names(formals(init))), match.call()))

The hash could be added to the teal_slices coming from the filter argument as an additional attribute. teal::teal_slices would then get an optional argument like app_id and that way the hash would be able to be stored and restored in the teal_slices.

There could be trouble, e.g. due to the teal_data refactor the same data will be passed in a different form, likely resulting in a different hash.

We don't even have to go that far, that line up there gives me a different string every time I run the same example app. I guess it might be caused by the fact that we use environments and they are never identical.

[chlebowa]

As I suspected, the different hash results from the fact that data and filter arguments in init effectively take environments (TealDataset and teal_slices, respectively).
Here is how to obtain a reproducible hash:

hashables <- c(mget(names(formals(init))), call = match.call())
hashables$data <- lapply(hashables$data$get_datanames(), function(dn) hashables$data$get_dataset(dn)$get_raw_data())
hashables$filter <- as.list(hashables$filter, recursive = TRUE)
rlang::hash(hashables)

Naturally, the second line will change once new teal_data is introduced.

[kartikeyakirar]

I experimented with a JSON file, making manual changes such as modifying and deleting fields or incorrect field name. When an invalid JSON file is uploaded, it causes the application to crash. Is it feasible to implement a mechanism to verify the presence of required fields and, if they are missing, display a notification indicating what's wrong with the JSON file?

[chlebowa]

I experimented with a JSON file, making manual changes such as modifying and deleting fields or incorrect field name. When an invalid JSON file is uploaded, it causes the application to crash. Is it feasible to implement a mechanism to verify the presence of required fields and, if they are missing, display a notification indicating what's wrong with the JSON file?

Please be more specific.
Any errors raised in such cases probably arise in slices_restore. If anything, we should add guards there. @m7pr

[kartikeyakirar]

Please be more specific.
Any errors raised in such cases probably arise in slices_restore.

My point was to ensure that no user action would cause the application to crash, even if they made manual changes or accidentally deleted a field. and Yes all the errors seem to originate from the slices_restore function. but I agree we can not save user from everything.

some of the scenarios I checked was : passing an empty JSON file and changing the JSON field names. These actions led to application crashes.

[chlebowa]

some of the scenarios I checked was : passing an empty JSON file and changing the JSON field names. These actions led to application crashes.

This is what I was asking about 👍

[donyunardi]

It felt solid to me. 👍🏼

[kartikeyakirar]

I've tested the JSON from sample app to teal.gallery app, and the inclusion of hashes is functioning well. The pull request provides excellent functionality. 👍 💯

I

[vedhav]

Can someone also review the related PR #930

[chlebowa]

Thanks to @m7pr for testing with a file sent over email. I believe the requested functionality is satisfied.

[chlebowa]

This PR is now blocked until 1) the change to slices_restore is resolved and 2) slices_store and slices_restore are moved to teal in a separate PR.