feat: get all integrations associated with a piece of content

[toph-allen]

Intent

Provide the ability to get a list of integrations associated with a particular piece of content.

Fixes #432

Approach

• In the last PR, feat: function to set all oauth integrations associated with a content item #431, I added content_set_integrations(). This PR renames that to set_integrations().
• You can now pass a Content class object to get_integrations() to get a list of integrations associated with that piece of content.
  • You can also still pass the Connect client object to get a list of all integrations on the server. Instead of using method dispatch, I just used conditional flow within the function itself. If this feels like a solid approach, I can adopt method dispatch in a future PR.
  • Docs are updated to indicate that the function can take a client or piece of content.
  • Although the API itself returns association objects for content, we make an extra few requests to get the actual integration objects. This feels more natural for R users.

Notes

• I considered tackling Add function to get content associated with an integration #433 in this one (get content by integration). However, I'm gonna do it in its own PR because it feels like there's stuff to figure out in it. There is already a get_content() function which gets a data frame of content from the server, so this is where newer-style functionality is hitting up against old connectapi namespace.

Checklist

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

[nealrichardson]

Can you help me understand this API? We have to make a GET /content/:guid/oauth/integrations/associations/, which must return a list of items each containing "oauth_integration_guid", and then for each of those, we have to do another GET to get the details for the integration? Does that make sense?

Having to make N + 1 requests here isn't awesome, but N should be a small enough number that it's not a huge deal in practice. But I wonder if we have the right APIs if this is what we have to do, or even if maybe we already have the right API and we just aren't calling it. Like, what does it mean that associations/ is nested under /content/:guid/oauth/integrations/--what does GET /content/:guid/oauth/integrations/ return?

[mconflitti-pbc]

I think this function may be trying to do too many things. get_integrations(connectClient) implies it is returning a list of records from the integrations table which is not related to content.

get_associations(client, contentGuid) makes more sense and aligns better with the python sdk. And that only returns the entries from the associations table without needing to make another call to get each integration's details. I would leave that up to the user if they need that information by using some sort of a get_integration(client, guid) function (this could be the start of the R-equivalent find_by function).

[toph-allen]

@nealrichardson You're right about the API — it returns a list of association objects which contain info on the integration, the content GUID, and the date created. I don't think the number of integrations returned will ever be much of a problem for the number of requests involved in this function.

@nealrichardson @mconflitti-pbc I don't think that an R user cares about the join table (a.k.a. associations) — their mental model would be, they associate an integration with content, and when getting info about the integrations associated with content, they'd expect to… see a list of the integrations. If we return a different object type representing an entry from a join table, it feels like we're exposing an implementation detail to users, and one they just aren't gonna care about. I also had a get_associations(content) function here, but after talking it over with @jonkeane I felt comfortable removing it from the implementation. (If a user is really interested in the history of adding and removing integrations to and from content, they can get more detailed info on that from the audit logs, right?)

@mconflitti-pbc Function-oriented method dispatch (i.e. foo(x) behaving differently depending on the type of x) is a common R pattern — it can feel a lot like overloading, but I think that since the action here is "get a list of integrations from this object" it actually feels pretty ergonomic and R-idiomatic to me — so I'm gonna push back here and say that I'd like to keep this approach. I do think it might be better to actually implement it using method dispatch than with an if/else.

[toph-allen]

After some discussion in the Enterprise sync, I'll add the get_associations() function back to this PR. (I'd also kinda like to implement get_integrations() using method dispatch rather than if/else. It feels a little cleaner to read to me.)

[jonkeane]

Is the plan to expose the implementation detail of associations to the user (or more specifically: force users to use that in order to get what they want)? That doesn't sound right to me. Neal's questions here are the right ones to be asking: if our API and sticking to being a thin wrapper around that is making this feel awkward, that's a sign that the API needs to be changed.

[toph-allen]

I don't think that users will care about the associations object personally — I think they'll want to get integrations for content, and get content for integrations.

But talking it over some in our sync meeting, I don't know that everyone else is fully bought into my take on that, and we aren't ready to make those kinds of model changes in e.g. the Python SDK yet, and folks made the argument for the name consistency between this and e.g. the Python SDK + our API. (@mconflitti-pbc please correct me if I'm misrepresenting!)

With connectapi we aren't necessarily striving for consistency with those other interfaces above all else, but I'm fine having get_assocations() included for completeness, so long as the more direct (from the user's perspective) get_integrations() function also exists. And I don't think we should wait on figuring out the direction for the API & Python SDK to move forward on this PR -- it would be helpful to merge this as-is (assuming that the code itself looks up to snuff) and continue this train of thought elsewhere.

(Aside, get_associations() would be good for some other functionality I'm planning to add after this merges: get content from integrations. get_content() already exists in a form that isn't as compatible with the new approach here e.g. with argument names etc., but get_associations() can easily be updated get the same association object type from an integration object).

[mconflitti-pbc]

set_integrations seems to ambiguous here. If we ever wanted to create helpers for hitting the integration endpoints that create integrations i think that this naming becomes confusing. I dont know R but is there is a better way to namespace things? In this instance, this should all be under the context of setting associations for content.. content.set_integrations() maybe? idk

[toph-allen]

I'm actually working on a PR right now for those helpers — create_integration() etc. — hoping that the names, function signatures, and documentation are clear enough to differentiate.

This function was originally called content_set_integrations() in #431 but I renamed it in this PR (in addition to moving it) after talking through some function naming things with @jonkeane. The really long function name feels kind of clunky, and doesn't feel very "R idiomatic". A shorter name, with argument names here and documentation that make its behavior clear, should hopefully be enough, and feel like they match with user expectations.

[toph-allen]

(I feel like this discussion is part of the same discussion with your comment above on get_integrations(), so my response is kinda split across those two threads).

[toph-allen]

connectapi CI is really flaky, or perhaps broken. Connect containers in the integration tests flow fail to start pretty often.

I attempted to fix just now by adding a cycle to wait until the ping endpoint could be reached, but that didn't seem to work. I wonder if there's been a change to the containers that would break CI.

I opened #438 yesterday to track.

[mconflitti-pbc]

LGTM but may need to defer to Josh