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

feat: Buckets in the HTTP API (SOFIE-2795) #1070

Conversation

ianshade
Copy link
Contributor

This PR is being opened on behalf of TV 2 Norge.

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    Exposes existing features regarding Buckets over the HTTP API

  • What is the current behavior? (You can also link to an open issue here)
    Buckets and Bucket AdLib management is only available over DDP. Only execution of bucket adlibs is available over HTTP.

  • What is the new behavior (if this is a feature change)?
    The following features are made available over HTTP:

    • Buckets can be fetched, created, deleted and emptied.
    • Bucket AdLibs can be imported (through the blueprint ingest flow) and deleted.
  • Other information:
    It is assumed that validation of the payload when importing an adlib is performed by the blueprints.

Status

  • Code documentation for the relevant parts in the code have been added/updated by the PR author
  • The functionality has been tested by the PR author
  • Automated tests to cover the new functionality and/or guard against regressions have been added
  • The functionality has been tested by NRK

Copy link

codecov bot commented Nov 13, 2023

Codecov Report

Attention: 565 lines in your changes are missing coverage. Please review.

Comparison is base (2a502c3) 58.12% compared to head (68ef09a) 57.80%.
Report is 16 commits behind head on release51.

Files Patch % Lines
meteor/server/api/rest/v1/buckets.ts 0.00% 281 Missing ⚠️
meteor/lib/api/rest/v1/buckets.ts 0.00% 120 Missing ⚠️
meteor/server/api/rest/v1/playlists.ts 0.00% 101 Missing ⚠️
meteor/server/api/rest/v1/typeConversion.ts 0.00% 24 Missing ⚠️
meteor/lib/api/rest/v1/playlists.ts 0.00% 23 Missing ⚠️
meteor/lib/collections/Users.ts 14.28% 6 Missing ⚠️
...eor/client/ui/Prompter/controller/joycon-device.ts 0.00% 4 Missing ⚠️
...client/ui/Prompter/controller/midi-pedal-device.ts 0.00% 3 Missing ⚠️
meteor/server/api/rest/v1/index.ts 0.00% 2 Missing ⚠️
meteor/lib/api/rest/v1/index.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##           release51    #1070      +/-   ##
=============================================
- Coverage      58.12%   57.80%   -0.33%     
=============================================
  Files            507      509       +2     
  Lines          81917    82594     +677     
  Branches        4325     4319       -6     
=============================================
+ Hits           47615    47740     +125     
- Misses         34254    34802     +548     
- Partials          48       52       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@Julusian Julusian left a comment

Choose a reason for hiding this comment

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

I don't see anything here for retrieving the contents of a bucket. Should that be here?

When it comes to exposing the adlibs in the LSG, I would suggest considering grouping the different versions of an adlib up as one document with 'variants'. I am not sure how that will look (or if it will work well), but the way we have multiple documents for each bucket item was a performance problem for us at one point.

packages/openapi/api/definitions/buckets.yaml Show resolved Hide resolved
meteor/lib/api/rest/v1/buckets.ts Outdated Show resolved Hide resolved
@ianshade
Copy link
Contributor Author

@Julusian Those are some very good points about the externalId and variants. Do you agree that when executing a bucket adlib it should also be possible to do so by the externalId? I'd say that it should be the backend's responsibility to find the actual adlib by given externalId, that is also matching the current variant (if set). In that case we could use the externalId as THE id for bucket adlibs both in the LSG and the HTTP API. But that would be a breaking change in regards to playlist's execute-adlib endpoint.

I don't see anything here for retrieving the contents of a bucket. Should that be here?

I'm working under the assumption that since regular adlibs can't be retrieved over HTTP either, the LSG will be used for that purpose, but I'm open to adding it if you think that's necessary.

@Julusian
Copy link
Member

I'm working under the assumption that since regular adlibs can't be retrieved over HTTP either, the LSG will be used for that purpose, but I'm open to adding it if you think that's necessary.

Makes sense, I haven't looked to closely at the other APIs, so I am fine that this follows what they are doing.

Those are some very good points about the externalId and variants. Do you agree that when executing a bucket adlib it should also be possible to do so by the externalId? I'd say that it should be the backend's responsibility to find the actual adlib by given externalId, that is also matching the current variant (if set). In that case we could use the externalId as THE id for bucket adlibs both in the LSG and the HTTP API. But that would be a breaking change in regards to playlist's execute-adlib endpoint.

One thing to note here is that the externalId is only guaranteed to be unique within the bucket. So if using this as the id when executing, you will also need to provide the bucket id (ie, a new endpoint will be needed to execute a bucket adlib from this id). Meaning this wouldn't need to be a breaking change, leave the code in the old endpoint to still work with the document id.
I think it would be reasonable for the backend to pick the correct variant to execute. Your client will still need to be aware of variants to know how to render, but it is unnecessary noise for the api call

I don't know if anyone else will disagree with this proposal? But I do think it makes sense to do to avoid clients needing to know how to group up the different versions of an adlib/adlib-action.

@jesperstarkar jesperstarkar added the Contribution from TV 2 Norge Contributions sponsored by TV 2 Norge (tv2.no) label Nov 23, 2023
@nytamin
Copy link
Contributor

nytamin commented Nov 28, 2023

Hey @ianshade !

Let's schedule a session in a few days time to discuss how to progress this PR.
If anyone else is interested in joining the discussion, let me know.

(If you haven’t already, please give our contribution guidelines a read.)

@jstarpl jstarpl changed the title feat: Buckets in the HTTP API feat: Buckets in the HTTP API (SOFIE-2795) Nov 30, 2023
@nytamin
Copy link
Contributor

nytamin commented Dec 4, 2023

Notes from the discussion

Attendees: @nytamin , @Julusian , @mint-dewit , @ianshade

  • Q: Should the API expose the different variants of the adlibs?
    A: No, we'll consider the variants to be internal to to Core.

  • Q: How should the endpoint for creating bucket adlibs look like?
    A: The current proposal is good. We'll use the ingestItem.externalId as the identifier for the adlib (and it's internal variants)

  • Q: How should the endpoint for deleting bucket adlibs look like?
    A: DELETE /buckets/{bucketId}/adlibs/{externalId}

  • We should also add a new method to execute bucket adlibs:
    incomplete pseudo code: executeBucketAdlid(bucketId, externalId)
    It should reside in the userActions api, since it affects playout.

@ianshade
Copy link
Contributor Author

ianshade commented Dec 7, 2023

Thank you all for your input! I believe I have now addressed all the comments.

@ianshade ianshade force-pushed the contribute/EAV-33/buckets-api-http-release51 branch from 54e9db4 to a3cf788 Compare December 20, 2023 11:18
@ianshade
Copy link
Contributor Author

I have now addressed the remaining comments, resolved merge conflicts, and fixed a bug that I found (incorrect selectors when fetching buckets).

@Julusian Julusian merged commit 5d94d8a into nrkno:release51 Dec 20, 2023
32 of 35 checks passed
@ianshade ianshade mentioned this pull request Dec 22, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Contribution from TV 2 Norge Contributions sponsored by TV 2 Norge (tv2.no)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants