MPT-17888: add first-level field annotations to catalog service models#231
Merged
MPT-17888: add first-level field annotations to catalog service models#231
Conversation
Add typed field annotations to all 15 remaining catalog service model classes, following the same pattern established for Parameter and ParameterGroup. All fields are typed as T | None since RQL select can exclude any field from the API response. Models updated: - Authorization, Item, Listing, PriceList, PriceListItem - PricingPolicy, PricingPolicyAttachment, Term, TermVariant - Document, ItemGroup, Media, Template, Product, UnitOfMeasure Each corresponding test file extended with fixture data and tests for: - Primitive field round-trip via to_dict() - Nested fields returning BaseModel instances - Absent fields raising AttributeError (tested with not hasattr) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
albertsola
commented
Mar 13, 2026
Comment on lines
+40
to
+59
| status: str | None | ||
| description: str | None | ||
| reason_for_change: str | None | ||
| unit_lp: float | None | ||
| unit_pp: float | None | ||
| markup: float | None | ||
| margin: float | None | ||
| unit_sp: float | None | ||
| p_px1: float | None | ||
| p_px_m: float | None | ||
| p_px_y: float | None | ||
| s_px1: float | None | ||
| s_px_m: float | None | ||
| s_px_y: float | None | ||
| l_px1: float | None | ||
| l_px_m: float | None | ||
| l_px_y: float | None | ||
| price_list: BaseModel | None | ||
| item: BaseModel | None | ||
| audit: BaseModel | None |
Contributor
Author
There was a problem hiding this comment.
This is not ideal way to transform the Pascal Case into snake case for those variables.
Probably we want to specify a map of variables so we can have better namings.
Contributor
Author
There was a problem hiding this comment.
We could specify a map for the given resource PriceListItem to map the json to attributes as we would like.
The solution will work only if the resource is loaded using PriceListItem, but if it is loaded as side/sub resource by RQL we will have non consisting mapping.
A better solution would be to keep a list of keywords and their mappings. Not great but better unified attribute naming.
@robcsegal any thoughts in here?
Contributor
There was a problem hiding this comment.
Yeah, a list of keywords and their mappings would better. Not sure of a better way right now.
📝 WalkthroughWalkthroughThis pull request introduces explicit field name mappings in the model base class and adds comprehensive public type-annotated attributes to 15 catalog resource models alongside corresponding test coverage verifying field behavior, serialization, and optional field handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment |
…letters Add _FIELD_NAME_MAPPINGS (MappingProxyType) in model.py for API fields that contain two or more consecutive uppercase letters (PPx1, SPxM, unitLP, totalGT, etc.). The generic camelCase<->snake_case regex cannot round-trip these correctly, so an explicit lookup table is checked first in both to_snake_case and to_camel_case. Affected fields from OpenAPI spec: - PP*/SP*/LP* price columns (PPx1→ppx1, SPxM→spxm, LPxY→lpxy, ...) - unit+acronym fields (unitLP→unit_lp, unitPP→unit_pp, unitSP→unit_sp) - total+acronym fields (totalGT→total_gt, totalPP→total_pp, ...) PriceListItem model annotations updated to use the corrected names (ppx1, spxm, lpx1, etc. instead of p_px1, s_px_m, l_px1, ...). to_dict() round-trip now works correctly for all price columns. Tests: - Merged price_list_item price fixture into main fixture for full round-trip test coverage - Added parametrized tests for to_snake_case and to_camel_case with consecutive-uppercase fields in tests/unit/models/test_model.py Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
albertsola
force-pushed
the
MPT-17886_catalog_resources
branch
from
9485e54 to
2c857dd
Compare
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unit/resources/catalog/test_pricing_policies.py (1)
191-199: Add explicit assertion forproductslist element coercion.Since
productsis now part of the typed contract, it’s worth asserting list elements areBaseModelinstances to lock in array-of-object behavior.Suggested test addition
def test_pricing_policy_nested_models(pricing_policy_data): result = PricingPolicy(pricing_policy_data) assert isinstance(result.external_ids, BaseModel) assert isinstance(result.client, BaseModel) assert isinstance(result.eligibility, BaseModel) + assert result.products is not None + assert isinstance(result.products[0], BaseModel) assert isinstance(result.statistics, 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/catalog/test_pricing_policies.py` around lines 191 - 199, The test test_pricing_policy_nested_models should also assert that the PricingPolicy.products field is coerced to a list of BaseModel instances: add assertions that result.products is a list and that every element in result.products satisfies isinstance(element, BaseModel) (e.g., using all(...) or a loop) so the array-of-object behavior is locked in; locate the test function test_pricing_policy_nested_models and the PricingPolicy construction to add these checks.tests/unit/resources/catalog/test_items.py (1)
79-86: Consider addingtermsto nested model assertions.The
Itemmodel hasterms: BaseModel | Noneannotated, but this test only asserts on 5 of the 6 nested BaseModel fields. Addingtermswould ensure complete coverage.♻️ Suggested addition
assert isinstance(result.external_ids, BaseModel) assert isinstance(result.group, BaseModel) assert isinstance(result.unit, BaseModel) + assert isinstance(result.terms, BaseModel) assert isinstance(result.product, 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/catalog/test_items.py` around lines 79 - 86, The test test_item_nested_base_models is missing an assertion for the Item.terms field; update the test to also verify that result.terms is either an instance of BaseModel or None (i.e., assert isinstance(result.terms, BaseModel) or result.terms is None) so the nested-field coverage for Item (the terms attribute) is complete.
🤖 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/catalog/test_items.py`:
- Around line 79-86: The test test_item_nested_base_models is missing an
assertion for the Item.terms field; update the test to also verify that
result.terms is either an instance of BaseModel or None (i.e., assert
isinstance(result.terms, BaseModel) or result.terms is None) so the nested-field
coverage for Item (the terms attribute) is complete.
In `@tests/unit/resources/catalog/test_pricing_policies.py`:
- Around line 191-199: The test test_pricing_policy_nested_models should also
assert that the PricingPolicy.products field is coerced to a list of BaseModel
instances: add assertions that result.products is a list and that every element
in result.products satisfies isinstance(element, BaseModel) (e.g., using
all(...) or a loop) so the array-of-object behavior is locked in; locate the
test function test_pricing_policy_nested_models and the PricingPolicy
construction to add these checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 09f15ab2-a36a-4d1a-a48c-9dbe338bfe31
📒 Files selected for processing (34)
.github/copilot-instructions.mdmpt_api_client/models/model.pympt_api_client/resources/catalog/authorizations.pympt_api_client/resources/catalog/items.pympt_api_client/resources/catalog/listings.pympt_api_client/resources/catalog/price_list_items.pympt_api_client/resources/catalog/price_lists.pympt_api_client/resources/catalog/pricing_policies.pympt_api_client/resources/catalog/pricing_policy_attachments.pympt_api_client/resources/catalog/product_term_variants.pympt_api_client/resources/catalog/product_terms.pympt_api_client/resources/catalog/products.pympt_api_client/resources/catalog/products_documents.pympt_api_client/resources/catalog/products_item_groups.pympt_api_client/resources/catalog/products_media.pympt_api_client/resources/catalog/products_templates.pympt_api_client/resources/catalog/units_of_measure.pyopenapi/openapi-dev.jsontests/unit/models/test_model.pytests/unit/resources/catalog/test_authorizations.pytests/unit/resources/catalog/test_items.pytests/unit/resources/catalog/test_listings.pytests/unit/resources/catalog/test_price_list_items.pytests/unit/resources/catalog/test_price_lists.pytests/unit/resources/catalog/test_pricing_policies.pytests/unit/resources/catalog/test_pricing_policy_attachments.pytests/unit/resources/catalog/test_product_term_variants.pytests/unit/resources/catalog/test_product_terms.pytests/unit/resources/catalog/test_products.pytests/unit/resources/catalog/test_products_documents.pytests/unit/resources/catalog/test_products_item_groups.pytests/unit/resources/catalog/test_products_media.pytests/unit/resources/catalog/test_products_templates.pytests/unit/resources/catalog/test_units_of_measure.py
robcsegal
approved these changes
Mar 13, 2026
d3rky
approved these changes
Mar 13, 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
3 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
Add typed field annotations to all 15 remaining catalog service model classes in
mpt_api_client/resources/catalog/, following the same pattern established forParameterandParameterGroupin #230.All fields are typed as
T | Nonesince RQLselectcan exclude any field from the API response.Models updated
Authorization— 11 fieldsItem— 11 fieldsListing— 10 fieldsPriceList— 10 fieldsPriceListItem— 20 fields (including price columns with non-standard casing)PricingPolicy— 11 fieldsPricingPolicyAttachment— 8 fieldsTerm— 6 fieldsTermVariant— 12 fieldsDocument— 11 fieldsItemGroup— 10 fieldsMedia— 11 fieldsTemplate— 6 fieldsProduct— 11 fieldsUnitOfMeasure— 4 fieldsTests
Each model's test file was extended with:
to_dict()BaseModelinstancesnot hasattr()(model raisesAttributeErrorfor absent fields)Notes
PriceListItemprice columns use non-standard casing (e.g.PPx1→p_px1) due to howto_snake_caseworks — round-trip viato_dict()is lossy for these fields, so only attribute-access tests are used for the price column grouplist[BaseModel] | Noneused for array-of-object fields (Item.parameters,PricingPolicy.products)Closes MPT-17888