Conversation
📝 WalkthroughWalkthroughAdds program-scoped Parameters and Templates models and sync/async services, exposes 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.
🧹 Nitpick comments (3)
tests/e2e/program/program/template/test_sync_template.py (1)
44-48: Simplify the delete test variable naming for readability.The
template_dataalias is unnecessary and can be replaced with direct usage ofcreated_template.id.Suggested simplification
def test_delete_template(mpt_vendor, program_id, created_template): - template_data = created_template - - result = mpt_vendor.program.programs.templates(program_id) - - result.delete(template_data.id) + service = mpt_vendor.program.programs.templates(program_id) + service.delete(created_template.id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/program/program/template/test_sync_template.py` around lines 44 - 48, Replace the unnecessary local alias template_data with direct usage of created_template.id: in the test function remove the template_data assignment and call delete(created_template.id) on the templates client returned by mpt_vendor.program.programs.templates(program_id) (the variable result can remain or be renamed if desired) so the test reads more directly and avoids an extra variable.tests/unit/resources/program/test_programs_templates.py (1)
13-20: Use a program-like ID value in fixtures for clearer test intent.
program_idcurrently uses a template-like prefix (PTM-001), which can be confusing when reading endpoint-contract tests.Suggested clarity-only diff
def templates_service(http_client): - return TemplatesService(http_client=http_client, endpoint_params={"program_id": "PTM-001"}) + return TemplatesService(http_client=http_client, endpoint_params={"program_id": "PRG-001"}) @@ def async_templates_service(async_http_client): return AsyncTemplatesService( - http_client=async_http_client, endpoint_params={"program_id": "PTM-001"} + http_client=async_http_client, endpoint_params={"program_id": "PRG-001"} ) @@ def test_endpoint(templates_service): - result = templates_service.path == "/public/v1/program/programs/PTM-001/templates" + result = templates_service.path == "/public/v1/program/programs/PRG-001/templates" @@ def test_async_endpoint(async_templates_service): - result = async_templates_service.path == "/public/v1/program/programs/PTM-001/templates" + result = async_templates_service.path == "/public/v1/program/programs/PRG-001/templates"Also applies to: 42-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/resources/program/test_programs_templates.py` around lines 13 - 20, Replace the template-like program_id value "PTM-001" used in the fixtures for TemplatesService and AsyncTemplatesService with a program-like identifier (e.g., "PROG-001") to make test intent clearer; update both the templates_service fixture (TemplatesService(http_client=..., endpoint_params={"program_id": "PTM-001"})) and async_templates_service fixture (AsyncTemplatesService(..., endpoint_params={"program_id": "PTM-001"})), and apply the same change to the other occurrences mentioned (lines 42-49) so all fixtures use the consistent program-style ID.tests/e2e/program/program/template/test_async_template.py (1)
43-48: Avoid fixture double-delete in the destructive test.
test_delete_templatedeletes a template that the fixture teardown also tries to delete, which creates predictable teardown noise and extra API work. Prefer a self-contained create/delete flow in this test.Suggested refactor
-async def test_delete_template(async_mpt_vendor, program_id, created_template): - template_data = created_template - - result = async_mpt_vendor.program.programs.templates(program_id) - - await result.delete(template_data.id) +async def test_delete_template(async_mpt_vendor, program_id, template_data): + service = async_mpt_vendor.program.programs.templates(program_id) + created_template = await service.create(template_data) + await service.delete(created_template.id)Based on learnings, "In end-to-end tests (e.g., tests/e2e/...), reuse existing API resources for read-only operations and safe mutations to speed up test execution. Reserve isolated fixtures (e.g., created_price_list) for destructive tests that require per-test creation and cleanup."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/program/program/template/test_async_template.py` around lines 43 - 48, test_delete_template currently deletes a template provided by the created_template fixture which causes a double-delete during fixture teardown; update the test to perform its own create/delete cycle (or use a dedicated destructive fixture) instead of deleting created_template: inside test_delete_template use async_mpt_vendor.program.programs.templates(program_id).create(...) to create a new template resource, capture its id, call await async_mpt_vendor.program.programs.templates(program_id).delete(new_id), and let teardown continue to manage the original created_template (or replace the fixture with a created_template_for_delete that the test owns).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/program/program/template/test_async_template.py`:
- Around line 43-48: test_delete_template currently deletes a template provided
by the created_template fixture which causes a double-delete during fixture
teardown; update the test to perform its own create/delete cycle (or use a
dedicated destructive fixture) instead of deleting created_template: inside
test_delete_template use
async_mpt_vendor.program.programs.templates(program_id).create(...) to create a
new template resource, capture its id, call await
async_mpt_vendor.program.programs.templates(program_id).delete(new_id), and let
teardown continue to manage the original created_template (or replace the
fixture with a created_template_for_delete that the test owns).
In `@tests/e2e/program/program/template/test_sync_template.py`:
- Around line 44-48: Replace the unnecessary local alias template_data with
direct usage of created_template.id: in the test function remove the
template_data assignment and call delete(created_template.id) on the templates
client returned by mpt_vendor.program.programs.templates(program_id) (the
variable result can remain or be renamed if desired) so the test reads more
directly and avoids an extra variable.
In `@tests/unit/resources/program/test_programs_templates.py`:
- Around line 13-20: Replace the template-like program_id value "PTM-001" used
in the fixtures for TemplatesService and AsyncTemplatesService with a
program-like identifier (e.g., "PROG-001") to make test intent clearer; update
both the templates_service fixture (TemplatesService(http_client=...,
endpoint_params={"program_id": "PTM-001"})) and async_templates_service fixture
(AsyncTemplatesService(..., endpoint_params={"program_id": "PTM-001"})), and
apply the same change to the other occurrences mentioned (lines 42-49) so all
fixtures use the consistent program-style ID.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 6de89205-5f57-4fe6-af07-c9302bf53004
📒 Files selected for processing (9)
e2e_config.test.jsonmpt_api_client/resources/program/programs.pympt_api_client/resources/program/programs_templates.pypyproject.tomltests/e2e/program/program/template/conftest.pytests/e2e/program/program/template/test_async_template.pytests/e2e/program/program/template/test_sync_template.pytests/unit/resources/program/test_programs.pytests/unit/resources/program/test_programs_templates.py
robcsegal
force-pushed
the
MPT-20328-add-endpoints-and-e-2-e-tests-for-program-templates
branch
from
45803a1 to
b3a6309
Compare
robcsegal
force-pushed
the
MPT-20328-add-endpoints-and-e-2-e-tests-for-program-templates
branch
from
a7712f9 to
42ab336
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/e2e/program/program/parameter/test_sync_parameter.py (1)
48-53: Delete test can be simplified by removing temporary variables.Current temporaries add noise without improving clarity.
♻️ Suggested cleanup
def test_delete_parameter(mpt_vendor, program_id, created_parameter): - parameter_data = created_parameter - - result = mpt_vendor.program.programs.parameters(program_id) - - result.delete(parameter_data.id) + mpt_vendor.program.programs.parameters(program_id).delete(created_parameter.id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/program/program/parameter/test_sync_parameter.py` around lines 48 - 53, The test_delete_parameter currently uses unnecessary temporaries (parameter_data and result); simplify by calling the API directly using created_parameter and the parameters accessor: replace uses of parameter_data.id with created_parameter.id and call mpt_vendor.program.programs.parameters(program_id).delete(created_parameter.id) instead of assigning result = ... and parameter_data = .... Update the test_delete_parameter function to remove the intermediate variables and call delete in a single expression.tests/unit/resources/program/test_programs_parameters.py (1)
72-87: Rename template-prefixed tests to parameter-prefixed names.These tests validate
Parameter, so thetemplatenaming hurts discoverability and intent.✏️ Suggested rename diff
-def test_template_primitive_fields(parameter_data): +def test_parameter_primitive_fields(parameter_data): @@ -def test_template_nested_field_types(parameter_data): +def test_parameter_nested_field_types(parameter_data): @@ -def test_template_optional_fields_absent(): +def test_parameter_optional_fields_absent():🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/resources/program/test_programs_parameters.py` around lines 72 - 87, Rename the test functions that are incorrectly prefixed with "template_" to use the "parameter_" prefix to match the tested class Parameter and improve discoverability; specifically, rename test_template_primitive_fields to test_parameter_primitive_fields and test_template_nested_field_types to test_parameter_nested_field_types (update any references/imports or test markers if present) while leaving behavior/assertions unchanged.tests/e2e/program/program/parameter/test_async_parameter.py (1)
48-53: Delete test can be simplified by removing temporary variables.This keeps the intent focused on the delete operation.
♻️ Suggested cleanup
async def test_delete_parameter(async_mpt_vendor, program_id, created_parameter): - parameter_data = created_parameter - - result = async_mpt_vendor.program.programs.parameters(program_id) - - await result.delete(parameter_data.id) + await async_mpt_vendor.program.programs.parameters(program_id).delete(created_parameter.id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/program/program/parameter/test_async_parameter.py` around lines 48 - 53, The test_delete_parameter function creates unnecessary temporaries (parameter_data and result); simplify by calling the delete directly on the parameters endpoint using the existing fixtures. Replace the two variables so the test calls the delete method directly via async_mpt_vendor.program.programs.parameters(program_id).delete(created_parameter.id) (keeping the same async behavior), ensuring references remain to test_delete_parameter, async_mpt_vendor, program_id, created_parameter, and programs.parameters(...).delete(...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/program/program/parameter/test_async_parameter.py`:
- Around line 48-53: The test_delete_parameter function creates unnecessary
temporaries (parameter_data and result); simplify by calling the delete directly
on the parameters endpoint using the existing fixtures. Replace the two
variables so the test calls the delete method directly via
async_mpt_vendor.program.programs.parameters(program_id).delete(created_parameter.id)
(keeping the same async behavior), ensuring references remain to
test_delete_parameter, async_mpt_vendor, program_id, created_parameter, and
programs.parameters(...).delete(...).
In `@tests/e2e/program/program/parameter/test_sync_parameter.py`:
- Around line 48-53: The test_delete_parameter currently uses unnecessary
temporaries (parameter_data and result); simplify by calling the API directly
using created_parameter and the parameters accessor: replace uses of
parameter_data.id with created_parameter.id and call
mpt_vendor.program.programs.parameters(program_id).delete(created_parameter.id)
instead of assigning result = ... and parameter_data = .... Update the
test_delete_parameter function to remove the intermediate variables and call
delete in a single expression.
In `@tests/unit/resources/program/test_programs_parameters.py`:
- Around line 72-87: Rename the test functions that are incorrectly prefixed
with "template_" to use the "parameter_" prefix to match the tested class
Parameter and improve discoverability; specifically, rename
test_template_primitive_fields to test_parameter_primitive_fields and
test_template_nested_field_types to test_parameter_nested_field_types (update
any references/imports or test markers if present) while leaving
behavior/assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f1db40a7-11c8-4c63-a829-b7af005d171e
📒 Files selected for processing (8)
e2e_config.test.jsonmpt_api_client/resources/program/programs.pympt_api_client/resources/program/programs_parameters.pytests/e2e/program/program/parameter/conftest.pytests/e2e/program/program/parameter/test_async_parameter.pytests/e2e/program/program/parameter/test_sync_parameter.pytests/unit/resources/program/test_programs.pytests/unit/resources/program/test_programs_parameters.py
✅ Files skipped from review due to trivial changes (2)
- tests/unit/resources/program/test_programs.py
- tests/e2e/program/program/parameter/conftest.py
|
robcsegal
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (4)
tests/e2e/program/program/template/test_sync_template.py (1)
48-53: Consider clearer variable naming.The variable
resulton line 51 holds a service instance, not a result. Consider renaming for clarity:def test_delete_template(mpt_vendor, program_id, created_template): - template_data = created_template - - result = mpt_vendor.program.programs.templates(program_id) - - result.delete(template_data.id) + service = mpt_vendor.program.programs.templates(program_id) + + service.delete(created_template.id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/program/program/template/test_sync_template.py` around lines 48 - 53, The variable named result in test_delete_template is misleading because it holds the templates service, not a result; rename it (for example to templates_service or templates_client) by replacing the assignment "result = mpt_vendor.program.programs.templates(program_id)" and all usages (currently "result.delete(template_data.id)") so the test reads clearly (e.g., "templates_service = mpt_vendor.program.programs.templates(program_id)" and "templates_service.delete(template_data.id)"), leaving created_template and test_delete_template unchanged.tests/e2e/program/program/parameter/test_sync_parameter.py (1)
48-53: Consider clearer variable naming.Same pattern as template tests -
resultholds a service:def test_delete_parameter(mpt_vendor, program_id, created_parameter): - parameter_data = created_parameter - - result = mpt_vendor.program.programs.parameters(program_id) - - result.delete(parameter_data.id) + service = mpt_vendor.program.programs.parameters(program_id) + + service.delete(created_parameter.id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/program/program/parameter/test_sync_parameter.py` around lines 48 - 53, The test uses a vague variable name `result` for the parameters service; rename it to something clearer (e.g., `parameters_service` or `parameters`) in test_delete_parameter so it's obvious that mpt_vendor.program.programs.parameters(program_id) returns a service object, and also simplify `parameter_data` to `parameter` to make intent clearer when calling parameters_service.delete(parameter.id).tests/e2e/program/program/template/test_async_template.py (1)
48-53: Consider clearer variable naming.Same as the sync version -
resultholds a service, not a result:async def test_delete_template(async_mpt_vendor, program_id, created_template): - template_data = created_template - - result = async_mpt_vendor.program.programs.templates(program_id) - - await result.delete(template_data.id) + service = async_mpt_vendor.program.programs.templates(program_id) + + await service.delete(created_template.id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/program/program/template/test_async_template.py` around lines 48 - 53, The test_delete_template uses a misleading variable name `result` for the service returned by async_mpt_vendor.program.programs.templates; rename `result` to a clearer identifier like `templates_service` (or `templates_client`) and optionally rename `template_data` to `template` to reflect it holds the created template; update the call site in test_delete_template where you do await result.delete(template_data.id) to use the new service name (e.g., await templates_service.delete(template.id)).tests/e2e/program/program/parameter/test_async_parameter.py (1)
48-53: Consider clearer variable naming.Same as other delete tests -
resultholds a service:async def test_delete_parameter(async_mpt_vendor, program_id, created_parameter): - parameter_data = created_parameter - - result = async_mpt_vendor.program.programs.parameters(program_id) - - await result.delete(parameter_data.id) + service = async_mpt_vendor.program.programs.parameters(program_id) + + await service.delete(created_parameter.id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/program/program/parameter/test_async_parameter.py` around lines 48 - 53, The test test_delete_parameter uses a vague variable name result for the service returned by async_mpt_vendor.program.programs.parameters(program_id); rename it to a clearer identifier (e.g., parameters_service or parameters_client) and update its usages (await parameters_service.delete(parameter_data.id)) so the code makes it explicit that this is a service object operating on parameters; ensure the same naming convention is applied consistently in other delete tests that call programs.parameters(...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/program/program/parameter/test_async_parameter.py`:
- Around line 48-53: The test test_delete_parameter uses a vague variable name
result for the service returned by
async_mpt_vendor.program.programs.parameters(program_id); rename it to a clearer
identifier (e.g., parameters_service or parameters_client) and update its usages
(await parameters_service.delete(parameter_data.id)) so the code makes it
explicit that this is a service object operating on parameters; ensure the same
naming convention is applied consistently in other delete tests that call
programs.parameters(...).
In `@tests/e2e/program/program/parameter/test_sync_parameter.py`:
- Around line 48-53: The test uses a vague variable name `result` for the
parameters service; rename it to something clearer (e.g., `parameters_service`
or `parameters`) in test_delete_parameter so it's obvious that
mpt_vendor.program.programs.parameters(program_id) returns a service object, and
also simplify `parameter_data` to `parameter` to make intent clearer when
calling parameters_service.delete(parameter.id).
In `@tests/e2e/program/program/template/test_async_template.py`:
- Around line 48-53: The test_delete_template uses a misleading variable name
`result` for the service returned by
async_mpt_vendor.program.programs.templates; rename `result` to a clearer
identifier like `templates_service` (or `templates_client`) and optionally
rename `template_data` to `template` to reflect it holds the created template;
update the call site in test_delete_template where you do await
result.delete(template_data.id) to use the new service name (e.g., await
templates_service.delete(template.id)).
In `@tests/e2e/program/program/template/test_sync_template.py`:
- Around line 48-53: The variable named result in test_delete_template is
misleading because it holds the templates service, not a result; rename it (for
example to templates_service or templates_client) by replacing the assignment
"result = mpt_vendor.program.programs.templates(program_id)" and all usages
(currently "result.delete(template_data.id)") so the test reads clearly (e.g.,
"templates_service = mpt_vendor.program.programs.templates(program_id)" and
"templates_service.delete(template_data.id)"), leaving created_template and
test_delete_template unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 55843418-3273-40fe-a4d5-3a62d243cdb1
📒 Files selected for processing (10)
e2e_config.test.jsonmpt_api_client/resources/program/programs.pympt_api_client/resources/program/programs_parameters.pytests/e2e/program/program/parameter/conftest.pytests/e2e/program/program/parameter/test_async_parameter.pytests/e2e/program/program/parameter/test_sync_parameter.pytests/e2e/program/program/template/test_async_template.pytests/e2e/program/program/template/test_sync_template.pytests/unit/resources/program/test_programs.pytests/unit/resources/program/test_programs_parameters.py
✅ Files skipped from review due to trivial changes (5)
- tests/unit/resources/program/test_programs.py
- tests/e2e/program/program/parameter/conftest.py
- tests/unit/resources/program/test_programs_parameters.py
- e2e_config.test.json
- mpt_api_client/resources/program/programs_parameters.py
jentyk
approved these changes
Apr 20, 2026
robcsegal
deleted the
MPT-20328-add-endpoints-and-e-2-e-tests-for-program-templates
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 support for managing program templates in the API client. It introduces new service classes for both synchronous and asynchronous operations on templates, updates the main Program services to expose these new capabilities, and provides comprehensive unit and end-to-end tests to verify the new functionality.
New Program Templates Support:
Added Template model and TemplatesService / AsyncTemplatesService classes to programs_templates.py, including endpoint configuration and resource mixins.
Updated ProgramsService and AsyncProgramsService classes in programs.py to provide .templates(program_id) methods for accessing the new services.
Testing Enhancements:
Added unit tests for the new services and model in test_programs_templates.py, covering endpoints, method presence, and model field parsing.
Added end-to-end tests for both sync and async template operations (create, update, get, delete, filter/select) in test_sync_template.py and test_async_template.py, with supporting fixtures in conftest.py.
Updated unit tests for Program services to check for the presence of the new .templates methods in test_programs.py.
Configuration Updates:
Added a seeded template ID to e2e_config.test.json for use in tests.
Closes MPT-20328
Added Template model with typed fields for content (name, content, type, status), behavior (default), and relationships (external_ids, program, audit).
Introduced TemplatesService and AsyncTemplatesService with endpoint configuration at /public/v1/program/programs/{program_id}/templates and support for managed resource operations (create, read, update, delete, list, filter, select).
Extended ProgramsService and AsyncProgramsService with templates(program_id) factory methods for accessing template services.
Added comprehensive end-to-end tests for synchronous and asynchronous template operations including create, update, get, delete, and filtered listing with field selection.
Added unit tests validating service configuration, endpoint paths, public method availability, and model field parsing.
Added pytest fixtures for template e2e tests with test data and configuration.
Closes MPT-20328