MPT-16255 Seeding for catalog #152
Conversation
WalkthroughRefactors seeding: replaces legacy default DI with container-provided wiring, adds helper utilities (require_context_id, init_resource), introduces price_list/authorization/listing seeds, rewrites product creation into create_* helpers, removes obsolete item/parameter modules, and converts seeding from concurrent tasks to sequential execution. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant seed_api
participant AccountsSeed as seed.accounts
participant CatalogSeed as seed.catalog
participant ProductSeed as seed.catalog.product
participant Helper as seed.helper
participant Container
User->>seed_api: seed_api(context)
activate seed_api
seed_api->>AccountsSeed: await seed_accounts(context)
activate AccountsSeed
AccountsSeed->>Container: resolve Provide[Container.*]
AccountsSeed-->>seed_api: accounts seeded
deactivate AccountsSeed
seed_api->>CatalogSeed: await seed_catalog(context)
activate CatalogSeed
CatalogSeed->>ProductSeed: await seed_product()
activate ProductSeed
ProductSeed->>Container: Provide[Container.mpt_vendor/context]
ProductSeed-->>CatalogSeed: product created (id cached)
deactivate ProductSeed
CatalogSeed->>Helper: require_context_id("catalog.product.id")
Helper-->>CatalogSeed: returns product id
CatalogSeed->>CatalogSeed: await seed_authorization()
CatalogSeed->>Container: Provide[Container.mpt_operations/context]
CatalogSeed-->>CatalogSeed: authorization created (id cached)
CatalogSeed->>CatalogSeed: await seed_price_list()
CatalogSeed->>Helper: require_context_id("catalog.product.id")
CatalogSeed-->>CatalogSeed: price_list created (id cached)
CatalogSeed->>CatalogSeed: await seed_listing()
CatalogSeed->>Helper: require_context_id("catalog.authorization.id","catalog.price_list.id")
CatalogSeed-->>CatalogSeed: listing created (id cached)
CatalogSeed-->>seed_api: catalog seeded
deactivate CatalogSeed
seed_api-->>User: Done
deactivate seed_api
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
✅ Found Jira issue key in the title: MPT-16255 Tests mirroring check (created files only)
|
albertsola
force-pushed
the
MPT-16255/Seeding-for-catalog
branch
7 times, most recently
from
ce6249e to
784510a
Compare
albertsola
requested review from
alephsur,
d3rky,
jentyk,
robcsegal,
ruben-sebrango and
svazquezco
as code owners
|
@robcsegal it looks like I need a seller id before creating the authorization: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
seed/context.py (1)
36-50: Updateload_contextdocstring to match function signatureThe docstring for
load_contextstill claims the function returns aContextinstance, but the function signature is-> None. The function correctly mutates the passedContextin place.Update the docstring to:
- Remove the "Returns" section
- Clarify that the function updates the context in place
Example:
def load_context(json_file: pathlib.Path, context: Context) -> None: """Load JSON data from a file into an existing Context in place. Args: json_file: JSON file path. context: Context instance to be updated. """Verification confirms no callers currently depend on a return value; all call sites use the function for its side effects only.
seed/accounts/licensee.py (1)
38-74: Unreachable code:group is Nonecheck will never be true.The
context.get_resource()method (fromseed/context.pylines 19-27) either returns aModelinstance or raisesValueError— it never returnsNone. The check on line 53 is therefore unreachable.Consider either:
- Wrapping the call in a try/except to handle
ValueError- Removing the None check if the ValueError is expected to propagate
- group = context.get_resource("accounts.user_group") - if group is None: - raise ValueError("User group is required in context") + try: + group = context.get_resource("accounts.user_group") + except ValueError: + raise ValueError("User group is required in context")
🧹 Nitpick comments (3)
seed/assets/assets.py (1)
1-7: Static asset path definitions look fineThe pathlib-based constants are straightforward and safe. If ICON and LOGO are intentionally the same file, this is fine; if they’re conceptually distinct, consider pointing LOGO to a separate asset later.
seed/catalog/catalog.py (1)
11-17: Docstring no longer matches whatseed_catalogactually seedsThe implementation now seeds product, authorization, price list, and listing in sequence, but the docstring still mentions "item groups, and parameters". Consider updating it to reflect the current behavior; the sequential ordering itself looks consistent with the downstream dependencies.
-async def seed_catalog() -> None: - """Seed catalog data including products, item groups, and parameters.""" +async def seed_catalog() -> None: + """Seed catalog data including products, authorizations, price lists, and listings."""seed/catalog/listing.py (1)
15-15: Remove unusednoqadirective.The static analysis tool indicates that
WPS210is not a recognized rule in the current linter configuration. Remove the unused directive.-async def create_listing( # noqa: WPS210 +async def create_listing(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
seed/assets/FIL-9920-4780-9379.pngis excluded by!**/*.pngseed/assets/empty.pdfis excluded by!**/*.pdfseed/assets/logo.pngis excluded by!**/*.png
📒 Files selected for processing (32)
mpt_api_client/resources/catalog/products.py(2 hunks)pyproject.toml(1 hunks)seed/accounts/api_tokens.py(3 hunks)seed/accounts/buyer.py(3 hunks)seed/accounts/licensee.py(4 hunks)seed/accounts/module.py(2 hunks)seed/accounts/seller.py(2 hunks)seed/accounts/user_group.py(3 hunks)seed/assets/assets.py(1 hunks)seed/catalog/authorization.py(1 hunks)seed/catalog/catalog.py(1 hunks)seed/catalog/item.py(7 hunks)seed/catalog/item_group.py(4 hunks)seed/catalog/listing.py(1 hunks)seed/catalog/price_list.py(1 hunks)seed/catalog/product.py(1 hunks)seed/catalog/product_parameters.py(0 hunks)seed/catalog/product_parameters_group.py(0 hunks)seed/container.py(1 hunks)seed/context.py(1 hunks)seed/defaults.py(0 hunks)seed/helper.py(1 hunks)seed/seed_api.py(1 hunks)tests/seed/catalog/test_authorization.py(1 hunks)tests/seed/catalog/test_catalog.py(1 hunks)tests/seed/catalog/test_listing.py(1 hunks)tests/seed/catalog/test_price_list.py(1 hunks)tests/seed/catalog/test_product.py(2 hunks)tests/seed/catalog/test_product_parameters.py(0 hunks)tests/seed/catalog/test_product_parameters_group.py(0 hunks)tests/seed/test_context.py(1 hunks)tests/seed/test_helper.py(1 hunks)
💤 Files with no reviewable changes (5)
- seed/catalog/product_parameters.py
- seed/catalog/product_parameters_group.py
- tests/seed/catalog/test_product_parameters_group.py
- tests/seed/catalog/test_product_parameters.py
- seed/defaults.py
🧰 Additional context used
🧬 Code graph analysis (19)
seed/helper.py (3)
seed/container.py (1)
Container(7-32)seed/context.py (2)
Context(12-33)get_string(15-17)mpt_api_client/models/model.py (1)
id(119-121)
seed/catalog/catalog.py (4)
seed/catalog/authorization.py (1)
seed_authorization(12-14)seed/catalog/listing.py (1)
seed_listing(10-12)seed/catalog/price_list.py (1)
seed_price_list(10-12)seed/catalog/product.py (1)
seed_product(39-51)
seed/seed_api.py (4)
seed/accounts/accounts.py (1)
seed_accounts(13-23)seed/catalog/catalog.py (1)
seed_catalog(11-19)seed/container.py (1)
Container(7-32)seed/context.py (3)
Context(12-33)load_context(36-50)save_context(53-61)
seed/accounts/buyer.py (6)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/buyers.py (1)
Buyer(26-27)seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)tests/e2e/conftest.py (1)
logger(63-64)
seed/accounts/seller.py (6)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/sellers.py (1)
Seller(13-14)seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)tests/e2e/conftest.py (1)
logger(63-64)
tests/seed/catalog/test_catalog.py (5)
seed/catalog/catalog.py (1)
seed_catalog(11-19)seed/catalog/product.py (1)
seed_product(39-51)seed/catalog/authorization.py (1)
seed_authorization(12-14)seed/catalog/price_list.py (1)
seed_price_list(10-12)seed/catalog/listing.py (1)
seed_listing(10-12)
seed/accounts/user_group.py (6)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/user_groups.py (1)
UserGroup(11-12)seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)tests/e2e/conftest.py (1)
logger(63-64)
seed/accounts/licensee.py (6)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/licensees.py (1)
Licensee(21-22)seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)tests/e2e/conftest.py (1)
mpt_client(43-44)
seed/catalog/authorization.py (4)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)seed/container.py (1)
Container(7-32)seed/context.py (1)
Context(12-33)seed/helper.py (2)
init_resource(43-83)require_context_id(23-40)
tests/seed/catalog/test_listing.py (3)
seed/catalog/listing.py (2)
create_listing(15-45)seed_listing(10-12)tests/seed/conftest.py (1)
context(14-15)seed/context.py (2)
Context(12-33)get_string(15-17)
tests/seed/catalog/test_price_list.py (5)
seed/catalog/price_list.py (2)
create_price_list(15-29)seed_price_list(10-12)tests/seed/conftest.py (2)
context(14-15)operations_client(24-25)seed/context.py (2)
Context(12-33)get_string(15-17)tests/seed/catalog/test_product.py (1)
context_with_product(26-29)mpt_api_client/models/model.py (1)
id(119-121)
seed/catalog/price_list.py (5)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/catalog/price_lists.py (1)
PriceList(15-16)seed/container.py (1)
Container(7-32)seed/context.py (1)
Context(12-33)seed/helper.py (2)
init_resource(43-83)require_context_id(23-40)
seed/accounts/api_tokens.py (5)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/api_tokens.py (1)
ApiToken(12-13)seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)
tests/seed/test_helper.py (3)
tests/seed/conftest.py (1)
context(14-15)seed/context.py (2)
Context(12-33)get_string(15-17)seed/helper.py (3)
ResourceRequiredError(13-20)init_resource(43-83)require_context_id(23-40)
seed/catalog/item.py (2)
seed/container.py (1)
Container(7-32)seed/context.py (1)
Context(12-33)
tests/seed/catalog/test_product.py (2)
seed/catalog/product.py (10)
create_document(161-177)create_item_group(180-197)create_parameter(133-158)create_parameter_group(118-130)create_product(28-36)create_product_item(226-254)create_template(91-103)create_terms(106-115)create_terms_variant(67-88)create_unit_of_measure(200-223)seed/context.py (1)
Context(12-33)
seed/accounts/module.py (4)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)
tests/seed/catalog/test_authorization.py (3)
seed/catalog/authorization.py (2)
create_authorization(17-38)seed_authorization(12-14)tests/seed/conftest.py (1)
context(14-15)seed/context.py (2)
Context(12-33)get_string(15-17)
mpt_api_client/resources/catalog/products.py (2)
mpt_api_client/http/mixins.py (4)
CreateFileMixin(115-144)UpdateFileMixin(147-184)AsyncCreateFileMixin(187-216)AsyncUpdateFileMixin(219-256)mpt_api_client/resources/catalog/mixins.py (2)
PublishableMixin(11-45)AsyncPublishableMixin(48-82)
🪛 Ruff (0.14.8)
seed/seed_api.py
20-20: Unused noqa directive (unknown: WPS229)
Remove unused noqa directive
(RUF100)
seed/catalog/authorization.py
12-12: Unused function argument: context
(ARG001)
tests/seed/catalog/test_listing.py
18-18: Unused noqa directive (unknown: WPS218)
Remove unused noqa directive
(RUF100)
seed/catalog/listing.py
15-15: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
tests/seed/catalog/test_product.py
4-4: Unused noqa directive (unknown: WPS235)
Remove unused noqa directive
(RUF100)
32-32: Unused noqa directive (unknown: WPS211)
Remove unused noqa directive
(RUF100)
33-33: Unused function argument: context
(ARG001)
seed/catalog/product.py
54-54: Unused function argument: context
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (47)
tests/seed/test_context.py (1)
7-17: Test correctly captures in-place context merge semanticsThe test accurately verifies that
load_contextoverwrites existing keys, preserves untouched ones, and adds new keys on the providedContextinstance.pyproject.toml (1)
113-139: Flake8 per-file ignores align with new seed and test modulesThe added ignore entries for
tests/seed/...andseed/...files look consistent with the new seeding code and only affect linting, not behavior.mpt_api_client/resources/catalog/products.py (1)
67-75: Generic mixins parameterized withProductimprove type safetyUsing
CreateFileMixin[Product],UpdateFileMixin[Product], andPublishableMixin[Product](and their async variants) aligns the mixins’ generics with_model_class = Productand keepsupdate_settingsand other methods correctly typed without changing runtime behavior.Also applies to: 130-139
tests/seed/catalog/test_catalog.py (1)
3-13: Orchestration test forseed_catalogmatches new seeding flowPatching the four seed functions and asserting each is called once is a concise way to validate the updated
seed_catalogorchestration (product → authorization → price list → listing).Also applies to: 17-20
seed/container.py (1)
45-47: Consolidated wiring viapackages=["seed"]is correctly implementedUsing
container.wire(packages=["seed"])correctly covers all seed modules with a simpler approach than wiring individually. The call towire_container()at the start ofmain()ensures wiring happens beforeseed_api()and any DI-wired functions are invoked.tests/seed/test_helper.py (1)
1-50: Good coverage for helper behaviorsTests thoroughly exercise
require_context_id(success and error cases) andinit_resource(existing id vs creation and caching). No changes needed here.tests/seed/catalog/test_price_list.py (1)
7-47: Price list tests correctly exercise create and seed pathsTests cover payload construction, skip behavior on existing id, and context mutation on creation for
seed_price_list. They align with the DI-based implementation; no changes needed.tests/seed/catalog/test_authorization.py (1)
7-53: Authorization tests look good but depend onseed_authorizationusing the passed contextThese tests correctly validate payload composition, skip behavior when an id already exists, and context mutation when seeding. To make them pass as intended,
seed_authorizationmust forward itscontextargument intoinit_resource(see comment inseed/catalog/authorization.py); otherwise, the seeding will operate on the DI-provided context instance instead ofcontext_with_data.seed/catalog/authorization.py (1)
17-38: Authorization payload and DI usage look consistent
create_authorizationcorrectly pullscatalog.product.id,accounts.seller.id, andaccounts.account.idfrom the context, generates a short UUID suffix, and builds a reasonable payload foroperations.catalog.authorizations.create. Aside from theseed_authorizationcontext forwarding issue above, the DI setup and payload structure look good.tests/seed/catalog/test_listing.py (1)
18-18: Remove obsolete# noqa: WPS218Ruff reports this
noqaas unused. Since there’s no WPS218 violation anymore, you can drop the directive:-async def test_create_listing(mocker, operations_client, context_with_data): # noqa: WPS218 +async def test_create_listing(mocker, operations_client, context_with_data):⛔ Skipped due to learnings
Learnt from: jentyk Repo: softwareone-platform/mpt-api-python-client PR: 136 File: tests/e2e/notifications/subscribers/conftest.py:14-25 Timestamp: 2025-12-01T10:48:52.586Z Learning: The repository softwareone-platform/mpt-api-python-client uses wemake-python-styleguide linter, so noqa directives for WPS rules should not be flagged as unused.seed/seed_api.py (1)
20-20: Drop unused# noqa: WPS229commentRuff flags WPS229 as an unknown code; the suppression is no longer needed and can be removed:
- try: # noqa: WPS229 + try:⛔ Skipped due to learnings
Learnt from: jentyk Repo: softwareone-platform/mpt-api-python-client PR: 136 File: tests/e2e/notifications/subscribers/conftest.py:14-25 Timestamp: 2025-12-01T10:48:52.586Z Learning: The repository softwareone-platform/mpt-api-python-client uses wemake-python-styleguide linter, so noqa directives for WPS rules should not be flagged as unused.seed/accounts/api_tokens.py (5)
4-9: LGTM! DI imports are correctly structured.The imports for
Provide,inject, andContainerare properly organized and align with the dependency injection pattern used across the seed modules.
14-32: LGTM! DI wiring forget_api_tokenis correct.The function properly uses
Provide[Container.context]andProvide[Container.mpt_operations]with the@injectdecorator, allowing both DI resolution and explicit parameter passing when needed.
35-48: LGTM! DI wiring forbuild_api_token_datais correct.The synchronous function correctly uses
@injectwithProvide[Container.context]for the context parameter.
51-67: LGTM! DI wiring forinit_api_tokenis correct.The function properly passes explicit arguments to
get_api_tokenandbuild_api_token_data, ensuring consistent context and client usage throughout the call chain.
70-75: LGTM!seed_api_tokencorrectly relies on DI.The parameterless function properly delegates to
init_api_token(), letting the DI container resolve dependencies.seed/accounts/seller.py (5)
5-10: LGTM! DI imports correctly added.The imports align with the container-based DI pattern.
15-33: LGTM! DI wiring forget_selleris correct.The function properly uses container-provided dependencies with consistent naming (
mpt_operations).
36-52: LGTM!build_seller_dataappropriately has no DI dependencies.This function only needs an optional
external_idparameter and correctly doesn't require context or client injection.
55-74: LGTM! DI wiring forinit_selleris correct.The function properly forwards DI-resolved dependencies to
get_sellerand usesbuild_seller_data()without context (as it doesn't need it).
77-82: LGTM!seed_sellercorrectly relies on DI.The parameterless function properly delegates to
init_seller().seed/accounts/module.py (4)
3-9: LGTM! DI imports correctly structured.The imports align with the container-based dependency injection pattern.
14-32: LGTM! DI wiring forget_moduleis correct.The function properly uses container-provided dependencies.
35-57: LGTM! DI wiring forrefresh_moduleis correct.The function properly forwards DI-resolved dependencies to
get_moduleand uses the async iteration pattern correctly for filtering modules.
60-72: LGTM!seed_modulecorrectly relies on DI.The function properly calls the injected
get_module()andrefresh_module()without parameters, letting DI resolve dependencies. The error handling withValueErrorfor missing modules is appropriate.seed/accounts/user_group.py (5)
5-10: LGTM! DI imports correctly added.The imports align with the container-based DI pattern.
15-33: LGTM! DI wiring forget_user_groupis correct.The function properly uses container-provided dependencies with consistent naming.
36-52: LGTM! DI wiring forbuild_user_group_datais correct.The function correctly uses
Provide[Container.context]to accessmodule_idfrom context.
55-76: LGTM! DI wiring forinit_user_groupis correct.The function properly forwards DI-resolved dependencies to
get_user_groupandbuild_user_group_data.
79-85: LGTM!seed_user_groupcorrectly relies on DI.The parameterless function properly delegates to
init_user_group().seed/accounts/licensee.py (4)
5-10: LGTM! DI imports correctly added.The imports align with the container-based DI pattern.
77-97: LGTM! DI wiring forinit_licenseeis correct.The function properly forwards DI-resolved dependencies to
get_licenseeandbuild_licensee_data.
100-105: LGTM!seed_licenseecorrectly relies on DI.The parameterless function properly delegates to
init_licensee().
17-35: LGTM! DI wiring forget_licenseeis correct.The function consistently uses
mpt_clientacross all licensee operations (bothget_licenseeandget_or_create_licensee), distinguishing it from other account modules (user_group,seller,buyer,module) which usempt_operations. This intentional separation appears appropriate for licensee operations.seed/catalog/listing.py (1)
10-12: LGTM!The
seed_listingandcreate_listingfunctions follow the established DI pattern consistently. The payload construction properly retrieves all required IDs from context before building the listing data.Also applies to: 26-45
seed/accounts/buyer.py (3)
17-35: LGTM!The
get_buyerfunction properly handles the case where the buyer is not in context, fetching from API and caching in context.
38-63: LGTM!The
build_buyer_datafunction has appropriate validation for required environment variable and context dependency.
66-94: LGTM!The
init_buyerandseed_buyerfunctions follow the established pattern. The icon file handling with explicit file open is appropriate.seed/catalog/item_group.py (3)
14-32: LGTM!The
get_item_groupfunction properly handles caching logic and API fetch fallback.
35-59: LGTM!The
set_item_groupandbuild_item_groupfunctions are clean and follow the established DI pattern.
62-87: LGTM!The
init_item_groupandseed_item_groupfunctions correctly orchestrate the get-or-create pattern with appropriate logging.seed/catalog/item.py (4)
14-48: LGTM!The
refresh_itemandget_itemfunctions properly handle caching and API fetch patterns.
51-72: LGTM!The
build_itemfunction correctly retrieves required IDs from context and constructs a complete item payload.
75-131: LGTM!The
create_item,review_item,publish_item, andseed_itemsfunctions follow the established DI pattern and correctly orchestrate the item lifecycle (create → review → publish).
62-64: The hardcoded unit ID"UNT-1229"is test fixture data used consistently across seed files and test configurations. Externalizing static test IDs is unnecessary and adds complexity without benefit. This pattern is acceptable for test fixtures.Likely an incorrect or invalid review comment.
tests/seed/catalog/test_product.py (2)
43-64: LGTM!The seed_product orchestration tests correctly verify both the skip scenario (when product already exists in context) and the full sequence scenario (when product needs to be created). The mocking and assertions appropriately validate the conditional execution flow.
67-177: LGTM!All
create_*function tests follow a consistent and appropriate pattern:
- Mock the relevant client methods
- Provide necessary context (product.id, terms.id, etc.)
- Verify return values and call expectations
The payload verification in
test_create_item_group(lines 148-150) andtest_create_product_item(lines 173-177) is particularly valuable, ensuring that the functions correctly construct payloads with required associations (product id, unit, group).
| publish_product, | ||
| review_product, | ||
| from mpt_api_client.resources.catalog.products import Product | ||
| from seed.catalog.product import ( # noqa: WPS235 |
There was a problem hiding this comment.
Remove unused noqa directives.
The noqa directives for WPS235 (line 4) and WPS211 (line 32) are not recognized by the current linter and should be removed.
Apply this diff:
-from seed.catalog.product import ( # noqa: WPS235
+from seed.catalog.product import (
create_document,-async def test_create_product( # noqa: WPS211
+async def test_create_product(
mocker, context: Context, vendor_client, productAlso applies to: 32-32
🧰 Tools
🪛 Ruff (0.14.8)
4-4: Unused noqa directive (unknown: WPS235)
Remove unused noqa directive
(RUF100)
🤖 Prompt for AI Agents
In tests/seed/catalog/test_product.py around lines 4 and 32, remove the unused
noqa directives: delete "noqa: WPS235" on the import at line 4 and delete "noqa:
WPS211" at line 32; keep the imports and surrounding code unchanged and ensure
the file still passes the linter and tests after removing these unsupported noqa
comments.
albertsola
force-pushed
the
MPT-16255/Seeding-for-catalog
branch
from
784510a to
f82b0c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
seed/accounts/licensee.py (1)
52-54: Dead code:if group is Noneis unreachable.
context.get_resource()raisesValueErrorwhen the resource is not found (seeseed/context.pylines 17-32). It never returnsNone, so the check on line 53 is unreachable.group = context.get_resource("accounts.user_group") - if group is None: - raise ValueError("User group is required in context")The
get_resourcecall already raisesValueErrorwith a similar message if the resource is missing.
♻️ Duplicate comments (18)
seed/seed_api.py (1)
20-22: CRITICAL: Reverse seeding order to fix the reported error.The current order (
seed_catalog()beforeseed_accounts()) causes the exactResourceRequiredError: Missing required resource 'accounts.seller.id'that you reported in the PR comments.create_authorization(called during catalog seeding) requiresaccounts.seller.idandaccounts.account.id, which are only populated byseed_accounts().Apply this diff to fix the order:
try: # noqa: WPS229 - await seed_catalog() await seed_accounts() + await seed_catalog()Note: The
noqa: WPS229directive on line 20 is for an unknown rule and can be removed per Ruff's suggestion.tests/seed/catalog/test_listing.py (1)
7-15: Update context keys to match catalog namespace.The fixture uses
accounts.authorization.idandaccounts.price_list.id, butseed/catalog/authorization.pyandseed/catalog/price_list.pystore tocatalog.authorization.idandcatalog.price_list.idrespectively. This mismatch will causeResourceRequiredErrorwhen running the integrated seeding flow.Apply this diff to align with the catalog namespace:
ctx["catalog.product.id"] = "prod-123" ctx["accounts.seller.id"] = "seller-456" - ctx["accounts.authorization.id"] = "auth-789" + ctx["catalog.authorization.id"] = "auth-789" ctx["accounts.account.id"] = "acct-321" - ctx["accounts.price_list.id"] = "pl-654" + ctx["catalog.price_list.id"] = "pl-654"seed/catalog/price_list.py (1)
15-29: Fix copy-paste error in action string.Line 20 uses
"Create authorization"as the action parameter, which would produce misleading error messages. It should reference the actual operation.- product_id = require_context_id(context, "catalog.product.id", "Create authorization") + product_id = require_context_id(context, "catalog.product.id", "Create price list")seed/catalog/authorization.py (1)
12-14: Forward context parameter to init_resource.The
contextparameter is declared but not passed toinit_resource, causing the function to always use the DI-provided singleton instead of the explicit context provided by callers/tests.async def seed_authorization(context: Context = Provide[Container.context]) -> None: """Seed authorization.""" - await init_resource("catalog.authorization.id", create_authorization) + await init_resource("catalog.authorization.id", create_authorization, context)seed/helper.py (1)
43-47: Missing@injectdecorator and docstring issues remain unaddressed.The issues identified in the previous review are still present:
- The function uses
Provide[Container.context](line 46) but lacks the@injectdecorator, causing the rawProvidemarker to be passed whencontextis omitted.- Line 35 documents
RuntimeErrorbut line 39 raisesResourceRequiredError.- Line 62 references non-existent
DEFAULT_CONTEXT.seed/catalog/listing.py (1)
21-21: Fix inconsistent operation description.The operation description says "Create authorization" but this is for creating a listing. This appears to be a copy-paste error.
- seller_id = require_context_id(context, "accounts.seller.id", "Create authorization") + seller_id = require_context_id(context, "accounts.seller.id", "Create listing")seed/catalog/product.py (11)
39-51: Missing@injectdecorator will prevent DI resolution.The
seed_productfunction usesProvide[Container.context]but lacks the@injectdecorator required for DI resolution.+@inject async def seed_product( context: Context = Provide[Container.context], ) -> None:
54-64: Unusedcontextparameter and missing@injectdecorator.The
contextparameter is declared but never used in this function. Since the parameter is unused, remove it entirely rather than adding@inject.-async def seed_product_child_resources(context: Context = Provide[Container.context]) -> None: +async def seed_product_child_resources() -> None: """Seed product child resources."""
67-88: Missing@injectdecorator oncreate_terms_variant.This function uses
Provide[Container.*]but lacks the@injectdecorator required for DI resolution.+@inject async def create_terms_variant( context: Context = Provide[Container.context], mpt_vendor: AsyncMPTClient = Provide[Container.mpt_vendor], ) -> TermVariant:
91-103: Missing@injectdecorator oncreate_template.+@inject async def create_template( context: Context = Provide[Container.context], mpt_vendor: AsyncMPTClient = Provide[Container.mpt_vendor], ) -> Template:
106-115: Missing@injectdecorator oncreate_terms.+@inject async def create_terms( context: Context = Provide[Container.context], mpt_vendor: AsyncMPTClient = Provide[Container.mpt_vendor], ) -> Term:
118-130: Missing@injectdecorator oncreate_parameter_group.+@inject async def create_parameter_group( context: Context = Provide[Container.context], mpt_vendor: AsyncMPTClient = Provide[Container.mpt_vendor], ) -> ParameterGroup:
133-158: Missing@injectdecorator oncreate_parameter.+@inject async def create_parameter( context: Context = Provide[Container.context], mpt_vendor: AsyncMPTClient = Provide[Container.mpt_vendor], ) -> Parameter:
161-177: Missing@injectdecorator oncreate_document.+@inject async def create_document( context: Context = Provide[Container.context], mpt_vendor: AsyncMPTClient = Provide[Container.mpt_vendor], ) -> Document:
180-197: Missing@injectdecorator oncreate_item_group.+@inject async def create_item_group( context: Context = Provide[Container.context], mpt_vendor: AsyncMPTClient = Provide[Container.mpt_vendor], ) -> ItemGroup:
200-208: Missing@injectdecorator oncreate_unit_of_measure.+@inject async def create_unit_of_measure( operations: AsyncMPTClient = Provide[Container.mpt_operations], ) -> UnitOfMeasure:
211-239: Missing@injectdecorator oncreate_product_item.+@inject async def create_product_item( context: Context = Provide[Container.context], mpt_vendor: AsyncMPTClient = Provide[Container.mpt_vendor], ) -> Item:tests/seed/catalog/test_product.py (1)
32-40: Remove unusedcontextparameter.The
contextfixture is declared but never used in this test function. Based on learnings, thenoqa: WPS211directive is valid for wemake-python-styleguide.async def test_create_product( # noqa: WPS211 - mocker, context: Context, vendor_client, product + mocker, vendor_client, product ):
🧹 Nitpick comments (2)
seed/accounts/seller.py (1)
36-37: Unnecessary@injecton function without DI parameters.
build_seller_datadoesn't use anyProvide[...]defaults, so the@injectdecorator is redundant. This is harmless but adds minor overhead.seed/accounts/licensee.py (1)
14-14: Fragile path resolution; inconsistent withbuyer.py.This uses a hardcoded path relative to CWD, which will break if the script is run from a different directory.
buyer.pyuses the more robustpathlib.Path(__file__).parent.parent / "data/logo.png"pattern.-icon = pathlib.Path("seed/data/logo.png").resolve() +icon = pathlib.Path(__file__).parent.parent / "data/logo.png"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
seed/assets/FIL-9920-4780-9379.pngis excluded by!**/*.pngseed/assets/empty.pdfis excluded by!**/*.pdfseed/assets/logo.pngis excluded by!**/*.png
📒 Files selected for processing (32)
mpt_api_client/resources/catalog/products.py(2 hunks)pyproject.toml(1 hunks)seed/accounts/api_tokens.py(3 hunks)seed/accounts/buyer.py(3 hunks)seed/accounts/licensee.py(4 hunks)seed/accounts/module.py(2 hunks)seed/accounts/seller.py(2 hunks)seed/accounts/user_group.py(3 hunks)seed/assets/assets.py(1 hunks)seed/catalog/authorization.py(1 hunks)seed/catalog/catalog.py(1 hunks)seed/catalog/item.py(7 hunks)seed/catalog/item_group.py(0 hunks)seed/catalog/listing.py(1 hunks)seed/catalog/price_list.py(1 hunks)seed/catalog/product.py(1 hunks)seed/catalog/product_parameters.py(0 hunks)seed/catalog/product_parameters_group.py(0 hunks)seed/container.py(1 hunks)seed/context.py(1 hunks)seed/defaults.py(0 hunks)seed/helper.py(1 hunks)seed/seed_api.py(1 hunks)tests/seed/catalog/test_authorization.py(1 hunks)tests/seed/catalog/test_catalog.py(1 hunks)tests/seed/catalog/test_listing.py(1 hunks)tests/seed/catalog/test_price_list.py(1 hunks)tests/seed/catalog/test_product.py(2 hunks)tests/seed/catalog/test_product_parameters.py(0 hunks)tests/seed/catalog/test_product_parameters_group.py(0 hunks)tests/seed/test_context.py(1 hunks)tests/seed/test_helper.py(1 hunks)
💤 Files with no reviewable changes (6)
- seed/defaults.py
- seed/catalog/item_group.py
- seed/catalog/product_parameters.py
- seed/catalog/product_parameters_group.py
- tests/seed/catalog/test_product_parameters.py
- tests/seed/catalog/test_product_parameters_group.py
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/seed/catalog/test_authorization.py
- seed/catalog/catalog.py
- mpt_api_client/resources/catalog/products.py
- pyproject.toml
- tests/seed/test_context.py
- seed/assets/assets.py
- seed/accounts/module.py
- seed/context.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-01T10:48:52.586Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 136
File: tests/e2e/notifications/subscribers/conftest.py:14-25
Timestamp: 2025-12-01T10:48:52.586Z
Learning: The repository softwareone-platform/mpt-api-python-client uses wemake-python-styleguide linter, so noqa directives for WPS rules should not be flagged as unused.
Applied to files:
tests/seed/catalog/test_product.py
🧬 Code graph analysis (9)
seed/accounts/api_tokens.py (4)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)
seed/seed_api.py (4)
seed/accounts/accounts.py (1)
seed_accounts(13-23)seed/catalog/catalog.py (1)
seed_catalog(11-19)seed/container.py (1)
Container(7-32)seed/context.py (3)
Context(12-33)load_context(36-50)save_context(53-61)
tests/seed/catalog/test_price_list.py (4)
seed/catalog/price_list.py (2)
create_price_list(15-29)seed_price_list(10-12)tests/seed/conftest.py (2)
context(14-15)operations_client(24-25)seed/context.py (2)
Context(12-33)get_string(15-17)tests/seed/catalog/test_product.py (1)
context_with_product(26-29)
seed/catalog/item.py (3)
seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)
seed/accounts/user_group.py (4)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)
seed/helper.py (4)
seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (2)
Context(12-33)get_string(15-17)mpt_api_client/models/model.py (1)
id(119-121)
tests/seed/test_helper.py (2)
seed/context.py (2)
Context(12-33)get_string(15-17)seed/helper.py (3)
ResourceRequiredError(13-20)init_resource(43-83)require_context_id(23-40)
seed/accounts/licensee.py (3)
mpt_api_client/resources/accounts/licensees.py (1)
Licensee(21-22)seed/container.py (1)
Container(7-32)seed/context.py (1)
Context(12-33)
seed/catalog/listing.py (4)
mpt_api_client/resources/catalog/listings.py (1)
Listing(11-12)seed/container.py (1)
Container(7-32)seed/context.py (1)
Context(12-33)seed/helper.py (2)
init_resource(43-83)require_context_id(23-40)
🪛 Ruff (0.14.8)
seed/catalog/authorization.py
12-12: Unused function argument: context
(ARG001)
seed/seed_api.py
20-20: Unused noqa directive (unknown: WPS229)
Remove unused noqa directive
(RUF100)
seed/catalog/product.py
54-54: Unused function argument: context
(ARG001)
tests/seed/catalog/test_product.py
4-4: Unused noqa directive (unknown: WPS235)
Remove unused noqa directive
(RUF100)
32-32: Unused noqa directive (unknown: WPS211)
Remove unused noqa directive
(RUF100)
33-33: Unused function argument: context
(ARG001)
seed/catalog/listing.py
15-15: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
tests/seed/catalog/test_listing.py
18-18: Unused noqa directive (unknown: WPS218)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (29)
seed/container.py (1)
45-47: LGTM! Package-level wiring consolidates DI setup.The shift from per-module wiring to package-level wiring (
packages=["seed"]) simplifies container configuration and enables consistentProvide[Container.*]usage across all seed modules.tests/seed/catalog/test_catalog.py (1)
6-20: LGTM! Test correctly verifies new sequential seeding flow.The test now validates that
seed_catalogorchestrates the four sequential operations (product → authorization → price_list → listing) as expected.tests/seed/test_helper.py (1)
7-50: LGTM! Comprehensive test coverage for helper utilities.The tests thoroughly validate
require_context_id(value retrieval and error handling) andinit_resource(both existing and new resource paths), ensuring the DI-based context management works correctly.seed/seed_api.py (1)
26-26: LGTM! Context persistence added correctly.Adding
save_contextin the finally block ensures the context is persisted even if seeding partially fails, which aids debugging and enables resume-from-checkpoint workflows.tests/seed/catalog/test_price_list.py (1)
7-47: LGTM! Comprehensive test coverage for price list seeding.The tests correctly validate both the payload construction (product.id from context) and the conditional seeding logic (skip if exists, create and store ID if new).
seed/catalog/price_list.py (1)
10-12: LGTM! Seeding flow correctly delegates to init_resource.The function properly uses
init_resourcefor lazy initialization with the catalog.price_list.id key.seed/catalog/authorization.py (1)
17-38: LGTM! Authorization payload construction is correct.The function properly validates required context IDs and constructs a complete authorization payload with all necessary fields.
tests/seed/catalog/test_listing.py (1)
18-18: Remove unusednoqadirective.The
noqa: WPS218directive is flagged as unused by Ruff since the function has only 3 parameters.-async def test_create_listing(mocker, operations_client, context_with_data): # noqa: WPS218 +async def test_create_listing(mocker, operations_client, context_with_data):⛔ Skipped due to learnings
Learnt from: jentyk Repo: softwareone-platform/mpt-api-python-client PR: 136 File: tests/e2e/notifications/subscribers/conftest.py:14-25 Timestamp: 2025-12-01T10:48:52.586Z Learning: The repository softwareone-platform/mpt-api-python-client uses wemake-python-styleguide linter, so noqa directives for WPS rules should not be flagged as unused.seed/accounts/api_tokens.py (1)
14-18: DI wiring is correctly implemented.The
@injectdecorator is properly applied to all functions usingProvide[...]defaults, ensuring dependency injection resolves correctly at runtime.seed/accounts/seller.py (1)
55-59: DI wiring correctly implemented.The
@injectdecorator andProvide[...]defaults are properly paired for dependency injection.seed/accounts/buyer.py (2)
14-14: Good: Relative path resolution using__file__.Using
pathlib.Path(__file__).parent.parent / "data/logo.png"ensures the icon path resolves correctly regardless of the current working directory. This is more robust than hardcoded paths.
66-70: DI wiring correctly implemented.The
@injectdecorator andProvide[...]defaults are properly paired for dependency injection.seed/accounts/licensee.py (1)
77-81: DI wiring correctly implemented.The
@injectdecorator andProvide[...]defaults are properly paired.seed/catalog/listing.py (1)
26-45: LGTM!The listing payload structure is well-organized with all required fields properly populated from context IDs.
seed/accounts/user_group.py (4)
15-33: LGTM!The DI migration for
get_user_groupis correctly implemented with the@injectdecorator and properProvide[Container.*]defaults.
36-52: LGTM!The
build_user_group_datafunction correctly uses DI for the context parameter.
55-76: LGTM!The
init_user_groupfunction properly wires DI dependencies and explicitly passes them to helper functions via keyword arguments.
79-85: LGTM!The
seed_user_groupfunction correctly relies on DI to resolve dependencies forinit_user_group.seed/catalog/item.py (6)
14-26: LGTM!The DI migration for
refresh_itemis correctly implemented with@injectdecorator and proper dependency defaults.
29-48: LGTM!The
get_itemfunction properly handles the cached/fetch pattern with DI-wired dependencies.
51-72: LGTM!The
build_itemfunction correctly uses DI for context and builds the item payload appropriately.
75-85: LGTM!The
create_itemfunction properly wires dependencies and passes context explicitly tobuild_item.
88-115: LGTM!The
review_itemandpublish_itemfunctions are correctly migrated to DI with appropriate decorators.
118-131: LGTM!The
seed_itemsorchestration function correctly passes DI-resolved dependencies to helper functions via keyword arguments.tests/seed/catalog/test_product.py (4)
3-16: LGTM!The imports are correctly updated to reflect the new granular
create_*helper functions.
20-29: LGTM!The fixtures are well-structured -
productcreates a basic Product instance andcontext_with_productprovides a context pre-populated with the product ID.
43-64: LGTM!The
test_seed_product_sequence_skipandtest_seed_product_sequencetests correctly verify the orchestration logic - skipping creation when product exists and calling child resource seeding.
67-177: LGTM!The individual
create_*helper tests are well-structured. Each test properly:
- Sets up context with required IDs
- Mocks the appropriate API call
- Verifies the return value and payload construction
seed/catalog/product.py (1)
27-36: LGTM!The
create_productfunction correctly uses@injectdecorator with DI-wiredmpt_vendorparameter.
albertsola
force-pushed
the
MPT-16255/Seeding-for-catalog
branch
3 times, most recently
from
7f480ad to
7e489c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
seed/catalog/authorization.py (1)
12-14: Missing@injectdecorator prevents DI resolution.
seed_authorizationusesProvide[Container.context]but lacks the@injectdecorator, so the container won't inject the dependency. Additionally, as noted in previous reviews, thecontextparameter is unused and should either be removed or forwarded toinit_resource.Apply this diff:
+@inject async def seed_authorization() -> None: """Seed authorization.""" await init_resource("catalog.authorization.id", create_authorization)seed/catalog/product.py (2)
37-52: Missing@injectdecorator and unusedcontextparameter.As flagged by static analysis and previous reviews,
seed_productusesProvide[Container.context]but lacks the@injectdecorator. Additionally, thecontextparameter is unused—either remove it or forward it to nested calls.Apply this diff:
+@inject async def seed_product( - context: Context = Provide[Container.context], ) -> None: """Seed product data."""
55-76: Missing@injectdecorator on multiplecreate_*functions.Functions
create_terms_variant,create_template,create_terms,create_parameter_group,create_parameter,create_document,create_item_group,create_unit_of_measure, andcreate_product_itemall useProvide[Container.*]but lack the required@injectdecorator. This is a systematic issue preventing DI resolution throughout the file.Add
@injectdecorator to each function. For example:+@inject async def create_terms_variant( context: Context = Provide[Container.context], mpt_vendor: AsyncMPTClient = Provide[Container.mpt_vendor], ) -> TermVariant:Apply the same pattern to all other
create_*functions listed above.seed/seed_api.py (1)
17-22: Seed accounts before catalog to satisfy authorization dependencies
seed_catalog()currently runs beforeseed_accounts(), but catalog authorization creation requiresaccounts.seller.id/accounts.account.idset by the accounts seeders. On a fresh context this causes theResourceRequiredError: Missing required resource 'accounts.seller.id' before Create authorizationyou observed.Swap the order so accounts are seeded first:
- try: # noqa: WPS229 - await seed_catalog() - await seed_accounts() + try: # noqa: WPS229 + await seed_accounts() + await seed_catalog()This keeps the structure intact while ensuring all account-related IDs exist before catalog seeding runs.
tests/seed/catalog/test_listing.py (1)
7-31: Align listing test context keys with catalog authorization/price list seeders
seed/catalog/authorization.pyandseed/catalog/price_list.pycache their IDs undercatalog.authorization.idandcatalog.price_list.id, but this fixture (andcreate_listing) currently useaccounts.authorization.id/accounts.price_list.id. When invoked viaseed_catalog, that mismatch means the keyscreate_listingneeds are never populated.Once
create_listingis updated to read the catalog-scoped IDs, adjust the fixture accordingly:ctx = Context() ctx["catalog.product.id"] = "prod-123" ctx["accounts.seller.id"] = "seller-456" - ctx["accounts.authorization.id"] = "auth-789" - ctx["accounts.account.id"] = "acct-321" - ctx["accounts.price_list.id"] = "pl-654" + ctx["catalog.authorization.id"] = "auth-789" + ctx["accounts.account.id"] = "acct-321" + ctx["catalog.price_list.id"] = "pl-654"The rest of the assertions can stay the same, since they only look at the constructed payload, not the specific context key names.
seed/catalog/listing.py (1)
1-45: Fix DI injection and catalog-vs-accounts context keys in listing seederTwo separate issues here will break end‑to‑end catalog seeding:
- Missing
@injectdecorators withProvide[...]defaults
seed_listing()is called asawait seed_listing()fromseed/catalog/catalog.py, so it relies on DI to supplyContext.create_listingis used as a zero‑arg factory ininit_resource, so it also relies on DI forAsyncMPTClientandContext.- Without
@inject, both functions receiveProvide(...)marker objects instead of real instances, and calls intorequire_context_id/ HTTP client methods will fail at runtime.
- Authorization/price list context keys should use the catalog namespace
seed/catalog/authorization.pyandseed/catalog/price_list.pystore their IDs undercatalog.authorization.idandcatalog.price_list.id.- Here,
create_listingreads fromaccounts.authorization.idandaccounts.price_list.id, so when orchestrated viaseed_catalog, those keys are never populated andrequire_context_idwill raiseResourceRequiredError.You can address all of this with something along these lines:
-from dependency_injector.wiring import Provide +from dependency_injector.wiring import Provide, inject @@ -async def seed_listing(context: Context = Provide[Container.context]) -> None: +@inject +async def seed_listing(context: Context = Provide[Container.context]) -> None: @@ -async def create_listing( # noqa: WPS210 - operations: AsyncMPTClient = Provide[Container.mpt_operations], - context: Context = Provide[Container.context], -) -> Listing: +@inject +async def create_listing( # noqa: WPS210 + operations: AsyncMPTClient = Provide[Container.mpt_operations], + context: Context = Provide[Container.context], +) -> Listing: @@ - product_id = require_context_id(context, "catalog.product.id", "Create listing") - seller_id = require_context_id(context, "accounts.seller.id", "Create authorization") - authorization_id = require_context_id(context, "accounts.authorization.id", "Create listing") - account_id = require_context_id(context, "accounts.account.id", "Create listing") - price_list_id = require_context_id(context, "accounts.price_list.id", "Create listing") + product_id = require_context_id(context, "catalog.product.id", "Create listing") + seller_id = require_context_id(context, "accounts.seller.id", "Create listing") + authorization_id = require_context_id( + context, + "catalog.authorization.id", + "Create listing", + ) + account_id = require_context_id(context, "accounts.account.id", "Create listing") + price_list_id = require_context_id( + context, + "catalog.price_list.id", + "Create listing", + )After this change, remember to update
tests/seed/catalog/test_listing.py’s fixture to setcatalog.authorization.idandcatalog.price_list.idinstead of the accounts‑scoped keys (see my comment there).tests/seed/catalog/test_product.py (1)
31-38: Remove unusedcontextparameter fromtest_create_product
context: Contextisn’t used in this test and is flagged by the linter as an unused argument. You can simplify the signature without changing behavior:-async def test_create_product( # noqa: WPS211 - mocker, context: Context, vendor_client, product -): +async def test_create_product( # noqa: WPS211 + mocker, vendor_client, product +):I’m intentionally not suggesting the removal of the
# noqa: WPS235/# noqa: WPS211comments here—those WPS codes come fromwemake-python-styleguideand are valid in this repository even if Ruff reports them as unknown. Based on learnings, this is expected and can be left as-is.
🧹 Nitpick comments (4)
seed/accounts/buyer.py (1)
14-14: Asset path resolution differs fromseed/assets/assets.pypattern.Line 14 uses
pathlib.Path(__file__).parent.parent / "data/logo.png"whereasseed/assets/assets.pyusespathlib.Path(__file__).parent / "...". Consider using the centralized asset constants (e.g.,from seed.assets.assets import LOGO) for consistency.seed/accounts/licensee.py (1)
14-14: Inconsistent asset path resolution across seed modules.Line 14 uses
pathlib.Path("seed/data/logo.png").resolve()(absolute from project root), whileseed/accounts/buyer.py:14usespathlib.Path(__file__).parent.parent / "data/logo.png"andseed/assets/assets.pyusespathlib.Path(__file__).parent / "...". Consider consolidating asset paths using the centralized constants inseed/assets/assets.py(e.g.,from seed.assets.assets import LOGO).seed/catalog/catalog.py (1)
11-17: Sequential catalog seeding order looks good; docstring is slightly staleThe new sequential flow
seed_product() → seed_authorization() → seed_price_list() → seed_listing()is consistent with the implied dependency chain and avoids the earlier async orchestration complexity.You may want to update the docstring to reflect all seeded resources:
- """Seed catalog data including products, item groups, and parameters.""" + """Seed catalog data including products, item groups, parameters, authorizations, price lists, and listings."""seed/context.py (1)
36-50: Updateload_contextdocstring to match in‑place,None-returning behavior
load_contextnow mutates the passedContextand returnsNone, but the docstring still advertises a returnedContext.Consider tightening it like this:
-def load_context(json_file: pathlib.Path, context: Context) -> None: - """Load context from JSON file. - - Args: - json_file: JSON file path. - context: Context instance. - - Returns: - Context instance. - - """ +def load_context(json_file: pathlib.Path, context: Context) -> None: + """Load context from JSON file into an existing Context instance. + + Args: + json_file: JSON file path. + context: Context instance to update in-place. + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
seed/assets/FIL-9920-4780-9379.pngis excluded by!**/*.pngseed/assets/empty.pdfis excluded by!**/*.pdfseed/assets/logo.pngis excluded by!**/*.png
📒 Files selected for processing (33)
mpt_api_client/resources/catalog/products.py(2 hunks)pyproject.toml(1 hunks)seed/accounts/api_tokens.py(3 hunks)seed/accounts/buyer.py(3 hunks)seed/accounts/licensee.py(4 hunks)seed/accounts/module.py(2 hunks)seed/accounts/seller.py(2 hunks)seed/accounts/user_group.py(3 hunks)seed/assets/assets.py(1 hunks)seed/catalog/authorization.py(1 hunks)seed/catalog/catalog.py(1 hunks)seed/catalog/item.py(7 hunks)seed/catalog/item_group.py(0 hunks)seed/catalog/listing.py(1 hunks)seed/catalog/price_list.py(1 hunks)seed/catalog/product.py(1 hunks)seed/catalog/product_parameters.py(0 hunks)seed/catalog/product_parameters_group.py(0 hunks)seed/container.py(1 hunks)seed/context.py(1 hunks)seed/defaults.py(0 hunks)seed/helper.py(1 hunks)seed/seed_api.py(1 hunks)tests/seed/catalog/test_authorization.py(1 hunks)tests/seed/catalog/test_catalog.py(1 hunks)tests/seed/catalog/test_item_group.py(0 hunks)tests/seed/catalog/test_listing.py(1 hunks)tests/seed/catalog/test_price_list.py(1 hunks)tests/seed/catalog/test_product.py(2 hunks)tests/seed/catalog/test_product_parameters.py(0 hunks)tests/seed/catalog/test_product_parameters_group.py(0 hunks)tests/seed/test_context.py(1 hunks)tests/seed/test_helper.py(1 hunks)
💤 Files with no reviewable changes (7)
- seed/catalog/product_parameters_group.py
- tests/seed/catalog/test_product_parameters_group.py
- seed/defaults.py
- tests/seed/catalog/test_item_group.py
- tests/seed/catalog/test_product_parameters.py
- seed/catalog/item_group.py
- seed/catalog/product_parameters.py
✅ Files skipped from review due to trivial changes (1)
- pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (10)
- seed/container.py
- seed/catalog/price_list.py
- tests/seed/test_helper.py
- tests/seed/catalog/test_price_list.py
- tests/seed/catalog/test_authorization.py
- tests/seed/test_context.py
- seed/accounts/user_group.py
- mpt_api_client/resources/catalog/products.py
- seed/helper.py
- seed/accounts/module.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-01T10:48:52.586Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 136
File: tests/e2e/notifications/subscribers/conftest.py:14-25
Timestamp: 2025-12-01T10:48:52.586Z
Learning: The repository softwareone-platform/mpt-api-python-client uses wemake-python-styleguide linter, so noqa directives for WPS rules should not be flagged as unused.
Applied to files:
tests/seed/catalog/test_product.py
🧬 Code graph analysis (11)
seed/catalog/catalog.py (4)
seed/catalog/authorization.py (1)
seed_authorization(12-14)seed/catalog/listing.py (1)
seed_listing(10-12)seed/catalog/price_list.py (1)
seed_price_list(10-12)seed/catalog/product.py (1)
seed_product(37-52)
seed/accounts/api_tokens.py (4)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)
seed/accounts/buyer.py (5)
mpt_api_client/resources/accounts/buyers.py (1)
Buyer(26-27)seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)tests/e2e/conftest.py (1)
logger(63-64)
seed/accounts/licensee.py (5)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/licensees.py (1)
Licensee(21-22)seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)
seed/catalog/listing.py (6)
mpt_api_client/resources/catalog/listings.py (1)
Listing(11-12)seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)seed/helper.py (2)
init_resource(43-83)require_context_id(23-40)tests/e2e/catalog/listings/conftest.py (1)
listing_data(10-29)
seed/accounts/seller.py (4)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/sellers.py (1)
Seller(13-14)seed/container.py (1)
Container(7-32)seed/context.py (1)
Context(12-33)
seed/catalog/item.py (3)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)seed/container.py (1)
Container(7-32)seed/context.py (1)
Context(12-33)
seed/context.py (1)
tests/seed/conftest.py (1)
context(14-15)
tests/seed/catalog/test_product.py (1)
seed/catalog/product.py (9)
create_document(149-165)create_item_group(168-185)create_parameter(121-146)create_parameter_group(106-118)create_product(26-34)create_product_item(199-227)create_template(79-91)create_terms(94-103)create_terms_variant(55-76)
seed/catalog/product.py (5)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/catalog/products.py (9)
items(79-83)items(142-146)Product(53-54)terms(121-123)terms(184-188)templates(115-119)templates(178-182)documents(103-107)documents(166-170)seed/container.py (1)
Container(7-32)seed/context.py (1)
Context(12-33)seed/helper.py (2)
init_resource(43-83)require_context_id(23-40)
tests/seed/catalog/test_catalog.py (5)
seed/catalog/catalog.py (1)
seed_catalog(11-19)seed/catalog/product.py (1)
seed_product(37-52)seed/catalog/authorization.py (1)
seed_authorization(12-14)seed/catalog/price_list.py (1)
seed_price_list(10-12)seed/catalog/listing.py (1)
seed_listing(10-12)
🪛 Ruff (0.14.8)
seed/seed_api.py
20-20: Unused noqa directive (unknown: WPS229)
Remove unused noqa directive
(RUF100)
seed/catalog/listing.py
15-15: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
tests/seed/catalog/test_product.py
4-4: Unused noqa directive (unknown: WPS235)
Remove unused noqa directive
(RUF100)
31-31: Unused noqa directive (unknown: WPS211)
Remove unused noqa directive
(RUF100)
32-32: Unused function argument: context
(ARG001)
seed/catalog/product.py
38-38: Unused function argument: context
(ARG001)
tests/seed/catalog/test_listing.py
18-18: Unused noqa directive (unknown: WPS218)
Remove unused noqa directive
(RUF100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (8)
seed/assets/assets.py (1)
3-5: ICON and LOGO reference the same file.Both constants point to
FIL-9920-4780-9379.png. Verify this is intentional—typically icon and logo assets differ in dimensions or branding.seed/accounts/api_tokens.py (1)
14-75: LGTM! DI wiring correctly implemented.The migration to container-based dependency injection is properly implemented with
@injectdecorators andProvide[Container.*]defaults throughout.seed/accounts/seller.py (1)
15-82: LGTM! DI wiring correctly implemented.The dependency injection migration is correctly implemented with proper
@injectdecorators andProvide[Container.*]defaults.seed/accounts/buyer.py (1)
17-94: LGTM! DI wiring correctly implemented.The dependency injection migration is properly implemented with
@injectdecorators andProvide[Container.*]defaults throughout.seed/accounts/licensee.py (1)
17-105: LGTM! DI wiring correctly implemented.The dependency injection migration is correctly implemented with proper
@injectdecorators andProvide[Container.*]defaults.seed/catalog/product.py (1)
25-34: LGTM! Product creation correctly implemented.The
create_productfunction properly uses the@injectdecorator and DI-provided dependencies.tests/seed/catalog/test_catalog.py (1)
6-20: LGTM! Test correctly reflects refactored catalog seeding flow.The test properly mocks the new seeding functions (
seed_authorization,seed_price_list,seed_listing) and verifies the orchestration order.seed/catalog/item.py (1)
14-131: DI migration for catalog items looks consistentThe refactor to use
Provide[Container.*]defaults plus@injecton all item-related functions is consistent and preserves the existing behavior (refresh/get/build/create/review/publish/seed). Callers can still pass explicit clients/context (as the tests do), while DI-backed entrypoints likeseed_items()remain usable.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
seed/context.py (1)
36-51: Docstring return type is inconsistent with actual behavior.The function signature changed to return
None, but the docstring still states "Returns: Context instance." Update the docstring to reflect the in-place mutation pattern.Args: json_file: JSON file path. context: Context instance. - Returns: - Context instance. - """
♻️ Duplicate comments (12)
seed/helper.py (1)
43-47: Critical: Missing@injectdecorator causes runtime failures.The function uses
Provide[Container.context]but lacks the required@injectdecorator. Without it, when callers omit thecontextargument, theProvidemarker object is passed directly, causingcontext.get_string(namespace)to fail withAttributeError. This is likely the root cause of theResourceRequiredErrormentioned in the PR comments.+from dependency_injector.wiring import Provide, inject -from dependency_injector.wiring import Provide +@inject async def init_resource( namespace: str, resource_factory: Callable[[], Awaitable[Any]], context: Context = Provide[Container.context], ) -> str:Also update the docstring at lines 61-62 to remove reference to
DEFAULT_CONTEXT:- context: The seeding Context used to read and persist the id. Defaults to - ``DEFAULT_CONTEXT``. + context: The seeding Context used to read and persist the id. Injected from + ``Container.context`` by default.seed/catalog/authorization.py (2)
12-14:seed_authorizationshould accept and forward context.The function doesn't accept a
contextparameter, so callers cannot pass an explicit context. This differs from the pattern inseed_listingandseed_price_listwhich accept context.-async def seed_authorization() -> None: +@inject +async def seed_authorization(context: Context = Provide[Container.context]) -> None: """Seed authorization.""" - await init_resource("catalog.authorization.id", create_authorization) + await init_resource("catalog.authorization.id", create_authorization, context)
17-38: Missing@injectdecorator oncreate_authorization.This function uses
Provide[Container.mpt_operations]andProvide[Container.context]but lacks the@injectdecorator required for DI resolution. Without it, theProvidemarkers will be passed as-is instead of resolved values.+@inject async def create_authorization( operations: AsyncMPTClient = Provide[Container.mpt_operations], context: Context = Provide[Container.context], ) -> Authorization:seed/catalog/listing.py (2)
10-12: Missing@injectdecorator will prevent DI resolution.The
seed_listingfunction usesProvide[Container.context]but lacks the@injectdecorator. Without it, the context parameter will receive theProvidemarker object instead of the actualContextinstance at runtime.
15-18: Missing@injectdecorator and unusednoqadirective.The
create_listingfunction usesProvide[Container.*]but lacks the@injectdecorator. Additionally, the static analysis confirms thenoqa: WPS210directive is unused and should be removed.Apply this diff:
-async def create_listing( # noqa: WPS210 +@inject +async def create_listing( operations: AsyncMPTClient = Provide[Container.mpt_operations], context: Context = Provide[Container.context], ) -> Listing:Also update the import on line 1:
-from dependency_injector.wiring import Provide +from dependency_injector.wiring import Provide, injecttests/seed/catalog/test_product.py (1)
31-39: Remove unusedcontextparameter.The
contextparameter is not used in this test. Thenoqa: WPS211is valid for wemake-python-styleguide per learnings.async def test_create_product( # noqa: WPS211 - mocker, context: Context, vendor_client, product + mocker, vendor_client, product ):seed/catalog/product.py (6)
37-52: Missing@injectdecorator and unusedcontextparameter.The
seed_productfunction usesProvide[Container.context]but lacks the@injectdecorator. Additionally, thecontextparameter is unused sinceinit_resourcecalls don't pass it (they get context via their own DI wiring). Either add@injectif you need to use context, or remove the parameter entirely.If context is intentionally unused here, remove it:
-async def seed_product( - context: Context = Provide[Container.context], -) -> None: +async def seed_product() -> None:
55-76: Missing@injectdecorator oncreate_terms_variant.This function uses
Provide[Container.*]but lacks the@injectdecorator required for DI resolution.+@inject async def create_terms_variant( context: Context = Provide[Container.context], mpt_vendor: AsyncMPTClient = Provide[Container.mpt_vendor], ) -> TermVariant:
79-103: Missing@injectdecorator oncreate_templateandcreate_terms.Both functions use
Provide[Container.*]but lack the@injectdecorator.+@inject async def create_template( context: Context = Provide[Container.context], mpt_vendor: AsyncMPTClient = Provide[Container.mpt_vendor], ) -> Template:+@inject async def create_terms( context: Context = Provide[Container.context], mpt_vendor: AsyncMPTClient = Provide[Container.mpt_vendor], ) -> Term:
106-146: Missing@injectdecorator oncreate_parameter_groupandcreate_parameter.Both functions use
Provide[Container.*]but lack the@injectdecorator.+@inject async def create_parameter_group(+@inject async def create_parameter(
149-185: Missing@injectdecorator oncreate_documentandcreate_item_group.Both functions use
Provide[Container.*]but lack the@injectdecorator.+@inject async def create_document(+@inject async def create_item_group(
188-227: Missing@injectdecorator oncreate_unit_of_measureandcreate_product_item.Both functions use
Provide[Container.*]but lack the@injectdecorator.+@inject async def create_unit_of_measure( operations: AsyncMPTClient = Provide[Container.mpt_operations], ) -> UnitOfMeasure:+@inject async def create_product_item( context: Context = Provide[Container.context], mpt_vendor: AsyncMPTClient = Provide[Container.mpt_vendor], ) -> Item:
🧹 Nitpick comments (5)
seed/assets/assets.py (1)
1-7: Consider adding module documentation and type hints.For improved maintainability:
- Add a module docstring explaining the purpose of these asset path constants
- Consider adding type hints:
ICON: pathlib.Path = ...Example:
+"""Asset file paths for catalog seeding.""" + import pathlib -ICON = pathlib.Path(__file__).parent / "FIL-9920-4780-9379.png" +ICON: pathlib.Path = pathlib.Path(__file__).parent / "FIL-9920-4780-9379.png" -LOGO = pathlib.Path(__file__).parent / "FIL-9920-4780-9379.png" +LOGO: pathlib.Path = pathlib.Path(__file__).parent / "FIL-9920-4780-9379.png" -PDF = pathlib.Path(__file__).parent / "empty.pdf" +PDF: pathlib.Path = pathlib.Path(__file__).parent / "empty.pdf"tests/seed/catalog/test_catalog.py (1)
6-20: LGTM! Consider verifying call order.The test correctly verifies that all four seed functions are invoked exactly once. Since the seeding order matters (accounts.seller.id must exist before authorization), you may optionally add order verification.
To verify the seeding order, you could use
unittest.mock.callwithassert_has_calls:from unittest.mock import call # After await seed_catalog() manager = mocker.Mock() manager.attach_mock(seed_product, 'seed_product') manager.attach_mock(seed_authorization, 'seed_authorization') # ... etcOr simply check
calledtimestamps if order is critical.tests/seed/catalog/test_listing.py (1)
18-18: Remove unusednoqadirective.The static analysis tool reports that
# noqa: WPS218is unused since the function has only 3 parameters.-async def test_create_listing(mocker, operations_client, context_with_data): # noqa: WPS218 +async def test_create_listing(mocker, operations_client, context_with_data):seed/seed_api.py (1)
20-20: Remove unusednoqadirective.Static analysis reports that
# noqa: WPS229is unused.- try: # noqa: WPS229 + try:seed/accounts/user_group.py (1)
79-84: Unnecessary@injectdecorator.The
seed_user_groupfunction has noProvide[...]parameters, so the@injectdecorator has no effect. It works becauseinit_user_group()is called without arguments, relying on its own DI wiring. Consider removing the decorator for clarity.-@inject async def seed_user_group() -> UserGroup | None: """Seed user group.""" logger.debug("Seeding user group ...") user_group = await init_user_group() logger.debug("Seeding user group completed.") return user_group
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
seed/assets/FIL-9920-4780-9379.pngis excluded by!**/*.pngseed/assets/empty.pdfis excluded by!**/*.pdfseed/assets/logo.pngis excluded by!**/*.png
📒 Files selected for processing (33)
mpt_api_client/resources/catalog/products.py(2 hunks)pyproject.toml(1 hunks)seed/accounts/api_tokens.py(3 hunks)seed/accounts/buyer.py(3 hunks)seed/accounts/licensee.py(4 hunks)seed/accounts/module.py(2 hunks)seed/accounts/seller.py(2 hunks)seed/accounts/user_group.py(3 hunks)seed/assets/assets.py(1 hunks)seed/catalog/authorization.py(1 hunks)seed/catalog/catalog.py(1 hunks)seed/catalog/item.py(7 hunks)seed/catalog/item_group.py(0 hunks)seed/catalog/listing.py(1 hunks)seed/catalog/price_list.py(1 hunks)seed/catalog/product.py(1 hunks)seed/catalog/product_parameters.py(0 hunks)seed/catalog/product_parameters_group.py(0 hunks)seed/container.py(1 hunks)seed/context.py(1 hunks)seed/defaults.py(0 hunks)seed/helper.py(1 hunks)seed/seed_api.py(1 hunks)tests/seed/catalog/test_authorization.py(1 hunks)tests/seed/catalog/test_catalog.py(1 hunks)tests/seed/catalog/test_item_group.py(0 hunks)tests/seed/catalog/test_listing.py(1 hunks)tests/seed/catalog/test_price_list.py(1 hunks)tests/seed/catalog/test_product.py(2 hunks)tests/seed/catalog/test_product_parameters.py(0 hunks)tests/seed/catalog/test_product_parameters_group.py(0 hunks)tests/seed/test_context.py(1 hunks)tests/seed/test_helper.py(1 hunks)
💤 Files with no reviewable changes (7)
- seed/catalog/item_group.py
- tests/seed/catalog/test_product_parameters_group.py
- tests/seed/catalog/test_item_group.py
- tests/seed/catalog/test_product_parameters.py
- seed/defaults.py
- seed/catalog/product_parameters_group.py
- seed/catalog/product_parameters.py
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/seed/catalog/test_authorization.py
- seed/container.py
- tests/seed/test_helper.py
- tests/seed/test_context.py
- tests/seed/catalog/test_price_list.py
- seed/accounts/licensee.py
- pyproject.toml
- seed/catalog/price_list.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-01T10:48:52.586Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 136
File: tests/e2e/notifications/subscribers/conftest.py:14-25
Timestamp: 2025-12-01T10:48:52.586Z
Learning: The repository softwareone-platform/mpt-api-python-client uses wemake-python-styleguide linter, so noqa directives for WPS rules should not be flagged as unused.
Applied to files:
tests/seed/catalog/test_product.py
🧬 Code graph analysis (16)
seed/accounts/buyer.py (6)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/buyers.py (1)
Buyer(26-27)seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)tests/e2e/conftest.py (1)
logger(63-64)
seed/accounts/module.py (3)
seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)
seed/seed_api.py (5)
seed/accounts/accounts.py (1)
seed_accounts(13-23)seed/catalog/catalog.py (1)
seed_catalog(11-19)seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (3)
Context(12-33)load_context(36-50)save_context(53-61)
seed/accounts/user_group.py (4)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)
seed/accounts/seller.py (6)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/sellers.py (1)
Seller(13-14)seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)tests/e2e/conftest.py (1)
logger(63-64)
tests/seed/catalog/test_product.py (1)
seed/catalog/product.py (9)
create_document(149-165)create_item_group(168-185)create_parameter(121-146)create_parameter_group(106-118)create_product(26-34)create_template(79-91)create_terms(94-103)create_terms_variant(55-76)create_unit_of_measure(188-196)
tests/seed/catalog/test_catalog.py (5)
seed/catalog/catalog.py (1)
seed_catalog(11-19)seed/catalog/product.py (1)
seed_product(37-52)seed/catalog/authorization.py (1)
seed_authorization(12-14)seed/catalog/price_list.py (1)
seed_price_list(10-12)seed/catalog/listing.py (1)
seed_listing(10-12)
seed/catalog/item.py (4)
mpt_api_client/resources/catalog/items.py (1)
Item(15-16)seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)
seed/context.py (1)
tests/seed/conftest.py (1)
context(14-15)
seed/catalog/authorization.py (5)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/catalog/authorizations.py (1)
Authorization(11-12)seed/container.py (1)
Container(7-32)seed/context.py (1)
Context(12-33)seed/helper.py (2)
init_resource(43-83)require_context_id(23-40)
mpt_api_client/resources/catalog/products.py (2)
mpt_api_client/http/mixins.py (4)
CreateFileMixin(115-144)UpdateFileMixin(147-184)AsyncCreateFileMixin(187-216)AsyncUpdateFileMixin(219-256)mpt_api_client/resources/catalog/mixins.py (2)
PublishableMixin(11-45)AsyncPublishableMixin(48-82)
seed/helper.py (5)
seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (2)
Context(12-33)get_string(15-17)tests/e2e/conftest.py (1)
logger(63-64)mpt_api_client/models/model.py (1)
id(119-121)
seed/catalog/catalog.py (4)
seed/catalog/authorization.py (1)
seed_authorization(12-14)seed/catalog/listing.py (1)
seed_listing(10-12)seed/catalog/price_list.py (1)
seed_price_list(10-12)seed/catalog/product.py (1)
seed_product(37-52)
seed/catalog/product.py (15)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/catalog/products.py (15)
items(79-83)items(142-146)Product(53-54)terms(121-123)terms(184-188)templates(115-119)templates(178-182)parameter_groups(91-95)parameter_groups(154-158)parameters(109-113)parameters(172-176)documents(103-107)documents(166-170)item_groups(85-89)item_groups(148-152)mpt_api_client/resources/catalog/items.py (1)
Item(15-16)mpt_api_client/resources/catalog/product_term_variants.py (1)
TermVariant(19-20)mpt_api_client/resources/catalog/product_terms.py (1)
Term(16-17)mpt_api_client/resources/catalog/products_documents.py (1)
Document(15-16)mpt_api_client/resources/catalog/products_item_groups.py (1)
ItemGroup(11-12)mpt_api_client/resources/catalog/products_parameter_groups.py (1)
ParameterGroup(11-12)mpt_api_client/resources/catalog/products_parameters.py (1)
Parameter(11-12)mpt_api_client/resources/catalog/products_templates.py (1)
Template(11-12)mpt_api_client/resources/catalog/units_of_measure.py (1)
UnitOfMeasure(11-12)seed/container.py (1)
Container(7-32)seed/context.py (1)
Context(12-33)seed/helper.py (2)
init_resource(43-83)require_context_id(23-40)mpt_api_client/http/mixins.py (6)
create(18-26)create(86-112)create(118-144)create(190-216)create(262-270)create(331-358)
seed/catalog/listing.py (5)
mpt_api_client/resources/catalog/listings.py (1)
Listing(11-12)seed/container.py (1)
Container(7-32)seed/context.py (1)
Context(12-33)seed/helper.py (2)
init_resource(43-83)require_context_id(23-40)tests/e2e/catalog/listings/conftest.py (1)
listing_data(10-29)
tests/seed/catalog/test_listing.py (3)
seed/catalog/listing.py (2)
create_listing(15-45)seed_listing(10-12)tests/seed/conftest.py (2)
context(14-15)operations_client(24-25)seed/context.py (2)
Context(12-33)get_string(15-17)
🪛 Ruff (0.14.8)
seed/seed_api.py
20-20: Unused noqa directive (unknown: WPS229)
Remove unused noqa directive
(RUF100)
tests/seed/catalog/test_product.py
4-4: Unused noqa directive (unknown: WPS235)
Remove unused noqa directive
(RUF100)
31-31: Unused noqa directive (unknown: WPS211)
Remove unused noqa directive
(RUF100)
32-32: Unused function argument: context
(ARG001)
seed/catalog/product.py
38-38: Unused function argument: context
(ARG001)
seed/catalog/listing.py
15-15: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
tests/seed/catalog/test_listing.py
18-18: Unused noqa directive (unknown: WPS218)
Remove unused noqa directive
(RUF100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (37)
seed/assets/assets.py (1)
3-5: Verify that ICON and LOGO should reference the same file.Both
ICONandLOGOpoint to "FIL-9920-4780-9379.png". If this is intentional (e.g., using the same image for both purposes in seed data), no action is needed. However, if they should reference different files, please update accordingly.seed/catalog/item.py (7)
14-27: LGTM!The
@injectdecorator is correctly applied, and the DI wiring withProvide[Container.context]andProvide[Container.mpt_vendor]is properly configured. The function logic for refreshing items remains unchanged.
29-48: LGTM!Consistent DI wiring pattern applied. The caching logic with fallback to API fetch is preserved.
51-72: LGTM!Sync function correctly uses
@injectwithProvide[Container.context]. The build logic is unchanged.
75-85: LGTM!DI wiring correctly applied to the create function.
88-100: LGTM!DI wiring for review function is correct. Uses
mpt_vendorappropriately.
103-115: LGTM!Correctly uses
mpt_operationsfor publishing, which aligns with the operations client pattern.
118-131: LGTM!The orchestration function correctly passes explicit context and client arguments to child functions, ensuring DI-provided values are forwarded consistently.
tests/seed/catalog/test_listing.py (2)
7-15: LGTM!The fixture correctly populates context keys matching the catalog namespace used in
seed/catalog/listing.py(catalog.authorization.id,catalog.price_list.id).
34-53: LGTM!Both
test_seed_listing_skipsandtest_seed_listing_createscorrectly verify the skip-if-exists and create-if-missing behaviors, including the context update assertion.seed/helper.py (2)
13-21: LGTM!The
ResourceRequiredErrorexception class is well-designed with contextual attributes for debugging.
23-40: LGTM!The
require_context_idfunction correctly validates context IDs and raises the appropriate exception. The docstring accurately reflects theResourceRequiredErrorthat is raised.seed/seed_api.py (2)
20-22: LGTM! Seeding order is correct.The accounts are now seeded before catalog, ensuring that
accounts.seller.idandaccounts.account.idexist beforeseed_authorizationrequires them.
16-17: LGTM!The
@injectdecorator is correctly applied withProvide[Container.context]for DI resolution.seed/accounts/module.py (3)
14-32: LGTM!The
get_modulefunction is correctly wired with DI. The caching logic with fallback to API fetch is preserved.
35-57: LGTM!The
refresh_modulefunction correctly forwards DI-provided parameters toget_moduleand handles the fallback filter query appropriately.
60-72: LGTM!The
seed_moduleorchestration function correctly uses@injectand delegates to the other injected functions. The error handling for missing modules is appropriate.seed/catalog/catalog.py (1)
11-19: Sequential seeding order looks correct.The order
seed_product → seed_authorization → seed_price_list → seed_listingensures each downstream seeder has access to the context IDs populated by its predecessors. This aligns with the dependencies shown in the relevant snippets (e.g.,create_listingrequirescatalog.product.id,catalog.authorization.id,catalog.price_list.id).seed/accounts/api_tokens.py (1)
14-32: DI wiring correctly applied.The
@injectdecorator is properly paired with theProvide[Container.*]defaults, ensuring the container resolvescontextandmpt_opsat runtime.mpt_api_client/resources/catalog/products.py (2)
67-76: Good type refinement for the service mixins.Specializing the generic type parameter from
ModeltoProductimproves type safety. Methods inherited fromCreateFileMixin,UpdateFileMixin, andPublishableMixinwill now correctly returnProductinstances rather than the baseModeltype.
130-139: Consistent type refinement for async service.Same improvement applied to
AsyncProductsService, maintaining consistency between sync and async implementations.seed/accounts/seller.py (2)
15-33: DI wiring correctly applied to seller functions.The
@injectdecorator andProvide[Container.*]defaults are properly paired.
77-82: No action required—seller seeding is already correctly orchestrated.The current code properly calls
seed_seller()beforeseed_authorization(). Inseed/seed_api.py,seed_accounts()is awaited beforeseed_catalog()(lines 21–22), and withinseed/accounts/accounts.py,seed_seller()is the first seeder invoked (line 16). Theinit_seller()function correctly populatescontext["accounts.seller.id"](line 68 inseed/accounts/seller.py), ensuring the required resource is available whenseed_authorization()attempts to create an authorization.seed/catalog/listing.py (1)
20-24: Consistent operation descriptions.Good that the operation descriptions now consistently say "Create listing" for all
require_context_idcalls. This addresses a previous review comment about copy-paste errors.seed/accounts/buyer.py (3)
14-14: Good use ofpathlibfor icon path resolution.Using
pathlib.Path(__file__).parent.parent / "data/logo.png"ensures the path is correctly resolved relative to the module location regardless of the working directory.
38-63: Good prerequisite validation inbuild_buyer_data.The function properly validates that
CLIENT_ACCOUNT_IDis set andaccounts.seller.idexists in context before building the buyer data. The error messages are clear about what's missing.
66-86: DI wiring and error handling look correct.The
@injectdecorator is properly paired withProvide[Container.*]defaults. The function correctly handles the creation workflow, including storing the created buyer in context and raisingValueErroron failure.seed/accounts/user_group.py (3)
15-33: LGTM!The DI wiring is correctly applied with
@injectdecorator andProvide[Container.*]defaults. The function properly handles both context retrieval and API fallback.
36-52: LGTM!The DI wiring is correctly applied. The function properly retrieves module_id from context.
55-76: LGTM!The function correctly uses DI and explicitly passes dependencies to called functions.
tests/seed/catalog/test_product.py (6)
4-15:noqa: WPS235is valid for wemake-python-styleguide.Based on learnings, the repository uses wemake-python-styleguide, so WPS noqa directives are valid. Ruff doesn't recognize these rules, hence the false positive.
42-63: LGTM!Tests for
create_templateandcreate_termsproperly mock the nested service calls and verify return values.
66-76: LGTM!The test correctly sets up the required context keys (
catalog.product.terms.id) and mocks the deeply nested variants service.
78-100: LGTM!Tests for parameter group and parameter creation properly set up context prerequisites and verify the mock calls.
103-125: LGTM!Tests for document and item group creation properly verify payload construction including the embedded
product.id.
128-152: LGTM!Tests for unit of measure and product item creation properly verify the payload construction with all required context IDs (
unit.id,item_group.id,product.id).seed/catalog/product.py (1)
25-34: LGTM!The
create_productfunction correctly uses@injectdecorator withProvide[Container.mpt_vendor].
albertsola
force-pushed
the
MPT-16255/Seeding-for-catalog
branch
from
7e489c9 to
e80c818
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
seed/seed_api.py (1)
4-26: Remove incorrect noqa suggestion;accounts.account.idmust be populated during seedingThe
# noqa: WPS229on the try block is necessary and should stay—WPS229 flags try bodies exceeding the default 1-line limit, and the two await statements here legitimately exceed that threshold.However, there is a critical blocker: the catalog seeding (
seed_authorizationandseed_listing) callsrequire_context_id()for bothaccounts.seller.idandaccounts.account.id(catalog/authorization.py:24 and catalog/listing.py:23), butaccounts.account.idis never written to context by any accounts seeding function. Onlyaccounts.seller.idis populated (seed/accounts/seller.py:31, 68).This will cause a runtime
ResourceRequiredErroron any fresh seeding run. You must add aninit_resourceor context assignment foraccounts.account.idin one of the accounts seeding functions (likelyseed_licensee()or a new account initialization step), or remove therequire_context_id()call if that key is not actually needed.
♻️ Duplicate comments (15)
seed/catalog/listing.py (3)
1-1: Missinginjectimport required for DI resolution.The module uses
Provide[...]defaults but doesn't importinject. This will prevent dependency injection from working.-from dependency_injector.wiring import Provide +from dependency_injector.wiring import Provide, inject
10-12: Missing@injectdecorator will prevent DI resolution.As flagged in a previous review,
seed_listingusesProvide[Container.context]but lacks the@injectdecorator. The context parameter will receive theProvidemarker object instead of the actualContextinstance.+@inject async def seed_listing(context: Context = Provide[Container.context]) -> None:
15-18: Missing@injectdecorator and stalenoqadirective.As flagged in a previous review,
create_listingneeds the@injectdecorator for DI to work. The static analysis also correctly identifies thatnoqa: WPS210is unused and should be removed.-async def create_listing( # noqa: WPS210 +@inject +async def create_listing( operations: AsyncMPTClient = Provide[Container.mpt_operations], context: Context = Provide[Container.context], ) -> Listing:tests/seed/catalog/test_product.py (2)
4-4: Remove unused noqa directive.The
noqa: WPS235directive is not recognized by the current linter and should be removed.Apply this diff:
-from seed.catalog.product import ( # noqa: WPS235 +from seed.catalog.product import ( create_document,
31-33: Remove unusedcontextparameter and noqa directive.The
contextparameter is not used in this test function, and thenoqa: WPS211directive is not recognized by the current linter.Apply this diff:
-async def test_create_product( # noqa: WPS211 - mocker, context: Context, vendor_client, product +async def test_create_product( + mocker, vendor_client, product ):seed/catalog/product.py (10)
37-52: Missing@injectdecorator and unusedcontextparameter.The
seed_productfunction usesProvide[Container.context]but lacks the@injectdecorator. Additionally, thecontextparameter is unused sinceinit_resourcehas its own DI-provided context. Remove the parameter entirely.Apply this diff:
+@inject -async def seed_product( - context: Context = Provide[Container.context], -) -> None: +async def seed_product() -> None: """Seed product data."""
55-76: Missing@injectdecorator oncreate_terms_variant.This function uses
Provide[Container.*]but lacks the@injectdecorator required for DI resolution.Apply this diff:
+@inject async def create_terms_variant( context: Context = Provide[Container.context], mpt_vendor: AsyncMPTClient = Provide[Container.mpt_vendor], ) -> TermVariant:
79-91: Missing@injectdecorator oncreate_template.Apply this diff:
+@inject async def create_template( context: Context = Provide[Container.context], mpt_vendor: AsyncMPTClient = Provide[Container.mpt_vendor], ) -> Template:
94-103: Missing@injectdecorator oncreate_terms.Apply this diff:
+@inject async def create_terms( context: Context = Provide[Container.context], mpt_vendor: AsyncMPTClient = Provide[Container.mpt_vendor], ) -> Term:
106-118: Missing@injectdecorator oncreate_parameter_group.Apply this diff:
+@inject async def create_parameter_group( context: Context = Provide[Container.context], mpt_vendor: AsyncMPTClient = Provide[Container.mpt_vendor], ) -> ParameterGroup:
121-146: Missing@injectdecorator oncreate_parameter.Apply this diff:
+@inject async def create_parameter( context: Context = Provide[Container.context], mpt_vendor: AsyncMPTClient = Provide[Container.mpt_vendor], ) -> Parameter:
149-165: Missing@injectdecorator oncreate_document.Apply this diff:
+@inject async def create_document( context: Context = Provide[Container.context], mpt_vendor: AsyncMPTClient = Provide[Container.mpt_vendor], ) -> Document:
168-185: Missing@injectdecorator oncreate_item_group.Apply this diff:
+@inject async def create_item_group( context: Context = Provide[Container.context], mpt_vendor: AsyncMPTClient = Provide[Container.mpt_vendor], ) -> ItemGroup:
188-196: Missing@injectdecorator oncreate_unit_of_measure.Apply this diff:
+@inject async def create_unit_of_measure( operations: AsyncMPTClient = Provide[Container.mpt_operations], ) -> UnitOfMeasure:
199-227: Missing@injectdecorator oncreate_product_item.Apply this diff:
+@inject async def create_product_item( context: Context = Provide[Container.context], mpt_vendor: AsyncMPTClient = Provide[Container.mpt_vendor], ) -> Item:
🧹 Nitpick comments (3)
seed/context.py (1)
36-50: Updateload_contextdocstring to match new in-place behavior
load_contextnow mutates the providedContextin place and correctly returnsNone, which fits the DI pattern used elsewhere. The docstring still says:Returns: Context instance.
That’s now misleading. Consider updating the “Returns” section (or removing it) to reflect that the function only performs an in-place update and logs the loaded items.
seed/catalog/catalog.py (1)
3-17: Catalog seeding sequence matches dependency graph; consider refreshing docstringThe new
seed_catalog()sequence:
await seed_product()await seed_authorization()await seed_price_list()await seed_listing()is consistent with the data dependencies between these resources and keeps the flow simple and deterministic.
The docstring (“Seed catalog data including products, item groups, and parameters.”) is now slightly out of date, since this function also orchestrates authorization, price list, and listing seeding. Consider expanding it to mention those additional responsibilities to avoid confusion for future readers.
seed/accounts/seller.py (1)
36-52: Unnecessary@injectdecorator.The
build_seller_datafunction doesn't use anyProvide[...]parameters, so the@injectdecorator has no effect. Consider removing it for clarity.-@inject def build_seller_data(external_id: str | None = None) -> dict[str, object]:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
seed/assets/FIL-9920-4780-9379.pngis excluded by!**/*.pngseed/assets/empty.pdfis excluded by!**/*.pdfseed/assets/logo.pngis excluded by!**/*.png
📒 Files selected for processing (33)
mpt_api_client/resources/catalog/products.py(2 hunks)pyproject.toml(1 hunks)seed/accounts/api_tokens.py(3 hunks)seed/accounts/buyer.py(3 hunks)seed/accounts/licensee.py(4 hunks)seed/accounts/module.py(2 hunks)seed/accounts/seller.py(2 hunks)seed/accounts/user_group.py(3 hunks)seed/assets/assets.py(1 hunks)seed/catalog/authorization.py(1 hunks)seed/catalog/catalog.py(1 hunks)seed/catalog/item.py(7 hunks)seed/catalog/item_group.py(0 hunks)seed/catalog/listing.py(1 hunks)seed/catalog/price_list.py(1 hunks)seed/catalog/product.py(1 hunks)seed/catalog/product_parameters.py(0 hunks)seed/catalog/product_parameters_group.py(0 hunks)seed/container.py(1 hunks)seed/context.py(1 hunks)seed/defaults.py(0 hunks)seed/helper.py(1 hunks)seed/seed_api.py(1 hunks)tests/seed/catalog/test_authorization.py(1 hunks)tests/seed/catalog/test_catalog.py(1 hunks)tests/seed/catalog/test_item_group.py(0 hunks)tests/seed/catalog/test_listing.py(1 hunks)tests/seed/catalog/test_price_list.py(1 hunks)tests/seed/catalog/test_product.py(2 hunks)tests/seed/catalog/test_product_parameters.py(0 hunks)tests/seed/catalog/test_product_parameters_group.py(0 hunks)tests/seed/test_context.py(1 hunks)tests/seed/test_helper.py(1 hunks)
💤 Files with no reviewable changes (7)
- tests/seed/catalog/test_item_group.py
- seed/catalog/item_group.py
- tests/seed/catalog/test_product_parameters.py
- seed/defaults.py
- seed/catalog/product_parameters_group.py
- tests/seed/catalog/test_product_parameters_group.py
- seed/catalog/product_parameters.py
🚧 Files skipped from review as they are similar to previous changes (8)
- seed/assets/assets.py
- seed/accounts/api_tokens.py
- seed/container.py
- seed/catalog/authorization.py
- tests/seed/test_helper.py
- tests/seed/test_context.py
- pyproject.toml
- seed/helper.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-01T10:48:52.586Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 136
File: tests/e2e/notifications/subscribers/conftest.py:14-25
Timestamp: 2025-12-01T10:48:52.586Z
Learning: The repository softwareone-platform/mpt-api-python-client uses wemake-python-styleguide linter, so noqa directives for WPS rules should not be flagged as unused.
Applied to files:
tests/seed/catalog/test_product.py
🧬 Code graph analysis (14)
seed/accounts/module.py (4)
seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)tests/e2e/conftest.py (1)
logger(63-64)
seed/accounts/seller.py (5)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)tests/e2e/conftest.py (1)
logger(63-64)
seed/accounts/licensee.py (6)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/licensees.py (1)
Licensee(21-22)seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)tests/e2e/conftest.py (1)
mpt_client(43-44)
tests/seed/catalog/test_product.py (3)
seed/catalog/product.py (10)
create_document(149-165)create_item_group(168-185)create_parameter(121-146)create_parameter_group(106-118)create_product(26-34)create_product_item(199-227)create_template(79-91)create_terms(94-103)create_terms_variant(55-76)create_unit_of_measure(188-196)tests/seed/catalog/test_price_list.py (1)
context_with_product(8-11)seed/context.py (1)
Context(12-33)
tests/seed/catalog/test_listing.py (2)
seed/catalog/listing.py (2)
create_listing(15-45)seed_listing(10-12)seed/context.py (2)
Context(12-33)get_string(15-17)
seed/accounts/buyer.py (4)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/buyers.py (1)
Buyer(26-27)seed/container.py (1)
Container(7-32)seed/context.py (1)
Context(12-33)
tests/seed/catalog/test_authorization.py (4)
seed/catalog/authorization.py (2)
create_authorization(17-38)seed_authorization(12-14)tests/seed/conftest.py (2)
context(14-15)operations_client(24-25)seed/context.py (1)
Context(12-33)seed/helper.py (1)
init_resource(43-83)
seed/accounts/user_group.py (6)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/user_groups.py (1)
UserGroup(11-12)seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)tests/e2e/conftest.py (1)
logger(63-64)
seed/catalog/catalog.py (4)
seed/catalog/authorization.py (1)
seed_authorization(12-14)seed/catalog/listing.py (1)
seed_listing(10-12)seed/catalog/price_list.py (1)
seed_price_list(10-12)seed/catalog/product.py (1)
seed_product(37-52)
seed/catalog/product.py (12)
mpt_api_client/resources/catalog/products.py (15)
items(79-83)items(142-146)Product(53-54)terms(121-123)terms(184-188)templates(115-119)templates(178-182)parameter_groups(91-95)parameter_groups(154-158)parameters(109-113)parameters(172-176)documents(103-107)documents(166-170)item_groups(85-89)item_groups(148-152)mpt_api_client/resources/catalog/items.py (1)
Item(15-16)mpt_api_client/resources/catalog/product_term_variants.py (1)
TermVariant(19-20)mpt_api_client/resources/catalog/product_terms.py (1)
Term(16-17)mpt_api_client/resources/catalog/products_documents.py (1)
Document(15-16)mpt_api_client/resources/catalog/products_item_groups.py (1)
ItemGroup(11-12)mpt_api_client/resources/catalog/products_parameter_groups.py (1)
ParameterGroup(11-12)mpt_api_client/resources/catalog/products_parameters.py (1)
Parameter(11-12)mpt_api_client/resources/catalog/products_templates.py (1)
Template(11-12)mpt_api_client/resources/catalog/units_of_measure.py (1)
UnitOfMeasure(11-12)seed/container.py (1)
Container(7-32)seed/helper.py (2)
init_resource(43-83)require_context_id(23-40)
seed/context.py (1)
tests/seed/conftest.py (1)
context(14-15)
mpt_api_client/resources/catalog/products.py (2)
mpt_api_client/http/mixins.py (4)
CreateFileMixin(115-144)UpdateFileMixin(147-184)AsyncCreateFileMixin(187-216)AsyncUpdateFileMixin(219-256)mpt_api_client/resources/catalog/mixins.py (2)
PublishableMixin(11-45)AsyncPublishableMixin(48-82)
seed/catalog/item.py (5)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/catalog/items.py (1)
Item(15-16)seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)
seed/catalog/listing.py (5)
mpt_api_client/resources/catalog/listings.py (1)
Listing(11-12)seed/container.py (1)
Container(7-32)seed/context.py (1)
Context(12-33)seed/helper.py (2)
init_resource(43-83)require_context_id(23-40)tests/e2e/catalog/listings/conftest.py (1)
listing_data(10-29)
🪛 Ruff (0.14.8)
tests/seed/catalog/test_product.py
4-4: Unused noqa directive (unknown: WPS235)
Remove unused noqa directive
(RUF100)
31-31: Unused noqa directive (unknown: WPS211)
Remove unused noqa directive
(RUF100)
32-32: Unused function argument: context
(ARG001)
tests/seed/catalog/test_listing.py
18-18: Unused noqa directive (unknown: WPS218)
Remove unused noqa directive
(RUF100)
seed/catalog/product.py
38-38: Unused function argument: context
(ARG001)
seed/seed_api.py
20-20: Unused noqa directive (unknown: WPS229)
Remove unused noqa directive
(RUF100)
seed/catalog/listing.py
15-15: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (16)
mpt_api_client/resources/catalog/products.py (1)
67-76: Product-specific generics on file/publish mixins look correctSpecializing
CreateFileMixin,UpdateFileMixin, andPublishableMixin(and async equivalents) withProductmatches_model_class = Productand tightens typing without changing behavior. No further changes needed here.Also applies to: 130-139
tests/seed/catalog/test_catalog.py (1)
3-20: Catalog seed orchestration test matches new flowPatching
seed_product,seed_authorization,seed_price_list, andseed_listingfromseed.catalog.catalogand asserting each is awaited exactly once cleanly verifies the new sequential orchestration inseed_catalog(). Looks good.tests/seed/catalog/test_price_list.py (1)
1-47: Price list seeding tests align with the new DI-based flowThe tests:
- Use a minimal
Contextfixture withcatalog.product.idset.- Verify
create_price_listpasses the correct product id into the payload and returns the client result.- Check that
seed_price_listskips creation whencatalog.price_list.idis already present and otherwise creates once and writes back the new id.This is consistent with the seeding helper patterns used elsewhere; no issues spotted.
tests/seed/catalog/test_authorization.py (1)
1-40: Authorization seeding tests validate required context and orchestrationThese tests look solid:
context_with_datapopulates exactly the IDscreate_authorizationrequires (catalog.product.id,accounts.seller.id,accounts.account.id).test_create_authorizationcorrectly mocks the async client, fixesuuid.uuid4()to a known value, and asserts that product/owner/vendor ids are wired into the payload.test_seed_authorizationensuresseed_authorization()delegates toinit_resourcewith thecatalog.authorization.idnamespace and thecreate_authorizationfactory.The coverage is appropriate for the new authorization seed flow.
seed/accounts/module.py (3)
14-32: LGTM! DI wiring is correctly applied.The
@injectdecorator is properly paired withProvide[Container.context]andProvide[Container.mpt_operations]defaults. The function logic correctly handles context lookup with API fallback.
35-57: LGTM! Refresh logic is well structured.The
refresh_modulefunction correctly propagates injected dependencies toget_moduleand handles the fallback query for "Access Management" module appropriately.
60-72: LGTM! Orchestration function is clean.The
seed_modulefunction correctly delegates to injected functions without needing its own DI parameters, and properly raises on failure.seed/accounts/buyer.py (3)
17-35: LGTM! DI wiring correctly applied to get_buyer.The
@injectdecorator is properly paired withProvide[Container.context]andProvide[Container.mpt_operations]. The context lookup with API fallback pattern is consistent with other modules.
38-63: LGTM! Validation logic is appropriate.The
build_buyer_datafunction correctly validates required dependencies before building the payload. The explicit error messages for missingCLIENT_ACCOUNT_IDandaccounts.seller.idwill help diagnose seeding order issues (relates to the PR comment about missing seller ID).
66-86: LGTM! Buyer initialization logic is correct.The
init_buyerfunction properly chains toget_buyerandbuild_buyer_data, handles icon file upload, and correctly stores the created buyer in context.seed/accounts/seller.py (2)
15-33: LGTM! DI wiring correctly applied to get_seller.The
@injectdecorator is properly paired withProvide[Container.context]andProvide[Container.mpt_operations]. Pattern is consistent with other account modules.
55-74: LGTM! Seller initialization logic is correct.The
init_sellerfunction correctly chains toget_seller, creates a new seller when needed, and stores the result in context.seed/catalog/listing.py (1)
20-44: LGTM! Listing payload construction is correct.The
require_context_idcalls properly validate all required dependencies before building the payload. The payload structure matches the pattern intests/e2e/catalog/listings/conftest.py.seed/accounts/licensee.py (1)
5-20: LGTM: DI migration correctly implemented.The dependency injection migration is properly implemented with the
@injectdecorator present on all functions usingProvide[Container.*]for parameter defaults.seed/accounts/user_group.py (1)
5-18: LGTM: DI migration correctly implemented.The dependency injection migration is properly implemented with the
@injectdecorator present on all functions usingProvide[Container.*]for parameter defaults.seed/catalog/item.py (1)
4-17: LGTM: DI migration correctly implemented.The dependency injection migration is properly implemented with the
@injectdecorator present on all functions usingProvide[Container.*]for parameter defaults.
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @albertsola. * #152 (comment) The following files were modified: * `seed/accounts/api_tokens.py` * `seed/accounts/buyer.py` * `seed/accounts/licensee.py` * `seed/accounts/module.py` * `seed/accounts/seller.py` * `seed/accounts/user_group.py` * `seed/catalog/authorization.py` * `seed/catalog/catalog.py` * `seed/catalog/item.py` * `seed/catalog/listing.py` * `seed/catalog/price_list.py` * `seed/catalog/product.py` * `seed/context.py` * `seed/helper.py` * `seed/seed_api.py` * `tests/seed/catalog/test_authorization.py` * `tests/seed/catalog/test_listing.py` * `tests/seed/catalog/test_price_list.py` * `tests/seed/catalog/test_product.py`
albertsola
force-pushed
the
MPT-16255/Seeding-for-catalog
branch
3 times, most recently
from
0b5d915 to
adbaf61
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
seed/seed_api.py (1)
16-26: Remove the stale# noqa: WPS229on line 20The
# noqa: WPS229directive is not recognized by Ruff (WPS229 is from wemake-python-styleguide, not a Ruff rule code). Ruff's RUF100 rule correctly flags it as unused. Remove it to eliminate the RUF100 violation.The sequential seeding order, DI-injected
Context, and error handling flow are sound.seed/accounts/licensee.py (1)
17-105: Icon file path is incorrect; will cause FileNotFoundErrorThe DI integration with
@injectandProvide[Container.*]is correct and properly supported by dependency-injector ≥4.48.2. TheSingleton(Context)ensures all functions share the same context instance, and context keys are properly validated before use.However, there is a critical bug: the hardcoded icon path references
seed/data/logo.png, but the actual file exists atseed/assets/logo.png. This will cause aFileNotFoundErrorwheninit_licensee()attempts to open the file. Either update the path to point to the correct file location, or add a validation check before opening.Minor cleanup:
@injectonseed_licensee()is redundant since it has no injectable parameters; remove the decorator.
♻️ Duplicate comments (5)
seed/helper.py (1)
49-77: Updateinit_resourcedocstring to reflect DI-based default contextThe implementation correctly uses
context: Context = Provide[Container.context]with@inject, but the docstring still says the context “Defaults toDEFAULT_CONTEXT”, which no longer exists. Please update that line to state that the default is injected fromContainer.contextvia dependency-injector so docs match behavior.Everything else in this helper (exception type, logging, lazy factory call) looks good.
Confirm in the latest dependency-injector docs that using `@inject` plus a default of `Provide[Container.context]` is the recommended pattern for injecting a container singleton into a function parameter.seed/catalog/listing.py (2)
16-16: Remove unusednoqadirective.The
# noqa: WPS210directive on line 16 is not recognized by the linter and should be removed.Apply this diff:
@inject -async def create_listing( # noqa: WPS210 +async def create_listing( operations: AsyncMPTClient = Provide[Container.mpt_operations], context: Context = Provide[Container.context], ) -> Listing:
10-12: Missing@injectdecorator will prevent DI resolution.The
seed_listingfunction callsinit_resourcewhich expects to be wired with the container, but the function lacks the@injectdecorator. This will cause issues when the DI container tries to resolve dependencies.Add the
@injectdecorator:+@inject async def seed_listing() -> None: """Seed listing.""" await init_resource("catalog.listing.id", create_listing)seed/accounts/buyer.py (1)
14-14: Incorrect logo path will cause FileNotFoundError.Line 14 points to
data/logo.png, but based on the previous review, the actual file is atassets/logo.png. This will cause a runtime error wheninit_buyerattempts to open the file on line 76.Apply this diff:
-icon = pathlib.Path(__file__).parent.parent / "data/logo.png" +icon = pathlib.Path(__file__).parent.parent / "assets/logo.png"Based on past review findings, the logo file exists at
seed/assets/logo.png.tests/seed/catalog/test_product.py (1)
31-33: Remove unusedcontextparameter fromtest_create_product
context: Contextis not used in this test body and triggers Ruff’s ARG001 warning. You can drop it from the signature without affecting fixture resolution.-async def test_create_product( # noqa: WPS211 - mocker, context: Context, vendor_client, product -): +async def test_create_product( # noqa: WPS211 + mocker, vendor_client, product +):
🧹 Nitpick comments (5)
seed/context.py (1)
36-50: Fixload_contextdocstring to matchNonereturn and consider log verbosity
load_contextnow mutates the providedContextin place and returnsNone, but the docstring still advertises aContextreturn value. Please drop the “Returns” section or update it to describe the in-place update.Also,
logger.info("Context loaded: %s", context.items())will log the entire context on every run; if this grows or carries IDs you consider sensitive/noisy, you might want to log only keys or counts instead.tests/seed/catalog/test_catalog.py (1)
6-20: Catalog seeding orchestration test is clear and focusedThe test cleanly verifies that
seed_catalog()delegates toseed_product,seed_authorization,seed_price_list, andseed_listingexactly once via async mocks. This gives good coverage of the orchestration layer; if you ever need to assert order, you could extend it with call ordering checks, but it’s not strictly necessary here.tests/seed/catalog/test_listing.py (2)
18-18: Remove unusednoqadirective.The
# noqa: WPS218directive on line 18 is not needed and should be removed.Apply this diff:
-async def test_create_listing(mocker, operations_client, context_with_data): # noqa: WPS218 +async def test_create_listing(mocker, operations_client, context_with_data):
34-34: Remove unused test parameter.The
context_with_datafixture is passed totest_seed_listingbut never used, since the test patchesinit_resourcedirectly. Remove it to avoid confusion.Apply this diff:
-async def test_seed_listing(mocker, context_with_data): +async def test_seed_listing(mocker):seed/accounts/user_group.py (1)
36-52: Consider enforcingaccounts.module.idpresence via a helper
build_user_group_datawill happily sendmodules: [{"id": ""}]ifaccounts.module.idis missing in the context. If this ID is actually required, consider mirroring other modules and failing fast with a dedicated helper instead of sending an invalid payload.For example:
-from seed.context import Context +from seed.context import Context +from seed.helper import require_context_id @@ - module_id = context.get_string("accounts.module.id") + module_id = require_context_id( + context, + "accounts.module.id", + "creating user group", + )This keeps error handling consistent with other seeding helpers that depend on context IDs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
seed/assets/FIL-9920-4780-9379.pngis excluded by!**/*.pngseed/assets/empty.pdfis excluded by!**/*.pdfseed/assets/logo.pngis excluded by!**/*.png
📒 Files selected for processing (34)
mpt_api_client/resources/catalog/products.py(2 hunks)pyproject.toml(1 hunks)seed/accounts/api_tokens.py(3 hunks)seed/accounts/buyer.py(3 hunks)seed/accounts/licensee.py(4 hunks)seed/accounts/module.py(2 hunks)seed/accounts/seller.py(2 hunks)seed/accounts/user_group.py(3 hunks)seed/assets/assets.py(1 hunks)seed/catalog/authorization.py(1 hunks)seed/catalog/catalog.py(1 hunks)seed/catalog/item.py(0 hunks)seed/catalog/item_group.py(0 hunks)seed/catalog/listing.py(1 hunks)seed/catalog/price_list.py(1 hunks)seed/catalog/product.py(1 hunks)seed/catalog/product_parameters.py(0 hunks)seed/catalog/product_parameters_group.py(0 hunks)seed/container.py(1 hunks)seed/context.py(1 hunks)seed/defaults.py(0 hunks)seed/helper.py(1 hunks)seed/seed_api.py(1 hunks)tests/seed/catalog/test_authorization.py(1 hunks)tests/seed/catalog/test_catalog.py(1 hunks)tests/seed/catalog/test_item.py(0 hunks)tests/seed/catalog/test_item_group.py(0 hunks)tests/seed/catalog/test_listing.py(1 hunks)tests/seed/catalog/test_price_list.py(1 hunks)tests/seed/catalog/test_product.py(2 hunks)tests/seed/catalog/test_product_parameters.py(0 hunks)tests/seed/catalog/test_product_parameters_group.py(0 hunks)tests/seed/test_context.py(1 hunks)tests/seed/test_helper.py(1 hunks)
💤 Files with no reviewable changes (9)
- seed/catalog/product_parameters_group.py
- seed/defaults.py
- tests/seed/catalog/test_product_parameters.py
- seed/catalog/item_group.py
- tests/seed/catalog/test_item_group.py
- seed/catalog/product_parameters.py
- seed/catalog/item.py
- tests/seed/catalog/test_item.py
- tests/seed/catalog/test_product_parameters_group.py
🚧 Files skipped from review as they are similar to previous changes (6)
- seed/container.py
- seed/assets/assets.py
- seed/accounts/api_tokens.py
- tests/seed/test_helper.py
- seed/catalog/price_list.py
- tests/seed/test_context.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-01T10:48:52.586Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 136
File: tests/e2e/notifications/subscribers/conftest.py:14-25
Timestamp: 2025-12-01T10:48:52.586Z
Learning: The repository softwareone-platform/mpt-api-python-client uses wemake-python-styleguide linter, so noqa directives for WPS rules should not be flagged as unused.
Applied to files:
tests/seed/catalog/test_product.py
🧬 Code graph analysis (17)
seed/accounts/licensee.py (6)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/licensees.py (1)
Licensee(21-22)seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)tests/e2e/conftest.py (1)
mpt_client(43-44)
seed/helper.py (2)
seed/container.py (1)
Container(7-32)seed/context.py (2)
Context(12-33)get_string(15-17)
seed/accounts/user_group.py (2)
seed/container.py (1)
Container(7-32)seed/context.py (1)
Context(12-33)
seed/catalog/catalog.py (4)
seed/catalog/authorization.py (1)
seed_authorization(12-14)seed/catalog/listing.py (1)
seed_listing(10-12)seed/catalog/price_list.py (1)
seed_price_list(10-12)seed/catalog/product.py (1)
seed_product(37-52)
seed/accounts/seller.py (4)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)
tests/seed/catalog/test_price_list.py (4)
seed/catalog/price_list.py (2)
create_price_list(16-30)seed_price_list(10-12)seed/context.py (1)
Context(12-33)seed/helper.py (1)
init_resource(44-84)mpt_api_client/models/model.py (1)
id(119-121)
seed/accounts/buyer.py (4)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/buyers.py (1)
Buyer(26-27)seed/container.py (1)
Container(7-32)seed/context.py (1)
Context(12-33)
seed/catalog/authorization.py (7)
mpt_api_client/resources/catalog/authorizations.py (1)
Authorization(11-12)seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)tests/e2e/conftest.py (4)
product_id(96-97)seller_id(127-128)account_id(122-123)short_uuid(101-102)tests/e2e/catalog/authorizations/conftest.py (1)
authorization_data(10-21)
seed/context.py (1)
tests/seed/conftest.py (1)
context(14-15)
seed/accounts/module.py (4)
seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)tests/e2e/conftest.py (1)
logger(63-64)
mpt_api_client/resources/catalog/products.py (2)
mpt_api_client/http/mixins.py (4)
CreateFileMixin(115-144)UpdateFileMixin(147-184)AsyncCreateFileMixin(187-216)AsyncUpdateFileMixin(219-256)mpt_api_client/resources/catalog/mixins.py (2)
PublishableMixin(11-45)AsyncPublishableMixin(48-82)
tests/seed/catalog/test_catalog.py (5)
seed/catalog/catalog.py (1)
seed_catalog(11-19)seed/catalog/product.py (1)
seed_product(37-52)seed/catalog/authorization.py (1)
seed_authorization(12-14)seed/catalog/price_list.py (1)
seed_price_list(10-12)seed/catalog/listing.py (1)
seed_listing(10-12)
tests/seed/catalog/test_authorization.py (4)
seed/catalog/authorization.py (2)
create_authorization(18-39)seed_authorization(12-14)tests/seed/conftest.py (2)
context(14-15)operations_client(24-25)tests/seed/catalog/test_listing.py (1)
context_with_data(8-15)seed/helper.py (1)
init_resource(44-84)
seed/seed_api.py (5)
seed/accounts/accounts.py (1)
seed_accounts(13-23)seed/catalog/catalog.py (1)
seed_catalog(11-19)seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (3)
Context(12-33)load_context(36-50)save_context(53-61)
tests/seed/catalog/test_product.py (1)
seed/catalog/product.py (10)
create_document(155-171)create_item_group(175-192)create_parameter(126-151)create_parameter_group(110-122)create_product(26-34)create_product_item(208-236)create_template(81-93)create_terms(97-106)create_terms_variant(56-77)create_unit_of_measure(196-204)
seed/catalog/listing.py (1)
mpt_api_client/resources/catalog/listings.py (1)
Listing(11-12)
tests/seed/catalog/test_listing.py (5)
seed/catalog/listing.py (2)
create_listing(16-46)seed_listing(10-12)tests/seed/conftest.py (2)
context(14-15)operations_client(24-25)seed/context.py (1)
Context(12-33)tests/seed/catalog/test_authorization.py (1)
context_with_data(8-13)seed/helper.py (1)
init_resource(44-84)
🪛 Ruff (0.14.8)
seed/seed_api.py
20-20: Unused noqa directive (unknown: WPS229)
Remove unused noqa directive
(RUF100)
tests/seed/catalog/test_product.py
4-4: Unused noqa directive (unknown: WPS235)
Remove unused noqa directive
(RUF100)
31-31: Unused noqa directive (unknown: WPS211)
Remove unused noqa directive
(RUF100)
32-32: Unused function argument: context
(ARG001)
seed/catalog/product.py
38-38: Unused function argument: context
(ARG001)
seed/catalog/listing.py
16-16: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
tests/seed/catalog/test_listing.py
18-18: Unused noqa directive (unknown: WPS218)
Remove unused noqa directive
(RUF100)
34-34: Unused function argument: context_with_data
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (17)
pyproject.toml (1)
134-138: Lint config updates look consistent with new seed modulesThe added per-file-ignores for
tests/seed/catalog/test_product.py,seed/*, and specific seed paths are coherent with the new seeding code and only affect static analysis, not runtime behavior. No changes requested.mpt_api_client/resources/catalog/products.py (1)
67-76: Narrowing mixin generics toProductis a good type-safety improvementSwitching the create/update/publish mixins to
*Mixin[Product]for bothProductsServiceandAsyncProductsServicealigns their type parameters with the concreteProductmodel and doesn’t affect runtime behavior. This should give better type-checker feedback around product operations.No further changes needed.
Also applies to: 130-139
seed/catalog/authorization.py (1)
12-39: Authorization seeding correctly leverages shared helpers and DI
seed_authorization+create_authorizationcleanly reuseinit_resourceandrequire_context_id, and the DI wiring foroperations/contextvia@injectis consistent with other seed modules. This will fail fast with a clearResourceRequiredErrorif required context keys like"catalog.product.id"or"accounts.seller.id"are missing, which is appropriate as long as callers run account/product seeding first. Test coverage forrequire_context_idwith missing context keys exists intests/seed/test_helper.py::test_require_context_id_raises_when_missing, explicitly validating the fast-fail behavior.No changes requested.
seed/accounts/user_group.py (2)
15-33: DI-basedget_user_groupimplementation looks solidCaching via
Contextand falling back tompt_operations.accounts.user_groups.getis straightforward and consistent with the rest of the seeding code. No changes needed here.
55-76:init_user_group/seed_user_grouporchestration is clearThe “get-or-create” flow plus top-level
seed_user_groupwrapper is clean, logs are useful, and DI usage is consistent with the rest of the module. No changes needed.tests/seed/catalog/test_product.py (3)
3-15: Imports align with new product creation APIImporting
Productand the newcreate_*helpers matches the refactored seeding surface inseed.catalog.product. Nothing to change here.
25-28: Fixturecontext_with_productmatches context expectationsSeeding helpers expect
catalog.product.idinContext, and this fixture sets it up cleanly for tests. Looks good.
42-152: Newcreate_*tests cover key linkages and mocks correctlyThe async tests for templates, terms, terms variants, parameter groups/parameters, documents, item groups, units of measure, and product items all:
- Mock the appropriate client methods on
vendor_client.- Use
Contextto provide required IDs (product, terms, parameter group, unit, item group).- Assert either the returned object or critical payload linkages (e.g., product/group/unit IDs).
This provides good coverage of the new creation helpers’ behavior.
seed/catalog/product.py (9)
1-21: Module-level setup and imports look consistentBringing in the specific catalog resource models,
ICON/Provide,inject,Container) matches how the helpers are used below. No issues here.
25-34:create_producthelper is correctly wiredUsing the icon asset as a file handle and delegating to
mpt_vendor.catalog.products.create(...)is straightforward and matches how the test patches this call. DI via the defaultmpt_vendorargument looks appropriate.Please ensure your DI container wiring includes this module so
create_product()can be called without explicitly passingmpt_vendorwhen used in the seeding flow.
55-77:create_terms_variantcorrectly enforces prerequisites and uploads PDFThe helper:
- Requires both
catalog.product.idandcatalog.product.terms.idviarequire_context_id, giving clear errors when prerequisites are missing.- Uses the shared
terms(product_id).variants(terms_id).create(...)).This matches the expectations from the tests.
Ensure the
require_context_idhelper raises the appropriate domain-specific exception (as used elsewhere) so missing IDs bubble up with clear messaging.
80-123: Template, terms, and parameter-group helpers are consistent and minimal
create_template,create_terms, andcreate_parameter_groupall:
- Validate
catalog.product.idviarequire_context_id.- Build small, focused payloads.
- Call the right sub-clients (
templates,terms,parameter_groups).This keeps them easy to test and reuse.
Double-check that the field names in these payloads (
type,displayOrder, etc.) match the current catalog API schema.
125-152:create_parameterpayload and context usage look correctThe function:
- Requires both the parameter-group ID and product ID from context.
- Constructs a detailed
parameter_datapayload including constraints, options, and group linkage.- Calls
products.parameters(product_id).create(parameter_data)as expected.No changes needed here.
It may be worth confirming against the API docs that the
optionsandconstraintsshapes (keys likehidden,readonly,hintText, etc.) are up to date.
154-171:create_documentcorrectly handles document uploadThe helper:
- Requires
catalog.product.id.- Builds a document payload and uploads a PDF via
documents(product_id).create(document_data, file=pdf_fd).This is consistent with how tests patch the call and how
Please confirm that the
documenttypefield name and allowed values (e.g.,"File") are still correct for the current API version.
174-193:create_item_grouppayload matches test expectationsItem group creation:
- Enforces presence of
catalog.product.id.- Includes the product reference, name/label/description, and flags like
default,multiple,required.- Tests assert that
payload["product"]["id"]matches the seeded product ID.Looks good as written.
If item groups have additional required fields in newer API versions, consider extending
item_group_dataaccordingly.
195-205:create_unit_of_measureis correctly parameterized and uniqueUsing an 8-character UUID suffix in the name is a reasonable way to avoid collisions, and delegating to
operations.catalog.units_of_measure.create(...)matches the separate operations client in the container.Confirm that creating many units over time with this pattern won’t violate any naming constraints or quotas in the backing API.
207-236:create_product_itemcorrectly links unit, group, and productThe product-item helper:
- Requires
catalog.unit.id,catalog.product.item_group.id, andcatalog.product.id.- Embeds those IDs under
unit,group, andproductinproduct_item_data.- Adds sensible defaults for
termsandexternalIds.vendor.- Is validated by tests asserting those IDs in the payload.
No changes needed.
As with other payloads, please recheck the
termsstructure (model,period,commitment) against the latest product-item API documentation.
| async def seed_catalog() -> None: | ||
| """Seed catalog data including products, item groups, and parameters.""" | ||
| logger.debug("Seeding catalog ...") | ||
| await seed_product() | ||
| await seed_groups_and_group_params() | ||
| await seed_items_and_params() | ||
| await seed_authorization() | ||
| await seed_price_list() | ||
| await seed_listing() | ||
|
|
There was a problem hiding this comment.
Missing prerequisite account seeding causes ResourceRequiredError.
The seed_catalog function calls seed_authorization (line 15) and seed_listing (line 17), both of which require accounts.seller.id and accounts.account.id in the context. However, seed_catalog never seeds these account resources, directly causing the ResourceRequiredError: Missing required resource 'accounts.seller.id' reported in the PR comments.
You need to ensure account resources are seeded before catalog seeding. Consider one of these approaches:
Option 1: Add account seeding to seed_catalog:
+from seed.accounts.seller import seed_seller
+from seed.accounts.account import seed_account # if this exists
+
async def seed_catalog() -> None:
"""Seed catalog data including products, item groups, and parameters."""
logger.debug("Seeding catalog ...")
+ await seed_seller()
+ await seed_account() # or whatever seeds accounts.account.id
await seed_product()
await seed_authorization()
await seed_price_list()
await seed_listing()Option 2: Document that seed_catalog requires pre-seeded account context and provide a higher-level orchestrator that seeds both accounts and catalog in the correct order.
Which approach would you prefer? I can help generate the appropriate code or documentation.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In seed/catalog/catalog.py around lines 11–18, seed_catalog calls
seed_authorization and seed_listing but never seeds account resources required
by those functions, causing ResourceRequiredError for accounts.seller.id and
accounts.account.id; fix by adding an await call to the account-seeding routine
(e.g., await seed_accounts()) before any calls to seed_authorization or
seed_listing, ensure the account seeding populates the expected context keys
(accounts.seller.id and accounts.account.id), and update the function docstring
to note that catalog seeding now includes account seeding so prerequisites are
satisfied.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
seed/context.py (1)
36-50: Fix docstring to match the None return type.The function now returns
None(line 36), but the docstring on lines 43-44 still states it returns aContext instance.Apply this diff:
def load_context(json_file: pathlib.Path, context: Context) -> None: """Load context from JSON file. Args: json_file: JSON file path. context: Context instance. - Returns: - Context instance. - """seed/accounts/buyer.py (1)
5-94: Fix improper DI usage ininit_buyerfunction.The DI wiring has issues:
get_buyer()at line 72 andbuild_buyer_data()at line 74 are called with explicitcontextandmpt_operationsarguments, which bypasses the dependency injection system. These functions have@injectdecorators withProvideannotations, so they should be invoked without arguments to allow the container to inject dependencies automatically.Change line 72 from:
buyer = await get_buyer(context=context, mpt_operations=mpt_operations)To:
buyer = await get_buyer()And line 74 from:
buyer_data = build_buyer_data(context=context)To:
buyer_data = build_buyer_data()The
seed_buyer()call toinit_buyer()at line 93 correctly omits arguments, properly leveraging DI.
♻️ Duplicate comments (2)
seed/accounts/buyer.py (1)
14-14: Correct the logo file path fromdata/logo.pngtoassets/logo.png.The path currently points to a non-existent location. The logo file exists at
seed/assets/logo.png.-icon = pathlib.Path(__file__).parent.parent / "data/logo.png" +icon = pathlib.Path(__file__).parent.parent / "assets/logo.png"seed/catalog/product.py (1)
37-39: Remove unusedcontextparameter.The
contextparameter is declared but never used. Sinceinit_resourceobtains context via its own DI wiring, this parameter is unnecessary.Apply this diff:
-async def seed_product( - context: Context = Provide[Container.context], -) -> None: +async def seed_product() -> None:Based on learnings, this repository uses wemake-python-styleguide, so WPS-prefixed noqa directives throughout the codebase are intentional and should not be flagged.
🧹 Nitpick comments (5)
tests/seed/catalog/test_listing.py (2)
18-18: Remove unnecessary noqa directive.The
# noqa: WPS218comment is flagged as unused. The function only has 3 parameters, so this directive can be safely removed.Apply this diff:
-async def test_create_listing(mocker, operations_client, context_with_data): # noqa: WPS218 +async def test_create_listing(mocker, operations_client, context_with_data):
34-43: Remove unused fixture parameter.The
context_with_datafixture parameter is not used intest_seed_listing. Sinceinit_resourceis patched and doesn't rely on the actual context, this parameter can be removed.Apply this diff:
-async def test_seed_listing(mocker, context_with_data): +async def test_seed_listing(mocker):seed/catalog/catalog.py (1)
11-12: Update docstring to reflect current functionality.The docstring mentions "products, item groups, and parameters" but the function now seeds product, authorization, price list, and listing. Update to reflect the actual seeding scope.
Apply this diff:
- """Seed catalog data including products, item groups, and parameters.""" + """Seed catalog data including products, authorizations, price lists, and listings."""seed/seed_api.py (1)
20-20: Remove unused noqa directive.The
# noqa: WPS229directive is flagged as unused by Ruff. It can be safely removed.Apply this diff:
- try: # noqa: WPS229 + try:tests/seed/catalog/test_product.py (1)
31-31: Remove unusedcontextparameter.The
contextfixture is declared but never used in this test.Apply this diff:
-async def test_create_product(mocker, context: Context, vendor_client, product): +async def test_create_product(mocker, vendor_client, product):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
seed/assets/FIL-9920-4780-9379.pngis excluded by!**/*.pngseed/assets/empty.pdfis excluded by!**/*.pdfseed/assets/logo.pngis excluded by!**/*.png
📒 Files selected for processing (34)
mpt_api_client/resources/catalog/products.py(2 hunks)pyproject.toml(1 hunks)seed/accounts/api_tokens.py(3 hunks)seed/accounts/buyer.py(3 hunks)seed/accounts/licensee.py(4 hunks)seed/accounts/module.py(2 hunks)seed/accounts/seller.py(2 hunks)seed/accounts/user_group.py(3 hunks)seed/assets/assets.py(1 hunks)seed/catalog/authorization.py(1 hunks)seed/catalog/catalog.py(1 hunks)seed/catalog/item.py(0 hunks)seed/catalog/item_group.py(0 hunks)seed/catalog/listing.py(1 hunks)seed/catalog/price_list.py(1 hunks)seed/catalog/product.py(1 hunks)seed/catalog/product_parameters.py(0 hunks)seed/catalog/product_parameters_group.py(0 hunks)seed/container.py(1 hunks)seed/context.py(1 hunks)seed/defaults.py(0 hunks)seed/helper.py(1 hunks)seed/seed_api.py(1 hunks)tests/seed/catalog/test_authorization.py(1 hunks)tests/seed/catalog/test_catalog.py(1 hunks)tests/seed/catalog/test_item.py(0 hunks)tests/seed/catalog/test_item_group.py(0 hunks)tests/seed/catalog/test_listing.py(1 hunks)tests/seed/catalog/test_price_list.py(1 hunks)tests/seed/catalog/test_product.py(2 hunks)tests/seed/catalog/test_product_parameters.py(0 hunks)tests/seed/catalog/test_product_parameters_group.py(0 hunks)tests/seed/test_context.py(1 hunks)tests/seed/test_helper.py(1 hunks)
💤 Files with no reviewable changes (9)
- seed/catalog/product_parameters.py
- seed/catalog/item.py
- tests/seed/catalog/test_product_parameters_group.py
- tests/seed/catalog/test_item.py
- seed/catalog/product_parameters_group.py
- seed/catalog/item_group.py
- tests/seed/catalog/test_item_group.py
- seed/defaults.py
- tests/seed/catalog/test_product_parameters.py
🚧 Files skipped from review as they are similar to previous changes (12)
- seed/helper.py
- seed/catalog/authorization.py
- seed/assets/assets.py
- seed/accounts/api_tokens.py
- seed/container.py
- tests/seed/test_context.py
- tests/seed/catalog/test_authorization.py
- pyproject.toml
- seed/accounts/user_group.py
- tests/seed/catalog/test_catalog.py
- tests/seed/test_helper.py
- seed/accounts/seller.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-01T10:48:52.586Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 136
File: tests/e2e/notifications/subscribers/conftest.py:14-25
Timestamp: 2025-12-01T10:48:52.586Z
Learning: The repository softwareone-platform/mpt-api-python-client uses wemake-python-styleguide linter, so noqa directives for WPS rules should not be flagged as unused.
Applied to files:
tests/seed/catalog/test_product.py
🧬 Code graph analysis (11)
seed/context.py (1)
tests/seed/conftest.py (1)
context(14-15)
seed/catalog/price_list.py (3)
mpt_api_client/resources/catalog/price_lists.py (1)
PriceList(15-16)seed/context.py (1)
Context(12-33)seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)
seed/seed_api.py (5)
seed/accounts/accounts.py (1)
seed_accounts(13-23)seed/catalog/catalog.py (1)
seed_catalog(11-19)seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (3)
Context(12-33)load_context(36-50)save_context(53-61)
seed/accounts/module.py (7)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/modules.py (1)
Module(11-12)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)tests/e2e/conftest.py (1)
logger(63-64)
seed/accounts/licensee.py (6)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/licensees.py (1)
Licensee(21-22)seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)tests/e2e/conftest.py (1)
mpt_client(43-44)
seed/catalog/listing.py (3)
mpt_api_client/resources/catalog/listings.py (1)
Listing(11-12)seed/container.py (1)
Container(7-32)seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)
mpt_api_client/resources/catalog/products.py (2)
mpt_api_client/http/mixins.py (4)
CreateFileMixin(115-144)UpdateFileMixin(147-184)AsyncCreateFileMixin(187-216)AsyncUpdateFileMixin(219-256)mpt_api_client/resources/catalog/mixins.py (2)
PublishableMixin(11-45)AsyncPublishableMixin(48-82)
seed/accounts/buyer.py (5)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/buyers.py (1)
Buyer(26-27)seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)
seed/catalog/catalog.py (4)
seed/catalog/authorization.py (1)
seed_authorization(12-14)seed/catalog/listing.py (1)
seed_listing(10-12)seed/catalog/price_list.py (1)
seed_price_list(10-12)seed/catalog/product.py (1)
seed_product(37-52)
tests/seed/catalog/test_product.py (2)
seed/catalog/product.py (10)
create_document(155-171)create_item_group(175-192)create_parameter(126-151)create_parameter_group(110-122)create_product(26-34)create_product_item(208-236)create_template(81-93)create_terms(97-106)create_terms_variant(56-77)create_unit_of_measure(196-204)seed/context.py (1)
Context(12-33)
seed/catalog/product.py (18)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/catalog/products.py (15)
items(79-83)items(142-146)Product(53-54)terms(121-123)terms(184-188)templates(115-119)templates(178-182)parameter_groups(91-95)parameter_groups(154-158)parameters(109-113)parameters(172-176)documents(103-107)documents(166-170)item_groups(85-89)item_groups(148-152)mpt_api_client/resources/catalog/items.py (1)
Item(15-16)mpt_api_client/resources/catalog/product_term_variants.py (1)
TermVariant(19-20)mpt_api_client/resources/catalog/product_terms.py (1)
Term(16-17)mpt_api_client/resources/catalog/products_documents.py (1)
Document(15-16)mpt_api_client/resources/catalog/products_item_groups.py (1)
ItemGroup(11-12)mpt_api_client/resources/catalog/products_parameter_groups.py (1)
ParameterGroup(11-12)mpt_api_client/resources/catalog/products_parameters.py (1)
Parameter(11-12)mpt_api_client/resources/catalog/products_templates.py (1)
Template(11-12)mpt_api_client/resources/catalog/units_of_measure.py (1)
UnitOfMeasure(11-12)seed/container.py (1)
Container(7-32)tests/seed/conftest.py (1)
context(14-15)seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)tests/e2e/conftest.py (4)
logger(63-64)mpt_vendor(19-20)product_id(96-97)short_uuid(101-102)tests/e2e/catalog/product/conftest.py (2)
parameter_group_id(10-11)item_group_id(20-21)tests/e2e/catalog/product/parameters/conftest.py (1)
parameter_data(5-21)tests/e2e/catalog/product/documents/conftest.py (1)
document_data(10-16)
🪛 Ruff (0.14.8)
seed/seed_api.py
20-20: Unused noqa directive (unknown: WPS229)
Remove unused noqa directive
(RUF100)
seed/catalog/listing.py
16-16: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
tests/seed/catalog/test_product.py
4-4: Unused noqa directive (unknown: WPS235)
Remove unused noqa directive
(RUF100)
31-31: Unused function argument: context
(ARG001)
seed/catalog/product.py
38-38: Unused function argument: context
(ARG001)
tests/seed/catalog/test_listing.py
18-18: Unused noqa directive (unknown: WPS218)
Remove unused noqa directive
(RUF100)
34-34: Unused function argument: context_with_data
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
mpt_api_client/resources/catalog/products.py (1)
68-70: LGTM! Type parameterization improves type safety.The explicit generic type parameters on the file-related and publishable mixins now align with the existing pattern used by other mixins (GetMixin, CollectionMixin, Service). This enables type checkers to correctly infer that methods return Product instances, improving IDE support and static analysis without affecting runtime behavior.
Also applies to: 131-133
tests/seed/catalog/test_price_list.py (1)
1-35: LGTM! Well-structured test coverage.The tests correctly validate:
- Payload construction with context-sourced IDs
- Integration with the mocked operations client
init_resourceorchestration via patchingseed/catalog/price_list.py (1)
1-30: LGTM! Clean DI-wired price list seeding.The implementation correctly:
- Uses
@injectdecorator oncreate_price_listfor DI resolution- Validates required context IDs with proper error messages
- Delegates orchestration to
init_resourceseed/accounts/module.py (1)
3-72: LGTM! Proper DI wiring throughout.The refactoring correctly applies dependency injection:
- All functions using
Providehave@injectdecorators- Container imports are properly added
- Existing logic is preserved
albertsola
force-pushed
the
MPT-16255/Seeding-for-catalog
branch
from
adbaf61 to
f0651f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
seed/context.py (1)
36-50: Remove inaccurate "Returns" section from docstringThe docstring for
load_contextstill claims "Returns: Context instance." on line 43-44, but the function has a-> Nonereturn type and mutates the passedContextin place. All callers (seed/seed_api.py:19,tests/seed/test_context.py:13) already use it correctly as a void operation, so remove or update the Returns section in the docstring to match the actual behavior.
♻️ Duplicate comments (4)
seed/catalog/catalog.py (1)
3-6: Ensure account prerequisites are seeded before catalog seeding
seed_catalog()now sequentially callsseed_product(),seed_authorization(),seed_price_list(), andseed_listing(). Both authorization and listing rely on context keys such asaccounts.seller.id(and likelyaccounts.account.id), which this function does not populate.Given the earlier
ResourceRequiredError: Missing required resource 'accounts.seller.id'mentioned in the PR, please double‑check that your top‑level seeding flow guarantees that the relevant account seeders run beforeseed_catalog()(or, alternatively, extendseed_catalog()itself to include those prerequisites, and/or update its docstring to document required preconditions).If accounts are already seeded earlier in the flow, a brief comment or docstring note here would help future callers avoid invoking
seed_catalog()in isolation and hitting the same error.#!/bin/bash # Optional: verify how seed_catalog is used and whether account seeders run first. rg -n "seed_catalog\(" -C3 rg -n "seed_seller\(" -C3 || true rg -n "seed_account" -C3 || trueAlso applies to: 11-17
tests/seed/catalog/test_listing.py (1)
18-31: Clean up unusednoqaand unused fixture argument in listing testsThe behavior checks look good, but there are two small lint issues:
- Line 18:
# noqa: WPS218is reported as unused (RUF100) and can be removed.- Line 34:
context_with_datais never used intest_seed_listing(ARG001); you can drop it from the signature.A minimal fix:
-async def test_create_listing(mocker, operations_client, context_with_data): # noqa: WPS218 +async def test_create_listing(mocker, operations_client, context_with_data): -async def test_seed_listing(mocker, context_with_data): +async def test_seed_listing(mocker):This keeps test behavior unchanged while satisfying the linter.
Also applies to: 34-43
seed/accounts/buyer.py (1)
14-14: Incorrect logo file path will cause runtime failure.The past review identified that the logo file exists at
seed/assets/logo.png, but this line still points toseed/data/logo.png. When Line 76 attempts to open the file for buyer creation, it will fail with a FileNotFoundError.Apply this diff:
-icon = pathlib.Path(__file__).parent.parent / "data/logo.png" +icon = pathlib.Path(__file__).parent.parent / "assets/logo.png"seed/seed_api.py (1)
20-22: Seeding order is still incorrect despite past review.Past review comment (marked addressed in 7e489c9) explicitly stated catalog seeding depends on
accounts.seller.idandaccounts.account.id, which are created byseed_accounts(). The PR comment confirms this: "ResourceRequiredError: Missing required resource 'accounts.seller.id' before Create authorization."However, the current code still seeds catalog before accounts, which will fail on a fresh context.
Apply this diff to fix the order:
try: # noqa: WPS229 - await seed_catalog() await seed_accounts() + await seed_catalog() except Exception:
🧹 Nitpick comments (5)
tests/seed/catalog/test_catalog.py (1)
8-13: Catalog seeding orchestration test is solid; consider asserting order if it mattersThe test correctly verifies that
seed_catalog()invokesseed_product,seed_authorization,seed_price_list, andseed_listingexactly once. If the order (product → authorization → price list → listing) is important to keep stable, you might optionally extend the test to assert invocation order as well; otherwise this level of coverage is fine.Also applies to: 17-20
seed/accounts/module.py (1)
3-4: Module seeding DI looks consistent; consider tightening docs and decoratorsThe overall flow (
get_module→refresh_module→seed_module) is sound and matches the new DI patterns (usingProvide[Container.context]andProvide[Container.mpt_operations]).Two small nits you might want to address:
refresh_module’s docstring says “always fetch”, but ifaccounts.module.idis already set and aModuleis present in the context, it just returns that cached instance without hitting the API. Either update the docstring to describe the actual behavior (“ensure module is present, fetch if missing”) or change the implementation to truly always fetch from the API when called.seed_moduleis decorated with@injectbut doesn’t take any injectable parameters; you can safely drop the decorator there, since injection is already handled inget_module/refresh_module.These are purely clarity/maintainability improvements; the runtime behavior looks fine.
Also applies to: 14-32, 35-57, 60-72
seed/accounts/seller.py (1)
59-59: Consider explicit parameter passing for consistency.For consistency with the buyer.py pattern (line 72), consider explicitly passing the injected dependencies:
- seller = await get_seller() + seller = await get_seller(context=context, mpt_operations=mpt_operations)Both approaches work with DI, but explicit passing makes the data flow clearer and aligns with patterns elsewhere in the seed modules.
tests/seed/catalog/test_product.py (1)
31-31: Unused test parameter.The
contextparameter is declared but never used in this test. Consider removing it for clarity:-async def test_create_product(mocker, context: Context, vendor_client, product): +async def test_create_product(mocker, vendor_client, product):seed/catalog/product.py (1)
37-39: Unused function parameter.The
contextparameter is declared but never used in the function body. Since eachinit_resourcecall obtains its own context via DI, this parameter can be removed:-async def seed_product( - context: Context = Provide[Container.context], -) -> None: +async def seed_product() -> None:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
seed/assets/FIL-9920-4780-9379.pngis excluded by!**/*.pngseed/assets/empty.pdfis excluded by!**/*.pdfseed/assets/logo.pngis excluded by!**/*.png
📒 Files selected for processing (34)
mpt_api_client/resources/catalog/products.py(2 hunks)pyproject.toml(1 hunks)seed/accounts/api_tokens.py(3 hunks)seed/accounts/buyer.py(3 hunks)seed/accounts/licensee.py(4 hunks)seed/accounts/module.py(2 hunks)seed/accounts/seller.py(3 hunks)seed/accounts/user_group.py(3 hunks)seed/assets/assets.py(1 hunks)seed/catalog/authorization.py(1 hunks)seed/catalog/catalog.py(1 hunks)seed/catalog/item.py(0 hunks)seed/catalog/item_group.py(0 hunks)seed/catalog/listing.py(1 hunks)seed/catalog/price_list.py(1 hunks)seed/catalog/product.py(1 hunks)seed/catalog/product_parameters.py(0 hunks)seed/catalog/product_parameters_group.py(0 hunks)seed/container.py(1 hunks)seed/context.py(1 hunks)seed/defaults.py(0 hunks)seed/helper.py(1 hunks)seed/seed_api.py(1 hunks)tests/seed/catalog/test_authorization.py(1 hunks)tests/seed/catalog/test_catalog.py(1 hunks)tests/seed/catalog/test_item.py(0 hunks)tests/seed/catalog/test_item_group.py(0 hunks)tests/seed/catalog/test_listing.py(1 hunks)tests/seed/catalog/test_price_list.py(1 hunks)tests/seed/catalog/test_product.py(2 hunks)tests/seed/catalog/test_product_parameters.py(0 hunks)tests/seed/catalog/test_product_parameters_group.py(0 hunks)tests/seed/test_context.py(1 hunks)tests/seed/test_helper.py(1 hunks)
💤 Files with no reviewable changes (9)
- tests/seed/catalog/test_product_parameters.py
- tests/seed/catalog/test_item_group.py
- tests/seed/catalog/test_item.py
- seed/defaults.py
- seed/catalog/item.py
- seed/catalog/product_parameters_group.py
- seed/catalog/item_group.py
- tests/seed/catalog/test_product_parameters_group.py
- seed/catalog/product_parameters.py
🚧 Files skipped from review as they are similar to previous changes (9)
- pyproject.toml
- tests/seed/test_helper.py
- tests/seed/catalog/test_authorization.py
- seed/catalog/authorization.py
- seed/assets/assets.py
- seed/helper.py
- tests/seed/catalog/test_price_list.py
- seed/accounts/licensee.py
- seed/accounts/user_group.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-01T10:48:52.586Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 136
File: tests/e2e/notifications/subscribers/conftest.py:14-25
Timestamp: 2025-12-01T10:48:52.586Z
Learning: The repository softwareone-platform/mpt-api-python-client uses wemake-python-styleguide linter, so noqa directives for WPS rules should not be flagged as unused.
Applied to files:
tests/seed/catalog/test_product.py
🧬 Code graph analysis (11)
seed/catalog/listing.py (6)
mpt_api_client/resources/catalog/listings.py (1)
Listing(11-12)seed/container.py (1)
Container(7-30)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)tests/e2e/catalog/listings/conftest.py (1)
listing_data(10-29)
seed/catalog/price_list.py (5)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/catalog/price_lists.py (1)
PriceList(15-16)seed/container.py (1)
Container(7-30)seed/context.py (1)
Context(12-33)seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)
seed/accounts/seller.py (6)
mpt_api_client/resources/accounts/sellers.py (1)
Seller(13-14)seed/container.py (1)
Container(7-30)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)tests/e2e/conftest.py (1)
logger(63-64)tests/seed/accounts/test_seller.py (1)
seller(9-10)
seed/seed_api.py (4)
seed/accounts/accounts.py (1)
seed_accounts(13-23)seed/catalog/catalog.py (1)
seed_catalog(11-19)seed/container.py (1)
Container(7-30)seed/context.py (3)
Context(12-33)load_context(36-50)save_context(53-61)
seed/context.py (1)
tests/seed/conftest.py (1)
context(14-15)
seed/catalog/catalog.py (6)
seed/catalog/authorization.py (1)
seed_authorization(12-14)seed/catalog/listing.py (1)
seed_listing(10-12)seed/catalog/price_list.py (1)
seed_price_list(10-12)tests/seed/catalog/test_product.py (1)
product(20-21)seed/catalog/product.py (1)
seed_product(37-52)tests/e2e/conftest.py (1)
logger(63-64)
tests/seed/catalog/test_product.py (2)
seed/catalog/product.py (9)
create_document(155-171)create_parameter(126-151)create_parameter_group(110-122)create_product(26-34)create_product_item(208-236)create_template(81-93)create_terms(97-106)create_terms_variant(56-77)create_unit_of_measure(196-204)seed/context.py (1)
Context(12-33)
seed/accounts/module.py (3)
seed/container.py (1)
Container(7-30)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)
seed/accounts/buyer.py (4)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/buyers.py (1)
Buyer(26-27)seed/container.py (1)
Container(7-30)seed/context.py (1)
Context(12-33)
tests/seed/catalog/test_catalog.py (5)
seed/catalog/catalog.py (1)
seed_catalog(11-19)seed/catalog/product.py (1)
seed_product(37-52)seed/catalog/authorization.py (1)
seed_authorization(12-14)seed/catalog/price_list.py (1)
seed_price_list(10-12)seed/catalog/listing.py (1)
seed_listing(10-12)
seed/catalog/product.py (13)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/catalog/products.py (15)
items(79-83)items(142-146)Product(53-54)terms(121-123)terms(184-188)templates(115-119)templates(178-182)parameter_groups(91-95)parameter_groups(154-158)parameters(109-113)parameters(172-176)documents(103-107)documents(166-170)item_groups(85-89)item_groups(148-152)mpt_api_client/resources/catalog/items.py (1)
Item(15-16)mpt_api_client/resources/catalog/product_term_variants.py (1)
TermVariant(19-20)mpt_api_client/resources/catalog/product_terms.py (1)
Term(16-17)mpt_api_client/resources/catalog/products_documents.py (1)
Document(15-16)mpt_api_client/resources/catalog/products_item_groups.py (1)
ItemGroup(11-12)mpt_api_client/resources/catalog/products_parameter_groups.py (1)
ParameterGroup(11-12)mpt_api_client/resources/catalog/products_parameters.py (1)
Parameter(11-12)mpt_api_client/resources/catalog/products_templates.py (1)
Template(11-12)mpt_api_client/resources/catalog/units_of_measure.py (1)
UnitOfMeasure(11-12)tests/seed/conftest.py (1)
context(14-15)seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)
🪛 Ruff (0.14.8)
seed/catalog/listing.py
16-16: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
seed/seed_api.py
20-20: Unused noqa directive (unknown: WPS229)
Remove unused noqa directive
(RUF100)
tests/seed/catalog/test_product.py
4-4: Unused noqa directive (unknown: WPS235)
Remove unused noqa directive
(RUF100)
31-31: Unused function argument: context
(ARG001)
seed/catalog/product.py
38-38: Unused function argument: context
(ARG001)
tests/seed/catalog/test_listing.py
18-18: Unused noqa directive (unknown: WPS218)
Remove unused noqa directive
(RUF100)
34-34: Unused function argument: context_with_data
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
mpt_api_client/resources/catalog/products.py (1)
67-76: Using Product as the mixin type parameter aligns the service’s generic surfaceSwitching the file-handling and publishable mixins to
CreateFileMixin[Product],UpdateFileMixin[Product], andPublishableMixin[Product](and their async variants) is consistent withService[Product]and tightens typing without changing behavior. No further changes needed here.Also applies to: 130-139
tests/seed/test_context.py (1)
7-17: load_context test accurately captures in-place merge behaviorThis test neatly covers the three key paths (preserve untouched keys, overwrite existing ones, add new ones) and matches the new “mutate provided Context” contract of
load_context. Looks good.seed/container.py (1)
41-43: Package-level wiring viapackages=["seed"]is a good simplificationWiring the whole
seedpackage keeps DI configuration concise and should cover allProvide[Container.*]usages underseed.*without having to list modules explicitly.
albertsola
force-pushed
the
MPT-16255/Seeding-for-catalog
branch
from
f0651f7 to
d6eaa77
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
seed/context.py (1)
36-50: Update the docstring to reflect the new return type.The function signature now returns
None, but the docstring at lines 43–44 still states "Returns: Context instance."Apply this diff:
Args: json_file: JSON file path. context: Context instance. - - Returns: - Context instance. - """
♻️ Duplicate comments (2)
seed/catalog/product.py (1)
37-52: Drop the unusedcontextparameter fromseed_product.
seed_productnever readscontext, and callers already invoke it asawait seed_product(). Keeping the parameter only triggers Ruff’s ARG001 and doesn’t buy anything. You can simplify to:-async def seed_product( - context: Context = Provide[Container.context], -) -> None: +async def seed_product() -> None: """Seed product data."""No call sites need to change.
tests/seed/catalog/test_product.py (1)
31-37: Remove unusedcontextparameter fromtest_create_product.
contextisn’t referenced in the body oftest_create_product, so it’s just triggering ARG001 from Ruff. If you don’t rely on thecontextfixture for side effects here, you can safely drop it from the signature.-async def test_create_product(mocker, context: Context, vendor_client, product): +async def test_create_product(mocker, vendor_client, product):
🧹 Nitpick comments (8)
seed/seed_api.py (1)
20-20: Remove unusednoqadirective.The
# noqa: WPS229comment is flagged as unused by Ruff.Apply this diff:
- try: # noqa: WPS229 + try:tests/seed/catalog/test_listing.py (2)
18-18: Remove unusednoqadirective.The
# noqa: WPS218comment is flagged as unused by Ruff.Apply this diff:
-async def test_create_listing(mocker, operations_client, context_with_data): # noqa: WPS218 +async def test_create_listing(mocker, operations_client, context_with_data):
34-43: Remove unused fixture parameter.The
context_with_datafixture is passed totest_seed_listingbut never used in the test body, sinceseed_listing()doesn't accept parameters andinit_resourceis mocked.Apply this diff:
-async def test_seed_listing(mocker, context_with_data): +async def test_seed_listing(mocker):seed/accounts/buyer.py (1)
37-45: Make the seller dependency and seeding order explicit.
build_buyer_datanow hard-requiresaccounts.seller.idin the context and raises if it’s missing. That’s reasonable, but it meansseed_buyer()will fail if invoked beforeseed_seller()or without running the seller seeding flow at all. Consider documenting/enforcing the expected order at the orchestration level (e.g., seller → buyer) or reusing the sharedrequire_context_idhelper for a consistent error type.seed/accounts/licensee.py (1)
50-52:Context.get_resourcewon’t returnNone; simplify the user-group check.
context.get_resource("accounts.user_group")raisesValueErrorif the group is missing or wrong-typed, sogroupcan’t beNonehere. If you want a clearer message (“User group is required in context”), either catch and re-raise theValueErrorwith that text or just drop theif group is Noneblock to avoid unreachable code.seed/accounts/seller.py (1)
53-60: Forward DI-provided dependencies frominit_sellerintoget_seller.
init_selleracceptscontextandmpt_operations, but currently callsget_seller()with no arguments, relying solely on its own DI defaults. For consistency with other modules (and easier testing/overrides), consider:- seller = await get_seller() + seller = await get_seller(context=context, mpt_operations=mpt_operations)This keeps behavior the same under normal wiring while ensuring explicitly-supplied dependencies are honored.
seed/catalog/product.py (1)
195-203: Aligncreate_unit_of_measuredocstring with the client actually used.The docstring says “in the vendor's catalog,” but this helper uses the
mpt_operationsclient. Either update the wording to something neutral like “in the catalog” or switch to the vendor client if “vendor” is the real intent.tests/seed/catalog/test_product.py (1)
101-110: Optionally assert the documentcreatemock was awaited.For consistency with the other tests (template, terms, parameter, unit of measure), you can also assert that the mocked
createcoroutine was awaited exactly once:- result = await create_document(context, vendor_client) - - assert result == {"id": "doc-1"} + result = await create_document(context, vendor_client) + + assert result == {"id": "doc-1"} + create_mock.assert_awaited_once()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
seed/static/FIL-9920-4780-9379.pngis excluded by!**/*.pngseed/static/empty.pdfis excluded by!**/*.pdfseed/static/logo.pngis excluded by!**/*.png
📒 Files selected for processing (34)
mpt_api_client/resources/catalog/products.py(2 hunks)pyproject.toml(1 hunks)seed/accounts/api_tokens.py(3 hunks)seed/accounts/buyer.py(3 hunks)seed/accounts/licensee.py(3 hunks)seed/accounts/module.py(2 hunks)seed/accounts/seller.py(3 hunks)seed/accounts/user_group.py(3 hunks)seed/catalog/authorization.py(1 hunks)seed/catalog/catalog.py(1 hunks)seed/catalog/item.py(0 hunks)seed/catalog/item_group.py(0 hunks)seed/catalog/listing.py(1 hunks)seed/catalog/price_list.py(1 hunks)seed/catalog/product.py(1 hunks)seed/catalog/product_parameters.py(0 hunks)seed/catalog/product_parameters_group.py(0 hunks)seed/container.py(1 hunks)seed/context.py(1 hunks)seed/defaults.py(0 hunks)seed/helper.py(1 hunks)seed/seed_api.py(1 hunks)seed/static/static.py(1 hunks)tests/seed/catalog/test_authorization.py(1 hunks)tests/seed/catalog/test_catalog.py(1 hunks)tests/seed/catalog/test_item.py(0 hunks)tests/seed/catalog/test_item_group.py(0 hunks)tests/seed/catalog/test_listing.py(1 hunks)tests/seed/catalog/test_price_list.py(1 hunks)tests/seed/catalog/test_product.py(2 hunks)tests/seed/catalog/test_product_parameters.py(0 hunks)tests/seed/catalog/test_product_parameters_group.py(0 hunks)tests/seed/test_context.py(1 hunks)tests/seed/test_helper.py(1 hunks)
💤 Files with no reviewable changes (9)
- seed/defaults.py
- seed/catalog/item_group.py
- seed/catalog/product_parameters.py
- tests/seed/catalog/test_item.py
- seed/catalog/product_parameters_group.py
- tests/seed/catalog/test_product_parameters_group.py
- tests/seed/catalog/test_product_parameters.py
- tests/seed/catalog/test_item_group.py
- seed/catalog/item.py
✅ Files skipped from review due to trivial changes (1)
- seed/static/static.py
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/seed/test_helper.py
- tests/seed/test_context.py
- seed/accounts/api_tokens.py
- mpt_api_client/resources/catalog/products.py
- tests/seed/catalog/test_authorization.py
- seed/container.py
- seed/helper.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-01T10:48:52.586Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 136
File: tests/e2e/notifications/subscribers/conftest.py:14-25
Timestamp: 2025-12-01T10:48:52.586Z
Learning: The repository softwareone-platform/mpt-api-python-client uses wemake-python-styleguide linter, so noqa directives for WPS rules should not be flagged as unused.
Applied to files:
tests/seed/catalog/test_product.py
🧬 Code graph analysis (12)
seed/accounts/buyer.py (3)
mpt_api_client/resources/accounts/buyers.py (1)
Buyer(26-27)seed/container.py (1)
Container(7-30)seed/context.py (1)
Context(12-33)
seed/seed_api.py (4)
seed/accounts/accounts.py (1)
seed_accounts(13-23)seed/catalog/catalog.py (1)
seed_catalog(11-19)seed/container.py (1)
Container(7-30)seed/context.py (3)
Context(12-33)load_context(36-50)save_context(53-61)
seed/accounts/licensee.py (5)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/licensees.py (1)
Licensee(21-22)seed/container.py (1)
Container(7-30)seed/context.py (1)
Context(12-33)tests/seed/accounts/test_licensee.py (1)
licensee(10-28)
seed/accounts/seller.py (4)
mpt_api_client/resources/accounts/sellers.py (1)
Seller(13-14)seed/container.py (1)
Container(7-30)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)
tests/seed/catalog/test_catalog.py (5)
seed/catalog/catalog.py (1)
seed_catalog(11-19)seed/catalog/product.py (1)
seed_product(37-52)seed/catalog/authorization.py (1)
seed_authorization(12-14)seed/catalog/price_list.py (1)
seed_price_list(10-12)seed/catalog/listing.py (1)
seed_listing(10-12)
seed/catalog/authorization.py (4)
mpt_api_client/resources/catalog/authorizations.py (1)
Authorization(11-12)seed/container.py (1)
Container(7-30)seed/context.py (1)
Context(12-33)seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)
seed/catalog/product.py (19)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/catalog/products.py (15)
items(79-83)items(142-146)Product(53-54)terms(121-123)terms(184-188)templates(115-119)templates(178-182)parameter_groups(91-95)parameter_groups(154-158)parameters(109-113)parameters(172-176)documents(103-107)documents(166-170)item_groups(85-89)item_groups(148-152)mpt_api_client/resources/catalog/items.py (1)
Item(15-16)mpt_api_client/resources/catalog/product_term_variants.py (1)
TermVariant(19-20)mpt_api_client/resources/catalog/product_terms.py (1)
Term(16-17)mpt_api_client/resources/catalog/products_documents.py (1)
Document(15-16)mpt_api_client/resources/catalog/products_item_groups.py (1)
ItemGroup(11-12)mpt_api_client/resources/catalog/products_parameter_groups.py (1)
ParameterGroup(11-12)mpt_api_client/resources/catalog/products_parameters.py (1)
Parameter(11-12)mpt_api_client/resources/catalog/products_templates.py (1)
Template(11-12)mpt_api_client/resources/catalog/units_of_measure.py (1)
UnitOfMeasure(11-12)seed/container.py (1)
Container(7-30)tests/seed/conftest.py (1)
context(14-15)seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)tests/e2e/conftest.py (3)
logger(63-64)mpt_vendor(19-20)product_id(96-97)tests/e2e/catalog/product/conftest.py (1)
parameter_group_id(10-11)tests/e2e/catalog/product/parameters/conftest.py (1)
parameter_data(5-21)tests/e2e/catalog/product/documents/conftest.py (1)
document_data(10-16)tests/e2e/catalog/product/item_groups/conftest.py (1)
item_group_data(5-15)
seed/accounts/module.py (2)
seed/container.py (1)
Container(7-30)seed/context.py (1)
Context(12-33)
seed/catalog/price_list.py (4)
mpt_api_client/resources/catalog/price_lists.py (1)
PriceList(15-16)seed/container.py (1)
Container(7-30)seed/context.py (1)
Context(12-33)seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)
seed/context.py (1)
tests/seed/conftest.py (1)
context(14-15)
tests/seed/catalog/test_product.py (1)
seed/catalog/product.py (10)
create_document(155-171)create_item_group(175-192)create_parameter(126-151)create_parameter_group(110-122)create_product(26-34)create_product_item(208-236)create_template(81-93)create_terms(97-106)create_terms_variant(56-77)create_unit_of_measure(196-204)
tests/seed/catalog/test_listing.py (3)
seed/catalog/listing.py (2)
create_listing(16-46)seed_listing(10-12)seed/context.py (1)
Context(12-33)seed/helper.py (1)
init_resource(44-84)
🪛 Ruff (0.14.8)
seed/seed_api.py
20-20: Unused noqa directive (unknown: WPS229)
Remove unused noqa directive
(RUF100)
seed/catalog/listing.py
16-16: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
seed/catalog/product.py
38-38: Unused function argument: context
(ARG001)
tests/seed/catalog/test_product.py
4-4: Unused noqa directive (unknown: WPS235)
Remove unused noqa directive
(RUF100)
31-31: Unused function argument: context
(ARG001)
tests/seed/catalog/test_listing.py
18-18: Unused noqa directive (unknown: WPS218)
Remove unused noqa directive
(RUF100)
34-34: Unused function argument: context_with_data
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (12)
pyproject.toml (1)
134-139: LGTM!The expanded per-file-ignores align with the new seed modules and testing additions introduced in this PR.
seed/seed_api.py (1)
16-22: LGTM!The sequential seeding order (accounts before catalog) correctly resolves the
ResourceRequiredErrorreported in the PR comments. The DI-based context parameter aligns well with the container-driven architecture introduced in this PR.seed/catalog/authorization.py (1)
12-39: LGTM!The implementation correctly uses
@inject,require_context_idfor dependency validation, and proper DI wiring. The authorization creation logic is clear and well-structured.tests/seed/catalog/test_price_list.py (1)
1-35: LGTM!The tests properly cover both the creation logic and the seeding orchestration. Mocking and assertions are appropriate.
seed/catalog/catalog.py (1)
11-19: LGTM!The sequential orchestration of catalog seeds is clear and maintains proper dependency order (product → authorization → price_list → listing). The account dependencies are correctly handled at the higher level in
seed_api.py.tests/seed/catalog/test_catalog.py (1)
6-20: LGTM!The test properly validates that
seed_catalogorchestrates all four seed functions in sequence. The use ofAsyncMockand assertions is appropriate.seed/accounts/user_group.py (1)
5-19: User-group DI wiring and seeding flow look consistent.The updated signatures using container-provided
Contextand operations are coherent with the rest of the seed modules, and the get/create/cache pattern remains unchanged.Also applies to: 36-60, 79-85
seed/catalog/price_list.py (1)
1-30: Price list seeding helper is coherent with the new helper pattern.
seed_price_listdelegating toinit_resource("catalog.price_list.id", create_price_list)and thecreate_price_listpayload (including product id and default markup) look correct and consistent with the other catalog seeders.seed/accounts/buyer.py (1)
4-33: Buyer seeding flow and DI wiring look good.The refactor to use container-provided
Contextand operations, the cached get-or-create logic, and the ICON-based file upload are all consistent with the rest of the seed modules and keep behavior clear.Also applies to: 64-84
seed/accounts/module.py (1)
3-72: Module DI wiring and seeding logic look sound.The switch to container-backed
Context/operations inget_module,refresh_module, andseed_modulepreserves the original behavior while aligning with the new DI approach; the “Access Management” fallback path remains intact.seed/catalog/product.py (1)
25-35: Product create/ helpers and seeding order look consistent and well-structured.*The new
create_*factories and theseed_productorchestration viainit_resourcefollow a clear pattern, respect context dependencies viarequire_context_id, and ensure resources like terms, parameters, documents, item groups, and items are created in a dependency-safe order.Also applies to: 55-78, 81-172, 175-236
tests/seed/catalog/test_product.py (1)
112-124: Good assertions on payload context propagation.The checks in
test_create_item_groupandtest_create_product_itemthat inspect the payload and validateproduct.id,unit.id, andgroup.idcome from theContextare solid and make these tests much more robust than just asserting return values.Also applies to: 136-149
| async def create_listing( # noqa: WPS210 | ||
| operations: AsyncMPTClient = Provide[Container.mpt_operations], | ||
| context: Context = Provide[Container.context], | ||
| ) -> Listing: |
There was a problem hiding this comment.
Remove unused # noqa: WPS210 on create_listing.
Ruff reports this as an unused/unknown noqa directive. Since there’s no active WPS210 check here, the comment only adds RUF100 noise; you can safely drop it.
🧰 Tools
🪛 Ruff (0.14.8)
16-16: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
🤖 Prompt for AI Agents
In seed/catalog/listing.py around lines 16 to 19, remove the unused inline noqa
directive "# noqa: WPS210" from the create_listing function definition; simply
delete that comment so the function signature reads normally without any noqa,
since WPS210 is not applicable and the directive is causing Ruff RUF100 noise.
d6eaa77 to
403cccd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
seed/accounts/api_tokens.py (1)
35-48: Add validation forCLIENT_ACCOUNT_IDenvironment variable.Unlike
build_user_group_datainuser_group.py(line 42-43), this function doesn't validate thatCLIENT_ACCOUNT_IDis set. If missing,account_idwill beNone, potentially causing API errors.@inject def build_api_token_data( context: Context = Provide[Container.context], ) -> dict[str, object]: """Get API token data dictionary for creation.""" account_id = os.getenv("CLIENT_ACCOUNT_ID") + if not account_id: + raise ValueError("CLIENT_ACCOUNT_ID environment variable is required") module_id = context.get_string("accounts.module.id") return {
♻️ Duplicate comments (2)
tests/seed/catalog/test_product.py (2)
4-4: noqa directive flagged by Ruff as unused.Ruff reports that the
noqa: WPS235directive is unknown. Based on learnings, the repository uses wemake-python-styleguide linter, so this may be a false positive from Ruff.
31-37: Remove unusedcontextparameter.The
contextparameter is not used in this test function.Apply this diff:
async def test_create_product( - mocker, context: Context, vendor_client, product + mocker, vendor_client, product ):
🧹 Nitpick comments (3)
seed/accounts/seller.py (1)
53-60: Forward DI dependencies frominit_sellerintoget_seller
init_selleracceptscontextandmpt_operations(DI-injected by default) but then callsget_seller()with no arguments. That’s fine for pure DI usage, but it makes explicit overrides (e.g., tests passing a customContextorAsyncMPTClient) inconsistent, becauseget_sellerwill still resolve its own defaults.Forwarding the parameters keeps the API predictable and composes DI more cleanly:
@inject async def init_seller( context: Context = Provide[Container.context], mpt_operations: AsyncMPTClient = Provide[Container.mpt_operations], ) -> Seller | None: """Get or create seller. Returns Seller if successful, None otherwise.""" - seller = await get_seller() + seller = await get_seller(context=context, mpt_operations=mpt_operations)seed/catalog/catalog.py (1)
11-17: Updateseed_catalogdocstring to reflect new responsibilitiesThe orchestration now also seeds authorization, price lists, and listings, but the docstring only mentions "products, item groups, and parameters". Consider broadening it to avoid confusion for callers:
-async def seed_catalog() -> None: - """Seed catalog data including products, item groups, and parameters.""" +async def seed_catalog() -> None: + """Seed catalog data (products, related entities, and listings)."""Optionally, you may also note that account resources are expected to be seeded beforehand (via
seed_apicallingseed_accounts()first).seed/helper.py (1)
43-77: Aligninit_resourcedocstring with DI-based default contextImplementation now injects the default
ContextviaProvide[Container.context]and@inject, but the docstring still refers to a non-existentDEFAULT_CONTEXT. This can confuse readers about how the default is provided.Consider updating the
contextparameter description:- context: The seeding Context used to read and persist the id. Defaults to - ``DEFAULT_CONTEXT``. + context: The seeding Context used to read and persist the id. By default + injected from ``Container.context`` via dependency-injector.The rest of the behavior description matches the implementation and tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
seed/static/FIL-9920-4780-9379.pngis excluded by!**/*.pngseed/static/empty.pdfis excluded by!**/*.pdfseed/static/logo.pngis excluded by!**/*.png
📒 Files selected for processing (34)
mpt_api_client/resources/catalog/products.py(2 hunks)pyproject.toml(1 hunks)seed/accounts/api_tokens.py(3 hunks)seed/accounts/buyer.py(3 hunks)seed/accounts/licensee.py(3 hunks)seed/accounts/module.py(2 hunks)seed/accounts/seller.py(3 hunks)seed/accounts/user_group.py(3 hunks)seed/catalog/authorization.py(1 hunks)seed/catalog/catalog.py(1 hunks)seed/catalog/item.py(0 hunks)seed/catalog/item_group.py(0 hunks)seed/catalog/listing.py(1 hunks)seed/catalog/price_list.py(1 hunks)seed/catalog/product.py(1 hunks)seed/catalog/product_parameters.py(0 hunks)seed/catalog/product_parameters_group.py(0 hunks)seed/container.py(1 hunks)seed/context.py(1 hunks)seed/defaults.py(0 hunks)seed/helper.py(1 hunks)seed/seed_api.py(1 hunks)seed/static/static.py(1 hunks)tests/seed/catalog/test_authorization.py(1 hunks)tests/seed/catalog/test_catalog.py(1 hunks)tests/seed/catalog/test_item.py(0 hunks)tests/seed/catalog/test_item_group.py(0 hunks)tests/seed/catalog/test_listing.py(1 hunks)tests/seed/catalog/test_price_list.py(1 hunks)tests/seed/catalog/test_product.py(2 hunks)tests/seed/catalog/test_product_parameters.py(0 hunks)tests/seed/catalog/test_product_parameters_group.py(0 hunks)tests/seed/test_context.py(1 hunks)tests/seed/test_helper.py(1 hunks)
💤 Files with no reviewable changes (9)
- tests/seed/catalog/test_item_group.py
- tests/seed/catalog/test_item.py
- seed/defaults.py
- seed/catalog/product_parameters_group.py
- seed/catalog/product_parameters.py
- seed/catalog/item_group.py
- tests/seed/catalog/test_product_parameters.py
- tests/seed/catalog/test_product_parameters_group.py
- seed/catalog/item.py
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/seed/test_context.py
- seed/context.py
- seed/accounts/module.py
- tests/seed/catalog/test_price_list.py
- seed/catalog/authorization.py
- seed/container.py
- tests/seed/catalog/test_authorization.py
- pyproject.toml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-01T10:48:52.586Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 136
File: tests/e2e/notifications/subscribers/conftest.py:14-25
Timestamp: 2025-12-01T10:48:52.586Z
Learning: The repository softwareone-platform/mpt-api-python-client uses wemake-python-styleguide linter, so noqa directives for WPS rules should not be flagged as unused.
Applied to files:
tests/seed/catalog/test_product.pyseed/catalog/listing.py
🧬 Code graph analysis (13)
seed/accounts/user_group.py (4)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/user_groups.py (1)
UserGroup(11-12)seed/container.py (1)
Container(7-30)seed/context.py (1)
Context(12-33)
seed/accounts/licensee.py (5)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/licensees.py (1)
Licensee(21-22)seed/container.py (1)
Container(7-30)seed/context.py (1)
Context(12-33)tests/seed/accounts/test_licensee.py (1)
licensee(10-28)
tests/seed/catalog/test_catalog.py (5)
seed/catalog/catalog.py (1)
seed_catalog(11-19)seed/catalog/product.py (1)
seed_product(37-50)seed/catalog/authorization.py (1)
seed_authorization(12-14)seed/catalog/price_list.py (1)
seed_price_list(10-12)seed/catalog/listing.py (1)
seed_listing(10-12)
mpt_api_client/resources/catalog/products.py (2)
mpt_api_client/http/mixins.py (4)
CreateFileMixin(115-144)UpdateFileMixin(147-184)AsyncCreateFileMixin(187-216)AsyncUpdateFileMixin(219-256)mpt_api_client/resources/catalog/mixins.py (2)
PublishableMixin(11-45)AsyncPublishableMixin(48-82)
seed/accounts/api_tokens.py (4)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)seed/container.py (1)
Container(7-30)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)
tests/seed/catalog/test_product.py (2)
seed/catalog/product.py (10)
create_document(153-169)create_item_group(173-190)create_parameter(124-149)create_parameter_group(108-120)create_product(26-34)create_product_item(206-234)create_template(79-91)create_terms(95-104)create_terms_variant(54-75)create_unit_of_measure(194-202)seed/context.py (1)
Context(12-33)
tests/seed/test_helper.py (4)
tests/seed/conftest.py (1)
context(14-15)seed/context.py (2)
Context(12-33)get_string(15-17)seed/helper.py (3)
ResourceRequiredError(13-20)init_resource(44-84)require_context_id(23-40)mpt_api_client/models/model.py (1)
id(119-121)
seed/catalog/catalog.py (4)
seed/catalog/authorization.py (1)
seed_authorization(12-14)seed/catalog/listing.py (1)
seed_listing(10-12)seed/catalog/price_list.py (1)
seed_price_list(10-12)seed/catalog/product.py (1)
seed_product(37-50)
seed/seed_api.py (5)
seed/accounts/accounts.py (1)
seed_accounts(13-23)seed/catalog/catalog.py (1)
seed_catalog(11-19)seed/container.py (1)
Container(7-30)tests/seed/conftest.py (1)
context(14-15)seed/context.py (2)
Context(12-33)load_context(36-50)
seed/helper.py (2)
seed/container.py (1)
Container(7-30)seed/context.py (2)
Context(12-33)get_string(15-17)
seed/catalog/price_list.py (5)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/catalog/price_lists.py (1)
PriceList(15-16)seed/container.py (1)
Container(7-30)seed/context.py (1)
Context(12-33)seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)
seed/accounts/seller.py (7)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/sellers.py (1)
Seller(13-14)seed/container.py (1)
Container(7-30)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)tests/e2e/conftest.py (1)
logger(63-64)tests/seed/accounts/test_seller.py (1)
seller(9-10)
seed/catalog/product.py (13)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/catalog/items.py (1)
Item(15-16)mpt_api_client/resources/catalog/product_term_variants.py (1)
TermVariant(19-20)mpt_api_client/resources/catalog/product_terms.py (1)
Term(16-17)mpt_api_client/resources/catalog/products_documents.py (1)
Document(15-16)mpt_api_client/resources/catalog/products_item_groups.py (1)
ItemGroup(11-12)mpt_api_client/resources/catalog/products_parameter_groups.py (1)
ParameterGroup(11-12)mpt_api_client/resources/catalog/products_parameters.py (1)
Parameter(11-12)mpt_api_client/resources/catalog/products_templates.py (1)
Template(11-12)mpt_api_client/resources/catalog/units_of_measure.py (1)
UnitOfMeasure(11-12)seed/container.py (1)
Container(7-30)seed/context.py (1)
Context(12-33)seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)
🪛 Ruff (0.14.8)
tests/seed/catalog/test_product.py
4-4: Unused noqa directive (unknown: WPS235)
Remove unused noqa directive
(RUF100)
31-31: Unused function argument: context
(ARG001)
seed/catalog/listing.py
16-16: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
seed/seed_api.py
20-20: Unused noqa directive (unknown: WPS229)
Remove unused noqa directive
(RUF100)
tests/seed/catalog/test_listing.py
18-18: Unused noqa directive (unknown: WPS218)
Remove unused noqa directive
(RUF100)
34-34: Unused function argument: context_with_data
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (38)
seed/static/static.py (1)
3-5: Verify intentional duplication of ICON and LOGO paths.Both
ICONandLOGOreference the same file (FIL-9920-4780-9379.png). If this is intentional (e.g., using the same image for both purposes during seeding), consider adding a brief comment to clarify. If different images are expected, update one of the paths.seed/accounts/user_group.py (5)
5-9: LGTM!The DI wiring migration is correctly implemented. The imports for
Provide,inject, andContainerare properly added, aligning with the project-wide shift to container-based dependency resolution.
15-33: LGTM!The function correctly implements the pattern of checking context first, then fetching from API if needed, and caching the result. DI wiring is properly applied.
36-52: LGTM!The function correctly validates the required environment variable and builds the user group data payload with proper context dependency injection.
55-76: LGTM!The init function follows the established pattern: attempt to retrieve, create if missing, and properly cache the result. Logging is appropriate.
79-85: LGTM!The seed entry point is clean and correctly delegates to
init_user_group.seed/accounts/api_tokens.py (4)
4-9: LGTM!The DI wiring imports are correctly added, consistent with the project-wide migration pattern.
14-32: LGTM!The function follows the established pattern for retrieving resources from context or API.
51-67: LGTM!The function correctly implements the get-or-create pattern with proper context caching.
70-75: LGTM!The seed entry point is clean and correctly delegates to
init_api_token.seed/catalog/price_list.py (3)
1-7: LGTM!The imports are correctly structured with DI wiring components and helper utilities.
10-12: LGTM!The function correctly delegates to
init_resourcewithout needing the@injectdecorator since it doesn't useProvide[...]parameters directly.
15-30: LGTM!The function is correctly decorated with
@inject, validates required context dependencies, and constructs the price list payload appropriately. The past review issues have been addressed.Verify that
defaultMarkupas a string ("20.0") matches the API's expected format. Some APIs expect numeric values for markup fields.mpt_api_client/resources/catalog/products.py (2)
67-76: LGTM!The type parameter change from
ModeltoProductforCreateFileMixin,UpdateFileMixin, andPublishableMixinimproves type safety and aligns with the existing pattern used by other mixins (GetMixin[Product],CollectionMixin[Product]). This provides more accurate return type hints.
130-139: LGTM!The async service class receives the same type parameter improvements, maintaining consistency between sync and async implementations.
tests/seed/catalog/test_catalog.py (1)
6-20: Orchestration test forseed_cataloglooks solidPatching the four seed functions and asserting each is called once is a clear, minimal way to verify the new catalog seeding flow. This provides good coverage at the orchestration layer without over-coupling to implementation details.
tests/seed/test_helper.py (1)
7-50: Good behavioral coverage forrequire_context_idandinit_resourceThese tests exercise both success and failure paths for
require_context_id, and both cached and creation paths forinit_resource, including context mutation and factory call expectations. This should catch regressions in the core helper behavior.seed/seed_api.py (1)
16-26: Seeding order fix inseed_apicorrectly addresses catalog prerequisitesCalling
seed_accounts()beforeseed_catalog()ensures that required account IDs (e.g.,accounts.seller.id,accounts.account.id) exist in the context before catalog seeders callrequire_context_id. With DI-backedcontextand the load/save around the try/except, this entrypoint should now handle the reportedResourceRequiredErrorcase cleanly.seed/catalog/listing.py (1)
10-46: Listing seeding flow and DI wiring are coherent
seed_listing()delegating toinit_resource("catalog.listing.id", create_listing)andcreate_listingresolvingoperationsandcontextvia DI is a clean fit with the new helper pattern. The required context keys (catalog.product.id,accounts.seller.id,catalog.authorization.id,accounts.account.id,catalog.price_list.id) align with what other seeders populate and what the tests assert on in the payload.No functional issues spotted here.
seed/accounts/buyer.py (4)
4-10: LGTM!The DI wiring imports and ICON asset import are correct and align with the project's dependency injection pattern.
15-33: LGTM!The DI wiring for
get_buyeris correctly implemented with the@injectdecorator andProvide[Container.*]parameters.
36-61: LGTM!The validation logic properly checks for required environment variables and context IDs with clear error messages.
64-84: LGTM!The DI wiring and ICON file handling are correctly implemented. The function properly handles buyer creation with appropriate logging and error handling.
seed/accounts/licensee.py (4)
4-10: LGTM!The DI wiring imports are consistent with the project's dependency injection pattern.
15-33: LGTM!The DI wiring is correctly implemented.
36-72: LGTM!The validation logic properly checks all required dependencies (seller, buyer, user group) with clear error messages.
75-95: LGTM!The DI wiring and ICON file handling are correctly implemented with appropriate error handling and logging.
tests/seed/catalog/test_product.py (1)
40-149: LGTM!The test coverage is comprehensive, validating both the created resources and payload structures, including proper ID propagation from context.
seed/catalog/product.py (10)
1-22: LGTM!The imports are comprehensive and all are utilized in the code. The addition of DI wiring and helper utilities aligns with the refactored architecture.
25-34: LGTM!The
create_productfunction correctly implements DI wiring and file handling.
37-50: LGTM!The orchestration is clear and sequential, using
init_resourceto manage resource creation and caching. The removal of the unusedcontextparameter was correctly applied.
53-75: LGTM!The function correctly uses
require_context_idfor validation and properly chains the API calls for creating a term variant.
78-91: LGTM!The function correctly implements DI wiring and validation.
94-104: LGTM!The function is correctly implemented.
107-149: LGTM!Both functions correctly implement DI wiring and validation with appropriate context ID checks.
152-169: LGTM!The function correctly handles PDF file upload with proper DI wiring.
172-190: LGTM!The function correctly creates an item group with proper product reference.
193-234: LGTM!Both functions are correctly implemented. The use of UUID for uniqueness in unit names and external IDs is appropriate, and the validation in
create_product_itemproperly checks all required context IDs.
| async def test_seed_listing(mocker, context_with_data): | ||
| init_resource = mocker.patch( | ||
| "seed.catalog.listing.init_resource", | ||
| new_callable=mocker.AsyncMock, | ||
| return_value=mocker.Mock(id="lst-999"), | ||
| ) | ||
|
|
||
| await seed_listing() | ||
|
|
||
| init_resource.assert_awaited_once_with("catalog.listing.id", create_listing) |
There was a problem hiding this comment.
Remove unused context_with_data parameter in test_seed_listing
context_with_data is injected into test_seed_listing but never used inside the test body, which triggers ARG001 and adds noise.
You can safely drop it from the signature; the fixture remains used by test_create_listing:
-async def test_seed_listing(mocker, context_with_data):
+async def test_seed_listing(mocker):
init_resource = mocker.patch(
"seed.catalog.listing.init_resource",
new_callable=mocker.AsyncMock,
return_value=mocker.Mock(id="lst-999"),
)🧰 Tools
🪛 Ruff (0.14.8)
34-34: Unused function argument: context_with_data
(ARG001)
🤖 Prompt for AI Agents
In tests/seed/catalog/test_listing.py around lines 34 to 43, the
test_seed_listing function declares an unused fixture parameter
context_with_data which triggers ARG001; remove context_with_data from the
test_seed_listing signature so the fixture is no longer passed to this test
(leave other tests that need it unchanged) and run the test suite to confirm no
dependent usages remain.
|
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.