vubiostat / redcapAPI

R interface to REDCap (http://www.project-redcap.org/)
20 stars 26 forks source link

importRecords compatibility with exportRecords #72

Closed spgarbet closed 1 year ago

spgarbet commented 1 year ago

This is just an open question. With exportRecordsTyped near complete, what can be done to make it compatible with importRecords? Is anything required? One issue I can see is the need to drop the mChoice variables.

nutterb commented 1 year ago

Superficially, removing mChoice fields is probably the biggest issue.

The less obvious issue is that we've given users enormous flexibility to cast variables, but importRecords is rather rigid and still tied very strictly to the data dictionary. I don't think that's usually going to be an issue. But if a user casts a checkbox to "selected" and "unselected", importRecords won't know what to do with it. How far down that rabbit hole do you want to go?

Then there's the issue that the code in importRecords is a big pile of ew.

spgarbet commented 1 year ago

The rabbit hole is deep and forbidding. User flexibility for the path back in the long run is a good idea, but ultimately has fewer use cases. The complexity allowed for data export is for doing things like having an export go straight into an ARIMA model for example. It's going to be mostly be used in analysis. However, it is not uncommon for folks to push up things like oddly coded data (e.g. -9=NA). In that case, they are managing their own code book. Those pushing data back up are generally "generators" or "modifiers" and typically will use the data in a closer to raw form in general.

Maybe we just strip mChoice for now and plan on revisiting late Summer. See what tickets folks open in the meantime.

nutterb commented 1 year ago

I think I will recommend relaxing the constraint at https://github.com/vubiostat/redcapAPI/blob/main/R/importRecords.R#L218-L224. Instead of returning a hard error, just send back the unrecognized names with a warning/message that they are being removed from the import. That should catch any mChoice fields as well as other fields that don't belong in the project.

Should be pretty easy to replace that coll$push with a message.

spgarbet commented 1 year ago

That sounds like a great plan.

nutterb commented 1 year ago

We might have to go down this rabbit hole afterall. From (https://github.com/nutterb/redcapAPI/issues/14)

I tried the code below and was able to reproduce the error. The error is actually coming from the API. What appears to be happening is

  1. R sees "1.0" and prints it as "1" when writing the data to go to the API
  2. The API requires "1.0" in order to process it

The only way to get satisfy the API is to write all of the fields to character vectors before sending them.

We might want to open a new issue to focus on this problem and stew on it a bit.

rcon <- redcapConnection(url = url, 
                         token = API_KEY)

NewData <- data.frame(record_id = 1, 
                      import_integer = 1, 
                      import_number = 1.234, 
                      import_number_1dp = 1.0, 
                      import_number_2dp = 1.23)

importRecords(rcon, 
              NewData)
spgarbet commented 1 year ago

@nutterb where did we land on this? Is this a close this one and open something targeted on the new problem?

nutterb commented 1 year ago

I think that is the right idea. The standard output of exportRecordsTyped seems to play nicely with importRecords as it exists.

The edges where importRecords doesn't play nicely are a separate problem. It might be worthwhile to come back and reevaluate much of how that function works (perhaps capturing a validation report, or implementing the casting functions, for instance)

But we're no worse off than we were before

spgarbet commented 1 year ago

Okay. I'll close this for now then and we'll dive in when something pops up.