Conversation
📝 WalkthroughWalkthroughThis PR introduces program media resource management by adding a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tests/e2e/program/program/media/conftest.py (1)
15-27: Optional: makemedia_data_factoryaccept arbitrary overrides.Allowing
**overridesimproves test ergonomics when new media fields are added, without duplicating fixture logic.Possible refactor
`@pytest.fixture` def media_data_factory(): - def factory(media_type: str = "Image"): - return { + def factory(media_type: str = "Image", **overrides): + payload = { "name": "E2E Created Program Media", "description": "E2E Created Program Media", "displayOrder": 1, "type": media_type, "mediatype": media_type, "url": "", "language": "en-us", } + payload.update(overrides) + return payload return factory🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/program/program/media/conftest.py` around lines 15 - 27, The fixture media_data_factory should accept arbitrary overrides so tests can customize fields without changing the factory; update the inner factory function (factory) signature to accept **overrides and merge those overrides into the returned dict (e.g., create the base payload as currently done and then apply overrides to it) so callers can pass any field to override defaults while preserving existing behavior when no overrides are provided.tests/e2e/conftest.py (1)
100-102: Consider makingvideo_urlconfigurable for environment stability.A small improvement is to allow an env override while keeping this default, so E2E environments can swap video hosts/URLs without code changes.
Proposed tweak
`@pytest.fixture`(scope="session") def video_url(): - return "https://www.youtube.com/watch?v=DUMMY1_0000" + return os.getenv( + "MPT_E2E_VIDEO_URL", + "https://www.youtube.com/watch?v=DUMMY1_0000", + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/conftest.py` around lines 100 - 102, The video_url fixture currently returns a hardcoded URL; make it configurable via an environment variable by having the video_url fixture read an env var (e.g., E2E_VIDEO_URL) with the current value "https://www.youtube.com/watch?v=DUMMY1_0000" as the fallback default and ensure imports include os in tests/e2e/conftest.py so CI/E2E runs can override the URL without changing code.tests/unit/resources/program/mixin/test_media_mixin.py (1)
60-93: Consider adding coverage foraccept=Nonedownload flow.A follow-up test for the implicit content-type branch (and missing-content-type error branch) would harden behavior inherited from
DownloadFileMixin/AsyncDownloadFileMixin.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/resources/program/mixin/test_media_mixin.py` around lines 60 - 93, Add tests covering the implicit accept (accept=None) branch and the missing-content-type error branch for both sync and async flows: create new tests (e.g., test_media_download_accept_none and test_media_download_missing_content_type, plus async equivalents) that call media_service.download("MED-123", accept=None) and await async_media_service.download(..., accept=None) and assert normal behavior when the response includes a Content-Type header, and create mocks returning responses WITHOUT a Content-Type header to assert the DownloadFileMixin/AsyncDownloadFileMixin raises the expected error; use the same respx/httpx pattern and assert mock_route.call_count, request.method, and either result.file_contents or that the appropriate exception is raised.tests/unit/resources/program/test_programs_media.py (1)
86-97: Optional-field absence test misses three declared optional fields
Mediaalso declaresdisplay_order,url, andprogramas optional. Adding them here would make the regression guard complete.Suggested enhancement
def test_media_optional_fields_absent(): result = Media({"id": "PMD-001"}) assert result.id == "PMD-001" assert not hasattr(result, "name") assert not hasattr(result, "type") assert not hasattr(result, "description") assert not hasattr(result, "status") assert not hasattr(result, "filename") assert not hasattr(result, "size") assert not hasattr(result, "content_type") + assert not hasattr(result, "display_order") + assert not hasattr(result, "url") + assert not hasattr(result, "program") assert not hasattr(result, "audit")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/resources/program/test_programs_media.py` around lines 86 - 97, The test_media_optional_fields_absent in tests/unit/resources/program/test_programs_media.py is missing checks for three optional fields declared on Media; update the test (function test_media_optional_fields_absent) to also assert that the Media instance does not have attributes "display_order", "url", and "program" (e.g., add not hasattr(result, "display_order"), not hasattr(result, "url"), and not hasattr(result, "program")) so the test covers all declared optional fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/program/program/media/test_async_media.py`:
- Around line 26-33: The fixture created_media_from_url is supposed to test
URL-only creation but currently passes file=logo_fd to
async_vendor_media_service.create, causing it to exercise upload+URL logic;
update the fixture so created_media_from_url (and any callers) call
async_vendor_media_service.create with only the URL payload (remove the
file=logo_fd argument) or create a separate fixture name like
created_media_with_file for tests that need multipart upload, ensuring you only
pass media_data containing the "url" field in the URL-only path.
In `@tests/e2e/program/program/media/test_sync_media.py`:
- Around line 26-31: The fixture created_media_from_url is meant to test
URL-only media creation but currently passes file=logo_fd to
vendor_media_service.create which performs a multipart upload; change the call
in created_media_from_url to invoke vendor_media_service.create with only the
media_data (i.e., remove the file=logo_fd argument) so the service receives just
the URL payload and the test exercises URL-based creation; if other tests rely
on this fixture, create a separate fixture (e.g., created_media_with_file) that
supplies file=logo_fd to vendor_media_service.create instead.
---
Nitpick comments:
In `@tests/e2e/conftest.py`:
- Around line 100-102: The video_url fixture currently returns a hardcoded URL;
make it configurable via an environment variable by having the video_url fixture
read an env var (e.g., E2E_VIDEO_URL) with the current value
"https://www.youtube.com/watch?v=DUMMY1_0000" as the fallback default and ensure
imports include os in tests/e2e/conftest.py so CI/E2E runs can override the URL
without changing code.
In `@tests/e2e/program/program/media/conftest.py`:
- Around line 15-27: The fixture media_data_factory should accept arbitrary
overrides so tests can customize fields without changing the factory; update the
inner factory function (factory) signature to accept **overrides and merge those
overrides into the returned dict (e.g., create the base payload as currently
done and then apply overrides to it) so callers can pass any field to override
defaults while preserving existing behavior when no overrides are provided.
In `@tests/unit/resources/program/mixin/test_media_mixin.py`:
- Around line 60-93: Add tests covering the implicit accept (accept=None) branch
and the missing-content-type error branch for both sync and async flows: create
new tests (e.g., test_media_download_accept_none and
test_media_download_missing_content_type, plus async equivalents) that call
media_service.download("MED-123", accept=None) and await
async_media_service.download(..., accept=None) and assert normal behavior when
the response includes a Content-Type header, and create mocks returning
responses WITHOUT a Content-Type header to assert the
DownloadFileMixin/AsyncDownloadFileMixin raises the expected error; use the same
respx/httpx pattern and assert mock_route.call_count, request.method, and either
result.file_contents or that the appropriate exception is raised.
In `@tests/unit/resources/program/test_programs_media.py`:
- Around line 86-97: The test_media_optional_fields_absent in
tests/unit/resources/program/test_programs_media.py is missing checks for three
optional fields declared on Media; update the test (function
test_media_optional_fields_absent) to also assert that the Media instance does
not have attributes "display_order", "url", and "program" (e.g., add not
hasattr(result, "display_order"), not hasattr(result, "url"), and not
hasattr(result, "program")) so the test covers all declared optional fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 4c2ce7c6-d347-4b82-870f-9e196f554685
📒 Files selected for processing (12)
e2e_config.test.jsonmpt_api_client/resources/program/mixins/__init__.pympt_api_client/resources/program/mixins/media_mixin.pympt_api_client/resources/program/programs.pympt_api_client/resources/program/programs_media.pytests/e2e/conftest.pytests/e2e/program/program/media/conftest.pytests/e2e/program/program/media/test_async_media.pytests/e2e/program/program/media/test_sync_media.pytests/unit/resources/program/mixin/test_media_mixin.pytests/unit/resources/program/test_programs.pytests/unit/resources/program/test_programs_media.py
robcsegal
deleted the
MPT-20325-add-endpoints-and-e-2-e-tests-for-program-media
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request adds full support for program media resources to the API client, including new mixins, service classes, and comprehensive tests for both sync and async usage. It introduces the ability to manage media files (such as images and videos) associated with programs, with all CRUD, publish/unpublish, and file upload/download operations covered. The changes also include new and updated tests to ensure the correctness of the implementation.
Program Media Resource Implementation
Mediamodel andMediaService/AsyncMediaServiceclasses inprograms_media.pyto support CRUD operations, file upload/download, and publish/unpublish for program media resources. The service includes all necessary configuration and integrates with the rest of the API client.MediaMixinandAsyncMediaMixininmedia_mixin.pyto encapsulate media-specific logic, including file and publishable operations, and updatedmixins/__init__.pyto export these mixins. [1] [2]Integration with Program Services
ProgramServiceandAsyncProgramServiceinprograms.pyto provide a.media(program_id)method, returning the appropriate media service for a given program. [1] [2] [3]Testing and Fixtures
test_programs_media.py,test_media_mixin.py, and updatedtest_programs.pyto check new service accessors. [1] [2] [3] [4] [5]These changes ensure that program media resources are fully supported and well-tested in the API client.
Closes MPT-20325
Mediaresource model with fields for name, type, description, status, filename, size, content type, display order, URL, program reference, and audit referenceMediaServiceandAsyncMediaServiceclasses for program media CRUD operations, file upload/download, and publish/unpublish functionalityMediaMixinandAsyncMediaMixinbase classes composing file creation, download, and publication capabilitiesProgramServiceandAsyncProgramServicewith.media(program_id)accessor method to access media services for a given program