-
Notifications
You must be signed in to change notification settings - Fork 26
feat: get content that uses a specified integration #441
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
Conversation
| x$guid, | ||
| "associations" | ||
| )) | ||
| purrr::map( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be a potentially large list, so I'm not sure how wise this.
I wonder if a better way to expose this would be through the content search, adding an integrations:guid keyword. There would be other benefits there too, but if nothing else, that would give you a list of content items already, so you wouldn't have to iterate over guids.
Alternatively, we could expose a different endpoint off of oauth/integrations/guid/ that would return a list of content records. But IMO running this through search would be better than adding a new endpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I'll give that a go.
| - New functions allow you to manage the OAuth integrations on your Connect | ||
| server: `create_integration()`, `update_integration()` and | ||
| `delete_integration()`. (#434) | ||
| - New `get_content_list()` function retrieves a list of content items associated with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing something specific to an oauth integration is not what I would expect a function called get_content_list() to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i would agree here. i think naming could be an issue. I almost always prefer explicit naming conventions like: get_associated_content_for_integration() or something like that. get_content_list is too similar to what I would expected to get a list of content objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this point of view — I would definitely be open to a more verbose name. If I did change it to something like get_content_for_integration(), then I should also change the get_integration.Content() method added here to something more descriptive.
For context: The way #437 PR works, get_integrations() returns a list of integration objects but uses a different method based on what it's called with: get_integrations.Connect() gets all integration on the server, and get_integrations.Content() gets all integrations used by the piece of content you provide. Following that naming scheme exactly, get_content.integration() (so called with get_content(integration)) would be how this is implemented, but there's an existing get_content() function that returns a data frame, and we want to keep return types consistent within a given function. The idea, though, is that (in a future PR) you'd call get_content_list(client) to get a list of Content objects — kinda like how get_job_list() works. So the idea is that it is a more general name, but uses method dispatch to get you the list of content objects that are determined by the object you pass in.
Yesterday @jonkeane and I talked about a few different approaches, and this is the one I decided to move forward with. But having a more descriptive name and not using method dispatch (I'd probably go with get_content_for_integration() and the converse get_integrations_for_content()) is another option we looked at, and I'd be happy to go with that if folks think it's a better option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if you implement the search keyword for this like we discussed in the other comment, maybe you don't even need a function for it, it's just search_content(client, integration=) or however you call search. So maybe we don't need to worry what to name this.
|
Coming back to this after PTO — I'm either going to rename the functions slightly and keep the somewhat janky "call the content endpoint multiple times", or punt on this until implementing it under the search endpoint and implementing a |
|
After discussing with @jonkeane this morning, I'm closing this pull request. This isn't a feature that people are clamoring for, and it would be better served by the search endpoint. I'm going to create follow-up issues in both |
Intent
Get a list of content that is associated with a specific integration.
Fixes #433
Approach
get_content_list(). It takes a single parameter, an integration, and returns a list of content objects.x, leaving it open to accepting e.g. a Connect client object in a future PR.Checklist
NEWS.md(referencing the connected issue if necessary)?devtools::document()?