From 252bcb1944b95911758d9d19e3d45eb7695131cf Mon Sep 17 00:00:00 2001 From: Jamie Whitehouse Date: Tue, 29 Jul 2025 12:16:23 +0200 Subject: [PATCH 1/4] update sort model (and docs) to use 'formula' --- R/records.R | 6 +++--- R/tableQuery.R | 4 ++-- man/addSort.Rd | 2 +- tests/testthat/test-records.R | 12 ++++++------ 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/R/records.R b/R/records.R index 881b97f..8009b2a 100644 --- a/R/records.R +++ b/R/records.R @@ -1221,7 +1221,7 @@ addFilter <- function(x, formulaFilter) { #' Usually one would use the [dplyr::arrange] verb instead. #' #' @param x the remote records object fetched with [activityinfo::getRecords]. -#' @param sort the sort object in the following format: list(list(dir = "ASC", field = "\[Name\]")) or list(list(dir = "DESC", field = "\[Name\]")) +#' @param sort the sort object in the following format: list(list(dir = "ASC", formula = "\[Name\]")) or list(list(dir = "DESC", formula = "\[Name\]")) #' #' @export addSort <- function(x, sort) { @@ -1713,14 +1713,14 @@ arrange.tbl_activityInfoRemoteRecords <- function(.data, ...) { if (is.symbol(expr2)) { chexpr <- rlang::as_name(expr2) if (chexpr %in% tblNames(.data)) { - return(addSort(x = .data, sort = list(list(dir = "ASC", field = chexpr)))) + return(addSort(x = .data, sort = list(list(dir = "ASC", formula = chexpr)))) } } else if (is.call(expr2)) { fn <- as.character(expr2[[1]]) if (fn == "desc") { chexpr <- as.character(expr2[[2]]) if (chexpr %in% tblNames(.data)) { - return(addSort(x = .data, sort = list(list(dir = "DESC", field = chexpr)))) + return(addSort(x = .data, sort = list(list(dir = "DESC", formula = chexpr)))) } } } diff --git a/R/tableQuery.R b/R/tableQuery.R index b37a550..8033853 100644 --- a/R/tableQuery.R +++ b/R/tableQuery.R @@ -214,9 +214,9 @@ checkSortList <- function(sort) { force(sort) stopifnot(is.list(sort)) invisible(lapply(sort, function(x) { - stopifnot(all(names(x) %in% c("dir", "field"))) + stopifnot(all(names(x) %in% c("dir", "formula"))) stopifnot(x[["dir"]]%in%c("ASC", "DESC")) - stopifnot(is.character(x[["field"]])) + stopifnot(is.character(x[["formula"]])) stopifnot(is.character(x[["dir"]])) })) } diff --git a/man/addSort.Rd b/man/addSort.Rd index f71ed02..d741385 100644 --- a/man/addSort.Rd +++ b/man/addSort.Rd @@ -9,7 +9,7 @@ addSort(x, sort) \arguments{ \item{x}{the remote records object fetched with \link{getRecords}.} -\item{sort}{the sort object in the following format: list(list(dir = "ASC", field = "[Name]")) or list(list(dir = "DESC", field = "[Name]"))} +\item{sort}{the sort object in the following format: list(list(dir = "ASC", formula = "[Name]")) or list(list(dir = "DESC", formula = "[Name]"))} } \description{ This adds a single sorted column to an ActivityInfo remote records object. diff --git a/tests/testthat/test-records.R b/tests/testthat/test-records.R index a54052f..f18bb62 100644 --- a/tests/testthat/test-records.R +++ b/tests/testthat/test-records.R @@ -1,4 +1,4 @@ - + testthat::test_that("add, update, and deleteRecord() works", { @@ -271,11 +271,11 @@ testthat::test_that("getRecords() works", { dfA <- rcrds %>% addFilter('[A logical column] == "True"') %>% - addSort(list(list(dir = "ASC", field = "_id"))) %>% + addSort(list(list(dir = "ASC", formula = "_id"))) %>% adjustWindow(offSet = 0L, limit = 10L) %>% collect() dfB <- rcrds %>% filter(`A logical column` == "True") %>% - addSort(list(list(dir = "ASC", field = "_id"))) %>% + addSort(list(list(dir = "ASC", formula = "_id"))) %>% slice_head(n = 10) %>% collect() attr(dfA, "remoteRecords") <- NULL @@ -310,7 +310,7 @@ testthat::test_that("getRecords() works", { testthat::expect_warning({ recordIds <- rcrds %>% addFilter('[A logical column] == "True"') %>% - addSort(list(list(dir = "DESC", field = "A date column"))) %>% + addSort(list(list(dir = "DESC", formula = "A date column"))) %>% head(n = 10) %>% select(`_id`) %>% collect() %>% @@ -333,7 +333,7 @@ testthat::test_that("getRecords() works", { rcrds %>% select(id = `_id`, date = `A date column`, logical = `A logical column`) %>% filter(logical == "True") %>% rename(date2 = date) %>% - addSort(list(list(dir = "DESC", field = "date"))) %>% + addSort(list(list(dir = "DESC", formula = "date"))) %>% head(10) %>% collect() }) @@ -370,7 +370,7 @@ testthat::test_that("getRecords() works", { recordIds <- rcrds %>% select(id = `_id`, date = `A date column`, logical = `A logical column`) %>% filter(logical == "True") %>% - addSort(list(list(dir = "DESC", field = "date"))) %>% + addSort(list(list(dir = "DESC", formula = "date"))) %>% head(10) %>% collect() %>% pull("id") From 3cb96bca11fdfbe78b05e44f9e8a81eb589cf546 Mon Sep 17 00:00:00 2001 From: Jamie Whitehouse Date: Tue, 29 Jul 2025 12:26:36 +0200 Subject: [PATCH 2/4] remove incorrect test asertions: filter/sort models can be applied on query via a formula which does not reference column ids --- tests/testthat/test-records.R | 26 -------------------------- 1 file changed, 26 deletions(-) diff --git a/tests/testthat/test-records.R b/tests/testthat/test-records.R index f18bb62..985ff42 100644 --- a/tests/testthat/test-records.R +++ b/tests/testthat/test-records.R @@ -304,20 +304,6 @@ testthat::test_that("getRecords() works", { }) }) - # removing columns required for a filter will result in an error - # expect warning using select after filter or sort - testthat::expect_error({ - testthat::expect_warning({ - recordIds <- rcrds %>% - addFilter('[A logical column] == "True"') %>% - addSort(list(list(dir = "DESC", formula = "A date column"))) %>% - head(n = 10) %>% - select(`_id`) %>% - collect() %>% - pull("_id") - }) - }) - # filters will work even if the column is renamed in a lazy remote records object testthat::expect_no_warning({ rcrds %>% @@ -327,18 +313,6 @@ testthat::test_that("getRecords() works", { collect() }) - # renaming columns required for a sort will result in an error - testthat::expect_error({ - testthat::expect_warning({ - rcrds %>% - select(id = `_id`, date = `A date column`, logical = `A logical column`) %>% - filter(logical == "True") %>% rename(date2 = date) %>% - addSort(list(list(dir = "DESC", formula = "date"))) %>% - head(10) %>% - collect() - }) - }) - testthat::test_that("Copying of schemas with extractSchemaFromFields()", { newSchema <- rcrds %>% select(id = `Identifier number`) %>% extractSchemaFromFields(databaseId = "dbid", label = "new form") newSchema2 <-rcrds %>% select(-`_id`, -`_lastEditTime`) %>% extractSchemaFromFields(databaseId = "dbid", label = "new form") From 8f9a038e5a3599e07dff6df7fc35b06cdfbb76e6 Mon Sep 17 00:00:00 2001 From: Jamie Whitehouse Date: Tue, 29 Jul 2025 14:58:46 +0200 Subject: [PATCH 3/4] fix sort model on getRecords::getTotalLastEditTime --- R/records.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/R/records.R b/R/records.R index 8009b2a..37e1796 100644 --- a/R/records.R +++ b/R/records.R @@ -1165,12 +1165,12 @@ tbl.src_activityInfo <- function(src, formTree, style = defaultColumnStyle(),... } getTotalLastEditTime <- function(formTree) { - df <- queryTable(formTree$root, columns = list("id"="_id", "lastEditTime" = "_lastEditTime"), asTibble = TRUE, makeNames = FALSE, window = c(0L,1L), sort=list(list(dir = "DESC", field = "_lastEditTime"))) + df <- queryTable(formTree$root, columns = list("id"="_id", "lastEditTime" = "_lastEditTime"), asTibble = TRUE, makeNames = FALSE, window = c(0L,1L), sort=list(list(dir = "DESC", formula = "_lastEditTime"))) totalRecords <- attr(df, "totalRows") if (totalRecords==0) { # required to check the formTree as queryTable used in totalRecords does not error if there are no permissions but returns 0 rows formTree <- getFormTree(formTree$root) - df <- queryTable(formTree$root, columns = list("id"="_id", "lastEditTime" = "_lastEditTime"), asTibble = TRUE, makeNames = FALSE, window = c(0L,1L), sort=list(list(dir = "DESC", field = "_lastEditTime"))) + df <- queryTable(formTree$root, columns = list("id"="_id", "lastEditTime" = "_lastEditTime"), asTibble = TRUE, makeNames = FALSE, window = c(0L,1L), sort=list(list(dir = "DESC", formula = "_lastEditTime"))) totalRecords <- attr(df, "totalRows") } lastEditTime <- df[[1,"lastEditTime"]] From c8f125608e2f98fc2b63f0167118653f37d711ec Mon Sep 17 00:00:00 2001 From: Jamie Whitehouse Date: Tue, 29 Jul 2025 17:34:33 +0200 Subject: [PATCH 4/4] Update sort formulas to use actual field names. Couple instances of '_id' being used referring to a renamed column of normal form field '[Identifier number]', not the inbuilt '_id' field --- tests/testthat/test-records.R | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/testthat/test-records.R b/tests/testthat/test-records.R index 985ff42..2abe33d 100644 --- a/tests/testthat/test-records.R +++ b/tests/testthat/test-records.R @@ -271,11 +271,11 @@ testthat::test_that("getRecords() works", { dfA <- rcrds %>% addFilter('[A logical column] == "True"') %>% - addSort(list(list(dir = "ASC", formula = "_id"))) %>% + addSort(list(list(dir = "ASC", formula = "[Identifier number]"))) %>% adjustWindow(offSet = 0L, limit = 10L) %>% collect() dfB <- rcrds %>% filter(`A logical column` == "True") %>% - addSort(list(list(dir = "ASC", formula = "_id"))) %>% + addSort(list(list(dir = "ASC", formula = "[Identifier number]"))) %>% slice_head(n = 10) %>% collect() attr(dfA, "remoteRecords") <- NULL @@ -344,7 +344,7 @@ testthat::test_that("getRecords() works", { recordIds <- rcrds %>% select(id = `_id`, date = `A date column`, logical = `A logical column`) %>% filter(logical == "True") %>% - addSort(list(list(dir = "DESC", formula = "date"))) %>% + addSort(list(list(dir = "DESC", formula = "[A date column]"))) %>% head(10) %>% collect() %>% pull("id")