[MPT-15443] Seeded accounts e2e data #144
Conversation
robcsegal
requested review from
albertsola,
alephsur,
d3rky,
jentyk,
ruben-sebrango and
svazquezco
as code owners
WalkthroughAdds a new accounts seeding subsystem (seller, buyer, module, api token, user group, licensee) with per-entity get/build/init/seed functions, wires them into the DI container, runs accounts seeding concurrently with catalog, updates product seeding to consult context, adds tests and pyfakefs dev dependency. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant SeedAPI as seed_api()
participant Accounts as seed_accounts()
participant Seller as seed_seller()
participant Buyer as seed_buyer()
participant Module as seed_module()
participant APIToken as seed_api_token()
participant UserGroup as seed_user_group()
participant Licensee as seed_licensee()
participant Context as Context
participant MPT as AsyncMPTClient
SeedAPI->>+Accounts: start accounts seeding (concurrent with catalog)
Accounts->>+Seller: await seed_seller()
Seller->>MPT: get/create seller
MPT-->>Seller: Seller resource
Seller->>Context: store seller id/resource
Accounts->>+Buyer: await seed_buyer()
Buyer->>MPT: get/create buyer (may upload logo)
MPT-->>Buyer: Buyer resource
Buyer->>Context: store buyer id/resource
Accounts->>+Module: await seed_module()
Module->>MPT: get or filter modules
MPT-->>Module: Module resource(s)
Module->>Context: store module id/resource
Accounts->>+APIToken: await seed_api_token()
APIToken->>MPT: get/create api token
MPT-->>APIToken: ApiToken resource
APIToken->>Context: store token id/resource
Accounts->>+UserGroup: await seed_user_group()
UserGroup->>MPT: get/create user group (refs modules/buyers)
MPT-->>UserGroup: UserGroup resource
UserGroup->>Context: store user_group id/resource
Accounts->>+Licensee: await seed_licensee()
Licensee->>MPT: get/create licensee (may upload icon)
MPT-->>Licensee: Licensee resource
Licensee->>Context: store licensee id/resource
Accounts-->>-SeedAPI: accounts seeding complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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-15443 Tests mirroring check (created files only)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (20)
seed/accounts/api_tokens.py (2)
40-48: Missing validation for required environment variable.
os.getenv("CLIENT_ACCOUNT_ID")returnsNoneif the variable isn't set, which will pass{"id": None}to the API. Consider adding validation or using a default/error handling.@inject def build_api_token_data( context: Context = DEFAULT_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") + if not module_id: + raise ValueError("accounts.module.id must be set in context") return {
27-32: Consider validating API response before accessing.id.After fetching from the API on line 28, the code accesses
api_token.idwithout verifying the response type. If the API returns an unexpected type, this could raise an AttributeError.if not isinstance(api_token, ApiToken): api_token = await mpt_ops.accounts.api_tokens.get(api_token_id) + if not isinstance(api_token, ApiToken): + return None context.set_resource("accounts.api_token", api_token) context["accounts.api_token.id"] = api_token.id - return api_token - return api_token + return api_tokenseed/accounts/buyer.py (2)
62-83: Consider forwarding injected dependencies explicitly.When
init_buyeris called with explicitcontextormpt_operationsparameters (bypassing DI defaults), those values won't propagate to the nestedget_buyer()andbuild_buyer_data()calls on lines 68 and 70. This could cause inconsistencies if callers expect explicit parameters to be honored throughout the call chain.If explicit parameter forwarding is desired, apply this diff:
async def init_buyer( context: Context = DEFAULT_CONTEXT, mpt_operations: AsyncMPTClient = DEFAULT_MPT_OPERATIONS, ) -> Buyer: """Get or create buyer.""" - buyer = await get_buyer() + buyer = await get_buyer(context=context, mpt_operations=mpt_operations) if buyer is None: - buyer_data = build_buyer_data() + buyer_data = build_buyer_data(context=context) logger.debug("Creating buyer ...") with pathlib.Path.open(icon, "rb") as icon_file: - created = await mpt_operations.accounts.buyers.create(buyer_data, file=icon_file) + created = await mpt_operations.accounts.buyers.create(buyer_data, file=icon_file)
81-81: Refactor exception message per linting guideline.The static analysis tool suggests avoiding long exception messages inline. Extract to a constant or use a predefined exception class.
Apply this diff:
- logger.warning("Buyer creation failed") - raise ValueError("Buyer creation failed") + error_msg = "Buyer creation failed" + logger.warning(error_msg) + raise ValueError(error_msg)seed/accounts/user_group.py (2)
52-67: Consider forwarding injected dependencies explicitly.The same DI parameter forwarding issue exists here as in
buyer.py. Wheninit_user_groupis called with explicit parameters, they won't propagate toget_user_group()(line 58) andbuild_user_group_data()(line 61).If explicit forwarding is desired:
async def init_user_group( context: Context = DEFAULT_CONTEXT, mpt_operations: AsyncMPTClient = DEFAULT_MPT_OPERATIONS, ) -> UserGroup: """Get or create user group.""" - user_group = await get_user_group() + user_group = await get_user_group(context=context, mpt_operations=mpt_operations) if user_group is None: logger.debug("Creating user group ...") - user_group_data = build_user_group_data() + user_group_data = build_user_group_data(context=context) user_group = await mpt_operations.accounts.user_groups.create(user_group_data)
70-76: Align return type with other seed functions.
seed_user_groupreturnsUserGroup, whileseed_buyerinbuyer.py(line 87) returnsNone. This inconsistency may confuse callers about the expected interface of seed functions.Consider standardizing return types. If orchestration doesn't need the return value, prefer
None:@inject -async def seed_user_group() -> UserGroup: +async def seed_user_group() -> None: """Seed user group.""" logger.debug("Seeding user group ...") - user_group = await init_user_group() + await init_user_group() logger.debug("Seeding user group completed.") - return user_grouptests/seed/accounts/test_accounts.py (1)
6-30: Remove unused noqa directive.The static analysis tool correctly identifies that the
WPS210noqa directive on line 6 is unused and can be removed.Apply this diff:
-async def test_seed_accounts() -> None: # noqa: WPS210 +async def test_seed_accounts() -> None: with (Otherwise, the test properly verifies that all six seed functions are invoked by the orchestrator.
seed/accounts/accounts.py (1)
13-13: Remove unusednoqadirective.The
WPS217code is from wemake-python-styleguide, which isn't recognized by Ruff. If you're not using wemake-python-styleguide in this project, remove the directive.-async def seed_accounts() -> None: # noqa: WPS217 +async def seed_accounts() -> None:tests/seed/accounts/test_module.py (4)
17-21: Chain the exception for proper traceback.Per Ruff B904, when re-raising a different exception type inside an
exceptclause, chain it withfromto preserve the traceback context.async def __anext__(self): try: return next(self._iterator) except StopIteration: - raise StopAsyncIteration + raise StopAsyncIteration from None
34-36: Remove unused function.The
async_iterfunction is not used anywhere in this test file. Additionally, despite its name, it's a synchronous generator (usesyield from), not an async iterator.-def async_iter(iter_items): - yield from iter_items -
72-87: Consider removing duplicate test.
test_refresh_module_get_newappears to be a duplicate oftest_refresh_module(lines 54-69). Both tests:
- Patch
get_moduleto returnNone- Mock
filter().iterate()to return aDummyAsyncIterator([module])- Assert
result == moduleIf these tests are intended to cover different scenarios, add distinguishing assertions or comments. Otherwise, consolidate into a single test.
90-99: Consider adding test coverage for additional paths.The test only covers the path where
get_modulereturnsNoneandrefresh_modulesucceeds. Per theseed_moduleimplementation, consider adding tests for:
- When
get_modulereturns an existing module (should skiprefresh_module).- When
refresh_modulereturnsNone(should raiseValueError).tests/seed/accounts/test_buyer.py (1)
89-97: Consider usingfake_file_factoryfor consistent file mocking.The
Path.openmock returnsAsyncMock(), but file operations expect a synchronous file-like object. Other tests in this PR (e.g.,test_product.py) use thefake_file_factoryfixture for this purpose.If
init_buyerreads the file contents, this mock may cause issues. Consider using the consistent pattern:+async def test_init_buyer_create_new( + context: Context, operations_client, buyers_service, buyer, fake_file_factory +) -> None: ... with ( patch("seed.accounts.buyer.get_buyer", return_value=None), - patch("pathlib.Path.open", return_value=AsyncMock()), + patch("pathlib.Path.open", new=lambda *args, **kwargs: fake_file_factory()), patch("seed.accounts.buyer.build_buyer_data", return_value=build_buyer_data(context)), ):tests/seed/accounts/test_api_token.py (1)
73-91: Duplicate test - consolidate or differentiate.
test_init_api_token_create_newis identical totest_init_api_token(lines 40-54). Both patchget_api_tokento returnNoneand assertcreateis called once.Consider:
- Removing the duplicate, or
- Converting one test to cover the "existing token found" path (where
get_api_tokenreturns the token andcreateshould not be called)tests/seed/catalog/test_product.py (1)
43-55: Good refactoring to usefake_file_factory.Using the shared
fake_file_factoryfixture for file mocking improves consistency across tests.Optional: To silence the Ruff warning about unused lambda arguments, you can use underscore convention:
- patch("pathlib.Path.open", new=lambda *args, **kwargs: fake_file_factory()), + patch("pathlib.Path.open", new=lambda *_args, **_kwargs: fake_file_factory()),tests/seed/accounts/test_user_group.py (1)
64-78: Clarify the contract forbuild_user_group_datavs. how this test sets up context
test_build_user_group_datasetscontext["accounts.account.id"], but the current implementation ofbuild_user_group_data(per snippet) pulls the account id fromos.getenv("CLIENT_ACCOUNT_ID"), not from the context. The test only passes because the env var happens to match the hard‑coded"ACC-1086-6867".To avoid confusion for future readers, consider either:
- Updating
build_user_group_datato derive the account id from the context (to match what the test is setting up), or- Explicitly documenting in the test (or via a fixture) that the account id comes from
CLIENT_ACCOUNT_IDand dropping the unusedcontext["accounts.account.id"]assignment.Right now the test implies the context value is used when it is not.
seed/accounts/module.py (2)
40-57:refresh_moduledocstring doesn’t quite match behaviorThe docstring says “always fetch”, but the function only falls back to the
filter(...).iterate()path whenget_modulereturnsNone; if a module is already in context, it just returns that cached instance without re‑fetching.Consider rewording the docstring to something like “Ensure a module is present in context, fetching by name if missing” to avoid confusion.
61-72: RuffTRY003on theValueErrormessageStatic analysis is flagging the long error message on Line 69 (
TRY003). To keep CI happy and make the message more concise, you could either shorten it or move the detail into a dedicated exception type.For a minimal change that should satisfy
TRY003, something like:- if refreshed is None: - raise ValueError("Could not seed module: no valid Module found.") + if refreshed is None: + raise ValueError("No valid module found to seed.")If you prefer, you can also introduce a custom exception class (e.g.,
ModuleSeedError) and raise that instead.tests/seed/accounts/test_licensee.py (1)
83-99: Ensureinit_licenseeforwardscontext/mpt_clientto its helpersThese tests correctly exercise the “create if missing” paths by patching
get_licenseeandbuild_licensee_data, but they don’t assert how those helpers are called. In similar seed modules (e.g., seller/user_group), it’s easy to accidentally write:licensee = await get_licensee() # uses defaults licensee_data = build_licensee_data() # uses defaultsinside
init_licensee, even thoughinit_licenseeitself acceptscontextandmpt_client. If that happens, callers passing a custom context/client intoinit_licenseewon’t have those dependencies honored.I suggest:
- Double‑checking that
init_licenseecalls:
await get_licensee(context=context, mpt_client=mpt_client)build_licensee_data(context=context)- Optionally, updating one of these tests to assert the helper call signatures, e.g. using
new_callable=AsyncMockforget_licenseeandassert_awaited_with(context=context, mpt_client=client_client).This will align the licensee seeding path with the rest of the DI‑aware seed modules and guard against subtle context mismatches.
Also applies to: 102-119
tests/seed/accounts/test_seller.py (1)
56-63: Consider asserting howget_seller/build_seller_dataare called to lock in DI behaviorThese tests correctly exercise the “create if missing” paths by patching
get_sellerandbuild_seller_dataand verifying thatsellers_service.createis called and the result is returned.Given
init_selleracceptscontextandmpt_operations, and we want it to forward those intoget_seller, you could make the tests more robust by also asserting the helper calls, for example:with patch("seed.accounts.seller.get_seller", new_callable=AsyncMock) as mock_get: mock_get.return_value = None result = await init_seller(context=context, mpt_operations=operations_client) mock_get.assert_awaited_with(context=context, mpt_operations=operations_client)This will catch any future regressions where
init_sellermight accidentally stop honoring the injected dependencies.Also applies to: 66-80
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
seed/accounts/logo.pngis excluded by!**/*.png
📒 Files selected for processing (23)
seed/accounts/accounts.py(1 hunks)seed/accounts/api_tokens.py(1 hunks)seed/accounts/buyer.py(1 hunks)seed/accounts/licensee.py(1 hunks)seed/accounts/module.py(1 hunks)seed/accounts/seller.py(1 hunks)seed/accounts/user_group.py(1 hunks)seed/catalog/product.py(1 hunks)seed/container.py(1 hunks)seed/seed_api.py(2 hunks)tests/e2e/accounts/buyers/conftest.py(1 hunks)tests/e2e/accounts/licensees/conftest.py(1 hunks)tests/e2e/accounts/sellers/conftest.py(1 hunks)tests/seed/accounts/test_accounts.py(1 hunks)tests/seed/accounts/test_api_token.py(1 hunks)tests/seed/accounts/test_buyer.py(1 hunks)tests/seed/accounts/test_licensee.py(1 hunks)tests/seed/accounts/test_module.py(1 hunks)tests/seed/accounts/test_seller.py(1 hunks)tests/seed/accounts/test_user_group.py(1 hunks)tests/seed/catalog/test_product.py(1 hunks)tests/seed/conftest.py(3 hunks)tests/seed/test_seed_api.py(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-27T00:05:36.332Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/conftest.py:0-0
Timestamp: 2025-11-27T00:05:36.332Z
Learning: When reviewing PRs that add fixtures to a parent conftest that may duplicate fixtures in child conftest files, do not suggest removing the duplicates from child conftest files that are outside the scope of the PR's changes.
Applied to files:
tests/seed/conftest.py
📚 Learning: 2025-11-27T00:05:54.691Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/accounts/accounts_users/test_async_accounts_users.py:0-0
Timestamp: 2025-11-27T00:05:54.691Z
Learning: In pytest-asyncio, async fixtures are automatically resolved before being passed to test functions (both sync and async). Test functions receive the yielded value from async fixtures, not coroutines, so fixture parameters should never be awaited. A sync test function can use async fixtures without any special handling.
Applied to files:
tests/seed/conftest.pytests/seed/catalog/test_product.py
🧬 Code graph analysis (15)
seed/accounts/seller.py (2)
mpt_api_client/resources/accounts/sellers.py (1)
Seller(13-14)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)
seed/accounts/licensee.py (4)
mpt_api_client/resources/accounts/licensees.py (1)
Licensee(21-22)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)tests/seed/accounts/test_licensee.py (1)
licensee(12-30)mpt_api_client/models/model.py (1)
id(119-121)
tests/seed/accounts/test_api_token.py (2)
seed/accounts/api_tokens.py (4)
build_api_token_data(36-48)get_api_token(15-32)init_api_token(52-67)seed_api_token(71-75)seed/context.py (1)
get_resource(19-29)
seed/accounts/accounts.py (6)
seed/accounts/api_tokens.py (1)
seed_api_token(71-75)seed/accounts/buyer.py (1)
seed_buyer(87-91)seed/accounts/licensee.py (1)
seed_licensee(93-97)seed/accounts/module.py (1)
seed_module(61-72)seed/accounts/seller.py (1)
seed_seller(73-77)seed/accounts/user_group.py (1)
seed_user_group(71-76)
tests/seed/accounts/test_module.py (2)
seed/accounts/module.py (3)
get_module(15-32)refresh_module(36-57)seed_module(61-72)seed/context.py (2)
Context(12-33)get_resource(19-29)
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/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)tests/e2e/conftest.py (1)
logger(63-64)tests/e2e/accounts/conftest.py (1)
user_group_id(37-38)tests/seed/accounts/test_user_group.py (1)
user_group(16-17)
tests/seed/conftest.py (1)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)
seed/catalog/product.py (6)
tests/seed/catalog/test_product.py (1)
product(17-18)tests/e2e/conftest.py (1)
mpt_vendor(19-20)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)mpt_api_client/resources/catalog/products.py (1)
Product(49-50)seed/context.py (1)
set_resource(31-33)mpt_api_client/models/model.py (1)
id(119-121)
tests/seed/accounts/test_user_group.py (3)
seed/accounts/user_group.py (4)
build_user_group_data(36-49)get_user_group(15-32)init_user_group(53-67)seed_user_group(71-76)seed/context.py (2)
Context(12-33)get_resource(19-29)mpt_api_client/models/model.py (1)
id(119-121)
seed/accounts/module.py (5)
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/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)mpt_api_client/models/model.py (1)
id(119-121)
tests/seed/accounts/test_seller.py (2)
seed/accounts/seller.py (4)
build_seller_data(36-51)get_seller(15-32)init_seller(55-69)seed_seller(73-77)seed/context.py (2)
Context(12-33)get_resource(19-29)
seed/accounts/buyer.py (3)
mpt_api_client/resources/accounts/buyers.py (1)
Buyer(26-27)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)mpt_api_client/models/model.py (1)
id(119-121)
seed/seed_api.py (4)
seed/accounts/accounts.py (1)
seed_accounts(13-23)seed/context.py (1)
load_context(36-52)tests/seed/conftest.py (1)
context(15-16)seed/catalog/catalog.py (1)
seed_catalog(32-39)
tests/seed/catalog/test_product.py (6)
tests/seed/conftest.py (3)
context(15-16)vendor_client(20-21)fake_file_factory(35-36)seed/context.py (1)
Context(12-33)tests/seed/catalog/test_item_group.py (1)
vendor_client(29-30)tests/seed/catalog/test_product_parameters.py (1)
vendor_client(30-31)seed/catalog/product.py (1)
init_product(45-65)tests/e2e/conftest.py (1)
mpt_vendor(19-20)
tests/seed/accounts/test_accounts.py (1)
seed/accounts/accounts.py (1)
seed_accounts(13-23)
🪛 Ruff (0.14.7)
seed/accounts/licensee.py
87-87: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/accounts.py
13-13: Unused noqa directive (unknown: WPS217)
Remove unused noqa directive
(RUF100)
tests/seed/accounts/test_module.py
21-21: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
seed/catalog/product.py
63-63: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/module.py
69-69: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/buyer.py
81-81: Avoid specifying long messages outside the exception class
(TRY003)
tests/seed/catalog/test_product.py
51-51: Unused lambda argument: args
(ARG005)
51-51: Unused lambda argument: kwargs
(ARG005)
tests/seed/accounts/test_accounts.py
6-6: 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 (25)
tests/e2e/accounts/sellers/conftest.py (1)
20-20: LGTM!Test data alignment to use "Los Angeles" consistently across fixtures is appropriate.
seed/catalog/product.py (1)
51-65: Clear control flow with proper resource management.The restructured logic cleanly separates the creation path from the retrieval path. The file context manager properly handles the icon file.
Minor style consideration: The static analysis tool flags the inline exception message on line 63 (TRY003). If you have a project convention for custom exception classes, you could define one, but for seed scripts this is acceptable.
seed/accounts/api_tokens.py (1)
51-67: LGTM!The get-or-create pattern is consistent with other seed modules. Logging at appropriate levels.
seed/accounts/licensee.py (3)
69-89: Solid get-or-create implementation with proper file handling.The context manager for file I/O and isinstance validation after creation are good practices. The pattern is consistent with
init_productin the catalog module.The static analysis hint (TRY003) on line 87 is a minor style consideration that can be addressed optionally.
17-35: Same pattern asget_api_token- consistent structure.Note: The same optional validation suggestion from
api_tokens.pyapplies here for the API response on line 31.
43-46: Add validation forCLIENT_ACCOUNT_IDenvironment variable.Line 43:
os.getenv("CLIENT_ACCOUNT_ID")may returnNoneif the environment variable is not set. ThisNonevalue is then passed to the API payload at line 60 ("account": {"id": account_id}). Consider validating that the environment variable is set before use, or provide a default/fallback value.tests/e2e/accounts/licensees/conftest.py (1)
35-35: LGTM!Test fixture data aligned with seed module (
seed/accounts/licensee.py) for consistency.seed/accounts/buyer.py (2)
17-35: LGTM!The retrieval logic correctly handles context caching and API fallback with appropriate type checking.
86-91: LGTM!The seed orchestration function is clear and appropriately delegates to
init_buyer.seed/accounts/user_group.py (1)
14-32: LGTM!The retrieval logic correctly handles context caching and API fallback.
tests/seed/test_seed_api.py (1)
23-39: LGTM!The test correctly patches and asserts the new
seed_accountscall, maintaining consistency with the existing catalog test pattern.tests/e2e/accounts/buyers/conftest.py (1)
32-32: LGTM!Test data alignment with the seeded buyer data.
seed/container.py (1)
60-67: LGTM!The DI wiring correctly includes all new accounts seed modules, enabling proper dependency injection across the accounts seeding workflow.
seed/seed_api.py (1)
17-33: LGTM!The concurrent execution of catalog and accounts seeding with proper error handling and guaranteed context persistence is well-implemented.
tests/seed/conftest.py (1)
10-11: LGTM!The test utilities provide clean abstractions for file I/O mocking and client mocking, supporting the new accounts seed test suite.
Also applies to: 29-36
seed/accounts/accounts.py (1)
15-23: LGTM!The sequential orchestration of seed functions follows a logical dependency order. The implementation correctly coordinates the seeding of all account-related entities.
Minor nit: The completion log says "Seeded accounts completed" while the start log says "Seeding accounts ...". Consider using consistent phrasing like "Seeding accounts completed." for symmetry.
tests/seed/accounts/test_buyer.py (2)
36-60: Verify the empty seller ID is intentional.The expected data includes
"sellers": [{"id": ""}]with an empty string for the seller ID. Ifbuild_buyer_dataretrieves the seller ID from context, this test doesn't set that context value, which may be why it's empty.Consider either:
- Setting the seller ID in the context before calling
build_buyer_data- Adding a comment explaining why an empty seller ID is expected in this test scenario
20-28: LGTM!The test correctly verifies that
get_buyerfetches the buyer when an ID exists in context and stores it as a resource.tests/seed/accounts/test_api_token.py (1)
24-32: LGTM!The test correctly verifies that
get_api_tokenfetches the token when an ID exists in context and stores it as a resource.seed/accounts/module.py (1)
14-32: Module lookup and context caching look correct
get_modulecleanly implements the “context first, then API” pattern and correctly caches back into theContexton a cache miss. Type check againstModuleplusValueErrorhandling fromContext.get_resourceare appropriate.No changes needed here.
seed/accounts/seller.py (2)
14-32:get_sellercaching logic is consistent and soundThe “id from context → resource from context → API fallback → cache back” pattern mirrors the other seed modules and uses
Context.get_resource/set_resourcecorrectly.No changes needed here.
35-51:build_seller_datais straightforward and suitable for seedingGenerating a unique
externalIdviauuid.uuid4()when not provided, along with fixed address/currency data, is appropriate for deterministic seed data. Keepingexternal_idinjectable is useful for tests and replayability.Looks good as‑is.
tests/seed/accounts/test_seller.py (3)
20-29:get_sellerhappy‑path test is solidThis test correctly verifies:
- The id is picked up from the context.
- The async service’s
getis used.- The retrieved
Selleris cached back into theContextviaget_resource.No changes needed here.
36-53: Seller data expectations are clear and match the builder
test_build_seller_datadefines the full expected payload and compares it directly tobuild_seller_data(external_id=external_id), which is concise and robust.All good here.
83-88:seed_sellerorchestration test is appropriatePatching
init_sellerwith anAsyncMockand asserting it is awaited once keeps theseed_sellertest focused on orchestration behavior and logging side‑effects.Looks good.
robcsegal
force-pushed
the
MPT-15443-seed-data-required-for-accounts-e-2-e-tests
branch
from
85ab0a4 to
5542929
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
seed/accounts/seller.py (1)
60-60: Past issue addressed.The
init_sellerfunction now correctly passescontextandmpt_operationstoget_seller, ensuring the caller's explicit choices are honored.seed/accounts/user_group.py (1)
40-42: Past issue addressed.The
CLIENT_ACCOUNT_IDenvironment variable is now properly validated, raising a clearValueErrorif missing.seed/accounts/buyer.py (1)
41-43: Past issue addressed.The
CLIENT_ACCOUNT_IDenvironment variable is now properly validated with a clear error message.
🧹 Nitpick comments (11)
tests/seed/conftest.py (1)
29-31: Type hint inconsistency.The return type annotation says
AsyncMockbut the fixture returnsMagicMock(spec=AsyncMPTClient). This follows the same pattern as the existingvendor_clientandoperations_clientfixtures, but the annotation is misleading.Consider fixing the type hint for accuracy:
@pytest.fixture -def client_client() -> AsyncMock: +def client_client() -> MagicMock: return MagicMock(spec=AsyncMPTClient)seed/seed_api.py (1)
28-31: Consider re-raising exceptions after logging.The current implementation catches all exceptions, logs them, and silently continues. This may mask failures during seeding, making it difficult to detect issues in CI/CD pipelines or automated runs.
If the seeding process should fail visibly when errors occur:
try: await asyncio.gather(*tasks) except Exception: logger.exception("Exception occurred during seeding.") + raise finally: save_context(context, context_file)Alternatively, if partial success is acceptable, consider returning a status or error summary.
tests/seed/accounts/test_accounts.py (1)
6-6: Remove unusednoqadirective.The static analysis indicates that
WPS210is not recognized, making this directive ineffective. Either the WPS plugin is not enabled in the linting configuration, or the directive is unnecessary.-async def test_seed_accounts() -> None: # noqa: WPS210 +async def test_seed_accounts() -> None:seed/accounts/accounts.py (1)
13-13: Remove unusednoqadirective.The static analysis indicates that
WPS217is not recognized, making this directive ineffective.-async def seed_accounts() -> None: # noqa: WPS217 +async def seed_accounts() -> None:tests/seed/accounts/test_module.py (2)
17-21: Chain exception properly in async iterator.Per Python best practice (PEP 3134), exceptions raised in
exceptblocks should be chained to preserve the traceback.async def __anext__(self): try: return next(self._iterator) except StopIteration: - raise StopAsyncIteration + raise StopAsyncIteration from None
34-36: Remove unused helper function.The
async_iterfunction is defined but never used in this test module.-def async_iter(iter_items): - yield from iter_items - -tests/seed/accounts/test_buyer.py (1)
36-60: Consider setting seller ID in context for complete payload validation.The
build_buyer_datatest expectssellers: [{"id": ""}]becauseaccounts.seller.idis not set in the context. While this validates the structure, consider adding a test case with the seller ID set to verify the complete payload.tests/seed/catalog/test_product.py (1)
51-51: Consider using underscore prefix for unused lambda parameters.The lambda ignores its parameters, which Ruff flags. While functional, using underscores signals intent.
- patch("pathlib.Path.open", new=lambda *args, **kwargs: fake_file_factory()), + patch("pathlib.Path.open", new=lambda *_args, **_kwargs: fake_file_factory()),tests/seed/accounts/test_user_group.py (1)
43-61: Consider testing the "existing user group" path.Both
test_init_user_groupandtest_init_user_group_create_newpatchget_user_groupto returnNone, testing the creation path. Consider modifying one test to return an existinguser_groupto verify the "already exists" branch wherecreate()should NOT be called.async def test_init_user_group( context: Context, operations_client, user_groups_service, user_group ) -> None: user_groups_service.create.return_value = user_group operations_client.accounts.user_groups = user_groups_service context["accounts.account.id"] = "ACC-1086-6867" context["accounts.module.id"] = "MOD-456" with ( - patch("seed.accounts.user_group.get_user_group", return_value=None), - patch( - "seed.accounts.user_group.build_user_group_data", - return_value=build_user_group_data(context), - ), + patch("seed.accounts.user_group.get_user_group", return_value=user_group), ): result = await init_user_group(context=context, mpt_operations=operations_client) assert result == user_group - user_groups_service.create.assert_called_once() + user_groups_service.create.assert_not_called()seed/accounts/licensee.py (1)
92-97: Minor: Inconsistent log level for completion message.
seed_licenseeuseslogger.infofor completion whileseed_accounts(inaccounts.pyline 23) useslogger.debug. Consider aligning for consistency.async def seed_licensee() -> None: """Seed licensee.""" logger.debug("Seeding licensee ...") await init_licensee() - logger.info("Seeding licensee completed.") + logger.debug("Seeding licensee completed.")seed/accounts/module.py (1)
35-57: Docstring overstates behavior and list materialization is unnecessary
- The docstring says “always fetch”, but
refresh_modulereturns the existing module fromget_modulewithout re-fetching when it’s already present. Either update the docstring to match the current “fetch-on-miss” behavior or change the implementation to always refresh from the API.- When falling back to
filter, you only ever use the first module but still build a full list. You could iterate until the first validModuleand return immediately to avoid extra work and allocations (use anasync forloop andbreak/ early return).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
seed/accounts/logo.pngis excluded by!**/*.png
📒 Files selected for processing (23)
seed/accounts/accounts.py(1 hunks)seed/accounts/api_tokens.py(1 hunks)seed/accounts/buyer.py(1 hunks)seed/accounts/licensee.py(1 hunks)seed/accounts/module.py(1 hunks)seed/accounts/seller.py(1 hunks)seed/accounts/user_group.py(1 hunks)seed/catalog/product.py(1 hunks)seed/container.py(1 hunks)seed/seed_api.py(2 hunks)tests/e2e/accounts/buyers/conftest.py(1 hunks)tests/e2e/accounts/licensees/conftest.py(1 hunks)tests/e2e/accounts/sellers/conftest.py(1 hunks)tests/seed/accounts/test_accounts.py(1 hunks)tests/seed/accounts/test_api_token.py(1 hunks)tests/seed/accounts/test_buyer.py(1 hunks)tests/seed/accounts/test_licensee.py(1 hunks)tests/seed/accounts/test_module.py(1 hunks)tests/seed/accounts/test_seller.py(1 hunks)tests/seed/accounts/test_user_group.py(1 hunks)tests/seed/catalog/test_product.py(1 hunks)tests/seed/conftest.py(3 hunks)tests/seed/test_seed_api.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- seed/container.py
- tests/seed/accounts/test_api_token.py
- tests/seed/test_seed_api.py
- tests/seed/accounts/test_licensee.py
- tests/seed/accounts/test_seller.py
- tests/e2e/accounts/licensees/conftest.py
- tests/e2e/accounts/buyers/conftest.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-27T00:05:54.701Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/accounts/accounts_users/test_async_accounts_users.py:0-0
Timestamp: 2025-11-27T00:05:54.701Z
Learning: In pytest-asyncio, async fixtures are automatically resolved before being passed to test functions (both sync and async). Test functions receive the yielded value from async fixtures, not coroutines, so fixture parameters should never be awaited. A sync test function can use async fixtures without any special handling.
Applied to files:
tests/seed/conftest.pytests/seed/catalog/test_product.py
🧬 Code graph analysis (12)
seed/seed_api.py (3)
seed/accounts/accounts.py (1)
seed_accounts(13-23)seed/context.py (1)
load_context(36-52)seed/catalog/catalog.py (1)
seed_catalog(32-39)
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/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)mpt_api_client/models/model.py (1)
id(119-121)
seed/accounts/api_tokens.py (4)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/api_tokens.py (1)
ApiToken(12-13)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)tests/seed/accounts/test_api_token.py (1)
api_token(15-16)
tests/seed/accounts/test_user_group.py (3)
seed/accounts/user_group.py (4)
build_user_group_data(36-51)get_user_group(15-32)init_user_group(55-69)seed_user_group(73-78)seed/context.py (2)
Context(12-33)get_resource(19-29)mpt_api_client/models/model.py (1)
id(119-121)
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/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)mpt_api_client/models/model.py (1)
id(119-121)
seed/accounts/module.py (4)
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/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)
seed/accounts/licensee.py (4)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/licensees.py (1)
Licensee(21-22)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)tests/seed/accounts/test_licensee.py (1)
licensee(12-30)
tests/seed/accounts/test_buyer.py (3)
seed/accounts/buyer.py (4)
build_buyer_data(39-61)get_buyer(18-35)init_buyer(65-85)seed_buyer(89-93)seed/context.py (2)
Context(12-33)get_resource(19-29)mpt_api_client/models/model.py (1)
id(119-121)
seed/accounts/buyer.py (3)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/buyers.py (1)
Buyer(26-27)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)
seed/accounts/accounts.py (6)
seed/accounts/api_tokens.py (1)
seed_api_token(71-75)seed/accounts/buyer.py (1)
seed_buyer(89-93)seed/accounts/licensee.py (1)
seed_licensee(93-97)seed/accounts/module.py (1)
seed_module(61-72)seed/accounts/seller.py (1)
seed_seller(73-77)seed/accounts/user_group.py (1)
seed_user_group(73-78)
tests/seed/accounts/test_accounts.py (1)
seed/accounts/accounts.py (1)
seed_accounts(13-23)
tests/seed/catalog/test_product.py (3)
tests/seed/conftest.py (2)
context(15-16)fake_file_factory(35-36)seed/context.py (1)
Context(12-33)seed/catalog/product.py (1)
init_product(45-65)
🪛 Ruff (0.14.7)
seed/accounts/user_group.py
42-42: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/module.py
69-69: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/licensee.py
87-87: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/buyer.py
43-43: Avoid specifying long messages outside the exception class
(TRY003)
83-83: Avoid specifying long messages outside the exception class
(TRY003)
seed/catalog/product.py
63-63: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/accounts.py
13-13: Unused noqa directive (unknown: WPS217)
Remove unused noqa directive
(RUF100)
tests/seed/accounts/test_accounts.py
6-6: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
tests/seed/accounts/test_module.py
21-21: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
tests/seed/catalog/test_product.py
51-51: Unused lambda argument: args
(ARG005)
51-51: Unused lambda argument: kwargs
(ARG005)
⏰ 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)
tests/e2e/accounts/sellers/conftest.py (1)
20-20: LGTM!Test data update aligns with the broader PR changes standardizing city values to "Los Angeles" across e2e seed fixtures.
seed/catalog/product.py (1)
51-65: LGTM!The inverted control flow is cleaner - creating only when
product is Noneand returning the existing product at the end. Error handling for creation failure is appropriately handled with clear logging.seed/accounts/buyer.py (1)
76-81: Good: Proper type validation on API response.Unlike
seller.pyanduser_group.py, this correctly validatesisinstance(created, Buyer)before using the result and handles errors appropriately. Consider applying this pattern to the other seed modules for consistency.tests/seed/conftest.py (1)
34-36: LGTM!The
fake_file_factoryfixture provides a clean way to mock file I/O in tests that need to patchPath.open.seed/seed_api.py (1)
24-26: LGTM!Running
seed_catalogandseed_accountsconcurrently is appropriate since they operate on independent domains. The task list with typed annotation and proper error handling with context persistence infinallyensures robustness.tests/seed/accounts/test_accounts.py (1)
7-30: LGTM!The test correctly verifies that
seed_accounts()orchestrates all six seed functions. The approach of collecting mocks and iterating for assertions is clean and maintainable.seed/accounts/accounts.py (1)
1-23: LGTM!The sequential execution order is appropriate given the dependencies between entities (e.g.,
seed_licenseedepends onseed_user_groupresources being available in context). The orchestration is clean and well-logged.tests/seed/accounts/test_module.py (1)
38-99: LGTM!The tests comprehensively cover the module seeding functionality including retrieval, refresh, and seed orchestration paths.
tests/seed/accounts/test_buyer.py (1)
63-105: LGTM!The tests properly verify both the "existing buyer" and "create new buyer" paths, including correct mock assertions for service calls.
tests/seed/catalog/test_product.py (1)
43-56: LGTM!The test correctly uses the
fake_file_factoryfixture, properly invokesinit_productwith named arguments, and maintains the strictassert_called_once()assertion.tests/seed/accounts/test_user_group.py (2)
64-78: Test relies onCLIENT_ACCOUNT_IDenvironment variable, not context.The test sets
context["accounts.account.id"]butbuild_user_group_datareads fromos.getenv("CLIENT_ACCOUNT_ID")per the implementation snippet. This test may fail if the environment variable isn't set, or the assertion at line 69 may pass coincidentally if the env var happens to match.Consider explicitly patching the environment variable to ensure test isolation:
+from unittest.mock import patch + def test_build_user_group_data(context: Context) -> None: - context["accounts.account.id"] = "ACC-1086-6867" context["accounts.module.id"] = "MOD-456" - expected_data = { - ... - "account": {"id": "ACC-1086-6867"}, - ... - } - - result = build_user_group_data(context) + with patch.dict("os.environ", {"CLIENT_ACCOUNT_ID": "ACC-1086-6867"}): + expected_data = { + "name": "E2E Seeded User Group", + "account": {"id": "ACC-1086-6867"}, + ... + } + result = build_user_group_data(context) + assert result == expected_data
98-106: LGTM!The
test_seed_user_grouptest correctly verifies thatinit_user_groupis awaited during seeding.seed/accounts/licensee.py (2)
38-66: LGTM!The
build_licensee_datafunction correctly retrieves dependencies from context and constructs a well-structured payload. The implicit requirement foraccounts.user_groupto exist is appropriate given the seeding order inseed_accounts.
17-35: LGTM!The
get_licenseefunction follows the established pattern for context-aware resource retrieval with API fallback.seed/accounts/module.py (1)
14-32: Module retrieval and caching logic looks goodThe
get_moduleflow (context-first, then API, with type-checking and caching back intoContext) is coherent and matches theContext.get_resource/set_resourcecontract. No changes needed here from a correctness perspective.seed/accounts/api_tokens.py (2)
14-32: API token retrieval/caching logic is consistent
get_api_tokencorrectly mirrors the module pattern: it looks up the id in context, tries the cached resource, then falls back to the API and updates context. This is clear and consistent.
70-75: Seeding wrapper is straightforward
seed_api_tokenis a thin async wrapper aroundinit_api_tokenwith debug logging, which is appropriate for the seeding entry point. No changes needed.
robcsegal
force-pushed
the
MPT-15443-seed-data-required-for-accounts-e-2-e-tests
branch
from
5542929 to
fcf3f67
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
seed/accounts/module.py (1)
60-72: TRY003 lint warning already addressed in past review.The Ruff TRY003 warning on line 69 was already flagged in a previous review. The suggestion to add
# noqa: TRY003or extract the message to a constant remains valid.seed/accounts/seller.py (1)
55-74: Return type mismatch: function can returnNonebut signature declares-> Seller.Line 72 returns
Noneon creation failure, but the signature at line 59 declares-> Seller. This causes mypy to flag unreachable code (hence the disable directive at line 1). For consistency withinit_licenseewhich raisesValueErroron failure, apply this fix:async def init_seller( context: Context = DEFAULT_CONTEXT, mpt_operations: AsyncMPTClient = DEFAULT_MPT_OPERATIONS, ) -> Seller: """Get or create seller.""" seller = await get_seller(context=context, mpt_operations=mpt_operations) if seller is None: logger.debug("Creating seller ...") seller_data = build_seller_data() created = await mpt_operations.accounts.sellers.create(seller_data) if isinstance(created, Seller): context.set_resource("accounts.seller", created) context["accounts.seller.id"] = created.id logger.info("Seller created: %s", created.id) return created logger.warning("Seller creation failed") - return None + raise ValueError("Seller creation failed") logger.info("Seller already exists: %s", seller.id) return sellerAfter this fix, remove the
# mypy: disable-error-code=unreachabledirective at line 1.seed/accounts/user_group.py (1)
55-74: Fix type inconsistency: function returns None but signature declares UserGroup.The function signature on line 59 declares a return type of
UserGroup, but line 72 returnsNonewhen creation fails. This violates the type contract and will cause type checking errors or runtime issues for callers expecting aUserGroupinstance.Apply this diff to raise an exception instead of returning None:
if isinstance(created, UserGroup): context.set_resource("accounts.user_group", created) context["accounts.user_group.id"] = created.id logger.info("User group created: %s", created.id) return created logger.warning("User group creation failed") - return None + raise ValueError("User group creation failed") logger.info("User group already exists: %s", user_group.id) return user_group
🧹 Nitpick comments (6)
seed/accounts/module.py (1)
35-57: Docstring is inconsistent with implementation.The docstring states "Refresh module in context (always fetch)" but the implementation returns early at line 57 if
get_module()succeeds, skipping the API fetch. Consider updating the docstring to accurately reflect the "get or fetch" behavior:- """Refresh module in context (always fetch).""" + """Get module from context or fetch from API by name filter."""tests/seed/accounts/test_seller.py (1)
56-80: Consider adding a test for the "seller exists" path.Both
test_init_sellerandtest_init_seller_create_newtest the creation path (whenget_sellerreturnsNone). The "seller already exists" path whereget_sellerreturns an existing seller is not covered. Consider adding:async def test_init_seller_exists( context: Context, operations_client, sellers_service, seller ) -> None: with patch("seed.accounts.seller.get_seller", return_value=seller): result = await init_seller(context=context, mpt_operations=operations_client) assert result == seller sellers_service.create.assert_not_called()tests/seed/catalog/test_product.py (1)
51-51: Simplify the lambda to remove unused arguments.The lambda accepts
*argsand**kwargsbut doesn't use them. You can simplify by directly usingfake_file_factorywithout the wrapper lambda.Apply this diff:
- patch("pathlib.Path.open", new=lambda *args, **kwargs: fake_file_factory()), + patch("pathlib.Path.open", new=fake_file_factory),seed/accounts/accounts.py (1)
13-13: Remove unusednoqadirective.The
# noqa: WPS217directive is flagged as unused by static analysis. If the WPS217 rule isn't active in your linting configuration, remove the directive to keep the code clean.Apply this diff:
-async def seed_accounts() -> None: # noqa: WPS217 +async def seed_accounts() -> None:tests/seed/accounts/test_module.py (1)
17-21: Add exception chaining for clarity.When converting
StopIterationtoStopAsyncIteration, useraise ... from errto preserve the exception chain and distinguish it from errors in exception handling.Apply this diff:
async def __anext__(self): try: return next(self._iterator) - except StopIteration: - raise StopAsyncIteration + except StopIteration as err: + raise StopAsyncIteration from errtests/seed/accounts/test_accounts.py (1)
6-6: Remove unusednoqadirective.The
# noqa: WPS210directive is flagged as unused by static analysis. If the WPS210 rule isn't active in your linting configuration, remove the directive to keep the code clean.Apply this diff:
-async def test_seed_accounts() -> None: # noqa: WPS210 +async def test_seed_accounts() -> None:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
seed/accounts/logo.pngis excluded by!**/*.png
📒 Files selected for processing (24)
pyproject.toml(2 hunks)seed/accounts/accounts.py(1 hunks)seed/accounts/api_tokens.py(1 hunks)seed/accounts/buyer.py(1 hunks)seed/accounts/licensee.py(1 hunks)seed/accounts/module.py(1 hunks)seed/accounts/seller.py(1 hunks)seed/accounts/user_group.py(1 hunks)seed/catalog/product.py(1 hunks)seed/container.py(1 hunks)seed/seed_api.py(2 hunks)tests/e2e/accounts/buyers/conftest.py(1 hunks)tests/e2e/accounts/licensees/conftest.py(1 hunks)tests/e2e/accounts/sellers/conftest.py(1 hunks)tests/seed/accounts/test_accounts.py(1 hunks)tests/seed/accounts/test_api_token.py(1 hunks)tests/seed/accounts/test_buyer.py(1 hunks)tests/seed/accounts/test_licensee.py(1 hunks)tests/seed/accounts/test_module.py(1 hunks)tests/seed/accounts/test_seller.py(1 hunks)tests/seed/accounts/test_user_group.py(1 hunks)tests/seed/catalog/test_product.py(1 hunks)tests/seed/conftest.py(3 hunks)tests/seed/test_seed_api.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- seed/accounts/api_tokens.py
- tests/seed/conftest.py
- tests/seed/accounts/test_user_group.py
- tests/seed/accounts/test_licensee.py
- seed/container.py
- tests/e2e/accounts/licensees/conftest.py
- tests/seed/accounts/test_api_token.py
- tests/e2e/accounts/buyers/conftest.py
🧰 Additional context used
🧠 Learnings (2)
📚 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:
pyproject.toml
📚 Learning: 2025-11-27T00:05:54.701Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/accounts/accounts_users/test_async_accounts_users.py:0-0
Timestamp: 2025-11-27T00:05:54.701Z
Learning: In pytest-asyncio, async fixtures are automatically resolved before being passed to test functions (both sync and async). Test functions receive the yielded value from async fixtures, not coroutines, so fixture parameters should never be awaited. A sync test function can use async fixtures without any special handling.
Applied to files:
tests/seed/catalog/test_product.py
🧬 Code graph analysis (9)
seed/seed_api.py (4)
seed/accounts/accounts.py (1)
seed_accounts(13-23)seed/context.py (1)
load_context(36-52)tests/seed/conftest.py (1)
context(15-16)seed/catalog/catalog.py (1)
seed_catalog(32-39)
seed/catalog/product.py (5)
tests/seed/catalog/test_product.py (1)
product(17-18)mpt_api_client/resources/catalog/products.py (1)
Product(49-50)tests/seed/conftest.py (1)
context(15-16)seed/context.py (1)
set_resource(31-33)mpt_api_client/models/model.py (1)
id(119-121)
tests/seed/accounts/test_seller.py (4)
mpt_api_client/resources/accounts/sellers.py (2)
AsyncSellersService(44-60)Seller(13-14)seed/accounts/seller.py (4)
build_seller_data(37-52)get_seller(16-33)init_seller(56-74)seed_seller(78-82)tests/seed/conftest.py (2)
context(15-16)operations_client(25-26)seed/context.py (2)
Context(12-33)get_resource(19-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/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)mpt_api_client/models/model.py (1)
id(119-121)
seed/accounts/buyer.py (3)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/buyers.py (1)
Buyer(26-27)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)
seed/accounts/accounts.py (6)
seed/accounts/api_tokens.py (1)
seed_api_token(71-75)seed/accounts/buyer.py (1)
seed_buyer(89-93)seed/accounts/licensee.py (1)
seed_licensee(93-97)seed/accounts/module.py (1)
seed_module(61-72)seed/accounts/seller.py (1)
seed_seller(78-82)seed/accounts/user_group.py (1)
seed_user_group(78-83)
tests/seed/accounts/test_accounts.py (1)
seed/accounts/accounts.py (1)
seed_accounts(13-23)
tests/seed/accounts/test_module.py (2)
seed/accounts/module.py (3)
get_module(15-32)refresh_module(36-57)seed_module(61-72)seed/context.py (2)
Context(12-33)get_resource(19-29)
tests/seed/catalog/test_product.py (5)
tests/seed/conftest.py (3)
context(15-16)vendor_client(20-21)fake_file_factory(35-36)seed/context.py (1)
Context(12-33)tests/seed/catalog/test_item_group.py (1)
vendor_client(29-30)tests/seed/catalog/test_product_parameters.py (1)
vendor_client(30-31)seed/catalog/product.py (1)
init_product(45-65)
🪛 Ruff (0.14.7)
seed/accounts/module.py
69-69: Avoid specifying long messages outside the exception class
(TRY003)
seed/catalog/product.py
63-63: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/buyer.py
43-43: Avoid specifying long messages outside the exception class
(TRY003)
83-83: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/licensee.py
87-87: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/accounts.py
13-13: Unused noqa directive (unknown: WPS217)
Remove unused noqa directive
(RUF100)
seed/accounts/user_group.py
43-43: Avoid specifying long messages outside the exception class
(TRY003)
tests/seed/accounts/test_accounts.py
6-6: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
tests/seed/accounts/test_module.py
21-21: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
tests/seed/catalog/test_product.py
51-51: Unused lambda argument: args
(ARG005)
51-51: Unused lambda argument: kwargs
(ARG005)
⏰ 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 (29)
tests/e2e/accounts/sellers/conftest.py (1)
19-23: City value change in seller fixture looks goodSwitching the seeded seller address city to
"Los Angeles"is a harmless test-data tweak and keeps the fixture structure intact. Just ensure there are no tests or downstream seed assertions still expecting the previous city value.pyproject.toml (1)
116-134: LGTM!The lint suppressions for WPS453 in
seed/accounts/*.pyandmpt_api_client/resources/accounts/*.pyare appropriate for the new seeding modules being introduced. Based on learnings, this repository uses wemake-python-styleguide, so these per-file-ignores are consistent with existing patterns.seed/accounts/module.py (1)
14-32: LGTM!The
get_modulefunction correctly implements the context-first lookup pattern with API fallback and caching. The type checking withisinstancebefore returning is a good defensive practice.seed/accounts/licensee.py (4)
17-35: LGTM!The
get_licenseefunction follows the established context-first lookup pattern with proper API fallback and caching.
38-66: LGTM with implicit dependency noted.The function correctly builds the licensee payload. Note that line 46 assumes
accounts.user_groupis already in context, which implies the user group must be seeded before the licensee. Ensure the orchestration inseed_accountsmaintains this ordering.
69-89: Good implementation with past feedback addressed.The function now correctly forwards
contextandmpt_clientto helper functions (lines 75, 77), addressing the concern from the previous review. The type validation on the API response at line 81 is also consistent with best practices seen in other seed modules.
92-97: LGTM!Simple orchestration wrapper consistent with the pattern used in other seed modules.
seed/accounts/seller.py (3)
15-33: LGTM!The
get_sellerfunction correctly implements the context-first lookup pattern with API fallback and caching, consistent with other seeding modules.
36-52: LGTM!The UUID-based
externalIdgeneration ensures uniqueness as required.
77-82: LGTM!Consistent orchestration pattern.
tests/seed/accounts/test_seller.py (4)
10-17: LGTM!Fixtures are appropriately defined for testing seller seeding functionality.
20-33: LGTM!Good coverage of the
get_sellerfunction: tests both the fetch path and the early-return-when-no-ID path.
36-53: LGTM!Thorough validation of the
build_seller_dataoutput structure.
83-88: LGTM!Appropriate test for the simple orchestration wrapper.
tests/seed/test_seed_api.py (1)
26-26: LGTM! Test coverage for accounts seeding added.The test now correctly verifies that
seed_accountsis invoked alongsideseed_catalogduringseed_apiexecution, aligning with the concurrent seeding implementation.Also applies to: 38-38
seed/seed_api.py (1)
7-7: LGTM! Concurrent seeding implementation is well-structured.The parallel execution of catalog and accounts seeding using
asyncio.gatherimproves performance. The error handling with a finally block ensures context is always saved, maintaining data integrity even when seeding fails.Also applies to: 20-20, 24-33
seed/catalog/product.py (1)
51-65: LGTM! Logic inversion improves code clarity.The refactored control flow makes both paths explicit: product creation when None, and returning the existing product otherwise. This improves maintainability without changing behavior.
tests/seed/catalog/test_product.py (1)
44-44: LGTM! Test updates align with the new fixture pattern.The test correctly uses the
fake_file_factoryfixture and keyword arguments forinit_product. The strict assertionassert_called_once()appropriately verifies that create is called exactly once in the creation path.Also applies to: 53-53, 56-56
seed/accounts/accounts.py (1)
13-23: LGTM! Account seeding orchestration is well-structured.The function correctly orchestrates all account-related seed operations in sequence, with appropriate logging for debugging and observability.
tests/seed/accounts/test_buyer.py (1)
1-105: LGTM! Comprehensive test coverage for buyer seeding.The test suite thoroughly covers all buyer seeding scenarios: retrieval with/without ID, data building, initialization with existing/new buyers, and orchestration. The tests use appropriate mocking, clear assertions, and follow pytest best practices.
tests/seed/accounts/test_module.py (1)
38-99: LGTM! Comprehensive test coverage for module seeding.The test suite thoroughly covers module retrieval, refresh with filtering, and seed orchestration. The tests properly mock async iteration and verify context updates, aligning well with the module seeding implementation.
tests/seed/accounts/test_accounts.py (1)
6-30: LGTM! Orchestration test correctly verifies all seeders are called.The test properly patches all six seed functions and verifies each is called exactly once, ensuring the
seed_accountsorchestrator functions correctly. The structure is clean and follows pytest best practices.seed/accounts/user_group.py (3)
15-33: LGTM!The function correctly handles the absence of a user group ID, validates the type of cached resources, and falls back to the API when needed.
36-52: LGTM!The function correctly validates the required environment variable and builds the user group data dictionary.
77-83: LGTM!The orchestration logic is sound. Once
init_user_groupis fixed to raise an exception instead of returningNone, this function will correctly propagate the created or existingUserGroup.seed/accounts/buyer.py (4)
17-35: LGTM!The function follows the established pattern for resource retrieval, properly handling context lookup, type validation, and API fallback.
38-61: LGTM!The function correctly validates the required environment variable and constructs a complete buyer data dictionary with contact and address information.
64-85: Verify whether re-fetching the buyer after creation is necessary.The function correctly handles creation and error cases. However, line 77 fetches the buyer again immediately after creation. If the
createcall already returns a completeBuyerinstance, this extra API call may be unnecessary.Please confirm whether this re-fetch is required to populate server-side fields or if the created instance is sufficient.
88-93: LGTM!The orchestration function correctly delegates to
init_buyerand provides appropriate logging. The return type ofNoneis consistent with its role as a side-effect operation.
robcsegal
force-pushed
the
MPT-15443-seed-data-required-for-accounts-e-2-e-tests
branch
from
fcf3f67 to
6772c50
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/catalog/product.py (1)
18-41: Forward injectedcontextintoget_product; note thatpublish_productcannot forward itsmpt_operationsclient
init_product,review_product, andpublish_productall acceptcontextand a client parameter, but they callget_product()with no arguments. This breaks dependency injection for tests or alternate wiring. However, there's an important constraint:get_productexpectsmpt_vendor(for catalog read operations), whilepublish_productreceivesmpt_operations(for operations). These are intentionally different clients.The correct fix is:
async def init_product( context: Context = DEFAULT_CONTEXT, mpt_vendor: AsyncMPTClient = DEFAULT_MPT_VENDOR, ) -> Product: """Get or create product.""" - product = await get_product() + product = await get_product(context=context, mpt_vendor=mpt_vendor) @@ async def review_product( context: Context = DEFAULT_CONTEXT, mpt_vendor: AsyncMPTClient = DEFAULT_MPT_VENDOR, ) -> Product | None: @@ - product = await get_product() + product = await get_product(context=context, mpt_vendor=mpt_vendor) @@ async def publish_product( context: Context = DEFAULT_CONTEXT, mpt_operations: AsyncMPTClient = DEFAULT_MPT_OPERATIONS, ) -> Product | None: @@ - product = await get_product() + product = await get_product(context=context)For
publish_product, only forwardcontextsinceget_productusesmpt_vendorfor fetching, notmpt_operations.Ruff also flags
ValueError("Product creation failed")(TRY003) at line 63. Either extract the message to a constant or add# noqa: TRY003.
♻️ Duplicate comments (2)
seed/accounts/module.py (1)
35-57: Clarify refresh semantics and address the TRY003 warning (repeat of earlier feedback)
refresh_module’s docstring promises “always fetch”, but the implementation returns the cached module when present and only hits the API whenget_modulereturnsNone. Either adjust the docstring to reflect the actual behavior (“fetch if missing”) or change the code to always re-fetch when called.Ruff’s TRY003 warning on
ValueError("Could not seed module: no valid Module found.")is still present. As suggested earlier, either move the message into a constant and raiseValueError(MODULE_SEED_ERROR)or keep the literal and append# noqa: TRY003on that line.Also applies to: 60-71
seed/accounts/user_group.py (1)
55-74: Fix type safety violation: return type mismatch.
init_user_groupdeclares return typeUserGroupbut returnsNoneon line 72 when creation fails. This violates the type contract and will cause runtime type errors whenseed_user_group(line 81) receivesNone.For consistency with
seed/accounts/buyer.py(lines 76-83), raise an exception instead of returningNone.Apply this diff:
created = await mpt_operations.accounts.user_groups.create(user_group_data) if isinstance(created, UserGroup): context.set_resource("accounts.user_group", created) context["accounts.user_group.id"] = created.id logger.info("User group created: %s", created.id) return created logger.warning("User group creation failed") - return None + raise ValueError("User group creation failed") logger.info("User group already exists: %s", user_group.id) return user_group
🧹 Nitpick comments (3)
seed/accounts/licensee.py (1)
38-66: Tighten env var handling and satisfy Ruff TRY003 on the error message
build_licensee_datapullsCLIENT_ACCOUNT_IDfrom the environment and passes it straight into the payload; if the env var is missing, you’ll send{"account": {"id": None}}, which will likely fail deeper in the stack. It’s safer to validate early (e.g., raise or log clearly when the env var is absent) rather than relying on downstream errors.Ruff also flags the literal message in
ValueError("Licensee creation failed")(TRY003). You can either move this string into a module-level constant and reuse it in the raise, or, if you prefer the literal, append# noqa: TRY003on that line.Also applies to: 69-87
tests/seed/accounts/test_accounts.py (1)
6-6: Remove unusednoqadirective.The
WPS210rule is not recognized by Ruff (used in this project). As per the static analysis hint, this directive can be removed.Apply this diff:
-async def test_seed_accounts() -> None: # noqa: WPS210 +async def test_seed_accounts() -> None:tests/seed/accounts/test_user_group.py (1)
48-48: Remove unused context assignment.Line 48 sets
context["accounts.account.id"]butbuild_user_group_datais patched in this test, so the value is never used. This can be removed for clarity.Apply this diff:
user_groups_service.create.return_value = user_group operations_client.accounts.user_groups = user_groups_service - context["accounts.account.id"] = "ACC-1086-6867" context["accounts.module.id"] = "MOD-456"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
seed/accounts/logo.pngis excluded by!**/*.png
📒 Files selected for processing (24)
pyproject.toml(2 hunks)seed/accounts/accounts.py(1 hunks)seed/accounts/api_tokens.py(1 hunks)seed/accounts/buyer.py(1 hunks)seed/accounts/licensee.py(1 hunks)seed/accounts/module.py(1 hunks)seed/accounts/seller.py(1 hunks)seed/accounts/user_group.py(1 hunks)seed/catalog/product.py(1 hunks)seed/container.py(1 hunks)seed/seed_api.py(2 hunks)tests/e2e/accounts/buyers/conftest.py(1 hunks)tests/e2e/accounts/licensees/conftest.py(1 hunks)tests/e2e/accounts/sellers/conftest.py(1 hunks)tests/seed/accounts/test_accounts.py(1 hunks)tests/seed/accounts/test_api_token.py(1 hunks)tests/seed/accounts/test_buyer.py(1 hunks)tests/seed/accounts/test_licensee.py(1 hunks)tests/seed/accounts/test_module.py(1 hunks)tests/seed/accounts/test_seller.py(1 hunks)tests/seed/accounts/test_user_group.py(1 hunks)tests/seed/catalog/test_product.py(1 hunks)tests/seed/conftest.py(3 hunks)tests/seed/test_seed_api.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/e2e/accounts/sellers/conftest.py
- seed/accounts/api_tokens.py
- seed/seed_api.py
- tests/seed/test_seed_api.py
- seed/accounts/seller.py
- pyproject.toml
- tests/seed/accounts/test_licensee.py
- tests/seed/accounts/test_buyer.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-27T00:05:54.701Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/accounts/accounts_users/test_async_accounts_users.py:0-0
Timestamp: 2025-11-27T00:05:54.701Z
Learning: In pytest-asyncio, async fixtures are automatically resolved before being passed to test functions (both sync and async). Test functions receive the yielded value from async fixtures, not coroutines, so fixture parameters should never be awaited. A sync test function can use async fixtures without any special handling.
Applied to files:
tests/seed/conftest.pytests/seed/catalog/test_product.py
🧬 Code graph analysis (12)
seed/accounts/licensee.py (3)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/licensees.py (1)
Licensee(21-22)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)
tests/seed/accounts/test_module.py (4)
mpt_api_client/resources/accounts/modules.py (2)
AsyncModulesService(29-32)Module(11-12)seed/accounts/module.py (3)
get_module(15-32)refresh_module(36-57)seed_module(61-72)seed/context.py (2)
Context(12-33)get_resource(19-29)mpt_api_client/models/model.py (1)
id(119-121)
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/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)mpt_api_client/models/model.py (1)
id(119-121)
tests/seed/accounts/test_seller.py (2)
seed/accounts/seller.py (4)
build_seller_data(37-52)get_seller(16-33)init_seller(56-74)seed_seller(78-82)seed/context.py (2)
Context(12-33)get_resource(19-29)
seed/accounts/module.py (4)
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/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)
tests/seed/accounts/test_user_group.py (2)
seed/accounts/user_group.py (4)
build_user_group_data(37-52)get_user_group(16-33)init_user_group(56-74)seed_user_group(78-83)seed/context.py (2)
Context(12-33)get_resource(19-29)
tests/seed/conftest.py (1)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)
tests/seed/accounts/test_api_token.py (2)
seed/accounts/api_tokens.py (4)
build_api_token_data(36-48)get_api_token(15-32)init_api_token(52-67)seed_api_token(71-75)seed/context.py (1)
get_resource(19-29)
seed/accounts/accounts.py (6)
seed/accounts/api_tokens.py (1)
seed_api_token(71-75)seed/accounts/buyer.py (1)
seed_buyer(89-93)seed/accounts/licensee.py (1)
seed_licensee(93-97)seed/accounts/module.py (1)
seed_module(61-72)seed/accounts/seller.py (1)
seed_seller(78-82)seed/accounts/user_group.py (1)
seed_user_group(78-83)
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/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)mpt_api_client/models/model.py (1)
id(119-121)
seed/catalog/product.py (5)
tests/seed/catalog/test_product.py (1)
product(17-18)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)mpt_api_client/resources/catalog/products.py (1)
Product(49-50)seed/context.py (1)
set_resource(31-33)mpt_api_client/models/model.py (1)
id(119-121)
tests/seed/accounts/test_accounts.py (1)
seed/accounts/accounts.py (1)
seed_accounts(13-23)
🪛 Ruff (0.14.7)
seed/accounts/licensee.py
87-87: Avoid specifying long messages outside the exception class
(TRY003)
tests/seed/accounts/test_module.py
21-21: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
seed/accounts/user_group.py
43-43: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/module.py
69-69: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/accounts.py
13-13: Unused noqa directive (unknown: WPS217)
Remove unused noqa directive
(RUF100)
seed/accounts/buyer.py
43-43: Avoid specifying long messages outside the exception class
(TRY003)
83-83: Avoid specifying long messages outside the exception class
(TRY003)
seed/catalog/product.py
63-63: Avoid specifying long messages outside the exception class
(TRY003)
tests/seed/accounts/test_accounts.py
6-6: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
tests/seed/catalog/test_product.py
51-51: Unused lambda argument: args
(ARG005)
51-51: Unused lambda argument: kwargs
(ARG005)
⏰ 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)
tests/e2e/accounts/licensees/conftest.py (1)
35-35: LGTM! Test data update aligns with PR objectives.The address line change to "123 Main St" is a straightforward test data update that aligns with the PR's goal of adding seeded accounts e2e data.
tests/e2e/accounts/buyers/conftest.py (1)
30-36: City test data update looks goodChanging the buyer address city to
"Los Angeles"is a straightforward fixture data adjustment and aligns with the rest of the seeded account data.seed/container.py (1)
45-68: Accounts seeding modules correctly added to DI wiringThe additional
seed.accounts.*modules wired here follow the same pattern as the existing catalog wiring and should allow dependency-injected seeding for all account-related resources.seed/accounts/accounts.py (1)
13-23: Remove the unusednoqa: WPS217directiveRuff reports
# noqa: WPS217onseed_accountsas an unused/unknown directive (RUF100). The function body is simple enough that this suppression isn’t needed; you can safely drop the# noqa: WPS217comment from the signature.⛔ 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.tests/seed/conftest.py (1)
10-36: LGTM!The new test fixtures and helper function follow existing patterns and provide useful test utilities for file I/O mocking and client instantiation.
tests/seed/accounts/test_api_token.py (1)
57-70: LGTM!Good use of
monkeypatch.setenvto properly mock the environment variable, addressing the previous review feedback.seed/accounts/buyer.py (1)
17-93: LGTM!The implementation follows established patterns, properly forwards injected dependencies, validates required environment variables, and includes type checking for API responses. Past review feedback has been addressed.
tests/seed/accounts/test_seller.py (1)
1-88: LGTM!Test coverage is comprehensive, covering retrieval, data building, initialization (both with and without mocked data builders), and seeding orchestration.
robcsegal
force-pushed
the
MPT-15443-seed-data-required-for-accounts-e-2-e-tests
branch
from
6772c50 to
e3db949
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
tests/seed/catalog/test_product.py (1)
62-103: Patch asyncget_productwithAsyncMockin remaining tests.Four tests still patch the async function
get_productwith a regular mock instead ofAsyncMock:
test_review_product_draft_status(line 69)test_review_product_non_draft_status(line 79)test_publish_product_reviewing_status(line 89)test_publish_product_non_reviewing_status(line 99)This causes
TypeErrorwhen the test code awaits the patched function.Apply this pattern to all four tests:
async def test_review_product_draft_status( context, products_service, vendor_client, product ) -> None: product.status = "Draft" products_service.review.return_value = product vendor_client.catalog.products = products_service - with ( - patch("seed.catalog.product.get_product", return_value=product), - ): + with patch("seed.catalog.product.get_product", new_callable=AsyncMock) as mock_get_product: + mock_get_product.return_value = product result = await review_product(context, mpt_vendor=vendor_client) assert result == product products_service.review.assert_called_once()Apply the same pattern to the other three tests at lines 79, 89, and 99.
seed/accounts/seller.py (1)
1-1: Drop obsoletemypy: disable-error-code=unreachable
init_selleris now correctly typed asSeller | None, and the control flow matches that signature, so theunreachablesuppression at module level is no longer justified. Removing it will let mypy surface any future real issues.
🧹 Nitpick comments (3)
seed/accounts/accounts.py (1)
13-23: Consider removing the unusednoqadirective.The sequential seeding logic is correct. However, Ruff reports that the
# noqa: WPS217directive on line 13 is unused (unknown code). Consider removing it.Apply this diff:
-async def seed_accounts() -> None: # noqa: WPS217 +async def seed_accounts() -> None: """Seed accounts data including account."""tests/seed/accounts/test_accounts.py (1)
6-6: Remove unused# noqa: WPS210directiveRuff reports this as an unknown/unused noqa code. Since the function body is simple and no longer needs a complexity suppression, you can safely drop the directive to keep lint output clean.
seed/accounts/user_group.py (1)
41-43: TRY003: consider extracting the env‑var error messageRuff’s TRY003 warning here is stylistic, but if you want to satisfy it you can move the message into a constant (or a small custom exception), e.g.:
- account_id = os.getenv("CLIENT_ACCOUNT_ID") - if not account_id: - raise ValueError("CLIENT_ACCOUNT_ID environment variable is required") + account_id = os.getenv("CLIENT_ACCOUNT_ID") + if not account_id: + raise ValueError(CLIENT_ACCOUNT_ID_MISSING_MSG)with
CLIENT_ACCOUNT_ID_MISSING_MSGdefined at module level. Functionally it’s fine as‑is, so this is optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
seed/accounts/logo.pngis excluded by!**/*.png
📒 Files selected for processing (24)
pyproject.toml(2 hunks)seed/accounts/accounts.py(1 hunks)seed/accounts/api_tokens.py(1 hunks)seed/accounts/buyer.py(1 hunks)seed/accounts/licensee.py(1 hunks)seed/accounts/module.py(1 hunks)seed/accounts/seller.py(1 hunks)seed/accounts/user_group.py(1 hunks)seed/catalog/product.py(1 hunks)seed/container.py(1 hunks)seed/seed_api.py(2 hunks)tests/e2e/accounts/buyers/conftest.py(1 hunks)tests/e2e/accounts/licensees/conftest.py(1 hunks)tests/e2e/accounts/sellers/conftest.py(1 hunks)tests/seed/accounts/test_accounts.py(1 hunks)tests/seed/accounts/test_api_token.py(1 hunks)tests/seed/accounts/test_buyer.py(1 hunks)tests/seed/accounts/test_licensee.py(1 hunks)tests/seed/accounts/test_module.py(1 hunks)tests/seed/accounts/test_seller.py(1 hunks)tests/seed/accounts/test_user_group.py(1 hunks)tests/seed/catalog/test_product.py(1 hunks)tests/seed/conftest.py(3 hunks)tests/seed/test_seed_api.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- seed/container.py
- seed/accounts/api_tokens.py
- tests/seed/accounts/test_api_token.py
- tests/seed/accounts/test_licensee.py
- tests/seed/accounts/test_user_group.py
- tests/seed/accounts/test_buyer.py
- tests/e2e/accounts/licensees/conftest.py
- pyproject.toml
- tests/seed/conftest.py
- tests/e2e/accounts/buyers/conftest.py
🧰 Additional context used
🧬 Code graph analysis (7)
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/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)mpt_api_client/models/model.py (1)
id(119-121)
tests/seed/accounts/test_module.py (2)
seed/accounts/module.py (3)
get_module(15-32)refresh_module(36-57)seed_module(61-72)seed/context.py (2)
Context(12-33)get_resource(19-29)
seed/accounts/module.py (4)
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/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)
seed/accounts/seller.py (8)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/sellers.py (1)
Seller(13-14)tests/seed/conftest.py (1)
context(15-16)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)tests/e2e/conftest.py (1)
logger(63-64)tests/e2e/accounts/conftest.py (1)
seller_id(27-28)tests/seed/accounts/test_seller.py (1)
seller(11-12)mpt_api_client/models/model.py (1)
id(119-121)
seed/accounts/licensee.py (4)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/licensees.py (1)
Licensee(21-22)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)tests/seed/accounts/test_licensee.py (1)
licensee(12-30)
tests/seed/accounts/test_accounts.py (1)
seed/accounts/accounts.py (1)
seed_accounts(13-23)
seed/accounts/user_group.py (3)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/user_groups.py (1)
UserGroup(11-12)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)
🪛 Ruff (0.14.7)
seed/accounts/buyer.py
43-43: Avoid specifying long messages outside the exception class
(TRY003)
83-83: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/module.py
69-69: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/licensee.py
87-87: Avoid specifying long messages outside the exception class
(TRY003)
tests/seed/accounts/test_accounts.py
6-6: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
seed/accounts/accounts.py
13-13: Unused noqa directive (unknown: WPS217)
Remove unused noqa directive
(RUF100)
seed/accounts/user_group.py
43-43: Avoid specifying long messages outside the exception class
(TRY003)
seed/catalog/product.py
63-63: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (23)
seed/catalog/product.py (1)
51-65: LGTM! Control flow restructuring is clear.The refactored control flow—guarding product creation behind
if product is None:and explicitly returning the existing product at the end—is functionally equivalent to the previous early-return pattern and improves readability.tests/e2e/accounts/sellers/conftest.py (1)
20-20: LGTM!The test data update aligns the city value with the seeded account data used elsewhere in the PR (e.g., buyer and licensee addresses also use "Los Angeles").
seed/accounts/buyer.py (4)
17-35: LGTM!The function correctly retrieves or fetches the buyer, with proper context caching and resource management.
38-61: LGTM!The environment variable validation added in line 42-43 ensures clear failure messages when CLIENT_ACCOUNT_ID is missing.
64-85: LGTM!The function correctly forwards injected parameters to helper functions and handles both existing and new buyer creation paths.
88-92: LGTM!The orchestration function provides appropriate logging around the buyer initialization.
seed/accounts/licensee.py (3)
17-35: LGTM!The function correctly retrieves or fetches the licensee, with proper context caching and resource management.
69-89: LGTM!The function correctly forwards injected parameters to helper functions and handles both existing and new licensee creation paths.
92-97: LGTM!The orchestration function provides appropriate logging around the licensee initialization.
seed/accounts/module.py (3)
14-32: LGTM!The function correctly retrieves the module from context or fetches from the API with proper caching.
35-57: LGTM!The function appropriately fetches a new module by filtering for "Access Management" and handles the async iteration pattern correctly.
60-72: LGTM!The orchestration correctly delegates to get_module and falls back to refresh_module when needed.
tests/seed/test_seed_api.py (2)
26-26: LGTM!The test now patches
seed_accountsto verify it's invoked by the updatedseed_apifunction.
38-38: LGTM!The assertion verifies that
seed_accountsis called once during the seeding process.seed/seed_api.py (2)
7-7: LGTM!The import enables the new accounts seeding orchestration.
20-26: LGTM!Concurrent execution of catalog and accounts seeding via
asyncio.gatheris an appropriate pattern for independent seeding tasks.seed/accounts/accounts.py (1)
1-8: LGTM!The imports correctly reference all account seeding functions.
tests/seed/accounts/test_module.py (5)
10-21: LGTM!The exception chaining on line 21 correctly addresses the B904 linting rule by propagating the StopIteration context.
38-46: LGTM!The test correctly verifies module retrieval and context storage.
49-51: LGTM!The test correctly verifies the behavior when no module ID is present.
54-70: LGTM!The test correctly uses AsyncMock for the async
get_modulefunction and verifies the refresh flow.
73-89: LGTM!The test correctly uses AsyncMock and validates module refresh when a new module is found.
tests/seed/accounts/test_seller.py (1)
10-88: Seller seeding tests provide solid coverageThe tests exercise the main retrieval, creation, and seeding paths (including context caching and DI usage) and look consistent with the current
seller.pyimplementation. No issues spotted.
robcsegal
force-pushed
the
MPT-15443-seed-data-required-for-accounts-e-2-e-tests
branch
from
e3db949 to
d6a9feb
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
tests/seed/catalog/test_product.py (1)
48-53: Remove duplicatepathlib.Path.openpatch.Lines 51 and 52 both patch
pathlib.Path.open. The second patch overrides the first, making line 51 effectively dead code. This was flagged in a previous review but the duplicate remains.Apply this diff to remove the duplicate:
with ( patch("seed.catalog.product.get_product", new_callable=AsyncMock) as mock_get_product, patch("seed.catalog.product.icon", new=MagicMock()), - patch("pathlib.Path.open", new=lambda *_, **_unused: fake_file_factory()), patch("pathlib.Path.open", new=lambda *_, **_unused: fake_file_factory()), ):seed/accounts/module.py (1)
68-69: Address Ruff TRY003 on theValueErrormessage.This issue was previously flagged. To satisfy the linter while keeping the descriptive message, either add
# noqa: TRY003or extract the message into a module-level constant.- raise ValueError("Could not seed module: no valid Module found.") + raise ValueError("Could not seed module: no valid Module found.") # noqa: TRY003
🧹 Nitpick comments (6)
seed/accounts/user_group.py (1)
1-1: Remove obsolete mypy suppression.All functions in this file correctly annotate their return types as
UserGroup | Nonewhere applicable, and there are no unreachable code paths. The# mypy: disable-error-code=unreachablesuppression on line 1 is no longer needed and should be removed.-# mypy: disable-error-code=unreachable import loggingseed/accounts/accounts.py (1)
13-13: Remove unusednoqadirective.Ruff flagged
# noqa: WPS217as unused since WPS217 is not a recognized Ruff rule code.-async def seed_accounts() -> None: # noqa: WPS217 +async def seed_accounts() -> None:tests/seed/accounts/test_buyer.py (1)
81-97: Consider asserting context updates after buyer creation.The test verifies
createis called but doesn't assert thatcontext.set_resourceandcontext["accounts.buyer.id"]are updated after creation. This would ensure the full creation flow including context caching is validated.tests/seed/accounts/test_accounts.py (2)
6-6: Remove unusednoqadirective.Ruff flagged
# noqa: WPS210as unused since WPS210 is not a recognized Ruff rule code.-async def test_seed_accounts() -> None: # noqa: WPS210 +async def test_seed_accounts() -> None:
29-30: Consider usingassert_awaited_once()for async mocks.Since these are
AsyncMockinstances, usingassert_awaited_once()is more semantically correct thanassert_called_once()as it specifically verifies the coroutine was awaited.for mock in mocks: - mock.assert_called_once() + mock.assert_awaited_once()seed/accounts/licensee.py (1)
26-35: Consider simplifying the conditional logic.After the
try/exceptblock,licenseeis either a validLicenseeorNone. Theisinstancecheck on line 30 is effectively equivalent toif licensee is None. Consider simplifying for clarity:try: licensee = context.get_resource("accounts.licensee", licensee_id) except ValueError: licensee = None - if not isinstance(licensee, Licensee): + if licensee is None: licensee = await mpt_client.accounts.licensees.get(licensee_id) context.set_resource("accounts.licensee", licensee) context["accounts.licensee.id"] = licensee.id - return licensee - return licensee + return licensee
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
seed/accounts/logo.pngis excluded by!**/*.png
📒 Files selected for processing (24)
pyproject.toml(2 hunks)seed/accounts/accounts.py(1 hunks)seed/accounts/api_tokens.py(1 hunks)seed/accounts/buyer.py(1 hunks)seed/accounts/licensee.py(1 hunks)seed/accounts/module.py(1 hunks)seed/accounts/seller.py(1 hunks)seed/accounts/user_group.py(1 hunks)seed/catalog/product.py(1 hunks)seed/container.py(1 hunks)seed/seed_api.py(2 hunks)tests/e2e/accounts/buyers/conftest.py(1 hunks)tests/e2e/accounts/licensees/conftest.py(1 hunks)tests/e2e/accounts/sellers/conftest.py(1 hunks)tests/seed/accounts/test_accounts.py(1 hunks)tests/seed/accounts/test_api_token.py(1 hunks)tests/seed/accounts/test_buyer.py(1 hunks)tests/seed/accounts/test_licensee.py(1 hunks)tests/seed/accounts/test_module.py(1 hunks)tests/seed/accounts/test_seller.py(1 hunks)tests/seed/accounts/test_user_group.py(1 hunks)tests/seed/catalog/test_product.py(1 hunks)tests/seed/conftest.py(3 hunks)tests/seed/test_seed_api.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- pyproject.toml
- tests/seed/accounts/test_module.py
- tests/e2e/accounts/buyers/conftest.py
- seed/accounts/api_tokens.py
- tests/seed/accounts/test_api_token.py
- seed/accounts/seller.py
- tests/e2e/accounts/sellers/conftest.py
- tests/seed/conftest.py
- seed/container.py
- tests/seed/accounts/test_seller.py
- tests/seed/accounts/test_licensee.py
- tests/e2e/accounts/licensees/conftest.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-27T00:05:54.701Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/accounts/accounts_users/test_async_accounts_users.py:0-0
Timestamp: 2025-11-27T00:05:54.701Z
Learning: In pytest-asyncio, async fixtures are automatically resolved before being passed to test functions (both sync and async). Test functions receive the yielded value from async fixtures, not coroutines, so fixture parameters should never be awaited. A sync test function can use async fixtures without any special handling.
Applied to files:
tests/seed/catalog/test_product.py
🧬 Code graph analysis (10)
tests/seed/accounts/test_accounts.py (1)
seed/accounts/accounts.py (1)
seed_accounts(13-23)
tests/seed/accounts/test_buyer.py (3)
mpt_api_client/resources/accounts/buyers.py (1)
Buyer(26-27)seed/accounts/buyer.py (4)
build_buyer_data(39-61)get_buyer(18-35)init_buyer(65-85)seed_buyer(89-93)seed/context.py (2)
Context(12-33)get_resource(19-29)
seed/accounts/module.py (5)
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/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)mpt_api_client/models/model.py (1)
id(119-121)
seed/accounts/licensee.py (3)
mpt_api_client/resources/accounts/licensees.py (1)
Licensee(21-22)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)mpt_api_client/models/model.py (1)
id(119-121)
seed/accounts/buyer.py (2)
mpt_api_client/resources/accounts/buyers.py (1)
Buyer(26-27)seed/context.py (3)
get_string(15-17)get_resource(19-29)set_resource(31-33)
seed/seed_api.py (3)
seed/accounts/accounts.py (1)
seed_accounts(13-23)seed/context.py (1)
load_context(36-52)seed/catalog/catalog.py (1)
seed_catalog(32-39)
tests/seed/catalog/test_product.py (2)
tests/seed/conftest.py (2)
context(15-16)fake_file_factory(35-36)seed/catalog/product.py (1)
init_product(45-65)
seed/catalog/product.py (7)
tests/seed/catalog/test_product.py (1)
product(17-18)tests/e2e/conftest.py (1)
logger(63-64)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)mpt_api_client/resources/catalog/products.py (1)
Product(49-50)tests/seed/conftest.py (1)
context(15-16)seed/context.py (1)
set_resource(31-33)mpt_api_client/models/model.py (1)
id(119-121)
tests/seed/accounts/test_user_group.py (2)
seed/accounts/user_group.py (4)
build_user_group_data(37-52)get_user_group(16-33)init_user_group(56-76)seed_user_group(80-85)seed/context.py (2)
Context(12-33)get_resource(19-29)
seed/accounts/accounts.py (6)
seed/accounts/api_tokens.py (1)
seed_api_token(71-75)seed/accounts/buyer.py (1)
seed_buyer(89-93)seed/accounts/licensee.py (1)
seed_licensee(95-99)seed/accounts/module.py (1)
seed_module(61-72)seed/accounts/seller.py (1)
seed_seller(78-82)seed/accounts/user_group.py (1)
seed_user_group(80-85)
🪛 Ruff (0.14.7)
tests/seed/accounts/test_accounts.py
6-6: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
seed/accounts/module.py
69-69: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/licensee.py
45-45: Avoid specifying long messages outside the exception class
(TRY003)
89-89: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/buyer.py
43-43: Avoid specifying long messages outside the exception class
(TRY003)
83-83: Avoid specifying long messages outside the exception class
(TRY003)
seed/catalog/product.py
63-63: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/accounts.py
13-13: Unused noqa directive (unknown: WPS217)
Remove unused noqa directive
(RUF100)
seed/accounts/user_group.py
43-43: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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)
tests/seed/test_seed_api.py (1)
26-38: LGTM!The test correctly mocks the new
seed_accountsfunction and asserts it's called once during theseed_apiexecution, maintaining consistency with the existingseed_catalogassertion pattern.seed/catalog/product.py (1)
51-65: LGTM!The refactored
init_productcorrectly guards product creation behind aNonecheck. The control flow is clear: create and return when missing, log and return when found. Error handling with theisinstancecheck andValueErroron failure is consistent with other seed modules.seed/accounts/buyer.py (1)
64-85: LGTM!The
init_buyerfunction correctly passes injected parameters toget_buyerandbuild_buyer_data, addressing previous review feedback. The create-then-fetch pattern ensures complete buyer data is cached.seed/accounts/user_group.py (1)
55-76: LGTM!The
init_user_groupfunction correctly validates the API response type after creation and handles both success and failure paths appropriately. The early return pattern for existing user groups improves readability.seed/seed_api.py (2)
20-20: LGTM!The type annotation
list[asyncio.Task[object]]correctly captures that the task list can contain tasks with heterogeneous return types.
24-29: Verify concurrent execution safety between catalog and accounts seeding.The
seed_catalog()andseed_accounts()tasks now run concurrently viaasyncio.gather. Ensure there are no shared dependencies between catalog entities (products, item groups, parameters) and account entities (seller, buyer, module, API token, user group, licensee) that could cause race conditions or ordering issues.seed/accounts/accounts.py (1)
16-21: Sequential execution is appropriate for dependent entities.The sequential ordering (seller → buyer → module → api_token → user_group → licensee) correctly handles dependencies between account entities. Based on the relevant code snippets,
build_buyer_datareferencesseller_idfrom context, confirming this ordering is intentional.tests/seed/accounts/test_buyer.py (1)
100-104: LGTM!The test correctly verifies that
seed_buyerdelegates toinit_buyerusingassert_awaited_once().tests/seed/accounts/test_user_group.py (2)
43-61: LGTM!The test correctly sets up context with required module ID, patches dependencies, and verifies the creation flow through
init_user_group.
64-78: LGTM!The test correctly uses
monkeypatch.setenvto setCLIENT_ACCOUNT_ID, addressing the previous review feedback about environment variable requirements.seed/accounts/module.py (2)
14-32: LGTM!The
get_modulefunction correctly implements the context-first lookup pattern with API fallback, properly handling theValueErrorfromcontext.get_resourcewhen the resource doesn't exist.
35-57: LGTM!The
refresh_modulefunction correctly forwards dependencies toget_moduleand handles the async iteration over filtered modules appropriately. The instance check before accessingfirst_module.idis a good defensive practice.seed/accounts/licensee.py (4)
1-14: LGTM!The imports and module setup are appropriate. The global
iconpath cleanly references the logo asset relative to the module's location.
38-68: LGTM - validation added as requested.The
CLIENT_ACCOUNT_IDvalidation has been properly implemented with a clear error message. The TRY003 static analysis hint is a minor style preference and acceptable for this use case.
71-91: LGTM - dependency forwarding addressed.The
contextandmpt_clientparameters are now properly forwarded to helper functions as previously requested.Minor style note: Line 81 uses
pathlib.Path.open(icon, "rb")which works, buticon.open("rb")would be more idiomatic sinceiconis already aPathinstance.
94-99: LGTM!The seeding orchestrator is clean and follows the established pattern used by other seed modules.
robcsegal
force-pushed
the
MPT-15443-seed-data-required-for-accounts-e-2-e-tests
branch
from
d6a9feb to
6edaaa8
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (7)
tests/seed/catalog/test_product.py (1)
44-55: Remove duplicatepathlib.Path.openpatch intest_get_or_create_product_create_new.Lines 51–52 patch
pathlib.Path.opentwice with the same lambda; the second patch simply overrides the first, so the first is redundant noise.You can collapse this to a single patch:
with ( patch("seed.catalog.product.get_product", new_callable=AsyncMock) as mock_get_product, patch("seed.catalog.product.icon", new=MagicMock()), - patch("pathlib.Path.open", new=lambda *_, **_unused: fake_file_factory()), - patch("pathlib.Path.open", new=lambda *_, **_unused: fake_file_factory()), + patch("pathlib.Path.open", new=lambda *_, **__: fake_file_factory()), ):This keeps the behavior the same while simplifying the test and avoiding double-patching the same target.
tests/seed/accounts/test_buyer.py (2)
36-60: Test will fail: missingCLIENT_ACCOUNT_IDenvironment variable and seller ID.Per
seed/accounts/buyer.pylines 38-42,build_buyer_datareadsCLIENT_ACCOUNT_IDfromos.getenv(), not from context. The test setscontext["accounts.account.id"]but never sets the environment variable, so the function will raiseValueError("CLIENT_ACCOUNT_ID environment variable is required"). Additionally, line 55 expects an empty seller ID, but the actual function requirescontext["accounts.seller.id"].Apply this diff to fix:
-def test_build_buyer_data(context: Context) -> None: - context["accounts.account.id"] = "ACC-1086-6867" +def test_build_buyer_data(context: Context, monkeypatch) -> None: + monkeypatch.setenv("CLIENT_ACCOUNT_ID", "ACC-1086-6867") + context["accounts.seller.id"] = "SEL-123" expected_data = { "name": "E2E Seeded Buyer", "account": { "id": "ACC-1086-6867", }, + "sellers": [{"id": "SEL-123"}], "contact": { "firstName": "first", "lastName": "last", "email": "created.buyer@example.com", }, "address": { "addressLine1": "123 Main St", "city": "Los Angeles", "state": "CA", "postCode": "12345", "country": "US", }, - "sellers": [{"id": ""}], }
81-98: Test setup will fail: missingCLIENT_ACCOUNT_IDenvironment variable.Line 92 evaluates
build_buyer_data(context)during patch setup. Sincecontext["accounts.account.id"]is set but theCLIENT_ACCOUNT_IDenvironment variable is not, the function will raiseValueErrorbefore the test runs.Apply this diff to fix:
async def test_init_buyer_create_new( context: Context, operations_client, buyers_service, buyer ) -> None: buyers_service.create.return_value = buyer operations_client.accounts.buyers = buyers_service - context["accounts.account.id"] = "ACC-1086-6867" + monkeypatch.setenv("CLIENT_ACCOUNT_ID", "ACC-1086-6867") + context["accounts.seller.id"] = "SEL-123" buyers_service.get.return_value = buyer with ( patch("seed.accounts.buyer.get_buyer", return_value=None), patch("pathlib.Path.open", return_value=AsyncMock()), patch("seed.accounts.buyer.build_buyer_data", return_value=build_buyer_data(context)), ): result = await init_buyer(context=context, mpt_operations=operations_client)Also add
monkeypatchparameter:-async def test_init_buyer_create_new( - context: Context, operations_client, buyers_service, buyer +async def test_init_buyer_create_new( + context: Context, operations_client, buyers_service, buyer, monkeypatch ) -> None:seed/accounts/seller.py (1)
1-1: Remove themypyerror suppression directive.The
# mypy: disable-error-code=unreachableat line 1 appears unnecessary. All code paths in the file are reachable:
get_seller()has three reachable return statementsinit_seller()returns eithercreatedorNoneorsellerdepending on conditionals- No control flow dead ends exist
The function signatures correctly reflect return types (
Seller | None), and past improvements to type validation have been applied. Remove the directive:-# mypy: disable-error-code=unreachable import loggingseed/accounts/buyer.py (1)
30-34: Add type validation after API fetch.When fetching buyer from the API on line 31, the result is stored in context without verifying it's a valid
Buyerinstance. If the API returns an error or unexpected type, this could corrupt the context.Apply this diff:
if not isinstance(buyer, Buyer): buyer = await mpt_operations.accounts.buyers.get(buyer_id) + if not isinstance(buyer, Buyer): + return None context.set_resource("accounts.buyer", buyer) context["accounts.buyer.id"] = buyer.id return buyerseed/accounts/api_tokens.py (1)
40-48: ValidateCLIENT_ACCOUNT_IDandmodule_idbefore building payload.Both
account_idandmodule_idcan beNoneor empty, leading to confusing API errors.Apply this diff:
account_id = os.getenv("CLIENT_ACCOUNT_ID") module_id = context.get_string("accounts.module.id") + if not account_id: + raise ValueError("CLIENT_ACCOUNT_ID environment variable is not set.") + if not module_id: + raise ValueError("accounts.module.id missing from context; seed module first.") return { "account": {"id": account_id},seed/accounts/user_group.py (1)
28-32: Add type validation after API fetch.Same issue as in
buyer.py: when fetching from API on line 29, the result is stored in context without verifying it's a validUserGroupinstance.Apply this diff:
if not isinstance(user_group, UserGroup): user_group = await mpt_operations.accounts.user_groups.get(user_group_id) + if not isinstance(user_group, UserGroup): + return None context.set_resource("accounts.user_group", user_group) context["accounts.user_group.id"] = user_group.id return user_group
🧹 Nitpick comments (10)
seed/catalog/product.py (1)
50-65: Threadcontext/mpt_vendorintoget_productfor consistency.
init_productacceptscontextandmpt_vendorbut callsget_product()with no arguments, so the existence check uses DI-injected defaults while the creation path uses the explicit arguments. If a caller passes a non-defaultContextor vendor client, you could end up checking one context and writing to another.Refactor to route the same dependencies through:
- product = await get_product() + product = await get_product(context=context, mpt_vendor=mpt_vendor)This keeps retrieval and creation aligned and makes the behavior clearer and more testable.
seed/accounts/accounts.py (1)
13-13: Remove unusednoqadirective.Ruff reports that
WPS217is not a recognized error code. Thisnoqadirective appears to be for the wemake-python-styleguide linter, which may not be active in this project.Apply this diff:
-async def seed_accounts() -> None: # noqa: WPS217 +async def seed_accounts() -> None: """Seed accounts data including account."""Based on static analysis hints.
tests/seed/accounts/test_accounts.py (1)
6-6: Remove unusednoqadirective.Ruff reports that
WPS210is not a recognized error code. Thisnoqadirective appears to be for the wemake-python-styleguide linter, which may not be active in this project.Apply this diff:
-async def test_seed_accounts() -> None: # noqa: WPS210 +async def test_seed_accounts() -> None: with (Based on static analysis hints.
seed/accounts/module.py (1)
69-69: Consider addressing Ruff TRY003 for the error message.Ruff suggests avoiding long messages directly in exception raises. You can either extract the message to a constant or add a
noqacomment if the explicit message is important.Option 1 - Add suppression:
- raise ValueError("Could not seed module: no valid Module found.") + raise ValueError("Could not seed module: no valid Module found.") # noqa: TRY003Option 2 - Extract to constant:
+MODULE_SEED_ERROR = "Could not seed module: no valid Module found." + @inject async def seed_module() -> Module: """Seed module.""" logger.debug("Seeding module ...") existing_module = await get_module() if existing_module is None: refreshed = await refresh_module() logger.debug("Seeding module completed.") if refreshed is None: - raise ValueError("Could not seed module: no valid Module found.") + raise ValueError(MODULE_SEED_ERROR)Based on static analysis hints.
seed/accounts/licensee.py (1)
45-45: Consider addressing Ruff TRY003 for error messages.Ruff suggests avoiding long messages directly in exception raises (lines 45 and 89). You can either add
noqacomments or extract messages to constants.Apply this diff:
- raise ValueError("CLIENT_ACCOUNT_ID environment variable is required") + raise ValueError("CLIENT_ACCOUNT_ID environment variable is required") # noqa: TRY003- raise ValueError("Licensee creation failed") + raise ValueError("Licensee creation failed") # noqa: TRY003Based on static analysis hints.
Also applies to: 89-89
seed/accounts/buyer.py (1)
44-48: Validate seller_id before building payload.
seller_idis retrieved from context with an empty string default. Ifaccounts.seller.idis missing, the payload will contain an empty seller ID, potentially causing an unclear API error.Apply this diff:
seller_id = context.get_string("accounts.seller.id") + if not seller_id: + raise ValueError("accounts.seller.id missing from context; seed seller first.") return { "name": "E2E Seeded Buyer",seed/accounts/api_tokens.py (2)
27-31: Add type validation after API fetch.Same issue as in
buyer.py: when fetching from API on line 28, the result is stored in context without verifying it's a validApiTokeninstance.Apply this diff:
if not isinstance(api_token, ApiToken): api_token = await mpt_ops.accounts.api_tokens.get(api_token_id) + if not isinstance(api_token, ApiToken): + return None context.set_resource("accounts.api_token", api_token) context["accounts.api_token.id"] = api_token.id return api_token
58-67: Add type validation on API response.Unlike
init_buyer, this function doesn't validate thecreate()response. For consistency and safety, verify the result is anApiTokeninstance.Apply this diff:
if api_token is None: logger.debug("Creating API token ...") api_token_data = build_api_token_data(context=context) - api_token = await mpt_ops.accounts.api_tokens.create(api_token_data) - context.set_resource("accounts.api_token", api_token) - context["accounts.api_token.id"] = api_token.id - logger.info("API token created: %s", api_token.id) + created = await mpt_ops.accounts.api_tokens.create(api_token_data) + if isinstance(created, ApiToken): + context.set_resource("accounts.api_token", created) + context["accounts.api_token.id"] = created.id + logger.info("API token created: %s", created.id) + return created + logger.warning("API token creation failed") + raise ValueError("API token creation failed") else: logger.info("API token found: %s", api_token.id) return api_tokenseed/accounts/user_group.py (2)
44-51: Validatemodule_idbefore building payload.
module_idcould be an empty string ifaccounts.module.idis missing from context, leading to an unclear API error.Apply this diff:
module_id = context.get_string("accounts.module.id") + if not module_id: + raise ValueError("accounts.module.id missing from context; seed module first.") return { "name": "E2E Seeded User Group",
1-1: Remove mypy suppression if no longer needed.The return types for
init_user_groupandseed_user_groupnow honestly includeNone. Verify if this suppression is still required, and if not, remove it to enable proper type checking.Run the following script to check if removing the suppression causes mypy errors:
#!/bin/bash # Remove suppression and run mypy on this file sed '1d' seed/accounts/user_group.py > /tmp/user_group_test.py echo "Testing mypy without suppression..." pip install mypy -q 2>/dev/null mypy --ignore-missing-imports /tmp/user_group_test.py 2>&1 || echo "Mypy check complete"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
seed/accounts/logo.pngis excluded by!**/*.png
📒 Files selected for processing (24)
pyproject.toml(2 hunks)seed/accounts/accounts.py(1 hunks)seed/accounts/api_tokens.py(1 hunks)seed/accounts/buyer.py(1 hunks)seed/accounts/licensee.py(1 hunks)seed/accounts/module.py(1 hunks)seed/accounts/seller.py(1 hunks)seed/accounts/user_group.py(1 hunks)seed/catalog/product.py(1 hunks)seed/container.py(1 hunks)seed/seed_api.py(2 hunks)tests/e2e/accounts/buyers/conftest.py(1 hunks)tests/e2e/accounts/licensees/conftest.py(1 hunks)tests/e2e/accounts/sellers/conftest.py(1 hunks)tests/seed/accounts/test_accounts.py(1 hunks)tests/seed/accounts/test_api_token.py(1 hunks)tests/seed/accounts/test_buyer.py(1 hunks)tests/seed/accounts/test_licensee.py(1 hunks)tests/seed/accounts/test_module.py(1 hunks)tests/seed/accounts/test_seller.py(1 hunks)tests/seed/accounts/test_user_group.py(1 hunks)tests/seed/catalog/test_product.py(1 hunks)tests/seed/conftest.py(3 hunks)tests/seed/test_seed_api.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- pyproject.toml
- tests/seed/accounts/test_module.py
- tests/seed/accounts/test_seller.py
- seed/container.py
- tests/seed/conftest.py
- tests/e2e/accounts/licensees/conftest.py
- tests/e2e/accounts/buyers/conftest.py
- tests/seed/test_seed_api.py
- tests/seed/accounts/test_api_token.py
- seed/seed_api.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-27T00:05:54.701Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/accounts/accounts_users/test_async_accounts_users.py:0-0
Timestamp: 2025-11-27T00:05:54.701Z
Learning: In pytest-asyncio, async fixtures are automatically resolved before being passed to test functions (both sync and async). Test functions receive the yielded value from async fixtures, not coroutines, so fixture parameters should never be awaited. A sync test function can use async fixtures without any special handling.
Applied to files:
tests/seed/catalog/test_product.py
🧬 Code graph analysis (13)
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)tests/seed/conftest.py (1)
context(15-16)tests/e2e/accounts/licensees/conftest.py (1)
licensee_id(15-16)tests/e2e/accounts/conftest.py (2)
account_id(30-31)seller_id(35-36)
tests/seed/catalog/test_product.py (2)
tests/seed/conftest.py (2)
context(15-16)fake_file_factory(35-36)seed/catalog/product.py (1)
init_product(45-65)
tests/seed/accounts/test_accounts.py (1)
seed/accounts/accounts.py (1)
seed_accounts(13-23)
seed/accounts/api_tokens.py (3)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/api_tokens.py (1)
ApiToken(12-13)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)
tests/seed/accounts/test_licensee.py (3)
mpt_api_client/resources/accounts/licensees.py (2)
AsyncLicenseesService(48-58)Licensee(21-22)seed/accounts/licensee.py (4)
build_licensee_data(39-68)get_licensee(18-35)init_licensee(72-91)seed_licensee(95-99)seed/context.py (3)
Context(12-33)get_resource(19-29)set_resource(31-33)
seed/accounts/buyer.py (3)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/buyers.py (1)
Buyer(26-27)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)
seed/accounts/module.py (2)
mpt_api_client/resources/accounts/modules.py (1)
Module(11-12)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)
seed/catalog/product.py (5)
tests/seed/catalog/test_product.py (1)
product(17-18)mpt_api_client/resources/catalog/products.py (1)
Product(53-54)tests/seed/conftest.py (1)
context(15-16)seed/context.py (1)
set_resource(31-33)mpt_api_client/models/model.py (1)
id(119-121)
seed/accounts/seller.py (5)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/sellers.py (1)
Seller(13-14)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)tests/seed/accounts/test_seller.py (1)
seller(11-12)mpt_api_client/models/model.py (1)
id(119-121)
tests/seed/accounts/test_buyer.py (2)
seed/accounts/buyer.py (4)
build_buyer_data(39-61)get_buyer(18-35)init_buyer(65-85)seed_buyer(89-93)seed/context.py (2)
Context(12-33)get_resource(19-29)
tests/seed/accounts/test_user_group.py (2)
seed/accounts/user_group.py (4)
build_user_group_data(37-52)get_user_group(16-33)init_user_group(56-76)seed_user_group(80-85)seed/context.py (2)
Context(12-33)get_resource(19-29)
seed/accounts/accounts.py (6)
seed/accounts/api_tokens.py (1)
seed_api_token(71-75)seed/accounts/buyer.py (1)
seed_buyer(89-93)seed/accounts/licensee.py (1)
seed_licensee(95-99)seed/accounts/module.py (1)
seed_module(61-72)seed/accounts/seller.py (1)
seed_seller(78-82)seed/accounts/user_group.py (1)
seed_user_group(80-85)
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/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)mpt_api_client/models/model.py (1)
id(119-121)
🪛 Ruff (0.14.7)
seed/accounts/licensee.py
45-45: Avoid specifying long messages outside the exception class
(TRY003)
89-89: Avoid specifying long messages outside the exception class
(TRY003)
tests/seed/accounts/test_accounts.py
6-6: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
seed/accounts/buyer.py
43-43: Avoid specifying long messages outside the exception class
(TRY003)
83-83: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/module.py
69-69: Avoid specifying long messages outside the exception class
(TRY003)
seed/catalog/product.py
63-63: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/accounts.py
13-13: Unused noqa directive (unknown: WPS217)
Remove unused noqa directive
(RUF100)
seed/accounts/user_group.py
43-43: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (6)
tests/e2e/accounts/sellers/conftest.py (1)
18-25: City change in seller_factory address looks goodSwitching the fixture city to
"Los Angeles"is fine and keeps the structure of the seeded seller data unchanged. Just ensure any assertions or external seed data that previously depended on the old city value remain consistent (e.g., string comparisons or filters by city).seed/accounts/buyer.py (2)
64-85: LGTM!The function properly validates the creation result with
isinstancecheck and raises an explicit exception on failure. The re-fetch after creation ensures the full buyer object is retrieved.
88-93: LGTM!The seed function follows the established pattern, delegating to
init_buyer()with dependency injection handling the defaults.seed/accounts/api_tokens.py (1)
70-75: LGTM!The seed function follows the established pattern.
seed/accounts/user_group.py (2)
55-76: LGTM with note on inconsistency.The function properly validates the creation result. However, note that this returns
Noneon failure (line 76) whileinit_buyerraisesValueError. Consider aligning the error handling approach across modules for consistency.
79-85: LGTM!The seed function follows the established pattern with return type aligned to
init_user_group.
robcsegal
force-pushed
the
MPT-15443-seed-data-required-for-accounts-e-2-e-tests
branch
from
6edaaa8 to
6ba1eef
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
tests/seed/accounts/test_buyer.py (2)
36-60: Test will fail: missingCLIENT_ACCOUNT_IDenvironment variable and seller ID.Per
seed/accounts/buyer.pylines 41-44,build_buyer_datareadsCLIENT_ACCOUNT_IDfromos.getenv()andaccounts.seller.idfrom context. The test setscontext["accounts.account.id"](unused key) but never sets the environment variable or seller ID.Apply this diff to fix:
-def test_build_buyer_data(context: Context) -> None: - context["accounts.account.id"] = "ACC-1086-6867" +def test_build_buyer_data(context: Context, monkeypatch) -> None: + monkeypatch.setenv("CLIENT_ACCOUNT_ID", "ACC-1086-6867") + context["accounts.seller.id"] = "SEL-123" expected_data = { "name": "E2E Seeded Buyer", "account": { "id": "ACC-1086-6867", }, + "sellers": [{"id": "SEL-123"}], "contact": { "firstName": "first", "lastName": "last", "email": "created.buyer@example.com", }, "address": { "addressLine1": "123 Main St", "city": "Los Angeles", "state": "CA", "postCode": "12345", "country": "US", }, - "sellers": [{"id": ""}], }
76-92: Test setup will fail due to eager evaluation ofbuild_buyer_data(context).Line 87 evaluates
build_buyer_data(context)during patch setup, which will raiseValueErrorbecauseCLIENT_ACCOUNT_IDis not set andaccounts.seller.idis missing from context.Apply this diff to fix:
async def test_init_buyer_create_new( - context: Context, operations_client, buyers_service, buyer + context: Context, operations_client, buyers_service, buyer, monkeypatch ) -> None: buyers_service.create.return_value = buyer operations_client.accounts.buyers = buyers_service - context["accounts.account.id"] = "ACC-1086-6867" + monkeypatch.setenv("CLIENT_ACCOUNT_ID", "ACC-1086-6867") + context["accounts.seller.id"] = "SEL-123" buyers_service.get.return_value = buyer with ( patch("seed.accounts.buyer.get_buyer", return_value=None), patch("pathlib.Path.open", return_value=AsyncMock()), - patch("seed.accounts.buyer.build_buyer_data", return_value=build_buyer_data(context)), + patch("seed.accounts.buyer.build_buyer_data", return_value={ + "name": "E2E Seeded Buyer", + "account": {"id": "ACC-1086-6867"}, + "sellers": [{"id": "SEL-123"}], + }), ): result = await init_buyer(context=context, mpt_operations=operations_client)seed/accounts/buyer.py (1)
30-34: Add type validation after API fetch.When fetching buyer from API on line 31, the result is stored in context without verifying it's a valid
Buyerinstance. If the API returns an error or unexpected type, this could corrupt the context.Apply this diff:
if not isinstance(buyer, Buyer): buyer = await mpt_operations.accounts.buyers.get(buyer_id) + if not isinstance(buyer, Buyer): + return None context.set_resource("accounts.buyer", buyer) context["accounts.buyer.id"] = buyer.id return buyerseed/accounts/api_tokens.py (1)
35-48: Validate required configuration before building payload.The function reads
CLIENT_ACCOUNT_IDfrom the environment (line 40) andmodule_idfrom context (line 41) without validating that they contain valid values. IfCLIENT_ACCOUNT_IDis unset,account_idwill beNone. If the module hasn't been seeded yet,module_idwill be an empty string. This leads to confusing API errors rather than clear, actionable failures.Apply this diff:
@inject def build_api_token_data( context: Context = DEFAULT_CONTEXT, ) -> dict[str, object]: """Get API token data dictionary for creation.""" account_id = os.getenv("CLIENT_ACCOUNT_ID") module_id = context.get_string("accounts.module.id") + if not account_id: + raise ValueError("CLIENT_ACCOUNT_ID environment variable is required") # noqa: TRY003 + if not module_id: + raise ValueError("accounts.module.id missing from context; seed module first") # noqa: TRY003 return { "account": {"id": account_id}, "name": "E2E Seeded API Token",Based on learnings, this validation pattern is used in
user_group.pyat lines 41-43 and should be consistently applied here.seed/accounts/user_group.py (1)
28-32: Add type validation after API fetch.After fetching the user group from the API (line 29), the code assumes the result is a valid
UserGroupand immediately accesses its.idattribute (line 31). If the API returns an unexpected type, this will cause anAttributeErrorat runtime.Based on learnings from past reviews and the pattern used in
init_user_group(line 69), add validation:Apply this diff:
if not isinstance(user_group, UserGroup): user_group = await mpt_operations.accounts.user_groups.get(user_group_id) + if not isinstance(user_group, UserGroup): + logger.warning("API returned non-UserGroup type for id: %s", user_group_id) + return None context.set_resource("accounts.user_group", user_group) context["accounts.user_group.id"] = user_group.id return user_group
🧹 Nitpick comments (9)
tests/seed/accounts/test_licensee.py (1)
110-130: Consider consolidating duplicate tests.
test_init_licenseeandtest_init_licensee_create_newappear to test the same creation path (both patchget_licenseeto returnNoneand verifycreateis called). Consider either removing one or differentiating their test scenarios.seed/accounts/seller.py (1)
1-1: Remove the mypy directive on line 1 if it's no longer needed.The
# mypy: disable-error-code=unreachabledirective appears unnecessary. The code has no unreachable paths:get_sellerhas reachable early returns and a final return, whileinit_sellerproperly handles all branches with appropriate returns for both success and failure cases. Consider removing the directive and running mypy to confirm it no longer reports unreachable code errors.tests/seed/accounts/test_accounts.py (1)
6-6: Remove unusednoqadirective.The static analyzer correctly flagged that
WPS210is not recognized. This directive can be safely removed.Apply this diff:
-async def test_seed_accounts() -> None: # noqa: WPS210 +async def test_seed_accounts() -> None:seed/accounts/accounts.py (1)
13-13: Remove unusednoqadirective.The
WPS217code is not recognized by the current linter configuration.Apply this diff:
-async def seed_accounts() -> None: # noqa: WPS217 +async def seed_accounts() -> None:seed/accounts/module.py (1)
69-69: Consider addressing the TRY003 linting hint.The static analyzer flagged the long exception message. While not critical, you can either extract the message to a module-level constant or add a suppression comment if the explicit message is preferred.
Apply this diff to suppress:
- raise ValueError("Could not seed module: no valid Module found.") + raise ValueError("Could not seed module: no valid Module found.") # noqa: TRY003Or extract to a constant:
+MODULE_SEED_ERROR = "Could not seed module: no valid Module found." + @inject async def seed_module() -> Module: """Seed module.""" logger.debug("Seeding module ...") existing_module = await get_module() if existing_module is None: refreshed = await refresh_module() logger.debug("Seeding module completed.") if refreshed is None: - raise ValueError("Could not seed module: no valid Module found.") + raise ValueError(MODULE_SEED_ERROR)seed/accounts/user_group.py (2)
1-1: Consider removing the mypy suppression.The return types now correctly reflect that functions can return
None(lines 19, 59, 80), so theunreachablesuppression may no longer be necessary.Apply this diff:
-# mypy: disable-error-code=unreachable import loggingThen verify with
mypythat no type errors are reported.
43-43: Consider addressing the TRY003 linting hint.Similar to
module.py, the long exception message is flagged. This is a style issue, not a defect.Apply this diff to suppress:
- raise ValueError("CLIENT_ACCOUNT_ID environment variable is required") + raise ValueError("CLIENT_ACCOUNT_ID environment variable is required") # noqa: TRY003seed/catalog/product.py (2)
50-65: init_product get‑or‑create flow looks good; consider forwarding dependencies into get_productThe new
get or createbehavior is sound: ifget_product()finds an existing product, you now log and return it; otherwise you create, persist incontext, and return the createdProduct. That matches the seeding use case and avoids unnecessary re‑creation.One small refactor: since
init_productalready acceptscontextandmpt_vendor, callingget_product()without arguments means any explicit arguments passed toinit_productwon’t be used byget_product. To keep things consistent and more testable, you could forward them:- product = await get_product() + product = await get_product(context=context, mpt_vendor=mpt_vendor)This preserves current behavior under DI but avoids surprises if
init_productis ever called with non‑default parameters.
53-63: Minor nits: deduplicate failure message and consider more idiomatic Path usageTwo tiny cleanups you might consider (purely optional):
- You log
"Product creation failed"and then raiseValueError("Product creation failed"). To avoid duplicating the literal, you could introduce a small module‑level constant (or a dedicated exception) and reuse it in both places.with pathlib.Path.open(icon, "rb") as icon_file:works, butwith icon.open("rb") as icon_file:is a bit more idiomatic and reads more clearly.Not critical, but they slightly improve readability and future maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
seed/accounts/logo.pngis excluded by!**/*.png
📒 Files selected for processing (24)
pyproject.toml(2 hunks)seed/accounts/accounts.py(1 hunks)seed/accounts/api_tokens.py(1 hunks)seed/accounts/buyer.py(1 hunks)seed/accounts/licensee.py(1 hunks)seed/accounts/module.py(1 hunks)seed/accounts/seller.py(1 hunks)seed/accounts/user_group.py(1 hunks)seed/catalog/product.py(1 hunks)seed/container.py(1 hunks)seed/seed_api.py(2 hunks)tests/e2e/accounts/buyers/conftest.py(1 hunks)tests/e2e/accounts/licensees/conftest.py(1 hunks)tests/e2e/accounts/sellers/conftest.py(1 hunks)tests/seed/accounts/test_accounts.py(1 hunks)tests/seed/accounts/test_api_token.py(1 hunks)tests/seed/accounts/test_buyer.py(1 hunks)tests/seed/accounts/test_licensee.py(1 hunks)tests/seed/accounts/test_module.py(1 hunks)tests/seed/accounts/test_seller.py(1 hunks)tests/seed/accounts/test_user_group.py(1 hunks)tests/seed/catalog/test_product.py(1 hunks)tests/seed/conftest.py(3 hunks)tests/seed/test_seed_api.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- tests/e2e/accounts/licensees/conftest.py
- tests/seed/test_seed_api.py
- tests/seed/accounts/test_seller.py
- pyproject.toml
- tests/seed/accounts/test_api_token.py
- seed/container.py
- tests/seed/catalog/test_product.py
- tests/seed/conftest.py
- seed/seed_api.py
🧰 Additional context used
🧬 Code graph analysis (12)
tests/seed/accounts/test_module.py (5)
mpt_api_client/resources/accounts/modules.py (2)
AsyncModulesService(29-32)Module(11-12)seed/accounts/module.py (3)
get_module(15-32)refresh_module(36-57)seed_module(61-72)tests/seed/conftest.py (2)
context(15-16)operations_client(25-26)seed/context.py (2)
Context(12-33)get_resource(19-29)mpt_api_client/models/model.py (1)
id(119-121)
seed/accounts/user_group.py (3)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/user_groups.py (1)
UserGroup(11-12)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)
seed/accounts/api_tokens.py (3)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/api_tokens.py (1)
ApiToken(12-13)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)
seed/accounts/licensee.py (3)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/licensees.py (1)
Licensee(21-22)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)
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/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)mpt_api_client/models/model.py (1)
id(119-121)
tests/seed/accounts/test_licensee.py (3)
mpt_api_client/resources/accounts/licensees.py (2)
AsyncLicenseesService(48-58)Licensee(21-22)seed/accounts/licensee.py (4)
build_licensee_data(39-68)get_licensee(18-35)init_licensee(72-91)seed_licensee(95-99)seed/context.py (3)
Context(12-33)get_resource(19-29)set_resource(31-33)
tests/seed/accounts/test_buyer.py (3)
seed/accounts/buyer.py (4)
build_buyer_data(39-61)get_buyer(18-35)init_buyer(65-85)seed_buyer(89-93)tests/seed/conftest.py (1)
context(15-16)seed/context.py (2)
Context(12-33)get_resource(19-29)
seed/accounts/module.py (4)
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/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)
tests/seed/accounts/test_user_group.py (4)
mpt_api_client/resources/accounts/user_groups.py (2)
AsyncUserGroupsService(32-38)UserGroup(11-12)seed/accounts/user_group.py (4)
build_user_group_data(37-52)get_user_group(16-33)init_user_group(56-76)seed_user_group(80-85)seed/context.py (2)
Context(12-33)get_resource(19-29)mpt_api_client/models/model.py (1)
id(119-121)
seed/accounts/accounts.py (6)
seed/accounts/api_tokens.py (1)
seed_api_token(71-75)seed/accounts/buyer.py (1)
seed_buyer(89-93)seed/accounts/licensee.py (1)
seed_licensee(95-99)seed/accounts/module.py (1)
seed_module(61-72)seed/accounts/seller.py (1)
seed_seller(78-82)seed/accounts/user_group.py (1)
seed_user_group(80-85)
seed/catalog/product.py (5)
tests/seed/catalog/test_product.py (1)
product(17-18)tests/e2e/conftest.py (1)
logger(63-64)mpt_api_client/resources/catalog/products.py (1)
Product(53-54)seed/context.py (1)
set_resource(31-33)mpt_api_client/models/model.py (1)
id(119-121)
tests/seed/accounts/test_accounts.py (1)
seed/accounts/accounts.py (1)
seed_accounts(13-23)
🪛 Ruff (0.14.7)
seed/accounts/user_group.py
43-43: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/licensee.py
45-45: Avoid specifying long messages outside the exception class
(TRY003)
89-89: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/buyer.py
43-43: Avoid specifying long messages outside the exception class
(TRY003)
83-83: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/module.py
69-69: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/accounts.py
13-13: Unused noqa directive (unknown: WPS217)
Remove unused noqa directive
(RUF100)
seed/catalog/product.py
63-63: Avoid specifying long messages outside the exception class
(TRY003)
tests/seed/accounts/test_accounts.py
6-6: 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 (14)
tests/e2e/accounts/sellers/conftest.py (1)
20-20: LGTM! Test data updated to align with seeded e2e data.The change from "Anytown" to "Los Angeles" aligns with the PR objective of adding seeded accounts e2e data. The fixture now uses realistic, consistent test values throughout (including address line, state, postal code, and country). No other files reference "Anytown", so this change is isolated and safe.
tests/e2e/accounts/buyers/conftest.py (1)
30-36: LGTM! Address data now consistent with seeded accounts.The city update to "Los Angeles" aligns with the buyer seed data in
seed/accounts/buyer.py, ensuring e2e test fixtures match the seeded account structure.tests/seed/accounts/test_buyer.py (1)
63-73: LGTM!The test correctly patches
get_buyerto return an existing buyer and verifies thatcreateis not called when a buyer already exists.seed/accounts/seller.py (1)
55-74: LGTM! Type validation and DI parameter forwarding now correct.The
init_sellerfunction correctly:
- Forwards
contextandmpt_operationstoget_seller(line 61)- Validates the API response type with
isinstance(created, Seller)(line 66)- Returns
Noneon failure, matching the declared return typePast review issues have been addressed.
seed/accounts/licensee.py (1)
71-91: LGTM! Proper DI forwarding and type validation.The
init_licenseefunction correctly:
- Forwards
contextandmpt_clienttoget_licenseeandbuild_licensee_data- Validates the API response type with
isinstance(created, Licensee)- Uses proper error handling with clear logging
Past review issues have been addressed.
tests/seed/accounts/test_licensee.py (1)
85-107: LGTM! Test setup now correctly configures environment before patch evaluation.The test properly sets the environment variable and context values (lines 91-95) before entering the
withblock, ensuringbuild_licensee_data(context)at line 101 has access to required values when evaluated during patch setup.seed/accounts/buyer.py (1)
64-85: LGTM! Proper DI forwarding and type validation.The
init_buyerfunction correctly:
- Forwards
contextandmpt_operationstoget_buyerandbuild_buyer_data(lines 70, 72)- Validates the API response type with
isinstance(created, Buyer)(line 76)- Re-fetches the buyer after creation to get the complete object (line 77)
- Uses proper error handling with clear logging
Past review issues regarding DI parameter forwarding have been addressed.
tests/seed/accounts/test_module.py (1)
1-102: LGTM!All previously identified issues have been resolved:
- AsyncMock is correctly used for patching async functions (lines 61, 81, 94)
- Exception chaining is properly implemented in
DummyAsyncIterator.__anext__(line 21)- Test coverage for
get_module,refresh_module, andseed_moduleis comprehensivetests/seed/accounts/test_accounts.py (1)
1-30: LGTM!The test comprehensively validates that
seed_accounts()orchestrates all six seeding functions correctly.seed/accounts/accounts.py (1)
13-23: LGTM!The orchestration sequentially seeds all account-related entities with appropriate logging.
tests/seed/accounts/test_user_group.py (1)
1-89: LGTM!All previously identified issues have been resolved:
CLIENT_ACCOUNT_IDenvironment variable is properly set usingmonkeypatchin tests (lines 48, 65)- Duplicate test has been removed
- AsyncMock is correctly used for patching async functions (line 85)
- Test coverage comprehensively validates user group seeding workflows
seed/accounts/module.py (1)
14-72: LGTM!The module seeding implementation is well-structured:
- Proper dependency injection with
@injectdecorator- Context-aware resource management with appropriate caching
- Fallback to API fetch when context resource is unavailable
- Type validation using
isinstancechecks- Clear separation of concerns: get, refresh, and seed operations
seed/accounts/user_group.py (1)
36-52: LGTM!The user group seeding implementation shows good practices:
- Environment variable validation with clear error messages (lines 41-43)
- Type validation for created resources (line 69)
- Proper dependency injection parameter forwarding (lines 61, 67)
- Appropriate logging for creation vs. reuse scenarios
Also applies to: 55-85
seed/accounts/api_tokens.py (1)
51-67: LGTM!The parameter forwarding has been correctly implemented:
contextandmpt_opsare properly passed toget_api_token(line 57)contextis correctly forwarded tobuild_api_token_data(line 60)- Logging distinguishes between creation and discovery scenarios
robcsegal
force-pushed
the
MPT-15443-seed-data-required-for-accounts-e-2-e-tests
branch
from
6ba1eef to
43c79cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (8)
tests/seed/catalog/test_product.py (1)
49-52: Remove redundantpathlib.Path.openpatch in test setup.Lines 51–52 patch
pathlib.Path.opentwice with the same lambda; the second patch just overrides the first and adds noise without changing behavior. A single patch is sufficient and clearer.You can simplify to:
with ( patch("seed.catalog.product.get_product", new_callable=AsyncMock) as mock_get_product, patch("seed.catalog.product.icon", new=MagicMock()), - patch("pathlib.Path.open", new=lambda *_, **_unused: fake_file_factory()), - patch("pathlib.Path.open", new=lambda *_, **_unused: fake_file_factory()), + patch("pathlib.Path.open", new=lambda *_, **__: fake_file_factory()), ):This keeps the behavior while avoiding redundant patching and matches the earlier recommendation.
seed/accounts/module.py (1)
61-71: Tightenseed_modulecompletion logging and fix TRY003 on the error message
logger.debug("Seeding module completed.")executes before checkingrefreshed is None, so failures still log as “completed”.- Ruff flags the long
ValueErrormessage (TRY003).You can fix both by introducing a constant and moving the debug log after the success check:
@@ -logger = logging.getLogger(__name__) +logger = logging.getLogger(__name__) + +MODULE_SEED_ERROR = "Could not seed module: no valid Module found." @@ async def seed_module() -> Module: @@ logger.debug("Seeding module ...") existing_module = await get_module() if existing_module is None: refreshed = await refresh_module() - logger.debug("Seeding module completed.") - if refreshed is None: - raise ValueError("Could not seed module: no valid Module found.") - return refreshed - logger.debug("Seeding module completed.") - return existing_module + if refreshed is None: + raise ValueError(MODULE_SEED_ERROR) + logger.debug("Seeding module completed.") + return refreshed + logger.debug("Seeding module completed.") + return existing_moduleThis keeps logs accurate and satisfies Ruff without suppressing TRY003.
seed/accounts/user_group.py (2)
1-1: Remove broad mypyunreachablesuppressionNow that
init_user_groupandseed_user_grouphonestly returnUserGroup | None, the top‑level# mypy: disable-error-code=unreachableis likely no longer needed and may hide future issues. Consider dropping it and addressing any remaining mypy warnings more locally if they appear.-# mypy: disable-error-code=unreachable
24-32: Add type validation after fetching user group from the API
mpt_operations.accounts.user_groups.get(user_group_id)is assumed to return aUserGroup, but if it doesn’t,context.set_resource(...)anduser_group.idcan fail.Align this with the defensive pattern used elsewhere:
except ValueError: user_group = None if not isinstance(user_group, UserGroup): user_group = await mpt_operations.accounts.user_groups.get(user_group_id) - context.set_resource("accounts.user_group", user_group) - context["accounts.user_group.id"] = user_group.id - return user_group + if not isinstance(user_group, UserGroup): + logger.warning( + "accounts.user_groups.get returned unexpected type for id %s", + user_group_id, + ) + return None + context.set_resource("accounts.user_group", user_group) + context["accounts.user_group.id"] = user_group.id + return user_groupseed/accounts/seller.py (1)
1-1: Consider removing global mypyunreachablesuppressionWith
init_sellernow returningSeller | Noneand handling the failure path explicitly, the top‑level# mypy: disable-error-code=unreachableis likely no longer required and can hide future real issues. Removing it should be safe once mypy runs cleanly:-# mypy: disable-error-code=unreachableseed/accounts/api_tokens.py (2)
23-31: Guard against non-ApiTokenvalues after APIgetIf
mpt_ops.accounts.api_tokens.get(api_token_id)returns something other than anApiToken, the subsequentcontext.set_resource(...)andapi_token.idaccess will fail.A defensive pattern similar to other seeders would help:
except ValueError: api_token = None if not isinstance(api_token, ApiToken): api_token = await mpt_ops.accounts.api_tokens.get(api_token_id) - context.set_resource("accounts.api_token", api_token) - context["accounts.api_token.id"] = api_token.id - return api_token + if not isinstance(api_token, ApiToken): + logger.warning( + "accounts.api_tokens.get returned unexpected type for id %s", + api_token_id, + ) + return None + context.set_resource("accounts.api_token", api_token) + context["accounts.api_token.id"] = api_token.id + return api_token
36-48: ValidateCLIENT_ACCOUNT_IDand module id before building API token payload
build_api_token_datacurrently builds a payload even when:
CLIENT_ACCOUNT_IDis unset (account_idbecomesNone), oraccounts.module.idis missing from the context (empty string).That can lead to confusing downstream API errors. Failing fast here is easier to diagnose:
def build_api_token_data( context: Context = DEFAULT_CONTEXT, ) -> dict[str, object]: """Get API token data dictionary for creation.""" account_id = os.getenv("CLIENT_ACCOUNT_ID") module_id = context.get_string("accounts.module.id") + if not account_id: + raise ValueError("CLIENT_ACCOUNT_ID environment variable is not set.") + if not module_id: + raise ValueError( + "accounts.module.id missing from context; seed module before API token." + ) return { "account": {"id": account_id}, @@ "modules": [{"id": module_id}], }seed/accounts/buyer.py (1)
17-35: Add type validation after API fetch.When fetching the buyer from API (line 31), the result is stored in context without verifying it's a valid
Buyerinstance. If the API returns an error or unexpected type, this could corrupt the context and cause downstream failures.if not isinstance(buyer, Buyer): buyer = await mpt_operations.accounts.buyers.get(buyer_id) + if not isinstance(buyer, Buyer): + logger.warning("Failed to fetch buyer: %s", buyer_id) + return None context.set_resource("accounts.buyer", buyer) context["accounts.buyer.id"] = buyer.id return buyer
🧹 Nitpick comments (8)
seed/catalog/product.py (1)
45-52: Threadcontextandmpt_vendorthrough toget_productfor consistency.At Line 50,
init_productignores itscontextandmpt_vendorparameters when callingget_product, relying on the defaults instead. This is surprising and can break callers that intentionally pass a custom context or vendor client, since lookup happens against the default context/client while creation uses the provided ones.Recommend wiring these through:
async def init_product( context: Context = DEFAULT_CONTEXT, mpt_vendor: AsyncMPTClient = DEFAULT_MPT_VENDOR, ) -> Product: """Get or create product.""" - product = await get_product() + product = await get_product(context=context, mpt_vendor=mpt_vendor)This keeps existing defaulted behavior intact while making the function’s parameters actually respected.
seed/accounts/module.py (1)
40-41:refresh_moduledocstring doesn’t match behaviorThe docstring says “always fetch”, but the implementation returns the cached module without any API call when
get_modulefinds one. Either update the docstring to describe the actual “fetch-if-missing, otherwise reuse cached” behavior or adjust the implementation to always re-fetch and refresh the context.seed/accounts/user_group.py (1)
41-43: Optional: ExtractCLIENT_ACCOUNT_IDerror message to a constant (TRY003)Ruff flags the inline
ValueErrormessage here (TRY003). If you want a clean lint run without suppression, you can move the string to a constant:-import logging +import logging @@ -logger = logging.getLogger(__name__) +logger = logging.getLogger(__name__) + +MISSING_CLIENT_ACCOUNT_ID_ERROR = ( + "CLIENT_ACCOUNT_ID environment variable is required" +) @@ - account_id = os.getenv("CLIENT_ACCOUNT_ID") - if not account_id: - raise ValueError("CLIENT_ACCOUNT_ID environment variable is required") + account_id = os.getenv("CLIENT_ACCOUNT_ID") + if not account_id: + raise ValueError(MISSING_CLIENT_ACCOUNT_ID_ERROR)Alternatively, if your team prefers, add
# noqa: TRY003to this line instead.seed/accounts/api_tokens.py (1)
51-67: Optionally validate type aftercreatefor symmetry with other seedersUnlike
init_seller/init_user_group,init_api_tokenassumescreate(...)always returns a validApiToken. If the API ever responds with an error model,api_token.idwill raise.For consistency and robustness, you might mirror the pattern used elsewhere:
if api_token is None: logger.debug("Creating API token ...") api_token_data = build_api_token_data(context=context) - api_token = await mpt_ops.accounts.api_tokens.create(api_token_data) - context.set_resource("accounts.api_token", api_token) - context["accounts.api_token.id"] = api_token.id - logger.info("API token created: %s", api_token.id) + created = await mpt_ops.accounts.api_tokens.create(api_token_data) + if isinstance(created, ApiToken): + context.set_resource("accounts.api_token", created) + context["accounts.api_token.id"] = created.id + logger.info("API token created: %s", created.id) + return created + logger.warning("API token creation failed") + raise ValueError("API token creation failed") @@ - return api_token + return api_tokentests/seed/accounts/test_licensee.py (1)
85-130: Consider consolidating or differentiating these tests.
test_init_licenseeandtest_init_licensee_create_neware functionally identical—both mockget_licenseeto returnNoneand verify the create path. Either remove the duplicate or differentiate them to test distinct scenarios (e.g., one for existing licensee, one for new creation).To test the "existing licensee" path:
async def test_init_licensee( - context: Context, client_client, licensees_service, licensee, monkeypatch + context: Context, client_client, licensees_service, licensee ) -> None: - licensees_service.create.return_value = licensee + licensees_service.get.return_value = licensee client_client.accounts.licensees = licensees_service - monkeypatch.setenv("CLIENT_ACCOUNT_ID", "ACC-1086-6867") - context["accounts.user_group.id"] = "UG-123" - context.set_resource("accounts.user_group", Model({"id": "UG-123"})) - context["accounts.seller.id"] = "SEL-123" - context["accounts.buyer.id"] = "BUY-123" + context["accounts.licensee.id"] = "LIC-123" with ( - patch("seed.accounts.licensee.get_licensee", return_value=None), - patch("pathlib.Path.open", return_value=AsyncMock()), - patch( - "seed.accounts.licensee.build_licensee_data", - return_value=build_licensee_data(context), - ), + patch("seed.accounts.licensee.get_licensee", return_value=licensee), ): result = await init_licensee(context=context, mpt_client=client_client) assert result == licensee - licensees_service.create.assert_called_once() + licensees_service.create.assert_not_called()seed/accounts/licensee.py (1)
71-91: Use idiomatic Path method call.Line 81 uses
pathlib.Path.open(icon, "rb")which works but is unconventional. The idiomatic form isicon.open("rb")sinceiconis already aPathobject.- with pathlib.Path.open(icon, "rb") as icon_file: + with icon.open("rb") as icon_file:seed/accounts/buyer.py (2)
64-85: Inconsistent pattern: extra API call after creation.Line 77 refetches the buyer after creation (
await mpt_operations.accounts.buyers.get(created.id)), whilelicensee.pyuses thecreatedobject directly. This adds an extra API call and is inconsistent.Either align with the licensee pattern:
if isinstance(created, Buyer): - buyer = await mpt_operations.accounts.buyers.get(created.id) - context.set_resource("accounts.buyer", buyer) - context["accounts.buyer.id"] = buyer.id - logger.info("Buyer created: %s", buyer.id) - return buyer + context.set_resource("accounts.buyer", created) + context["accounts.buyer.id"] = created.id + logger.info("Buyer created: %s", created.id) + return createdOr if the refetch is intentional (e.g., to get complete data not returned by create), add a comment explaining why and apply the same pattern to
licensee.pyfor consistency.
74-74: Use idiomatic Path method call.Same issue as in
licensee.py: useicon.open("rb")instead ofpathlib.Path.open(icon, "rb").- with pathlib.Path.open(icon, "rb") as icon_file: + with icon.open("rb") as icon_file:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
seed/accounts/logo.pngis excluded by!**/*.png
📒 Files selected for processing (24)
pyproject.toml(2 hunks)seed/accounts/accounts.py(1 hunks)seed/accounts/api_tokens.py(1 hunks)seed/accounts/buyer.py(1 hunks)seed/accounts/licensee.py(1 hunks)seed/accounts/module.py(1 hunks)seed/accounts/seller.py(1 hunks)seed/accounts/user_group.py(1 hunks)seed/catalog/product.py(1 hunks)seed/container.py(1 hunks)seed/seed_api.py(2 hunks)tests/e2e/accounts/buyers/conftest.py(1 hunks)tests/e2e/accounts/licensees/conftest.py(1 hunks)tests/e2e/accounts/sellers/conftest.py(1 hunks)tests/seed/accounts/test_accounts.py(1 hunks)tests/seed/accounts/test_api_token.py(1 hunks)tests/seed/accounts/test_buyer.py(1 hunks)tests/seed/accounts/test_licensee.py(1 hunks)tests/seed/accounts/test_module.py(1 hunks)tests/seed/accounts/test_seller.py(1 hunks)tests/seed/accounts/test_user_group.py(1 hunks)tests/seed/catalog/test_product.py(1 hunks)tests/seed/conftest.py(3 hunks)tests/seed/test_seed_api.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- tests/e2e/accounts/buyers/conftest.py
- tests/e2e/accounts/licensees/conftest.py
- pyproject.toml
- tests/seed/conftest.py
- seed/seed_api.py
- tests/seed/accounts/test_api_token.py
- tests/seed/accounts/test_user_group.py
- tests/seed/accounts/test_seller.py
- seed/container.py
- tests/e2e/accounts/sellers/conftest.py
- tests/seed/accounts/test_buyer.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-27T00:05:54.701Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/accounts/accounts_users/test_async_accounts_users.py:0-0
Timestamp: 2025-11-27T00:05:54.701Z
Learning: In pytest-asyncio, async fixtures are automatically resolved before being passed to test functions (both sync and async). Test functions receive the yielded value from async fixtures, not coroutines, so fixture parameters should never be awaited. A sync test function can use async fixtures without any special handling.
Applied to files:
tests/seed/catalog/test_product.py
🧬 Code graph analysis (9)
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/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)mpt_api_client/models/model.py (1)
id(119-121)
tests/seed/accounts/test_accounts.py (1)
seed/accounts/accounts.py (1)
seed_accounts(13-23)
tests/seed/catalog/test_product.py (4)
tests/seed/conftest.py (3)
context(15-16)vendor_client(20-21)fake_file_factory(35-36)tests/seed/catalog/test_item_group.py (1)
vendor_client(29-30)tests/seed/catalog/test_product_parameters.py (1)
vendor_client(30-31)seed/catalog/product.py (1)
init_product(45-65)
seed/accounts/buyer.py (3)
mpt_api_client/resources/accounts/buyers.py (1)
Buyer(26-27)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)mpt_api_client/models/model.py (1)
id(119-121)
seed/accounts/api_tokens.py (4)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/api_tokens.py (1)
ApiToken(12-13)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)tests/seed/accounts/test_api_token.py (1)
api_token(15-16)
seed/accounts/licensee.py (5)
mpt_api_client/resources/accounts/licensees.py (1)
Licensee(21-22)tests/seed/conftest.py (1)
context(15-16)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)tests/seed/accounts/test_licensee.py (1)
licensee(12-30)mpt_api_client/models/model.py (1)
id(119-121)
seed/accounts/accounts.py (6)
seed/accounts/api_tokens.py (1)
seed_api_token(71-75)seed/accounts/buyer.py (1)
seed_buyer(89-93)seed/accounts/licensee.py (1)
seed_licensee(95-99)seed/accounts/module.py (1)
seed_module(61-72)seed/accounts/seller.py (1)
seed_seller(78-82)seed/accounts/user_group.py (1)
seed_user_group(80-85)
seed/catalog/product.py (4)
tests/seed/catalog/test_product.py (1)
product(17-18)mpt_api_client/resources/catalog/products.py (1)
Product(53-54)seed/context.py (1)
set_resource(31-33)mpt_api_client/models/model.py (1)
id(119-121)
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)tests/seed/conftest.py (1)
context(15-16)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)
🪛 Ruff (0.14.7)
tests/seed/accounts/test_accounts.py
6-6: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
seed/accounts/buyer.py
43-43: Avoid specifying long messages outside the exception class
(TRY003)
83-83: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/licensee.py
45-45: Avoid specifying long messages outside the exception class
(TRY003)
89-89: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/accounts.py
13-13: Unused noqa directive (unknown: WPS217)
Remove unused noqa directive
(RUF100)
seed/catalog/product.py
63-63: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/module.py
69-69: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/user_group.py
43-43: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (11)
seed/accounts/user_group.py (1)
79-85:seed_user_groupcontract looks good
seed_user_groupsimply defers toinit_user_groupand returns the resultingUserGroup | Nonewith clear start/end logging. This matches the pattern used by other account seeders and keeps the API simple.tests/seed/test_seed_api.py (1)
23-39: Test update correctly tracksseed_accountsorchestrationPatching
seed.seed_api.seed_accountswithAsyncMockand asserting it’s called once keepstest_seed_api_successaligned with the new seeding flow while still verifying context load/save behavior.tests/seed/accounts/test_module.py (1)
10-101: Module seeding tests cover key async paths correctlyUsing
AsyncMockforget_module/refresh_moduleand theDummyAsyncIteratorfor filtered modules gives good coverage of both cache‑hit and refresh paths, plus theseed_modulefallback behavior.seed/accounts/module.py (1)
24-32: Remove unnecessaryisinstancecheck after API call; address unhandled exceptions insteadThe API's
get()method returnsModuleor raises an exception (TypeError, HTTPError)—it cannot returnNoneor a non-Module type due to the type system andfrom_response()implementation. Theisinstance(module, Module)check on lines 27–28 is redundant and will never fail if no exception occurred.The actual risk is unhandled exceptions from malformed API responses or HTTP errors, which propagate uncaught. Consider wrapping the API call in a try-except block to handle potential failures gracefully:
if not isinstance(module, Module): try: module = await mpt_operations.accounts.modules.get(module_id) except (TypeError, HTTPError): logger.warning("Failed to fetch module %s from API", module_id) return None context.set_resource("accounts.module", module) context["accounts.module.id"] = module.id return moduleLikely an incorrect or invalid review comment.
tests/seed/accounts/test_licensee.py (2)
11-35: LGTM! Fixtures are well-defined.The fixtures provide appropriate mock data for testing the licensee functionality.
133-137: LGTM!The orchestration test correctly verifies that
init_licenseeis invoked.seed/accounts/licensee.py (3)
17-35: LGTM!The
get_licenseefunction correctly handles cache lookup, API fetch fallback, and context updates.
38-68: LGTM!The
build_licensee_datafunction properly validates the environment variable and constructs the payload using context values.
94-99: LGTM!The
seed_licenseefunction correctly orchestrates the seeding with appropriate logging.seed/accounts/buyer.py (2)
38-61: LGTM!The
build_buyer_datafunction properly validates the environment variable and constructs the payload.
88-93: LGTM!The
seed_buyerfunction correctly orchestrates the seeding with appropriate logging.
robcsegal
force-pushed
the
MPT-15443-seed-data-required-for-accounts-e-2-e-tests
branch
from
43c79cb to
e1e9586
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
seed/accounts/seller.py (1)
28-32: Add type validation after APIgetcall.When fetching from the API on line 29, the result is used directly without verifying it's a valid
Sellerinstance. Ifmpt_operations.accounts.sellers.get(seller_id)returns an unexpected type, accessingseller.idon line 31 will fail with an unclearAttributeError.Apply this diff:
if not isinstance(seller, Seller): seller = await mpt_operations.accounts.sellers.get(seller_id) + if not isinstance(seller, Seller): + logger.warning("Failed to fetch seller: %s", seller_id) + return None context.set_resource("accounts.seller", seller) context["accounts.seller.id"] = seller.id return sellerseed/accounts/user_group.py (2)
28-32: Add type validation after APIgetcall.Same issue as in
seller.py: when fetching from API on line 29, the result is used without verifying it's a validUserGroupinstance.if not isinstance(user_group, UserGroup): user_group = await mpt_operations.accounts.user_groups.get(user_group_id) + if not isinstance(user_group, UserGroup): + logger.warning("Failed to fetch user group: %s", user_group_id) + return None context.set_resource("accounts.user_group", user_group) context["accounts.user_group.id"] = user_group.id return user_group
44-52: Validatemodule_idbefore building payload.
context.get_string("accounts.module.id")returns an empty string if the key is missing. This would result in a payload containing"modules": [{"id": ""}], likely causing an API error.module_id = context.get_string("accounts.module.id") + if not module_id: + raise ValueError( + "accounts.module.id missing from context; seed module before user group." + ) return { "name": "E2E Seeded User Group",seed/accounts/module.py (1)
60-71: Resolve Ruff TRY003 onValueErrormessage inseed_moduleRuff still flags the inline error message at Line 69 (TRY003). To satisfy the linter and keep the explicit text, either extract the message into a constant or add a targeted suppression.
For example:
logger = logging.getLogger(__name__) +MODULE_SEED_ERROR_MSG = "Could not seed module: no valid Module found." + ... - if refreshed is None: - raise ValueError("Could not seed module: no valid Module found.") + if refreshed is None: + raise ValueError(MODULE_SEED_ERROR_MSG)(or, if you prefer to keep it inline, append
# noqa: TRY003).seed/accounts/buyer.py (1)
17-35: (Repeat) Validate Buyer type after API fetch before caching
get_buyerrefetches viampt_operations.accounts.buyers.get(buyer_id)and then unconditionally caches/uses the result. If the API ever returnsNoneor a non‑Buyerpayload, you can end up with a bad object in the context or hit anAttributeErroronbuyer.id.Consider adding an explicit type check here, similar to:
if not isinstance(buyer, Buyer): buyer = await mpt_operations.accounts.buyers.get(buyer_id) - context.set_resource("accounts.buyer", buyer) - context["accounts.buyer.id"] = buyer.id - return buyer + if not isinstance(buyer, Buyer): + logger.warning("Buyer %s fetched from API is not a Buyer instance.", buyer_id) + return None + context.set_resource("accounts.buyer", buyer) + context["accounts.buyer.id"] = buyer.id + return buyerThis mirrors the defensive pattern you’re using elsewhere and keeps the context consistent.
seed/accounts/accounts.py (1)
13-23: Remove unused# noqa: WPS217onseed_accountsRuff reports the
# noqa: WPS217onseed_accountsas an unused suppression (RUF100). The function body is straightforward, so the directive is no longer needed:-async def seed_accounts() -> None: # noqa: WPS217 +async def seed_accounts() -> None:This will keep the file lint‑clean without changing behavior.
tests/seed/accounts/test_accounts.py (1)
6-30: Drop unused# noqa: WPS210fromtest_seed_accountsThe
# noqa: WPS210on the test function is reported as an unused suppression (RUF100), and the test body is small enough not to need it. You can safely remove it:-async def test_seed_accounts() -> None: # noqa: WPS210 +async def test_seed_accounts() -> None:No behavioral change, and it keeps lint output cleaner.
seed/accounts/api_tokens.py (2)
27-31: Add type validation after API fetch.After fetching from the API (line 28), the code assumes the result is a valid
ApiTokenand directly accesses its.idattribute (line 30). If the API returnsNoneor an unexpected type, this will raise an AttributeError.Apply this diff to add validation:
if not isinstance(api_token, ApiToken): api_token = await mpt_ops.accounts.api_tokens.get(api_token_id) + if not isinstance(api_token, ApiToken): + logger.warning("API returned non-ApiToken type for id: %s", api_token_id) + return None context.set_resource("accounts.api_token", api_token) context["accounts.api_token.id"] = api_token.id return api_token
35-48: ValidateCLIENT_ACCOUNT_IDand module id before building payload.Right now,
build_api_token_datawill construct a payload even if:
CLIENT_ACCOUNT_IDis not set (line 40,account_idbecomesNone), oraccounts.module.idis missing from context (line 41, empty string default).This can lead to confusing API errors or tokens associated with invalid ids. Fail fast with a clear error instead.
Apply this diff to add validation:
@inject def build_api_token_data( context: Context = DEFAULT_CONTEXT, ) -> dict[str, object]: """Get API token data dictionary for creation.""" account_id = os.getenv("CLIENT_ACCOUNT_ID") module_id = context.get_string("accounts.module.id") + if not account_id: + raise ValueError("CLIENT_ACCOUNT_ID environment variable is not set.") + if not module_id: + raise ValueError("accounts.module.id missing from context; seed module first.") return { "account": {"id": account_id}, "name": "E2E Seeded API Token", "description": "This is a seeded API token for end-to-end testing.", "icon": "", "modules": [{"id": module_id}], }
🧹 Nitpick comments (10)
tests/seed/accounts/test_module.py (1)
34-36: Remove unused helper function.The
async_iterfunction is defined but never used in this test file. It appears to be leftover code.-def async_iter(iter_items): - yield from iter_items - -tests/seed/accounts/test_seller.py (2)
60-63: UseAsyncMockfor patching asyncget_seller.
get_selleris an async function, but it's patched withreturn_value=Nonewhich creates a non-async mock. While this may work in this specific test becauseinit_selleronly checks if the result isNone, it's inconsistent with the pattern used elsewhere (e.g.,test_seed_selleron line 85) and could mask issues in future test modifications.- with patch("seed.accounts.seller.get_seller", return_value=None): + with patch("seed.accounts.seller.get_seller", new_callable=AsyncMock, return_value=None): result = await init_seller(context=context, mpt_operations=operations_client)
71-78: UseAsyncMockfor patching asyncget_seller.Same issue as above—
get_selleris async and should be patched withAsyncMockfor consistency.with ( - patch("seed.accounts.seller.get_seller", return_value=None), + patch("seed.accounts.seller.get_seller", new_callable=AsyncMock, return_value=None), patch( "seed.accounts.seller.build_seller_data", return_value=build_seller_data("test-external-id"), ), ):seed/accounts/user_group.py (1)
1-1: Consider removing the mypy suppression.Similar to
seller.py, this directive may no longer be needed since return types are now correctly annotated asUserGroup | None. Verify if mypy passes without this suppression.seed/accounts/seller.py (1)
1-1: Remove the mypy suppression directive.The
# mypy: disable-error-code=unreachableat line 1 is no longer necessary. All code paths in this file are reachable: the return statements at lines 32–33 and 70–74 are properly conditioned, and there are no unreachable code branches. With the correctedSeller | Nonereturn types already in place, mypy should pass without this suppression.seed/accounts/module.py (2)
15-32: Validate API response type before caching the moduleAfter
mpt_operations.accounts.modules.get(module_id)you assume the result is aModuleand immediately cache it and accessmodule.id. If the API ever returnsNoneor an unexpected type, this will either break withAttributeErroror pollute the context with a bad object.Consider guarding the cache/update with an explicit type check and logging when the type is wrong, e.g.:
if not isinstance(module, Module): - module = await mpt_operations.accounts.modules.get(module_id) - context.set_resource("accounts.module", module) - context["accounts.module.id"] = module.id - return module + module = await mpt_operations.accounts.modules.get(module_id) + if not isinstance(module, Module): + logger.warning("Module %s fetched from API is not a Module instance.", module_id) + return None + context.set_resource("accounts.module", module) + context["accounts.module.id"] = module.id + return moduleThis keeps the context consistent and lets callers handle a missing/invalid module via the existing
Nonepath.
36-57: Alignrefresh_moduledocstring with its behaviorThe docstring says “always fetch”, but
refresh_moduledelegates toget_moduleand returns the cached module when it exists, only hitting the API when there is no module. Either update the docstring to reflect the current behavior or change the implementation to always refetch when a module ID is present.seed/accounts/licensee.py (2)
17-35: Type‑check APIlicenseebefore caching into contextIn
get_licensee, if the cached value is missing/invalid you refetch viampt_client.accounts.licensees.get(licensee_id)and then unconditionally cache and use the result. If the API returnsNoneor some non‑Licenseemodel, this will break or corrupt the cached resource.Consider mirroring a defensive pattern here:
if not isinstance(licensee, Licensee): - licensee = await mpt_client.accounts.licensees.get(licensee_id) - context.set_resource("accounts.licensee", licensee) - context["accounts.licensee.id"] = licensee.id - return licensee + licensee = await mpt_client.accounts.licensees.get(licensee_id) + if not isinstance(licensee, Licensee): + logger.warning( + "Licensee %s fetched from API is not a Licensee instance.", + licensee_id, + ) + return None + context.set_resource("accounts.licensee", licensee) + context["accounts.licensee.id"] = licensee.id + return licenseeThat keeps the context consistent and preserves the
Nonecontract for callers.
39-89: Tidy up longValueErrormessages flagged by Ruff (TRY003)Ruff is flagging the inline
ValueErrormessages inbuild_licensee_data(Line 45) andinit_licensee(Line 89). To keep the explicit messages while satisfying the linter, either:
- Extract them into module‑level constants and use those in the raises/logs, or
- Append a focused suppression like
# noqa: TRY003on those lines if your team is comfortable with that.Centralizing the strings is a small cleanup that also avoids duplicating error text.
seed/accounts/buyer.py (1)
38-61: Resolve Ruff TRY003 for Buyer‑relatedValueErrormessagesRuff is flagging the inline
ValueErrormessages inbuild_buyer_data(Line 43) andinit_buyer(Line 83). To keep the descriptive messages without tripping TRY003, you can:
- Extract them into module‑level constants and reuse them, or
- Add explicit
# noqa: TRY003on those raise lines if you intentionally keep the literals inline.Using constants has the nice side effect of centralizing user‑facing error text for reuse.
Also applies to: 64-85
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
seed/accounts/logo.pngis excluded by!**/*.png
📒 Files selected for processing (24)
pyproject.toml(2 hunks)seed/accounts/accounts.py(1 hunks)seed/accounts/api_tokens.py(1 hunks)seed/accounts/buyer.py(1 hunks)seed/accounts/licensee.py(1 hunks)seed/accounts/module.py(1 hunks)seed/accounts/seller.py(1 hunks)seed/accounts/user_group.py(1 hunks)seed/catalog/product.py(1 hunks)seed/container.py(1 hunks)seed/seed_api.py(2 hunks)tests/e2e/accounts/buyers/conftest.py(1 hunks)tests/e2e/accounts/licensees/conftest.py(1 hunks)tests/e2e/accounts/sellers/conftest.py(1 hunks)tests/seed/accounts/test_accounts.py(1 hunks)tests/seed/accounts/test_api_token.py(1 hunks)tests/seed/accounts/test_buyer.py(1 hunks)tests/seed/accounts/test_licensee.py(1 hunks)tests/seed/accounts/test_module.py(1 hunks)tests/seed/accounts/test_seller.py(1 hunks)tests/seed/accounts/test_user_group.py(1 hunks)tests/seed/catalog/test_product.py(1 hunks)tests/seed/conftest.py(3 hunks)tests/seed/test_seed_api.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- seed/container.py
- tests/e2e/accounts/buyers/conftest.py
- tests/seed/catalog/test_product.py
- tests/seed/accounts/test_user_group.py
- tests/e2e/accounts/sellers/conftest.py
- tests/seed/accounts/test_buyer.py
- tests/seed/accounts/test_api_token.py
- tests/seed/conftest.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:
pyproject.toml
🧬 Code graph analysis (11)
tests/seed/accounts/test_module.py (4)
mpt_api_client/resources/accounts/modules.py (2)
AsyncModulesService(29-32)Module(11-12)seed/accounts/module.py (3)
get_module(15-32)refresh_module(36-57)seed_module(61-72)seed/context.py (2)
Context(12-33)get_resource(19-29)mpt_api_client/models/model.py (1)
id(119-121)
seed/accounts/accounts.py (6)
seed/accounts/api_tokens.py (1)
seed_api_token(71-75)seed/accounts/buyer.py (1)
seed_buyer(89-93)seed/accounts/licensee.py (1)
seed_licensee(95-99)seed/accounts/module.py (1)
seed_module(61-72)seed/accounts/seller.py (1)
seed_seller(78-82)seed/accounts/user_group.py (1)
seed_user_group(80-85)
tests/seed/accounts/test_accounts.py (1)
seed/accounts/accounts.py (1)
seed_accounts(13-23)
seed/accounts/module.py (4)
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/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)
seed/accounts/buyer.py (2)
mpt_api_client/resources/accounts/buyers.py (1)
Buyer(26-27)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)
seed/seed_api.py (4)
seed/accounts/accounts.py (1)
seed_accounts(13-23)seed/context.py (1)
load_context(36-52)tests/seed/conftest.py (1)
context(15-16)seed/catalog/catalog.py (1)
seed_catalog(32-39)
tests/seed/accounts/test_seller.py (2)
seed/accounts/seller.py (4)
build_seller_data(37-52)get_seller(16-33)init_seller(56-74)seed_seller(78-82)seed/context.py (2)
Context(12-33)get_resource(19-29)
seed/accounts/api_tokens.py (3)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/api_tokens.py (1)
ApiToken(12-13)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)
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/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)mpt_api_client/models/model.py (1)
id(119-121)
seed/accounts/user_group.py (5)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/user_groups.py (1)
UserGroup(11-12)tests/seed/conftest.py (1)
context(15-16)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)tests/seed/accounts/test_user_group.py (1)
user_group(16-17)
seed/catalog/product.py (5)
tests/seed/catalog/test_product.py (1)
product(17-18)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)mpt_api_client/resources/catalog/products.py (1)
Product(53-54)seed/context.py (1)
set_resource(31-33)mpt_api_client/models/model.py (1)
id(119-121)
🪛 Ruff (0.14.7)
seed/accounts/accounts.py
13-13: Unused noqa directive (unknown: WPS217)
Remove unused noqa directive
(RUF100)
seed/accounts/licensee.py
45-45: Avoid specifying long messages outside the exception class
(TRY003)
89-89: Avoid specifying long messages outside the exception class
(TRY003)
tests/seed/accounts/test_accounts.py
6-6: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
seed/accounts/module.py
69-69: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/buyer.py
43-43: Avoid specifying long messages outside the exception class
(TRY003)
83-83: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/user_group.py
43-43: Avoid specifying long messages outside the exception class
(TRY003)
seed/catalog/product.py
63-63: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (11)
pyproject.toml (1)
117-135: LGTM!The per-file-ignores additions for
seed/accounts/*.pyand the accounts resources are appropriate for the new seeding modules. The WPS453 suppression aligns with the patterns used in similar seeding code. Based on learnings, this repository uses wemake-python-styleguide, so these WPS rule suppressions are valid.tests/e2e/accounts/licensees/conftest.py (1)
29-35: LGTM!The address data update aligns the test fixture with the seeding module conventions, maintaining consistency across the test suite.
seed/catalog/product.py (1)
51-65: LGTM!The restructured control flow is clear and defensive:
- Guards creation behind the
Nonecheck- Validates the API response type with
isinstance(created, Product)before use- Provides explicit logging for both success and failure paths
- Raises a clear exception on creation failure
This pattern is consistent with other seed modules in the PR.
tests/seed/accounts/test_module.py (1)
92-101: LGTM!The test properly uses
AsyncMockfor patching the asyncget_moduleandrefresh_modulefunctions, addressing the previous review feedback. The pattern correctly sets up the mock return value before invoking the function under test.tests/seed/accounts/test_seller.py (1)
83-88: LGTM!The
test_seed_sellercorrectly usesAsyncMockfor patching the asyncinit_sellerfunction, following proper async testing patterns.seed/accounts/user_group.py (1)
55-76: LGTM!The
init_user_groupfunction now properly:
- Validates the created resource with
isinstance(created, UserGroup)before use- Returns
Noneon failure with appropriate logging- Correctly passes injected parameters to
get_user_grouptests/seed/test_seed_api.py (1)
23-39: Updated orchestration test forseed_accountslooks correctPatching
seed.seed_api.seed_accountsalongsideseed_catalogand asserting both mocks are called once cleanly exercises the new concurrent seeding path without hitting real I/O. This aligns with the updatedseed_apiimplementation.seed/seed_api.py (1)
20-33: Concurrent catalog/accounts seeding and context persistence look goodUsing
asyncio.create_taskplusasyncio.gatherto runseed_catalog()andseed_accounts()in parallel, while wrapping the gather in a try/except and always callingsave_contextinfinally, is a solid pattern for this orchestration. Behavior matches the updated tests and keeps context persistence robust even on failures.seed/accounts/api_tokens.py (3)
1-11: LGTM!Imports and logger setup are clean and appropriate for the module's functionality.
51-67: Parameter propagation issue resolved.The function now correctly passes
contextandmpt_opsto bothget_api_token(line 57) andbuild_api_token_data(line 60), ensuring that non-default dependencies are properly respected. This fix enables correct behavior for tests and alternative contexts.
70-75: LGTM!The wrapper function correctly delegates to
init_api_token, which will use its own injected defaults. The logging provides appropriate visibility into the seeding process.
robcsegal
force-pushed
the
MPT-15443-seed-data-required-for-accounts-e-2-e-tests
branch
from
e1e9586 to
14e8cfa
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (10)
seed/accounts/user_group.py (3)
28-32: Add type validation after API fetch.When fetching from API on line 29, the result is used without verifying it's a valid
UserGroupinstance. If the API returns an error or unexpected type, setting context resources will fail.Apply this diff:
if not isinstance(user_group, UserGroup): user_group = await mpt_operations.accounts.user_groups.get(user_group_id) + if not isinstance(user_group, UserGroup): + return None context.set_resource("accounts.user_group", user_group) context["accounts.user_group.id"] = user_group.id return user_group
44-52: Add validation formodule_idbefore building user group payload.
context.get_string("accounts.module.id")returns an empty string if the key is missing. This would produce"modules": [{"id": ""}]in the payload, causing an API error.Apply this diff:
if not account_id: raise ValueError("CLIENT_ACCOUNT_ID environment variable is required") module_id = context.get_string("accounts.module.id") + if not module_id: + raise ValueError( + "accounts.module.id missing from context; seed module before user group." + ) return {
1-1: Remove the mypy suppression now that return types are aligned.The
# mypy: disable-error-code=unreachablesuppression at line 1 was needed when the return type wasUserGroupbut the function could returnNone. Now thatinit_user_groupandseed_user_groupcorrectly returnUserGroup | None, this suppression can be safely removed.-# mypy: disable-error-code=unreachable import loggingseed/accounts/accounts.py (1)
13-13: Remove unused# noqa: WPS217directive.Ruff reports this as an unused noqa (RUF100). Since the function body is straightforward, drop the directive:
-async def seed_accounts() -> None: # noqa: WPS217 +async def seed_accounts() -> None:tests/seed/accounts/test_accounts.py (1)
6-6: Remove unused# noqa: WPS210directive.Ruff flags this as an unused suppression (RUF100). The test function is straightforward and doesn't trigger WPS210.
-async def test_seed_accounts() -> None: # noqa: WPS210 +async def test_seed_accounts() -> None:seed/accounts/module.py (1)
69-69: Address Ruff TRY003 on theValueErrormessage.Ruff flags the long exception message. Consider suppressing with
# noqa: TRY003if the explicit message is preferred for clarity.- raise ValueError("Could not seed module: no valid Module found.") + raise ValueError("Could not seed module: no valid Module found.") # noqa: TRY003seed/accounts/seller.py (1)
28-32: Add type validation after APIgetcall.After fetching via
mpt_operations.accounts.sellers.get(seller_id), the code accessesseller.idwithout confirming the result is aSeller. If the API returns an unexpected type, this will raise anAttributeError.if not isinstance(seller, Seller): seller = await mpt_operations.accounts.sellers.get(seller_id) + if not isinstance(seller, Seller): + logger.warning("Unexpected type returned for seller id %s", seller_id) + return None context.set_resource("accounts.seller", seller) context["accounts.seller.id"] = seller.id return sellerThis matches the defensive pattern used in
init_sellerat lines 66-72.seed/accounts/api_tokens.py (2)
27-31: Add type validation after API fetch.After fetching the API token from the API on line 28, the code assumes the result is a valid
ApiTokeninstance and directly accessesapi_token.idon line 30. If the API returns an unexpected type or error response, this will raise anAttributeError.Apply this diff to add validation:
if not isinstance(api_token, ApiToken): api_token = await mpt_ops.accounts.api_tokens.get(api_token_id) + if not isinstance(api_token, ApiToken): + logger.warning("API returned non-ApiToken type for id: %s", api_token_id) + return None context.set_resource("accounts.api_token", api_token) context["accounts.api_token.id"] = api_token.id return api_token
35-48: Validate required environment variable and context value.The function reads
CLIENT_ACCOUNT_IDfrom the environment (line 40) andaccounts.module.idfrom context (line 41) without validation. If either is missing, the API creation call will fail with an unclear error.Apply this diff to add validation:
@inject def build_api_token_data( context: Context = DEFAULT_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") + if not module_id: + raise ValueError("accounts.module.id missing from context; seed module first") return { "account": {"id": account_id},seed/accounts/buyer.py (1)
27-34: Add type validation after API fetch.After fetching the buyer from the API on line 31, the code assumes the result is a valid
Buyerinstance and directly accessesbuyer.idon line 33. If the API returns an unexpected type or error response, this will raise anAttributeError.Apply this diff to add validation:
if not isinstance(buyer, Buyer): buyer = await mpt_operations.accounts.buyers.get(buyer_id) + if not isinstance(buyer, Buyer): + logger.warning("API returned non-Buyer type for id: %s", buyer_id) + return None context.set_resource("accounts.buyer", buyer) context["accounts.buyer.id"] = buyer.id return buyer
🧹 Nitpick comments (3)
tests/seed/accounts/test_module.py (1)
34-35: Remove unusedasync_iterfunction.This helper function is not used anywhere in the test file.
DummyAsyncIteratoris used instead for async iteration.-def async_iter(iter_items): - yield from iter_items - - async def test_get_module(context: Context, operations_client, module, modules_service) -> None:tests/seed/accounts/test_seller.py (1)
56-80: Consider consolidating similar tests.
test_init_sellerandtest_init_seller_create_newboth test the "create new seller" path by patchingget_sellerto returnNone. The second test adds a patch forbuild_seller_databut doesn't provide additional coverage sincecreate.assert_called_once()is the same assertion.Consider removing one or differentiating them (e.g., test the "existing seller" path in one test).
seed/accounts/api_tokens.py (1)
51-67: Consider validating API token creation result.While the parameters are now correctly passed (addressing previous feedback), the code assumes
create()on line 61 returns a validApiTokenand accesses.idon line 63 without validation. Although API creation typically fails loudly, adding validation would make error handling more explicit and consistent with the pattern used ininit_buyer.Consider applying this diff:
if api_token is None: logger.debug("Creating API token ...") api_token_data = build_api_token_data(context=context) api_token = await mpt_ops.accounts.api_tokens.create(api_token_data) + if not isinstance(api_token, ApiToken): + logger.warning("API token creation failed") + raise ValueError("API token creation failed") context.set_resource("accounts.api_token", api_token) context["accounts.api_token.id"] = api_token.id logger.info("API token created: %s", api_token.id)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
seed/accounts/logo.pngis excluded by!**/*.png
📒 Files selected for processing (24)
pyproject.toml(2 hunks)seed/accounts/accounts.py(1 hunks)seed/accounts/api_tokens.py(1 hunks)seed/accounts/buyer.py(1 hunks)seed/accounts/licensee.py(1 hunks)seed/accounts/module.py(1 hunks)seed/accounts/seller.py(1 hunks)seed/accounts/user_group.py(1 hunks)seed/catalog/product.py(1 hunks)seed/container.py(1 hunks)seed/seed_api.py(2 hunks)tests/e2e/accounts/buyers/conftest.py(1 hunks)tests/e2e/accounts/licensees/conftest.py(1 hunks)tests/e2e/accounts/sellers/conftest.py(1 hunks)tests/seed/accounts/test_accounts.py(1 hunks)tests/seed/accounts/test_api_token.py(1 hunks)tests/seed/accounts/test_buyer.py(1 hunks)tests/seed/accounts/test_licensee.py(1 hunks)tests/seed/accounts/test_module.py(1 hunks)tests/seed/accounts/test_seller.py(1 hunks)tests/seed/accounts/test_user_group.py(1 hunks)tests/seed/catalog/test_product.py(1 hunks)tests/seed/conftest.py(3 hunks)tests/seed/test_seed_api.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/e2e/accounts/licensees/conftest.py
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/e2e/accounts/buyers/conftest.py
- tests/seed/accounts/test_buyer.py
- seed/container.py
- tests/e2e/accounts/sellers/conftest.py
- pyproject.toml
- tests/seed/conftest.py
- tests/seed/catalog/test_product.py
- tests/seed/test_seed_api.py
🧰 Additional context used
🧬 Code graph analysis (9)
tests/seed/accounts/test_licensee.py (4)
mpt_api_client/models/model.py (2)
Model(65-125)id(119-121)mpt_api_client/resources/accounts/licensees.py (2)
AsyncLicenseesService(48-58)Licensee(21-22)seed/accounts/licensee.py (4)
build_licensee_data(39-68)get_licensee(18-35)init_licensee(72-91)seed_licensee(95-99)seed/context.py (3)
Context(12-33)get_resource(19-29)set_resource(31-33)
tests/seed/accounts/test_user_group.py (3)
seed/accounts/user_group.py (4)
build_user_group_data(37-52)get_user_group(16-33)init_user_group(56-76)seed_user_group(80-85)tests/seed/conftest.py (2)
context(15-16)operations_client(25-26)seed/context.py (2)
Context(12-33)get_resource(19-29)
seed/accounts/module.py (4)
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/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)
seed/seed_api.py (4)
seed/accounts/accounts.py (1)
seed_accounts(13-23)seed/context.py (1)
load_context(36-52)tests/seed/conftest.py (1)
context(15-16)seed/catalog/catalog.py (1)
seed_catalog(32-39)
seed/accounts/user_group.py (3)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/user_groups.py (1)
UserGroup(11-12)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)
tests/seed/accounts/test_accounts.py (1)
seed/accounts/accounts.py (1)
seed_accounts(13-23)
seed/accounts/api_tokens.py (4)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/api_tokens.py (1)
ApiToken(12-13)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)tests/seed/accounts/test_api_token.py (1)
api_token(15-16)
seed/accounts/seller.py (3)
mpt_api_client/resources/accounts/sellers.py (1)
Seller(13-14)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)mpt_api_client/models/model.py (1)
id(119-121)
seed/accounts/buyer.py (3)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/buyers.py (1)
Buyer(26-27)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)
🪛 Ruff (0.14.7)
seed/accounts/module.py
69-69: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/user_group.py
43-43: Avoid specifying long messages outside the exception class
(TRY003)
tests/seed/accounts/test_accounts.py
6-6: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
seed/accounts/licensee.py
45-45: Avoid specifying long messages outside the exception class
(TRY003)
89-89: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/buyer.py
43-43: Avoid specifying long messages outside the exception class
(TRY003)
83-83: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/accounts.py
13-13: Unused noqa directive (unknown: WPS217)
Remove unused noqa directive
(RUF100)
seed/catalog/product.py
63-63: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (32)
seed/catalog/product.py (1)
51-65: LGTM! Clean control flow with proper type validation.The refactored logic correctly guards creation when no product exists, validates the API response type, and returns the existing product otherwise. The
isinstance(created, Product)check follows the same pattern used in other seeders.seed/seed_api.py (1)
20-32: LGTM! Proper parallel seeding with robust error handling.The parallel execution of
seed_catalog()andseed_accounts()is appropriate since these are independent operations. The exception handling andfinallyblock ensure context is persisted regardless of success or failure.seed/accounts/accounts.py (1)
14-23: LGTM! Correct seeding order with proper dependency handling.The sequential execution ensures
seed_module()completes beforeseed_user_group(), which depends onmodule_idin context. The logging provides good visibility into the seeding lifecycle.tests/seed/accounts/test_api_token.py (5)
1-21: LGTM! Well-structured test fixtures.The fixtures properly create mock objects with appropriate specs, enabling type-safe testing of async API interactions.
24-37: LGTM! Good coverage of get_api_token paths.The tests cover both the successful retrieval case and the edge case where no ID exists in context.
40-54: LGTM! Proper test of the creation path.The test correctly patches dependencies to isolate the creation logic and verifies the API call is made.
57-70: LGTM! Correct environment variable handling.Using
monkeypatch.setenvproperly isolates the test from the actual environment while verifying the payload structure.
73-78: LGTM! Concise orchestration test.The test verifies that
seed_api_tokenproperly delegates toinit_api_token.tests/seed/accounts/test_module.py (5)
10-21: LGTM! Proper async iterator with exception chaining.The
DummyAsyncIteratorcorrectly chainsStopAsyncIteration from err, addressing the B904 lint rule.
38-51: LGTM! Good test coverage for get_module.Both the successful retrieval and the missing ID edge case are properly tested.
54-70: LGTM! Proper async mocking for refresh_module.The test correctly uses
AsyncMockfor the asyncget_modulefunction andDummyAsyncIteratorfor filter iteration.
73-89: LGTM! Tests the new module retrieval path.The test verifies the module refresh behavior when fetching a new module through iteration.
92-102: LGTM! Proper async orchestration test.The test correctly uses
AsyncMockfor both patched async functions and verifies the delegation torefresh_module.tests/seed/accounts/test_accounts.py (1)
7-30: LGTM!The test correctly patches all six seed functions called by
seed_accounts()and verifies each is invoked exactly once. The use ofAsyncMockwithnew_callableensures proper async behavior.seed/accounts/seller.py (3)
36-52: LGTM!The
build_seller_datafunction correctly generates a uniqueexternalIdwhen none is provided and constructs the required payload structure.
55-74: LGTM!The
init_sellerfunction correctly forwards injected dependencies toget_seller, validates the API response type before caching, and handles both success and failure paths appropriately.
77-82: LGTM!The
seed_sellerorchestration function correctly relies on dependency injection for defaults and provides appropriate logging.tests/seed/accounts/test_seller.py (4)
10-17: LGTM!Fixtures are well-defined with proper specs for type safety and clear test data.
20-33: LGTM!Both
get_sellertests are well-structured, covering the happy path with context caching and the early return when no ID is present.
36-53: LGTM!The test validates the complete payload structure and correctly verifies the
externalIdpassthrough.
83-88: LGTM!The test correctly verifies that
seed_sellerinvokesinit_sellerexactly once.tests/seed/accounts/test_user_group.py (4)
15-22: LGTM!Fixtures are properly defined with
AsyncMockand appropriate specs.
25-40: LGTM!Both
get_user_grouptests properly verify retrieval with caching and the early return path.
43-61: LGTM!The test correctly uses
monkeypatch.setenvto set the required environment variable before the patch setup evaluatesbuild_user_group_data(context). This addresses the previously flagged issue.
64-89: LGTM!Both tests are well-structured.
test_build_user_group_datacorrectly usesmonkeypatchfor the environment variable, andtest_seed_user_groupproperly verifies the orchestration.seed/accounts/module.py (1)
35-57: LGTM!The
refresh_modulefunction correctly forwards dependencies, uses proper type validation for the filtered results, and handles edge cases with appropriate logging.seed/accounts/licensee.py (3)
38-68: LGTM!The
build_licensee_datafunction correctly validates the requiredCLIENT_ACCOUNT_IDenvironment variable and properly retrieves dependent resources from context.
71-91: LGTM!The
init_licenseefunction correctly forwards dependencies, uses proper file handling with context manager, and validates the API response type before caching.
94-99: LGTM!The
seed_licenseeorchestration function correctly relies on dependency injection and provides appropriate logging.tests/seed/accounts/test_licensee.py (1)
11-35: Licensee seeding tests comprehensively cover env/context wiring and async flowsFixtures, env setup, and AsyncMock-based patches now align with the production
seed.accounts.licenseelogic (includingCLIENT_ACCOUNT_IDandpathlib.Path.openusage). Both the lookup and create paths forinit_licenseeplus the top-levelseed_licenseeorchestration are exercised, and I don’t see remaining correctness issues here.Also applies to: 38-52, 54-83, 85-138, 141-146
seed/accounts/buyer.py (2)
38-61: LGTM!The validation of
CLIENT_ACCOUNT_IDproperly addresses the previous review comment, and the function correctly constructs the buyer payload from environment and context.
64-85: LGTM!The function correctly passes injected parameters to downstream calls (addressing previous feedback) and implements a robust create-or-get pattern with proper validation and error handling.
robcsegal
force-pushed
the
MPT-15443-seed-data-required-for-accounts-e-2-e-tests
branch
from
91b8405 to
7b9f708
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/catalog/product.py (1)
87-96: Propagatecontextintoget_productinpublish_product.
publish_productaccepts acontextparameter but currently callsget_product()with no arguments, which means a caller-provided context is ignored and the default context is used instead. This can desync behavior in tests or multi-context scenarios.Consider:
async def publish_product( context: Context = DEFAULT_CONTEXT, mpt_operations: AsyncMPTClient = DEFAULT_MPT_OPERATIONS, ) -> Product | None: """Publish product if in reviewing status.""" - product = await get_product() + product = await get_product(context=context)
♻️ Duplicate comments (14)
seed/accounts/module.py (2)
61-69: Address Ruff TRY003 onValueErrormessage inseed_module.Ruff flags the long literal message here. To keep the explicit error while satisfying the linter, either extract it to a constant or add a targeted suppression:
+MODULE_SEED_ERROR = "Could not seed module: no valid Module found." ... - raise ValueError("Could not seed module: no valid Module found.") + raise ValueError(MODULE_SEED_ERROR)or:
- raise ValueError("Could not seed module: no valid Module found.") + raise ValueError("Could not seed module: no valid Module found.") # noqa: TRY003
20-32: Add type validation aftermpt_operations.accounts.modules.getbefore usingmodule.id.After an API failure or unexpected response,
modulemight not be aModule, but it’s written into context andmodule.idis accessed unconditionally. This can corrupt context and raise at runtime.Consider:
if not isinstance(module, Module): module = await mpt_operations.accounts.modules.get(module_id) - context.set_resource("accounts.module", module) - context["accounts.module.id"] = module.id - return module + if not isinstance(module, Module): + logger.warning("Unexpected type returned for module id %s", module_id) + return None + context.set_resource("accounts.module", module) + context["accounts.module.id"] = module.id + return moduleseed/accounts/user_group.py (3)
1-1: Remove now-unnecessarymypyunreachable suppression.
init_user_groupandseed_user_groupnow returnUserGroup | None, so there is no unreachable branch. The top-level# mypy: disable-error-code=unreachablecan be dropped to let type-checking work normally.[save]
21-33: Add type validation aftermpt_operations.accounts.user_groups.getbefore usinguser_group.id.Similar to buyer/module, this path assumes the API returns a
UserGroupand immediately writes it into context.Suggested pattern:
if not isinstance(user_group, UserGroup): user_group = await mpt_operations.accounts.user_groups.get(user_group_id) - context.set_resource("accounts.user_group", user_group) - context["accounts.user_group.id"] = user_group.id - return user_group + if not isinstance(user_group, UserGroup): + logger.warning("Unexpected type returned for user group id %s", user_group_id) + return None + context.set_resource("accounts.user_group", user_group) + context["accounts.user_group.id"] = user_group.id + return user_group
41-52: Validatemodule_idbefore building payload (and consider TRY003).
module_id = context.get_string("accounts.module.id")can produce an empty string, which would send"modules": [{"id": ""}]to the API and likely fail with a less clear error. Also, Ruff flags the longValueErrormessage for CLIENT_ACCOUNT_ID.You can:
account_id = os.getenv("CLIENT_ACCOUNT_ID") if not account_id: - raise ValueError("CLIENT_ACCOUNT_ID environment variable is required") + raise ValueError("CLIENT_ACCOUNT_ID environment variable is required") # noqa: TRY003 module_id = context.get_string("accounts.module.id") + if not module_id: + raise ValueError( + "accounts.module.id missing from context; seed module before user group." + ) # noqa: TRY003or extract the messages into constants if you prefer not to use
noqa.seed/accounts/accounts.py (1)
13-21: Remove unused# noqa: WPS217onseed_accounts.Ruff reports this as an unused/unknown suppression. The function body is short, so the directive isn’t needed—dropping it keeps lint output clean.
-async def seed_accounts() -> None: # noqa: WPS217 +async def seed_accounts() -> None:seed/accounts/buyer.py (1)
23-35: Validate type aftermpt_operations.accounts.buyers.getbefore using/storing.If the API ever returns an unexpected type,
buyer.idand the cached resource will be wrong, leading to confusing runtime errors.You can mirror the pattern used elsewhere:
if not isinstance(buyer, Buyer): buyer = await mpt_operations.accounts.buyers.get(buyer_id) - context.set_resource("accounts.buyer", buyer) - context["accounts.buyer.id"] = buyer.id - return buyer + if not isinstance(buyer, Buyer): + logger.warning("Unexpected type returned for buyer id %s", buyer_id) + return None + context.set_resource("accounts.buyer", buyer) + context["accounts.buyer.id"] = buyer.id + return buyertests/seed/accounts/test_accounts.py (1)
6-6: Remove unusednoqadirective.The
# noqa: WPS210suppression is flagged as unused by Ruff. Since the test is simple enough and doesn't trigger WPS210, the suppression should be removed:-async def test_seed_accounts() -> None: # noqa: WPS210 +async def test_seed_accounts() -> None:Based on static analysis hints and past review comments.
tests/seed/catalog/test_product.py (1)
51-52: Remove duplicatepathlib.Path.openpatch.Lines 51 and 52 both patch
pathlib.Path.open, causing the second patch to override the first. This creates dead code and confusion.with ( patch("seed.catalog.product.get_product", new_callable=AsyncMock) as mock_get_product, patch("seed.catalog.product.icon", new=MagicMock()), - patch("pathlib.Path.open", new=lambda *_, **_unused: fake_file_factory()), - patch("pathlib.Path.open", new=lambda *_, **_unused: fake_file_factory()), + patch("pathlib.Path.open", new=lambda *_, **__: fake_file_factory()), ):Based on past review comments.
seed/accounts/seller.py (1)
28-33: Add type validation after APIgetcall.Line 29 calls
await mpt_operations.accounts.sellers.get(seller_id)and immediately accessesseller.idon line 31 without validating the response type. If the API returns an unexpected type or error response, this will raise anAttributeError.if not isinstance(seller, Seller): seller = await mpt_operations.accounts.sellers.get(seller_id) + if not isinstance(seller, Seller): + logger.warning( + "accounts.sellers.get returned unexpected type for id %s", + seller_id, + ) + return None context.set_resource("accounts.seller", seller) context["accounts.seller.id"] = seller.id return sellerThis adds the same defensive check used in
init_seller(lines 66-72).Based on past review comments.
seed/accounts/licensee.py (2)
39-54: Tightenbuild_licensee_datavalidation and clean up linter noise.A few issues are clustered here:
Context.get_resource("accounts.user_group")either returns aModelor raisesValueError; theif group is None:branch is effectively unreachable and never surfaces your clearer error.- Ruff reports
RUF100for the# noqa: WPS238directive because that code is no longer associated with a known rule; leaving it in produces unnecessary lint noise.- Ruff also flags TRY003 on the long
ValueErrormessages, which you may want to address by centralizing messages or using dedicated exception types/constants.You can improve clarity and satisfy the linters with something like:
-@inject -def build_licensee_data( # noqa: WPS238 +@inject +def build_licensee_data( @@ - 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 as exc: + raise ValueError( + "accounts.user_group missing from context; seed user group before licensee." + ) from exc # noqa: TRY003Optionally, you can also extract the four validation checks into a small helper (as suggested earlier) to completely remove the need for WPS238 suppression.
30-34: Add defensive type check after APIgetinget_licensee.After falling back to
await mpt_client.accounts.licensees.get(licensee_id), you assume the result is aLicenseeand immediately access.id. If the API or client ever returns an unexpected type, this will either crash or pollute the context with an invalid resource.Recommend validating the type before using it, mirroring the earlier suggestion:
if not isinstance(licensee, Licensee): licensee = await mpt_client.accounts.licensees.get(licensee_id) - context.set_resource("accounts.licensee", licensee) - context["accounts.licensee.id"] = licensee.id - return licensee + if not isinstance(licensee, Licensee): + logger.warning("Unexpected type returned for licensee id: %s", licensee_id) + return None + context.set_resource("accounts.licensee", licensee) + context["accounts.licensee.id"] = licensee.id + return licenseeThis keeps
get_licenseerobust against bad responses while preserving the existing “get or create” behavior ininit_licensee.seed/accounts/api_tokens.py (2)
27-32: Add type validation aftermpt_ops.accounts.api_tokens.getinget_api_token.After the fallback fetch, the code assumes the result is an
ApiTokenand immediately accesses.id. If the API or client returns an unexpected type, this will fail or contaminate the context.Recommend guarding the result:
if not isinstance(api_token, ApiToken): api_token = await mpt_ops.accounts.api_tokens.get(api_token_id) - context.set_resource("accounts.api_token", api_token) - context["accounts.api_token.id"] = api_token.id - return api_token + if not isinstance(api_token, ApiToken): + logger.warning("API returned non-ApiToken type for id: %s", api_token_id) + return None + context.set_resource("accounts.api_token", api_token) + context["accounts.api_token.id"] = api_token.id + return api_tokenThis keeps
get_api_tokensafe without changing its public contract.
35-48: ValidateCLIENT_ACCOUNT_IDandaccounts.module.idbefore building the API token payload.Right now,
account_idmay beNoneandmodule_idmay be an empty string, but the function still returns a payload. That can lead to confusing API errors or tokens tied to invalid IDs.Align this with the stricter validation you use for licensee seeding:
"""Get API token data dictionary for creation.""" account_id = os.getenv("CLIENT_ACCOUNT_ID") module_id = context.get_string("accounts.module.id") + if not account_id: + raise ValueError("CLIENT_ACCOUNT_ID environment variable is not set.") + if not module_id: + raise ValueError("accounts.module.id missing from context; seed module first.") return { "account": {"id": account_id},Failing fast here makes seeding failures easier to diagnose and avoids sending malformed requests to the API.
🧹 Nitpick comments (8)
seed/accounts/module.py (1)
40-57: Alignrefresh_moduledocstring with behavior (or always re-fetch).The docstring says “always fetch”, but the implementation returns the cached module when present and only queries by name when nothing is found. Either update the docstring to reflect this “refresh if missing” behavior, or change the code to always re-fetch from the API when a module is already in context.
seed/accounts/buyer.py (1)
41-47: Handle Ruff TRY003 for explicitValueErrormessages.The three raised
ValueErrors here use relatively long literal messages and are flagged by Ruff. If these messages are important to keep as-is, either:
- Extract them into named constants and reference those, or
- Add
# noqa: TRY003to the specific lines.Example constant approach:
+CLIENT_ACCOUNT_ID_REQUIRED = "CLIENT_ACCOUNT_ID environment variable is required" +SELLER_ID_MISSING = "accounts.seller.id missing from context; seed seller before buyer." +BUYER_CREATION_FAILED = "Buyer creation failed" ... - if not buyer_account_id: - raise ValueError("CLIENT_ACCOUNT_ID environment variable is required") + if not buyer_account_id: + raise ValueError(CLIENT_ACCOUNT_ID_REQUIRED) ... - if not seller_id: - raise ValueError("accounts.seller.id missing from context; seed seller before buyer.") + if not seller_id: + raise ValueError(SELLER_ID_MISSING) ... - logger.warning("Buyer creation failed") - raise ValueError("Buyer creation failed") + logger.warning(BUYER_CREATION_FAILED) + raise ValueError(BUYER_CREATION_FAILED)Also applies to: 84-84
seed/catalog/product.py (1)
57-63: Optionally handle TRY003 on"Product creation failed"ValueError.Ruff flags this string; if you want to keep the explicit message, either move it into a constant or add a focused noqa:
-PRODUCT_CREATION_FAILED_MSG = "Product creation failed" ... - logger.warning("Product creation failed") - raise ValueError("Product creation failed") + logger.warning(PRODUCT_CREATION_FAILED_MSG) + raise ValueError(PRODUCT_CREATION_FAILED_MSG)or:
- logger.warning("Product creation failed") - raise ValueError("Product creation failed") + logger.warning("Product creation failed") + raise ValueError("Product creation failed") # noqa: TRY003tests/seed/accounts/test_seller.py (1)
66-81: Consider removing or differentiating duplicate test.
test_init_seller_create_newis very similar totest_init_seller(lines 56-64). Both patchget_sellerto returnNoneand assertcreate()is called once. The only difference is the additionalbuild_seller_datapatch, which doesn't add meaningful coverage sinceinit_sellercallsbuild_seller_data()without arguments.If the intent is to verify
build_seller_datais called with specific arguments, add an assertion likemock_build_seller_data.assert_called_once_with(...). Otherwise, consider removing this duplicate test.tests/seed/accounts/test_module.py (1)
54-90: Consider consolidating duplicatetest_refresh_moduletests.
test_refresh_module(lines 54-71) andtest_refresh_module_get_new(lines 73-90) appear to be functionally identical—both set up the same mocks, context, and assertions. If they're testing the same behavior, consider consolidating them into a single test. If they're meant to test different scenarios, clarify the distinction with comments or different setup/assertions.seed/accounts/seller.py (1)
1-1: Clarify themypy: disable-error-code=unreachabledirective.The global
mypy: disable-error-code=unreachablesuppression suggests a type inconsistency that's being masked rather than fixed. While the current signatureinit_seller(...) -> Seller | Noneand thereturn Noneon line 72 are technically consistent, this pattern differs from similar functions in the codebase (e.g.,init_productraisesValueErroron failure).Consider either:
- Removing the directive if it's no longer needed after recent fixes
- Aligning error handling with the
init_productpattern (raise on failure instead of returningNone)- Adding a comment explaining why this directive is necessary
Based on past review comments and comparison with
seed/catalog/product.py.seed/accounts/licensee.py (1)
83-95: Improve robustness and lint-friendliness ininit_licenseecreation path.The creation flow looks correct, but there are two minor points:
pathlib.Path.open(icon, "rb")works, buticon.open("rb")is clearer and more idiomatic.- Ruff flags TRY003 on
raise ValueError("Licensee creation failed"); if TRY003 is enforced in CI, consider either using a dedicated exception type or moving the message to a constant to quiet the rule.Example tweaks:
- logger.debug("Creating licensee ...") - with pathlib.Path.open(icon, "rb") as icon_file: + logger.debug("Creating licensee ...") + with icon.open("rb") as icon_file: @@ - logger.warning("Licensee creation failed") - raise ValueError("Licensee creation failed") + logger.warning("Licensee creation failed") + raise ValueError("Licensee creation failed") # noqa: TRY003Or replace the
ValueErrorwith a small customLicenseeCreationErrorto keep messages in the exception class instead of the call site.seed/accounts/api_tokens.py (1)
51-67: Optionally guard thecreateresponse type ininit_api_token.You assume
mpt_ops.accounts.api_tokens.create()always returns anApiTokenand immediately access.id. If that assumption ever breaks, logging will crash before you see the underlying issue.Consider a lightweight check:
- api_token = await mpt_ops.accounts.api_tokens.create(api_token_data) - context.set_resource("accounts.api_token", api_token) - context["accounts.api_token.id"] = api_token.id - logger.info("API token created: %s", api_token.id) + api_token = await mpt_ops.accounts.api_tokens.create(api_token_data) + if not isinstance(api_token, ApiToken): + logger.error("API returned non-ApiToken on create") + raise TypeError("Unexpected API token create response type") + context.set_resource("accounts.api_token", api_token) + context["accounts.api_token.id"] = api_token.id + logger.info("API token created: %s", api_token.id)Not mandatory, but it will make any contract breakages much easier to debug.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
seed/accounts/logo.pngis excluded by!**/*.png
📒 Files selected for processing (24)
pyproject.toml(2 hunks)seed/accounts/accounts.py(1 hunks)seed/accounts/api_tokens.py(1 hunks)seed/accounts/buyer.py(1 hunks)seed/accounts/licensee.py(1 hunks)seed/accounts/module.py(1 hunks)seed/accounts/seller.py(1 hunks)seed/accounts/user_group.py(1 hunks)seed/catalog/product.py(2 hunks)seed/container.py(1 hunks)seed/seed_api.py(2 hunks)tests/e2e/accounts/buyers/conftest.py(1 hunks)tests/e2e/accounts/licensees/conftest.py(1 hunks)tests/e2e/accounts/sellers/conftest.py(1 hunks)tests/seed/accounts/test_accounts.py(1 hunks)tests/seed/accounts/test_api_token.py(1 hunks)tests/seed/accounts/test_buyer.py(1 hunks)tests/seed/accounts/test_licensee.py(1 hunks)tests/seed/accounts/test_module.py(1 hunks)tests/seed/accounts/test_seller.py(1 hunks)tests/seed/accounts/test_user_group.py(1 hunks)tests/seed/catalog/test_product.py(1 hunks)tests/seed/conftest.py(3 hunks)tests/seed/test_seed_api.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/e2e/accounts/licensees/conftest.py
- tests/seed/accounts/test_licensee.py
- tests/e2e/accounts/sellers/conftest.py
- tests/e2e/accounts/buyers/conftest.py
- tests/seed/accounts/test_buyer.py
- pyproject.toml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-27T00:05:54.701Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/accounts/accounts_users/test_async_accounts_users.py:0-0
Timestamp: 2025-11-27T00:05:54.701Z
Learning: In pytest-asyncio, async fixtures are automatically resolved before being passed to test functions (both sync and async). Test functions receive the yielded value from async fixtures, not coroutines, so fixture parameters should never be awaited. A sync test function can use async fixtures without any special handling.
Applied to files:
tests/seed/catalog/test_product.pytests/seed/conftest.py
🧬 Code graph analysis (11)
tests/seed/catalog/test_product.py (2)
tests/seed/conftest.py (2)
context(15-16)fake_file_factory(35-36)seed/catalog/product.py (1)
init_product(45-65)
seed/accounts/seller.py (3)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/sellers.py (1)
Seller(13-14)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)
tests/seed/accounts/test_seller.py (3)
mpt_api_client/resources/accounts/sellers.py (2)
AsyncSellersService(44-60)Seller(13-14)seed/accounts/seller.py (4)
build_seller_data(37-52)get_seller(16-33)init_seller(56-74)seed_seller(78-82)seed/context.py (2)
Context(12-33)get_resource(19-29)
tests/seed/accounts/test_module.py (4)
seed/accounts/module.py (3)
get_module(15-32)refresh_module(36-57)seed_module(61-72)tests/seed/conftest.py (2)
context(15-16)operations_client(25-26)seed/context.py (2)
Context(12-33)get_resource(19-29)mpt_api_client/models/model.py (1)
id(119-121)
seed/accounts/accounts.py (6)
seed/accounts/api_tokens.py (1)
seed_api_token(71-75)seed/accounts/buyer.py (1)
seed_buyer(90-94)seed/accounts/licensee.py (1)
seed_licensee(101-105)seed/accounts/module.py (1)
seed_module(61-72)seed/accounts/seller.py (1)
seed_seller(78-82)seed/accounts/user_group.py (1)
seed_user_group(80-85)
seed/catalog/product.py (4)
tests/seed/catalog/test_product.py (1)
product(17-18)mpt_api_client/resources/catalog/products.py (1)
Product(53-54)seed/context.py (1)
set_resource(31-33)mpt_api_client/models/model.py (1)
id(119-121)
seed/accounts/api_tokens.py (4)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/api_tokens.py (1)
ApiToken(12-13)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)mpt_api_client/models/model.py (1)
id(119-121)
tests/seed/conftest.py (1)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)
seed/seed_api.py (3)
seed/accounts/accounts.py (1)
seed_accounts(13-23)seed/context.py (1)
load_context(36-52)seed/catalog/catalog.py (1)
seed_catalog(32-39)
seed/accounts/user_group.py (8)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/user_groups.py (1)
UserGroup(11-12)tests/seed/conftest.py (1)
context(15-16)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)tests/e2e/conftest.py (1)
logger(63-64)tests/e2e/accounts/conftest.py (2)
user_group_id(30-31)module_id(35-36)tests/seed/accounts/test_user_group.py (1)
user_group(16-17)mpt_api_client/models/model.py (1)
id(119-121)
tests/seed/accounts/test_accounts.py (1)
seed/accounts/accounts.py (1)
seed_accounts(13-23)
🪛 Ruff (0.14.7)
seed/accounts/accounts.py
13-13: Unused noqa directive (unknown: WPS217)
Remove unused noqa directive
(RUF100)
seed/accounts/buyer.py
43-43: Avoid specifying long messages outside the exception class
(TRY003)
46-46: Avoid specifying long messages outside the exception class
(TRY003)
84-84: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/licensee.py
39-39: Unused noqa directive (unknown: WPS238)
Remove unused noqa directive
(RUF100)
45-45: Avoid specifying long messages outside the exception class
(TRY003)
48-48: Avoid specifying long messages outside the exception class
(TRY003)
51-51: Avoid specifying long messages outside the exception class
(TRY003)
54-54: Avoid specifying long messages outside the exception class
(TRY003)
95-95: Avoid specifying long messages outside the exception class
(TRY003)
seed/catalog/product.py
63-63: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/module.py
69-69: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/user_group.py
43-43: Avoid specifying long messages outside the exception class
(TRY003)
tests/seed/accounts/test_accounts.py
6-6: 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 (10)
tests/seed/test_seed_api.py (1)
23-39: Test correctly asserts newseed_accountsintegration.Patching
seed.seed_api.seed_accountsalongsideseed_catalogand asserting both are called once cleanly verifies the updated orchestration without over-specifying implementation details.seed/container.py (1)
48-68: Accounts seeding modules correctly wired into the container.Adding
seed.accountsand its submodules towire_containeraligns DI wiring with the new seeding workflow and should make the@injectusage in accounts seeders work as intended.seed/catalog/product.py (2)
45-65:init_productcreation path looks correct and aligns with DI pattern.Using
get_product(context=context, mpt_vendor=mpt_vendor)before creating, validatingcreatedwithisinstance(Product), and only then populating context provides a clear and safe flow for seeding the product.
69-81: Context-awareget_productusage inreview_productis consistent.Passing
contextandmpt_vendorthrough toget_productensures tests and callers using custom contexts/clients behave as expected while still caching refreshed products.tests/seed/conftest.py (1)
1-12: New file/fixture helpers are straightforward and useful for tests.
fake_fileandfake_file_factorygive a clean way to inject file-like objects into seed tests, andclient_clientfollows the same pattern as the other AsyncMPTClient mocks.Also applies to: 29-36
seed/seed_api.py (1)
20-26: LGTM! Parallel seeding implementation is clean.The parallel execution of catalog and accounts seeding via
asyncio.gatheris well-structured, with proper typing and error handling preserved.tests/seed/accounts/test_api_token.py (1)
1-80: LGTM! Comprehensive test coverage with correct async mocking.The test suite properly covers:
- Resource retrieval with context caching
- Initialization and creation flows
- Payload construction with environment variable handling
- Seed orchestration
The use of
AsyncMockfor async functions andmonkeypatchfor environment variables is appropriate.tests/seed/accounts/test_user_group.py (1)
1-93: LGTM! Tests properly handle async mocking and environment variables.The test suite correctly:
- Uses
AsyncMockfor async functions- Sets environment variables via
monkeypatchbefore they're needed- Validates context caching and resource storage
- Covers initialization, data building, and seeding flows
seed/accounts/licensee.py (1)
100-105:seed_licenseeflow looks good and consistent with other seeders.The function logs start/end and delegates all work to
init_licensee(), which keeps orchestration simple and consistent with the other seeding modules.seed/accounts/api_tokens.py (1)
70-75:seed_api_tokenorchestration looks correct.The seed function just logs and delegates to
init_api_token(), which keeps the orchestration simple and matches the pattern used in other seed modules.
robcsegal
force-pushed
the
MPT-15443-seed-data-required-for-accounts-e-2-e-tests
branch
from
7b9f708 to
def7b8f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (15)
seed/accounts/module.py (2)
27-31: Add type validation after APIgetcall.After fetching via
mpt_operations.accounts.modules.get(module_id), the code accessesmodule.idwithout confirming the result is aModule. This could raise anAttributeErrorif the API returns an unexpected type.Apply this diff:
if not isinstance(module, Module): module = await mpt_operations.accounts.modules.get(module_id) + if not isinstance(module, Module): + logger.warning("Unexpected type returned for module id %s", module_id) + return None context.set_resource("accounts.module", module) context["accounts.module.id"] = module.id return module
60-72: Ruff TRY003: Address theValueErrormessage pattern.Ruff flagged the
ValueErrorat Line 69 (TRY003). To keep the explicit message while satisfying the linter, add a suppression comment.- raise ValueError("Could not seed module: no valid Module found.") + raise ValueError("Could not seed module: no valid Module found.") # noqa: TRY003seed/accounts/user_group.py (3)
1-1: Remove mypy suppression and align return types.The
# mypy: disable-error-code=unreachablesuppression masks a type contract violation.init_user_groupcan returnNone(line 76), and the signature now correctly reflects-> UserGroup | None. The suppression appears unnecessary.-# mypy: disable-error-code=unreachable import logging
28-31: Add type validation after API fetch.When fetching from API on line 29, the result is used without verifying it's a valid
UserGroupinstance.if not isinstance(user_group, UserGroup): user_group = await mpt_operations.accounts.user_groups.get(user_group_id) + if not isinstance(user_group, UserGroup): + return None context.set_resource("accounts.user_group", user_group) context["accounts.user_group.id"] = user_group.id return user_group
44-51: Validatemodule_idbefore building user group payload.
context.get_string("accounts.module.id")returns an empty string if the key is missing. This would result in an invalid payload"modules": [{"id": ""}], causing an API error.module_id = context.get_string("accounts.module.id") + if not module_id: + raise ValueError( # noqa: TRY003 + "accounts.module.id missing from context; seed module before user group." + ) return {seed/accounts/accounts.py (1)
13-13: Remove unused# noqa: WPS217directive.Ruff reports this as an unused
noqa(RUF100). The function body is straightforward and doesn't trigger WPS217.-async def seed_accounts() -> None: # noqa: WPS217 +async def seed_accounts() -> None:seed/accounts/buyer.py (1)
30-34: Add type validation after API fetch.After fetching buyer via
mpt_operations.accounts.buyers.get(buyer_id), the code accessesbuyer.idwithout confirming the result is aBuyer. This follows the same pattern flagged in other seed modules.if not isinstance(buyer, Buyer): buyer = await mpt_operations.accounts.buyers.get(buyer_id) + if not isinstance(buyer, Buyer): + logger.warning("Unexpected type returned for buyer id %s", buyer_id) + return None context.set_resource("accounts.buyer", buyer) context["accounts.buyer.id"] = buyer.id return buyertests/seed/accounts/test_buyer.py (1)
36-61: Test will fail: missingCLIENT_ACCOUNT_IDenvironment variable.
build_buyer_datareadsCLIENT_ACCOUNT_IDfromos.getenv(), not from context. The test setscontext["accounts.account.id"]but never sets the environment variable, so the function will raiseValueError("CLIENT_ACCOUNT_ID environment variable is required").Apply this diff:
-def test_build_buyer_data(context: Context): - context["accounts.account.id"] = "ACC-1086-6867" +def test_build_buyer_data(context: Context, monkeypatch): + monkeypatch.setenv("CLIENT_ACCOUNT_ID", "ACC-1086-6867") context["accounts.seller.id"] = "SEL-9999-9999"tests/seed/accounts/test_accounts.py (1)
6-6: Remove unusednoqadirective.The
# noqa: WPS210suppression is flagged as unused by Ruff.Apply this diff:
-async def test_seed_accounts(mocker): # noqa: WPS210 +async def test_seed_accounts(mocker):seed/accounts/api_tokens.py (2)
27-31: Add type validation after APIgetcall.After fetching from the API (line 28), the code directly accesses
api_token.idwithout verifying the result is anApiToken. If the API returns an unexpected type, this will fail.Apply this diff:
if not isinstance(api_token, ApiToken): api_token = await mpt_ops.accounts.api_tokens.get(api_token_id) + if not isinstance(api_token, ApiToken): + logger.warning("API returned non-ApiToken type for id: %s", api_token_id) + return None context.set_resource("accounts.api_token", api_token) context["accounts.api_token.id"] = api_token.id return api_token
40-48: Validate required IDs before building payload.
CLIENT_ACCOUNT_IDmay be unset (returningNone), andaccounts.module.idmay be missing from context (returning an empty string). Using these without validation will produce invalid payloads like{"account": {"id": None}}or{"modules": [{"id": ""}]}, leading to confusing API errors.Apply this diff:
account_id = os.getenv("CLIENT_ACCOUNT_ID") module_id = context.get_string("accounts.module.id") + if not account_id: + raise ValueError("CLIENT_ACCOUNT_ID environment variable is required") # noqa: TRY003 + if not module_id: + raise ValueError("accounts.module.id missing from context; seed module first") # noqa: TRY003 return { "account": {"id": account_id},seed/accounts/licensee.py (2)
52-54: Fix unreachable None check for user group.
context.get_resource("accounts.user_group")either returns aModelor raisesValueError—it never returnsNone. Theif group is Nonecheck on lines 53-54 is unreachable, so your clearer error message will never be shown.Apply this diff to provide a helpful error when the user group is missing:
- 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 as exc: + raise ValueError( + "accounts.user_group missing from context; seed user group before licensee." + ) from exc # noqa: TRY003
30-34: Add type validation after APIgetcall.After fetching from the API (line 31), the code directly accesses
licensee.idwithout verifying the result is aLicensee. If the API returns an unexpected type, this will fail at runtime.Apply this diff:
if not isinstance(licensee, Licensee): licensee = await mpt_client.accounts.licensees.get(licensee_id) + if not isinstance(licensee, Licensee): + logger.warning("API returned non-Licensee type for id: %s", licensee_id) + return None context.set_resource("accounts.licensee", licensee) context["accounts.licensee.id"] = licensee.id return licenseeseed/catalog/product.py (1)
86-101: Propagatecontextintoget_productinpublish_productto avoid context desync
publish_productaccepts acontextparameter but still callsget_product()with no arguments (Line 92). This means it always usesDEFAULT_CONTEXTinstead of any explicitly passed context, which can desynchronize behavior compared toinit_product/review_productand makes tests using a customContextpotentially misleading.Consider aligning this with the other helpers:
async def publish_product( context: Context = DEFAULT_CONTEXT, mpt_operations: AsyncMPTClient = DEFAULT_MPT_OPERATIONS, ) -> Product | None: """Publish product if in reviewing status.""" - product = await get_product() + product = await get_product(context=context)This keeps retrieval aligned with the caller’s context while still relying on the default vendor client injection.
tests/seed/catalog/test_product.py (1)
1-1: DirectAsyncMockimport vs. project mocking conventionsThere’s a prior review preference in this project for relying on pytest’s
mockerfixture rather than importing fromunittest.mockdirectly. Please double‑check that keepingfrom unittest.mock import AsyncMockaligns with agreed conventions here; if not, consider switching to amocker-based approach and dropping this import.
🧹 Nitpick comments (16)
mpt_api_client/resources/commerce/assets.py (1)
24-40: Public rename may break consumers; consider BC aliases and docstring alignmentThe introduction of
AssetServiceConfigand the renaming toAssetService/AsyncAssetServicekeep behavior intact, but they do change the public class names. If external users were importingAssetsServiceConfig,AssetsService, orAsyncAssetsService, this will be a breaking change unless you’re planning a major version bump.If you want a softer transition, consider adding backwards‑compatibility aliases and marking them as deprecated, e.g.:
+ # Backwards‑compatibility aliases (deprecated; remove in a future major release) + AssetsServiceConfig = AssetServiceConfig + AssetsService = AssetService + AsyncAssetsService = AsyncAssetServiceAlso, the docstrings still say “Assets service config.” / “Assets service.” — you might optionally update them to match the new singular class names for clarity.
Also applies to: 66-75
tests/e2e/conftest.py (1)
110-113: Makelogo_fdpath independent of current working directoryUsing
pathlib.Path("tests/data/logo.png").resolve()assumes tests are always run with the project root as CWD; running fromtests/or other dirs will break this path. You already haveproject_root_path, so you can build an absolute, project-root–relative path and avoid depending on CWD.-@pytest.fixture -def logo_fd(): - file_path = pathlib.Path("tests/data/logo.png").resolve() - return file_path.open("rb") +@pytest.fixture +def logo_fd(project_root_path): + file_path = project_root_path / "tests" / "data" / "logo.png" + return file_path.open("rb")seed/accounts/buyer.py (2)
14-14: Relative path may fail depending on working directory.
pathlib.Path("seed/data/logo.png").resolve()is evaluated at module load time and assumes the working directory is the repository root. If the module is imported from a different location, this path resolution will fail.Consider using
__file__to construct a path relative to this module:-icon = pathlib.Path("seed/data/logo.png").resolve() +icon = pathlib.Path(__file__).parent.parent / "data" / "logo.png"
76-76: Remove unusednoqadirective.Ruff reports that
# noqa: PTH123is unused since the rule is not enabled.- with open(str(icon), "rb") as icon_file: # noqa: PTH123 + with open(str(icon), "rb") as icon_file:tests/seed/accounts/test_buyer.py (1)
74-74: Remove unusednoqadirective.Ruff reports
# noqa: WPS211as unused since WPS211 is not enabled.-async def test_init_buyer_create_new( # noqa: WPS211 +async def test_init_buyer_create_new(tests/seed/accounts/test_user_group.py (1)
41-41: Remove unusednoqadirective.Ruff flags the
# noqa: WPS211as unused.Apply this diff:
-async def test_init_user_group( # noqa: WPS211 +async def test_init_user_group(tests/seed/accounts/test_licensee.py (2)
85-85: Remove unusednoqadirectives.Both function definitions have
# noqa: WPS211suppressions flagged as unused by Ruff.Apply this diff:
-async def test_init_licensee( # noqa: WPS211 +async def test_init_licensee( context: Context, client_client, licensees_service, licensee, monkeypatch, mocker, fs ):-async def test_init_licensee_create_new( # noqa: WPS211 +async def test_init_licensee_create_new( context: Context, client_client, licensees_service, licensee, monkeypatch, fs, mocker ):Also applies to: 106-106
106-119: Remove unusedmockerparameter.The test doesn't patch any functions, so the
mockerfixture parameter is unused.Apply this diff:
async def test_init_licensee_create_new( - context: Context, client_client, licensees_service, licensee, monkeypatch, fs, mocker + context: Context, client_client, licensees_service, licensee, monkeypatch, fs ):seed/accounts/licensee.py (3)
39-39: Remove unusednoqadirective.The
# noqa: WPS238suppression is no longer needed and is flagged as unused by Ruff.Apply this diff:
-def build_licensee_data( # noqa: WPS238 +def build_licensee_data(
45-45: Consider suppressing TRY003 for descriptive error messages.Ruff flags several
ValueErrorraises with long messages (TRY003). Since these messages provide helpful context about missing environment variables and seeding order, you may want to suppress the rule rather than extract them to constants.Add
# noqa: TRY003to the affected raises:- raise ValueError("CLIENT_ACCOUNT_ID environment variable is required") + raise ValueError("CLIENT_ACCOUNT_ID environment variable is required") # noqa: TRY003- raise ValueError("Seller ID is required in context") + raise ValueError("Seller ID is required in context") # noqa: TRY003- raise ValueError("Buyer ID is required in context") + raise ValueError("Buyer ID is required in context") # noqa: TRY003- raise ValueError("Licensee creation failed") + raise ValueError("Licensee creation failed") # noqa: TRY003Also applies to: 48-48, 51-51, 95-95
87-87: Remove unusednoqadirective.The
# noqa: PTH123suppression is flagged as unused (the rule is not enabled).Apply this diff:
- with open(str(icon), "rb") as icon_file: # noqa: PTH123 + with open(str(icon), "rb") as icon_file:seed/catalog/product.py (1)
50-65: Init path looks solid; consider small lint/idiomatic cleanupsThe create-or-return logic and context caching look correct. Two minor improvements:
- Line 53: the
# noqa: PTH123is now unused; you can both drop it and use the more idiomaticPath.open:- logger.debug("Creating product ...") - with open(str(icon), "rb") as icon_file: # noqa: PTH123 + logger.debug("Creating product ...") + with icon.open("rb") as icon_file: created = await mpt_vendor.catalog.products.create( {"name": "E2E Seeded", "website": "https://www.example.com"}, file=icon_file )
- Lines 62–63: you already log
"Product creation failed"just before raising; to satisfy TRY003 and avoid duplicating the message, you can raise a bareValueError:- logger.warning("Product creation failed") - raise ValueError("Product creation failed") + logger.warning("Product creation failed") + raise ValueErrortests/seed/catalog/test_product.py (4)
43-55: Tidy up unusednoqaand consider avoiding hard‑coded icon pathThe create-new-product test looks good and uses
fs+AsyncMockin a straightforward way. Two minor cleanups:
- Line 43:
# noqa: WPS211is now reported as unused; it can be safely removed:-async def test_get_or_create_product_create_new( # noqa: WPS211 +async def test_get_or_create_product_create_new(
- Lines 48–50: to make the test less brittle to path changes, consider deriving the icon path from the module instead of hard‑coding the string. For example:
from seed.catalog import product as product_module fs.create_file(str(product_module.icon), contents=b"fake_icon_bytes")so the test follows whatever path
seed.catalog.productactually uses.
70-75: Optional: assert that review is not called for non‑draft statusesThe non‑draft test currently only checks that
review_product()returns the originalproduct. For extra safety and clearer intent, consider also asserting that the review API is not invoked (e.g., via a patchedproducts_service.review.assert_not_called()), so regressions that unnecessarily hit the API for non‑draft products are caught.
77-83: Strengthenpublishassertion with argument checkThis test correctly verifies that a
"Reviewing"product gets published and thatpublishis called once. To make it more intention‑revealing, you could also assert the call arguments:operations_client.catalog.products.publish.assert_called_once_with(product.id)This ensures we don’t accidentally change the call signature without tests failing.
86-90: Optional: ensure publish is skipped for non‑reviewing statusesSimilar to the non‑draft review test, this only checks that
publish_product()returns the originalproduct. If you want stronger guarantees, consider wiring in anoperations_client(or appropriate mock) here and asserting thatpublishis never called when status is not"Reviewing".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
seed/data/logo.pngis excluded by!**/*.pngtests/data/logo.pngis excluded by!**/*.pnguv.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
mpt_api_client/resources/commerce/assets.py(2 hunks)mpt_api_client/resources/commerce/commerce.py(3 hunks)pyproject.toml(3 hunks)seed/accounts/accounts.py(1 hunks)seed/accounts/api_tokens.py(1 hunks)seed/accounts/buyer.py(1 hunks)seed/accounts/licensee.py(1 hunks)seed/accounts/module.py(1 hunks)seed/accounts/seller.py(1 hunks)seed/accounts/user_group.py(1 hunks)seed/catalog/product.py(2 hunks)seed/container.py(1 hunks)seed/seed_api.py(2 hunks)tests/e2e/accounts/buyers/conftest.py(1 hunks)tests/e2e/accounts/licensees/conftest.py(1 hunks)tests/e2e/accounts/sellers/conftest.py(1 hunks)tests/e2e/conftest.py(1 hunks)tests/seed/accounts/test_accounts.py(1 hunks)tests/seed/accounts/test_api_token.py(1 hunks)tests/seed/accounts/test_buyer.py(1 hunks)tests/seed/accounts/test_licensee.py(1 hunks)tests/seed/accounts/test_module.py(1 hunks)tests/seed/accounts/test_seller.py(1 hunks)tests/seed/accounts/test_user_group.py(1 hunks)tests/seed/catalog/test_product.py(3 hunks)tests/seed/conftest.py(3 hunks)tests/seed/test_seed_api.py(2 hunks)tests/unit/resources/commerce/test_assets.py(1 hunks)tests/unit/resources/commerce/test_commerce.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- pyproject.toml
- tests/e2e/accounts/licensees/conftest.py
- tests/seed/test_seed_api.py
- seed/container.py
- tests/seed/accounts/test_seller.py
- seed/accounts/seller.py
- tests/e2e/accounts/sellers/conftest.py
- tests/seed/accounts/test_api_token.py
- seed/seed_api.py
- tests/e2e/accounts/buyers/conftest.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-27T00:05:36.356Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/conftest.py:0-0
Timestamp: 2025-11-27T00:05:36.356Z
Learning: When reviewing PRs that add fixtures to a parent conftest that may duplicate fixtures in child conftest files, do not suggest removing the duplicates from child conftest files that are outside the scope of the PR's changes.
Applied to files:
tests/e2e/conftest.py
📚 Learning: 2025-11-27T00:05:54.701Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/accounts/accounts_users/test_async_accounts_users.py:0-0
Timestamp: 2025-11-27T00:05:54.701Z
Learning: In pytest-asyncio, async fixtures are automatically resolved before being passed to test functions (both sync and async). Test functions receive the yielded value from async fixtures, not coroutines, so fixture parameters should never be awaited. A sync test function can use async fixtures without any special handling.
Applied to files:
tests/seed/conftest.pytests/seed/catalog/test_product.py
🧬 Code graph analysis (14)
tests/unit/resources/commerce/test_commerce.py (2)
mpt_api_client/resources/commerce/commerce.py (2)
assets(33-35)assets(60-62)mpt_api_client/resources/commerce/assets.py (2)
AssetService(32-63)AsyncAssetService(66-100)
tests/unit/resources/commerce/test_assets.py (3)
mpt_api_client/resources/commerce/commerce.py (2)
assets(33-35)assets(60-62)mpt_api_client/resources/commerce/assets.py (2)
AssetService(32-63)AsyncAssetService(66-100)tests/unit/conftest.py (2)
http_client(17-18)async_http_client(22-23)
seed/accounts/user_group.py (7)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/user_groups.py (1)
UserGroup(11-12)tests/seed/conftest.py (1)
context(15-16)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)tests/e2e/conftest.py (1)
logger(63-64)tests/e2e/accounts/conftest.py (2)
user_group_id(30-31)module_id(35-36)tests/seed/accounts/test_user_group.py (1)
user_group(16-17)
mpt_api_client/resources/commerce/commerce.py (1)
mpt_api_client/resources/commerce/assets.py (2)
AssetService(32-63)AsyncAssetService(66-100)
tests/seed/accounts/test_module.py (4)
mpt_api_client/resources/accounts/modules.py (2)
AsyncModulesService(29-32)Module(11-12)seed/accounts/module.py (3)
get_module(15-32)refresh_module(36-57)seed_module(61-72)seed/context.py (2)
Context(12-33)get_resource(19-29)mpt_api_client/models/model.py (1)
id(119-121)
tests/seed/accounts/test_user_group.py (4)
mpt_api_client/resources/accounts/user_groups.py (2)
AsyncUserGroupsService(32-38)UserGroup(11-12)seed/accounts/user_group.py (4)
build_user_group_data(37-52)get_user_group(16-33)init_user_group(56-76)seed_user_group(80-85)seed/context.py (2)
Context(12-33)get_resource(19-29)mpt_api_client/models/model.py (1)
id(119-121)
tests/seed/accounts/test_accounts.py (1)
seed/accounts/accounts.py (1)
seed_accounts(13-23)
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/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)mpt_api_client/models/model.py (1)
id(119-121)
seed/accounts/api_tokens.py (3)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/api_tokens.py (1)
ApiToken(12-13)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)
seed/catalog/product.py (5)
tests/seed/catalog/test_product.py (1)
product(17-18)tests/seed/conftest.py (1)
context(15-16)mpt_api_client/resources/catalog/products.py (1)
Product(53-54)seed/context.py (1)
set_resource(31-33)mpt_api_client/models/model.py (1)
id(119-121)
tests/seed/catalog/test_product.py (3)
tests/seed/conftest.py (3)
context(15-16)vendor_client(20-21)operations_client(25-26)seed/context.py (1)
Context(12-33)seed/catalog/product.py (5)
get_product(19-41)init_product(45-65)review_product(69-83)publish_product(87-101)seed_product(104-110)
seed/accounts/accounts.py (6)
seed/accounts/api_tokens.py (1)
seed_api_token(71-75)seed/accounts/buyer.py (1)
seed_buyer(90-94)seed/accounts/licensee.py (1)
seed_licensee(101-105)seed/accounts/module.py (1)
seed_module(61-72)seed/accounts/seller.py (1)
seed_seller(78-82)seed/accounts/user_group.py (1)
seed_user_group(80-85)
tests/seed/accounts/test_buyer.py (2)
seed/accounts/buyer.py (4)
build_buyer_data(39-63)get_buyer(18-35)init_buyer(67-86)seed_buyer(90-94)seed/context.py (2)
Context(12-33)get_resource(19-29)
tests/seed/accounts/test_licensee.py (3)
seed/accounts/licensee.py (4)
build_licensee_data(39-74)get_licensee(18-35)init_licensee(78-97)seed_licensee(101-105)tests/seed/conftest.py (2)
context(15-16)client_client(30-31)seed/context.py (3)
Context(12-33)get_resource(19-29)set_resource(31-33)
🪛 Ruff (0.14.7)
seed/accounts/user_group.py
43-43: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/licensee.py
39-39: Unused noqa directive (unknown: WPS238)
Remove unused noqa directive
(RUF100)
45-45: Avoid specifying long messages outside the exception class
(TRY003)
48-48: Avoid specifying long messages outside the exception class
(TRY003)
51-51: Avoid specifying long messages outside the exception class
(TRY003)
54-54: Avoid specifying long messages outside the exception class
(TRY003)
87-87: Unused noqa directive (non-enabled: PTH123)
Remove unused noqa directive
(RUF100)
95-95: Avoid specifying long messages outside the exception class
(TRY003)
tests/seed/accounts/test_user_group.py
41-41: Unused noqa directive (unknown: WPS211)
Remove unused noqa directive
(RUF100)
tests/seed/accounts/test_accounts.py
6-6: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
seed/accounts/buyer.py
43-43: Avoid specifying long messages outside the exception class
(TRY003)
46-46: Avoid specifying long messages outside the exception class
(TRY003)
76-76: Unused noqa directive (non-enabled: PTH123)
Remove unused noqa directive
(RUF100)
84-84: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/module.py
69-69: Avoid specifying long messages outside the exception class
(TRY003)
seed/catalog/product.py
53-53: Unused noqa directive (non-enabled: PTH123)
Remove unused noqa directive
(RUF100)
63-63: Avoid specifying long messages outside the exception class
(TRY003)
tests/seed/catalog/test_product.py
43-43: Unused noqa directive (unknown: WPS211)
Remove unused noqa directive
(RUF100)
seed/accounts/accounts.py
13-13: Unused noqa directive (unknown: WPS217)
Remove unused noqa directive
(RUF100)
tests/seed/accounts/test_buyer.py
74-74: Unused noqa directive (unknown: WPS211)
Remove unused noqa directive
(RUF100)
tests/seed/accounts/test_licensee.py
85-85: Unused noqa directive (unknown: WPS211)
Remove unused noqa directive
(RUF100)
106-106: Unused noqa directive (unknown: WPS211)
Remove unused noqa directive
(RUF100)
107-107: Unused function argument: mocker
(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 (10)
tests/unit/resources/commerce/test_assets.py (1)
6-16: Renamed asset service fixtures correctly aligned with new APIImporting
AssetService/AsyncAssetServiceand using them in the sync/async fixtures matches the updated assets module and keeps the tests consistent with the new public API surface.tests/unit/resources/commerce/test_commerce.py (1)
6-62: Commerce asset accessors now tested against the renamed service classesThe imports and parametrized expectations for the
assetsattribute (sync and async) correctly reflectAssetServiceandAsyncAssetService, matching the updatedCommerce/AsyncCommerceimplementations.mpt_api_client/resources/commerce/commerce.py (1)
3-36: Assets properties correctly updated to the renamed servicesThe sync and async
assetsproperties now import and returnAssetService/AsyncAssetServicewith the existinghttp_client, which is consistent with the updated assets module and the revised tests.Also applies to: 59-62
seed/accounts/accounts.py (1)
16-21: LGTM!The orchestration correctly sequences the seeding in the required dependency order (seller → buyer → module → api_token → user_group → licensee).
tests/seed/accounts/test_buyer.py (1)
86-88: Verify the fake file path matches the actual icon path.The test creates a file at
/mpt_api_client/seed/data/logo.png, butbuyer.pyresolves the path aspathlib.Path("seed/data/logo.png").resolve(). Depending on the pyfakefs configuration and working directory, this mismatch may cause the test to fail wheninit_buyertries to open the icon file.Please verify the test passes and the fake file path is correctly intercepted by pyfakefs. If not, you may need to create the file at the resolved absolute path or patch the
iconvariable directly.seed/catalog/product.py (1)
74-81: Explicitly passing context/vendor intoreview_productis correctUsing
await get_product(context=context, mpt_vendor=mpt_vendor)here keeps the behavior consistent withinit_productand avoids any divergence between the caller’s context/client and the defaults. No further issues spotted in this block.tests/seed/catalog/test_product.py (4)
26-35:test_get_productcorrectly exercises refresh + context cache pathThis test nicely covers the case where an ID exists in the context but no cached resource is present, ensuring the code falls back to
vendor_client.catalog.products.getand then stores the refreshedProductviaset_resource. Assertions on bothresultandcontext.get_resourcelook good.
37-41:test_get_product_without_idcleanly covers the “no id” branchThis test clearly verifies that
get_productreturnsNonewhen no product ID is present in theContext. The behavior and expectations are consistent with the implementation.
59-67: Draft review path is well coveredThis test correctly sets the product to
"Draft", ensuresget_productyields that object, and verifies thatreview_productcalls the underlyingproducts_service.reviewexactly once and returns the reviewed product. The flow matches the implementation.
93-100:test_seed_product_sequencecleanly verifies orchestration orderPatching
init_product,review_product, andpublish_productas async mocks and asserting each is called exactly once is a clear, low‑coupling way to verify the seeding pipeline wiring. This looks good and should be robust to internal changes in those helpers.
robcsegal
force-pushed
the
MPT-15443-seed-data-required-for-accounts-e-2-e-tests
branch
2 times, most recently
from
98bd303 to
4433c35
Compare
robcsegal
force-pushed
the
MPT-15443-seed-data-required-for-accounts-e-2-e-tests
branch
from
4433c35 to
7433ffc
Compare
robcsegal
force-pushed
the
MPT-15443-seed-data-required-for-accounts-e-2-e-tests
branch
from
7433ffc to
52f78a0
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/catalog/product.py (1)
92-92: Propagatecontextandmpt_operationstoget_product.Similar to the fix applied to
init_product(line 50) andreview_product(line 74),publish_productshould pass its injected dependencies toget_productinstead of using defaults.Apply this diff:
- product = await get_product() + product = await get_product(context=context, mpt_operations=mpt_operations)Note:
get_productexpectsmpt_vendorparameter, butpublish_productreceivesmpt_operations. Verify the parameter name matches the intended client.
♻️ Duplicate comments (10)
seed/accounts/accounts.py (1)
13-13: Remove unusednoqadirective.Ruff reports the
# noqa: WPS217directive is unused.Apply this diff:
-async def seed_accounts() -> None: # noqa: WPS217 +async def seed_accounts() -> None:tests/seed/accounts/test_accounts.py (1)
4-4: Remove unusednoqadirective.Ruff reports the
# noqa: WPS210directive is unused.Apply this diff:
-async def test_seed_accounts(mocker): # noqa: WPS210 +async def test_seed_accounts(mocker):seed/accounts/module.py (1)
27-31: Add type validation after API fetch.After fetching from the API at line 28, the code accesses
module.idat line 30 without validating that the result is aModule. This could raise anAttributeErrorif the API returns an unexpected type.Apply this diff:
if not isinstance(module, Module): module = await mpt_operations.accounts.modules.get(module_id) + if not isinstance(module, Module): + logger.warning("API returned non-Module type for id: %s", module_id) + return None context.set_resource("accounts.module", module) context["accounts.module.id"] = module.id return moduleseed/accounts/api_tokens.py (2)
27-31: Add type validation after API fetch.After fetching from the API at line 28, the code accesses
api_token.idat line 30 without validating that the result is anApiToken. This could raise anAttributeErrorif the API returns an unexpected type.Apply this diff:
if not isinstance(api_token, ApiToken): api_token = await mpt_ops.accounts.api_tokens.get(api_token_id) + if not isinstance(api_token, ApiToken): + logger.warning("API returned non-ApiToken type for id: %s", api_token_id) + return None context.set_resource("accounts.api_token", api_token) context["accounts.api_token.id"] = api_token.id return api_token
40-41: ValidateCLIENT_ACCOUNT_IDand module id before building payload.The function will construct a payload even if
CLIENT_ACCOUNT_IDis not set (line 40 returnsNone) or ifaccounts.module.idis missing from context (line 41 returns empty string). This can lead to confusing API errors or tokens with invalid ids.Apply this diff:
def build_api_token_data( context: Context = DEFAULT_CONTEXT, ) -> dict[str, object]: """Get API token data dictionary for creation.""" account_id = os.getenv("CLIENT_ACCOUNT_ID") module_id = context.get_string("accounts.module.id") + if not account_id: + raise ValueError("CLIENT_ACCOUNT_ID environment variable is not set.") + if not module_id: + raise ValueError("accounts.module.id missing from context; seed module first.") return { "account": {"id": account_id},seed/accounts/licensee.py (2)
30-34: Add type validation after API fetch.After fetching via
mpt_client.accounts.licensees.get(licensee_id), the code accesseslicensee.idwithout confirming the result is aLicensee. This is inconsistent with the defensive pattern used ininit_licensee(lines 89-95).if not isinstance(licensee, Licensee): licensee = await mpt_client.accounts.licensees.get(licensee_id) + if not isinstance(licensee, Licensee): + logger.warning("Unexpected type returned for licensee id %s", licensee_id) + return None context.set_resource("accounts.licensee", licensee) context["accounts.licensee.id"] = licensee.id return licensee
52-54: UnreachableNonecheck:get_resourceraises, never returnsNone.
context.get_resource("accounts.user_group")either returns aModelor raisesValueError—it never returnsNone. Theif group is Nonecheck is unreachable. Wrap the call in try/except to catch theValueErrorand provide a clearer error message about seeding order.- 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 as exc: + raise ValueError( + "accounts.user_group missing from context; seed user group before licensee." + ) from excseed/accounts/user_group.py (2)
44-52: Add validation formodule_idbefore building user group payload.
context.get_string("accounts.module.id")returns an empty string if the key is missing (default is""). This empty value would be used in the payload as"modules": [{"id": ""}], causing an API error. Add validation matching the pattern used foraccount_id.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") + if not module_id: + raise ValueError( + "accounts.module.id missing from context; seed module before user group." + ) return {
28-32: Add type validation after API fetch.After fetching via
mpt_operations.accounts.user_groups.get(user_group_id), the code accessesuser_group.idwithout confirming the result is aUserGroup. This is inconsistent with the defensive pattern used ininit_user_group(lines 69-76).if not isinstance(user_group, UserGroup): user_group = await mpt_operations.accounts.user_groups.get(user_group_id) + if not isinstance(user_group, UserGroup): + logger.warning("Unexpected type returned for user_group id %s", user_group_id) + return None context.set_resource("accounts.user_group", user_group) context["accounts.user_group.id"] = user_group.id return user_groupseed/accounts/buyer.py (1)
30-34: Add type validation after API fetch.After fetching via
mpt_operations.accounts.buyers.get(buyer_id), the code accessesbuyer.idwithout confirming the result is aBuyer. This is inconsistent with the defensive pattern used ininit_buyer(lines 78-84).if not isinstance(buyer, Buyer): buyer = await mpt_operations.accounts.buyers.get(buyer_id) + if not isinstance(buyer, Buyer): + logger.warning("Unexpected type returned for buyer id %s", buyer_id) + return None context.set_resource("accounts.buyer", buyer) context["accounts.buyer.id"] = buyer.id return buyer
🧹 Nitpick comments (11)
seed/catalog/product.py (1)
53-53: Remove unusednoqadirective.Ruff reports the
# noqa: PTH123directive is unused.Apply this diff:
- with open(str(icon), "rb") as icon_file: # noqa: PTH123 + with open(str(icon), "rb") as icon_file:seed/accounts/module.py (1)
69-69: Consider addingnoqafor TRY003.Ruff flags this line for specifying a long message outside the exception class. If the explicit message is important, you can suppress the linter.
Apply this diff:
- raise ValueError("Could not seed module: no valid Module found.") + raise ValueError("Could not seed module: no valid Module found.") # noqa: TRY003tests/seed/catalog/test_product.py (1)
41-41: Remove unusednoqadirective.Ruff reports the
# noqa: WPS211directive is unused.Apply this diff:
-async def test_get_or_create_product_create_new( # noqa: WPS211 +async def test_get_or_create_product_create_new( context: Context, vendor_client, products_service, product, fs, mocker ):tests/seed/accounts/test_buyer.py (1)
72-72: Remove unusednoqadirective.Ruff reports the
# noqa: WPS211directive is unused.Apply this diff:
-async def test_init_buyer_create_new( # noqa: WPS211 +async def test_init_buyer_create_new( context: Context, operations_client,tests/seed/accounts/test_user_group.py (1)
39-55: Remove unusednoqadirective.The
# noqa: WPS211directive is flagged as unused by Ruff (RUF100). The WPS211 rule is not recognized/enabled.-async def test_init_user_group( # noqa: WPS211 +async def test_init_user_group( context: Context, operations_client, user_groups_service, user_group, monkeypatch, mocker ):seed/accounts/licensee.py (2)
38-39: Remove unusednoqadirective.The
# noqa: WPS238directive is flagged as unused by Ruff (RUF100).@inject -def build_licensee_data( # noqa: WPS238 +def build_licensee_data( context: Context = DEFAULT_CONTEXT, ) -> dict[str, object]:
86-87: Remove unusednoqadirective.The
# noqa: PTH123directive is flagged as unused by Ruff (RUF100).- with open(str(icon), "rb") as icon_file: # noqa: PTH123 + with open(str(icon), "rb") as icon_file:seed/accounts/buyer.py (1)
75-76: Remove unusednoqadirective.The
# noqa: PTH123directive is flagged as unused by Ruff (RUF100).- with open(str(icon), "rb") as icon_file: # noqa: PTH123 + with open(str(icon), "rb") as icon_file:tests/seed/accounts/test_licensee.py (2)
83-84: Remove unusednoqadirective.The
# noqa: WPS211directive is flagged as unused by Ruff (RUF100).-async def test_init_licensee( # noqa: WPS211 +async def test_init_licensee( context: Context, client_client, licensees_service, licensee, monkeypatch, mocker, fs ):
106-108: Remove unusednoqadirective and unusedmockerargument.The
# noqa: WPS211directive is flagged as unused by Ruff (RUF100), andmockeris not used in this test function (ARG001).-async def test_init_licensee_create_new( # noqa: WPS211 - context: Context, client_client, licensees_service, licensee, monkeypatch, fs, mocker +async def test_init_licensee_create_new( + context: Context, client_client, licensees_service, licensee, monkeypatch, fs ):seed/accounts/user_group.py (1)
1-1: Consider removing the mypy suppression if no longer needed.The
# mypy: disable-error-code=unreachablesuppression at the module level may be masking legitimate type issues. Since return types are nowUserGroup | None, this suppression may no longer be necessary. Verify by removing it and running mypy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
seed/data/logo.pngis excluded by!**/*.pngtests/data/logo.pngis excluded by!**/*.pnguv.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
mpt_api_client/resources/commerce/assets.py(2 hunks)mpt_api_client/resources/commerce/commerce.py(3 hunks)pyproject.toml(3 hunks)seed/accounts/accounts.py(1 hunks)seed/accounts/api_tokens.py(1 hunks)seed/accounts/buyer.py(1 hunks)seed/accounts/licensee.py(1 hunks)seed/accounts/module.py(1 hunks)seed/accounts/seller.py(1 hunks)seed/accounts/user_group.py(1 hunks)seed/catalog/product.py(2 hunks)seed/container.py(1 hunks)seed/seed_api.py(2 hunks)tests/e2e/accounts/buyers/conftest.py(1 hunks)tests/e2e/accounts/licensees/conftest.py(1 hunks)tests/e2e/accounts/sellers/conftest.py(1 hunks)tests/e2e/conftest.py(1 hunks)tests/seed/accounts/test_accounts.py(1 hunks)tests/seed/accounts/test_api_token.py(1 hunks)tests/seed/accounts/test_buyer.py(1 hunks)tests/seed/accounts/test_licensee.py(1 hunks)tests/seed/accounts/test_module.py(1 hunks)tests/seed/accounts/test_seller.py(1 hunks)tests/seed/accounts/test_user_group.py(1 hunks)tests/seed/catalog/conftest.py(0 hunks)tests/seed/catalog/test_product.py(2 hunks)tests/seed/conftest.py(1 hunks)tests/seed/test_seed_api.py(2 hunks)tests/unit/resources/commerce/test_assets.py(1 hunks)tests/unit/resources/commerce/test_commerce.py(3 hunks)
💤 Files with no reviewable changes (1)
- tests/seed/catalog/conftest.py
🚧 Files skipped from review as they are similar to previous changes (15)
- tests/e2e/accounts/buyers/conftest.py
- tests/e2e/conftest.py
- tests/seed/accounts/test_api_token.py
- tests/e2e/accounts/licensees/conftest.py
- pyproject.toml
- seed/container.py
- tests/seed/accounts/test_module.py
- seed/accounts/seller.py
- tests/seed/accounts/test_seller.py
- tests/seed/conftest.py
- tests/unit/resources/commerce/test_assets.py
- tests/unit/resources/commerce/test_commerce.py
- tests/seed/test_seed_api.py
- mpt_api_client/resources/commerce/assets.py
- tests/e2e/accounts/sellers/conftest.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-27T00:05:54.701Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/accounts/accounts_users/test_async_accounts_users.py:0-0
Timestamp: 2025-11-27T00:05:54.701Z
Learning: In pytest-asyncio, async fixtures are automatically resolved before being passed to test functions (both sync and async). Test functions receive the yielded value from async fixtures, not coroutines, so fixture parameters should never be awaited. A sync test function can use async fixtures without any special handling.
Applied to files:
tests/seed/catalog/test_product.py
🧬 Code graph analysis (12)
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/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)tests/seed/accounts/test_buyer.py (1)
buyer(9-10)mpt_api_client/models/model.py (1)
id(119-121)
seed/accounts/accounts.py (6)
seed/accounts/api_tokens.py (1)
seed_api_token(71-75)seed/accounts/buyer.py (1)
seed_buyer(90-94)seed/accounts/licensee.py (1)
seed_licensee(101-105)seed/accounts/module.py (1)
seed_module(61-72)seed/accounts/seller.py (1)
seed_seller(78-82)seed/accounts/user_group.py (1)
seed_user_group(80-85)
seed/accounts/module.py (4)
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/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)
tests/seed/accounts/test_user_group.py (2)
seed/accounts/user_group.py (4)
build_user_group_data(37-52)get_user_group(16-33)init_user_group(56-76)seed_user_group(80-85)seed/context.py (2)
Context(12-33)get_resource(19-29)
seed/accounts/licensee.py (4)
mpt_api_client/resources/accounts/licensees.py (1)
Licensee(21-22)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)tests/seed/accounts/test_licensee.py (1)
licensee(10-28)mpt_api_client/models/model.py (1)
id(119-121)
seed/seed_api.py (4)
seed/accounts/accounts.py (1)
seed_accounts(13-23)seed/context.py (1)
load_context(36-52)tests/seed/conftest.py (1)
context(14-15)seed/catalog/catalog.py (1)
seed_catalog(32-39)
seed/accounts/api_tokens.py (3)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/api_tokens.py (1)
ApiToken(12-13)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)
tests/seed/catalog/test_product.py (1)
seed/catalog/product.py (5)
get_product(19-41)init_product(45-65)review_product(69-83)publish_product(87-101)seed_product(104-110)
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)tests/seed/conftest.py (1)
context(14-15)seed/context.py (4)
Context(12-33)get_string(15-17)get_resource(19-29)set_resource(31-33)
mpt_api_client/resources/commerce/commerce.py (2)
mpt_api_client/mpt_client.py (2)
commerce(49-51)commerce(98-105)mpt_api_client/resources/commerce/assets.py (2)
AssetService(32-63)AsyncAssetService(66-100)
tests/seed/accounts/test_licensee.py (2)
seed/accounts/licensee.py (4)
build_licensee_data(39-74)get_licensee(18-35)init_licensee(78-97)seed_licensee(101-105)seed/context.py (3)
Context(12-33)get_resource(19-29)set_resource(31-33)
tests/seed/accounts/test_accounts.py (1)
seed/accounts/accounts.py (1)
seed_accounts(13-23)
🪛 Ruff (0.14.7)
seed/accounts/buyer.py
43-43: Avoid specifying long messages outside the exception class
(TRY003)
46-46: Avoid specifying long messages outside the exception class
(TRY003)
76-76: Unused noqa directive (non-enabled: PTH123)
Remove unused noqa directive
(RUF100)
84-84: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/accounts.py
13-13: Unused noqa directive (unknown: WPS217)
Remove unused noqa directive
(RUF100)
tests/seed/accounts/test_buyer.py
72-72: Unused noqa directive (unknown: WPS211)
Remove unused noqa directive
(RUF100)
seed/accounts/module.py
69-69: Avoid specifying long messages outside the exception class
(TRY003)
tests/seed/accounts/test_user_group.py
39-39: Unused noqa directive (unknown: WPS211)
Remove unused noqa directive
(RUF100)
seed/accounts/licensee.py
39-39: Unused noqa directive (unknown: WPS238)
Remove unused noqa directive
(RUF100)
45-45: Avoid specifying long messages outside the exception class
(TRY003)
48-48: Avoid specifying long messages outside the exception class
(TRY003)
51-51: Avoid specifying long messages outside the exception class
(TRY003)
54-54: Avoid specifying long messages outside the exception class
(TRY003)
87-87: Unused noqa directive (non-enabled: PTH123)
Remove unused noqa directive
(RUF100)
95-95: Avoid specifying long messages outside the exception class
(TRY003)
seed/catalog/product.py
53-53: Unused noqa directive (non-enabled: PTH123)
Remove unused noqa directive
(RUF100)
63-63: Avoid specifying long messages outside the exception class
(TRY003)
tests/seed/catalog/test_product.py
41-41: Unused noqa directive (unknown: WPS211)
Remove unused noqa directive
(RUF100)
seed/accounts/user_group.py
43-43: Avoid specifying long messages outside the exception class
(TRY003)
tests/seed/accounts/test_licensee.py
83-83: Unused noqa directive (unknown: WPS211)
Remove unused noqa directive
(RUF100)
106-106: Unused noqa directive (unknown: WPS211)
Remove unused noqa directive
(RUF100)
107-107: Unused function argument: mocker
(ARG001)
tests/seed/accounts/test_accounts.py
4-4: 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 (18)
mpt_api_client/resources/commerce/commerce.py (3)
3-3: Import correctly updated to match renamed classes.The import aligns with the
AssetServiceandAsyncAssetServiceclass definitions inassets.py.
32-35: LGTM!The property correctly reflects the renamed
AssetServiceclass.
59-62: LGTM!The async property correctly mirrors the sync version with the renamed
AsyncAssetServiceclass.seed/catalog/product.py (1)
50-51: LGTM! Context propagation fixed.The previous issue with
init_productignoring itscontextandmpt_vendorparameters has been resolved. The parameters are now correctly passed toget_product.tests/seed/catalog/test_product.py (1)
20-21: LGTM! Test modernization complete.The tests have been successfully updated to use the
mockerfixture andpyfakefs(fs fixture), addressing previous review feedback. The consistent use ofmocker.patch,mocker.AsyncMock, andfs.create_fileimproves test reliability and maintainability.Also applies to: 46-52
seed/accounts/api_tokens.py (1)
57-60: LGTM! Context propagation fixed.The previous issue with
init_api_tokenignoring its injectedcontextandmpt_opsparameters has been resolved. Bothget_api_tokenandbuild_api_token_datanow receive the correct dependencies.seed/accounts/accounts.py (1)
16-21: LGTM! Clear orchestration pattern.The sequential seeding flow is well-structured and follows a clear pattern. The ordering ensures dependencies are seeded before dependent entities.
tests/seed/accounts/test_accounts.py (1)
5-33: LGTM! Proper use of mocker fixture.The test correctly uses the
mockerfixture withAsyncMockfor all async function patches, and validates that each seeder is called exactly once. This addresses previous review feedback.seed/seed_api.py (1)
20-26: LGTM! Concurrent seeding implementation.The refactor to run catalog and accounts seeding concurrently is well-implemented. The explicit type annotation and separate task creation improve code clarity.
tests/seed/accounts/test_buyer.py (1)
65-66: LGTM! Proper use of test fixtures.The tests correctly use the
mockerfixture,monkeypatchfor environment variables, andpyfakefs(fs) for file system operations. This addresses previous review feedback about using pytest fixtures consistently.Also applies to: 82-86, 92-93
tests/seed/accounts/test_user_group.py (2)
23-31: LGTM!The test correctly verifies that
get_user_groupfetches from the API when the ID is in context and properly stores the result in the context resource store.
58-72: LGTM!The test properly sets the
CLIENT_ACCOUNT_IDenvironment variable viamonkeypatchand validates the payload structure, addressing the previously flagged issue.seed/accounts/licensee.py (1)
77-97: LGTM!The
init_licenseefunction properly validates the creation result withisinstance(created, Licensee)before accessing its properties, following defensive programming patterns.seed/accounts/buyer.py (2)
38-63: LGTM!The
build_buyer_datafunction properly validates bothCLIENT_ACCOUNT_IDenvironment variable andseller_idfrom context before building the payload, following the established pattern.
66-86: LGTM!The
init_buyerfunction correctly forwards dependencies to helper functions and validates the creation result withisinstance(created, Buyer).tests/seed/accounts/test_licensee.py (2)
52-80: LGTM!The
test_build_licensee_datatest properly sets up the environment variable and all required context values (seller, buyer, user_group), matching the validation requirements inbuild_licensee_data.
122-127: LGTM!The
test_seed_licenseetest correctly usesmocker.AsyncMockto patchinit_licenseeand verifies it was awaited.seed/accounts/user_group.py (1)
55-76: LGTM!The
init_user_groupfunction properly validates the creation result withisinstance(created, UserGroup)and correctly returnsNoneon failure, which aligns with theUserGroup | Nonereturn type annotation.
|
robcsegal
deleted the
MPT-15443-seed-data-required-for-accounts-e-2-e-tests
branch
Seeded accounts e2e data
https://softwareone.atlassian.net/browse/MPT-15443
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.