From 64e16d745b13acf22d0f9a20aa36e1ea5845941f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 26 Jan 2026 17:29:43 +0000 Subject: [PATCH 1/4] Initial plan From 71c45b4557eb4c89b3ff25f3e9e8ec0d95e88919 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 26 Jan 2026 17:32:53 +0000 Subject: [PATCH 2/4] Fix max matching votes skipping for unconfigured options Changed logic to skip limit check entirely for options not in voting_options_max_matching_votes instead of defaulting to 3. Added tests to validate the fix. Co-authored-by: IvoLeist <28399610+IvoLeist@users.noreply.github.com> --- R/mod_voting.R | 4 +- tests/testthat/test-voting-module.R | 149 +++++++++++++++++++++++++++- 2 files changed, 151 insertions(+), 2 deletions(-) diff --git a/R/mod_voting.R b/R/mod_voting.R index 0e8896b..d47dcd2 100644 --- a/R/mod_voting.R +++ b/R/mod_voting.R @@ -718,8 +718,10 @@ votingServer <- function( print(paste("vote_col:", vote_col)) max_votes <- vote_max_map[[option_key]] print(paste("max_votes:", max_votes)) + # Skip limit check if option is not in voting_options_max_matching_votes if (is.null(max_votes) || is.na(max_votes)) { - max_votes <- 3 + print(paste("Skipping limit check for option:", option_key)) + next } print(paste("max_votes after check:", max_votes)) max_votes <- as.numeric(max_votes) diff --git a/tests/testthat/test-voting-module.R b/tests/testthat/test-voting-module.R index fc55c8c..eddca29 100644 --- a/tests/testthat/test-voting-module.R +++ b/tests/testthat/test-voting-module.R @@ -817,4 +817,151 @@ testthat::test_that("get_mutation returns done when all screenshots have 3+ vote testthat::expect_equal(res$coordinates, "done") } ) -}) \ No newline at end of file +}) +testthat::test_that("options not in voting_options_max_matching_votes are never skipped", { + # This test validates the fix for the issue where options not configured + # in voting_options_max_matching_votes were being skipped with a default limit of 3 + + # Set up environment with 2 coordinates + env <- setup_voting_env(c("chr1:1000", "chr2:2000")) + args <- make_args(env$annotations_file) + cleanup_db <- setup_test_db(args) + on.exit(cleanup_db()) + + # Set vote_count_none_of_above to a high value (e.g., 10) + # Since 'none_of_above' is NOT in voting_options_max_matching_votes, + # it should NOT be skipped regardless of vote count + DBI::dbExecute( + args$db_pool, + "UPDATE annotations SET vote_count_none_of_above = 10 WHERE coordinates = 'chr1:1000'" + ) + + # Verify the vote count was set correctly + chr1_votes <- DBI::dbGetQuery( + args$db_pool, + "SELECT vote_count_none_of_above FROM annotations WHERE coordinates = 'chr1:1000'" + ) + testthat::expect_equal(chr1_votes$vote_count_none_of_above, 10) + + my_session <- MockShinySession$new() + my_session$clientData <- shiny::reactiveValues( + url_search = "" + ) + + args$cfg <- ShinyImgVoteR::load_config( + config_file_path = system.file( + "shiny-app", + "default_env", + "config", + "config.yaml", + package = "ShinyImgVoteR" + ) + ) + + # Verify that 'none_of_above' is NOT in voting_options_max_matching_votes + testthat::expect_false( + "none_of_above" %in% names(args$cfg$voting_options_max_matching_votes) + ) + + testServer( + votingServer, + session = my_session, + args = args, + { + session$userData$userAnnotationsFile <- env$annotations_file + session$userData$votingInstitute <- cfg$test_institute + session$userData$shinyauthr_session_id <- "test_no_limit_session" + + # Trigger the mutation loading + session$flushReact() + + # get_mutation should return chr1:1000, NOT skip it + # even though vote_count_none_of_above is 10 + res <- get_mutation() + testthat::expect_equal(res$coordinates, "chr1:1000") + + # Verify the annotations file does NOT have a skip reason for chr1:1000 + annotations <- read.delim( + env$annotations_file, + stringsAsFactors = FALSE + ) + chr1_row <- annotations[annotations$coordinates == "chr1:1000", ] + # The agreement should be NA (not voted yet), not a skip reason + testthat::expect_true(is.na(chr1_row$agreement)) + } + ) +}) + +testthat::test_that("options in voting_options_max_matching_votes ARE skipped when limit reached", { + # This test confirms that options explicitly configured in + # voting_options_max_matching_votes still get skipped correctly + + # Set up environment with 2 coordinates + env <- setup_voting_env(c("chr1:1000", "chr2:2000")) + args <- make_args(env$annotations_file) + cleanup_db <- setup_test_db(args) + on.exit(cleanup_db()) + + args$cfg <- ShinyImgVoteR::load_config( + config_file_path = system.file( + "shiny-app", + "default_env", + "config", + "config.yaml", + package = "ShinyImgVoteR" + ) + ) + + # Get the configured max votes for 'yes' option + max_yes_votes <- args$cfg$voting_options_max_matching_votes[["yes"]] + testthat::expect_false(is.null(max_yes_votes)) + + # Set vote_count_correct to the max limit + DBI::dbExecute( + args$db_pool, + paste0("UPDATE annotations SET vote_count_correct = ", max_yes_votes, " WHERE coordinates = 'chr1:1000'") + ) + + # Verify the vote count was set correctly + chr1_votes <- DBI::dbGetQuery( + args$db_pool, + "SELECT vote_count_correct FROM annotations WHERE coordinates = 'chr1:1000'" + ) + testthat::expect_equal(chr1_votes$vote_count_correct, max_yes_votes) + + my_session <- MockShinySession$new() + my_session$clientData <- shiny::reactiveValues( + url_search = "" + ) + + testServer( + votingServer, + session = my_session, + args = args, + { + session$userData$userAnnotationsFile <- env$annotations_file + session$userData$votingInstitute <- cfg$test_institute + session$userData$shinyauthr_session_id <- "test_skip_configured_session" + + # Trigger the mutation loading + session$flushReact() + + # get_mutation should skip chr1:1000 and return chr2:2000 + res <- get_mutation() + testthat::expect_equal(res$coordinates, "chr2:2000") + + # Verify the annotations file has a skip reason for chr1:1000 + annotations <- read.delim( + env$annotations_file, + stringsAsFactors = FALSE + ) + chr1_row <- annotations[annotations$coordinates == "chr1:1000", ] + expected_skip_reason <- paste0( + "skipped - max matching votes (", + max_yes_votes, + ") for option (yes) reached" + ) + testthat::expect_equal(chr1_row$agreement, expected_skip_reason) + } + ) +}) From 5b532fcfb4acda234039bf3daba49d7198a64361 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 26 Jan 2026 17:35:26 +0000 Subject: [PATCH 3/4] Address code review feedback - use parameterized SQL and add comment Co-authored-by: IvoLeist <28399610+IvoLeist@users.noreply.github.com> --- tests/testthat/test-voting-module.R | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/testthat/test-voting-module.R b/tests/testthat/test-voting-module.R index eddca29..9f73570 100644 --- a/tests/testthat/test-voting-module.R +++ b/tests/testthat/test-voting-module.R @@ -919,7 +919,8 @@ testthat::test_that("options in voting_options_max_matching_votes ARE skipped wh # Set vote_count_correct to the max limit DBI::dbExecute( args$db_pool, - paste0("UPDATE annotations SET vote_count_correct = ", max_yes_votes, " WHERE coordinates = 'chr1:1000'") + "UPDATE annotations SET vote_count_correct = ? WHERE coordinates = ?", + params = list(max_yes_votes, "chr1:1000") ) # Verify the vote count was set correctly @@ -956,6 +957,7 @@ testthat::test_that("options in voting_options_max_matching_votes ARE skipped wh stringsAsFactors = FALSE ) chr1_row <- annotations[annotations$coordinates == "chr1:1000", ] + # Expected format matches the skip_reason in mod_voting.R lines 755-760 expected_skip_reason <- paste0( "skipped - max matching votes (", max_yes_votes, From ddc9fc0cfc27c4c87abfbb644832d4bb84feb0e7 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 26 Jan 2026 17:36:13 +0000 Subject: [PATCH 4/4] Fix remaining SQL queries to use parameterized queries Co-authored-by: IvoLeist <28399610+IvoLeist@users.noreply.github.com> --- tests/testthat/test-voting-module.R | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/testthat/test-voting-module.R b/tests/testthat/test-voting-module.R index 9f73570..601c055 100644 --- a/tests/testthat/test-voting-module.R +++ b/tests/testthat/test-voting-module.R @@ -833,13 +833,15 @@ testthat::test_that("options not in voting_options_max_matching_votes are never # it should NOT be skipped regardless of vote count DBI::dbExecute( args$db_pool, - "UPDATE annotations SET vote_count_none_of_above = 10 WHERE coordinates = 'chr1:1000'" + "UPDATE annotations SET vote_count_none_of_above = ? WHERE coordinates = ?", + params = list(10, "chr1:1000") ) # Verify the vote count was set correctly chr1_votes <- DBI::dbGetQuery( args$db_pool, - "SELECT vote_count_none_of_above FROM annotations WHERE coordinates = 'chr1:1000'" + "SELECT vote_count_none_of_above FROM annotations WHERE coordinates = ?", + params = list("chr1:1000") ) testthat::expect_equal(chr1_votes$vote_count_none_of_above, 10) @@ -926,7 +928,8 @@ testthat::test_that("options in voting_options_max_matching_votes ARE skipped wh # Verify the vote count was set correctly chr1_votes <- DBI::dbGetQuery( args$db_pool, - "SELECT vote_count_correct FROM annotations WHERE coordinates = 'chr1:1000'" + "SELECT vote_count_correct FROM annotations WHERE coordinates = ?", + params = list("chr1:1000") ) testthat::expect_equal(chr1_votes$vote_count_correct, max_yes_votes)