MPT-19906: add /integration/extensions/{id}/terms service#286
Merged
albertsola merged 2 commits intomainfrom Apr 9, 2026
Merged
MPT-19906: add /integration/extensions/{id}/terms service#286albertsola merged 2 commits intomainfrom
albertsola merged 2 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughAdds ExtensionTerm and ExtensionTermVariant models, sync/async services, a PublishableMixin, extensions.terms(...) accessors, updates an e2e config value, and introduces unit and E2E tests covering create/filter/update/publish/unpublish/delete flows. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 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 |
albertsola
force-pushed
the
MPT-19906/integration-extension-terms
branch
from
be2da71 to
fc378ce
Compare
…endpoint Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
albertsola
force-pushed
the
MPT-19906/integration-extension-terms
branch
2 times, most recently
from
ad6f288 to
7222c15
Compare
![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/unit/resources/integration/test_extension_terms.py (1)
90-94: Remove redundanttest_extension_term_createassertion.
createis already covered by the parametrized mixin-presence tests, so this test can be dropped to reduce duplication.♻️ Suggested cleanup
-def test_extension_term_create(terms_service: ExtensionTermsService) -> None: - result = hasattr(terms_service, "create") - - assert result is True - - def test_extension_terms_variants_accessor(terms_service: ExtensionTermsService) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/resources/integration/test_extension_terms.py` around lines 90 - 94, Remove the redundant test function test_extension_term_create that only checks hasattr(terms_service, "create"); locate the test function named test_extension_term_create in the tests/unit/resources/integration/test_extension_terms.py file and delete it (the behavior is already covered by the parametrized mixin-presence tests), ensuring no other tests reference that function name.tests/e2e/integration/extension_terms/test_async_extension_terms.py (1)
44-45: Strengthen delete test with an explicit outcome check.Consider asserting the term is no longer retrievable after delete to ensure the test verifies behavior, not only lack of exception.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/integration/extension_terms/test_async_extension_terms.py` around lines 44 - 45, The delete test currently only calls async_extension_terms_service.delete(async_created_term.id) without verifying outcome; update test_delete_extension_term to assert the term was actually removed by attempting to retrieve it after deletion (call async_extension_terms_service.get or list by id) and assert the call either raises the expected NotFound/KeyError or returns None/empty result for async_created_term.id; use the same async_extension_terms_service.delete and async_created_term.id symbols to locate and modify the test.tests/e2e/integration/extension_terms/test_sync_extension_terms.py (1)
42-43: Add a post-delete verification to prevent false positives.This test currently passes if
delete()returns without raising, even if the resource remains. Add a follow-up assertion (e.g., filtered result is empty or fetch by id fails) to verify the deletion outcome.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/integration/extension_terms/test_sync_extension_terms.py` around lines 42 - 43, The test test_delete_extension_term only calls extension_terms_service.delete(created_term.id) but lacks verification; after calling extension_terms_service.delete(created_term.id) add an assertion that the term is actually gone by either calling extension_terms_service.get(created_term.id) and asserting it raises/returns None, or calling extension_terms_service.list(filter_by_id=created_term.id) / extension_terms_service.filter(...) and asserting the result is empty; update references to created_term.id and use the service's existing get/list/filter methods (or expected exception) to verify deletion.
🤖 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/integration/extension_terms/test_async_extension_terms.py`:
- Around line 44-45: The delete test currently only calls
async_extension_terms_service.delete(async_created_term.id) without verifying
outcome; update test_delete_extension_term to assert the term was actually
removed by attempting to retrieve it after deletion (call
async_extension_terms_service.get or list by id) and assert the call either
raises the expected NotFound/KeyError or returns None/empty result for
async_created_term.id; use the same async_extension_terms_service.delete and
async_created_term.id symbols to locate and modify the test.
In `@tests/e2e/integration/extension_terms/test_sync_extension_terms.py`:
- Around line 42-43: The test test_delete_extension_term only calls
extension_terms_service.delete(created_term.id) but lacks verification; after
calling extension_terms_service.delete(created_term.id) add an assertion that
the term is actually gone by either calling
extension_terms_service.get(created_term.id) and asserting it raises/returns
None, or calling extension_terms_service.list(filter_by_id=created_term.id) /
extension_terms_service.filter(...) and asserting the result is empty; update
references to created_term.id and use the service's existing get/list/filter
methods (or expected exception) to verify deletion.
In `@tests/unit/resources/integration/test_extension_terms.py`:
- Around line 90-94: Remove the redundant test function
test_extension_term_create that only checks hasattr(terms_service, "create");
locate the test function named test_extension_term_create in the
tests/unit/resources/integration/test_extension_terms.py file and delete it (the
behavior is already covered by the parametrized mixin-presence tests), ensuring
no other tests reference that function name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1cd09ec3-68b8-4426-accf-152a8685b74c
📒 Files selected for processing (11)
e2e_config.test.jsonmpt_api_client/resources/integration/extension_term_variants.pympt_api_client/resources/integration/extension_terms.pympt_api_client/resources/integration/extensions.pympt_api_client/resources/integration/mixins/__init__.pympt_api_client/resources/integration/mixins/publishable_mixin.pytests/e2e/integration/extension_terms/__init__.pytests/e2e/integration/extension_terms/conftest.pytests/e2e/integration/extension_terms/test_async_extension_terms.pytests/e2e/integration/extension_terms/test_sync_extension_terms.pytests/unit/resources/integration/test_extension_terms.py
…sionId}/terms Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
albertsola
force-pushed
the
MPT-19906/integration-extension-terms
branch
from
7222c15 to
d429f76
Compare
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/resources/integration/mixins/test_publishable_mixin.py (1)
42-137: Consider reducing duplication across sync/async action tests.The request mock setup and assertions are repeated in four tests. Extracting shared helpers (route setup + common assertions) would make future endpoint/action updates less error-prone.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/resources/integration/mixins/test_publishable_mixin.py` around lines 42 - 137, The tests duplicate route setup and assertions across test_action_with_data, test_action_no_data, test_async_action_with_data, and test_async_action_no_data; refactor by extracting reusable helpers: (1) a setup_route(action, expected_response) helper that creates and returns the respx mock_route used in all tests, and (2) an assert_request_post(mock_route, expected_content) helper that checks mock_route.call_count, request.method and request.content and optionally validates result type/structure; update calls in publishable_service and async_publishable_service tests to use these helpers to reduce duplication while keeping the same behavior.
🤖 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/unit/resources/integration/mixins/test_publishable_mixin.py`:
- Around line 42-137: The tests duplicate route setup and assertions across
test_action_with_data, test_action_no_data, test_async_action_with_data, and
test_async_action_no_data; refactor by extracting reusable helpers: (1) a
setup_route(action, expected_response) helper that creates and returns the respx
mock_route used in all tests, and (2) an assert_request_post(mock_route,
expected_content) helper that checks mock_route.call_count, request.method and
request.content and optionally validates result type/structure; update calls in
publishable_service and async_publishable_service tests to use these helpers to
reduce duplication while keeping the same behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 8992d936-f4f1-4c08-9027-255c2141bdf3
📒 Files selected for processing (6)
e2e_config.test.jsontests/e2e/integration/extension_terms/__init__.pytests/e2e/integration/extension_terms/conftest.pytests/e2e/integration/extension_terms/test_async_extension_terms.pytests/e2e/integration/extension_terms/test_sync_extension_terms.pytests/unit/resources/integration/mixins/test_publishable_mixin.py
✅ Files skipped from review due to trivial changes (1)
- e2e_config.test.json
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/integration/extension_terms/test_sync_extension_terms.py
- tests/e2e/integration/extension_terms/test_async_extension_terms.py
robcsegal
approved these changes
Apr 9, 2026
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.
Summary
Implements the
/public/v1/integration/extensions/{extensionId}/termsendpoint group as part of MPT-13310.Changes
mpt_api_client/resources/integration/extension_terms.py— PublishableMixin + ManagedResourceMixin + CollectionMixin; exposesvariants(term_id)accessormpt_api_client/resources/integration/extension_term_variants.py— stub only (full impl in MPT-19907)extensions.pyto exposeterms(extension_id)accessortests/unit/resources/integration/test_extension_terms.pytests/e2e/integration/extension_terms/Depends on
#277 (MPT-19892 — rename extensibility → integration)
Closes MPT-19906
/public/v1/integration/extensions/{extensionId}/termsAPI resource withExtensionTermsServiceandAsyncExtensionTermsServiceExtensionTermmodel (name, revision, description, display_order, status, extension, audit)PublishableMixin,ManagedResourceMixin, andCollectionMixininto terms services to support CRUD, publish/unpublish, and collection operationsvariants(term_id)accessor on terms services to return scopedExtensionTermVariantsService/AsyncExtensionTermVariantsServiceExtensionTermVariantsServiceandAsyncExtensionTermVariantsServicefor/terms/{termId}/variants(full implementation deferred to MPT-19907)PublishableMixinandAsyncPublishableMixinproviding publish/unpublish methodsExtensionsServiceandAsyncExtensionsServiceto exposeterms(extension_id)accessorintegration.extension.idine2e_config.test.jsonfromEXT-4401-0953toEXT-6587-4477