Align datapack sitetool code #44
Conversation
The correct UID should be part of the object, thus, no need to go back to the model to try and get it.
| #' disaggs used in a specific sheet. | ||
| defunctDisaggs <- function(d) { | ||
|
|
||
| defunct <- d$data$extract %>% |
There was a problem hiding this comment.
Maybe it would make sense if possible to have this as a configuration file? We know there are likely going to be defunct disaggs next year, so a more flexible combination of indicator code and disagg might work?
| dplyr::filter(value < 0) %>% | ||
| dplyr::pull(indicatorCode) %>% | ||
| dplyr::pull(indicator_code) %>% |
There was a problem hiding this comment.
change to indicator code will require very minor change to datapack commons code (https://github.com/pepfar-datim/data-pack-commons/search?q=indicatorCode&unscoped_q=indicatorCode) I added an issue to that repo (pepfar-datim/data-pack-commons#22)
| names(readxl::read_excel( | ||
| d$keychain$submission_path, | ||
| sheet = "Home", | ||
| range = "B25" |
There was a problem hiding this comment.
Is there anyway we could get this out as a hardcode and into a schema/configuration?
|
I unpacked a Lesotho data pack and completed a diff against the original unpacked data pack I also checked datim$PSNUxIM and datim$SUBNAT_IMPATT and found no differences. |
|
@jacksonsj I agree with Jason's comments about pulling out some of the hard coded bits that are tightly coupled to COP19 and placing in a config file. Additionally we have already discussed the need to improve our naming across the board. Lines like this are a pretty good example of our inconsistency.
This should probably be That said I think we can do a general renaming once the main refactoring is done. The changes in this pull request have a very limited impact on the code I wrote and am maintaining. |
| # Check OU name and UID match up | ||
| interactive_print("Checking the OU name and UID on HOME tab...") | ||
| d <- checkSiteToolOUinfo(d) | ||
| countries <- |
There was a problem hiding this comment.
I have a function for metadata calls in datapack commons (https://github.com/pepfar-datim/data-pack-commons/blob/f1bedc5bc1a7c25e2a1993920d0c934813e68242/R/dhis2api.R#L278)
I wanted to make a small change to this function to move base_url to the end with a default of getOption("baseurl")
This would make the equivalent call something like:
datapackcommons::getMetadata(end_point = "organisationUnits",
filters = c("organisationUnitGroups.name:eq:Country",
paste0("children.id:in:[",site_uids,"]")),
fields = "id,name")
We should coordinate how we want to make common api calls like this. I bet Jason has his own version somewhere as well.
| d$info$warningMsg <- NULL | ||
| d$info$has_error <- FALSE | ||
| site_uids <- d$data$targets %>% | ||
| dplyr::pull(site_uid) %>% |
There was a problem hiding this comment.
Any chance this will result in too many sites to make the api call - there is some maximum length to the api call.
| readxl::read_excel( | ||
| path = d$keychain$submission_path, | ||
| sheet = "Home", | ||
| range = "B23") %>% |
There was a problem hiding this comment.
@jacksonsj I think currently ou uid is in B25, has this changed?
|
@jacksonsj for these most recent changes. I was able to unpack Lesotho data pack without issue, and confirmed the distributedMer output, SNUxIM output, and datim$PSNUxIM matched the original unpacking I did for Lesotho. |
|
@jacksonsj is this still a valid pull request/branch? As this makes a lot of changes I don't think we can merge it into master at this moment - I think it represent post cop 19 refactoring. Should we close the pull request without merging? |
|
@jacksonsj Can we close this pull request and delete the branch? |
@jason-p-pickering & @sam-bao - Could you both review this code and send feedback?
The purpose of these changes was to better align code used to process Data Packs and Site Tools, which had been largely duplicative and clunky.
Because of the many downstream implications of some of these changes, I'd like to ask for both of you to review to identify ways these changes may impact your code.
Major changes include:
unPackTool.indicator_code(instead ofindicatorCode)