Review round 1: tests in R CMD check, API errors, safer rmGeoIdentity

.Rbuildignore

@@ -8,9 +8,10 @@
 ^CODE_OF_CONDUCT\.md$
 ^README\.md$
 ^CONTRIBUTING\.md$
-^tests$
 ^dev$
 ^logo\.svg$
 ^codemeta\.json$
 ^tic\.R$
 ^LICENSE\.md$
+^\.github$
+^AGENTS\.md$

.github/workflows/R-CMD-check.yaml

@@ -0,0 +1,50 @@
+# Standard R CMD check on Linux, macOS and Windows (r-lib/actions v2)
+on:
+  push:
+    branches: [master]
+  pull_request:
+  workflow_dispatch:
+
+name: R-CMD-check
+
+permissions: read-all
+
+jobs:
+  R-CMD-check:
+    runs-on: ${{ matrix.config.os }}
+    name: ${{ matrix.config.os }} (${{ matrix.config.r }})
+    strategy:
+      fail-fast: false
+      matrix:
+        config:
+          - {os: ubuntu-latest, r: 'release'}
+          - {os: macos-latest, r: 'release'}
+          - {os: windows-latest, r: 'release'}
+    env:
+      GITHUB_PAT: ${{ secrets.GITHUB_TOKEN }}
+      R_KEEP_PKG_SOURCE: yes
+      CLIMMOB_KEY: ${{ secrets.CLIMMOB_KEY }}
+    steps:
+      - uses: actions/checkout@v4
+      - uses: r-lib/actions/setup-pandoc@v2
+      - uses: r-lib/actions/setup-r@v2
+        with:
+          r-version: ${{ matrix.config.r }}
+          use-public-rspm: true
+      - uses: r-lib/actions/setup-r-dependencies@v2
+        with:
+          extra-packages: any::rcmdcheck
+          needs: check
+      - uses: r-lib/actions/check-r-package@v2
+        with:
+          upload-snapshots: true
+          build_args: 'c("--no-manual","--compact-vignettes=gs+qpdf")'
+      - name: Show check errors as annotations
+        if: failure()
+        shell: bash
+        run: |
+          for f in check/*.Rcheck/00check.log check/*.Rcheck/tests/testthat.Rout.fail; do
+            [ -f "$f" ] || continue
+            msg=$(tail -c 6000 "$f" | sed 's/%/%25/g' | sed ':a;N;$!ba;s/\n/%0A/g')
+            echo "::error title=$(basename "$f")::${msg}"
+          done

AGENTS.md

@@ -0,0 +1,47 @@
+# Instructions for AI agents
+
+ClimMobTools is the R client for the ClimMob platform (https://climmob.net), which runs tricot trials. It downloads project data through the ClimMob API, designs the randomised packages for a trial, and prepares the data for analysis. People share this repository with an assistant either to **work with their ClimMob data** or to **change the package**. Find out which before you start.
+
+## Before you write any code
+
+1. Check the installed version with `packageVersion("ClimMobTools")` and read the help of every function you use. Arguments change between versions, so do not rely on memory.
+2. The analysis of rankings happens in gosset and PlackettLuce, not here. For methods and evidence on tricot, read the knowledge base at https://github.com/AgrDataSci/tricot-knowledge-base (start with its `AGENTS.md`).
+
+## API keys and personal data
+
+- **Never write an API key in a script, a test, an issue or a message.** Ask the user to store it in the environment variable `CLIMMOB_KEY` and read it with `Sys.getenv("CLIMMOB_KEY")`.
+- ClimMob data hold names, phone numbers and locations of participants. Never put real data in examples, tests or issues. Use simulated data.
+- Before sharing coordinates, displace them with `rmGeoIdentity()` and **do not set or publish its seed**. With the seed, anyone can recover the exact locations.
+
+## Which function for which task
+
+| Task | Function |
+|---|---|
+| List the projects a key can access | `getProjectsCM()` |
+| Download the data of a project | `getDataCM()`, then `as.data.frame()` options for wide or long format |
+| Check how many submissions each data collection moment has | `getProjectProgress()` |
+| List the traits assessed in a project | `getTraitList()` |
+| Export tidy tables for analysis | `exportTricotRanks()`, `exportMeasuredTraits()`, `exportBlockData()`, `exportTrialMetadata()`, `exportVariablesDescription()` |
+| Design the packages of a new trial (which items each participant receives) | `randomise()` or `randomize()` |
+| Build Plackett-Luce rankings from tricot data | prefer `gosset::rank_tricot()`; `rankTricot()` here is a copy kept for compatibility |
+| Displace coordinates before sharing | `rmGeoIdentity()` |
+
+The `server` argument selects the ClimMob instance (for example `"climmob3"`, `"1000farms"`, `"avisa"`, `"rtb"`).
+
+## Trial design
+
+`randomise()` allocates items to packages so that items appear about equally often, are spread across the positions A, B and C, and form a connected comparison network. `availability` caps how many packages each item can go to, and `props` sets unequal shares. Call `set.seed()` before it when the design must be reproducible, and keep the design with the project records.
+
+## Changing the package
+
+- Use `=` for assignment in new code. Use base R where possible.
+- Document with roxygen and run `devtools::document()`. Never edit files in `man/` by hand.
+- Every fix needs a test in `tests/testthat/` that fails before the fix and passes after it. Tests that call the API must use `skip_without_key()` (see `tests/testthat/helper-key.R`). Run `devtools::test()` and `devtools::check()`, and report what was actually run.
+- Record changes in `NEWS.md`. Keep changes backwards compatible, or record the break and the reason.
+- ClimMob-analysis (https://github.com/AgrDataSci/ClimMob-analysis) depends on this package. Check it when changing what a function returns.
+- Work on a branch and open a pull request. Do not merge without review and passing checks.
+
+## Where to find more
+
+- Reference and vignettes: https://agrdatasci.github.io/ClimMobTools/
+- Issues: https://github.com/AgrDataSci/ClimMobTools/issues

DESCRIPTION

@@ -39,7 +39,8 @@ Suggests:
     rmarkdown,
     sf,
     PlackettLuce,
-    testthat (>= 2.1.0)
+    testthat (>= 3.0.0)
 Language: en-US
 RoxygenNote: 7.3.2
 VignetteBuilder: knitr
+Config/testthat/edition: 3

NEWS.md

@@ -1,3 +1,34 @@
+ClimMobTools (development version)
+=========================
+
+### PRIVACY
+
+* Tests read the API key from the environment variable `CLIMMOB_KEY`
+  instead of a key written in the test files, and are skipped when it is
+  not set.
+* `rmGeoIdentity()` no longer prints the seed it uses. The random
+  displacement does not depend on the original coordinates, so anyone who
+  knows the seed can recover the exact locations. Leave `seed = NULL` for
+  data that will be shared. A given seed no longer changes the caller's
+  random number stream. The function now calls 'sf' by namespace, so 'sf'
+  no longer needs to be attached.
+
+### BUG FIXES
+
+* `getDataCM()` and `getProjectProgress()` build the request body with
+  'jsonlite', so quotes in project codes or user names no longer break the
+  request, and stop with a clear message when the server returns an error
+  status instead of failing while parsing the response.
+
+### TESTS
+
+* Tests now run under `R CMD check` (they were excluded in `.Rbuildignore`).
+  Outdated tests for removed functions and arguments were replaced by tests
+  of the current `randomise()` design: distinct items per package, equal
+  item counts, balance across positions, a connected comparison network,
+  availability limits and reproducibility. Added tests for `rmGeoIdentity()`.
+* Added a standard R CMD check workflow for Linux, macOS and Windows.
+
 ClimMobTools 1.8.2 (2025-11-26)
 =========================
 

R/AAA-getDataCM.R

@@ -59,13 +59,14 @@ getDataCM = function(key,
 
   cmdata = httr::RETRY(verb = "GET", 
                        url = url,
-                       query = list(Body = paste0('{"project_cod":"', project, '",
-                                                   "user_owner":"',userowner,'"}'),
+                       query = list(Body = .request_body(project, userowner),
                                     Apikey = key),
                        httr::accept_json(), 
                        terminate_on = c(403, 404))
 
-  cmdata = httr::content(cmdata, as = "text")
+  .check_response(cmdata)
+
+  cmdata = httr::content(cmdata, as = "text", encoding = "UTF-8")
 
   if (as.text) {
     return(cmdata)

R/getProjectProgress.R

@@ -31,13 +31,14 @@ getProjectProgress = function(key, project, userowner, server = "climmob3"){
 
   dat = httr::RETRY(verb = "GET", 
                     url = url,
-                    query = list(Body = paste0('{"project_cod":"', project, '",
-                                                   "user_owner":"',userowner,'"}'),
+                    query = list(Body = .request_body(project, userowner),
                                  Apikey = key),
                     httr::accept_json(), 
                     terminate_on = c(403, 404))
 
-  dat = httr::content(dat, as = "text")
+  .check_response(dat)
+
+  dat = httr::content(dat, as = "text", encoding = "UTF-8")
 
   dat = jsonlite::fromJSON(dat)
 

R/internal_functions.R

@@ -201,3 +201,30 @@
 
 }
 
+#' JSON body for project requests
+#' Built with jsonlite so that quotes or other special characters in
+#' project codes or user names cannot break the request
+#' @noRd
+.request_body = function(project, userowner) {
+  jsonlite::toJSON(list(project_cod = project, user_owner = userowner),
+                   auto_unbox = TRUE)
+}
+
+#' Stop with a clear message when the server does not return success
+#' @noRd
+.check_response = function(response) {
+  status = httr::status_code(response)
+  if (status == 401 || status == 403) {
+    stop("ClimMob refused the request (HTTP ", status, "). ",
+         "Check the API key and that you have access to this project.", call. = FALSE)
+  }
+  if (status == 404) {
+    stop("ClimMob could not find the project or user (HTTP 404). ",
+         "Check the project code and the user owner.", call. = FALSE)
+  }
+  if (status >= 400) {
+    stop("ClimMob returned HTTP ", status, ". Try again later or check the server name.",
+         call. = FALSE)
+  }
+  invisible(TRUE)
+}

R/rmGeoIdentity.R

@@ -8,8 +8,11 @@
 #' @param longlat a data.frame or matrix with geographical coordinates long lat
 #' @param dist numeric, buffer distance for all \var{lonlat}
 #' @param nQuadSegs integer, number of segments per quadrant
-#' @param seed integer, the seed for random number generation.
-#'  If NULL (the default), ClimMobTools will set the seed randomly
+#' @param seed integer, the seed for random number generation. Leave it
+#'  \code{NULL} (the default) for data that will be shared. The random
+#'  displacement does not depend on the original coordinates, so anyone who
+#'  knows the seed can reproduce it and recover the exact locations. Use a
+#'  seed only for tests or internal reproducibility, and never publish it
 #' @param ... further arguments passed to \code{\link[sf]{sf}} methods
 #' @return A data frame with the random coordinates long lat within the buffer
 #' @examplesIf interactive()
@@ -20,7 +23,7 @@
 #'                11.254428, 60.822457),
 #'              nrow = 3, ncol = 2, byrow = TRUE)
 #' 
-#' rmGeoIdentity(xy, seed = 1501)
+#' rmGeoIdentity(xy)
 #' 
 #' #' the function also handles NAs
 #' 
@@ -37,6 +40,10 @@
 #' @export
 rmGeoIdentity = function(longlat, dist = 0.015, nQuadSegs = 2L, seed = NULL, ...){
 
+  if (!requireNamespace("sf", quietly = TRUE)) {
+    stop("rmGeoIdentity() needs the package 'sf'; please install it")
+  }
+
   longlat = as.matrix(longlat)
 
   n = nrow(longlat)
@@ -53,36 +60,44 @@ rmGeoIdentity = function(longlat, dist = 0.015, nQuadSegs = 2L, seed = NULL, ...
   # transform into sf points
   longlat = lapply(longlat, function(l) {
     a = list(x = l)
-    do.call("st_point", a)
+    do.call(sf::st_point, a)
   })
 
   # and then into a geometry list column
-  longlat = do.call("st_sfc", longlat)
+  longlat = do.call(sf::st_sfc, longlat)
 
   args = list(x = longlat,
               dist = dist, 
               nQuadSegs = nQuadSegs)
 
-  lonlatb = do.call("st_buffer", args)
+  lonlatb = do.call(sf::st_buffer, args)
 
   result = split(lonlatb, seq_len(n))
 
-  # check if a seed is provided
-  if (is.null(seed)) {
-    seed = as.integer(stats::runif(1, 0, 1000000))
-    message("seed ", seed, " was used in the sampling \n")
+  # A seed makes the displacement reproducible, and anyone who knows it
+  # can recover the original points. Without a seed the current random
+  # stream is used and nothing is printed. With a seed, the caller's
+  # random number state is restored on exit
+  if (!is.null(seed)) {
+    if (exists(".Random.seed", envir = globalenv(), inherits = FALSE)) {
+      old_seed = get(".Random.seed", envir = globalenv(), inherits = FALSE)
+      on.exit(assign(".Random.seed", old_seed, envir = globalenv()), add = TRUE)
+    } else {
+      on.exit(if (exists(".Random.seed", envir = globalenv(), inherits = FALSE)) {
+        rm(".Random.seed", envir = globalenv())
+      }, add = TRUE)
+    }
+    set.seed(seed)
   }
 
-  set.seed(seed)
-
   result[!anyNAs] = lapply(result[!anyNAs], function(x){
     a = list(x = x, size = 1, type = "random", by_polygon = TRUE)
-    do.call("st_sample", a)
+    do.call(sf::st_sample, a)
   })
 
   result = do.call(rbind, result)
 
-  result = do.call("st_sfc", result)
+  result = do.call(sf::st_sfc, result)
 
   r = matrix(NA, nrow = n, ncol = 2)
 

tests/testthat/helper-key.R

@@ -0,0 +1,8 @@
+# API tests need a ClimMob key in the environment variable CLIMMOB_KEY.
+# Never write a key in a test file: this repository is public.
+climmob_key = function() Sys.getenv("CLIMMOB_KEY")
+
+skip_without_key = function() {
+  skip_on_cran()
+  skip_if(climmob_key() == "", "CLIMMOB_KEY is not set")
+}

tests/testthat/test-getDataCM.R

@@ -1,36 +1,20 @@
-context("test-getDataCM")
-
-load("../test_data.rda")
-
-key <- "d39a3c66-5822-4930-a9d4-50e7da041e77"
-
-test_that("pivot.wider", {
-
-  cm <- getDataCM(key,
-                  "chocolates",
-                  pivot.wider = TRUE,
-                  tidynames = TRUE)
-
-  cm <- all(cm == chocolates, na.rm = TRUE)
-
-  expect_true(cm)
-
+test_that("data from the example project match the stored copy", {
+  skip_without_key()
+  load(test_path("..", "test_data.rda"))
+  cm = getDataCM(climmob_key(), "chocolates",
+                 pivot.wider = TRUE, tidynames = TRUE)
+  expect_true(all(cm == chocolates, na.rm = TRUE))
 })
 
-test_that("no API key", {
-
-  expect_error(getDataCM("1234", "x"))
-
+test_that("an invalid key gives a clear error", {
+  skip_on_cran()
+  skip_if_offline("climmob.net")
+  expect_error(getDataCM("1234", "x", "y"))
 })
 
-
-test_that("error no data", {
-  expect_error(
-    getDataCM(key,
-              "empty")
-  )
-}
-)
-
-
-
+test_that("request bodies are valid JSON even with quotes in names", {
+  b = ClimMobTools:::.request_body('my "project"', "user")
+  parsed = jsonlite::fromJSON(b)
+  expect_equal(parsed$project_cod, 'my "project"')
+  expect_equal(parsed$user_owner, "user")
+})