MPT-19057: add first-level field annotations to commerce resource models#233
Merged
albertsola merged 1 commit intomainfrom Mar 16, 2026
Merged
Conversation
📝 WalkthroughWalkthroughCommerce resource models expanded with optional, typed public attributes (many referencing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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. 📝 Coding Plan
Comment Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the |
Add typed field annotations (str | None, bool | None, BaseModel | None, list[BaseModel] | None) to all commerce resource Model subclasses: - Agreement, AgreementAttachment (agreements.py, agreements_attachments.py) - Asset (assets.py) - Order (orders.py) - OrdersAsset (orders_asset.py) - OrderSubscription (orders_subscription.py) - Subscription (subscriptions.py) Fields are derived from openapi-dev.json first-level properties. id is inherited from Model and skipped. $meta is skipped (invalid identifier). camelCase API names are converted to snake_case Python attributes. All fields are | None since no fields appear in the OpenAPI required array. Add corresponding unit tests for each model covering: - primitive field round-trip (to_dict) - nested object fields resolved as BaseModel instances - optional fields absent when not in input data
albertsola
force-pushed
the
MPT-19057/commerce-resource-attributes
branch
from
3500f25 to
9f357b8
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/commerce/test_agreements.py (1)
106-156: Consider asserting list-field element typing as well.The new tests cover nested object fields well, but they don’t validate list-field conversion (
lines,assets,subscriptions,terms_and_conditions,certificates) into nested model items.Proposed test addition
+def test_agreement_nested_list_fields(agreement_data): + result = Agreement(agreement_data) + + assert isinstance(result.lines[0], BaseModel) + assert isinstance(result.assets[0], BaseModel) + assert isinstance(result.subscriptions[0], BaseModel) + assert isinstance(result.terms_and_conditions[0], BaseModel) + assert isinstance(result.certificates[0], BaseModel)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/resources/commerce/test_agreements.py` around lines 106 - 156, Add assertions that list fields are converted to lists of nested models: in the test_agreement_* suite (eg. test_agreement_primitive_fields or create a new test), after creating result = Agreement(agreement_data) assert that result.lines, result.assets, result.subscriptions, result.terms_and_conditions, and result.certificates are lists and that each element is an instance of BaseModel (or the specific model class used), and optionally check length/first-element typing to ensure proper conversion.tests/unit/resources/commerce/test_orders.py (1)
270-340: Add explicit assertions for list-typed nested fields.Current tests validate dict-like nested objects, but they don’t verify conversion of list fields (
lines,subscriptions,assets,terms_and_conditions,certificates) to nested model items.Proposed test addition
+def test_order_nested_list_fields(order_data): + result = Order(order_data) + + assert isinstance(result.lines[0], BaseModel) + assert isinstance(result.subscriptions[0], BaseModel) + assert isinstance(result.assets[0], BaseModel) + assert isinstance(result.terms_and_conditions[0], BaseModel) + assert isinstance(result.certificates[0], BaseModel)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/resources/commerce/test_orders.py` around lines 270 - 340, Add assertions that list-typed nested fields are converted into Python lists of nested models: in the tests (e.g., add a new test or extend test_order_nested_pricing_fields/test_order_nested_billing_fields) instantiate Order(order_data) and assert that result.lines, result.subscriptions, result.assets, result.terms_and_conditions (or termsAndConditions mapping), and result.certificates are lists, that each list has the expected length (>=1), and that each element is a BaseModel with the original id preserved (e.g., element.id == "LIN-001", "SUB-001", "ASS-001", "TAC-001", "CRT-001") to verify conversion from input arrays to nested model instances.tests/unit/resources/commerce/test_assets.py (1)
47-77: Add one assertion for list-field nested conversion (lines).
linesis part of the new typed surface, but current tests don’t verify that list elements are converted to nested models.Proposed test addition
def test_asset_nested_relation_fields(asset_data): # noqa: WPS218 result = Asset(asset_data) @@ assert isinstance(result.listing, BaseModel) assert isinstance(result.licensee, BaseModel) + assert isinstance(result.lines[0], BaseModel) assert isinstance(result.audit, BaseModel)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/resources/commerce/test_assets.py` around lines 47 - 77, Tests omit verifying that the "lines" list is converted to nested models; update the test (e.g., in test_asset_nested_pricing_fields or add to test_asset_nested_relation_fields) to assert that result.lines is a list and that its elements are converted to nested models by checking isinstance(result.lines, list) and isinstance(result.lines[0], BaseModel) (and optionally that result.lines[0].to_dict() matches the original {"id": "LIN-001"}); reference the Asset model and the result variable to locate where to add these assertions.
🤖 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/commerce/test_agreements.py`:
- Around line 106-156: Add assertions that list fields are converted to lists of
nested models: in the test_agreement_* suite (eg.
test_agreement_primitive_fields or create a new test), after creating result =
Agreement(agreement_data) assert that result.lines, result.assets,
result.subscriptions, result.terms_and_conditions, and result.certificates are
lists and that each element is an instance of BaseModel (or the specific model
class used), and optionally check length/first-element typing to ensure proper
conversion.
In `@tests/unit/resources/commerce/test_assets.py`:
- Around line 47-77: Tests omit verifying that the "lines" list is converted to
nested models; update the test (e.g., in test_asset_nested_pricing_fields or add
to test_asset_nested_relation_fields) to assert that result.lines is a list and
that its elements are converted to nested models by checking
isinstance(result.lines, list) and isinstance(result.lines[0], BaseModel) (and
optionally that result.lines[0].to_dict() matches the original {"id":
"LIN-001"}); reference the Asset model and the result variable to locate where
to add these assertions.
In `@tests/unit/resources/commerce/test_orders.py`:
- Around line 270-340: Add assertions that list-typed nested fields are
converted into Python lists of nested models: in the tests (e.g., add a new test
or extend test_order_nested_pricing_fields/test_order_nested_billing_fields)
instantiate Order(order_data) and assert that result.lines,
result.subscriptions, result.assets, result.terms_and_conditions (or
termsAndConditions mapping), and result.certificates are lists, that each list
has the expected length (>=1), and that each element is a BaseModel with the
original id preserved (e.g., element.id == "LIN-001", "SUB-001", "ASS-001",
"TAC-001", "CRT-001") to verify conversion from input arrays to nested model
instances.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 959fb143-2d89-4a62-abaa-4f445f46a034
📒 Files selected for processing (15)
mpt_api_client/resources/commerce/agreements.pympt_api_client/resources/commerce/agreements_attachments.pympt_api_client/resources/commerce/assets.pympt_api_client/resources/commerce/orders.pympt_api_client/resources/commerce/orders_asset.pympt_api_client/resources/commerce/orders_subscription.pympt_api_client/resources/commerce/subscriptions.pytests/unit/resources/commerce/test_agreements.pytests/unit/resources/commerce/test_agreements_attachments.pytests/unit/resources/commerce/test_assets.pytests/unit/resources/commerce/test_order_subcription.pytests/unit/resources/commerce/test_order_subscription.pytests/unit/resources/commerce/test_orders.pytests/unit/resources/commerce/test_orders_asset.pytests/unit/resources/commerce/test_subscriptions.py
💤 Files with no reviewable changes (1)
- tests/unit/resources/commerce/test_order_subcription.py
|
robcsegal
approved these changes
Mar 16, 2026
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/unit/resources/commerce/test_orders_asset.py (1)
71-80: Add one assertion forlinesnested-list conversion.This test already checks dict-backed nested fields; adding a
linesassertion would protect thelist[BaseModel]path too.Suggested diff
def test_orders_asset_nested_fields(orders_asset_data): # noqa: WPS218 result = OrdersAsset(orders_asset_data) assert isinstance(result.external_ids, BaseModel) assert isinstance(result.price, BaseModel) assert isinstance(result.template, BaseModel) assert isinstance(result.parameters, BaseModel) assert isinstance(result.terms, BaseModel) assert isinstance(result.audit, BaseModel) + assert all(isinstance(item, BaseModel) for item in result.lines)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/resources/commerce/test_orders_asset.py` around lines 71 - 80, Add an assertion in the test_orders_asset_nested_fields test to verify that the OrdersAsset.lines field is converted into a list of BaseModel instances: locate the test function test_orders_asset_nested_fields and after the existing isinstance checks add an assertion that checks isinstance(result.lines, list) and that each element is an instance of BaseModel (i.e., iterate result.lines or assert isinstance(result.lines[0], BaseModel)) so the list[BaseModel] conversion path for OrdersAsset.lines is covered.tests/unit/resources/commerce/test_assets.py (1)
68-76: Add explicit coverage forlineslist element typing.A single assertion here would verify the list-conversion branch, not just dict-backed nested fields.
Suggested diff
def test_asset_nested_relation_fields(asset_data): # noqa: WPS218 result = Asset(asset_data) assert isinstance(result.agreement, BaseModel) assert isinstance(result.product, BaseModel) assert isinstance(result.price_list, BaseModel) assert isinstance(result.listing, BaseModel) assert isinstance(result.licensee, BaseModel) assert isinstance(result.audit, BaseModel) + assert all(isinstance(item, BaseModel) for item in result.lines)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/resources/commerce/test_assets.py` around lines 68 - 76, The test_asset_nested_relation_fields test currently checks nested dict-backed fields but misses verifying that list elements (specifically the lines attribute) are converted to model instances; update test_asset_nested_relation_fields to also assert that result.lines is a list and that each element (e.g., result.lines[0]) is an instance of BaseModel (or the specific model type expected) to cover the list-conversion branch of Asset.tests/unit/resources/commerce/test_agreements.py (1)
128-156: Consider asserting list-backed nested fields asBaseModelelements.You validate many nested dict fields already; adding list-element assertions would harden coverage for all new list annotations.
Suggested diff
def test_agreement_nested_identity_fields(agreement_data): # noqa: WPS218 result = Agreement(agreement_data) assert isinstance(result.seller, BaseModel) assert isinstance(result.product, BaseModel) assert isinstance(result.external_ids, BaseModel) assert isinstance(result.split, BaseModel) assert isinstance(result.audit, BaseModel) + assert all(isinstance(item, BaseModel) for item in result.lines) + assert all(isinstance(item, BaseModel) for item in result.assets) + assert all(isinstance(item, BaseModel) for item in result.subscriptions) + assert all(isinstance(item, BaseModel) for item in result.terms_and_conditions) + assert all(isinstance(item, BaseModel) for item in result.certificates)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/resources/commerce/test_agreements.py` around lines 128 - 156, Add assertions that verify list-backed nested fields contain BaseModel elements: update the tests (test_agreement_nested_relation_fields and test_agreement_nested_identity_fields) to locate any Agreement attributes that are lists (e.g., parameters, external_ids, split — check Agreement model annotations) and assert isinstance(result.<field>[0], BaseModel) for each list-backed field instead of only asserting the container; ensure you guard for empty lists if necessary.
🤖 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/commerce/test_agreements.py`:
- Around line 128-156: Add assertions that verify list-backed nested fields
contain BaseModel elements: update the tests
(test_agreement_nested_relation_fields and
test_agreement_nested_identity_fields) to locate any Agreement attributes that
are lists (e.g., parameters, external_ids, split — check Agreement model
annotations) and assert isinstance(result.<field>[0], BaseModel) for each
list-backed field instead of only asserting the container; ensure you guard for
empty lists if necessary.
In `@tests/unit/resources/commerce/test_assets.py`:
- Around line 68-76: The test_asset_nested_relation_fields test currently checks
nested dict-backed fields but misses verifying that list elements (specifically
the lines attribute) are converted to model instances; update
test_asset_nested_relation_fields to also assert that result.lines is a list and
that each element (e.g., result.lines[0]) is an instance of BaseModel (or the
specific model type expected) to cover the list-conversion branch of Asset.
In `@tests/unit/resources/commerce/test_orders_asset.py`:
- Around line 71-80: Add an assertion in the test_orders_asset_nested_fields
test to verify that the OrdersAsset.lines field is converted into a list of
BaseModel instances: locate the test function test_orders_asset_nested_fields
and after the existing isinstance checks add an assertion that checks
isinstance(result.lines, list) and that each element is an instance of BaseModel
(i.e., iterate result.lines or assert isinstance(result.lines[0], BaseModel)) so
the list[BaseModel] conversion path for OrdersAsset.lines is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 8699d423-29b8-4ae1-a323-363cf3461f28
📒 Files selected for processing (15)
mpt_api_client/resources/commerce/agreements.pympt_api_client/resources/commerce/agreements_attachments.pympt_api_client/resources/commerce/assets.pympt_api_client/resources/commerce/orders.pympt_api_client/resources/commerce/orders_asset.pympt_api_client/resources/commerce/orders_subscription.pympt_api_client/resources/commerce/subscriptions.pytests/unit/resources/commerce/test_agreements.pytests/unit/resources/commerce/test_agreements_attachments.pytests/unit/resources/commerce/test_assets.pytests/unit/resources/commerce/test_order_subcription.pytests/unit/resources/commerce/test_order_subscription.pytests/unit/resources/commerce/test_orders.pytests/unit/resources/commerce/test_orders_asset.pytests/unit/resources/commerce/test_subscriptions.py
💤 Files with no reviewable changes (1)
- tests/unit/resources/commerce/test_order_subcription.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/resources/commerce/test_agreements_attachments.py
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.
Add typed field annotations (str | None, bool | None, BaseModel | None, list[BaseModel] | None) to all commerce resource Model subclasses:
Fields are derived from openapi-dev.json first-level properties. id is inherited from Model and skipped. $meta is skipped (invalid identifier). camelCase API names are converted to snake_case Python attributes. All fields are | None since no fields appear in the OpenAPI required array.
Add corresponding unit tests for each model covering:
Closes MPT-19057
Release Notes