Feature download csv #13

yashshah1995 · 2025-01-20T15:53:52Z

Implement download button for events table
Percentage and number should be in separate columns
A disclaimer should be presented before the download letting the user know it is a unvalidated output (follow dv.listings strategy)

[release] rc/v0.0.2

iglauss

I did not review the entire PR code-wise, but tested the mock app. A few things which I noticed:

The old checkbox label should be removed. You can do it as in dv.listings where only "I agree to the following:" is printed and the mod function contains an argument intended_use_label = "Use only for internal review and monitoring during the conduct of clinical trials.", which will be pasted to the first string.
Only the first level gets downloaded. Could we change this to download the entire table?
I'm not sure if commas as separators are a good choice, at least in the mock app, the download gets a bit messy, because AEBODYSYS contains commas as well.
There are no column headers in the exported file.

- csv to excel download

yashshah1995 · 2025-01-29T16:43:16Z

@iglauss thanks for the review. The requested features are implemented.

For 3., it now downloads as excel

For 4., Because we separate columns into two (N and per), Adding Overall Patients to column header was messy. So they are added as a first row in the dataframe.

zsigmas · 2025-02-04T10:29:32Z

R/mod_export_count_table.R

+      download_enable <- shiny::eventReactive(
+        c(input[[EXP$ID$FILENAME_BOX]], input[[EXP$ID$DATAPROTECT_BOX]]),
+        {
+          if (
+            (input[[EXP$ID$FILENAME_BOX]] != "") &
+              input[[EXP$ID$DATAPROTECT_BOX]]
+          ) { # nolint end
+            return(TRUE)
+          } else {
+            return(FALSE)
+          }
+        }
+      )
+
+      # Enable/disable download button separately (to allow testing of logic)
+      shiny::observeEvent(download_enable(), {
+        if (download_enable()) {
+          shinyjs::enable(EXP$ID$DOWNLOAD_BUTTON)
+        } else {
+          shinyjs::disable(EXP$ID$DOWNLOAD_BUTTON)
+        }
+      })


Can we compact these lines in a single observe? download_enable is not used anywhere else as far as I can see

Combined in one Observe

zsigmas · 2025-02-04T10:30:19Z

R/mod_export_count_table.R

+
+        # If there is no dataset being displayed
+        if (is.null(dataset)) {
+          shinyFeedback::showToast(


Do we need shinyFeedback in this case? I would rather use shiny::validate to avoid an extra dependency. Or the native showNotification in shiny as we are inside an obvserve.

Replaced with shiny::validate

zsigmas · 2025-02-04T10:36:04Z

R/mod_export_count_table.R

+          # Get col names and total patients
+          total_colname <- dataset[["meta"]]$n_denominator
+
+          #Get event variables
+          event_vars <- dataset[["meta"]]$hierarchy
+
+          excelfile <- dataset[["df"]] |>
+            dplyr::select(
+              event_vars,
+              names(total_colname)
+            ) |>
+            dplyr::mutate(dplyr::across(
+              dplyr::where(is.list),
+              ~ purrr::map_chr(., "count")
+            )) |>
+            dplyr::mutate(dplyr::across(
+              dplyr::all_of(names(total_colname)),
+              ~ gsub("\u2014", NA, .)
+            )) |>
+            dplyr::mutate(dplyr::across(
+              dplyr::all_of(event_vars),
+              ~ gsub("\035", "Total", .)
+            ))
+
+          excelfile <- add_total_patient(
+            excelfile,
+            total_colname
+          )


I would extract this in a separate function, and test is in a non reactive environment.

Maybe with the following structure

count_table <- call_table_computing_function(xxx) call_data_extraction(count_table)

This way if we change the way we count in the table we break the test and remember the coupling between both of them

zsigmas · 2025-02-04T10:37:43Z

R/utils-export_table.R

+#' Function to separate a column
+#'
+#' @details
+#' Columns where `(` is encountered is split into two and new columns
+#' are mutated
+#'
+#' @param df dataframe
+#' @param col column name
+#'
+#' @return dataframe with new mutated separated columns
+#' @keywords internal
+separate_column <- function(df, col) {
+  df |>
+    tidyr::separate(col, into = paste0(col, c("_N", "_per")),
+                    sep = " \\(",
+                    remove = TRUE,
+                    fill = "right") %>%
+    dplyr::mutate(dplyr::across(dplyr::ends_with("_per"), ~stringr::str_remove(., "\\)")))
+}
+
+
+#' Adds Overall number of patients
+#'
+#' @details
+#' Extracts overall number of patient (html table output column headers)
+#' from metadata, adds it to first row of the downloaded table.
+#'
+#'
+#' @param df dataframe
+#' @param total_colname named int, col names with overall patients
+#'
+#' @return dataframe with additional row containing overall patients
+#'
+#' @keywords internal
+add_total_patient <- function(df, total_colname) {
+
+  # empty row with column name placeholder
+  new_row <- setNames(data.frame(matrix(ncol = ncol(df), nrow = 1)), names(df))
+
+  new_row[[1, 1]] <- "Overall No. of Patients"
+
+  # match col names to add total patients
+  matched_cols <- match(names(total_colname), names(new_row))
+  new_row[1, matched_cols] <- total_colname
+
+  return(rbind(new_row, df))
+
+
+}


I would compact this in a single function together with the code that is now inside the download handler.

Put different export utils (and the dplyr operation from mod into main function).

iglauss

Thanks, Yash! I'm almost completely happy. Just one remaining issue: When I add a minimum % >0, this is not reflected in the PDF, right? Could we change this?

iglauss · 2025-02-04T14:25:59Z

DESCRIPTION

 Config/testthat/edition: 3
 Config/testthat/parallel: false
 Imports: shiny (>= 1.7.1),dplyr (>= 1.0.7), purrr (>= 0.3.4),
        tidyr (>= 1.1.4),
        rlang, checkmate (>= 2.0.0), htmltools, 
-        stats, pharmaverseadam
+        stats, pharmaverseadam, writexl


Just to make sure that we do not add unnecessary independencies: Could we use openxlsx instead of writexl? Becuase dv.listings is already using openxlsx. If not, this is also fine. Just want to question it.

Thanks for pointing out previous comment, Mistake in passing non-reactive object. Fixed!
Replaced with openxlsx

- checks

iglauss · 2025-02-13T09:52:56Z

R/mod_hierarchical_count_table.R

@@ -473,7 +475,8 @@ hierarchical_count_table_server <- function(
    subjid_var,
    show_modal_on_click = FALSE,
    default_hierarchy = NULL,
-    default_group = NULL) {
+    default_group = NULL,
+    intended_use_label = NULL) {


We still need to add the intended_use_label in the roxygen docu above

ml-ebs-ext

Here's a bit of feedback. Thanks for the good work!

ml-ebs-ext · 2025-02-14T09:42:38Z

R/utils-export_table.R

+        remove = TRUE,
+        fill = "right"
+      ) %>%
+      dplyr::mutate(dplyr::across(dplyr::ends_with("_per"), ~ stringr::str_remove(., "\\)")))


Since this is the only use of stringr in the whole package, we could rely instead on base::sub and cut a dependency:

Suggested change

dplyr::mutate(dplyr::across(dplyr::ends_with("_per"), ~ stringr::str_remove(., "\\)")))

dplyr::mutate(dplyr::across(dplyr::ends_with("_per"), ~ sub(")", "", . )))

ml-ebs-ext · 2025-02-14T09:52:38Z

DESCRIPTION

@@ -13,13 +13,14 @@ LazyData: true
 Roxygen: list(markdown = TRUE)
 RoxygenNote: 7.3.1
 Suggests: dv.manager (>= 2.1.4), jsonlite, rmarkdown, testthat (>=
-        3.0.0), shinytest2, devtools, knitr, tibble, utils
+        3.0.0), shinytest2, devtools, knitr, tibble, utils,
+        shinyjs, stringr


There is a solitary call to stringr::str_remove which I've suggested to replace with base::sub. If you think that's OK, then we could drop stringr.

I believe both stringr (if we were to keep it) and shinyjs should be under Imports instead of under Suggests. Reasoning.

ml-ebs-ext · 2025-02-14T10:02:33Z

R/mod_export_count_table.R

+        # check validity of parameters
+
+
+        checkmate::assert(


When an assertion triggers inside an observer, it kills the app.
We could instead either:

shiny::req() the checks, which would prevent the crash but fail silently. Probably would leave users scratching their heads.

Use a shiny::showNotification similar like the one you have below describing the problem to the user.

Hi @ml-ebs-ext thanks for the comment. Just to make a case for the checkmate assertion into the observer; Wouldn't it be better to let the app gracefully crash, as the intended person for this check would be app creator rather than app user? Since this way they would know something is wrong in the dataset before publishing.

Letting the app creator know something is wrong is perfectly OK, but

gracefully crash

sounds like an oxymoron to me 😋.

Also, in this case app creators need to press the export button in order to trigger the error. So, if they forget to do it, it's the final user that gets the crash and no message.

Still, I see there's only one caller of the export_counttable module and it already performs some checks which happen without user interaction, so I'll leave it to you to judge the best course of action here.

Good point, I replaced it with simple if and showNotifications.

ml-ebs-ext · 2025-02-14T10:27:03Z

R/mod_export_count_table.R

+        # Check if the inputs exist
+        if (!is.null(input[[EXP$ID$FILENAME_BOX]]) && !is.null(input[[EXP$ID$DATAPROTECT_BOX]])) {
+          # Check if the conditions are met
+          if (input[[EXP$ID$FILENAME_BOX]] != "" && input[[EXP$ID$DATAPROTECT_BOX]]) {
+            shinyjs::enable(EXP$ID$DOWNLOAD_BUTTON)
+          } else {
+            shinyjs::disable(EXP$ID$DOWNLOAD_BUTTON)
+          }
+        }


This is mostly a stylistic suggestion. The only thing of value is the rejection of invalid destination file names. The rest, you can ignore:

Suggested change

# Check if the inputs exist

if (!is.null(input[[EXP$ID$FILENAME_BOX]]) && !is.null(input[[EXP$ID$DATAPROTECT_BOX]])) {

# Check if the conditions are met

if (input[[EXP$ID$FILENAME_BOX]] != "" && input[[EXP$ID$DATAPROTECT_BOX]]) {

shinyjs::enable(EXP$ID$DOWNLOAD_BUTTON)

} else {

shinyjs::disable(EXP$ID$DOWNLOAD_BUTTON)

}

}

sanitized_fname <- fs::path_sanitize(input[[EXP$ID$FILENAME_BOX]])

enable_button <- (nchar(sanitized_fname) > 0) && isTRUE(input[[EXP$ID$DATAPROTECT_BOX]])

shinyjs::toggleState(id = EXP$ID$DOWNLOAD_BUTTON, condition = enable_button)

ml-ebs-ext · 2025-02-14T10:29:34Z

R/mod_export_count_table.R

+      # Download
+      output[[EXP$ID$DOWNLOAD_BUTTON]] <- shiny::downloadHandler(
+        filename = function() {
+          paste0(input[[EXP$ID$FILENAME_BOX]], ".xlsx")


Avoids illegal destination file names:

Suggested change

paste0(input[[EXP$ID$FILENAME_BOX]], ".xlsx")

sanitized_fname <- fs::path_sanitize(input[[EXP$ID$FILENAME_BOX]])

paste0(sanitized_fname, ".xlsx")

…rate checks.

…internal task 242403.

Tweak docs, de-exported symbols, update API-spec-related module wrappers

- prevent app crash

zsigmas and others added 5 commits December 3, 2024 10:40

Merge pull request #11 from Boehringer-Ingelheim/rc/v0.0.2

45a9a75

[release] rc/v0.0.2

table download button

fdb8032

- download table condition check

946c780

- checks, documentation

1379b57

- devtools check

68df513

yashshah1995 requested review from zsigmas and iglauss January 20, 2025 15:53

yashshah1995 self-assigned this Jan 20, 2025

yashshah1995 requested a review from a team as a code owner January 20, 2025 15:53

yashshah1995 removed the request for review from a team January 20, 2025 15:58

iglauss reviewed Jan 20, 2025

View reviewed changes

yashshah1995 added 7 commits January 28, 2025 16:15

- change intended use message

166e95f

- Download entire table

0a3fea7

- csv to excel download

- intended label

ecb93be

- intended label fix

53d95e3

- remove print debugging

d5cebd6

- Additional row, overall patients

4b6dffe

- lintr

9dd4cdf

zsigmas reviewed Feb 4, 2025

View reviewed changes

iglauss requested changes Feb 4, 2025

View reviewed changes

yashshah1995 added 7 commits February 7, 2025 15:22

- combine to single observe

1c594ce

- remove shinyFeedback dependency

eeae89b

- combine preprocess functions

11a5ab8

- replace writexl with openxlsx

2332137

- pass reactive expression

4190556

- unit tests

7fea4d1

- checks

- lintr, checkmate fix

386299b

iglauss reviewed Feb 13, 2025

View reviewed changes

ml-ebs-ext reviewed Feb 14, 2025

View reviewed changes

ml-ebs-ext and others added 10 commits February 14, 2025 11:41

Update CM, DR, add intended_use_label to module API spec and regene…

af7ae46

…rate checks.

Restyle documentation as described in internal task 252145.

55cd269

Avoid "object is masked" pack_of_constants messages as required by …

fe44024

…internal task 242403.

De-export label setters and getters as required by internal task 242403.

21822df

Merge pull request #14 from Boehringer-Ingelheim/misc_tweaks

5532fca

Tweak docs, de-exported symbols, update API-spec-related module wrappers

- remove stringr opr., replace with base

e7f0259

- shinyjs under imports

b08489d

- refactor (miguel's comments)

84d2fb8

devtools::check

b4b4b85

-lintr

b7eeb15

yashshah1995 linked an issue Feb 18, 2025 that may be closed by this pull request

[feature] As a user I want to download the AE hierarchy table as a CSV, or other editable file #12

Closed

yashshah1995 added the enhancement New feature or request label Feb 18, 2025

yashshah1995 requested a review from iglauss February 18, 2025 07:34

- replace checkmate checks with notification

e69a698

- prevent app crash

yashshah1995 merged commit c1cee1d into dev Feb 18, 2025
5 checks passed

yashshah1995 deleted the feature_download_csv branch February 18, 2025 13:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature download csv #13

Feature download csv #13

yashshah1995 commented Jan 20, 2025 •

edited

Loading

iglauss left a comment

yashshah1995 commented Jan 29, 2025

zsigmas Feb 4, 2025

yashshah1995 Feb 12, 2025

zsigmas Feb 4, 2025

yashshah1995 Feb 12, 2025

zsigmas Feb 4, 2025

zsigmas Feb 4, 2025

yashshah1995 Feb 12, 2025

iglauss left a comment

iglauss Feb 4, 2025

yashshah1995 Feb 12, 2025

iglauss Feb 13, 2025

ml-ebs-ext left a comment

ml-ebs-ext Feb 14, 2025

ml-ebs-ext Feb 14, 2025

ml-ebs-ext Feb 14, 2025

yashshah1995 Feb 17, 2025 •

edited

Loading

ml-ebs-ext Feb 17, 2025

yashshah1995 Feb 18, 2025

ml-ebs-ext Feb 14, 2025

ml-ebs-ext Feb 14, 2025

	dplyr::mutate(dplyr::across(dplyr::ends_with("_per"), ~ stringr::str_remove(., "\\)")))
	dplyr::mutate(dplyr::across(dplyr::ends_with("_per"), ~ sub(")", "", . )))

	paste0(input[[EXP$ID$FILENAME_BOX]], ".xlsx")
	sanitized_fname <- fs::path_sanitize(input[[EXP$ID$FILENAME_BOX]])
	paste0(sanitized_fname, ".xlsx")

Feature download csv #13

Feature download csv #13

Conversation

yashshah1995 commented Jan 20, 2025 • edited Loading

iglauss left a comment

Choose a reason for hiding this comment

yashshah1995 commented Jan 29, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iglauss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ml-ebs-ext left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yashshah1995 Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yashshah1995 commented Jan 20, 2025 •

edited

Loading

yashshah1995 Feb 17, 2025 •

edited

Loading