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

🐛 Fix coltypes for projects with dags #472

Merged
merged 9 commits into from
Apr 18, 2023

Conversation

pwildenhain
Copy link
Contributor

Fixes #467

Copy link
Member

@wibeasley wibeasley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pwildenhain, this looks great. I have only one comment involving code.

In addition, can you please add an entry in https://github.com/OuhscBbmc/REDCapR/blob/main/NEWS.md#minor-enhancements? Maybe along the lines of "redcap_meta_coltypes() correctly determines data type for autonumber fields. It suggests a character if the project has DAGs, and an integer if not. (@pwildenhain , #472)"

R/redcap-metadata-coltypes.R Outdated Show resolved Hide resolved
@wibeasley wibeasley self-assigned this Feb 27, 2023
@wibeasley
Copy link
Member

@pwildenhain, I see you just added me as a reviewer. I think you've added everything except for the line that explicitly specifies "character" for autonumbering w/ dags. Am I overlooking it?

As the waterfall is written now, it shouldn't affect the recommended type. But two benefits are

  1. an explicit explanatory comment for the humans to read and
  2. more robustness (like I mess up the waterfall later, or something goes weird like a user is able to add a date validation on the field before it became an autonumber)

@pwildenhain
Copy link
Contributor Author

Oh I see what you're saying. I took a different approach. I decided to say if there was autonumbering and there were not dags, then the field could still be considered an integer. Otherwise, I saw that it would fall to the catchall "character" case at the end of the waterfall:

TRUE ~ paste0("col_character()" , "~~validation doesn't have an associated col_type. Tell us in a new REDCapR issue. "),

But it sounds like you're saying it would be nice to add a case to the waterfall for the situation where you have autonumbering and dags, so we could explain to the user why we assigned a character?

Just want to make sure I understand before I go add that second case

@wibeasley
Copy link
Member

yeah, that's a good way to say it.

Ideally the col_types entry has an informative comment/explanation.

If it helps to visualize, this is an example current output:

# A simple project
token      <- "9A81268476645C4E5F03428B8AC3AA7B" # 153
col_types  <- redcap_metadata_coltypes(uri, token)
#> # col_types <- readr::cols_only( # Use `readr::cols_only()` to restrict the retrieval to only these columns
#> col_types <- readr::cols( # Use `readr::cols()` to include unspecified columns
#>   # [field]                       [readr col_type]            [explanation for col_type]
#>   record_id                     = readr::col_integer()    , # record_autonumbering is enabled for the project
#>   name_first                    = readr::col_character()  , # field_type is text and validation isn't set
#>   name_last                     = readr::col_character()  , # field_type is text and validation isn't set
#>   address                       = readr::col_character()  , # field_type is note
#>   telephone                     = readr::col_character()  , # validation is 'phone'
#>   email                         = readr::col_character()  , # validation is 'email'
#>   dob                           = readr::col_date()       , # validation is 'date_ymd'
#>   age                           = readr::col_character()  , # field_type is text and validation isn't set
#>   sex                           = readr::col_character()  , # field_type is radio
...

@pwildenhain
Copy link
Contributor Author

Hold off on merging this for now -- I'm just now realizing that this function will fail if the user is currently assigned to a DAG -- since you can't access the "Export DAGs" API endpoint when you assigned to a DAG. I think we can maneuver around this though.

@pwildenhain
Copy link
Contributor Author

Ok @wibeasley I've added some code to account for that edge case, and I've tested it with one of my projects to make sure it works. PR is ready for review again.

@pwildenhain
Copy link
Contributor Author

Pinging @wibeasley again to review -- not sure why the tests failed

@wibeasley wibeasley changed the base branch from main to dev April 18, 2023 14:09
@wibeasley
Copy link
Member

I meant to check it out more deeply. I thought it was something fluky and that's why I hit some limit of four rerunning. I've changed the base to dev and I'll try some things now.

@wibeasley wibeasley merged commit f6d9d00 into OuhscBbmc:dev Apr 18, 2023
@@ -380,7 +380,8 @@ redcap_metadata_internal <- function(
dplyr::mutate(
response =
dplyr::case_when(
autonumber & !dags ~ paste0("col_integer()" , "~~record_autonumbering is enabled for a project without DAGs"),
dags ~ paste0("col_character()" , "~~DAGs are enabled for the project"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for so much blabbing over one line. Can you check my logic? Wouldn't it be better to change dags to autonumber & dags? Otherwise, I think it would execute for all variables in any project with dags.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries haha. dags will only evaluate to TRUE when the field is the .record_id field AND the project has DAGs (similar to how autonumber already works

https://github.com/pwildenhain/REDCapR/blob/ef2171960807ede2136172e59b866953b7928666/R/redcap-metadata-coltypes.R#L375-L379

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected Behavior: redcap_metadata_coltypes assumes that "record_id" field is an integer
2 participants