bedatadriven / activityinfo-R

ActivityInfo R Language Client
https://www.activityinfo.org/support/docs/R/
18 stars 12 forks source link

Improve messages from `add/updateRecord()` functions #28

Closed Ryo-N7 closed 1 year ago

Ryo-N7 commented 1 year ago

A successful upload of add/updateRecord() returns a NULL when trying to save the output as an object. Below is the output from when I wrap purrr::safely() to updateRecord() and the update succeeds without issue:

update-add-record-result

While there is the Sending POST request to … resources/update and the OK (HTTP 200) messages that appear in the console, these messages aren't returned and can't be saved as objects. I have to go out of my way to use capture.output() to grab these messages but the outcome is less than ideal as the strings aren't formatted properly.

upload-res-save upload-res-save2 upload-res-save3

Current message that shows up in the console: Sending POST request to … resources/update is unhelpful to user as they don't really give meaningful info. Could be replaced with something like:

Updating/Adding record ID: __(id)__ in Form: __(id or label)__

Basically anything that can tell me what exactly is being acted on in the API/database. Also compared to other functions, when add/updateRecord() is successful it doesn't actually return anything. I would rather have it return some 'text' whether it's the message example provided above or something similar:

Record ID: __(id)__ in Form: __(id or label)__ OK! (HTTP 200)

Good example of something similar that already exists is the message one gets when running getFormSchema():

formsch <- activityinfo::getFormSchema(formId = "cb62ijfl8k2ymxx3")
Sending GET request to https://www.activityinfo.org/resources/form/cb62ijfl8k2ymxx3/schema
OK (HTTP 200).

Of course, when getFormSchema() is successful, it returns the actual schema object, so I don't need the message returned as an object with the result. This was just to show what I would like add/updateRecord()'s messages to look more like.

The good thing is that the form/record IDs/labels are all created as R objects inside the add/updateRecord() functions themselves OR they are directly passed into the functions as arguments so it wouldn't be hard to create the proper message texts as the info already exists inside the function itself.

nickdickinson commented 1 year ago

How about a package wide option to turn off the messages? I've refactored the code in my fork and this should be simple to do.

options(activityInfoAPIVerbose = FALSE)

Here's one package specific implementation with helper functions: https://github.com/r-spatial/mapview/blob/fdfc7717d16e1389b7bc67cf7a78cca8f1c299f6/R/options.R

Perhaps a vignette that explains how to capture and log messages to their own file, e.g. with tryCatchLog. I have created a specific condition class for the messages/errors returned from the API calls so one could separetely log just those status/code messages.

add/updateRecord could return the record ID. It will throw an error on failure.

We may need to be careful with changing return values to be sure not to break exisiting scripts. I was surprised to find that updateSchema returns the whole database structure instead of the specific schema which could have been made into an ActivityInfo schema object but perhaps some people are using this return value.

nickdickinson commented 1 year ago

Creating a new issue to have addRecord fail if the record has already been added

nickdickinson commented 1 year ago

Return the recordId or s3 class with both recordId and formId (recordRef with an s3 class)

Ryo-N7 commented 1 year ago

this is a more general point about the messages across all functions so not just related to this issue.

My idea for these issues were that the not-helpful API messages would be deleted and the 'new' messages were going to replace them completely.

is there a way to not show the API Sending GET..., Sending POST..., etc. messages? after testing the new branch they keep showing up (and i imagine will still clutter up the log files when i run them)

updaterec-success-message

updaterec-fail-message

queryTable-messages

Ryo-N7 commented 1 year ago

another example from using addDatabaseUser():

addUser-messages

with both the Send POST request to... and the response success/fail message POST request to ... success code 200 it's gotten even more verbose. Ideally it would just be the latter message instead of both.


this also kind of ties into discussions going forward for https://github.com/bedatadriven/activityinfo-R/issues/37 because if it's successful, but we're not looking for a returned object, then we need more consistency on what the API is returning besides the messages that are given in the console . but again, we can continue that discussion in that other issue

nickdickinson commented 1 year ago

I think we can silence these easily as they are just three lines in the three functions to get, post, and put resources.

On Thu, Dec 15, 2022, 12:35 Ryo Nakagawara @.***> wrote:

another example using addDatabaseUser():

[image: addUser-messages] https://user-images.githubusercontent.com/19657164/207849062-494d5ba2-ab8c-4682-821b-9648b137a406.PNG

— Reply to this email directly, view it on GitHub https://github.com/bedatadriven/activityinfo-R/issues/28#issuecomment-1352932467, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOP4AA6L4P6Y7J7HQK4KCTWNL7BDANCNFSM6AAAAAASLC6IB4 . You are receiving this because you were assigned.Message ID: @.***>

nickdickinson commented 1 year ago

Addressed in 4.30

nickdickinson commented 1 year ago

The default options in 4.30 are as follows:

options(activityinfo.verbose.requests = FALSE)
options(activityinfo.verbose.tasks = TRUE)

This then only provides the richer task based messaging with the relevant object IDs for each request. If you set activityinfo.verbose.tasks = FALSE then you will not have any messages at all. If you set activityinfo.verbose.requests = TRUE then it will also give the API call messages with URLs (most verbose).