Conversation
📝 WalkthroughWalkthroughIntroduces enrollment attachments functionality to the MPT API client, including a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/e2e/program/enrollment/attachment/test_async_attachment.py (1)
85-90: Verify delete effect, not only request success.The test currently calls delete but does not validate that the attachment is actually gone.
Suggested follow-up check
async def test_delete_enrollment_attachment(enrollment_attachments, created_enrollment_attachment): result = created_enrollment_attachment await enrollment_attachments.delete(result.id) + with pytest.raises(MPTAPIError, match=r"404 Not Found"): + await enrollment_attachments.get(result.id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/program/enrollment/attachment/test_async_attachment.py` around lines 85 - 90, The test_delete_enrollment_attachment currently calls enrollment_attachments.delete(result.id) but does not assert the deletion; update the test to verify the effect by attempting to retrieve the deleted attachment (e.g., call enrollment_attachments.get(result.id) or list attachments) and assert the expected failure/absence (raise NotFound/404 or returns None/does not appear in list). Ensure you reference the same result.id from created_enrollment_attachment and handle the client’s specific error/response semantics when asserting deletion.tests/e2e/program/enrollment/attachment/test_sync_attachment.py (1)
33-37: Strengthen assertions to validate behavior, not just existence.For core E2E paths, asserting specific fields (e.g., returned
id, updatedname/description) would catch more regressions thanis not Nonechecks alone.Also applies to: 66-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/program/enrollment/attachment/test_sync_attachment.py` around lines 33 - 37, The current test only checks non-null result; strengthen assertions in test_get_enrollment_attachment_by_id by validating that result.id == attachment_id and that key fields (e.g., result.name, result.description, result.created_at or other expected attributes) match the expected values returned by enrollment_attachments.get(attachment_id); also apply the same pattern to the related update/return tests in this file (the tests covering update/attachment sync) to assert specific changed fields and IDs rather than only checking for non-None results.
🤖 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/enrollment/attachment/test_async_attachment.py`:
- Around line 91-94: In test_download_enrollment_attachment, strengthen the
assertion on result.file_contents returned by
enrollment_attachments.download(attachment_id) so empty byte payloads don't
pass: replace the current "is not None" check with a truthy/length-based
assertion (e.g., assert result.file_contents and len(result.file_contents) > 0)
and optionally verify the type (e.g., bytes) to ensure a non-empty binary
payload is returned by download.
---
Nitpick comments:
In `@tests/e2e/program/enrollment/attachment/test_async_attachment.py`:
- Around line 85-90: The test_delete_enrollment_attachment currently calls
enrollment_attachments.delete(result.id) but does not assert the deletion;
update the test to verify the effect by attempting to retrieve the deleted
attachment (e.g., call enrollment_attachments.get(result.id) or list
attachments) and assert the expected failure/absence (raise NotFound/404 or
returns None/does not appear in list). Ensure you reference the same result.id
from created_enrollment_attachment and handle the client’s specific
error/response semantics when asserting deletion.
In `@tests/e2e/program/enrollment/attachment/test_sync_attachment.py`:
- Around line 33-37: The current test only checks non-null result; strengthen
assertions in test_get_enrollment_attachment_by_id by validating that result.id
== attachment_id and that key fields (e.g., result.name, result.description,
result.created_at or other expected attributes) match the expected values
returned by enrollment_attachments.get(attachment_id); also apply the same
pattern to the related update/return tests in this file (the tests covering
update/attachment sync) to assert specific changed fields and IDs rather than
only checking for non-None results.
🪄 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: 82f83797-2777-4fe4-94d0-fb41e1fdd0a8
📒 Files selected for processing (11)
e2e_config.test.jsonmpt_api_client/resources/program/enrollments.pympt_api_client/resources/program/enrollments_attachments.pympt_api_client/resources/program/mixins/attachment_mixin.pypyproject.tomltests/e2e/program/enrollment/attachment/conftest.pytests/e2e/program/enrollment/attachment/test_async_attachment.pytests/e2e/program/enrollment/attachment/test_sync_attachment.pytests/unit/resources/program/mixin/test_attachment_mixin.pytests/unit/resources/program/test_enrollments.pytests/unit/resources/program/test_enrollments_attachments.py



This pull request adds support for program enrollment attachments in the API client, including synchronous and asynchronous service classes, a reusable attachment mixin, and comprehensive end-to-end and unit test coverage.
Enrollment Attachments Service Implementation:
EnrollmentAttachmentmodel andEnrollmentAttachmentsService/AsyncEnrollmentAttachmentsServiceclasses inmpt_api_client/resources/program/enrollments_attachments.py, supporting collection listing, get, create (file upload), update, delete, and download operations.AttachmentMixinandAsyncAttachmentMixininmpt_api_client/resources/program/mixins/attachment_mixin.pyto compose file attachment operations for both sync and async services.Integration with Enrollment Service:
.attachments(enrollment_id)method toEnrollmentServiceandAsyncEnrollmentServiceinmpt_api_client/resources/program/enrollments.pyto expose the attachments sub-service.Testing Enhancements:
tests/e2e/program/enrollment/attachment/test_sync_attachment.pyandtest_async_attachment.py, covering upload, retrieval, listing, update, download, and deletion.tests/e2e/program/enrollment/attachment/conftest.py.EnrollmentAttachmentsService,AsyncEnrollmentAttachmentsService, and the attachment mixin intests/unit/resources/program/test_enrollments_attachments.pyandtests/unit/resources/program/mixin/test_attachment_mixin.py.Configuration and Linting Updates:
e2e_config.test.jsonwithprogram.enrollment.attachment.idfor E2E testing.pyproject.tomlto ignoreWPS235for program resource files.Closes MPT-20438