feat: add activate-adlib-testing API endpoint#1636
feat: add activate-adlib-testing API endpoint#1636imaretic wants to merge 1 commit intoSofie-Automation:mainfrom
Conversation
WalkthroughA new API endpoint is added to activate AdLib testing mode for a playlist/rundown combination, including server implementation, REST API interface, OpenAPI specification documentation, and test coverage. The implementation validates inputs and dispatches a StudioJobs action. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@meteor/server/api/rest/v1/playlists.ts`:
- Line 623: The log message emitted by logger.info when activating AdLib testing
mode contains a stray closing brace in the template literal; fix the string in
the logger.info call so it reads `API PUT: activate AdLib testing mode, playlist
${rundownPlaylistId}, rundown ${rundownId}` (remove the extra `}`) in the
function where logger.info is called, ensuring the template variables
rundownPlaylistId and rundownId remain unchanged.
- Around line 625-626: The handler is only validating rundownPlaylistId but not
rundownId; add an explicit validation check(rundownId, String) before calling
serverAPI.activateAdLibTesting to mirror other routes and the implementation
(which also checks rundownId at line 94). Locate the route handler that calls
serverAPI.activateAdLibTesting(connection, event, rundownPlaylistId, rundownId)
and insert the check for rundownId immediately after the existing
check(rundownPlaylistId, String) so invalid inputs are rejected early and
consistent with the pattern used elsewhere.
In `@packages/openapi/src/__tests__/playlists.spec.ts`:
- Around line 65-71: The test uses a hardcoded rundownId and should be gated by
the test harness; wrap the test that calls playlistsApi.activateAdlibTesting
(which uses playlistIds[0] and the rundownId path param) inside an if
(testServer) block so it only runs when test-specific data exists, and add an
else branch with test.todo() to document the dependency in non-test
environments; update the surrounding test scope accordingly to mirror the
pattern used by setNextSegment / executeAdLib.
🧹 Nitpick comments (3)
packages/openapi/api/definitions/playlists.yaml (1)
92-117: Consider documenting additional error responses.The endpoint currently documents 404 for
playlistNotFound, but the rundownId parameter validation could also fail. Consider:
- The error mapping in the route handler (Lines 615-618 in
meteor/server/api/rest/v1/playlists.ts) includesUserErrorMessage.RundownAlreadyActivefor 412, but this isn't documented here.- If an invalid
rundownIdis provided (e.g., doesn't belong to the playlist), that error case isn't documented.Aligning the OpenAPI spec with the actual error responses improves API discoverability for consumers.
meteor/server/lib/rest/v1/playlists.ts (1)
48-56: JSDoc could be more complete for consistency.The JSDoc for
activateAdLibTestingis minimal compared to other methods in the interface. Consider adding parameter descriptions and expected error conditions for consistency:📝 Suggested JSDoc enhancement
- /** - * Activates AdLibs testing mode. - */ + /** + * Activates AdLibs testing mode. + * + * Throws if there is already an active Playlist for the studio that the Playlist belongs to. + * `@param` connection Connection data including client and header details + * `@param` event User event string + * `@param` rundownPlaylistId Playlist to activate testing mode for. + * `@param` rundownId Rundown to activate testing mode for. + */ activateAdLibTesting(meteor/server/api/rest/v1/playlists.ts (1)
612-612: Body type declaration is unused.The route declares
{ rehearsal: boolean }as the body type, but the body parameter is ignored (_body). Since this endpoint doesn't require a request body (as per the OpenAPI spec which has norequestBody), consider usingneverfor the body type to match similar endpoints likedeactivate.♻️ Suggested fix
- registerRoute<{ playlistId: string; rundownId: string }, { rehearsal: boolean }, void>( + registerRoute<{ playlistId: string; rundownId: string }, never, void>(
| async (serverAPI, connection, event, params, _body) => { | ||
| const rundownPlaylistId = protectString<RundownPlaylistId>(params.playlistId) | ||
| const rundownId = protectString<RundownId>(params.rundownId) | ||
| logger.info(`API PUT: activate AdLib testing mode, playlist ${rundownPlaylistId}, rundown ${rundownId}}`) |
There was a problem hiding this comment.
Typo in log message: extra closing brace.
Line 623 has an extra } at the end of the log message.
🐛 Fix typo
- logger.info(`API PUT: activate AdLib testing mode, playlist ${rundownPlaylistId}, rundown ${rundownId}}`)
+ logger.info(`API PUT: activate AdLib testing mode, playlist ${rundownPlaylistId}, rundown ${rundownId}`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger.info(`API PUT: activate AdLib testing mode, playlist ${rundownPlaylistId}, rundown ${rundownId}}`) | |
| logger.info(`API PUT: activate AdLib testing mode, playlist ${rundownPlaylistId}, rundown ${rundownId}`) |
🤖 Prompt for AI Agents
In `@meteor/server/api/rest/v1/playlists.ts` at line 623, The log message emitted
by logger.info when activating AdLib testing mode contains a stray closing brace
in the template literal; fix the string in the logger.info call so it reads `API
PUT: activate AdLib testing mode, playlist ${rundownPlaylistId}, rundown
${rundownId}` (remove the extra `}`) in the function where logger.info is
called, ensuring the template variables rundownPlaylistId and rundownId remain
unchanged.
| check(rundownPlaylistId, String) | ||
| return await serverAPI.activateAdLibTesting(connection, event, rundownPlaylistId, rundownId) |
There was a problem hiding this comment.
Missing validation for rundownId parameter.
The route handler only validates rundownPlaylistId but not rundownId. Other route handlers validate all path parameters. Additionally, the implementation method at line 94 performs check(rundownId, String), so adding it here would be consistent with the pattern and provide earlier validation.
🛡️ Add rundownId validation
check(rundownPlaylistId, String)
+ check(rundownId, String)
return await serverAPI.activateAdLibTesting(connection, event, rundownPlaylistId, rundownId)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| check(rundownPlaylistId, String) | |
| return await serverAPI.activateAdLibTesting(connection, event, rundownPlaylistId, rundownId) | |
| check(rundownPlaylistId, String) | |
| check(rundownId, String) | |
| return await serverAPI.activateAdLibTesting(connection, event, rundownPlaylistId, rundownId) |
🤖 Prompt for AI Agents
In `@meteor/server/api/rest/v1/playlists.ts` around lines 625 - 626, The handler
is only validating rundownPlaylistId but not rundownId; add an explicit
validation check(rundownId, String) before calling
serverAPI.activateAdLibTesting to mirror other routes and the implementation
(which also checks rundownId at line 94). Locate the route handler that calls
serverAPI.activateAdLibTesting(connection, event, rundownPlaylistId, rundownId)
and insert the check for rundownId immediately after the existing
check(rundownPlaylistId, String) so invalid inputs are rejected early and
consistent with the pattern used elsewhere.
| test('can activate adlib testing mode', async () => { | ||
| const active = await playlistsApi.activateAdlibTesting({ | ||
| playlistId: playlistIds[0], | ||
| rundownId: 'rundownId', | ||
| }) | ||
| expect(active.status).toBe(200) | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/openapi/src/__tests__/playlists.spec.ts | head -150Repository: Sofie-Automation/sofie-core
Length of output: 5769
🏁 Script executed:
# Search for how rundownId is retrieved or used in similar tests
rg -n "rundownId" packages/openapi/src/__tests__/ -B 3 -A 3Repository: Sofie-Automation/sofie-core
Length of output: 649
🏁 Script executed:
# Search for rundownId patterns in the broader codebase to understand how it's typically obtained
rg -n "rundownId" packages/openapi/src/ --type typescript --type typescriptreact -B 2 -A 2 | head -100Repository: Sofie-Automation/sofie-core
Length of output: 105
🏁 Script executed:
# Search for rundownId patterns in the codebase more broadly
rg -n "rundownId" packages/openapi/src/ -B 2 -A 2 | head -150Repository: Sofie-Automation/sofie-core
Length of output: 486
🏁 Script executed:
# Check the API definition to understand what activateAdlibTesting expects
rg -n "activateAdlibTesting" packages/openapi/src/ -B 5 -A 5Repository: Sofie-Automation/sofie-core
Length of output: 901
🏁 Script executed:
# Find and examine the PlaylistsApi client implementation
find packages/openapi -name "*.ts" -type f | grep -v node_modules | grep -v __tests__ | head -20Repository: Sofie-Automation/sofie-core
Length of output: 168
🏁 Script executed:
# The test file imports from '../../client/ts/index.js', let's find that directory structure
find packages/openapi -type d -name "client" -o -type d -name "ts"Repository: Sofie-Automation/sofie-core
Length of output: 53
🏁 Script executed:
# Let's check if there's a generated or built client directory
find packages/openapi -type f -name "*api*" | grep -i playlist | head -20Repository: Sofie-Automation/sofie-core
Length of output: 53
🏁 Script executed:
# List the full structure of packages/openapi
find packages/openapi -type f -name "*.ts" -o -type f -name "*.js" | head -50Repository: Sofie-Automation/sofie-core
Length of output: 667
🏁 Script executed:
# Check if there's a client directory at the root or in packages
find . -type d -name "client" | grep -v node_modules | head -10Repository: Sofie-Automation/sofie-core
Length of output: 98
🏁 Script executed:
# Check if client directory exists or is in .gitignore
ls -la packages/openapi/ | grep -E "client|^d"
cat packages/openapi/.gitignore 2>/dev/null || echo "No .gitignore found"Repository: Sofie-Automation/sofie-core
Length of output: 460
🏁 Script executed:
# The client directory is gitignored (likely generated). Let's check jest.setup.js and test configuration
cat packages/openapi/jest.setup.jsRepository: Sofie-Automation/sofie-core
Length of output: 808
🏁 Script executed:
# Let's check the api/actions.yaml to understand the API structure
cat packages/openapi/api/actions.yaml | grep -A 20 "activateAdlibTesting" | head -40Repository: Sofie-Automation/sofie-core
Length of output: 1231
🏁 Script executed:
# Check the playlists.yaml definition
cat packages/openapi/api/definitions/playlists.yaml | grep -A 30 "activateAdlibTesting"Repository: Sofie-Automation/sofie-core
Length of output: 1084
🏁 Script executed:
# Check if the playlists response includes rundown data that could be extracted
rg -n "rundown" packages/openapi/src/__tests__/playlists.spec.ts -iRepository: Sofie-Automation/sofie-core
Length of output: 100
🏁 Script executed:
# Let's look at the full playlists response to see if rundownId is included
cat packages/openapi/api/definitions/playlists.yaml | grep -A 100 "Playlist:" | head -150Repository: Sofie-Automation/sofie-core
Length of output: 3793
Test uses hardcoded rundownId that may not exist.
The test passes a hardcoded string 'rundownId' which is a required path parameter but unlikely to correspond to an actual rundown. This test should follow the established pattern in this file and be wrapped with if (testServer) similar to other tests that depend on specific test data (e.g., lines 107-118 for setNextSegment, lines 125-135 for executeAdLib).
Wrap this test in an if (testServer) condition, or add a complementary test.todo() block for non-test environments to clarify the dependency on test-specific data.
🤖 Prompt for AI Agents
In `@packages/openapi/src/__tests__/playlists.spec.ts` around lines 65 - 71, The
test uses a hardcoded rundownId and should be gated by the test harness; wrap
the test that calls playlistsApi.activateAdlibTesting (which uses playlistIds[0]
and the rundownId path param) inside an if (testServer) block so it only runs
when test-specific data exists, and add an else branch with test.todo() to
document the dependency in non-test environments; update the surrounding test
scope accordingly to mirror the pattern used by setNextSegment / executeAdLib.
1 participant
About the Contributor
This PR is posted on behalf of EVS Broadcast Equipment.
Type of Contribution
This is a: Feature
Current Behavior
There is no API endpoint to activate adlib testing mode.
New Behavior
A new endpoint for user actions has been added. Adlib testing mode can now be activated by sending a POST request to
/playlists/{playlistId}/rundowns/{rundownId}/activate-adlib-testing.Testing
Affected areas
User actions API.
Status
Summary
This PR introduces a new API endpoint to activate ad-lib testing mode for a rundown within a playlist.
Changes Overview
API Implementation (
meteor/server/api/rest/v1/playlists.ts):activateAdLibTestingmethod toPlaylistsServerAPIthat validates therundownPlaylistIdandrundownIdparameters and dispatchesStudioJobs.ActivateAdlibTestingwith the provided payload/playlists/:playlistId/rundowns/:rundownId/activate-adlib-testingto handle requestsAPI Interface (
meteor/server/lib/rest/v1/playlists.ts):activateAdLibTestingreturningPromise<ClientAPI.ClientResponse<void>>OpenAPI Specification:
packages/openapi/api/actions.yamlpackages/openapi/api/definitions/playlists.yamlwith:activateAdlibTestingplaylistIdandrundownId(both strings)Testing (
packages/openapi/src/__tests__/playlists.spec.ts):Impact
The new endpoint enables users to activate ad-lib testing mode for a specific rundown within a playlist via a PUT request, extending the User actions API surface.