Conversation
📝 WalkthroughWalkthroughThis PR adds program enrollment functionality to the MPT API client library, introducing an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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.
🧹 Nitpick comments (3)
tests/e2e/program/enrollment/test_sync_enrollment.py (1)
83-90: Consider asserting the delete post-condition explicitly.This test currently passes on successful request execution only; adding a
get-after-delete assertion makes the behavior check stronger.Suggested test hardening
def test_delete_enrollment(mpt_client, created_enrollment): enrollment_data = created_enrollment - result = mpt_client.program.enrollments - - result.delete(enrollment_data.id) + mpt_client.program.enrollments.delete(enrollment_data.id) + with pytest.raises(MPTAPIError, match=r"404 Not Found"): + mpt_client.program.enrollments.get(enrollment_data.id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/program/enrollment/test_sync_enrollment.py` around lines 83 - 90, The test_delete_enrollment function only calls mpt_client.program.enrollments.delete(enrollment_data.id) without verifying the resource was actually removed; update the test to perform a follow-up retrieval using mpt_client.program.enrollments.get(enrollment_data.id) and assert the expected post-delete behavior (e.g., that get raises a 404/NotFound exception or returns None/False), handling the client’s error/response shape accordingly so the test fails if the enrollment still exists.tests/e2e/program/enrollment/test_async_enrollment.py (1)
9-19: Avoid double-delete flow in the delete test and assert the postcondition.
test_delete_enrollmentdeletes an entity already scheduled for deletion increated_enrollmentteardown, and it does not verify that the resource is actually gone. Prefer creating/deleting within the test (or a dedicated no-teardown fixture) and assert a subsequentget()returns 404.✅ Example tightening of delete verification
-async def test_delete_enrollment(async_mpt_client, created_enrollment): - enrollment_data = created_enrollment - - result = async_mpt_client.program.enrollments - - await result.delete(enrollment_data.id) +async def test_delete_enrollment(async_mpt_client, enrollment_data): + enrollment = await async_mpt_client.program.enrollments.create(enrollment_data) + await async_mpt_client.program.enrollments.delete(enrollment.id) + with pytest.raises(MPTAPIError, match=r"404 Not Found"): + await async_mpt_client.program.enrollments.get(enrollment.id)Based on learnings: "Reuse existing resources for read-only/safe mutations; reserve isolated create/cleanup flows for destructive tests."
Also applies to: 87-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/program/enrollment/test_async_enrollment.py` around lines 9 - 19, The test is double-deleting an enrollment because the created_enrollment fixture already schedules delete in its teardown; change the destructive test to create and delete its own enrollment (use async_mpt_client.program.enrollments.create within test_delete_enrollment) or make a no-teardown fixture variant, then after calling async_mpt_client.program.enrollments.delete(enrollment.id) assert the resource is gone by calling async_mpt_client.program.enrollments.get(enrollment.id) and verifying it raises MPTAPIError with a 404/NotFound status; update any other destructive test cases (the ones around the other delete flow) the same way and remove the redundant deletion from the shared created_enrollment fixture when used by destructive tests.mpt_api_client/resources/program/enrollments.py (1)
62-132: Consider a small helper to remove action-method duplication.
validate/query/process/complete/submit/failare structurally identical in both sync and async services. A helper reduces copy/paste risk and keeps future action additions safer.♻️ Example refactor
class EnrollmentService( @@ ): @@ + def _post_action( + self, action: str, resource_id: str, resource_data: ResourceData | None = None + ) -> Enrollment: + return self._resource(resource_id).post(action, json=resource_data) + def validate(self, resource_id: str, resource_data: ResourceData | None = None) -> Enrollment: @@ - return self._resource(resource_id).post("validate", json=resource_data) + return self._post_action("validate", resource_id, resource_data) @@ - return self._resource(resource_id).post("query", json=resource_data) + return self._post_action("query", resource_id, resource_data) @@ - return self._resource(resource_id).post("process", json=resource_data) + return self._post_action("process", resource_id, resource_data) @@ - return self._resource(resource_id).post("complete", json=resource_data) + return self._post_action("complete", resource_id, resource_data) @@ - return self._resource(resource_id).post("submit", json=resource_data) + return self._post_action("submit", resource_id, resource_data) @@ - return self._resource(resource_id).post("fail", json=resource_data) + return self._post_action("fail", resource_id, resource_data)class AsyncEnrollmentService( @@ ): @@ + async def _post_action( + self, action: str, resource_id: str, resource_data: ResourceData | None = None + ) -> Enrollment: + return await self._resource(resource_id).post(action, json=resource_data) + @@ - return await self._resource(resource_id).post("validate", json=resource_data) + return await self._post_action("validate", resource_id, resource_data) @@ - return await self._resource(resource_id).post("query", json=resource_data) + return await self._post_action("query", resource_id, resource_data) @@ - return await self._resource(resource_id).post("process", json=resource_data) + return await self._post_action("process", resource_id, resource_data) @@ - return await self._resource(resource_id).post("complete", json=resource_data) + return await self._post_action("complete", resource_id, resource_data) @@ - return await self._resource(resource_id).post("submit", json=resource_data) + return await self._post_action("submit", resource_id, resource_data) @@ - return await self._resource(resource_id).post("fail", json=resource_data) + return await self._post_action("fail", resource_id, resource_data)Also applies to: 144-224
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpt_api_client/resources/program/enrollments.py` around lines 62 - 132, The six methods (validate, query, process, complete, submit, fail) duplicate the same pattern; add a single helper like a private _action method that takes resource_id, action (string) and resource_data and calls self._resource(resource_id).post(action, json=resource_data) (and an async counterpart for the async service that awaits the call), then have each public method return a call to that helper (e.g., validate -> return self._action(resource_id, "validate", resource_data)); update both sync and async classes to use this helper to remove repetition and keep behavior identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@mpt_api_client/resources/program/enrollments.py`:
- Around line 62-132: The six methods (validate, query, process, complete,
submit, fail) duplicate the same pattern; add a single helper like a private
_action method that takes resource_id, action (string) and resource_data and
calls self._resource(resource_id).post(action, json=resource_data) (and an async
counterpart for the async service that awaits the call), then have each public
method return a call to that helper (e.g., validate -> return
self._action(resource_id, "validate", resource_data)); update both sync and
async classes to use this helper to remove repetition and keep behavior
identical.
In `@tests/e2e/program/enrollment/test_async_enrollment.py`:
- Around line 9-19: The test is double-deleting an enrollment because the
created_enrollment fixture already schedules delete in its teardown; change the
destructive test to create and delete its own enrollment (use
async_mpt_client.program.enrollments.create within test_delete_enrollment) or
make a no-teardown fixture variant, then after calling
async_mpt_client.program.enrollments.delete(enrollment.id) assert the resource
is gone by calling async_mpt_client.program.enrollments.get(enrollment.id) and
verifying it raises MPTAPIError with a 404/NotFound status; update any other
destructive test cases (the ones around the other delete flow) the same way and
remove the redundant deletion from the shared created_enrollment fixture when
used by destructive tests.
In `@tests/e2e/program/enrollment/test_sync_enrollment.py`:
- Around line 83-90: The test_delete_enrollment function only calls
mpt_client.program.enrollments.delete(enrollment_data.id) without verifying the
resource was actually removed; update the test to perform a follow-up retrieval
using mpt_client.program.enrollments.get(enrollment_data.id) and assert the
expected post-delete behavior (e.g., that get raises a 404/NotFound exception or
returns None/False), handling the client’s error/response shape accordingly so
the test fails if the enrollment still exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: b54450da-38d1-4af7-ab98-a5cba29b3dc3
📒 Files selected for processing (11)
e2e_config.test.jsonmpt_api_client/resources/program/enrollments.pympt_api_client/resources/program/mixins/render_mixin.pympt_api_client/resources/program/program.pypyproject.tomltests/e2e/program/enrollment/conftest.pytests/e2e/program/enrollment/test_async_enrollment.pytests/e2e/program/enrollment/test_sync_enrollment.pytests/unit/resources/program/mixin/test_render_mixin.pytests/unit/resources/program/test_enrollments.pytests/unit/resources/program/test_program.py
albertsola
approved these changes
Apr 22, 2026
robcsegal
deleted the
MPT-20437-add-endpoints-and-e-2-e-tests-for-program-enrollments
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 comprehensive support for program enrollments in the API client, including both synchronous and asynchronous service classes, as well as extensive end-to-end and unit test coverage. It introduces new mixins for rendering resources, updates configuration and test settings, and integrates the enrollments service into the main program module.
Program Enrollment Service Implementation:
EnrollmentServiceandAsyncEnrollmentServiceclasses inmpt_api_client/resources/program/enrollments.py, providing CRUD operations and additional actions (validate, query, process, complete, submit, fail) for program enrollments, along with theEnrollmentmodel and service configuration.RenderMixinandAsyncRenderMixininmpt_api_client/resources/program/mixins/render_mixin.pyto enable rendering resources for both sync and async services.Integration with Program Module:
enrollmentsandasync_enrollmentsproperties) in the main program API modules inmpt_api_client/resources/program/program.py. [1] [2] [3]Testing Enhancements:
tests/e2e/program/enrollment/test_sync_enrollment.pyandtest_async_enrollment.py. [1] [2]tests/e2e/program/enrollment/conftest.py.tests/unit/resources/program/mixin/test_render_mixin.py.Configuration and Linting Updates:
e2e_config.test.jsonwith new enrollment-related IDs for testing.pyproject.tomlto ignore specific warnings for new enrollment test files.Closes MPT-20437
Enrollmentmodel with comprehensive fields (name, certificate, program, vendor, licensee, eligibility, template, audit, status, type, applicable_to, parameters)EnrollmentServiceandAsyncEnrollmentServicewith full CRUD operations and action methods: validate, query, process, complete, submit, failRenderMixinandAsyncRenderMixinfor rendering resources via GET requests to therenderaction endpointenrollmentsandasync_enrollmentsproperties inProgramandAsyncProgramclassesEnrollmentService,AsyncEnrollmentService, and render mixin implementations