From fefd15ec2d9ddc9171b7916b98cfb8872c189fe7 Mon Sep 17 00:00:00 2001
From: James McMahon <james.mcmahon@phs.scot>
Date: Tue, 2 Apr 2024 16:22:02 +0100
Subject: [PATCH] Remove some custom code in favour of `httr::modify_url`

---
 R/get_dataset.R      |  2 +-
 R/get_resource.R     |  3 +--
 R/get_resource_sql.R |  2 +-
 R/phs_GET.R          |  5 ++---
 R/request_url.R      | 24 +++++++++++++++---------
 5 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/R/get_dataset.R b/R/get_dataset.R
index 0afd7fc..bc93a93 100644
--- a/R/get_dataset.R
+++ b/R/get_dataset.R
@@ -23,7 +23,7 @@ get_dataset <- function(dataset_name, max_resources = NULL, rows = NULL) {
   check_dataset_name(dataset_name)
 
   # define query and try API call
-  query <- paste0("id=", dataset_name)
+  query <- list("id" = dataset_name)
   content <- try(
     phs_GET("package_show", query),
     silent = TRUE
diff --git a/R/get_resource.R b/R/get_resource.R
index 55b06d1..3b1b0cf 100644
--- a/R/get_resource.R
+++ b/R/get_resource.R
@@ -47,8 +47,7 @@ get_resource <- function(res_id, rows = NULL, row_filters = NULL, col_select = N
   query[null_q_field] <- NULL
 
   # fetch the data
-  q <- paste0(paste0(names(query), "=", query), collapse = "&")
-  res_content <- phs_GET("datastore_search", q)
+  res_content <- phs_GET("datastore_search", query)
 
   # if the total number of rows is greater than the
   # number of rows fetched
diff --git a/R/get_resource_sql.R b/R/get_resource_sql.R
index fbc588e..d87cc55 100644
--- a/R/get_resource_sql.R
+++ b/R/get_resource_sql.R
@@ -85,7 +85,7 @@ get_resource_sql <- function(sql) {
   }
 
   # add query field prefix
-  query <- paste0("sql=", sql)
+  query <- list("sql" = sql)
 
   # attempt get request
   content <- phs_GET("datastore_search_sql", query)
diff --git a/R/phs_GET.R b/R/phs_GET.R
index f5e588a..38d33ca 100644
--- a/R/phs_GET.R
+++ b/R/phs_GET.R
@@ -1,7 +1,6 @@
 #' Send a GET request to the PHS CKAN API
 #'
-#' @param action The API endpoint you want to use, e.g., "package_show" / "datastore_search".
-#' @param query The query to pass to the endpoint defined by the action argument.
+#' @inheritParams request_url
 #' @param verbose TRUE or FALSE. If TRUE, a success message will be printed to the console.
 #' @return content of a httr::GET request
 #'
@@ -11,7 +10,7 @@ phs_GET <- function(action, query, verbose = FALSE) {
 
   # attempt GET request
   response <- httr::GET(
-    url,
+    url = url,
     user_agent = httr::user_agent(
       "https://github.com/Public-Health-Scotland/phsmethods"
     )
diff --git a/R/request_url.R b/R/request_url.R
index 0798ec9..c789983 100644
--- a/R/request_url.R
+++ b/R/request_url.R
@@ -14,17 +14,23 @@ request_url <- function(action, query) {
     ))
   }
 
-  # return dump URL
+  base_url <- "https://www.opendata.nhs.scot"
+
   if (action == "dump") {
-    return(paste0(
-      "https://www.opendata.nhs.scot/datastore/dump/",
-      query, "?bom=true"
-    ))
+    # return dump URL
+    url <- httr::modify_url(
+      url = base_url,
+      path = c("datastore/dump", query),
+      query = list(bom = "true")
+    )
+  } else {
+    url <- httr::modify_url(
+      url = base_url,
+      path = c("api/3/action", action),
+      query = query
+    )
   }
 
   # return standard API endpoint (i.e., not dump)
-  return(paste0(
-    "https://www.opendata.nhs.scot/api/3/action/",
-    action, "?", query
-  ))
+  return(url)
 }