USAID-OHA-SI / grabr

OHA/SI APIs package
https://usaid-oha-si.github.io/grabr/
Other
1 stars 2 forks source link

Credential Issue > Error Message #31

Closed achafetz closed 10 months ago

achafetz commented 1 year ago

When running the DATIM API, if you're credentials are outdated (ie you are using an old password) or you don't actually have access to the database you are hitting (eg final.datim.org), you get the following returned

"<webMessage xmlns=\"http://dhis2.org/schema/dxf/2.0\" httpStatus=\"Unauthorized\" httpStatusCode=\"401\" status=\"ERROR\">Unauthorized"

Can we implement a error handling through tryCatch to provide a useful message back to the user saying to check their credentials or make sure they have access to the baseurl?

I had trouble with tryCatch but would this work @baboyma?

  test_creds <- function(username, password, baseurl){
      r <- httr::GET(baseurl, httr::authenticate(username, password))

    if(r$status_code == 401)
      usethis::ui_stop("No access to the url. Check that your credentials, used or stored, are current and that you have access to {baseurl}")
    }

Test

library(tidyverse)

  test_creds(glamr::datim_user(),
             "wrong password",
             "https://final.datim.org/api") 

  test_creds(glamr::datim_user(),
             glamr::datim_pwd(),
             "https://final.datim.org/api") 
baboyma commented 1 year ago

Agreed - definitely a tryCatch.

Will update.

baboyma commented 1 year ago

Fixed within datim_execute_query()

# Reject non 200 (OK) responses
      if (.res$status_code != 200) {
        usethis::ui_stop(paste0(.status$reason, " - ", .status$message))
      }
achafetz commented 1 year ago

Test script

devtools::load_all()

#test UPDATE - Datim query errors - flag non 200 statuses
test_url <- "https://www.datim.org/api/analytics.json?dimension=LxhLO68FcXm%3AudCop657yzi&dimension=pe%3A2022Oct&dimension=ou%3ALEVEL-yzQSdm94CZT%3Bybg3MO3hcf4%3ALEVEL-yzQSdm94CZT%3Bybg3MO3hcf4&outputIdScheme=NAME&filter=RUkVjD3BsS1%3APE5QVF0w4xj"

#should work
datim_process_query(test_url, glamr::datim_user(), glamr::datim_pwd())

#should error and give flag for wrong credentials
datim_process_query(test_url, username = "blah", password = "blah")

With the wrong credential line, we're not getting the desired output. image

The output gives the query url and status code and then repeats the same url in orange, given an error msg about the baseurl and then an Error line.

baseurl does not reside anywhere, so I extracted the base from query_url and pasted that in.

There is still way to much going on. image

I would say there is no need to repeat the url unless there is an error since this is TMI (or at least add a param to turn this on if desired, but the default should be to not show. So I would say removing these lines https://github.com/USAID-OHA-SI/grabr/blob/775f4f19cf746ce38a11afb3a867cb4bd475a04b/R/extract_datim.R#L376-L377

Then the error function at the end isn't working properly. It's not outputting the url or error. This shouldn't also pop up now that you have error messages based on the status in ln381-388

baboyma commented 11 months ago

Resolved and pushed to develop - ready for testing

achafetz commented 11 months ago

Looks good! Error came up immediately image