910 Remove Report Previewer column #919

chlebowa · 2023-09-15T08:52:22Z

Fixes #910

The column named "Report previewer" is removed from the mapping matrix display.
Since the data.frame sent to renderTable potentially has one less column, the align argument is modified accordingly.

Additionally, "Report previewer" has been reserved as a module label during module creation, much like "global_filters".
Since the actual previewer is supposed to have that label, it is set after the module is created.

This will only work as long as the name "Report previewer" does not change.

github-actions · 2023-09-15T08:57:50Z

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                            77      28  63.64%   171-178, 183-204, 216-218
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        148     105  29.05%   71-82, 109-118, 122-134, 136-143, 149-164, 177-200, 203-214, 217-223, 237, 258-281
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.R                     57      12  78.95%   118-131
R/utils.R                           21       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                             1389     382  72.50%

Diff against main

Filename                         Stmts    Miss  Cover
-----------------------------  -------  ------  -------
R/modules.R                        +10      +5  -2.78%
R/reporter_previewer_module.R       +1       0  +0.74%
TOTAL                              +11      +5  -0.14%

Results for commit: fe4f9c5

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

github-actions · 2023-09-15T08:58:38Z

Unit Tests Summary

    1 files   16 suites 16s ⏱️
171 tests 171 ✔️ 0 💤 0 ❌
341 runs 341 ✔️ 0 💤 0 ❌

Results for commit 053405c.

♻️ This comment has been updated with latest results.

averissimo

LGTM! 💯

Tested on {teal.gallery} longitudinal app with module_specific = TRUE param.

It's unfortunate that we need to rely on a hardcoded string, but it's unlikely it will change.

Food for thought before merging:

Do you think it's worth checking if it's also the last element
- that is: avoid removing modules if a user manually names one "Report previewer"

chlebowa · 2023-09-15T12:07:15Z

Tricky question. If an app dev decides to name a module "Report previewer", the actual previewer will be the last one, so a check like that would remove the previewer but not the module. However, if that happens in an app that doesn't use the reporter and that module is placed at the end, it will still be removed.

I guess the question is how likely is it that an app dev names a module "Report previewer"? Also, didn't that use to be forbidden?

averissimo · 2023-09-15T12:29:42Z

Tricky indeed. Then it would have to check if there is there is any modulePreviewer beforehand.

That looks like a dark and long rabbit hole.

As far as I can see, it's not easy to detect if the reporter_previewer_module was appened from the filter_manager_modal_srv context (without passing that information along)

Also, didn't that use to be forbidden?

I tried to name it and it seemed to work (and hide on filter manager)

averissimo · 2023-09-15T12:35:07Z

However, this seems so farfetched, that it may be better to accept this scenario or:

prevent that name from being used as module title
output a warning when it's used

chlebowa · 2023-09-15T12:45:12Z

Also, didn't that use to be forbidden?

I tried to name it and it seemed to work (and hide on filter manager)

It does but I have a feeling it shouldn't and it didn't use to.

@donyunardi @lcd2yyz do you think we can forbid that?

donyunardi · 2023-09-15T17:30:00Z

This line will always force "Report previewer" to be the last module, after all other modules.

teal/R/module_teal.R

Lines 238 to 240 in 139971a

    
           if (is_arg_used(modules, "reporter") && !is_any_previewer(modules)) { 
        
             modules <- append_module(modules, reporter_previewer_module()) 
        
           }

An alternative way is to combine the ideas that you both presented: check if the last element of the filtered_data_list is equal to Report previewer.

reporter_previewer_module is exported, and may be an issue if the label argument for this module is updated to anything other than Report previewer if a user chooses to customize the teal framework.

teal/R/reporter_previewer_module.R

Lines 15 to 17 in 139971a

    
           #' @return `teal_module` containing the `teal.reporter` previewer functionality 
        
           #' @export 
        
           reporter_previewer_module <- function(label = "Report previewer", server_args = list()) {

At the moment, I'd say it's very unlikely.

@chlebowa WDYT?

It does but I have a feeling it shouldn't and it didn't use to.

I am not aware of any restrictions on module names. I'd suggest we pursue this option if we see it's happening to avoid scope creep of this PR.

donyunardi

Maybe a record in NEWS?

chlebowa · 2023-09-18T06:48:06Z

This line will always force "Report previewer" to be the last module, after all other modules.

Not always because one can run an app with modules that do not use reporters and name the first module "Report previewer", however unlikely that may be.

reporter_previewer_module is exported, and may be an issue if the label argument for this module is updated to anything other than Report previewer if a user chooses to customize the teal framework.

Then again, the module is not meant to be called explicitly and is added automatically by srv_teal:

    if (is_arg_used(modules, "reporter") && !is_any_previewer(modules)) {
      modules <- append_module(modules, reporter_previewer_module())
    }

donyunardi · 2023-09-18T20:07:44Z

R/module_filter_manager.R

+        # Report Previewer will not be displayed.
+        mm[!grepl("Report previewer", names(mm))]


Not always because one can run an app with modules that do not use reporters and name the first module "Report previewer", however unlikely that may be.

I am aware of that. To make it clear, I was thinking something like this:

Suggested change

# Report Previewer will not be displayed.

mm[!grepl("Report previewer", names(mm))]

# Report Previewer will not be displayed.

if ("Report previewer" %in% names(mm)[length(mm)]) {

mm <- mm[, -length(mm)]

}

mm

This way, even though we have a user module with "Report previewer" as the label name in the first module, it will not be removed. We only check the last element of mm.

WDYT?

donyunardi

Made suggestion on optimizing some code.
Everything else looks good.

Thanks @chlebowa !

donyunardi · 2023-09-19T19:21:21Z

R/modules.R

+    stop(
+      sprintf("module(label = \"%s\", ...\n  ", label),
+      "Label 'global_filters' is reserved in teal. Please change to something else.",
+      call. = FALSE
+    )
+  }
+  if (label == "Report previewer") {
+    stop(
+      sprintf("module(label = \"%s\", ...\n  ", label),
+      "Label 'Report previewer' is reserved in teal.",
+      call. = FALSE
+    )


Suggested change

stop(

sprintf("module(label = \"%s\", ...\n ", label),

"Label 'global_filters' is reserved in teal. Please change to something else.",

call. = FALSE

)

}

if (label == "Report previewer") {

stop(

sprintf("module(label = \"%s\", ...\n ", label),

"Label 'Report previewer' is reserved in teal.",

call. = FALSE

)

if (label %in% c("global_filters", "Report previewer")) {

stop(

sprintf(

"module(label = %1$s, ...

Label %1$s is reserved in teal. Please change to something else.

",

label

),

call. = FALSE

)

}

Perhaps we can combine the code for optimization?

The line break within the fmt string adds loads of white space to the message, you should do

sprintf( "module(label = \"%s\", ...\n %s", label, "Label 'global_filters' is reserved in teal. Please change to something else." )

The benchmark is exactly the same, down to a microsecond.

Unit: microseconds expr min lq mean median uq max neval cld new 36.793 41.2085 51.34256 44.1210 52.019 275.206 500 a current 36.873 41.2010 51.51580 44.2335 50.337 289.881 500 a

The current is 3 lines less.

not needed

drop Report previewer from mapping matrix

bad48f6

chlebowa added the core label Sep 15, 2023

chlebowa requested a review from donyunardi September 15, 2023 08:52

linter

2922610

averissimo self-assigned this Sep 15, 2023

averissimo approved these changes Sep 15, 2023

View reviewed changes

donyunardi requested changes Sep 15, 2023

View reviewed changes

amend

8f66b89

chlebowa requested a review from donyunardi September 18, 2023 17:38

donyunardi reviewed Sep 18, 2023

View reviewed changes

Aleksander Chlebowski added 3 commits September 19, 2023 16:24

reserve module label 'Report previewer'

18e5147

improve logical index in mapping matrix

19bf60c

circumvent reserved label in reporter_previewer_module

053405c

chlebowa requested a review from donyunardi September 19, 2023 14:33

averissimo removed their assignment Sep 19, 2023

donyunardi previously requested changes Sep 19, 2023

View reviewed changes

chlebowa merged commit be307cf into main Sep 20, 2023

chlebowa deleted the 910_report_previewer_out_of_mapping_matrix@main branch September 20, 2023 09:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

910 Remove Report Previewer column #919

910 Remove Report Previewer column #919

chlebowa commented Sep 15, 2023 •

edited

Loading

github-actions bot commented Sep 15, 2023 •

edited

Loading

github-actions bot commented Sep 15, 2023 •

edited

Loading

averissimo left a comment

chlebowa commented Sep 15, 2023

averissimo commented Sep 15, 2023

averissimo commented Sep 15, 2023

chlebowa commented Sep 15, 2023

donyunardi commented Sep 15, 2023

donyunardi left a comment

chlebowa commented Sep 18, 2023

donyunardi Sep 18, 2023 •

edited

Loading

donyunardi left a comment

donyunardi Sep 19, 2023

chlebowa Sep 20, 2023

		# Report Previewer will not be displayed.
		mm[!grepl("Report previewer", names(mm))]

910 Remove Report Previewer column #919

910 Remove Report Previewer column #919

Conversation

chlebowa commented Sep 15, 2023 • edited Loading

github-actions bot commented Sep 15, 2023 • edited Loading

Code Coverage Summary

Diff against main

github-actions bot commented Sep 15, 2023 • edited Loading

Unit Tests Summary

averissimo left a comment

Choose a reason for hiding this comment

chlebowa commented Sep 15, 2023

averissimo commented Sep 15, 2023

averissimo commented Sep 15, 2023

chlebowa commented Sep 15, 2023

donyunardi commented Sep 15, 2023

donyunardi left a comment

Choose a reason for hiding this comment

chlebowa commented Sep 18, 2023

donyunardi Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

donyunardi left a comment

Choose a reason for hiding this comment

donyunardi Sep 19, 2023

Choose a reason for hiding this comment

chlebowa Sep 20, 2023

Choose a reason for hiding this comment

chlebowa commented Sep 15, 2023 •

edited

Loading

github-actions bot commented Sep 15, 2023 •

edited

Loading

github-actions bot commented Sep 15, 2023 •

edited

Loading

donyunardi Sep 18, 2023 •

edited

Loading