{teal} module returns a teal_report object that extends from teal_data

[averissimo]

Pull Request

Fixes:

• [Question]: How to return report to teal? #1526

Built on top of:

• Redesign of teal.reporter Cards #1499 (will be closed once this PR is stable)

Companion PRs:

• {teal} module returns a teal_report object that extends from teal_data #1541
• {teal} module returns a teal_report object that extends from teal_data teal.code#255
• {teal} module returns a teal_report object that extends from teal_data teal.data#370
• {teal} module returns a teal_report object that extends from teal_data teal.reporter#331
• {teal} module returns a teal_report object that extends from teal_data teal.modules.general#884

Changes description

• Adds a "Add to reporter" when a teal_report object is returned from the module
• Changes modify_reactive_output() to after()

Sample app

# Make sure the required branches are checked-out
# - teal_reporter (all 4: teal, teal.reporter, teal.code, teal.data)
# - redesign@main (teal, teal.reporter, teal.code)
pkgload::load_all("../teal.code")
pkgload::load_all("../teal.data")
pkgload::load_all("../teal.reporter")
pkgload::load_all("../teal")

example_extended <- function(label = "example teal module",
                                 datanames = "all",
                                 transformators = list(),
                                 decorators = list()) {
  checkmate::assert_string(label)
  checkmate::assert_list(decorators, "teal_transform_module")
  
  mod <- example_module(label, datanames, transformators, decorators)
  
  module(
    label,
    server = function(id, data, decorators) {
      moduleServer(id, function(input, output, session) {
        result <- mod$server("example", data, decorators)
        
        reactive({
          data <- result()
          report(data) <- c(doc("## Code"), report(data), "## Table", data$object)  
          data
        })
        
      })
    },
    ui = function(id, decorators) mod$ui(shiny::NS(id, "example"), decorators),
    ui_args = mod$ui_args,
    server_args = mod$server_args,
    datanames = mod$datanames,
    transformators = mod$transformators
  )
}

example_old_reporter <- function(label = "example teal module",
                                 datanames = "all",
                                 transformators = list(),
                                 decorators = list()) {
  checkmate::assert_string(label)
  checkmate::assert_list(decorators, "teal_transform_module")
  
  ans <- module(
    label,
    server = function(id, data, decorators, reporter, filter_panel_api) {
      moduleServer(id, function(input, output, session) {
        result <- example_module()$server("example", data, decorators)
        
        teal.widgets::verbatim_popup_srv(
          id = "rcode",
          verbatim_content = reactive(teal.code::get_code(req(result()))),
          title = "Example Code"
        )
        
        if (inherits(reporter, "Reporter")) {
          card_fun <- function(comment, label) {
            card <- teal::report_card_template(
              title = "Example plot",
              label = label,
              with_filter = FALSE,
              filter_panel_api = filter_panel_api
            )
            card$append_rcode(get_code(result()))
            card$append_text("Table", "header3")
            card$append_table(result()[["object"]])
            card
          }
          teal.reporter::simple_reporter_srv("simple_reporter", reporter = reporter, card_fun = card_fun)
        }
      })
    },
    ui = function(id, decorators) {
      ns <- NS(id)
      teal.widgets::standard_layout(
        output = verbatimTextOutput(ns("example-text")),
        encoding = tags$div(
          teal.reporter::simple_reporter_ui(ns("simple_reporter")),
          selectInput(ns("example-dataname"), "Choose a dataset", choices = NULL),
          ui_transform_teal_data(ns("example-decorate"), transformators = decorators),
          teal.widgets::verbatim_popup_ui(ns("rcode"), "Show R code")
        )
      )
    },
    ui_args = list(decorators = decorators),
    server_args = list(decorators = decorators),
    datanames = datanames,
    transformators = transformators
  )
  attr(ans, "teal_bookmarkable") <- TRUE
  ans
}


teal::init(
  data = within(teal_data(), {iris <- iris}),
  modules = modules(
    example_extended(label = "🆕 Module (extended)"),
    example_old_reporter(label = "⏲️ Old reporter"),
    example_module(label = "🆕 Module (from {teal})"),
    example_module(label = "❌️ No reporter") |> disable_report()
  )
) |> shiny::runApp()

[averissimo]

@gogonzo is this a typo? call_module is not defined anywhere

Suggested change

	do.call(call_module, args = original_args)
	do.call(shiny::callModule, args = original_args)

[github-actions]

Code Coverage Summary

Filename                          Stmts    Miss  Cover    Missing
------------------------------  -------  ------  -------  ----------------------------------------------------------------------------------------------------------------------------------------
R/after.R                            63      63  0.00%    16-90
R/checkmate.R                        24       0  100.00%
R/dummy_functions.R                  67      11  83.58%   46, 48, 90-98
R/include_css_js.R                   22      17  22.73%   12-38, 76-82
R/init.R                            148     102  31.08%   139-160, 189-197, 207-232, 235-242, 249-258, 261-270, 273-282, 286-296, 298
R/landing_popup_module.R             34      34  0.00%    22-57
R/module_bookmark_manager.R         161     130  19.25%   47-68, 88-142, 147-148, 160, 207, 242-319
R/module_data_summary.R             177      10  94.35%   25-26, 40, 50, 205, 236-240
R/module_filter_data.R               64       2  96.88%   22-23
R/module_filter_manager.R           230      57  75.22%   56-62, 73-82, 90-95, 108-112, 117-118, 291-314, 340, 367, 379, 386-387
R/module_init_data.R                 84       0  100.00%
R/module_nested_tabs.R              319     173  45.77%   40-214, 246, 271-273, 379-382, 386-389, 393-396, 440
R/module_session_info.R              18       7  61.11%   35-41
R/module_snapshot_manager.R         222     152  31.53%   89-95, 104-113, 121-133, 152-153, 170-186, 190-205, 207-214, 221-236, 240-244, 246-252, 255-268, 271-279, 309-323, 326-337, 340-346, 360
R/module_teal_data.R                149      76  48.99%   43-149
R/module_teal_lockfile.R            131      55  58.02%   33-37, 45-57, 60-62, 76, 86-88, 92-96, 100-102, 110-119, 122, 124, 126-127, 161-162, 196-201
R/module_teal_with_splash.R          33      33  0.00%    24-61
R/module_teal.R                     160      51  68.12%   50-99, 116, 130-131, 169
R/module_transform_data.R           116       7  93.97%   20, 46, 130-134
R/modules.R                         285      51  82.11%   174-178, 233-236, 360-380, 388, 394, 571-577, 590-598, 613-628
R/reporter_previewer_module.R        59       0  100.00%
R/show_rcode_modal.R                 23      23  0.00%    17-41
R/tdata.R                            14      14  0.00%    19-61
R/teal_data_module-eval_code.R       22       0  100.00%
R/teal_data_module-within.R           7       0  100.00%
R/teal_data_module.R                 20       0  100.00%
R/teal_data_utils.R                  40       0  100.00%
R/teal_modifiers.R                   71      71  0.00%    26-214
R/teal_reporter.R                   184      95  48.37%   69, 77-82, 131-132, 135, 152, 201-291, 296, 347-351
R/teal_slices-store.R                29       0  100.00%
R/teal_slices.R                      63       0  100.00%
R/teal_transform_module.R            45       0  100.00%
R/TealAppDriver.R                   385     385  0.00%    57-776
R/utils.R                           250      38  84.80%   400-449
R/validate_inputs.R                  32       0  100.00%
R/validations.R                      58      37  36.21%   118-406
R/zzz.R                              15      11  26.67%   4-18
TOTAL                              3824    1705  55.41%

Diff against main

Filename                          Stmts    Miss  Cover
------------------------------  -------  ------  --------
R/after.R                           +63     +63  +100.00%
R/init.R                             +6      +6  -1.31%
R/module_init_data.R                +10       0  +100.00%
R/module_nested_tabs.R               -2      +1  -0.65%
R/module_snapshot_manager.R          +6      +6  -0.88%
R/module_teal.R                      +7       0  +1.46%
R/modules.R                           0      -2  +0.70%
R/reporter_previewer_module.R       +40      -1  +5.26%
R/teal_data_module-eval_code.R       -2       0  +100.00%
R/teal_data_utils.R                 +30       0  +100.00%
R/teal_reporter.R                  +114     +87  -40.20%
TOTAL                              +272    +160  -1.09%

Results for commit: abb2307

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

[averissimo]

Some thoughts before closing for the week:

• "Add to reporter" is vulnerable to module output having a validation error / shinysilenterror (See tm_missing_data and see it momentarily show the same validation error as the module)
• teal.reporter::disable_reporter() returns NULL, this seems like a bad practice if we want to expand on the feature.

[pawelru]

I think that this is a really bad name that can mean anything. It feels very much async related. Please reassess the name.

[m7pr]

Just to let you know @pawelru - this is a function that edits/manipulates teal_data (now extended to teal_report class) after it was returned by module's server. It is applied on a module level.

So basically it is:

module() |>
    edit_data_returned_by_server()

[donyunardi]

How about modify_teal_module or edit_teal_module?
Then, we'll have .modify/edit_teal_module_ui and .modify/edit_teal_module_srv

EDIT: I saw the comment that this was previously called modify_reactive_output.

[m7pr]

Since after is applied like below

<teal_module> |> after(
        server = function(data) {
          teal.reporter::teal_card(data) <- c(
            teal.reporter::teal_card(data),
            "# added content",
            "This content has been added through `after`"
          )
          data
        }
      )

I think

#' @param x (`teal_data`)

should be updated to

#' @param x (`teal_module`)

and

server = function(input, output, session, data) data

to

server = function(data) data

?

[m7pr]

Are those checks even working if we pass server function without input, output and session?

<teal_module> |> after(
        server = function(data) {
          teal.reporter::teal_card(data) <- c(
            teal.reporter::teal_card(data),
            "# added content",
            "This content has been added through `after`"
          )
          data
        }
      )

[m7pr]

Suggested change

	#' @param x (`teal_data`)
	#' @param x (`teal_module`)

[donyunardi]

First off, I really appreciate the exploration and research that been poured into this PR.

I have two observations so far:

Automatic conversion from teal_data to teal_report in module's server

With the current approach, teal_data is automatically converted into a teal_report object as soon as it enters a module's server, even if I don’t intend to use the reporter feature in my custom teal module.

I personally would prefer to see teal_data object unless I intentionally need a reporter feature.
It makes more sense to keep the data as teal_data and only convert to teal_report when I decided to use the reporter feature using the as.teal_report() function.

Extending teal_data class to make teal_report class

I do see value in converting teal_data to teal_report. This approach allows me to continue using all teal_data methods while also gaining access to new teal_report methods. In practice, I would just need to learn how to use teal_card and how to apply the keep_output argument within eval_code() and within() to capture objects for reporting.

However, this raises a design question: is it appropriate for teal_report to extend teal_data?

Historically, teal_data and teal_report have served different purposes and been treated as separate entity. Personally, I can see them as naturally related. Our reporting feature is about capturing the results of data visualizations or outputs, which are typically driven by the data, filter setup, and transformation inside teal_data.

However, it's worth noting that a teal_card can include any content that may not be related to teal_data object. If that happens, this may weaken the relationship between teal_report and teal_data and might justify the reason to keep the two class separate.

I'll keep thinking about this PR.

[gogonzo]

With the current approach, teal_data is automatically converted into a teal_report object as soon as it enters a module's server, even if I don’t intend to use the reporter feature in my custom teal module.

This happens anyway now, nobody raised this problem before and even it was appreciated:

• If module's server doesn't use join_keys teal_data isn't needed as qenv is enough.
• If module's server doesn't use reproducibility feature, qenv wouldn't be needed neither - list containing data would be enough.

I personally would prefer to see teal_data object unless I intentionally need a reporter feature. It makes more sense to keep the data as teal_data and only convert to teal_report when I decided to use the reporter feature using the as.teal_report() function.

Using this in all modules wouldn't be a big deal.

function(id, data, ...) {
  d <- reactive(as.teal_report(data))
  
  ... # use d
}

Extending teal_data class to make teal_report class

... In practice, I would just need to learn how to use teal_card and how to apply the keep_output argument within eval_code() and within() to capture objects for reporting.

Not true. In practice module-developer doesn't have to learn anything unless one decide to migrate to new reporting system.

However, it's worth noting that a teal_card can include any content that may not be related to teal_data object. If that happens, this may weaken the relationship between teal_report and teal_data and might justify the reason to keep the two class separate.

Yes, it would weaken a relationship, expose a report for more errors and limit functionality. It would also have it's codebase consequences with increased number of lines in teal_module's server.
To build the report one would have to write an extra reactive and reuse teal_data used in the module's server and insert into a report according to the order. This sounds good on a paper because @code is unsubsettable. It means that get_code would return the whole code as a single code-chunk

If one undermines teal_report on top of teal_data and says that code and environment are conceptually related, I'd come up with the idea that code is one of the elements of the document and it's coexistence with environment in qenv is driven by specific historical (hehe) need. code is really a part of the document.

• code_chunk is a separate class same as markdown content and metadata (header or document properties)
• card contains blocks and it can be anything
• teal_report inherits card and environment and serves as a environment with the document
• teal_data inherits from teal_report

This is just a quick justification or a provocation, because at the end object delivered to the module would have the same methods as it has now (on the feature branch).

Now, jokes aside. I consider current feature branch design as a good one where we didn't really made any compromises. Every new feature extends previous classes and adds new slots/methods on top.

[m7pr]

I will just add that I think learning about teal_report after acknowleding teal_data is easier than learning the current teal.reporter R6 ReportCards, which I still struggle with after 2-3 years on a project.

[vedhav]

I prefer a solution on top of teal_data as well. I think in general R users find it hard to use R6 classes primarily due to the fact that they are not as common as S3 or S4 objects.
Building teal_reporter on top of teal_data makes learning so much easier. In my head the whole teal framework is an abstraction on top of teal_data and shiny modules. And we should keep it this way.

[donyunardi]

This happens anyway now, nobody raised this problem before and even it was appreciated:
If module's server doesn't use join_keys teal_data isn't needed as qenv is enough.
If module's server doesn't use reproducibility feature, qenv wouldn't be needed neither - list containing data would be enough.

When I conduct training/workshop, I usually show users that the data in custom module is teal_data class and create that connection of using teal_data() constructor when creating teal_data object in init(). If we automatically convert this to teal_report class, I am worried that it will create confusion and thus have to explain why we convert this to teal_report class.

We agreed that teal_data is that default data object that's being passed around teal framework. Is qenv or list of containing data enough? Sure. But I appreciate the consistency of just using teal_data and train user to understand teal_data, whether or not they're joining datasets or using the code reproducibility feature.

---

Although it's possible, it's hard for me to think that someone will intentionally include any content that is not related to the teal_data/real_report object. Arguably, I can even do that with the current R6 solution with the $append_text() method.

---

In addition to teal_data and cdisc_data, with the current approach, I can now do:

x <- teal_report(
  iris = iris, 
  mtcars = mtcars
)

Is the idea to also introduce teal_report as a constructor? I think this may be one too many. Perhaps this is also further support my suggestion to convert teal_data to teal_report inside the module.

---

From release perspective, this means teal.data will have to be released before teal.reporter. Not a big deal we agree if this is the way to go.

[donyunardi]

The team had another go at discussing this solution.

The fundamental change in this approach is that we are now including teal.reporter as a default feature of the teal framework. This is reflected by the fact that the object we pass to the module will be reporter-ready.

We also talked about the benefit of automatically converting this to teal_report, which allows the initial teal_card creation to be handled by teal. This means we can create a separate code chunk for data pre-processing and filtering activity before the object enters the module server. The user can then focus only on adding the module’s analysis code chunk to the teal_report object.

This also means that module developers no longer need to worry about passing the filter_panel_api object to the module, which is mostly used to include filtering information when crafting the reporter. Advanced teal module developers can still bring in the filter_panel_api if needed as we're not removing this access.

Modules need to return a reactive teal_report object, so developers need to ensure this is the final object in the module. We need to make this convention clear. When followed, the "Add to Report" card will be added automatically, with no UI or server setup needed. It would be helpful to add a check to alert developers if the module doesn’t return a reactive teal_report object.

From the user’s perspective, I think this solution will be well received by our module developers and will simplify the process of building teal modules.

This solution may also enable automatic creation of the "Show R Code" button, another feature we may offer by default in the teal framework.

We will not introduce teal_report as a separate constructor, and teal_data will continue to be the entry point. We won’t expose this change in our documentation.

There is one question that I thought when drafting this summary, if in the future we decide to add another feature involving both data and reporting, how easy will it be to extend the proposed design? Would we need to extend teal_report into another S4 class to add new slots and methods? Is bookmarking feature relatable with this topic?

[kumamiao]

From user's perspective, I like that this proposal makes it much simpler for the user (module developer) to be able to create a module with the reporter feature, in a nice structured way.

In terms of converting the teal_data object to teal_report, in my opinion, most users would not really care too much about the switch of class of the object, as long as we provide clear guidance on how this object is used to add the report feature within the module. We can also mention the switch in the vignette. Another point is that teal_data still inherits the teal_data class, as demonstrated in the call, if user ever encounters any confusion.

Agree to Not expose or instruct users to use teal_report to create a teal_data object, and keep teal_data as entry point.