From 81d37c634aa09598ad4169b656334efeaf2da5bd Mon Sep 17 00:00:00 2001
From: James McMahon <james.mcmahon@phs.scot>
Date: Thu, 28 Mar 2024 09:34:24 +0000
Subject: [PATCH 01/17] Return the res_id and name with the data.

---
 R/get_dataset.R | 52 +++++++++++++++++++++++++++++++++++--------------
 1 file changed, 37 insertions(+), 15 deletions(-)

diff --git a/R/get_dataset.R b/R/get_dataset.R
index 0afd7fc..badbe48 100644
--- a/R/get_dataset.R
+++ b/R/get_dataset.R
@@ -11,7 +11,6 @@
 #' @seealso [get_resource()] for downloading a single resource
 #' from a dataset.
 #'
-#' @importFrom magrittr %>%
 #' @return a [tibble][tibble::tibble-package] with the data
 #' @export
 #'
@@ -37,13 +36,17 @@ get_dataset <- function(dataset_name, max_resources = NULL, rows = NULL) {
 
   # define list of resource IDs to get
   all_ids <- purrr::map_chr(content$result$resources, ~ .x$id)
+  all_names <- purrr::map_chr(content$result$resources, ~ .x$name)
+
   n_res <- length(all_ids)
   res_index <- 1:min(n_res, max_resources)
-  ids_selection <- all_ids[res_index]
+
+  selection_ids <- all_ids[res_index]
+  selection_names <- all_names[res_index]
 
   # get all resources
   all_data <- purrr::map(
-    ids_selection,
+    selection_ids,
     get_resource,
     rows = rows
   )
@@ -51,9 +54,10 @@ get_dataset <- function(dataset_name, max_resources = NULL, rows = NULL) {
   # resolve class issues
   types <- purrr::map(
     all_data,
-    ~ unlist(lapply(.x, class))
+    ~ purrr::map_chr(.x, class)
   )
 
+
   # for each df, check if next df class matches
   inconsistencies <- vector(length = length(types) - 1, mode = "list")
   for (i in seq_along(types)) {
@@ -73,8 +77,7 @@ get_dataset <- function(dataset_name, max_resources = NULL, rows = NULL) {
   }
 
   # define which columns to coerce and warn
-  conflicts <- unlist(inconsistencies)
-  to_coerce <- unique(names(conflicts))
+  to_coerce <- unique(names(unlist(inconsistencies)))
 
   if (length(to_coerce) > 0) {
     cli::cli_warn(c(
@@ -82,19 +85,38 @@ get_dataset <- function(dataset_name, max_resources = NULL, rows = NULL) {
       the following {cli::qty(to_coerce)} column{?s} ha{?s/ve} been coerced to type character:",
       "{.val {to_coerce}}"
     ))
-  }
 
-  # combine
-  combined <- purrr::map_df(
-    all_data,
-    ~ dplyr::mutate(
-      .x,
-      dplyr::across(
-        dplyr::any_of(to_coerce),
-        as.character
+    all_data <- purrr::map(
+      all_data,
+      ~ dplyr::mutate(
+        .x,
+        dplyr::across(
+          dplyr::any_of(to_coerce),
+          as.character
+        )
       )
     )
+  }
+
+  # Add names
+  all_data <- purrr::pmap(
+    list(
+      "data" = all_data,
+      "ids" = selection_ids,
+      "names" = selection_names
+    ),
+    function(data, ids, names) {
+      dplyr::mutate(
+        data,
+        "res_id" = ids,
+        "res_name" = names,
+        .before = dplyr::everything()
+      )
+    }
   )
 
+  # combine
+  combined <- purrr::list_rbind(all_data)
+
   return(combined)
 }

From c9c453d7a6203eb452d01719e3715ba7e2e02a6c Mon Sep 17 00:00:00 2001
From: James McMahon <james.mcmahon@phs.scot>
Date: Wed, 3 Apr 2024 15:40:24 +0100
Subject: [PATCH 02/17] Add 'resource_show' to the valid actions list

Also, improve the clarity of the error message (likely only developers will see this error)
---
 R/request_url.R | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/R/request_url.R b/R/request_url.R
index 0798ec9..7d8b32b 100644
--- a/R/request_url.R
+++ b/R/request_url.R
@@ -6,11 +6,18 @@
 #'
 request_url <- function(action, query) {
   # check action is valid
-  valid_actions <- c("datastore_search", "datastore_search_sql", "dump", "package_show", "package_list")
+  valid_actions <- c(
+    "datastore_search",
+    "datastore_search_sql",
+    "dump",
+    "package_show",
+    "package_list",
+    "resource_show"
+  )
   if (!(action %in% valid_actions)) {
     cli::cli_abort(c(
       "API call failed.",
-      x = "Invalid {.var action} argument in request."
+      x = "{.val {action}} is an invalid {.arg action} argument."
     ))
   }
 

From 1413e00ff7e297a9032fcbdf14529e3580078e4a Mon Sep 17 00:00:00 2001
From: James McMahon <james.mcmahon@phs.scot>
Date: Wed, 3 Apr 2024 15:43:26 +0100
Subject: [PATCH 03/17] Add an argument to allow adding 'context' to the data

---
 R/get_resource.R    | 47 +++++++++++++++++++++++++++++++++++++++++----
 man/get_resource.Rd | 27 ++++++++++++++++++++++----
 2 files changed, 66 insertions(+), 8 deletions(-)

diff --git a/R/get_resource.R b/R/get_resource.R
index 55b06d1..643180f 100644
--- a/R/get_resource.R
+++ b/R/get_resource.R
@@ -3,10 +3,15 @@
 #' @param res_id The resource ID as found on
 #' \href{https://www.opendata.nhs.scot/}{NHS Open Data platform}
 #' @param rows (optional) specify the max number of rows to return.
-#' @param row_filters (optional) a named list or vector that specifies values of columns/fields to keep.
+#' @param row_filters (optional) a named list or vector that specifies values of
+#'  columns/fields to keep.
 #' e.g. list(Date = 20220216, Sex = "Female").
-#' @param col_select (optional) a character vector containing the names of desired columns/fields.
+#' @param col_select (optional) a character vector containing the names of
+#' desired columns/fields.
 #' e.g. c("Date", "Sex").
+#' @param include_context (optional) If `TRUE` additional information about the
+#' resource will be added as columns to the data, including the resource ID, the
+#' resource name, the creation date and the last modified/updated date.
 #'
 #' @seealso [get_dataset()] for downloading all resources
 #' from a given dataset.
@@ -17,11 +22,22 @@
 #'
 #' @examples
 #' res_id <- "ca3f8e44-9a84-43d6-819c-a880b23bd278"
+#'
+#' data <- get_resource(res_id)
+#'
 #' filters <- list("HB" = "S08000030", "Month" = "202109")
 #' wanted_cols <- c("HB", "Month", "TotalPatientsSeen")
 #'
-#' df <- get_resource(res_id = res_id, row_filters = filters, col_select = wanted_cols)
-get_resource <- function(res_id, rows = NULL, row_filters = NULL, col_select = NULL) {
+#' filtered_data <- get_resource(
+#'   res_id = res_id,
+#'   row_filters = filters,
+#'   col_select = wanted_cols
+#' )
+get_resource <- function(res_id,
+                         rows = NULL,
+                         row_filters = NULL,
+                         col_select = NULL,
+                         include_context = FALSE) {
   # check res_id
   check_res_id(res_id)
 
@@ -74,6 +90,7 @@ get_resource <- function(res_id, rows = NULL, row_filters = NULL, col_select = N
     ))
   }
 
+
   # extract data from response content
   data <- purrr::map_dfr(
     res_content$result$records, ~.x
@@ -82,5 +99,27 @@ get_resource <- function(res_id, rows = NULL, row_filters = NULL, col_select = N
     -dplyr::matches("_id")
   )
 
+  if (include_context) {
+    # Get resource context if required
+    context_content <- phs_GET(
+      action = "resource_show",
+      query = paste0("id=", res_id)
+    )
+
+    res_id <- context_content$result$id
+    res_name <- context_content$result$name
+    res_created_date <- context_content$result$created
+    res_modified_date <- context_content$result$modified
+
+    data <- data %>%
+      dplyr::mutate(
+        res_id = res_id,
+        res_name = res_name,
+        res_created_date = res_created_date,
+        res_modified_date = res_modified_date,
+        .before = dplyr::everything()
+      )
+  }
+
   return(data)
 }
diff --git a/man/get_resource.Rd b/man/get_resource.Rd
index 52118e1..8fa1fcf 100644
--- a/man/get_resource.Rd
+++ b/man/get_resource.Rd
@@ -4,7 +4,13 @@
 \alias{get_resource}
 \title{Get Open Data resource}
 \usage{
-get_resource(res_id, rows = NULL, row_filters = NULL, col_select = NULL)
+get_resource(
+  res_id,
+  rows = NULL,
+  row_filters = NULL,
+  col_select = NULL,
+  include_context = FALSE
+)
 }
 \arguments{
 \item{res_id}{The resource ID as found on
@@ -12,11 +18,17 @@ get_resource(res_id, rows = NULL, row_filters = NULL, col_select = NULL)
 
 \item{rows}{(optional) specify the max number of rows to return.}
 
-\item{row_filters}{(optional) a named list or vector that specifies values of columns/fields to keep.
+\item{row_filters}{(optional) a named list or vector that specifies values of
+columns/fields to keep.
 e.g. list(Date = 20220216, Sex = "Female").}
 
-\item{col_select}{(optional) a character vector containing the names of desired columns/fields.
+\item{col_select}{(optional) a character vector containing the names of
+desired columns/fields.
 e.g. c("Date", "Sex").}
+
+\item{include_context}{(optional) If \code{TRUE} additional information about the
+resource will be added as columns to the data, including the resource ID, the
+resource name, the creation date and the last modified/updated date.}
 }
 \value{
 a \link[tibble:tibble-package]{tibble} with the data
@@ -26,10 +38,17 @@ Get Open Data resource
 }
 \examples{
 res_id <- "ca3f8e44-9a84-43d6-819c-a880b23bd278"
+
+data <- get_resource(res_id)
+
 filters <- list("HB" = "S08000030", "Month" = "202109")
 wanted_cols <- c("HB", "Month", "TotalPatientsSeen")
 
-df <- get_resource(res_id = res_id, row_filters = filters, col_select = wanted_cols)
+filtered_data <- get_resource(
+  res_id = res_id,
+  row_filters = filters,
+  col_select = wanted_cols
+)
 }
 \seealso{
 \code{\link[=get_dataset]{get_dataset()}} for downloading all resources

From cb38ea0effb80dc757c94ea6c113434b0118a564 Mon Sep 17 00:00:00 2001
From: James McMahon <james.mcmahon@phs.scot>
Date: Wed, 3 Apr 2024 15:44:05 +0100
Subject: [PATCH 04/17] Set project-specific options so %>% is used by default

This is currently used throughout the package.
---
 phsopendata.Rproj | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/phsopendata.Rproj b/phsopendata.Rproj
index 69fafd4..e4e949a 100644
--- a/phsopendata.Rproj
+++ b/phsopendata.Rproj
@@ -20,3 +20,5 @@ BuildType: Package
 PackageUseDevtools: Yes
 PackageInstallArgs: --no-multiarch --with-keep.source
 PackageRoxygenize: rd,collate,namespace
+
+UseNativePipeOperator: No

From 20d046650294f55f8f6e06ccb955eaefbcc0f07c Mon Sep 17 00:00:00 2001
From: James McMahon <james.mcmahon@phs.scot>
Date: Thu, 4 Apr 2024 12:43:32 +0100
Subject: [PATCH 05/17] Add `include_context` argument to get_dataset

---
 R/get_dataset.R    | 48 +++++++++++++++++++++++++++-------------------
 man/get_dataset.Rd | 11 ++++++++++-
 2 files changed, 38 insertions(+), 21 deletions(-)

diff --git a/R/get_dataset.R b/R/get_dataset.R
index badbe48..4873030 100644
--- a/R/get_dataset.R
+++ b/R/get_dataset.R
@@ -7,6 +7,7 @@
 #' it will return the n latest resources
 #' @param rows (optional) specify the max number of rows
 #' to return for each resource.
+#' @inheritParams get_resource
 #'
 #' @seealso [get_resource()] for downloading a single resource
 #' from a dataset.
@@ -17,7 +18,10 @@
 #' @examples get_dataset("gp-practice-populations",
 #'   max_resources = 2, rows = 10
 #' )
-get_dataset <- function(dataset_name, max_resources = NULL, rows = NULL) {
+get_dataset <- function(dataset_name,
+                        max_resources = NULL,
+                        rows = NULL,
+                        include_context = FALSE) {
   # throw error if name type/format is invalid
   check_dataset_name(dataset_name)
 
@@ -36,13 +40,11 @@ get_dataset <- function(dataset_name, max_resources = NULL, rows = NULL) {
 
   # define list of resource IDs to get
   all_ids <- purrr::map_chr(content$result$resources, ~ .x$id)
-  all_names <- purrr::map_chr(content$result$resources, ~ .x$name)
 
   n_res <- length(all_ids)
   res_index <- 1:min(n_res, max_resources)
 
   selection_ids <- all_ids[res_index]
-  selection_names <- all_names[res_index]
 
   # get all resources
   all_data <- purrr::map(
@@ -98,24 +100,30 @@ get_dataset <- function(dataset_name, max_resources = NULL, rows = NULL) {
     )
   }
 
-  # Add names
-  all_data <- purrr::pmap(
-    list(
-      "data" = all_data,
-      "ids" = selection_ids,
-      "names" = selection_names
-    ),
-    function(data, ids, names) {
-      dplyr::mutate(
-        data,
-        "res_id" = ids,
-        "res_name" = names,
-        .before = dplyr::everything()
-      )
-    }
-  )
+  if (include_context) {
+    # Add the 'resource context' as columns to the data
+    all_data <- purrr::pmap(
+      list(
+        "data" = all_data,
+        "ids" = selection_ids,
+        "names" = purrr::map_chr(content$result$resources[res_index], ~ .x$name),
+        "created_dates" = purrr::map_chr(content$result$resources[res_index], ~ .x$created),
+        "modified_dates" = purrr::map_chr(content$result$resources[res_index], ~ .x$last_modified)
+      ),
+      function(data, ids, names, created_dates, modified_dates) {
+        dplyr::mutate(
+          data,
+          "res_id" = ids,
+          "res_name" = names,
+          "res_created_date" = created_dates,
+          "res_modified_date" = modified_dates,
+          .before = dplyr::everything()
+        )
+      }
+    )
+  }
 
-  # combine
+  # Combine the list of resources into a single tibble
   combined <- purrr::list_rbind(all_data)
 
   return(combined)
diff --git a/man/get_dataset.Rd b/man/get_dataset.Rd
index c12995a..855f102 100644
--- a/man/get_dataset.Rd
+++ b/man/get_dataset.Rd
@@ -4,7 +4,12 @@
 \alias{get_dataset}
 \title{Get Open Data resources from a dataset}
 \usage{
-get_dataset(dataset_name, max_resources = NULL, rows = NULL)
+get_dataset(
+  dataset_name,
+  max_resources = NULL,
+  rows = NULL,
+  include_context = FALSE
+)
 }
 \arguments{
 \item{dataset_name}{name of the dataset as found on
@@ -16,6 +21,10 @@ it will return the n latest resources}
 
 \item{rows}{(optional) specify the max number of rows
 to return for each resource.}
+
+\item{include_context}{(optional) If \code{TRUE} additional information about the
+resource will be added as columns to the data, including the resource ID, the
+resource name, the creation date and the last modified/updated date.}
 }
 \value{
 a \link[tibble:tibble-package]{tibble} with the data

From 87fce3e0ca096dafeb5c1c5aa98aa99d683707fc Mon Sep 17 00:00:00 2001
From: James McMahon <james.mcmahon@phs.scot>
Date: Wed, 10 Apr 2024 14:33:54 +0100
Subject: [PATCH 06/17] Add some new tests for `get_dataset`

Also make tests skip if it looks like the OpenData platform isn't available.
---
 tests/testthat/test-get_dataset.R | 38 ++++++++++++++++++++++++++++---
 1 file changed, 35 insertions(+), 3 deletions(-)

diff --git a/tests/testthat/test-get_dataset.R b/tests/testthat/test-get_dataset.R
index cab33b0..3c973c3 100644
--- a/tests/testthat/test-get_dataset.R
+++ b/tests/testthat/test-get_dataset.R
@@ -1,13 +1,45 @@
+skip_if_offline(host = "www.opendata.nhs.scot")
+
 test_that("returns data in the expected format", {
+  n_resources <- 2
+  n_rows <- 2
   data <- get_dataset(
     dataset_name = "gp-practice-populations",
-    max_resources = 2,
-    rows = 2
+    max_resources = n_resources,
+    rows = n_rows
   )
 
   expect_s3_class(data, "tbl_df")
+  expect_equal(nrow(data), n_resources * n_rows)
   expect_length(data, 24)
-  expect_equal(nrow(data), 2 * 2)
+  expect_named(data)
+})
+
+test_that("returns expected context with the data", {
+  n_resources <- 2
+  n_rows <- 2
+  data_col_names <- names(
+    get_dataset(
+      dataset_name = "gp-practice-populations",
+      max_resources = 1,
+      rows = 1
+    )
+  )
+  data <- get_dataset(
+    dataset_name = "gp-practice-populations",
+    max_resources = n_resources,
+    rows = n_rows,
+    include_context = TRUE
+  )
+
+  expect_s3_class(data, "tbl_df")
+  expect_equal(nrow(data), n_resources * n_rows)
+  expect_length(data, 28)
+  expect_named(
+    data,
+    c("res_id", "res_name", "res_created_date", "res_modified_date", data_col_names)
+  )
+  expect_length(unique(data[["res_id"]]), n_resources)
 })
 
 test_that("errors properly", {

From 39b798a1dc0abfb7dad352da860edb35da5ed6d6 Mon Sep 17 00:00:00 2001
From: James McMahon <james.mcmahon@phs.scot>
Date: Wed, 10 Apr 2024 16:39:05 +0100
Subject: [PATCH 07/17] Add some tests for `get_resource`

---
 R/get_resource.R                   |  5 ++++
 tests/testthat/test-get_resource.R | 48 ++++++++++++++++++++++++++++--
 2 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/R/get_resource.R b/R/get_resource.R
index 643180f..e0b1090 100644
--- a/R/get_resource.R
+++ b/R/get_resource.R
@@ -111,6 +111,11 @@ get_resource <- function(res_id,
     res_created_date <- context_content$result$created
     res_modified_date <- context_content$result$modified
 
+    # Catch if the resource has never been modified
+    if (is.null(res_modified_date)) {
+      res_modified_date <- NA_character_
+    }
+
     data <- data %>%
       dplyr::mutate(
         res_id = res_id,
diff --git a/tests/testthat/test-get_resource.R b/tests/testthat/test-get_resource.R
index bdb44f6..4c51738 100644
--- a/tests/testthat/test-get_resource.R
+++ b/tests/testthat/test-get_resource.R
@@ -1,3 +1,5 @@
+skip_if_offline(host = "www.opendata.nhs.scot")
+
 test_that("returns data in the expected format", {
   gp_list_apr_2021 <- "a794d603-95ab-4309-8c92-b48970478c14"
 
@@ -7,6 +9,7 @@ test_that("returns data in the expected format", {
   expect_s3_class(data, "tbl_df")
   expect_length(data, 15)
   expect_equal(nrow(data), 1)
+  expect_named(data)
 
   # with query
   data_q <- get_resource(
@@ -15,8 +18,49 @@ test_that("returns data in the expected format", {
     col_select = c("PracticeCode", "AddressLine1")
   )
 
-  expect_true(all(names(data_q) == c("PracticeCode", "AddressLine1")))
-  expect_true(all(data_q$PracticeCode == 10002))
+  expect_named(data_q, c("PracticeCode", "AddressLine1"))
+  expect_equal(data_q[["PracticeCode"]], 10002)
+})
+
+test_that("returns expected context with the data", {
+  gp_list_apr_2021 <- "a794d603-95ab-4309-8c92-b48970478c14"
+  data_col_names <- names(get_resource(res_id = gp_list_apr_2021, rows = 1))
+
+  # without query
+  data <- get_resource(
+    res_id = gp_list_apr_2021,
+    rows = 1,
+    include_context = TRUE
+  )
+
+  expect_s3_class(data, "tbl_df")
+  expect_length(data, 19)
+  expect_equal(nrow(data), 1)
+  expect_named(data, c(
+    "res_id",
+    "res_name",
+    "res_created_date",
+    "res_modified_date",
+    data_col_names
+  ))
+
+  # with query
+  data_q <- get_resource(
+    gp_list_apr_2021,
+    row_filters = list(PracticeCode = 10002),
+    col_select = c("PracticeCode", "AddressLine1"),
+    include_context = TRUE
+  )
+
+  expect_named(data_q, c(
+    "res_id",
+    "res_name",
+    "res_created_date",
+    "res_modified_date",
+    "PracticeCode",
+    "AddressLine1"
+  ))
+  expect_equal(data_q[["PracticeCode"]], 10002)
 })
 
 test_that("checks res_id properly", {

From ad5d12bbd01d9a3b7bb24e39ec22fc44b318d8a3 Mon Sep 17 00:00:00 2001
From: James McMahon <james.mcmahon@phs.scot>
Date: Thu, 18 Apr 2024 16:26:31 +0100
Subject: [PATCH 08/17] Update add context to also work with the dump download
 endpoint

---
 R/get_resource.R | 76 +++++++++++++++++++++++-------------------------
 1 file changed, 37 insertions(+), 39 deletions(-)

diff --git a/R/get_resource.R b/R/get_resource.R
index e0b1090..bc28d0c 100644
--- a/R/get_resource.R
+++ b/R/get_resource.R
@@ -51,53 +51,51 @@ get_resource <- function(res_id,
 
   # if dump should be used, use it
   if (use_dump_check(query, rows)) {
-    return(dump_download(res_id))
-  }
-
-  # if there is no row limit set
-  # set limit to CKAN max
-  if (is.null(query$limit)) query$limit <- 99999
+    data <- dump_download(res_id)
+  } else {
+    # if there is no row limit set
+    # set limit to CKAN max
+    if (is.null(query$limit)) query$limit <- 99999
 
-  # remove null values from query
-  null_q_field <- sapply(query, is.null)
-  query[null_q_field] <- NULL
+    # remove null values from query
+    null_q_field <- sapply(query, is.null)
+    query[null_q_field] <- NULL
 
-  # fetch the data
-  q <- paste0(paste0(names(query), "=", query), collapse = "&")
-  res_content <- phs_GET("datastore_search", q)
+    # fetch the data
+    q <- paste0(paste0(names(query), "=", query), collapse = "&")
+    res_content <- phs_GET("datastore_search", q)
 
-  # if the total number of rows is greater than the
-  # number of rows fetched
-  # AND the user was not aware of this limit (`rows` defaulted to NULL)
-  # warn the user about this limit.
-  total_rows <- res_content$result$total
-  if (is.null(rows) && query$limit < total_rows) {
-    cli::cli_warn(c(
-      "Returning the first {query$limit}
+    # if the total number of rows is greater than the
+    # number of rows fetched
+    # AND the user was not aware of this limit (`rows` defaulted to NULL)
+    # warn the user about this limit.
+    total_rows <- res_content$result$total
+    if (is.null(rows) && query$limit < total_rows) {
+      cli::cli_warn(c(
+        "Returning the first {query$limit}
       results (rows) of your query.
       {total_rows} rows match your query in total.",
-      i = "To get ALL matching rows you will need to download
+        i = "To get ALL matching rows you will need to download
       the whole resource and apply filters/selections locally."
-    ))
-  }
-
-  # if more rows were requested than received
-  # let the user know
-  if (!is.null(rows) && query$limit > total_rows) {
-    cli::cli_alert_warning(c(
-      "You set {.var rows} to {query$limit} but
-      only {total_rows} rows matched your query."
-    ))
-  }
+      ))
+    }
 
+    # if more rows were requested than received
+    # let the user know
+    if (!is.null(rows) && query$limit > total_rows) {
+      cli::cli_warn(
+        "You set {.var rows} to {.val {query$limit}} but only {.val {total_rows}} rows matched your query."
+      )
+    }
 
-  # extract data from response content
-  data <- purrr::map_dfr(
-    res_content$result$records, ~.x
-  ) %>% dplyr::select(
-    -dplyr::starts_with("rank "),
-    -dplyr::matches("_id")
-  )
+    # extract data from response content
+    data <- purrr::map_dfr(
+      res_content$result$records, ~.x
+    ) %>% dplyr::select(
+      -dplyr::starts_with("rank "),
+      -dplyr::matches("_id")
+    )
+  }
 
   if (include_context) {
     # Get resource context if required

From 9a55110c315db5d6e95a72a00aa28d245684d377 Mon Sep 17 00:00:00 2001
From: James McMahon <james.mcmahon@phs.scot>
Date: Thu, 18 Apr 2024 16:30:29 +0100
Subject: [PATCH 09/17] Run tests in parallel and split scripts to help with
 this

Tests should run faster
---
 DESCRIPTION                                | 29 ++++---
 tests/testthat.R                           |  8 ++
 tests/testthat/test-get_dataset.R          | 27 ------
 tests/testthat/test-get_dataset_context.R  | 28 +++++++
 tests/testthat/test-get_resource.R         | 97 +---------------------
 tests/testthat/test-get_resource_context.R | 42 ++++++++++
 tests/testthat/test-get_resource_dump.R    | 35 ++++++++
 7 files changed, 133 insertions(+), 133 deletions(-)
 create mode 100644 tests/testthat/test-get_dataset_context.R
 create mode 100644 tests/testthat/test-get_resource_context.R
 create mode 100644 tests/testthat/test-get_resource_dump.R

diff --git a/DESCRIPTION b/DESCRIPTION
index 6ed5bdd..88d82dc 100644
--- a/DESCRIPTION
+++ b/DESCRIPTION
@@ -1,32 +1,35 @@
 Package: phsopendata
 Title: Extract Open Data from opendata.nhs.scot
 Version: 0.1.0
-Authors@R: 
-    c(person(given = "Csilla", family = "Scharle", role = c("aut", "cre"), email = "csilla.scharle2@phs.scot"),
-      person("James", "McMahon", email = "james.mcmahon@phs.scot", role = "aut"),
-      person("David", "Aikman", email = "david.aikman@phs.scot", role = "aut"))
-Description: Functions to extract and interact with data from the Scottish Health and Social Care Open Data platform.
+Authors@R: c(
+    person("Csilla", "Scharle", , "csilla.scharle2@phs.scot", role = c("aut", "cre")),
+    person("James", "McMahon", , "james.mcmahon@phs.scot", role = "aut"),
+    person("David", "Aikman", , "david.aikman@phs.scot", role = "aut")
+  )
+Description: Functions to extract and interact with data from the Scottish
+    Health and Social Care Open Data platform.
 License: GPL (>= 2)
+URL: https://github.com/Public-Health-Scotland/phsopendata,
+    https://public-health-scotland.github.io/phsopendata/
+BugReports: https://github.com/Public-Health-Scotland/phsopendata/issues
 Imports:
+    cli,
     dplyr (>= 1.0.0),
-    httr (>= 1.0.0),
     glue (>= 1.0.0),
-    purrr,
-    tibble (>= 3.0.0),
+    httr (>= 1.0.0),
     jsonlite (>= 1.0.0),
     magrittr (>= 1.0.0),
+    purrr,
     readr (>= 1.0.0),
     stringdist,
-    cli,
+    tibble (>= 3.0.0),
     xml2
 Suggests: 
     covr,
     testthat (>= 3.0.0)
+Config/testthat/edition: 3
+Config/testthat/parallel: true
 Encoding: UTF-8
 LazyData: true
 Roxygen: list(markdown = TRUE)
 RoxygenNote: 7.3.1
-Config/testthat/edition: 3
-URL: https://github.com/Public-Health-Scotland/phsopendata,
-    https://public-health-scotland.github.io/phsopendata/
-BugReports: https://github.com/Public-Health-Scotland/phsopendata/issues
diff --git a/tests/testthat.R b/tests/testthat.R
index d6ff61b..7a7ddb5 100644
--- a/tests/testthat.R
+++ b/tests/testthat.R
@@ -1,3 +1,11 @@
+# This file is part of the standard setup for testthat.
+# It is recommended that you do not modify it.
+#
+# Where should you do additional test configuration?
+# Learn more about the roles of various files in:
+# * https://r-pkgs.org/testing-design.html#sec-tests-files-overview
+# * https://testthat.r-lib.org/articles/special-files.html
+
 library(testthat)
 library(phsopendata)
 
diff --git a/tests/testthat/test-get_dataset.R b/tests/testthat/test-get_dataset.R
index 3c973c3..cbfb74f 100644
--- a/tests/testthat/test-get_dataset.R
+++ b/tests/testthat/test-get_dataset.R
@@ -15,33 +15,6 @@ test_that("returns data in the expected format", {
   expect_named(data)
 })
 
-test_that("returns expected context with the data", {
-  n_resources <- 2
-  n_rows <- 2
-  data_col_names <- names(
-    get_dataset(
-      dataset_name = "gp-practice-populations",
-      max_resources = 1,
-      rows = 1
-    )
-  )
-  data <- get_dataset(
-    dataset_name = "gp-practice-populations",
-    max_resources = n_resources,
-    rows = n_rows,
-    include_context = TRUE
-  )
-
-  expect_s3_class(data, "tbl_df")
-  expect_equal(nrow(data), n_resources * n_rows)
-  expect_length(data, 28)
-  expect_named(
-    data,
-    c("res_id", "res_name", "res_created_date", "res_modified_date", data_col_names)
-  )
-  expect_length(unique(data[["res_id"]]), n_resources)
-})
-
 test_that("errors properly", {
   expect_error(get_dataset("Mal-formed-name"),
     regexp = "The dataset name supplied `Mal-formed-name` is invalid"
diff --git a/tests/testthat/test-get_dataset_context.R b/tests/testthat/test-get_dataset_context.R
new file mode 100644
index 0000000..2ad2182
--- /dev/null
+++ b/tests/testthat/test-get_dataset_context.R
@@ -0,0 +1,28 @@
+skip_if_offline(host = "www.opendata.nhs.scot")
+
+test_that("returns expected context with the data", {
+  n_resources <- 2
+  n_rows <- 2
+  data_col_names <- names(
+    get_dataset(
+      dataset_name = "gp-practice-populations",
+      max_resources = 1,
+      rows = 1
+    )
+  )
+  data <- get_dataset(
+    dataset_name = "gp-practice-populations",
+    max_resources = n_resources,
+    rows = n_rows,
+    include_context = TRUE
+  )
+
+  expect_s3_class(data, "tbl_df")
+  expect_equal(nrow(data), n_resources * n_rows)
+  expect_length(data, 28)
+  expect_named(
+    data,
+    c("res_id", "res_name", "res_created_date", "res_modified_date", data_col_names)
+  )
+  expect_length(unique(data[["res_id"]]), n_resources)
+})
diff --git a/tests/testthat/test-get_resource.R b/tests/testthat/test-get_resource.R
index 4c51738..128a0ce 100644
--- a/tests/testthat/test-get_resource.R
+++ b/tests/testthat/test-get_resource.R
@@ -22,100 +22,11 @@ test_that("returns data in the expected format", {
   expect_equal(data_q[["PracticeCode"]], 10002)
 })
 
-test_that("returns expected context with the data", {
+test_that("returns data with row specifications", {
   gp_list_apr_2021 <- "a794d603-95ab-4309-8c92-b48970478c14"
-  data_col_names <- names(get_resource(res_id = gp_list_apr_2021, rows = 1))
 
-  # without query
-  data <- get_resource(
-    res_id = gp_list_apr_2021,
-    rows = 1,
-    include_context = TRUE
-  )
-
-  expect_s3_class(data, "tbl_df")
-  expect_length(data, 19)
-  expect_equal(nrow(data), 1)
-  expect_named(data, c(
-    "res_id",
-    "res_name",
-    "res_created_date",
-    "res_modified_date",
-    data_col_names
-  ))
-
-  # with query
-  data_q <- get_resource(
-    gp_list_apr_2021,
-    row_filters = list(PracticeCode = 10002),
-    col_select = c("PracticeCode", "AddressLine1"),
-    include_context = TRUE
-  )
-
-  expect_named(data_q, c(
-    "res_id",
-    "res_name",
-    "res_created_date",
-    "res_modified_date",
-    "PracticeCode",
-    "AddressLine1"
-  ))
-  expect_equal(data_q[["PracticeCode"]], 10002)
-})
-
-test_that("checks res_id properly", {
-  # wrong type
-  expect_error(
-    get_resource(res_id = 123),
-    regexp = "(must be of type character)"
-  )
-  # Invalid format (doesn't match regex)
-  expect_error(
-    get_resource("a794d603-95ab-4309-8c92-b48970478c1"),
-    regexp = "(is in an invalid format.)"
-  )
-  # res_id is a vector of length > 1
-  expect_error(
-    get_resource(1:5),
-    regexp = "(must be of length 1.)"
-  )
-  # Correct format but not real
-  expect_error(
-    get_resource("00000000-0000-0000-0000-000000000000"),
-    regexp = "(Can't find resource with ID)"
-  )
-})
-
-test_that("returns full data if only res_id is input", {
-  gp_list_apr_2021 <- "a794d603-95ab-4309-8c92-b48970478c14"
-
-  data <- get_resource(res_id = gp_list_apr_2021)
-
-  expect_equal(nrow(data), 926)
-})
-
-test_that("returns full data if rows is set to over 99999", {
-  gp_list_apr_2021 <- "a794d603-95ab-4309-8c92-b48970478c14"
-
-  expect_warning(
-    data <- get_resource(
-      res_id = gp_list_apr_2021,
-      rows = 9999999,
-      row_filters = c("GPPracticeName" = "The Blue Practice")
-    ),
-    regexp = "Can't request over 99,999 rows"
-  )
-
-  expect_equal(nrow(data), 926)
-})
-
-test_that("first 99999 rows returned if query matches > 99999 rows", {
-  prescriptions_apr_2021 <- "51b7ad3f-6d52-4165-94f4-92e322656c85"
-
-  expect_warning(
-    df <- get_resource(prescriptions_apr_2021, col_select = c("HBT")),
-    regexp = "(Returning the first 99999 results)"
-  )
+  expect_equal(nrow(get_resource(res_id = gp_list_apr_2021, rows = 926)), 926)
 
-  expect_true(nrow(df) == 99999)
+  expect_equal(nrow(get_resource(res_id = gp_list_apr_2021, rows = 999)), 926) %>%
+    expect_warning()
 })
diff --git a/tests/testthat/test-get_resource_context.R b/tests/testthat/test-get_resource_context.R
new file mode 100644
index 0000000..e9a922f
--- /dev/null
+++ b/tests/testthat/test-get_resource_context.R
@@ -0,0 +1,42 @@
+skip_if_offline(host = "www.opendata.nhs.scot")
+
+test_that("returns expected context with the data", {
+  gp_list_apr_2021 <- "a794d603-95ab-4309-8c92-b48970478c14"
+  data_col_names <- names(get_resource(res_id = gp_list_apr_2021, rows = 1))
+
+  # without query
+  data <- get_resource(
+    res_id = gp_list_apr_2021,
+    rows = 1,
+    include_context = TRUE
+  )
+
+  expect_s3_class(data, "tbl_df")
+  expect_length(data, 19)
+  expect_equal(nrow(data), 1)
+  expect_named(data, c(
+    "res_id",
+    "res_name",
+    "res_created_date",
+    "res_modified_date",
+    data_col_names
+  ))
+
+  # with query
+  data_q <- get_resource(
+    gp_list_apr_2021,
+    row_filters = list(PracticeCode = 10002),
+    col_select = c("PracticeCode", "AddressLine1"),
+    include_context = TRUE
+  )
+
+  expect_named(data_q, c(
+    "res_id",
+    "res_name",
+    "res_created_date",
+    "res_modified_date",
+    "PracticeCode",
+    "AddressLine1"
+  ))
+  expect_equal(data_q[["PracticeCode"]], 10002)
+})
diff --git a/tests/testthat/test-get_resource_dump.R b/tests/testthat/test-get_resource_dump.R
new file mode 100644
index 0000000..624e5c3
--- /dev/null
+++ b/tests/testthat/test-get_resource_dump.R
@@ -0,0 +1,35 @@
+skip_if_offline(host = "www.opendata.nhs.scot")
+
+test_that("returns full data if only res_id is input", {
+  gp_list_apr_2021 <- "a794d603-95ab-4309-8c92-b48970478c14"
+
+  data <- get_resource(res_id = gp_list_apr_2021)
+
+  expect_equal(nrow(data), 926)
+})
+
+test_that("returns full data if rows is set to over 99999", {
+  gp_list_apr_2021 <- "a794d603-95ab-4309-8c92-b48970478c14"
+
+  expect_warning(
+    data <- get_resource(
+      res_id = gp_list_apr_2021,
+      rows = 9999999,
+      row_filters = c("GPPracticeName" = "The Blue Practice")
+    ),
+    regexp = "Can't request over 99,999 rows"
+  )
+
+  expect_equal(nrow(data), 926)
+})
+
+test_that("first 99999 rows returned if query matches > 99999 rows", {
+  prescriptions_apr_2021 <- "51b7ad3f-6d52-4165-94f4-92e322656c85"
+
+  expect_warning(
+    df <- get_resource(prescriptions_apr_2021, col_select = c("HBT")),
+    regexp = "(Returning the first 99999 results)"
+  )
+
+  expect_true(nrow(df) == 99999)
+})

From 29079e4ec95fefc9007896456b23ed93350d2b6d Mon Sep 17 00:00:00 2001
From: Moohan <Moohan@users.noreply.github.com>
Date: Fri, 19 Apr 2024 12:52:18 +0000
Subject: [PATCH 10/17] Style code (GHA)

---
 R/get_resource.R | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/R/get_resource.R b/R/get_resource.R
index 5a0fa2a..f151010 100644
--- a/R/get_resource.R
+++ b/R/get_resource.R
@@ -61,17 +61,17 @@ get_resource <- function(res_id,
     null_q_field <- sapply(query, is.null)
     query[null_q_field] <- NULL
 
-  # fetch the data
-  res_content <- phs_GET("datastore_search", query)
+    # fetch the data
+    res_content <- phs_GET("datastore_search", query)
 
-  # if the total number of rows is greater than the
-  # number of rows fetched
-  # AND the user was not aware of this limit (`rows` defaulted to NULL)
-  # warn the user about this limit.
-  total_rows <- res_content$result$total
-  if (is.null(rows) && query$limit < total_rows) {
-    cli::cli_warn(c(
-      "Returning the first {query$limit}
+    # if the total number of rows is greater than the
+    # number of rows fetched
+    # AND the user was not aware of this limit (`rows` defaulted to NULL)
+    # warn the user about this limit.
+    total_rows <- res_content$result$total
+    if (is.null(rows) && query$limit < total_rows) {
+      cli::cli_warn(c(
+        "Returning the first {query$limit}
       results (rows) of your query.
       {total_rows} rows match your query in total.",
         i = "To get ALL matching rows you will need to download

From 2985f4487d9313431c061265989b82577f2010bc Mon Sep 17 00:00:00 2001
From: James McMahon <james.mcmahon@phs.scot>
Date: Fri, 19 Apr 2024 14:34:29 +0100
Subject: [PATCH 11/17] Refactor the context related code to its own function

---
 R/add_context.R  | 17 +++++++++++++++++
 R/get_dataset.R  | 19 +++++--------------
 R/get_resource.R | 16 +++++-----------
 3 files changed, 27 insertions(+), 25 deletions(-)
 create mode 100644 R/add_context.R

diff --git a/R/add_context.R b/R/add_context.R
new file mode 100644
index 0000000..57f00bc
--- /dev/null
+++ b/R/add_context.R
@@ -0,0 +1,17 @@
+add_context <- function(data, id, name, created_date, modified_date) {
+  # Catch if the resource has never been modified
+  if (is.null(modified_date)) {
+    modified_date <- NA_character_
+  }
+
+  data_with_context <- dplyr::mutate(
+    data,
+    "res_id" = id,
+    "res_name" = name,
+    "res_created_date" = created_date,
+    "res_modified_date" = modified_date,
+    .before = dplyr::everything()
+  )
+
+  return(data_with_context)
+}
diff --git a/R/get_dataset.R b/R/get_dataset.R
index f17395d..98cef2f 100644
--- a/R/get_dataset.R
+++ b/R/get_dataset.R
@@ -105,21 +105,12 @@ get_dataset <- function(dataset_name,
     all_data <- purrr::pmap(
       list(
         "data" = all_data,
-        "ids" = selection_ids,
-        "names" = purrr::map_chr(content$result$resources[res_index], ~ .x$name),
-        "created_dates" = purrr::map_chr(content$result$resources[res_index], ~ .x$created),
-        "modified_dates" = purrr::map_chr(content$result$resources[res_index], ~ .x$last_modified)
+        "id" = selection_ids,
+        "name" = purrr::map_chr(content$result$resources[res_index], ~ .x$name),
+        "created_date" = purrr::map_chr(content$result$resources[res_index], ~ .x$created),
+        "modified_date" = purrr::map_chr(content$result$resources[res_index], ~ .x$last_modified)
       ),
-      function(data, ids, names, created_dates, modified_dates) {
-        dplyr::mutate(
-          data,
-          "res_id" = ids,
-          "res_name" = names,
-          "res_created_date" = created_dates,
-          "res_modified_date" = modified_dates,
-          .before = dplyr::everything()
-        )
-      }
+      add_context
     )
   }
 
diff --git a/R/get_resource.R b/R/get_resource.R
index f151010..2167c9d 100644
--- a/R/get_resource.R
+++ b/R/get_resource.R
@@ -108,18 +108,12 @@ get_resource <- function(res_id,
     res_created_date <- context_content$result$created
     res_modified_date <- context_content$result$modified
 
-    # Catch if the resource has never been modified
-    if (is.null(res_modified_date)) {
-      res_modified_date <- NA_character_
-    }
-
     data <- data %>%
-      dplyr::mutate(
-        res_id = res_id,
-        res_name = res_name,
-        res_created_date = res_created_date,
-        res_modified_date = res_modified_date,
-        .before = dplyr::everything()
+      add_context(
+        id = res_id,
+        name = res_name,
+        created_date = res_created_date,
+        modified_date = res_modified_date
       )
   }
 

From 8c74ee90c1c871c45a346219a9d28ae409e54497 Mon Sep 17 00:00:00 2001
From: James McMahon <james.mcmahon@phs.scot>
Date: Fri, 19 Apr 2024 14:39:04 +0100
Subject: [PATCH 12/17] Parse created and modified dates

---
 R/add_context.R                            | 4 ++++
 tests/testthat/test-get_resource_context.R | 9 +++++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/R/add_context.R b/R/add_context.R
index 57f00bc..2272089 100644
--- a/R/add_context.R
+++ b/R/add_context.R
@@ -4,6 +4,10 @@ add_context <- function(data, id, name, created_date, modified_date) {
     modified_date <- NA_character_
   }
 
+  # Parse the date values
+  created_date <- strptime(created_date, format = "%FT%X", tz = "UTC")
+  modified_date <- strptime(modified_date, format = "%FT%X", tz = "UTC")
+
   data_with_context <- dplyr::mutate(
     data,
     "res_id" = id,
diff --git a/tests/testthat/test-get_resource_context.R b/tests/testthat/test-get_resource_context.R
index e9a922f..504855c 100644
--- a/tests/testthat/test-get_resource_context.R
+++ b/tests/testthat/test-get_resource_context.R
@@ -7,13 +7,18 @@ test_that("returns expected context with the data", {
   # without query
   data <- get_resource(
     res_id = gp_list_apr_2021,
-    rows = 1,
+    rows = 10,
     include_context = TRUE
   )
 
   expect_s3_class(data, "tbl_df")
+  expect_type(data$res_id, "character")
+  expect_type(data$res_name, "character")
+  expect_s3_class(data$res_created_date, "POSIXlt")
+  expect_s3_class(data$res_modified_date, "POSIXlt")
+
   expect_length(data, 19)
-  expect_equal(nrow(data), 1)
+  expect_equal(nrow(data), 10)
   expect_named(data, c(
     "res_id",
     "res_name",

From 54a99c6ee3249ad9e6418d8d0ee0b4765be8d18b Mon Sep 17 00:00:00 2001
From: James McMahon <james.mcmahon@phs.scot>
Date: Wed, 24 Apr 2024 15:10:44 +0100
Subject: [PATCH 13/17] Use `last_modified` instead of `modified`

---
 R/add_context.R  | 7 +++++++
 R/get_resource.R | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/R/add_context.R b/R/add_context.R
index 2272089..6915571 100644
--- a/R/add_context.R
+++ b/R/add_context.R
@@ -8,6 +8,13 @@ add_context <- function(data, id, name, created_date, modified_date) {
   created_date <- strptime(created_date, format = "%FT%X", tz = "UTC")
   modified_date <- strptime(modified_date, format = "%FT%X", tz = "UTC")
 
+  # The platform can record the modified date as being before the created date
+  # by a few microseconds, this will catch any rounding which ensure
+  # created_date is always <= modified_date
+  if (modified_date < created_date) {
+    modified_date <- created_date
+  }
+
   data_with_context <- dplyr::mutate(
     data,
     "res_id" = id,
diff --git a/R/get_resource.R b/R/get_resource.R
index 2167c9d..0631e18 100644
--- a/R/get_resource.R
+++ b/R/get_resource.R
@@ -106,7 +106,7 @@ get_resource <- function(res_id,
     res_id <- context_content$result$id
     res_name <- context_content$result$name
     res_created_date <- context_content$result$created
-    res_modified_date <- context_content$result$modified
+    res_modified_date <- context_content$result$last_modified
 
     data <- data %>%
       add_context(

From 59ab799b5b8bfd0fe903e9570c2555a33e7e6d65 Mon Sep 17 00:00:00 2001
From: James McMahon <james.mcmahon@phs.scot>
Date: Wed, 24 Apr 2024 15:39:16 +0100
Subject: [PATCH 14/17] Add a test to check resource and dataset have the same
 context added

---
 tests/testthat/test-add_context.R | 54 +++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 tests/testthat/test-add_context.R

diff --git a/tests/testthat/test-add_context.R b/tests/testthat/test-add_context.R
new file mode 100644
index 0000000..4ebb6d2
--- /dev/null
+++ b/tests/testthat/test-add_context.R
@@ -0,0 +1,54 @@
+test_that("Returned context is the same for resource and dataset", {
+  dataset <- get_dataset(
+    "general-practitioner-contact-details",
+    rows = 10,
+    include_context = TRUE
+  )
+
+  res_id_1 <- "647b256e-4a03-4963-8402-bf559c9e2fff"
+  resource_1 <- get_resource(
+    res_id = res_id_1,
+    rows = 10,
+    include_context = TRUE
+  )
+
+  res_id_2 <- "e37c14fe-51f7-4935-87d1-c79b30fe8824"
+  resource_2 <- get_resource(
+    res_id = res_id_2,
+    rows = 10,
+    include_context = TRUE
+  )
+
+  # --- Remove from here
+  # This code works around an issue with vctrs
+  # https://github.com/r-lib/vctrs/issues/1930
+  dataset_has_POSIXlt <- inherits(dataset$res_created_date, "POSIXlt")
+
+  # If this test fails, that's good (probably) and this can all be removed
+  expect_error(stopifnot(dataset_has_POSIXlt))
+
+  if(!dataset_has_POSIXlt) {
+  dataset <- dataset %>%
+    dplyr::mutate(
+      dplyr::across(c("res_created_date","res_modified_date"),
+                      as.POSIXlt)
+    )
+  }
+  # --- Remove to here
+
+  expect_equal(
+    dataset %>%
+      dplyr::filter(res_id == res_id_1) %>%
+      dplyr::select(!dplyr::where(~anyNA(.x))),
+    resource_1,
+    list_as_map = TRUE
+  )
+  expect_equal(
+    dataset %>%
+      dplyr::filter(res_id == res_id_2) %>%
+      dplyr::select(!dplyr::where(~anyNA(.x))),
+    resource_2,
+    list_as_map = TRUE
+  )
+  # list_as_map = TRUE will sort variable names before comparing
+})

From 24d7eee8bfae6e2d864d5a4d700e4ddcee624ee1 Mon Sep 17 00:00:00 2001
From: Moohan <Moohan@users.noreply.github.com>
Date: Wed, 24 Apr 2024 14:41:06 +0000
Subject: [PATCH 15/17] Style code (GHA)

---
 tests/testthat/test-add_context.R | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/tests/testthat/test-add_context.R b/tests/testthat/test-add_context.R
index 4ebb6d2..aa1ef35 100644
--- a/tests/testthat/test-add_context.R
+++ b/tests/testthat/test-add_context.R
@@ -27,26 +27,28 @@ test_that("Returned context is the same for resource and dataset", {
   # If this test fails, that's good (probably) and this can all be removed
   expect_error(stopifnot(dataset_has_POSIXlt))
 
-  if(!dataset_has_POSIXlt) {
-  dataset <- dataset %>%
-    dplyr::mutate(
-      dplyr::across(c("res_created_date","res_modified_date"),
-                      as.POSIXlt)
-    )
+  if (!dataset_has_POSIXlt) {
+    dataset <- dataset %>%
+      dplyr::mutate(
+        dplyr::across(
+          c("res_created_date", "res_modified_date"),
+          as.POSIXlt
+        )
+      )
   }
   # --- Remove to here
 
   expect_equal(
     dataset %>%
       dplyr::filter(res_id == res_id_1) %>%
-      dplyr::select(!dplyr::where(~anyNA(.x))),
+      dplyr::select(!dplyr::where(~ anyNA(.x))),
     resource_1,
     list_as_map = TRUE
   )
   expect_equal(
     dataset %>%
       dplyr::filter(res_id == res_id_2) %>%
-      dplyr::select(!dplyr::where(~anyNA(.x))),
+      dplyr::select(!dplyr::where(~ anyNA(.x))),
     resource_2,
     list_as_map = TRUE
   )

From c9336246d0af2c55f9d00ca8bde7dc2f5cc18aa7 Mon Sep 17 00:00:00 2001
From: James McMahon <james.mcmahon@phs.scot>
Date: Thu, 25 Apr 2024 10:56:38 +0100
Subject: [PATCH 16/17] Use POSIXct instead of POSIXlt for dates

This was the advice I got back from the `{vctrs}` team.
---
 R/add_context.R                            |  4 ++--
 tests/testthat/test-add_context.R          | 19 -------------------
 tests/testthat/test-get_dataset_context.R  |  5 +++++
 tests/testthat/test-get_resource_context.R |  4 ++--
 4 files changed, 9 insertions(+), 23 deletions(-)

diff --git a/R/add_context.R b/R/add_context.R
index 6915571..1083b8e 100644
--- a/R/add_context.R
+++ b/R/add_context.R
@@ -5,8 +5,8 @@ add_context <- function(data, id, name, created_date, modified_date) {
   }
 
   # Parse the date values
-  created_date <- strptime(created_date, format = "%FT%X", tz = "UTC")
-  modified_date <- strptime(modified_date, format = "%FT%X", tz = "UTC")
+  created_date <- as.POSIXct(created_date, format = "%FT%X", tz = "UTC")
+  modified_date <- as.POSIXct(modified_date, format = "%FT%X", tz = "UTC")
 
   # The platform can record the modified date as being before the created date
   # by a few microseconds, this will catch any rounding which ensure
diff --git a/tests/testthat/test-add_context.R b/tests/testthat/test-add_context.R
index aa1ef35..44580f8 100644
--- a/tests/testthat/test-add_context.R
+++ b/tests/testthat/test-add_context.R
@@ -19,25 +19,6 @@ test_that("Returned context is the same for resource and dataset", {
     include_context = TRUE
   )
 
-  # --- Remove from here
-  # This code works around an issue with vctrs
-  # https://github.com/r-lib/vctrs/issues/1930
-  dataset_has_POSIXlt <- inherits(dataset$res_created_date, "POSIXlt")
-
-  # If this test fails, that's good (probably) and this can all be removed
-  expect_error(stopifnot(dataset_has_POSIXlt))
-
-  if (!dataset_has_POSIXlt) {
-    dataset <- dataset %>%
-      dplyr::mutate(
-        dplyr::across(
-          c("res_created_date", "res_modified_date"),
-          as.POSIXlt
-        )
-      )
-  }
-  # --- Remove to here
-
   expect_equal(
     dataset %>%
       dplyr::filter(res_id == res_id_1) %>%
diff --git a/tests/testthat/test-get_dataset_context.R b/tests/testthat/test-get_dataset_context.R
index 2ad2182..e16a39a 100644
--- a/tests/testthat/test-get_dataset_context.R
+++ b/tests/testthat/test-get_dataset_context.R
@@ -18,6 +18,11 @@ test_that("returns expected context with the data", {
   )
 
   expect_s3_class(data, "tbl_df")
+  expect_type(data$res_id, "character")
+  expect_type(data$res_name, "character")
+  expect_s3_class(data$res_created_date, "POSIXct")
+  expect_s3_class(data$res_modified_date, "POSIXct")
+
   expect_equal(nrow(data), n_resources * n_rows)
   expect_length(data, 28)
   expect_named(
diff --git a/tests/testthat/test-get_resource_context.R b/tests/testthat/test-get_resource_context.R
index 504855c..20f7ebc 100644
--- a/tests/testthat/test-get_resource_context.R
+++ b/tests/testthat/test-get_resource_context.R
@@ -14,8 +14,8 @@ test_that("returns expected context with the data", {
   expect_s3_class(data, "tbl_df")
   expect_type(data$res_id, "character")
   expect_type(data$res_name, "character")
-  expect_s3_class(data$res_created_date, "POSIXlt")
-  expect_s3_class(data$res_modified_date, "POSIXlt")
+  expect_s3_class(data$res_created_date, "POSIXct")
+  expect_s3_class(data$res_modified_date, "POSIXct")
 
   expect_length(data, 19)
   expect_equal(nrow(data), 10)

From 24aa4e1454550e5844fb501d4d9f12d7294f55c3 Mon Sep 17 00:00:00 2001
From: James McMahon <james.mcmahon@phs.scot>
Date: Thu, 9 May 2024 15:18:12 +0100
Subject: [PATCH 17/17] Update the variable names to use CamelCase

---
 R/add_context.R                            |  8 ++++----
 tests/testthat/test-add_context.R          |  4 ++--
 tests/testthat/test-get_dataset_context.R  | 12 +++++------
 tests/testthat/test-get_resource_context.R | 24 +++++++++++-----------
 4 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/R/add_context.R b/R/add_context.R
index 1083b8e..c473b4d 100644
--- a/R/add_context.R
+++ b/R/add_context.R
@@ -17,10 +17,10 @@ add_context <- function(data, id, name, created_date, modified_date) {
 
   data_with_context <- dplyr::mutate(
     data,
-    "res_id" = id,
-    "res_name" = name,
-    "res_created_date" = created_date,
-    "res_modified_date" = modified_date,
+    "ResID" = id,
+    "ResName" = name,
+    "ResCreatedDate" = created_date,
+    "ResModifiedDate" = modified_date,
     .before = dplyr::everything()
   )
 
diff --git a/tests/testthat/test-add_context.R b/tests/testthat/test-add_context.R
index 44580f8..c144ce0 100644
--- a/tests/testthat/test-add_context.R
+++ b/tests/testthat/test-add_context.R
@@ -21,14 +21,14 @@ test_that("Returned context is the same for resource and dataset", {
 
   expect_equal(
     dataset %>%
-      dplyr::filter(res_id == res_id_1) %>%
+      dplyr::filter(ResID == res_id_1) %>%
       dplyr::select(!dplyr::where(~ anyNA(.x))),
     resource_1,
     list_as_map = TRUE
   )
   expect_equal(
     dataset %>%
-      dplyr::filter(res_id == res_id_2) %>%
+      dplyr::filter(ResID == res_id_2) %>%
       dplyr::select(!dplyr::where(~ anyNA(.x))),
     resource_2,
     list_as_map = TRUE
diff --git a/tests/testthat/test-get_dataset_context.R b/tests/testthat/test-get_dataset_context.R
index e16a39a..065106a 100644
--- a/tests/testthat/test-get_dataset_context.R
+++ b/tests/testthat/test-get_dataset_context.R
@@ -18,16 +18,16 @@ test_that("returns expected context with the data", {
   )
 
   expect_s3_class(data, "tbl_df")
-  expect_type(data$res_id, "character")
-  expect_type(data$res_name, "character")
-  expect_s3_class(data$res_created_date, "POSIXct")
-  expect_s3_class(data$res_modified_date, "POSIXct")
+  expect_type(data$ResID, "character")
+  expect_type(data$ResName, "character")
+  expect_s3_class(data$ResCreatedDate, "POSIXct")
+  expect_s3_class(data$ResModifiedDate, "POSIXct")
 
   expect_equal(nrow(data), n_resources * n_rows)
   expect_length(data, 28)
   expect_named(
     data,
-    c("res_id", "res_name", "res_created_date", "res_modified_date", data_col_names)
+    c("ResID", "ResName", "ResCreatedDate", "ResModifiedDate", data_col_names)
   )
-  expect_length(unique(data[["res_id"]]), n_resources)
+  expect_length(unique(data[["ResID"]]), n_resources)
 })
diff --git a/tests/testthat/test-get_resource_context.R b/tests/testthat/test-get_resource_context.R
index 20f7ebc..d6a97e7 100644
--- a/tests/testthat/test-get_resource_context.R
+++ b/tests/testthat/test-get_resource_context.R
@@ -12,18 +12,18 @@ test_that("returns expected context with the data", {
   )
 
   expect_s3_class(data, "tbl_df")
-  expect_type(data$res_id, "character")
-  expect_type(data$res_name, "character")
-  expect_s3_class(data$res_created_date, "POSIXct")
-  expect_s3_class(data$res_modified_date, "POSIXct")
+  expect_type(data$ResID, "character")
+  expect_type(data$ResName, "character")
+  expect_s3_class(data$ResCreatedDate, "POSIXct")
+  expect_s3_class(data$ResModifiedDate, "POSIXct")
 
   expect_length(data, 19)
   expect_equal(nrow(data), 10)
   expect_named(data, c(
-    "res_id",
-    "res_name",
-    "res_created_date",
-    "res_modified_date",
+    "ResID",
+    "ResName",
+    "ResCreatedDate",
+    "ResModifiedDate",
     data_col_names
   ))
 
@@ -36,10 +36,10 @@ test_that("returns expected context with the data", {
   )
 
   expect_named(data_q, c(
-    "res_id",
-    "res_name",
-    "res_created_date",
-    "res_modified_date",
+    "ResID",
+    "ResName",
+    "ResCreatedDate",
+    "ResModifiedDate",
     "PracticeCode",
     "AddressLine1"
   ))