Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve messages from add/updateRecord() functions #28

Closed
Ryo-N7 opened this issue Nov 25, 2022 · 8 comments
Closed

Improve messages from add/updateRecord() functions #28

Ryo-N7 opened this issue Nov 25, 2022 · 8 comments
Assignees

Comments

@Ryo-N7
Copy link
Collaborator

Ryo-N7 commented Nov 25, 2022

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
Copy link
Collaborator

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 nickdickinson changed the title Improve messages/output from add/updateRecord() functions Improve messages from add/updateRecord() functions Dec 13, 2022
@nickdickinson
Copy link
Collaborator

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

@nickdickinson
Copy link
Collaborator

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

@Ryo-N7
Copy link
Collaborator Author

Ryo-N7 commented Dec 13, 2022

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

@nickdickinson nickdickinson self-assigned this Dec 14, 2022
@Ryo-N7
Copy link
Collaborator Author

Ryo-N7 commented Dec 15, 2022

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 #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
Copy link
Collaborator

nickdickinson commented Dec 15, 2022 via email

@nickdickinson
Copy link
Collaborator

Addressed in 4.30

@nickdickinson
Copy link
Collaborator

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).

@akbertram akbertram moved this from In Progress to Done in ACDI/VOCA Enhancements Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

2 participants