316 use etnservice as a dependency and drop sql helpers

[PietrH]

• update from v2.3
• add patch note to NEWS
• remove sql helpers
• check if experimental code is replaced by more general helper

When I first started working on this, I didn't implement the changes to the helper immediately, but rather experimented in a single function: b03e88f, later on I moved this change to the forward_to_api() helper, meaning that a lot of lines can be removed. All this code still exists of course, just in etnservice instead of here as a duplicate.

From this PR onward, the API and local database connection should always return exactly the same result, any differences resulting from unintended type parsing should be considered a bug.

It seems not all the changes from v2.3 are present in this branch, namely the vignettes.

• fix merge conflict
• check if branch is up to date with v2.3
• pass R CMD CHECK except for Test if API and local SQL queries result in same objects #330
• remove unused dependencies -> indirect, now moved to etnservice moved to issue v2.3.1: remove dependencies that have now become indirect via etnservice #331

API - SQL mismatch

mismatches for:

• get_cpod_projects
• list_animal_project_codes
• list_acoustic_project_codes
• get_animals
• list_receiver_ids
• get_tags
• get_animal_projects
• list_scientific_names
• get_acoustic_projects

get_cpod_projects

[1] "get_cpod_projects"
Error: `api_result` (`actual`) not identical to `sql_result` (`expected`).

`actual$moratorium` is a character vector ('0', '1', '1', '1', '0', ...)
`expected$moratorium` is a logical vector (FALSE, TRUE, TRUE, TRUE, FALSE, ...)

list_animal_ids: missing animal ID

56314 known id is missing from both the API and SQL result, I've sent an email to Claudia

[PietrH]

Should we have a seperate test file that checks if the API and SQL return the same results? Or should these tests be included in the function test files?

I'm quite tempted to use:

for(function_to_test in getNamespaceExports("etnservice")) {
  test_that(paste(
    function_to_test,
    "returns same result over api as over local db connection"
  ),
  {
    skip_if_offline()
    
    api_result <- do.call(function_to_test, list(api = TRUE))
    sql_result <- do.call(function_to_test, list(api = FALSE))
    expect_identical(api_result, sql_result, label = function_to_test)
  })
}

But not all functions run without arguments (returned objects too large?)

[peterdesmet]

Maybe this is a good use case for a custom expectation.

For example, you could create a expect_call_agnostic(), with one argument: a function call. The helper could grab the function call (and its arguments), run it locally and over API and expect the result to be the same.

It could then be called as part of a regular test-function.R file:

testthat("get_animals() returns same result locally and via API", {
  expect_call_agnostic(get_animals(animal_id = "5"))
})

[PietrH]

That sounds great! Do you think we should include that here, or rather as a separate PR?

[PietrH]

9 functions currently fail to return the same result over the api as over a local connection.

[peterdesmet]

That sounds great! Do you think we should include that here, or rather as a separate PR?

I'll leave that to you

[PietrH]

Ready for review, this is a merge to the next release branch. There will be R CMD Check errors due to a mismatch between the API and SQL returned results, apart from that, it should work!

Quite a nice reduction, this will make keeping etn and etnservice in sync much much easier 🌻

[PietrH]

I have dropped a number of dependencies and helpers that are now unused (they were moved to etnservice)

[peterdesmet]

Nice cleanup. Some minor suggestions in comments.

[peterdesmet]

Don't update the version number until release. It should stay <latest-release>.9000 (so 2.2.1.9000). See #334

[peterdesmet]

Since 2.3.0 has not been released, I would include this change in 2.3.0

[peterdesmet]

Please integrate the text in the section below: maybe it's only necessary that API and R Studio will return the same results.

[peterdesmet]

I would argue that some of the functions in utils.R are that big and crucial that they should be moved to their own file, e.g. conduct_parent_to_helpers.R. Doesn't necessarily need to be tackled in this PR.