Conversation
📝 WalkthroughWalkthroughIntroduces five new shared model classes ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/resources/program/test_programs_documents.py (1)
92-105:⚠️ Potential issue | 🟡 MinorAdd
languageto the nullability assertions.The updated optional-field test still leaves
languageunverified even though this fixture and resource surface include it. That leaves one nullable document field without regression coverage.Suggested fix
assert result.description is None + assert result.language is None assert result.status is None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/resources/program/test_programs_documents.py` around lines 92 - 105, The test test_document_optional_fields_absent is missing an assertion for the optional language field; update the test to include an assertion that the Document created with only {"id": "PDM-001"} has language == None (i.e., add assert result.language is None) so the nullability of the language attribute on the Document resource is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/unit/resources/program/test_programs_documents.py`:
- Around line 92-105: The test test_document_optional_fields_absent is missing
an assertion for the optional language field; update the test to include an
assertion that the Document created with only {"id": "PDM-001"} has language ==
None (i.e., add assert result.language is None) so the nullability of the
language attribute on the Document resource is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 8f493f58-d1f0-4590-a9e5-2e698f033ef6
📒 Files selected for processing (47)
mpt_api_client/models/__init__.pympt_api_client/models/attachment_model.pympt_api_client/models/document_model.pympt_api_client/models/file_resource_model.pympt_api_client/models/media_model.pympt_api_client/models/term_variant_model.pympt_api_client/resources/billing/credit_memo_attachments.pympt_api_client/resources/billing/custom_ledger_attachments.pympt_api_client/resources/billing/invoice_attachments.pympt_api_client/resources/billing/journal_attachments.pympt_api_client/resources/billing/ledger_attachments.pympt_api_client/resources/billing/statement_attachments.pympt_api_client/resources/catalog/pricing_policy_attachments.pympt_api_client/resources/catalog/product_term_variants.pympt_api_client/resources/catalog/products_documents.pympt_api_client/resources/catalog/products_media.pympt_api_client/resources/helpdesk/chat_attachments.pympt_api_client/resources/integration/extension_documents.pympt_api_client/resources/integration/extension_media.pympt_api_client/resources/integration/extension_term_variants.pympt_api_client/resources/program/enrollments_attachments.pympt_api_client/resources/program/programs_documents.pympt_api_client/resources/program/programs_media.pympt_api_client/resources/program/programs_terms_variant.pytests/unit/conftest.pytests/unit/models/test_attachment_model.pytests/unit/models/test_document_model.pytests/unit/models/test_file_resource_model.pytests/unit/models/test_media_model.pytests/unit/models/test_term_variant_model.pytests/unit/resources/billing/test_credit_memo_attachments.pytests/unit/resources/billing/test_custom_ledger_attachments.pytests/unit/resources/billing/test_invoice_attachments.pytests/unit/resources/billing/test_journal_attachments.pytests/unit/resources/billing/test_ledger_attachments.pytests/unit/resources/billing/test_statement_attachments.pytests/unit/resources/catalog/test_pricing_policy_attachments.pytests/unit/resources/catalog/test_product_term_variants.pytests/unit/resources/catalog/test_products_documents.pytests/unit/resources/catalog/test_products_media.pytests/unit/resources/helpdesk/test_chat_attachments.pytests/unit/resources/integration/test_extension_documents.pytests/unit/resources/integration/test_extension_media.pytests/unit/resources/integration/test_extension_term_variants.pytests/unit/resources/program/test_programs_documents.pytests/unit/resources/program/test_programs_media.pytests/unit/resources/program/test_programs_terms_variant.py
jentyk
reviewed
May 1, 2026
AI Generated. Extract FileResourceModel as a shared base for fields common to attachments, documents, media, and term variants (name, type, size, description, content_type). Add AttachmentModel, DocumentModel, MediaModel, and TermVariantModel as specialized subclasses that layer on the fields unique to each resource family. Migrate the corresponding resource classes across billing, catalog, commerce, helpdesk, integration, and program domains to inherit from the new base models, removing duplicated field declarations while keeping resource-specific fields (parent references, audit, language, revision, etc.) on the subclasses. Update unit tests to match the new convention where missing optional fields default to None instead of raising AttributeError, add inherited-field fixtures in tests/unit/conftest.py, and add dedicated test files for each new base model under tests/unit/models/.
robcsegal
force-pushed
the
MPT-20684-update-models-and-base-models-to-reduce-code-duplication
branch
from
3eab7da to
6ed09ac
Compare
Contributor
Author
resolved |
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/resources/program/test_programs_terms_variant.py (1)
103-118:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd targeted suppression for assertion-count rule on Line 103.
test_term_variant_optional_fields_absentnow has many assertions and should match the repo pattern used in adjacent files to avoid lint instability.As per coding guidelines: “add targeted noqa (e.g., WPS218) only when assertion count makes it unavoidable.”Proposed fix
-def test_term_variant_optional_fields_absent() -> None: +def test_term_variant_optional_fields_absent() -> None: # noqa: WPS218🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/resources/program/test_programs_terms_variant.py` around lines 103 - 118, The test function test_term_variant_optional_fields_absent contains many assertions and should include a targeted noqa to suppress the assertion-count lint rule; update the test function definition line (the def test_term_variant_optional_fields_absent(...) declaration) to append a focused noqa for the assertion-count rule used in this repo (e.g., add " # noqa: WPS218") so only that rule is suppressed while leaving other linters active.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/unit/resources/program/test_programs_terms_variant.py`:
- Around line 103-118: The test function
test_term_variant_optional_fields_absent contains many assertions and should
include a targeted noqa to suppress the assertion-count lint rule; update the
test function definition line (the def
test_term_variant_optional_fields_absent(...) declaration) to append a focused
noqa for the assertion-count rule used in this repo (e.g., add " # noqa:
WPS218") so only that rule is suppressed while leaving other linters active.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e8dab837-2336-4c4d-bfd9-303d3712c4bd
📒 Files selected for processing (48)
mpt_api_client/models/__init__.pympt_api_client/models/attachment_model.pympt_api_client/models/document_model.pympt_api_client/models/file_resource_model.pympt_api_client/models/media_model.pympt_api_client/models/term_variant_model.pympt_api_client/resources/billing/credit_memo_attachments.pympt_api_client/resources/billing/custom_ledger_attachments.pympt_api_client/resources/billing/invoice_attachments.pympt_api_client/resources/billing/journal_attachments.pympt_api_client/resources/billing/ledger_attachments.pympt_api_client/resources/billing/statement_attachments.pympt_api_client/resources/catalog/pricing_policy_attachments.pympt_api_client/resources/catalog/product_term_variants.pympt_api_client/resources/catalog/products_documents.pympt_api_client/resources/catalog/products_media.pympt_api_client/resources/helpdesk/chat_attachments.pympt_api_client/resources/integration/extension_documents.pympt_api_client/resources/integration/extension_media.pympt_api_client/resources/integration/extension_term_variants.pympt_api_client/resources/program/enrollments_attachments.pympt_api_client/resources/program/programs_documents.pympt_api_client/resources/program/programs_media.pympt_api_client/resources/program/programs_terms_variant.pytests/e2e/helpdesk/chats/attachment/test_async_attachment.pytests/e2e/helpdesk/chats/attachment/test_sync_attachment.pytests/unit/conftest.pytests/unit/models/test_attachment_model.pytests/unit/models/test_document_model.pytests/unit/models/test_file_resource_model.pytests/unit/models/test_media_model.pytests/unit/models/test_term_variant_model.pytests/unit/resources/billing/test_credit_memo_attachments.pytests/unit/resources/billing/test_custom_ledger_attachments.pytests/unit/resources/billing/test_invoice_attachments.pytests/unit/resources/billing/test_journal_attachments.pytests/unit/resources/billing/test_ledger_attachments.pytests/unit/resources/billing/test_statement_attachments.pytests/unit/resources/catalog/test_pricing_policy_attachments.pytests/unit/resources/catalog/test_product_term_variants.pytests/unit/resources/catalog/test_products_documents.pytests/unit/resources/catalog/test_products_media.pytests/unit/resources/integration/test_extension_documents.pytests/unit/resources/integration/test_extension_media.pytests/unit/resources/integration/test_extension_term_variants.pytests/unit/resources/program/test_programs_documents.pytests/unit/resources/program/test_programs_media.pytests/unit/resources/program/test_programs_terms_variant.py
✅ Files skipped from review due to trivial changes (9)
- mpt_api_client/models/attachment_model.py
- mpt_api_client/models/media_model.py
- mpt_api_client/resources/billing/credit_memo_attachments.py
- mpt_api_client/resources/billing/statement_attachments.py
- mpt_api_client/resources/billing/journal_attachments.py
- mpt_api_client/resources/billing/invoice_attachments.py
- mpt_api_client/resources/billing/custom_ledger_attachments.py
- tests/unit/conftest.py
- mpt_api_client/resources/billing/ledger_attachments.py
🚧 Files skipped from review as they are similar to previous changes (16)
- tests/unit/resources/integration/test_extension_media.py
- mpt_api_client/models/document_model.py
- mpt_api_client/models/file_resource_model.py
- tests/unit/resources/program/test_programs_documents.py
- tests/unit/resources/program/test_programs_media.py
- mpt_api_client/resources/program/enrollments_attachments.py
- mpt_api_client/resources/catalog/pricing_policy_attachments.py
- mpt_api_client/resources/integration/extension_documents.py
- mpt_api_client/resources/program/programs_media.py
- mpt_api_client/resources/helpdesk/chat_attachments.py
- mpt_api_client/resources/catalog/products_documents.py
- mpt_api_client/resources/integration/extension_media.py
- mpt_api_client/resources/program/programs_terms_variant.py
- tests/unit/resources/integration/test_extension_term_variants.py
- tests/unit/resources/integration/test_extension_documents.py
- mpt_api_client/models/term_variant_model.py
robcsegal
deleted the
MPT-20684-update-models-and-base-models-to-reduce-code-duplication
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.
🤖 AI-generated PR — Please review carefully.
Summary
Reduces duplicated field declarations across the model layer by extracting a
shared
FileResourceModelbase for attachments, documents, media, and termvariants, plus four specialized subclasses (
AttachmentModel,DocumentModel,MediaModel,TermVariantModel). Resource classes across billing, catalog,commerce, helpdesk, integration, and program domains now inherit from the new
base models instead of redeclaring the same fields locally.
What changed
New base models (
mpt_api_client/models/)file_resource_model.py— shared fields:name,type,size,description,content_typeattachment_model.py— attachment-specific fields on top of the basedocument_model.py— document-specific fieldsmedia_model.py— media-specific fieldsterm_variant_model.py— term-variant-specific fields__init__.pyre-exports the new classesMigrated resource classes
billing/: credit_memo_attachments, custom_ledger_attachments, invoice_attachments, journal_attachments, ledger_attachments, statement_attachmentscatalog/: pricing_policy_attachments, product_term_variants, products_documents, products_mediahelpdesk/: chat_attachmentsintegration/: extension_documents, extension_media, extension_term_variantsprogram/: enrollments_attachments, programs_documents, programs_media, programs_terms_variantResource-specific fields (parent references, audit, language, revision, etc.)
remain on the subclasses.
Tests
tests/unit/conftest.pyfixtures for inherited-field coveragetests/unit/models/optional fields default to
Noneinstead of raisingAttributeErrorTesting
make check(lint + types)make test(unit suite)response parsing still works against the live API
Jira
MPT-20684
Closes MPT-20684
Release Notes
Introduced shared base models to reduce code duplication for file-like resources:
FileResourceModel: Base class with common fields (name,type,size,description,content_type)AttachmentModel: Specialization ofFileResourceModelfor attachment resourcesDocumentModel: ExtendsFileResourceModelwith document-specific fields (status,filename,url)MediaModel: ExtendsFileResourceModelwith media-specific fields (status,filename,display_order,url)TermVariantModel: ExtendsFileResourceModelwith term variant fields (asset_url,language_code,status,filename,file_id)Updated resource classes across multiple modules to inherit from appropriate base models instead of directly from
Model:CreditMemoAttachment,CustomLedgerAttachment,InvoiceAttachment,JournalAttachment,LedgerAttachment,StatementAttachmentPricingPolicyAttachment,TermVariant,Document,MediaChatAttachmentand updated service to useAttachmentModeldirectlyExtensionDocument,ExtensionMedia,ExtensionTermVariantDocument,Media,TermVariant,EnrollmentAttachmentEliminated duplicate field declarations by migrating resource classes to inherit shared fields from their respective base models
Added comprehensive unit test coverage for new base models including field initialization, camelCase-to-snake_case mapping, serialization round-trips, and
repr()formattingUpdated existing resource tests to validate inherited field behavior with optional fields defaulting to
Noneinstead of being absentUpdated E2E tests for chat attachments to validate against
AttachmentModelinstead of the removedChatAttachmentclassExpanded module exports in
mpt_api_client/models/__init__.pyto include new model classes for public API access