MPT-19660 Improve resource action implementation#261
Merged
albertsola merged 1 commit intomainfrom Mar 30, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces centralized Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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}
albertsola
force-pushed
the
MPT-19660/Review-duplicated-resource-action
branch
from
0dad396 to
2100741
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
mpt_api_client/resources/accounts/mixins/activatable_mixin.py (1)
1-1: Prefer importing decorators from the publicmpt_api_client.httpAPI surface.This keeps mixins decoupled from internal module layout (
http.resource) and uses the stable export you just added.Proposed change
-from mpt_api_client.http.resource import async_resource_action, resource_action +from mpt_api_client.http import async_resource_action, resource_action🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpt_api_client/resources/accounts/mixins/activatable_mixin.py` at line 1, The mixin imports decorators from the internal module mpt_api_client.http.resource; change the import to use the public API surface by importing async_resource_action and resource_action from mpt_api_client.http instead (update the import statement that currently references mpt_api_client.http.resource to import these two symbols from mpt_api_client.http so ActivatableMixin / any functions in activatable_mixin.py continue to use the public decorators).
🤖 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/accounts/mixins/activatable_mixin.py`:
- Line 1: The mixin imports decorators from the internal module
mpt_api_client.http.resource; change the import to use the public API surface by
importing async_resource_action and resource_action from mpt_api_client.http
instead (update the import statement that currently references
mpt_api_client.http.resource to import these two symbols from
mpt_api_client.http so ActivatableMixin / any functions in activatable_mixin.py
continue to use the public decorators).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: b6112a3c-73d5-48c1-87a0-58665b70452f
📒 Files selected for processing (5)
mpt_api_client/http/__init__.pympt_api_client/http/mixins/update_mixin.pympt_api_client/http/resource.pympt_api_client/resources/accounts/mixins/activatable_mixin.pympt_api_client/resources/catalog/mixins/activatable_mixin.py
✅ Files skipped from review due to trivial changes (1)
- mpt_api_client/resources/catalog/mixins/activatable_mixin.py
🚧 Files skipped from review as they are similar to previous changes (1)
- mpt_api_client/http/mixins/update_mixin.py
robcsegal
approved these changes
Mar 30, 2026
albertsola
force-pushed
the
MPT-19660/Review-duplicated-resource-action
branch
3 times, most recently
from
7313469 to
5a364a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
mpt_api_client/resources/notifications/categories.py (1)
31-47: Consider reusing the existing publishable mixin here.These four methods now mirror
mpt_api_client/resources/catalog/mixins/publishable_mixin.pyalmost line-for-line. Pulling that mixin up to a shared location and inheriting it here would keep future accessor changes centralized.Also applies to: 58-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpt_api_client/resources/notifications/categories.py` around lines 31 - 47, These publish/unpublish methods duplicate logic in mpt_api_client/resources/catalog/mixins/publishable_mixin.py; refactor by moving that mixin to a shared location and have this resource class inherit the shared PublishableMixin instead of reimplementing publish and unpublish (also apply the same change for the other two methods referenced at lines 58-74); remove the duplicate implementations of publish, unpublish (and the matching pair at 58-74), add the import for the shared PublishableMixin, and ensure the existing calls that use _resource(), ResourceData and Model types keep working with the mixin’s method names.mpt_api_client/resources/billing/mixins/recalculatable_mixin.py (1)
7-32: Method overlap betweenIssuableMixinandRecalculatableMixin— design consideration.Both mixins define
recalculate()andqueue()methods with identical signatures. While no class currently inherits from both mixins simultaneously, having the same method in two separate mixins is redundant and could cause silent MRO shadowing if combined in the future. Consider consolidating these overlapping methods into a single mixin or renaming them to clarify their distinct purposes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpt_api_client/resources/billing/mixins/recalculatable_mixin.py` around lines 7 - 32, Rework the duplicate recalculate and queue methods across IssuableMixin and RecalculatableMixin by consolidating the shared implementations into a single mixin (e.g., RecalculatableMixin as the canonical source) or a new common mixin that both can inherit; remove the duplicate method definitions from the other mixin(s) so only one implementation of recalculate() and queue() exists, and update any classes that previously inherited both mixins to import/use the single consolidated mixin; reference the existing method names recalculate, queue and the mixin classes IssuableMixin and RecalculatableMixin when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mpt_api_client/http/async_service.py`:
- Around line 21-32: The _resource method currently uses
urljoin(f"{self.path}/", resource_id) which treats resource_id as an absolute
path/URL; change it to build the URL by safely joining and escaping path
segments (e.g., use posixpath.join or manual "/" joining with urllib.parse.quote
on resource_id) so leading slashes or absolute URLs cannot drop or override
self.path, and return AsyncResourceAccessor(self.http_client, safe_joined_url,
self._model_class); apply the same fix pattern to the similar join logic in
resource_accessor.py where action names are concatenated to resource URLs.
---
Nitpick comments:
In `@mpt_api_client/resources/billing/mixins/recalculatable_mixin.py`:
- Around line 7-32: Rework the duplicate recalculate and queue methods across
IssuableMixin and RecalculatableMixin by consolidating the shared
implementations into a single mixin (e.g., RecalculatableMixin as the canonical
source) or a new common mixin that both can inherit; remove the duplicate method
definitions from the other mixin(s) so only one implementation of recalculate()
and queue() exists, and update any classes that previously inherited both mixins
to import/use the single consolidated mixin; reference the existing method names
recalculate, queue and the mixin classes IssuableMixin and RecalculatableMixin
when making the change.
In `@mpt_api_client/resources/notifications/categories.py`:
- Around line 31-47: These publish/unpublish methods duplicate logic in
mpt_api_client/resources/catalog/mixins/publishable_mixin.py; refactor by moving
that mixin to a shared location and have this resource class inherit the shared
PublishableMixin instead of reimplementing publish and unpublish (also apply the
same change for the other two methods referenced at lines 58-74); remove the
duplicate implementations of publish, unpublish (and the matching pair at
58-74), add the import for the shared PublishableMixin, and ensure the existing
calls that use _resource(), ResourceData and Model types keep working with the
mixin’s method names.
🪄 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: 45058694-b640-4a28-861c-2ba3cc498b86
📒 Files selected for processing (34)
mpt_api_client/http/async_service.pympt_api_client/http/mixins/delete_mixin.pympt_api_client/http/mixins/disable_mixin.pympt_api_client/http/mixins/download_file_mixin.pympt_api_client/http/mixins/enable_mixin.pympt_api_client/http/mixins/get_mixin.pympt_api_client/http/mixins/update_mixin.pympt_api_client/http/resource_accessor.pympt_api_client/http/service.pympt_api_client/resources/accounts/buyers.pympt_api_client/resources/accounts/mixins/activatable_mixin.pympt_api_client/resources/accounts/mixins/blockable_mixin.pympt_api_client/resources/accounts/mixins/invitable_mixin.pympt_api_client/resources/accounts/mixins/validate_mixin.pympt_api_client/resources/accounts/sellers.pympt_api_client/resources/accounts/users.pympt_api_client/resources/billing/mixins/acceptable_mixin.pympt_api_client/resources/billing/mixins/issuable_mixin.pympt_api_client/resources/billing/mixins/recalculatable_mixin.pympt_api_client/resources/billing/mixins/regeneratable_mixin.pympt_api_client/resources/catalog/mixins/activatable_mixin.pympt_api_client/resources/catalog/mixins/publishable_mixin.pympt_api_client/resources/catalog/pricing_policies.pympt_api_client/resources/catalog/products.pympt_api_client/resources/commerce/mixins/render_mixin.pympt_api_client/resources/commerce/mixins/template_mixin.pympt_api_client/resources/commerce/mixins/terminate_mixin.pympt_api_client/resources/commerce/orders.pympt_api_client/resources/helpdesk/cases.pympt_api_client/resources/helpdesk/chat_answers.pympt_api_client/resources/helpdesk/forms.pympt_api_client/resources/helpdesk/queues.pympt_api_client/resources/notifications/categories.pympt_api_client/resources/notifications/contacts.py
✅ Files skipped from review due to trivial changes (1)
- mpt_api_client/resources/notifications/contacts.py
🚧 Files skipped from review as they are similar to previous changes (1)
- mpt_api_client/resources/catalog/mixins/activatable_mixin.py
albertsola
force-pushed
the
MPT-19660/Review-duplicated-resource-action
branch
3 times, most recently
from
1321f15 to
e02a234
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/http/test_url_utils.py (1)
60-61: Makejoin_url_path()'s typing match itsNonebehavior.Line 61 needs
# type: ignore[arg-type]only because the helper still advertises*segments: str. If skippingNoneis intentional, widen the signature tostr | Noneso callers do not need type escapes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/http/test_url_utils.py` around lines 60 - 61, The test shows join_url_path accepts None segments at runtime but its signature is declared as *segments: str, so update join_url_path's type signature to accept optional strings (e.g., *segments: str | None or *segments: Optional[str]) and adjust any internal typing (imports from typing if using Optional) so callers don't need # type: ignore; keep the runtime behavior of skipping None intact and update the function docstring/type hints accordingly.
🤖 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/unit/http/test_url_utils.py`:
- Around line 42-45: The test currently asserts that join_url_path preserves a
dot-segment ("../other"), but we must instead treat dot-segments as unsafe and
either reject or escape them; update the implementation of join_url_path to
detect path segments equal to ".." or "." and either raise a ValueError (or
return an error) or percent-encode those segments before joining, and then
change test_dotdot_segment_is_literal to expect the new behavior (error/escape)
rather than the raw "../other"; use the symbol join_url_path and the test name
test_dotdot_segment_is_literal to locate and update the code and test.
---
Nitpick comments:
In `@tests/unit/http/test_url_utils.py`:
- Around line 60-61: The test shows join_url_path accepts None segments at
runtime but its signature is declared as *segments: str, so update
join_url_path's type signature to accept optional strings (e.g., *segments: str
| None or *segments: Optional[str]) and adjust any internal typing (imports from
typing if using Optional) so callers don't need # type: ignore; keep the runtime
behavior of skipping None intact and update the function docstring/type hints
accordingly.
🪄 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: 5c02cf3a-710c-4550-8dd0-7bfc3ffbc1a8
📒 Files selected for processing (41)
mpt_api_client/http/async_service.pympt_api_client/http/mixins/delete_mixin.pympt_api_client/http/mixins/disable_mixin.pympt_api_client/http/mixins/download_file_mixin.pympt_api_client/http/mixins/enable_mixin.pympt_api_client/http/mixins/get_mixin.pympt_api_client/http/mixins/update_file_mixin.pympt_api_client/http/mixins/update_mixin.pympt_api_client/http/resource_accessor.pympt_api_client/http/service.pympt_api_client/http/url_utils.pympt_api_client/resources/accounts/buyers.pympt_api_client/resources/accounts/mixins/activatable_mixin.pympt_api_client/resources/accounts/mixins/blockable_mixin.pympt_api_client/resources/accounts/mixins/invitable_mixin.pympt_api_client/resources/accounts/mixins/validate_mixin.pympt_api_client/resources/accounts/sellers.pympt_api_client/resources/accounts/users.pympt_api_client/resources/billing/custom_ledgers.pympt_api_client/resources/billing/journals.pympt_api_client/resources/billing/mixins/acceptable_mixin.pympt_api_client/resources/billing/mixins/issuable_mixin.pympt_api_client/resources/billing/mixins/recalculatable_mixin.pympt_api_client/resources/billing/mixins/regeneratable_mixin.pympt_api_client/resources/catalog/mixins/activatable_mixin.pympt_api_client/resources/catalog/mixins/publishable_mixin.pympt_api_client/resources/catalog/pricing_policies.pympt_api_client/resources/catalog/products.pympt_api_client/resources/commerce/mixins/render_mixin.pympt_api_client/resources/commerce/mixins/template_mixin.pympt_api_client/resources/commerce/mixins/terminate_mixin.pympt_api_client/resources/commerce/orders.pympt_api_client/resources/helpdesk/cases.pympt_api_client/resources/helpdesk/chat_answers.pympt_api_client/resources/helpdesk/forms.pympt_api_client/resources/helpdesk/queues.pympt_api_client/resources/notifications/categories.pympt_api_client/resources/notifications/contacts.pypyproject.tomltests/unit/http/test_resource_accessor.pytests/unit/http/test_url_utils.py
✅ Files skipped from review due to trivial changes (14)
- mpt_api_client/http/mixins/enable_mixin.py
- pyproject.toml
- mpt_api_client/http/url_utils.py
- mpt_api_client/http/mixins/disable_mixin.py
- mpt_api_client/resources/catalog/pricing_policies.py
- mpt_api_client/resources/accounts/sellers.py
- mpt_api_client/resources/accounts/mixins/blockable_mixin.py
- mpt_api_client/resources/commerce/mixins/terminate_mixin.py
- mpt_api_client/resources/commerce/orders.py
- mpt_api_client/resources/helpdesk/chat_answers.py
- mpt_api_client/resources/accounts/buyers.py
- mpt_api_client/resources/billing/mixins/issuable_mixin.py
- mpt_api_client/resources/accounts/users.py
- mpt_api_client/resources/catalog/mixins/publishable_mixin.py
🚧 Files skipped from review as they are similar to previous changes (19)
- mpt_api_client/http/mixins/delete_mixin.py
- mpt_api_client/resources/commerce/mixins/render_mixin.py
- mpt_api_client/resources/commerce/mixins/template_mixin.py
- mpt_api_client/resources/accounts/mixins/validate_mixin.py
- mpt_api_client/resources/helpdesk/forms.py
- mpt_api_client/http/mixins/get_mixin.py
- mpt_api_client/resources/notifications/categories.py
- mpt_api_client/resources/accounts/mixins/activatable_mixin.py
- mpt_api_client/resources/helpdesk/queues.py
- mpt_api_client/resources/accounts/mixins/invitable_mixin.py
- mpt_api_client/resources/billing/mixins/recalculatable_mixin.py
- mpt_api_client/http/mixins/download_file_mixin.py
- mpt_api_client/http/mixins/update_mixin.py
- mpt_api_client/resources/notifications/contacts.py
- mpt_api_client/resources/billing/mixins/acceptable_mixin.py
- mpt_api_client/resources/catalog/mixins/activatable_mixin.py
- mpt_api_client/resources/billing/mixins/regeneratable_mixin.py
- mpt_api_client/resources/helpdesk/cases.py
- mpt_api_client/resources/catalog/products.py
albertsola
force-pushed
the
MPT-19660/Review-duplicated-resource-action
branch
6 times, most recently
from
58c6a66 to
24ec029
Compare
albertsola
force-pushed
the
MPT-19660/Review-duplicated-resource-action
branch
from
24ec029 to
9bdbf50
Compare
|
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.
Closes MPT-19660