feat: function to set all oauth integrations associated with a content item

[toph-allen]

Intent

Add a function to set a content item's OAuth associations.

Example usage:

client <- connect()

content <- content_item(client, "12345678-90ab-cdef-1234-567890abcdef")

integrations <- get_integrations(client)

# Associate a single integration
github_integration <- purrr::keep(integrations, \(x) x$template == "github")[[1]]
content_set_all_integrations(content, github_integration)

# Associate multiple integrations at once
selected_integrations <- integrations[1:2]
content_set_all_integrations(content, selected_integrations)

Fixes #414

Approach

• Added a function, content_set_all_integrations(content, integrations), which sets all integration associations for a provided content item.
  • The first parameter, content, is a content item. The second parameter, integrations, can be a single connect_integration class object or a list of such objects.
  • Mirroring what the API does, this doesn't return anything right now. If a future PR adds a class to represent association type objects, maybe this'll return that later, but that'd require another request to the Connect server.
• Adds class to represent an integration, connect_integration.
  • This is pretty simple — it's just the integration data returned by the server.
  • Added functions to construct and validate from a list, added that class to the list of objects returned by the get_integrations() function.
  • Added function integration(client, guid) to the details of a single integration from the server given its guid.
  • Added a nice print method.
• Fully documented. Updated cross-references.
• Added tests for the new code.

Checklist

• Does this change update NEWS.md (referencing the connected issue if necessary)?
• Does this change need documentation? Have you run devtools::document()?

Validation

• I ran this against Dogfood and validated that I was able to set one and multiple associations on a piece of content.

Note

At time of writing (7:30 PM on Friday) CI is failing. There was a problem with some of my S3 method consistency, but maybe also some Docker errors in there, I'm not sure. Attempted one fix, but I'm not going to get into an iteration cycle at this time; I'll fix Monday AM.

[nealrichardson]

Is this necessary? You're only going to get one of these objects from the server, which we control.

[toph-allen]

I think so. Otherwise a user could pass in any list to the as_integration() function and then hit server errors about incorrect GUIDs when they pass it into things. (Granted, we could rely on those errors!) It's also just… like, what I understand to be good form for S3 classes, even if those are classes that will mostly be created internally. I'd rather leave this in, but don't feel like it's necessary.

I could make it more concise by checking the field names against the ptype names we already have defined in connectapi_ptypes, use that as the canonical place where prototypes are defined.

[nealrichardson]

Otherwise a user could pass in any list to the as_integration() function and then hit server errors about incorrect GUIDs when they pass it into things.

Can you show me where that happens? I only see as_integration() being called inside get_integrations() and (get_)integration(), on data that is coming from the server.

[toph-allen]

I'm confused what you're asking about. We export as_integration(), so a user could pass other data into it. But that possibility isn't something that happens in our codebase.

Perhaps the supporting change here is to not export as_integration() — is that what you're implying?

[toph-allen]

I removed the as_integration() export and the validation.

[nealrichardson]

The fact that the validator made sure that you had an id and guid suggested to me that you would only get that from a server API response, and that if you were constructing a payload to create an integration (which I don't see supported yet), you wouldn't include those fields. So I couldn't see a scenario where this validation would be called on anything other than the thing that comes back from the server, which we can assume to be the shape it is because we control the other side of that API.

[toph-allen]

I made the changes, so I'm going to merge bypassing your remaining requested changes. Thanks again for the review!

[mconflitti-pbc]

Looks good to me. Is the plan to include the complementary getter for content's integration's as well?

[toph-allen]

Looks good to me. Is the plan to include the complementary getter for content's integration's as well?

Yep! I was going to do that in this PR, but I held off for the next one, because it also requires thinking about how I want to handle Associations (i.e. return them as a class, or just return the set of integrations — I'm not sure if it makes sense for them to be a user-facing class in this package).

[mconflitti-pbc]

Looks good to me. Is the plan to include the complementary getter for content's integration's as well?

Yep! I was going to do that in this PR, but I held off for the next one, because it also requires thinking about how I want to handle Associations (i.e. return them as a class, or just return the set of integrations — I'm not sure if it makes sense for them to be a user-facing class in this package).

They are technically their own record in the db which is what the python sdk returns. it contains a subset of info from the integrations

[toph-allen]

They are technically their own record in the db which is what the python sdk returns. it contains a subset of info from the integrations

Yeah, I saw that they were their own object in the API and that the Python API returned info on them. The date associated is probably useful to be able to access, but I also as a data scientist could imagine it feeling weird that I need to think about what is essentially the join table between the two object types I care about.

My thought was to have something like a content_get_integrations() method for content items that gets the associations and then looks up the relevant integrations, and then maybe a content_get_oauth_associations() to get the association metadata. 🤔 I def don't want the get_integrations() function to return associations — I feel like that would be counterintuitive behavior.

[toph-allen]

GitHub says this merge bypasses the rules; just noting that I have another approval and made the changes that were requested in that review.