MPT-16326 Refactor seeding accounts #158
Conversation
albertsola
requested review from
alephsur,
d3rky,
jentyk,
robcsegal,
ruben-sebrango and
svazquezco
as code owners
WalkthroughThis PR refactors the seed/accounts module to standardize resource initialization. It replaces previous get-or-create flows with deterministic create-only patterns using Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
albertsola
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (15)
tests/seed/accounts/test_accounts.py (1)
1-1: Remove unused import.The
osmodule is imported but not used in this file. Environment variable manipulation is handled viamonkeypatch.-import os - import pytestseed/accounts/accounts.py (2)
16-21: Remove unusednoqadirectives.Per static analysis,
RUF029is not enabled, making the directive unnecessary. Theasynckeyword is justified forinit_resourcecompatibility as noted.-async def get_account() -> Account: # noqa: RUF029 async for compatibility purposes with init_resource +async def get_account() -> Account: + """Get account ID from environment variable. + + Note: This function is async for compatibility with init_resource, + which expects an async factory callable. + """Moving the rationale into the docstring improves clarity without relying on a disabled linter rule.
24-24: Remove unusednoqadirective.The static analysis indicates
WPS213andWPS217are unknown rules (not enabled in this project).-async def seed_accounts() -> None: # noqa: WPS213 WPS217 +async def seed_accounts() -> None:tests/seed/accounts/test_seller.py (1)
47-52: Consider adding assertion on arguments.The test verifies
init_resourceis awaited but doesn't verify it's called with the correct arguments. For consistency withtest_seed_user_group, consider usingassert_awaited_once_with.async def test_seed_seller(mocker): init_resource = mocker.patch( "seed.accounts.seller.init_resource", new_callable=mocker.AsyncMock ) await seed_seller() # act - init_resource.assert_awaited_once() + init_resource.assert_awaited_once_with("accounts.seller.id", create_seller)tests/seed/accounts/test_api_tokens.py (2)
16-30: Potentially unnecessary environment variable setup.Line 17 sets
CLIENT_ACCOUNT_IDviamonkeypatch, butbuild_api_token_datareadsaccounts.account.idfrom the context (line 19), not from the environment. Themonkeypatch.setenvcall appears to be unnecessary unless there's another code path that reads it.def test_build_api_token_data(context, monkeypatch): - monkeypatch.setenv("CLIENT_ACCOUNT_ID", "ACC-1086-6867") context["accounts.module.id"] = "MOD-456" context["accounts.account.id"] = "ACC-1086-6867"
47-52: Consider adding assertion on arguments for consistency.Similar to
test_seed_user_group, consider verifyinginit_resourceis called with the expected arguments.async def test_seed_api_token(mocker): init_resource = mocker.patch( "seed.accounts.api_tokens.init_resource", new_callable=mocker.AsyncMock ) await seed_api_token() # act - init_resource.assert_awaited_once() + init_resource.assert_awaited_once_with("accounts.api_token", create_api_token)tests/seed/accounts/test_user_group.py (1)
17-32: Potentially unnecessary environment variable setup.Line 18 sets
CLIENT_ACCOUNT_IDviamonkeypatch, butbuild_user_group_datareadsaccounts.account.idfrom the context (line 20), not from the environment.def test_build_user_group_data(context: Context, monkeypatch): - monkeypatch.setenv("CLIENT_ACCOUNT_ID", "ACC-1086-6867") context["accounts.module.id"] = "MOD-456" context["accounts.account.id"] = "ACC-1086-6867"tests/seed/accounts/test_module.py (2)
8-9: Remove unusednoqadirectives.Ruff doesn't recognize
WPS110(a wemake-python-styleguide code). If wemake-python-styleguide is no longer part of the linting stack, these directives are unnecessary noise.- def __init__(self, items): # noqa: WPS110 - self._iterator = iter(items) # noqa: WPS110 + def __init__(self, items): + self._iterator = iter(items)
60-64: Consider asserting the arguments passed toinit_resource.The test verifies
init_resourcewas awaited, but doesn't validate the namespace or factory function arguments. This could miss regressions if the wrong namespace or factory is passed.async def test_seed_module(mocker): init_resource = mocker.patch( "seed.accounts.module.init_resource", new_callable=mocker.AsyncMock ) await seed_module() # act - init_resource.assert_awaited_once() + init_resource.assert_awaited_once_with("accounts.module.id", mocker.ANY)seed/accounts/module.py (1)
22-27: Avoid materializing all modules when only the first is needed.The current code collects all matching modules into a list before returning the first one. This is inefficient if there are many matches.
- modules = [module async for module in filtered.iterate()] - - if not modules: - raise ValueError("Module 'Access Management' not found.") - return modules[0] + async for module in filtered.iterate(): + return module + raise ValueError("Module 'Access Management' not found.")tests/seed/accounts/test_buyer.py (1)
51-54: Consider assertinginit_resourcearguments for better coverage.Similar to
test_seed_module, this test only verifiesinit_resourcewas called without checking the namespace or factory.async def test_seed_buyer(mocker): init_resource = mocker.patch("seed.accounts.buyer.init_resource", new_callable=mocker.AsyncMock) await seed_buyer() # act - init_resource.assert_awaited_once() + init_resource.assert_awaited_once_with("accounts.buyer.id", mocker.ANY)seed/accounts/licensee.py (1)
15-15: Remove unusednoqadirective.Ruff doesn't recognize
WPS238. If wemake-python-styleguide is not part of the linting stack, this directive is unnecessary.-def build_licensee_data( # noqa: WPS238 +def build_licensee_data(tests/seed/accounts/test_licensee.py (2)
31-37: Remove obsolete environment variable setup.Line 32 sets
CLIENT_ACCOUNT_IDenvironment variable, but the refactoredbuild_licensee_datafunction now usesrequire_context_id(context, "accounts.account.id", ...)to fetch the account ID from context (line 37), not from environment variables. This setup is now redundant.Similarly, line 34 sets the user_group resource object, but the new implementation only requires the ID string in context (line 33).
def test_build_licensee_data(context: Context, monkeypatch): - 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.account.id"] = "ACC-1086-6867"
74-79: Consider verifyinginit_resourcearguments for more precise testing.The test confirms
init_resourceis called but doesn't verify the correct namespace and factory are passed. A more precise assertion would catch regressions if the namespace string changes.async def test_seed_licensee(mocker): init_resource = mocker.patch( "seed.accounts.licensee.init_resource", new_callable=mocker.AsyncMock ) await seed_licensee() # act - init_resource.assert_awaited_once() + init_resource.assert_awaited_once_with("accounts.licensee.id", create_licensee)seed/accounts/user_group.py (1)
41-42: Inconsistent error message phrasing.The
actionparameter uses "creating user group" on line 41 but "create user group" on line 42. Use consistent phrasing for better error message clarity.account_id = require_context_id(context, "accounts.account.id", "creating user group") - module_id = require_context_id(context, "accounts.module.id", "create user group") + module_id = require_context_id(context, "accounts.module.id", "creating user group")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
seed/accounts/accounts.py(1 hunks)seed/accounts/api_tokens.py(2 hunks)seed/accounts/buyer.py(2 hunks)seed/accounts/licensee.py(2 hunks)seed/accounts/module.py(1 hunks)seed/accounts/seller.py(2 hunks)seed/accounts/user_group.py(3 hunks)seed/static/static.py(1 hunks)tests/seed/accounts/test_accounts.py(2 hunks)tests/seed/accounts/test_api_token.py(0 hunks)tests/seed/accounts/test_api_tokens.py(1 hunks)tests/seed/accounts/test_buyer.py(2 hunks)tests/seed/accounts/test_licensee.py(3 hunks)tests/seed/accounts/test_module.py(2 hunks)tests/seed/accounts/test_seller.py(2 hunks)tests/seed/accounts/test_user_group.py(3 hunks)
💤 Files with no reviewable changes (1)
- tests/seed/accounts/test_api_token.py
🧰 Additional context used
🧬 Code graph analysis (11)
tests/seed/accounts/test_api_tokens.py (4)
mpt_api_client/resources/accounts/api_tokens.py (1)
ApiToken(12-13)seed/accounts/api_tokens.py (3)
build_api_token_data(15-27)create_api_token(31-36)seed_api_token(40-44)tests/seed/conftest.py (2)
context(14-15)operations_client(24-25)seed/helper.py (1)
init_resource(44-84)
tests/seed/accounts/test_licensee.py (2)
seed/accounts/licensee.py (3)
build_licensee_data(15-44)create_licensee(47-49)seed_licensee(53-57)seed/helper.py (1)
init_resource(44-84)
tests/seed/accounts/test_buyer.py (4)
mpt_api_client/resources/accounts/buyers.py (1)
Buyer(26-27)seed/accounts/buyer.py (3)
build_buyer_data(15-35)create_buyer(39-46)seed_buyer(50-54)seed/context.py (1)
Context(12-33)seed/helper.py (1)
init_resource(44-84)
tests/seed/accounts/test_seller.py (4)
mpt_api_client/resources/accounts/sellers.py (1)
Seller(13-14)seed/accounts/seller.py (3)
build_seller_data(14-29)create_seller(32-37)seed_seller(40-44)tests/seed/conftest.py (1)
operations_client(24-25)seed/helper.py (1)
init_resource(44-84)
seed/accounts/accounts.py (2)
mpt_api_client/resources/accounts/account.py (1)
Account(27-28)seed/helper.py (1)
init_resource(44-84)
seed/accounts/licensee.py (2)
seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)seed/context.py (1)
Context(12-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/container.py (1)
Container(7-30)seed/helper.py (1)
init_resource(44-84)
tests/seed/accounts/test_user_group.py (4)
mpt_api_client/resources/accounts/user_groups.py (1)
UserGroup(11-12)seed/accounts/user_group.py (3)
build_user_group_data(37-50)create_user_group(53-58)seed_user_group(62-66)tests/seed/conftest.py (2)
context(14-15)operations_client(24-25)seed/helper.py (1)
init_resource(44-84)
seed/accounts/user_group.py (2)
seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)mpt_api_client/resources/accounts/user_groups.py (1)
UserGroup(11-12)
seed/accounts/buyer.py (3)
seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)seed/context.py (1)
Context(12-33)seed/container.py (1)
Container(7-30)
seed/accounts/module.py (3)
seed/helper.py (1)
init_resource(44-84)seed/container.py (1)
Container(7-30)mpt_api_client/resources/accounts/modules.py (1)
Module(11-12)
🪛 GitHub Actions: PR build and merge
tests/seed/accounts/test_accounts.py
[error] 1-1: Black formatting check failed: 1 file would be reformatted. Run 'black' to format the code.
🪛 Ruff (0.14.8)
tests/seed/accounts/test_module.py
8-8: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
9-9: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
tests/seed/accounts/test_accounts.py
23-23: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
seed/accounts/accounts.py
16-16: Unused noqa directive (non-enabled: RUF029)
Remove unused noqa directive
(RUF100)
20-20: Avoid specifying long messages outside the exception class
(TRY003)
24-24: Unused noqa directive (unknown: WPS213, WPS217)
Remove unused noqa directive
(RUF100)
seed/accounts/licensee.py
15-15: Unused noqa directive (unknown: WPS238)
Remove unused noqa directive
(RUF100)
seed/accounts/module.py
26-26: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (18)
seed/accounts/seller.py (2)
32-37: LGTM!The
create_sellerfunction is clean and follows the new pattern. It correctly uses dependency injection viaProvideand delegates to the API client for creation.
40-44: LGTM!The
seed_sellerfunction correctly delegates toinit_resourcewith the appropriate namespace and factory function.tests/seed/accounts/test_accounts.py (2)
9-20: LGTM!The tests for
get_accountproperly cover both the failure case (missing env var raisesValueError) and the success case (returnsAccountwith correct id).
23-23: Remove unusednoqadirective and fix formatting.The static analysis indicates
WPS210is unknown. Additionally, the pipeline shows a Black formatting failure for this file.-async def test_seed_accounts(mocker): # noqa: WPS210 +async def test_seed_accounts(mocker):Run
blackon this file to resolve the formatting issue.⛔ Skipped due to learnings
Learnt from: jentyk Repo: softwareone-platform/mpt-api-python-client PR: 136 File: tests/e2e/notifications/subscribers/conftest.py:14-25 Timestamp: 2025-12-01T10:48:52.586Z Learning: The repository softwareone-platform/mpt-api-python-client uses wemake-python-styleguide linter, so noqa directives for WPS rules should not be flagged as unused.seed/accounts/accounts.py (1)
27-34: LGTM!The seeding order is correct: account is seeded first via
init_resource, then downstream resources (seller, buyer, module, etc.) that depend onaccounts.account.idbeing present in context.tests/seed/accounts/test_seller.py (1)
36-44: LGTM!The test correctly verifies that
create_sellerdelegates to the API client and returns the expected seller object.tests/seed/accounts/test_api_tokens.py (1)
33-44: LGTM!The test properly mocks both the API client and
build_api_token_data, verifying the create flow works correctly.tests/seed/accounts/test_user_group.py (1)
35-53: LGTM!The tests properly verify
create_user_groupandseed_user_groupbehavior. Theassert_awaited_once_withpattern intest_seed_user_groupis a good practice that should be adopted in other similar tests.seed/accounts/buyer.py (3)
15-35: LGTM!The
build_buyer_datafunction correctly usesrequire_context_idto validate that bothaccounts.account.idandaccounts.seller.idare present in the context before building the buyer data.
49-54: LGTM!The
seed_buyerfunction correctly usesinit_resourcewith the appropriate namespace and factory function, consistent with the refactoring pattern across other seed modules.
38-46: Verify if the@injectdecorator is necessary forcreate_buyer.The
create_buyerfunction uses both@injectdecorator andProvide[]defaults. However,create_sellerandcreate_licenseesuccessfully useProvide[]defaults without@inject. Sinceinit_resourcecalls the factory with no arguments and dependency-injector resolvesProvide[]defaults during wiring, the decorator may be redundant here. Consider removing it for consistency with other create functions in the same module.seed/static/static.py (1)
5-5: Thelogo.pngfile exists in theseed/static/directory, so this constant definition is correct.tests/seed/accounts/test_module.py (1)
35-57: Test coverage forfind_modulelooks good.The tests correctly validate:
- Finding the first "Access Management" module when available
- Raising
ValueErrorwhen no module is foundseed/accounts/module.py (1)
30-37: LGTM!The
seed_modulefunction correctly usesinit_resourcewith the appropriate namespace and factory function.seed/accounts/api_tokens.py (1)
14-36: LGTM!The refactored
build_api_token_dataandcreate_api_tokenfunctions correctly userequire_context_idfor dependency validation and follow the new create pattern with proper DI wiring.tests/seed/accounts/test_licensee.py (1)
63-71: LGTM!Test correctly verifies that
create_licenseeinvokes the API client's create method. The mock setup is adequate for testing the integration.seed/accounts/user_group.py (2)
61-66: LGTM!The
seed_user_groupfunction correctly follows the newinit_resourcepattern for resource initialization. The implementation is clean and consistent with the refactoring approach.
36-50: LGTM!The
build_user_group_datafunction correctly usesrequire_context_idto validate required dependencies are present before building the data dictionary. The@injectdecorator ensures proper DI resolution.
albertsola
force-pushed
the
MPT-16326-Refactor-accounts-seeding
branch
from
af002fd to
161092f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
tests/seed/accounts/test_buyer.py (1)
41-48: Use consistent ID prefix for seller.Line 42 assigns an account-prefixed ID (
"ACC-1086-6867") toaccounts.seller.id, whiletest_build_buyer_dataat line 15 correctly uses a seller-prefixed ID ("SEL-9999-9999"). This inconsistency reduces test clarity.Apply this diff to use a seller prefix:
- context["accounts.seller.id"] = "ACC-1086-6867" + context["accounts.seller.id"] = "SEL-9999-9999"seed/accounts/api_tokens.py (1)
43-43: Inconsistent namespace: missing.idsuffix.Other seed functions use namespaces ending with
.id(e.g.,"accounts.buyer.id"at seed/accounts/buyer.py:53,"accounts.seller.id"at seed/accounts/seller.py:43), but this uses"accounts.api_token". This inconsistency could cause retrieval issues elsewhere in the codebase.Apply this diff:
- await init_resource("accounts.api_token", create_api_token) + await init_resource("accounts.api_token.id", create_api_token)seed/accounts/user_group.py (1)
53-58: Critical: Missing@injectdecorator will cause DI resolution failure.The
create_user_groupfunction usesProvide[Container.mpt_operations]as a default parameter but lacks the@injectdecorator. Whenseed_user_groupcallsinit_resource("accounts.user_group.id", create_user_group), the factory is invoked with no arguments. Without@inject, dependency-injector cannot resolve theProvidemarker, causingmpt_operationsto remain as an unresolvedProvideobject rather than the actual client instance.Based on seed/helper.py lines 43-83,
init_resourcecallsresource_factory()with no arguments, expecting DI to resolve defaults.Apply this diff:
+@inject async def create_user_group( mpt_operations: AsyncMPTClient = Provide[Container.mpt_operations], ) -> UserGroup:seed/accounts/licensee.py (2)
19-19: Incorrect action string in error message.The action string says "creating API token" but this function builds licensee data. This produces a confusing error if
accounts.account.idis missing.- account_id = require_context_id(context, "accounts.account.id", "creating API token") + account_id = require_context_id(context, "accounts.account.id", "create licensee")
47-49: Missing@injectdecorator oncreate_licensee.The function uses
Provide[Container.mpt_client]as a default parameter but lacks the@injectdecorator. Without it, dependency injection may not resolve the default value correctly at runtime.+@inject async def create_licensee(mpt_client: AsyncMPTClient = Provide[Container.mpt_client]) -> Licensee: """Create licensee.""" return await mpt_client.accounts.licensees.create(build_licensee_data())
🧹 Nitpick comments (6)
tests/seed/accounts/test_accounts.py (1)
23-23: Remove unused noqa directive.The
noqa: WPS210directive is flagged as unused by Ruff. Consider removing it to keep the code clean.Apply this diff:
-async def test_seed_accounts(mocker): # noqa: WPS210 +async def test_seed_accounts(mocker):seed/accounts/accounts.py (2)
16-16: Remove unused noqa directive.The
noqa: RUF029directive is flagged as unused (non-enabled rule) by Ruff. Consider removing it.Apply this diff:
-async def get_account() -> Account: # noqa: RUF029 async for compatibility purposes with init_resource +async def get_account() -> Account: # async for compatibility purposes with init_resource
24-24: Remove unused noqa directive.The
noqa: WPS213, WPS217directive is flagged as unused (unknown rules) by Ruff. Consider removing it.Apply this diff:
-async def seed_accounts() -> None: # noqa: WPS213 WPS217 +async def seed_accounts() -> None:seed/accounts/user_group.py (1)
16-33: Remove the unusedget_user_groupfunction.This function is not referenced anywhere in the codebase and appears to be legacy code from the old get-or-create pattern. Removing it will reduce maintenance burden.
seed/accounts/licensee.py (1)
15-15: Remove unusednoqadirective.The
# noqa: WPS238directive is no longer needed as flagged by Ruff.-def build_licensee_data( # noqa: WPS238 +def build_licensee_data(tests/seed/accounts/test_module.py (1)
8-9: Remove unusednoqadirectives.The
# noqa: WPS110directives are no longer needed as flagged by Ruff.- def __init__(self, items): # noqa: WPS110 - self._iterator = iter(items) # noqa: WPS110 + def __init__(self, items): + self._iterator = iter(items)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
seed/accounts/accounts.py(1 hunks)seed/accounts/api_tokens.py(2 hunks)seed/accounts/buyer.py(2 hunks)seed/accounts/licensee.py(2 hunks)seed/accounts/module.py(1 hunks)seed/accounts/seller.py(2 hunks)seed/accounts/user_group.py(3 hunks)seed/static/static.py(1 hunks)tests/seed/accounts/test_accounts.py(2 hunks)tests/seed/accounts/test_api_token.py(0 hunks)tests/seed/accounts/test_api_tokens.py(1 hunks)tests/seed/accounts/test_buyer.py(2 hunks)tests/seed/accounts/test_licensee.py(3 hunks)tests/seed/accounts/test_module.py(2 hunks)tests/seed/accounts/test_seller.py(2 hunks)tests/seed/accounts/test_user_group.py(3 hunks)
💤 Files with no reviewable changes (1)
- tests/seed/accounts/test_api_token.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/seed/accounts/test_user_group.py
- tests/seed/accounts/test_api_tokens.py
🧰 Additional context used
🧬 Code graph analysis (9)
seed/accounts/module.py (4)
seed/helper.py (1)
init_resource(44-84)mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)seed/container.py (1)
Container(7-30)mpt_api_client/resources/accounts/modules.py (1)
Module(11-12)
tests/seed/accounts/test_accounts.py (3)
mpt_api_client/resources/accounts/account.py (1)
Account(27-28)seed/accounts/accounts.py (2)
get_account(16-21)seed_accounts(24-36)mpt_api_client/models/model.py (1)
id(119-121)
seed/accounts/user_group.py (1)
seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)
seed/accounts/seller.py (4)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/sellers.py (1)
Seller(13-14)seed/container.py (1)
Container(7-30)seed/helper.py (1)
init_resource(44-84)
tests/seed/accounts/test_licensee.py (4)
mpt_api_client/resources/accounts/licensees.py (1)
Licensee(21-22)seed/accounts/licensee.py (2)
build_licensee_data(15-44)create_licensee(47-49)tests/seed/conftest.py (2)
context(14-15)operations_client(24-25)seed/helper.py (1)
init_resource(44-84)
seed/accounts/api_tokens.py (3)
seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)seed/context.py (1)
Context(12-33)seed/container.py (1)
Container(7-30)
seed/accounts/buyer.py (1)
seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)
seed/accounts/licensee.py (3)
seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)seed/context.py (1)
Context(12-33)seed/container.py (1)
Container(7-30)
tests/seed/accounts/test_module.py (3)
seed/accounts/module.py (2)
find_module(15-27)seed_module(31-38)tests/seed/conftest.py (1)
operations_client(24-25)seed/helper.py (1)
init_resource(44-84)
🪛 GitHub Actions: PR build and merge
tests/seed/accounts/test_accounts.py
[error] 1-1: Would reformat: tests/seed/accounts/test_accounts.py. 1 file would be reformatted.
🪛 Ruff (0.14.8)
seed/accounts/module.py
26-26: Avoid specifying long messages outside the exception class
(TRY003)
tests/seed/accounts/test_accounts.py
23-23: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
seed/accounts/accounts.py
16-16: Unused noqa directive (non-enabled: RUF029)
Remove unused noqa directive
(RUF100)
20-20: Avoid specifying long messages outside the exception class
(TRY003)
24-24: Unused noqa directive (unknown: WPS213, WPS217)
Remove unused noqa directive
(RUF100)
seed/accounts/licensee.py
15-15: Unused noqa directive (unknown: WPS238)
Remove unused noqa directive
(RUF100)
tests/seed/accounts/test_module.py
8-8: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
9-9: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (19)
seed/static/static.py (1)
5-5: LGTM!The logo path update is straightforward and aligns the constant with the actual file name.
seed/accounts/buyer.py (4)
9-9: LGTM!The addition of
init_resourceandrequire_context_idhelpers aligns with the PR's refactoring objective to standardize resource initialization across the codebase.
17-18: LGTM!The use of
require_context_idensures that required IDs are validated before creating the buyer, providing clear error messages if dependencies are missing.
38-46: LGTM!The refactored
create_buyerfunction correctly uses the@injectdecorator and follows the new pattern of direct resource creation throughinit_resource.
53-53: LGTM!The seed function now leverages
init_resourcefor idempotent resource initialization, consistent with the refactoring pattern across other account seeders.tests/seed/accounts/test_accounts.py (1)
8-20: LGTM!The new tests for
get_accountcorrectly verify both the failure case (missing env var) and success case (env var present), ensuring proper validation of theCLIENT_ACCOUNT_IDenvironment variable.seed/accounts/accounts.py (2)
16-21: LGTM!The
get_accountfunction correctly retrieves and validates theCLIENT_ACCOUNT_IDenvironment variable, providing a clear error message when it's missing. The async signature is appropriate for compatibility withinit_resource.
27-27: LGTM!The integration of
init_resourceto seed the account ID is consistent with the refactoring pattern applied across all account seeders.tests/seed/accounts/test_seller.py (2)
36-44: LGTM!The test correctly validates the new
create_sellerflow by mocking the async create method and verifying the return value.
47-52: LGTM!The test properly validates that
seed_sellerdelegates toinit_resourceas expected in the new seeding pattern.seed/accounts/module.py (3)
14-27: LGTM!The
find_modulefunction correctly uses dependency injection with@injectand properly handles the not-found case by raising a descriptiveValueError.
30-37: LGTM!The
seed_modulefunction follows the newinit_resourcepattern consistently with other seeders in this refactor.
14-27: Missing@injectdecorator onfind_module.The function uses
Provide[Container.mpt_operations]as a default parameter but lacks the@injectdecorator. Without it, dependency injection may not resolve the default value correctly at runtime. Other functions in this file and related files (e.g.,seed_moduleat line 30) properly include@inject.Apply this diff:
@inject async def find_module( mpt_operations: AsyncMPTClient = Provide[Container.mpt_operations], ) -> Module:Wait—
@injectis already present at line 14. My initial read was incorrect. The decorator applies tofind_module.seed/accounts/licensee.py (1)
52-57: LGTM!The
seed_licenseefunction correctly follows the newinit_resourcepattern.tests/seed/accounts/test_module.py (2)
35-57: LGTM!The tests properly cover both the success case and the not-found error case for
find_module, with appropriate assertions and mocking.
60-65: LGTM!The test correctly validates that
seed_moduledelegates toinit_resource.tests/seed/accounts/test_licensee.py (3)
63-71: LGTM!The test correctly validates the
create_licenseeflow by mocking the create method and patchingbuild_licensee_data.
74-79: LGTM!The test properly validates that
seed_licenseedelegates toinit_resourceas expected in the new seeding pattern.
31-60: LGTM!The test correctly sets up all required context IDs (
accounts.account.id,accounts.seller.id,accounts.buyer.id,accounts.user_group.id) and validates the expected licensee data structure.
albertsola
force-pushed
the
MPT-16326-Refactor-accounts-seeding
branch
3 times, most recently
from
b562336 to
3fbc11f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
seed/accounts/licensee.py (2)
20-20: Incorrect action string in error message.The action string says "creating API token" but this function builds licensee data. This would produce a confusing error message if
accounts.account.idis missing.Apply this diff:
- account_id = require_context_id(context, "accounts.account.id", "creating API token") + account_id = require_context_id(context, "accounts.account.id", "create licensee")
47-51: Critical: Missing@injectdecorator oncreate_licensee.The function uses
Provide[Container.mpt_client]as a default parameter but lacks the@injectdecorator. Without it, dependency injection will not resolve the default value correctly at runtime, causingmpt_clientto remain as an unresolvedProvideobject instead of anAsyncMPTClientinstance.Apply this diff:
+@inject async def create_licensee(mpt_client: AsyncMPTClient = Provide[Container.mpt_client]) -> Licensee: """Create licensee.""" licensee_data = build_licensee_data()
🧹 Nitpick comments (6)
seed/accounts/licensee.py (1)
16-16: Remove unused noqa directive.The
noqa: WPS238directive is not recognized by the active linters.Based on static analysis hints.
tests/seed/accounts/test_accounts.py (1)
22-56: LGTM: Comprehensive seed orchestration test.The test properly verifies that all seeding functions and
init_resourceare called once during theseed_accountsflow.Note: The
noqa: WPS210directive is unused and can be removed. Based on static analysis hints.seed/accounts/accounts.py (2)
16-21: LGTM: Environment-based account retrieval.The function correctly reads the account ID from the environment variable and validates its presence. The error message is clear and actionable.
Note: The
noqa: RUF029directive appears to be unused or references a non-enabled rule. Based on static analysis hints.
24-36: LGTM: Consolidated seeding orchestration.The function correctly sequences the account seeding flow, initializing the account resource first before seeding dependent entities.
Note: The
noqa: WPS213 WPS217directives are unused and can be removed. Based on static analysis hints.tests/seed/accounts/test_seller.py (1)
47-54: LGTM: Updated to test init_resource pattern.Correctly verifies that
seed_sellerdelegates toinit_resourcefor resource initialization.Consider asserting the specific arguments passed to
init_resource(namespace and factory) for more complete verification, similar to the pattern intest_user_group.pyline 53.tests/seed/accounts/test_module.py (1)
8-9: Remove unusednoqadirectives.The
noqadirectives forWPS110are not recognized by Ruff and should be removed to keep the code clean.Apply this diff:
- def __init__(self, items): # noqa: WPS110 - self._iterator = iter(items) # noqa: WPS110 + def __init__(self, items): + self._iterator = iter(items)As per static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
docker-compose.yml(1 hunks)seed/accounts/accounts.py(1 hunks)seed/accounts/api_tokens.py(2 hunks)seed/accounts/buyer.py(2 hunks)seed/accounts/licensee.py(2 hunks)seed/accounts/module.py(1 hunks)seed/accounts/seller.py(2 hunks)seed/accounts/user_group.py(2 hunks)seed/static/static.py(1 hunks)tests/seed/accounts/test_accounts.py(2 hunks)tests/seed/accounts/test_api_token.py(0 hunks)tests/seed/accounts/test_api_tokens.py(1 hunks)tests/seed/accounts/test_buyer.py(2 hunks)tests/seed/accounts/test_licensee.py(3 hunks)tests/seed/accounts/test_module.py(2 hunks)tests/seed/accounts/test_seller.py(2 hunks)tests/seed/accounts/test_user_group.py(3 hunks)
💤 Files with no reviewable changes (1)
- tests/seed/accounts/test_api_token.py
🚧 Files skipped from review as they are similar to previous changes (2)
- seed/static/static.py
- tests/seed/accounts/test_api_tokens.py
🧰 Additional context used
🧬 Code graph analysis (10)
seed/accounts/accounts.py (3)
mpt_api_client/resources/accounts/account.py (1)
Account(27-28)seed/accounts/buyer.py (1)
seed_buyer(49-53)seed/helper.py (1)
init_resource(44-84)
seed/accounts/seller.py (4)
seed/helper.py (1)
init_resource(44-84)mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)seed/container.py (1)
Container(7-30)mpt_api_client/resources/accounts/sellers.py (1)
Seller(13-14)
tests/seed/accounts/test_module.py (3)
seed/accounts/module.py (2)
find_module(15-27)seed_module(30-34)tests/seed/conftest.py (1)
operations_client(24-25)seed/helper.py (1)
init_resource(44-84)
seed/accounts/user_group.py (4)
seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)seed/context.py (1)
Context(12-33)seed/container.py (1)
Container(7-30)mpt_api_client/resources/accounts/user_groups.py (1)
UserGroup(11-12)
tests/seed/accounts/test_user_group.py (2)
seed/accounts/user_group.py (3)
build_user_group_data(16-29)create_user_group(33-38)seed_user_group(41-45)seed/helper.py (1)
init_resource(44-84)
tests/seed/accounts/test_licensee.py (4)
mpt_api_client/resources/accounts/licensees.py (1)
Licensee(21-22)seed/accounts/licensee.py (3)
build_licensee_data(16-44)create_licensee(47-51)seed_licensee(54-58)tests/seed/conftest.py (2)
context(14-15)operations_client(24-25)seed/helper.py (1)
init_resource(44-84)
seed/accounts/api_tokens.py (2)
seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)seed/context.py (1)
Context(12-33)
seed/accounts/licensee.py (3)
seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)seed/context.py (1)
Context(12-33)mpt_api_client/resources/accounts/licensees.py (1)
Licensee(21-22)
seed/accounts/module.py (4)
seed/helper.py (1)
init_resource(44-84)seed/container.py (1)
Container(7-30)mpt_api_client/resources/accounts/modules.py (1)
Module(11-12)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)
tests/seed/accounts/test_accounts.py (3)
mpt_api_client/resources/accounts/account.py (1)
Account(27-28)seed/accounts/accounts.py (2)
get_account(16-21)seed_accounts(24-36)mpt_api_client/models/model.py (1)
id(119-121)
🪛 Ruff (0.14.8)
seed/accounts/accounts.py
16-16: Unused noqa directive (non-enabled: RUF029)
Remove unused noqa directive
(RUF100)
20-20: Avoid specifying long messages outside the exception class
(TRY003)
24-24: Unused noqa directive (unknown: WPS213, WPS217)
Remove unused noqa directive
(RUF100)
tests/seed/accounts/test_module.py
8-8: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
9-9: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
seed/accounts/licensee.py
16-16: Unused noqa directive (unknown: WPS238)
Remove unused noqa directive
(RUF100)
seed/accounts/module.py
26-26: Avoid specifying long messages outside the exception class
(TRY003)
tests/seed/accounts/test_accounts.py
22-22: 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 (28)
docker-compose.yml (1)
30-30: LGTM: Extended test coverage to include seed tests.The addition of
tests/seedto the pytest command correctly aligns with the new seed-related test files introduced in this PR.seed/accounts/module.py (2)
15-27: LGTM: Module discovery via API filtering.The implementation correctly filters modules by name and returns the first match. The error handling appropriately raises
ValueErrorwhen the module is not found.
30-34: LGTM: Clean resource initialization pattern.The seeding flow correctly delegates to
init_resource, which will cache the module ID in the context for downstream use.tests/seed/accounts/test_accounts.py (2)
7-11: LGTM: Proper error handling test.Correctly verifies that
get_accountraisesValueErrorwhen the required environment variable is missing.
14-19: LGTM: Successful path test.Correctly verifies that
get_accountreturns anAccountinstance with the expected ID from the environment variable.tests/seed/accounts/test_user_group.py (3)
17-32: LGTM: Test data updated for new context requirements.Correctly sets
accounts.account.idin the context to align with the refactoredbuild_user_group_datathat usesrequire_context_id.
35-43: LGTM: Tests new create_user_group function.Properly verifies that
create_user_groupcalls the API create method with the expected data.
46-53: LGTM: Updated to test init_resource pattern.Correctly verifies that
seed_user_groupdelegates toinit_resourcewith the appropriate namespace and factory function.tests/seed/accounts/test_seller.py (1)
36-44: LGTM: Tests new create_seller function.Properly verifies that
create_sellercalls the API create method and returns the expected seller.seed/accounts/seller.py (2)
32-38: LGTM: Properly decorated create function.The
@injectdecorator is present and correctly enables dependency injection for thempt_operationsparameter. The function cleanly creates and returns a seller.
41-45: LGTM: Clean resource initialization pattern.The seeding flow correctly delegates to
init_resource, which will cache the seller ID in the context for downstream use.tests/seed/accounts/test_module.py (3)
35-45: LGTM!The test correctly validates that
find_moduleretrieves the expected module using the operations client.
48-57: LGTM!The test properly verifies that
find_moduleraisesValueErrorwhen no module is found.
60-67: LGTM!The test correctly mocks
init_resourceand verifies the seeding flow.seed/accounts/buyer.py (3)
17-18: LGTM!The use of
require_context_idproperly enforces that required dependencies (account and seller) are available before creating the buyer.
39-46: LGTM!The refactored
create_buyerfunction is clean and follows the new pattern of directly creating resources without context-based retrieval logic.
49-53: LGTM!The seeding flow correctly delegates to
init_resource, which handles caching and idempotency.tests/seed/accounts/test_buyer.py (2)
41-48: LGTM!The test correctly validates the
create_buyerfunction, and the seller_id now uses the proper "SEL-" prefix consistently withtest_build_buyer_data. The past review concern about inconsistent ID formats has been resolved.
51-56: LGTM!The test properly mocks
init_resourceand verifies the seed flow.seed/accounts/api_tokens.py (3)
19-20: LGTM!The use of
require_context_idproperly enforces that required dependencies are available before creating the API token.
30-36: LGTM!The
@injectdecorator is present and thecreate_api_tokenfunction is correctly structured. The past review concern about the missing decorator has been resolved.
39-43: LGTM!The namespace
"accounts.api_token.id"is now consistent with other seed functions (includes the.idsuffix). The past review concern about inconsistent namespace has been resolved.tests/seed/accounts/test_licensee.py (3)
31-60: LGTM!The test correctly sets up all required context IDs (including the newly added
accounts.account.idon line 37) and validates thatbuild_licensee_dataproduces the expected output.
63-71: LGTM!The test appropriately validates the
create_licenseefunction by mocking the creation call and verifying it's awaited.
74-81: LGTM!The test correctly mocks
init_resourceand verifies the seed flow.seed/accounts/user_group.py (3)
20-21: LGTM!The use of
require_context_idproperly enforces that required dependencies are available before creating the user group.
32-38: LGTM!The
@injectdecorator is present and thecreate_user_groupfunction is correctly structured. The past review concern about the missing decorator has been resolved.
41-45: LGTM!The seeding flow correctly delegates to
init_resource, which handles caching and idempotency.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
seed/accounts/accounts.py (1)
2-36: Tidyget_account/seed_accounts: docstring & unnecessarynoqacleanupA few small polish points:
- Docstring accuracy
get_accountreturns anAccountinstance, not just an ID. Update the docstring:-async def get_account() -> Account: # noqa: RUF029 async for compatibility purposes with init_resource - """Get account ID from environment variable.""" +async def get_account() -> Account: + """Get account from environment variable. + + Async for compatibility with ``init_resource``. + """
- Remove unnecessary
noqafor WPS rulesThe
# noqa: WPS213 WPS217onseed_accountsis unnecessary. WPS rules are handled by flake8 (wemake-python-styleguide), not Ruff, so thisnoqacomment won't suppress Ruff warnings. Remove it:-async def seed_accounts() -> None: # noqa: WPS213 WPS217 +async def seed_accounts() -> None:The RUF029
noqaonget_accountis justified since the function is intentionally async for API compatibility, so it can remain if needed; however, since RUF029 is not enabled in the linter config, you may remove it along with the comment for clarity.
🧹 Nitpick comments (5)
tests/seed/accounts/test_buyer.py (1)
51-56: Optionally tighten assertion oninit_resourcecall
test_seed_buyercurrently only checks thatinit_resourcewas awaited once. If you want slightly stronger protection against regressions, consider also asserting the namespace:- init_resource.assert_awaited_once() + init_resource.assert_awaited_once_with("accounts.buyer.id", mocker.ANY)This keeps the test resilient to internal changes while ensuring the expected context key is used.
tests/seed/accounts/test_seller.py (1)
3-54: Seller tests correctly exercise builder, creator, and seed flowThe seller tests are in good shape:
test_build_seller_dataverifies the exact payload including the providedexternalId.test_create_sellerwires anAsyncMockonoperations_client.accounts.sellers.createand ensurescreate_sellerreturns thatSeller.test_seed_sellerconfirmsinit_resourceis awaited once when runningseed_seller.If you want slightly stricter coverage, you could also assert the namespace in
test_seed_seller:- init_resource.assert_awaited_once() + init_resource.assert_awaited_once_with("accounts.seller.id", mocker.ANY)But as-is, the tests already validate the new flow well.
tests/seed/accounts/test_module.py (1)
7-18: Remove unusednoqadirectives.The
WPS110rule is from wemake-python-styleguide which doesn't appear to be active in this project. Ruff correctly flags these as unused.class DummyAsyncIterator: - def __init__(self, items): # noqa: WPS110 - self._iterator = iter(items) # noqa: WPS110 + def __init__(self, items): + self._iterator = iter(items) def __aiter__(self): return selfseed/accounts/licensee.py (2)
20-23: Inconsistent action string format.The action strings use inconsistent verb forms:
"creating licensee"on line 20 vs"create licensee"on lines 21-23. Consider normalizing for consistency.- account_id = require_context_id(context, "accounts.account.id", "creating licensee") - seller_id = require_context_id(context, "accounts.seller.id", "create licensee") - buyer_id = require_context_id(context, "accounts.buyer.id", "create licensee") - user_group_id = require_context_id(context, "accounts.user_group.id", "create licensee") + account_id = require_context_id(context, "accounts.account.id", "create licensee") + seller_id = require_context_id(context, "accounts.seller.id", "create licensee") + buyer_id = require_context_id(context, "accounts.buyer.id", "create licensee") + user_group_id = require_context_id(context, "accounts.user_group.id", "create licensee")
16-16: Remove unusednoqadirective.The
WPS238rule is not active in this project's linter configuration.-def build_licensee_data( # noqa: WPS238 +def build_licensee_data(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
docker-compose.yml(1 hunks)seed/accounts/accounts.py(1 hunks)seed/accounts/api_tokens.py(2 hunks)seed/accounts/buyer.py(2 hunks)seed/accounts/licensee.py(2 hunks)seed/accounts/module.py(1 hunks)seed/accounts/seller.py(2 hunks)seed/accounts/user_group.py(2 hunks)seed/static/static.py(1 hunks)tests/seed/accounts/test_accounts.py(2 hunks)tests/seed/accounts/test_api_token.py(0 hunks)tests/seed/accounts/test_api_tokens.py(1 hunks)tests/seed/accounts/test_buyer.py(2 hunks)tests/seed/accounts/test_licensee.py(3 hunks)tests/seed/accounts/test_module.py(2 hunks)tests/seed/accounts/test_seller.py(2 hunks)tests/seed/accounts/test_user_group.py(3 hunks)
💤 Files with no reviewable changes (1)
- tests/seed/accounts/test_api_token.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/seed/accounts/test_user_group.py
- seed/accounts/buyer.py
- tests/seed/accounts/test_api_tokens.py
🧰 Additional context used
🧬 Code graph analysis (10)
seed/accounts/accounts.py (2)
mpt_api_client/resources/accounts/account.py (1)
Account(27-28)seed/helper.py (1)
init_resource(44-84)
seed/accounts/module.py (3)
seed/helper.py (1)
init_resource(44-84)mpt_api_client/resources/accounts/modules.py (1)
Module(11-12)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)
tests/seed/accounts/test_module.py (3)
seed/accounts/module.py (2)
find_module(15-27)seed_module(30-34)tests/seed/conftest.py (1)
operations_client(24-25)seed/helper.py (1)
init_resource(44-84)
tests/seed/accounts/test_seller.py (3)
mpt_api_client/resources/accounts/sellers.py (1)
Seller(13-14)seed/accounts/seller.py (3)
build_seller_data(14-29)create_seller(33-38)seed_seller(41-45)seed/helper.py (1)
init_resource(44-84)
seed/accounts/seller.py (4)
seed/helper.py (1)
init_resource(44-84)mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)seed/container.py (1)
Container(7-30)mpt_api_client/resources/accounts/sellers.py (1)
Seller(13-14)
seed/accounts/user_group.py (1)
seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)
seed/accounts/licensee.py (3)
seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)seed/context.py (1)
Context(12-33)seed/container.py (1)
Container(7-30)
seed/accounts/api_tokens.py (3)
seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)seed/context.py (1)
Context(12-33)mpt_api_client/resources/accounts/api_tokens.py (1)
ApiToken(12-13)
tests/seed/accounts/test_buyer.py (3)
mpt_api_client/resources/accounts/buyers.py (1)
Buyer(26-27)seed/accounts/buyer.py (3)
build_buyer_data(15-35)create_buyer(39-46)seed_buyer(49-53)seed/helper.py (1)
init_resource(44-84)
tests/seed/accounts/test_accounts.py (3)
mpt_api_client/resources/accounts/account.py (1)
Account(27-28)seed/accounts/accounts.py (2)
get_account(16-21)seed_accounts(24-36)mpt_api_client/models/model.py (1)
id(119-121)
🪛 Ruff (0.14.8)
seed/accounts/accounts.py
16-16: Unused noqa directive (non-enabled: RUF029)
Remove unused noqa directive
(RUF100)
20-20: Avoid specifying long messages outside the exception class
(TRY003)
24-24: Unused noqa directive (unknown: WPS213, WPS217)
Remove unused noqa directive
(RUF100)
seed/accounts/module.py
26-26: Avoid specifying long messages outside the exception class
(TRY003)
tests/seed/accounts/test_module.py
8-8: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
9-9: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
seed/accounts/licensee.py
16-16: Unused noqa directive (unknown: WPS238)
Remove unused noqa directive
(RUF100)
tests/seed/accounts/test_accounts.py
22-22: 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 (19)
docker-compose.yml (1)
28-31: Expandedapp_testcommand looks consistent with new seed testsIncluding
pytest tests/unit tests/seedafter the existing lint/type checks fits the prior pattern and ensures the new seeding tests run in CI. No issues spotted.seed/static/static.py (1)
3-5: LOGO path update is safe and localizedSwitching
LOGOtologo.pngwhile keeping the same directory and constant name is a straightforward asset rename; no behavioral concerns here.tests/seed/accounts/test_buyer.py (1)
3-48: Buyer creation tests align with new seeding APIThe imports and both tests (
test_build_buyer_dataandtest_create_buyer) correctly exercise the newbuild_buyer_data/create_buyerflow and the updated context keys for account and seller IDs. The AsyncMock wiring againstoperations_client.accounts.buyers.createis appropriate, and asserting thatcreate_buyerreturns theBuyerfixture keeps the test focused.seed/accounts/api_tokens.py (1)
9-42: API token builder and seeding are consistent withinit_resourcepatternUsing
require_context_idfor"accounts.account.id"and"accounts.module.id"plus seeding viainit_resource("accounts.api_token.id", create_api_token)brings this module in line with the other account seeders and centralizes the caching behavior. Structure and types look good.seed/accounts/user_group.py (1)
10-45: User group seeding correctly migrated toinit_resource+require_context_idThe user group builder and
create_user_group/seed_user_groupflow now follow the same context‑driven pattern as other account seeders, with consistent"accounts.user_group.id"naming and required context IDs for account and module. This looks coherent and maintainable.tests/seed/accounts/test_accounts.py (2)
1-21:get_accounttests cover both error and success paths
test_get_account_failsandtest_get_account_successneatly exercise the two main branches ofget_account, usingmonkeypatchto controlCLIENT_ACCOUNT_ID. Asserting on the concreteAccount.idvalue is a good sanity check on the model wrapper.
22-56: Seed orchestration test is well-structured; keep thenoqafor legitimate complexity suppressionThe
test_seed_accountsorchestration test correctly patchesinit_resourceand all the individual seed functions, then verifies each is called exactly once afterseed_accounts().One clarification:
- Keep the
noqa: WPS210commentThe
# noqa: WPS210on line 22 suppresses a Flake8-WPS violation (not Ruff), which is legitimate for this test function. With 7 mock patches and 8+ assertion statements, the function complexity warrants the suppression. This is consistent with similar test functions throughout the codebase that use the same suppression.
- (Optional) Assert expected parameters for
init_resourceIf you want to guard against accidental parameter changes, you could assert the exact call:
- for mock in mocks: - mock.assert_called_once() + for mock in mocks: + mock.assert_called_once() + + mock_init_resource.assert_awaited_once_with("accounts.account.id", get_account)This keeps the generic "called once" checks while adding a more precise assertion for the context key and account resolver.
Likely an incorrect or invalid review comment.
seed/accounts/seller.py (2)
32-38: LGTM!The
@injectdecorator is correctly applied, and the function properly delegates to the MPT client for seller creation. The implementation aligns with the refactored seeding pattern.
41-45: LGTM!The
seed_sellerfunction correctly usesinit_resourcefor idempotent seeding, consistent with the PR's refactoring goals.seed/accounts/module.py (2)
14-27: LGTM!The
find_modulefunction correctly uses RQL filtering and handles the not-found case with an appropriate error. The@injectdecorator ensures proper DI resolution.
30-34: LGTM!The
seed_modulefunction correctly usesinit_resourcefor idempotent module seeding.tests/seed/accounts/test_module.py (3)
35-45: LGTM!The test correctly verifies
find_modulereturns the expected module when the filter yields results.
48-57: LGTM!Good coverage for the error case when the "Access Management" module is not found.
60-67: LGTM!The test correctly verifies that
seed_moduledelegates toinit_resource.seed/accounts/licensee.py (2)
47-52: LGTM!The
@injectdecorator is correctly applied, and the ICON file is properly handled with a context manager ensuring the file descriptor is closed.
55-59: LGTM!The
seed_licenseefunction correctly usesinit_resourcefor idempotent seeding.tests/seed/accounts/test_licensee.py (3)
31-60: LGTM!The test comprehensively sets up context with all required IDs and verifies the expected licensee data structure.
74-81: LGTM!The test correctly verifies that
seed_licenseedelegates toinit_resource.
63-71: The fixture naming is correct and requires no change.The test appropriately uses the
operations_clientfixture and passes it as a positional argument tocreate_licensee, which correctly overrides the dependency injection. While the parameter is namedmpt_clientin the function signature (referencingContainer.mpt_client), the fixture nameoperations_clientis semantically correct—it represents the MPT client for account operations. Renaming it toclientormpt_clientwould actually reduce code clarity. No action needed.Likely an incorrect or invalid review comment.
albertsola
force-pushed
the
MPT-16326-Refactor-accounts-seeding
branch
from
3fbc11f to
6e6bbb8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (13)
seed/accounts/accounts.py (2)
16-21: Clean up unusednoqadirectives.The static analysis indicates
RUF029is not enabled, making thenoqadirective unnecessary. Consider removing it for cleaner code.-async def get_account() -> Account: # noqa: RUF029 async for compatibility purposes with init_resource +async def get_account() -> Account:
24-36: Clean up unusednoqadirective.Static analysis indicates
WPS213andWPS217are unknown rules (not enabled in Ruff). The directive can be removed.-async def seed_accounts() -> None: # noqa: WPS213 WPS217 +async def seed_accounts() -> None:tests/seed/accounts/test_accounts.py (2)
22-22: Clean up unusednoqadirective.Static analysis indicates
WPS210is an unknown rule. The directive can be removed.-async def test_seed_accounts(mocker): # noqa: WPS210 +async def test_seed_accounts(mocker):
46-56: Consider assertinginit_resourcewas called with correct arguments.The test asserts
mock_init_resource.assert_called_once()but doesn't verify it was called with the expected arguments ("accounts.account.id", get_account). Other tests in this PR (e.g.,test_seed_user_group,test_seed_module) verify the exact call arguments.+from seed.accounts.accounts import get_account, seed_accounts + ... mocks = [ mock_seed_seller, mock_seed_buyer, mock_seed_module, mock_seed_api_token, mock_seed_user_group, mock_seed_licensee, - mock_init_resource, ] for mock in mocks: mock.assert_called_once() + mock_init_resource.assert_awaited_once_with("accounts.account.id", get_account)tests/seed/accounts/test_module.py (2)
8-9: Clean up unusednoqadirectives.Static analysis indicates
WPS110is an unknown rule (not enabled in Ruff). The directives can be removed.- def __init__(self, items): # noqa: WPS110 - self._iterator = iter(items) # noqa: WPS110 + def __init__(self, items): + self._iterator = iter(items)
60-67: Consider assertinginit_resourcewas called with correct arguments.For consistency with
test_seed_user_group, consider verifying the exact call arguments to ensure the seeding is properly wired.- init_resource.assert_awaited_once() + init_resource.assert_awaited_once_with("accounts.module.id", find_module)seed/accounts/module.py (1)
14-27: LGTM! Clean module lookup implementation.The
find_modulefunction properly uses dependency injection and async iteration to find the target module. The error handling for missing modules is appropriate.Regarding the static analysis hint (TRY003): consider extracting the error message to a custom exception class if this pattern is used elsewhere, but it's acceptable as-is for a single occurrence.
seed/accounts/licensee.py (2)
20-23: Inconsistent action string format.The action string on line 20 uses "creating licensee" (gerund form) while lines 21-23 use "create licensee" (infinitive form). For consistency and clearer error messages, use the same format throughout.
account_id = require_context_id(context, "accounts.account.id", "creating licensee") - seller_id = require_context_id(context, "accounts.seller.id", "create licensee") - buyer_id = require_context_id(context, "accounts.buyer.id", "create licensee") - user_group_id = require_context_id(context, "accounts.user_group.id", "create licensee") + seller_id = require_context_id(context, "accounts.seller.id", "creating licensee") + buyer_id = require_context_id(context, "accounts.buyer.id", "creating licensee") + user_group_id = require_context_id(context, "accounts.user_group.id", "creating licensee")
16-16: Remove unusednoqadirective.The static analysis tool indicates that
WPS238is an unknown code for Ruff. This directive is no longer needed and can be removed.-def build_licensee_data( # noqa: WPS238 +def build_licensee_data( context: Context = Provide[Container.context], ) -> dict[str, object]:tests/seed/accounts/test_buyer.py (1)
51-56: Consider asserting the arguments passed toinit_resource.While the test verifies
init_resourcewas awaited, it doesn't validate the namespace or factory function passed. Consider usingassert_awaited_once_withfor stronger verification.async def test_seed_buyer(mocker): init_resource = mocker.patch("seed.accounts.buyer.init_resource", new_callable=mocker.AsyncMock) await seed_buyer() # act - init_resource.assert_awaited_once() + init_resource.assert_awaited_once_with("accounts.buyer.id", create_buyer)tests/seed/accounts/test_licensee.py (2)
31-37: Remove dead test setup code.Line 32 sets the
CLIENT_ACCOUNT_IDenvironment variable, butbuild_licensee_datano longer reads from environment variables—it usesrequire_context_idinstead. Similarly, line 34 sets the user_group resource, but only the ID (line 33) is used by the function.def test_build_licensee_data(context: Context, monkeypatch): - 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.account.id"] = "ACC-1086-6867"Also update the function signature to remove the unused
monkeypatchparameter:-def test_build_licensee_data(context: Context, monkeypatch): +def test_build_licensee_data(context: Context):
74-81: Consider asserting the arguments passed toinit_resource.Same as the buyer test—use
assert_awaited_once_withfor stronger verification of the namespace and factory function.async def test_seed_licensee(mocker): init_resource = mocker.patch( "seed.accounts.licensee.init_resource", new_callable=mocker.AsyncMock ) await seed_licensee() # act - init_resource.assert_awaited_once() + init_resource.assert_awaited_once_with("accounts.licensee.id", create_licensee)seed/accounts/user_group.py (1)
10-29: Context-based ID resolution for account & module looks solidSwitching
build_user_group_data()to userequire_context_idfor"accounts.account.id"and"accounts.module.id"ties user-group creation strictly to previously-seeded context and fails fast when prerequisites are missing, which is a good, explicit contract. The newinit_resource/require_context_idimports are correctly used and keep the function free of environment access.If these
"create user group"action strings start to repeat across modules, you might later want to centralize them, but that’s purely cosmetic and can wait.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
docker-compose.yml(1 hunks)seed/accounts/accounts.py(1 hunks)seed/accounts/api_tokens.py(2 hunks)seed/accounts/buyer.py(2 hunks)seed/accounts/licensee.py(2 hunks)seed/accounts/module.py(1 hunks)seed/accounts/seller.py(2 hunks)seed/accounts/user_group.py(2 hunks)seed/static/static.py(1 hunks)tests/seed/accounts/test_accounts.py(2 hunks)tests/seed/accounts/test_api_token.py(0 hunks)tests/seed/accounts/test_api_tokens.py(1 hunks)tests/seed/accounts/test_buyer.py(2 hunks)tests/seed/accounts/test_licensee.py(3 hunks)tests/seed/accounts/test_module.py(2 hunks)tests/seed/accounts/test_seller.py(2 hunks)tests/seed/accounts/test_user_group.py(3 hunks)
💤 Files with no reviewable changes (1)
- tests/seed/accounts/test_api_token.py
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/seed/accounts/test_seller.py
- seed/static/static.py
- docker-compose.yml
- tests/seed/accounts/test_api_tokens.py
- seed/accounts/buyer.py
🧰 Additional context used
🧬 Code graph analysis (8)
seed/accounts/accounts.py (7)
mpt_api_client/resources/accounts/account.py (1)
Account(27-28)seed/accounts/api_tokens.py (1)
seed_api_token(39-43)seed/accounts/buyer.py (1)
seed_buyer(49-53)seed/accounts/licensee.py (1)
seed_licensee(55-59)seed/accounts/module.py (1)
seed_module(30-34)seed/accounts/seller.py (1)
seed_seller(41-45)seed/helper.py (1)
init_resource(44-84)
seed/accounts/seller.py (4)
seed/helper.py (1)
init_resource(44-84)mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)seed/container.py (1)
Container(7-30)mpt_api_client/resources/accounts/sellers.py (1)
Seller(13-14)
seed/accounts/api_tokens.py (5)
seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)seed/context.py (1)
Context(12-33)seed/container.py (1)
Container(7-30)mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/resources/accounts/api_tokens.py (1)
ApiToken(12-13)
tests/seed/accounts/test_user_group.py (4)
mpt_api_client/resources/accounts/user_groups.py (1)
UserGroup(11-12)seed/accounts/user_group.py (3)
build_user_group_data(16-29)create_user_group(33-38)seed_user_group(41-45)tests/seed/conftest.py (2)
context(14-15)operations_client(24-25)seed/helper.py (1)
init_resource(44-84)
tests/seed/accounts/test_licensee.py (2)
seed/accounts/licensee.py (3)
build_licensee_data(16-44)create_licensee(48-52)seed_licensee(55-59)seed/helper.py (1)
init_resource(44-84)
tests/seed/accounts/test_buyer.py (4)
mpt_api_client/resources/accounts/buyers.py (1)
Buyer(26-27)seed/accounts/buyer.py (3)
build_buyer_data(15-35)create_buyer(39-46)seed_buyer(49-53)seed/context.py (1)
Context(12-33)seed/helper.py (1)
init_resource(44-84)
tests/seed/accounts/test_module.py (3)
seed/accounts/module.py (2)
find_module(15-27)seed_module(30-34)tests/seed/conftest.py (1)
operations_client(24-25)seed/helper.py (1)
init_resource(44-84)
seed/accounts/user_group.py (2)
seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)seed/context.py (1)
Context(12-33)
🪛 Ruff (0.14.8)
seed/accounts/module.py
26-26: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/accounts.py
16-16: Unused noqa directive (non-enabled: RUF029)
Remove unused noqa directive
(RUF100)
20-20: Avoid specifying long messages outside the exception class
(TRY003)
24-24: Unused noqa directive (unknown: WPS213, WPS217)
Remove unused noqa directive
(RUF100)
seed/accounts/licensee.py
16-16: Unused noqa directive (unknown: WPS238)
Remove unused noqa directive
(RUF100)
tests/seed/accounts/test_module.py
8-8: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
9-9: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
tests/seed/accounts/test_accounts.py
22-22: 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 (17)
seed/accounts/seller.py (2)
32-38: LGTM! DI pattern correctly implemented.The
@injectdecorator is properly applied, and the function correctly usesProvide[Container.mpt_operations]for dependency injection. The implementation follows the new seeding pattern consistently with other seed modules.
41-45: LGTM! Seed function properly refactored.The
seed_sellerfunction correctly usesinit_resourceto manage resource creation and caching, aligning with the new seeding architecture.seed/accounts/accounts.py (1)
27-34: LGTM! Seed orchestration properly updated.The
seed_accountsfunction now correctly initializes the account resource first viainit_resource, then seeds dependent resources in the appropriate order.tests/seed/accounts/test_accounts.py (1)
7-19: LGTM! Good test coverage forget_account.The tests properly cover both the failure case (missing environment variable) and success case, validating the returned
Accountobject's type and id.tests/seed/accounts/test_user_group.py (3)
17-32: LGTM! Test correctly updated for new context requirements.The test properly sets
accounts.account.idin the context (line 20), which aligns with the updatedbuild_user_group_datathat now usesrequire_context_idfor account lookup.
35-43: LGTM! Good test forcreate_user_group.The test properly mocks the dependencies and verifies that the user group creation flow works correctly through the operations client.
46-53: LGTM! Seed test correctly validatesinit_resourceusage.The test properly asserts that
init_resourceis called with the correct namespace and factory function, ensuring the seeding flow is wired correctly.tests/seed/accounts/test_module.py (2)
35-45: LGTM! Test correctly validatesfind_moduleflow.The test properly sets up the mock filter chain and validates that the module is returned correctly when found.
48-57: LGTM! Good error case coverage.The test properly validates that
find_moduleraises aValueErrorwith the expected message when the module is not found.seed/accounts/module.py (1)
30-33: LGTM!The seeding flow correctly delegates to
init_resourcewith the appropriate namespace.seed/accounts/licensee.py (1)
47-52: LGTM!The
@injectdecorator is correctly applied, and the function properly builds data and creates the licensee with the icon file.tests/seed/accounts/test_buyer.py (1)
41-48: LGTM!The test correctly sets up context, mocks the API call, and verifies the buyer creation flow.
seed/accounts/api_tokens.py (2)
14-27: LGTM!The data building function correctly uses
require_context_idfor both account and module IDs, and the returned dictionary structure is well-formed.
30-43: LGTM!The
@injectdecorator is correctly applied, and the seed flow properly usesinit_resourcewith the.idnamespace suffix consistent with other seed modules.tests/seed/accounts/test_licensee.py (1)
63-71: LGTM!The test correctly mocks
build_licensee_dataand verifies the creation flow via the operations client.seed/accounts/user_group.py (2)
32-38:create_user_groupfactory matches theinit_resourcepatternThe async
create_user_group()factory with an injectedAsyncMPTClientand no required positional arguments is exactly whatinit_resource(..., create_user_group)expects. Centralizing payload construction throughbuild_user_group_data()keeps the template in one place and is easy to override in tests.Please just make sure your wiring/tests cover this path so that
mpt_operationsis indeed injected at runtime.
41-45: Function signature correctly reflects thatseed_user_group()returnsNoneGood call—the change to return
Noneand delegate toinit_resource()is safe. All call sites (seed/accounts/accounts.py:33and the test intests/seed/accounts/test_user_group.py:51) invoke the function withawait seed_user_group()without assigning the result, confirming they depend only on side effects.
| create = mocker.AsyncMock(return_value=api_token) | ||
| operations_client.accounts.api_tokens.create = create | ||
| build_api_token_data = mocker.patch( | ||
| "seed.accounts.api_tokens.build_api_token_data", new_callable=mocker.Mock |
There was a problem hiding this comment.
new_callable=mocker.Mock seems obsolete here and spec missing
| create = mocker.AsyncMock(return_value=api_token) | ||
| operations_client.accounts.api_tokens.create = create |
There was a problem hiding this comment.
I think this could be done in one line and also we should use spec here.
| async def test_create_buyer(mocker, context: Context, operations_client, buyer): | ||
| context["accounts.seller.id"] = "SEL-9999-9999" | ||
| context["accounts.account.id"] = "ACC-1086-6867" | ||
| operations_client.accounts.buyers.create = mocker.AsyncMock(return_value=buyer) |
tests/seed/accounts/test_accounts.py
Outdated
|
|
||
| async def test_seed_accounts(mocker): # noqa: WPS210 | ||
| mock_init_resource = mocker.patch( | ||
| "seed.accounts.accounts.init_resource", new_callable=mocker.AsyncMock |
There was a problem hiding this comment.
Can we use spec here? Also new_callable seems obsolete
tests/seed/accounts/test_licensee.py
Outdated
| mock_init_licensee = mocker.patch( | ||
| "seed.accounts.licensee.init_licensee", new_callable=mocker.AsyncMock | ||
| init_resource = mocker.patch( | ||
| "seed.accounts.licensee.init_resource", new_callable=mocker.AsyncMock |
There was a problem hiding this comment.
new_callable=mocker.Mock seems obsolete here and spec missing
tests/seed/accounts/test_module.py
Outdated
| mock_refresh_module = mocker.patch( | ||
| "seed.accounts.module.refresh_module", new_callable=mocker.AsyncMock | ||
| init_resource = mocker.patch( | ||
| "seed.accounts.module.init_resource", new_callable=mocker.AsyncMock |
There was a problem hiding this comment.
new_callable=mocker.Mock seems obsolete here and spec missing
| async def test_create_seller(mocker, operations_client): | ||
| seller = Seller({"id": "SEL-123"}) | ||
| create_mock = mocker.AsyncMock(return_value=seller) | ||
| operations_client.accounts.sellers.create = create_mock |
tests/seed/accounts/test_seller.py
Outdated
| mock_init_seller = mocker.patch( | ||
| "seed.accounts.seller.init_seller", new_callable=mocker.AsyncMock | ||
| init_resource = mocker.patch( | ||
| "seed.accounts.seller.init_resource", new_callable=mocker.AsyncMock |
There was a problem hiding this comment.
new_callable=mocker.Mock seems obsolete here and spec missing
| mock_init_user_group = mocker.patch( | ||
| "seed.accounts.user_group.init_user_group", new_callable=mocker.AsyncMock | ||
| init_resource = mocker.patch( | ||
| "seed.accounts.user_group.init_resource", new_callable=mocker.AsyncMock |
There was a problem hiding this comment.
new_callable=mocker.Mock seems obsolete here and spec missing
albertsola
force-pushed
the
MPT-16326-Refactor-accounts-seeding
branch
2 times, most recently
from
0ce0c55 to
cc4af75
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/seed/accounts/test_licensee.py (1)
63-71: Missingspecparameter on mocks.The mocks at lines 64 and 66 lack the
specparameter, which was noted in previous reviews. Addingspechelps catch typos and ensures mocks match the actual interface.- create_mock = mocker.AsyncMock(return_value=licensee) + create_mock = mocker.AsyncMock(return_value=licensee, spec=True) operations_client.accounts.licensees.create = create_mock - mocker.patch("seed.accounts.licensee.build_licensee_data") + mocker.patch("seed.accounts.licensee.build_licensee_data", autospec=True)
🧹 Nitpick comments (6)
tests/seed/accounts/test_accounts.py (1)
22-22: Remove unused noqa directive.The
noqa: WPS210directive is flagged as unused by Ruff, indicating the "too many local variables" issue it was suppressing is no longer present.Apply this diff to remove the unused directive:
-async def test_seed_accounts(mocker): # noqa: WPS210 +async def test_seed_accounts(mocker):tests/seed/accounts/test_seller.py (1)
47-52: Consider assertinginit_resourcecall arguments (not just “awaited once”)
This makes the test stricter about wiring.- init_resource.assert_awaited_once() + init_resource.assert_awaited_once_with("accounts.seller.id", create_seller)tests/seed/accounts/test_module.py (1)
60-65:seed_moduletest wiring viainit_resourcepatch aligns with the refactor
Optional tightening: assert the call args ("accounts.module.id", find_module) if you want stricter wiring checks.seed/accounts/licensee.py (2)
20-23: Inconsistent action string format.The action strings are inconsistent: line 20 uses
"creating licensee"while lines 21-23 use"create licensee". For consistency, use the same format throughout.- account_id = require_context_id(context, "accounts.account.id", "creating licensee") - seller_id = require_context_id(context, "accounts.seller.id", "create licensee") - buyer_id = require_context_id(context, "accounts.buyer.id", "create licensee") - user_group_id = require_context_id(context, "accounts.user_group.id", "create licensee") + account_id = require_context_id(context, "accounts.account.id", "create licensee") + seller_id = require_context_id(context, "accounts.seller.id", "create licensee") + buyer_id = require_context_id(context, "accounts.buyer.id", "create licensee") + user_group_id = require_context_id(context, "accounts.user_group.id", "create licensee")
16-16: Remove unusednoqadirective.The
# noqa: WPS238comment is flagged as unused by static analysis. This directive should be removed.-def build_licensee_data( # noqa: WPS238 +def build_licensee_data(seed/accounts/buyer.py (1)
15-35: Missing@injectdecorator onbuild_buyer_data.The function uses
Provide[Container.context]as a default parameter but lacks the@injectdecorator. Whilecreate_buyerpassescontextexplicitly, ifbuild_buyer_data()is called directly without arguments (e.g., in tests or future code), theProvidemarker won't resolve.This is inconsistent with other files in this PR (e.g.,
build_licensee_dataandbuild_user_group_datahave@inject).+@inject def build_buyer_data(context: Context = Provide[Container.context]) -> dict[str, object]: """Build buyer data dictionary for creation."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
docker-compose.yml(1 hunks)seed/accounts/accounts.py(1 hunks)seed/accounts/api_tokens.py(2 hunks)seed/accounts/buyer.py(2 hunks)seed/accounts/licensee.py(2 hunks)seed/accounts/module.py(1 hunks)seed/accounts/seller.py(2 hunks)seed/accounts/user_group.py(2 hunks)seed/static/static.py(1 hunks)tests/seed/accounts/test_accounts.py(2 hunks)tests/seed/accounts/test_api_token.py(0 hunks)tests/seed/accounts/test_api_tokens.py(1 hunks)tests/seed/accounts/test_buyer.py(2 hunks)tests/seed/accounts/test_licensee.py(3 hunks)tests/seed/accounts/test_module.py(2 hunks)tests/seed/accounts/test_seller.py(2 hunks)tests/seed/accounts/test_user_group.py(3 hunks)tests/seed/catalog/test_authorization.py(1 hunks)tests/seed/catalog/test_catalog.py(1 hunks)tests/seed/catalog/test_listing.py(1 hunks)tests/seed/catalog/test_price_list.py(1 hunks)tests/seed/test_seed_api.py(2 hunks)
💤 Files with no reviewable changes (1)
- tests/seed/accounts/test_api_token.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/seed/accounts/test_user_group.py
- tests/seed/accounts/test_api_tokens.py
- tests/seed/accounts/test_buyer.py
🧰 Additional context used
🧬 Code graph analysis (10)
seed/accounts/accounts.py (2)
mpt_api_client/resources/accounts/account.py (1)
Account(27-28)seed/helper.py (1)
init_resource(44-84)
seed/accounts/module.py (3)
seed/helper.py (1)
init_resource(44-84)mpt_api_client/resources/accounts/modules.py (1)
Module(11-12)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)
tests/seed/catalog/test_authorization.py (2)
seed/catalog/authorization.py (2)
seed_authorization(12-14)create_authorization(18-39)seed/helper.py (1)
init_resource(44-84)
seed/accounts/seller.py (3)
seed/helper.py (1)
init_resource(44-84)seed/container.py (1)
Container(7-30)mpt_api_client/resources/accounts/sellers.py (1)
Seller(13-14)
seed/accounts/buyer.py (3)
seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)seed/context.py (1)
Context(12-33)seed/container.py (1)
Container(7-30)
tests/seed/catalog/test_catalog.py (5)
seed/catalog/catalog.py (1)
seed_catalog(11-19)seed/catalog/product.py (1)
seed_product(37-51)seed/catalog/authorization.py (1)
seed_authorization(12-14)seed/catalog/price_list.py (1)
seed_price_list(10-12)seed/catalog/listing.py (1)
seed_listing(10-12)
seed/accounts/user_group.py (3)
seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)seed/context.py (1)
Context(12-33)seed/container.py (1)
Container(7-30)
tests/seed/accounts/test_module.py (3)
seed/accounts/module.py (2)
find_module(15-27)seed_module(30-34)tests/seed/conftest.py (1)
operations_client(24-25)seed/helper.py (1)
init_resource(44-84)
seed/accounts/api_tokens.py (3)
seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)seed/context.py (1)
Context(12-33)seed/container.py (1)
Container(7-30)
tests/seed/accounts/test_accounts.py (3)
mpt_api_client/resources/accounts/account.py (1)
Account(27-28)seed/accounts/accounts.py (2)
get_account(16-21)seed_accounts(24-36)mpt_api_client/models/model.py (1)
id(119-121)
🪛 Ruff (0.14.8)
seed/accounts/accounts.py
16-16: Unused noqa directive (non-enabled: RUF029)
Remove unused noqa directive
(RUF100)
20-20: Avoid specifying long messages outside the exception class
(TRY003)
24-24: Unused noqa directive (unknown: WPS213, WPS217)
Remove unused noqa directive
(RUF100)
seed/accounts/module.py
26-26: Avoid specifying long messages outside the exception class
(TRY003)
tests/seed/accounts/test_module.py
8-8: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
9-9: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
seed/accounts/licensee.py
16-16: Unused noqa directive (unknown: WPS238)
Remove unused noqa directive
(RUF100)
tests/seed/accounts/test_accounts.py
22-22: 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 (33)
docker-compose.yml (1)
30-30: LGTM! Seed tests now included in CI.The addition of
tests/seedto the test path ensures the new seed-related tests are executed as part of the test suite.seed/accounts/api_tokens.py (4)
9-9: LGTM! Proper use of helper functions.The imports of
init_resourceandrequire_context_idalign with the PR-wide refactoring to centralize resource initialization and context validation.
19-20: LGTM! Context IDs properly validated.Using
require_context_idensures that required context values are present before attempting to create the API token, providing clear error messages if prerequisites are missing.
31-36: LGTM! Clean separation of concerns.The
create_api_tokenfunction properly uses the@injectdecorator and delegates data construction tobuild_api_token_data, maintaining a clean separation between data preparation and API interaction.
42-42: LGTM! Consistent with init_resource pattern.The namespace
"accounts.api_token.id"follows the convention established across other seed modules, and the use ofinit_resourceensures idempotent seeding.tests/seed/catalog/test_authorization.py (1)
35-40: LGTM! Improved test robustness with autospec.Using
autospec=Trueensures the mock enforces the actual signature ofinit_resource, andassert_awaited_once_withcorrectly verifies the awaited call. These changes improve test reliability.tests/seed/test_seed_api.py (2)
23-24: LGTM! Consistent autospec usage.Adding
autospec=Trueto both patches ensures the mocks validate against the actual function signatures, catching potential signature mismatches at test time.
35-36: LGTM! Correct async assertion method.Using
assert_awaited_once()properly validates that the async functions were awaited, which is the correct assertion for coroutines.tests/seed/catalog/test_catalog.py (2)
8-11: LGTM! Comprehensive autospec coverage.All seed function patches now use
autospec=True, ensuring the test validates against the actual function signatures and catches interface changes.
15-18: LGTM! Proper async assertions.All assertions correctly use
assert_awaited_once()to verify the async seed functions were properly awaited during execution.tests/seed/catalog/test_price_list.py (1)
29-29: LGTM! Consistent autospec pattern.The addition of
autospec=Truealigns with the test improvements across the PR, ensuring the mock validates against the actualinit_resourcesignature.tests/seed/accounts/test_accounts.py (4)
7-11: LGTM! Proper error handling test.The test correctly verifies that
get_accountraisesValueErrorwhen the required environment variable is missing, ensuring defensive behavior.
14-19: LGTM! Successful path well tested.The test properly validates that
get_accountreturns anAccountinstance with the correct ID when the environment variable is set.
23-30: LGTM! Comprehensive mocking with autospec.All seed functions and
init_resourceare properly mocked withautospec=True, ensuring the tests validate against actual function signatures.
32-42: LGTM! Thorough verification of all seed calls.The test correctly verifies that all seed functions and
init_resourceare awaited exactly once, ensuring the complete seed flow executes as expected.seed/static/static.py (1)
5-5: No action needed — logo.png file exists at the expected location.The file
seed/static/logo.pngis present, and the path construction in the LOGO constant is correct. The runtime reference to this constant will work as intended.seed/accounts/accounts.py (2)
24-36: Seed orchestration viainit_resource(..., get_account)looks consistent with the new pattern
Initializing"accounts.account.id"up-front before seeding dependent resources matchesseed.helper.init_resource’s caching behavior.
24-28: Remove unused/unknownnoqaonseed_accounts(RUF100)
Ruff reports the# noqa: WPS213 WPS217is unused/unknown; drop it to avoid lint failures.-async def seed_accounts() -> None: # noqa: WPS213 WPS217 +async def seed_accounts() -> None: """Seed accounts data including account.""" logger.debug("Seeding accounts ...") await init_resource("accounts.account.id", get_account)⛔ 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/catalog/test_listing.py (1)
35-38:autospec=Trueforinit_resourcepatch is a good tightening
Keeps the mock aligned with the real callable’s signature and still supportsassert_awaited_once_with(...).seed/accounts/seller.py (2)
32-38:create_sellerfactory +@injectis the right shape forinit_resource
Keeps the factory zero-arg forinit_resource()while still resolvingmpt_operationsthrough DI.
41-45:seed_sellerusinginit_resource("accounts.seller.id", create_seller)is consistent and cache-friendly
Avoids re-creating sellers when the context already has the ID.tests/seed/accounts/test_module.py (2)
35-45:test_find_moduleasync-iteration stub looks solid
TheDummyAsyncIteratorapproach exercises theasync forpath without needing a real service implementation.
48-58: Good coverage for the “module not found” error path
TheValueErrormessage match makes the failure mode explicit.seed/accounts/module.py (2)
14-27: LGTM! Well-structured module finder with proper DI.The
@injectdecorator is correctly applied, and the function properly filters and returns the first matching module. The error handling for missing module is appropriate.Regarding the static analysis hint (TRY003): the inline message is acceptable here given the simplicity of the error case.
30-34: LGTM! Clean seed orchestration.The
seed_modulefunction correctly delegates toinit_resourcewith thefind_modulefactory. No@injectdecorator is needed here sinceinit_resourceitself handles the DI resolution.seed/accounts/licensee.py (2)
47-52: LGTM! Properly decorated with@inject.The
@injectdecorator is now correctly applied, addressing the previous review concern. The function properly builds data and creates the licensee with an icon file.
55-59: LGTM! Clean seed orchestration.The
seed_licenseefunction correctly usesinit_resourceto handle the create-or-retrieve flow.tests/seed/accounts/test_licensee.py (1)
74-79: LGTM! Clean seed test.The test correctly patches
init_resourceat the module level and verifies it's awaited once.seed/accounts/user_group.py (3)
15-29: LGTM! Clean data builder with proper DI.The function correctly uses
require_context_idto fetch dependencies and returns a well-structured dictionary for user group creation.
32-38: LGTM! Critical issue from previous review addressed.The
@injectdecorator is now correctly applied, resolving the previous critical issue where DI would fail to resolve theProvidemarker at runtime.
41-45: LGTM! Clean seed orchestration.The
seed_user_groupfunction correctly delegates toinit_resourcewith thecreate_user_groupfactory.seed/accounts/buyer.py (2)
38-46: LGTM! Properly decorated with@inject.The
create_buyerfunction is correctly decorated and handles the buyer creation with icon file upload.
49-53: LGTM! Clean seed orchestration.The
seed_buyerfunction correctly usesinit_resourceto handle the create-or-retrieve flow.
| async def get_account() -> Account: # noqa: RUF029 async for compatibility purposes with init_resource | ||
| """Get account ID from environment variable.""" | ||
| account_id = os.getenv("CLIENT_ACCOUNT_ID") | ||
| if not account_id: | ||
| raise ValueError("CLIENT_ACCOUNT_ID environment variable is required") | ||
| return Account({"id": account_id}) |
There was a problem hiding this comment.
Fix Ruff findings: remove unused noqa + address TRY003 with a custom exception
Ruff reports # noqa: RUF029 is unused and the ValueError("...") message triggers TRY003; both are straightforward to fix.
+class MissingClientAccountIdError(ValueError):
+ """CLIENT_ACCOUNT_ID environment variable is required."""
+
async def get_account() -> Account: # noqa: RUF029 async for compatibility purposes with init_resource
"""Get account ID from environment variable."""
account_id = os.getenv("CLIENT_ACCOUNT_ID")
if not account_id:
- raise ValueError("CLIENT_ACCOUNT_ID environment variable is required")
+ raise MissingClientAccountIdError
return Account({"id": account_id})…and then remove the now-unneeded # noqa: RUF029.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.8)
16-16: Unused noqa directive (non-enabled: RUF029)
Remove unused noqa directive
(RUF100)
20-20: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In seed/accounts/accounts.py around lines 16 to 21, remove the unnecessary "#
noqa: RUF029" and replace the generic ValueError raised when CLIENT_ACCOUNT_ID
is missing with a custom exception (e.g., define a MissingClientAccountError in
this module or import one from a shared errors module) so Ruff's TRY003 is
satisfied; add the custom exception definition near the top of the file (or
import it), then raise MissingClientAccountError("CLIENT_ACCOUNT_ID environment
variable is required") instead of ValueError, and delete the now-unneeded noqa
comment.
| def __init__(self, items): # noqa: WPS110 | ||
| self._iterator = iter(items) # noqa: WPS110 | ||
|
|
There was a problem hiding this comment.
Remove unused/unknown # noqa: WPS110 (RUF100)
These are flagged as unused; they can break lint if Ruff is enforced.
class DummyAsyncIterator:
- def __init__(self, items): # noqa: WPS110
- self._iterator = iter(items) # noqa: WPS110
+ def __init__(self, items):
+ self._iterator = iter(items)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def __init__(self, items): # noqa: WPS110 | |
| self._iterator = iter(items) # noqa: WPS110 | |
| class DummyAsyncIterator: | |
| def __init__(self, items): | |
| self._iterator = iter(items) |
🧰 Tools
🪛 Ruff (0.14.8)
8-8: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
9-9: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
🤖 Prompt for AI Agents
In tests/seed/accounts/test_module.py around lines 8 to 10, the inline noqa
comments "# noqa: WPS110" are unused/unknown and should be removed; delete the
trailing "# noqa: WPS110" on the __init__ signature and the iterator assignment
so the linter (Ruff) won't flag unknown codes and the file uses standard noqa if
needed.
| async def test_create_seller(mocker, operations_client): | ||
| seller = Seller({"id": "SEL-123"}) | ||
| create_mock = mocker.AsyncMock(return_value=seller) | ||
| operations_client.accounts.sellers.create = create_mock | ||
|
|
||
| result = await create_seller(operations_client) | ||
|
|
||
| async def test_init_seller_create_new( | ||
| context: Context, operations_client, sellers_service, seller, mocker | ||
| ): | ||
| sellers_service.create.return_value = seller | ||
| operations_client.accounts.sellers = sellers_service | ||
| mocker.patch("seed.accounts.seller.get_seller", return_value=None) | ||
| mocker.patch( | ||
| "seed.accounts.seller.build_seller_data", return_value=build_seller_data("test-external-id") | ||
| ) | ||
| result = await init_seller(context, mpt_operations=operations_client) | ||
| assert result == seller | ||
| sellers_service.create.assert_called_once() | ||
| create_mock.assert_called_once() | ||
|
|
There was a problem hiding this comment.
Use assert_awaited_once() for AsyncMock instead of assert_called_once()
This ensures the coroutine was actually awaited.
assert result == seller
- create_mock.assert_called_once()
+ create_mock.assert_awaited_once()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def test_create_seller(mocker, operations_client): | |
| seller = Seller({"id": "SEL-123"}) | |
| create_mock = mocker.AsyncMock(return_value=seller) | |
| operations_client.accounts.sellers.create = create_mock | |
| result = await create_seller(operations_client) | |
| async def test_init_seller_create_new( | |
| context: Context, operations_client, sellers_service, seller, mocker | |
| ): | |
| sellers_service.create.return_value = seller | |
| operations_client.accounts.sellers = sellers_service | |
| mocker.patch("seed.accounts.seller.get_seller", return_value=None) | |
| mocker.patch( | |
| "seed.accounts.seller.build_seller_data", return_value=build_seller_data("test-external-id") | |
| ) | |
| result = await init_seller(context, mpt_operations=operations_client) | |
| assert result == seller | |
| sellers_service.create.assert_called_once() | |
| create_mock.assert_called_once() | |
| async def test_create_seller(mocker, operations_client): | |
| seller = Seller({"id": "SEL-123"}) | |
| create_mock = mocker.AsyncMock(return_value=seller) | |
| operations_client.accounts.sellers.create = create_mock | |
| result = await create_seller(operations_client) | |
| assert result == seller | |
| create_mock.assert_awaited_once() |
🤖 Prompt for AI Agents
In tests/seed/accounts/test_seller.py around lines 36 to 45, the test uses
create_mock.assert_called_once() on an AsyncMock; replace that call with
create_mock.assert_awaited_once() so the test verifies the coroutine was
actually awaited, updating this assertion (and any similar AsyncMock assertions
in the file) to use assert_awaited_once().
cc4af75 to
00a2382
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
seed/accounts/accounts.py (1)
16-21: Fix Ruff: remove unusednoqaand avoid TRY003 by using a custom exception
Ruff flags the# noqa: RUF029as unused and the inline exception message as TRY003; simplest fix is a small custom exception type and dropping the unused noqa.+class MissingClientAccountIdError(ValueError): + """CLIENT_ACCOUNT_ID environment variable is required.""" + -async def get_account() -> Account: # noqa: RUF029 async for compatibility purposes with init_resource +async def get_account() -> Account: """Get account ID from environment variable.""" account_id = os.getenv("CLIENT_ACCOUNT_ID") if not account_id: - raise ValueError("CLIENT_ACCOUNT_ID environment variable is required") + raise MissingClientAccountIdError return Account({"id": account_id})tests/seed/accounts/test_seller.py (1)
36-44: Useassert_awaited_once()for AsyncMock.Line 44 should use
assert_awaited_once()instead ofassert_called_once()to properly verify the coroutine was awaited.Apply this diff:
assert result == seller - create_mock.assert_called_once() + create_mock.assert_awaited_once()tests/seed/accounts/test_buyer.py (1)
41-47: Prefer patchingbuyers.createwith autospec (vs direct assignment) to keep the mock honest.
Directly assigningoperations_client.accounts.buyers.create = AsyncMock(...)skips signature/attribute validation; patching with autospec would better match the intent of this PR’s “autospec” move.async def test_create_buyer(mocker, context: Context, operations_client, buyer): @@ - operations_client.accounts.buyers.create = mocker.AsyncMock(return_value=buyer) + mocker.patch.object( + operations_client.accounts.buyers, + "create", + autospec=True, + new_callable=mocker.AsyncMock, + return_value=buyer, + ) @@ result = await create_buyer(context, operations_client)
🧹 Nitpick comments (3)
tests/seed/test_seed_api.py (1)
22-37: Minor: setmock_seed_accounts.return_value = Nonefor consistency
Not required, but avoids accidentally propagating an AsyncMock return object if the implementation ever starts using the result.mock_seed_catalog.return_value = None + mock_seed_accounts.return_value = None mock_context_file.return_value = pathlib.Path("test_context.json")tests/seed/accounts/test_accounts.py (1)
22-22: Consider removing the unused noqa directive.Ruff flags
# noqa: WPS210as unknown. If the project no longer uses wemake-python-styleguide or this specific check isn't needed, remove the directive.Apply this diff if the directive is not needed:
-async def test_seed_accounts(mocker): # noqa: WPS210 +async def test_seed_accounts(mocker):Based on learnings, WPS directives are valid for wemake-python-styleguide. However, verify whether this specific check is still applicable here.
tests/seed/accounts/test_buyer.py (1)
51-56: Strengthentest_seed_buyerby asserting the awaited call arguments.
assert_awaited_once()can pass even if the namespace/factory are wrong; asserting args would lock down the behavior.async def test_seed_buyer(mocker): init_resource = mocker.patch("seed.accounts.buyer.init_resource", autospec=True) @@ - init_resource.assert_awaited_once() + init_resource.assert_awaited_once() + # optionally: + # init_resource.assert_awaited_once_with("accounts.buyer.id", mocker.ANY)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
seed/accounts/accounts.py(1 hunks)seed/accounts/api_tokens.py(2 hunks)seed/accounts/buyer.py(2 hunks)seed/accounts/licensee.py(2 hunks)seed/accounts/module.py(1 hunks)seed/accounts/seller.py(2 hunks)seed/accounts/user_group.py(2 hunks)seed/static/static.py(1 hunks)tests/seed/accounts/test_accounts.py(2 hunks)tests/seed/accounts/test_api_token.py(0 hunks)tests/seed/accounts/test_api_tokens.py(1 hunks)tests/seed/accounts/test_buyer.py(2 hunks)tests/seed/accounts/test_licensee.py(3 hunks)tests/seed/accounts/test_module.py(2 hunks)tests/seed/accounts/test_seller.py(2 hunks)tests/seed/accounts/test_user_group.py(3 hunks)tests/seed/catalog/test_authorization.py(1 hunks)tests/seed/catalog/test_catalog.py(1 hunks)tests/seed/catalog/test_listing.py(1 hunks)tests/seed/catalog/test_price_list.py(1 hunks)tests/seed/test_seed_api.py(2 hunks)
💤 Files with no reviewable changes (1)
- tests/seed/accounts/test_api_token.py
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/seed/catalog/test_listing.py
- tests/seed/accounts/test_api_tokens.py
- seed/static/static.py
- tests/seed/catalog/test_price_list.py
- tests/seed/catalog/test_authorization.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-01T10:48:52.586Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 136
File: tests/e2e/notifications/subscribers/conftest.py:14-25
Timestamp: 2025-12-01T10:48:52.586Z
Learning: The repository softwareone-platform/mpt-api-python-client uses wemake-python-styleguide linter, so noqa directives for WPS rules should not be flagged as unused.
Applied to files:
tests/seed/accounts/test_module.py
🧬 Code graph analysis (12)
tests/seed/accounts/test_seller.py (3)
mpt_api_client/resources/accounts/sellers.py (1)
Seller(13-14)seed/accounts/seller.py (3)
build_seller_data(14-29)create_seller(33-38)seed_seller(41-45)seed/helper.py (1)
init_resource(44-84)
seed/accounts/module.py (1)
seed/helper.py (1)
init_resource(44-84)
seed/accounts/buyer.py (2)
seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)seed/context.py (1)
Context(12-33)
tests/seed/accounts/test_module.py (3)
seed/accounts/module.py (2)
find_module(15-27)seed_module(30-34)tests/seed/conftest.py (1)
operations_client(24-25)seed/helper.py (1)
init_resource(44-84)
seed/accounts/accounts.py (2)
mpt_api_client/resources/accounts/account.py (1)
Account(27-28)seed/helper.py (1)
init_resource(44-84)
tests/seed/accounts/test_user_group.py (4)
mpt_api_client/resources/accounts/user_groups.py (1)
UserGroup(11-12)seed/accounts/user_group.py (3)
build_user_group_data(16-29)create_user_group(33-38)seed_user_group(41-45)tests/seed/conftest.py (2)
context(14-15)operations_client(24-25)seed/helper.py (1)
init_resource(44-84)
tests/seed/catalog/test_catalog.py (5)
seed/catalog/catalog.py (1)
seed_catalog(11-19)seed/catalog/product.py (1)
seed_product(37-51)seed/catalog/authorization.py (1)
seed_authorization(12-14)seed/catalog/price_list.py (1)
seed_price_list(10-12)seed/catalog/listing.py (1)
seed_listing(10-12)
seed/accounts/licensee.py (4)
seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)seed/context.py (1)
Context(12-33)seed/container.py (1)
Container(7-30)mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)
seed/accounts/user_group.py (3)
seed/helper.py (2)
init_resource(44-84)require_context_id(23-40)seed/context.py (1)
Context(12-33)seed/container.py (1)
Container(7-30)
tests/seed/accounts/test_licensee.py (4)
mpt_api_client/resources/accounts/licensees.py (1)
Licensee(21-22)seed/accounts/licensee.py (2)
build_licensee_data(16-44)create_licensee(48-52)tests/seed/conftest.py (2)
context(14-15)operations_client(24-25)seed/helper.py (1)
init_resource(44-84)
seed/accounts/seller.py (4)
seed/helper.py (1)
init_resource(44-84)mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)seed/container.py (1)
Container(7-30)mpt_api_client/resources/accounts/sellers.py (1)
Seller(13-14)
tests/seed/accounts/test_accounts.py (3)
mpt_api_client/resources/accounts/account.py (1)
Account(27-28)seed/accounts/accounts.py (2)
get_account(16-21)seed_accounts(24-36)mpt_api_client/models/model.py (1)
id(119-121)
🪛 Ruff (0.14.8)
seed/accounts/module.py
26-26: Avoid specifying long messages outside the exception class
(TRY003)
tests/seed/accounts/test_module.py
8-8: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
9-9: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
seed/accounts/accounts.py
16-16: Unused noqa directive (non-enabled: RUF029)
Remove unused noqa directive
(RUF100)
20-20: Avoid specifying long messages outside the exception class
(TRY003)
24-24: Unused noqa directive (unknown: WPS213, WPS217)
Remove unused noqa directive
(RUF100)
seed/accounts/licensee.py
16-16: Unused noqa directive (unknown: WPS238)
Remove unused noqa directive
(RUF100)
tests/seed/accounts/test_accounts.py
22-22: 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)
seed/accounts/seller.py (2)
32-38:create_sellerfactory wiring looks correct (inject + Provide)
Factory is zero-arg callable forinit_resource, and@injectensuresProvide[Container.mpt_operations]resolves.
41-45: Seeding viainit_resource("accounts.seller.id", create_seller)is consistent with the new patternseed/accounts/api_tokens.py (3)
14-27: Good: explicit context prerequisites viarequire_context_id
This makes missing"accounts.account.id"/"accounts.module.id"fail fast and with better intent than ad-hoc context access.
30-37: Good:create_api_tokenis compatible withinit_resource(zero-arg + injected client)
39-43: Namespace"accounts.api_token.id"matches the.idconvention used elsewheretests/seed/catalog/test_catalog.py (1)
7-18: Test assertions now match async behavior (assert_awaited_once) and autospec improves safetyseed/accounts/accounts.py (1)
24-28: Remove unused/unknownnoqaonseed_accounts
Ruff reports the# noqa: WPS213 WPS217as unused/unknown; better to delete it and let real complexity warnings surface if/when enabled.-async def seed_accounts() -> None: # noqa: WPS213 WPS217 +async def seed_accounts() -> None:⛔ 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/accounts/test_accounts.py (2)
7-19: LGTM!The new tests for
get_accountprovide good coverage of both the failure case (missing env var) and the success case (correct Account returned).
23-42: LGTM!The test updates are excellent:
- All mocks now use
autospec=Truefor better type safetyassert_awaited_once()correctly verifies async callsinit_resourceproperly included in the assertion listThese changes address all previous feedback about adding spec and using proper async assertions.
tests/seed/accounts/test_seller.py (1)
48-52: LGTM!The test correctly uses
autospec=Truefor mocking andassert_awaited_once()for async verification.tests/seed/accounts/test_user_group.py (2)
17-32: LGTM!The test correctly sets up the required context (including
accounts.account.id) and validates the expected data structure.
35-51: LGTM!Both tests demonstrate excellent patterns:
test_create_user_groupproperly usesAsyncMockwithassert_awaited_once()test_seed_user_groupusesautospec=Trueand verifies the exact arguments passed toinit_resourcetests/seed/accounts/test_module.py (1)
35-65: LGTM!The tests are well-structured:
test_find_modulevalidates successful module retrievaltest_find_module_not_foundproperly tests the ValueError casetest_seed_moduleusesautospec=Trueand correct async assertionsseed/accounts/module.py (2)
15-27: LGTM!The
find_modulefunction is well-implemented:
- Clear RQL filtering by module name
- Proper error handling when module not found
- Returns the first matching module
30-34: LGTM!The updated
seed_modulefunction correctly delegates toinit_resourcewith thefind_modulefactory, aligning with the broader seeding refactor.tests/seed/accounts/test_licensee.py (2)
31-60: LGTM!The test comprehensively validates
build_licensee_data, ensuring all required context IDs are set and the output structure is complete.
63-79: LGTM!Both tests demonstrate excellent async testing patterns:
test_create_licenseeproperly mocks and verifies the async create flowtest_seed_licenseeusesautospec=Trueand correct async assertionsseed/accounts/user_group.py (3)
15-29: LGTM!The
build_user_group_datafunction correctly usesrequire_context_idto ensure required IDs are present, improving error handling over environment variable access.
32-38: LGTM!The
create_user_groupfunction is correctly implemented with the@injectdecorator, ensuring dependency injection works properly. This addresses the previous feedback about the missing decorator.
41-45: LGTM!The updated
seed_user_groupfunction correctly delegates toinit_resource, aligning with the standardized seeding pattern across the module.seed/accounts/buyer.py (2)
15-35: LGTM!The
build_buyer_datafunction correctly usesrequire_context_idto validate required IDs are present, and returns a comprehensive buyer data structure.
38-53: LGTM!Both functions are well-implemented:
create_buyerproperly uses the@injectdecorator, handles file I/O with a context manager, and creates the buyerseed_buyercorrectly delegates toinit_resourcefollowing the standardized patternseed/accounts/licensee.py (3)
47-52: Verify DI wiring + file upload semantics forcreate_licensee().
Sincecreate_licensee()andbuild_licensee_data()are invoked with no args and rely ondependency-injector(@inject+Provide[...]), ensure the application wiresseed.accounts.licenseeinto the container at runtime. Also confirmmpt_client.accounts.licensees.create(..., file=icon_fd)fully reads the file during the awaited call (so thewith ICON.open(...)scope is safe).
55-59:seed_licenseeorchestration viainit_resourcelooks consistent with the new seeding pattern.
Good use of a single id namespace (accounts.licensee.id) and a create-only factory.
16-16: Remove unused# noqa: WPS238to satisfy Ruff.
Ruff reports the directive is unused on Line 16.⛔ 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.
| account_id = require_context_id(context, "accounts.account.id", "creating licensee") | ||
| seller_id = require_context_id(context, "accounts.seller.id", "create licensee") | ||
| buyer_id = require_context_id(context, "accounts.buyer.id", "create licensee") | ||
| user_group_id = require_context_id(context, "accounts.user_group.id", "create licensee") |
There was a problem hiding this comment.
Unify action strings passed to require_context_id for consistent error messages.
Line 20 uses "creating licensee" while Lines 21-23 use "create licensee". Pick one phrasing (e.g., "create licensee") so missing-context errors read consistently.
🤖 Prompt for AI Agents
In seed/accounts/licensee.py around lines 20 to 23, the action string passed to
require_context_id is inconsistent: line 20 uses "creating licensee" while lines
21–23 use "create licensee"; change the action on line 20 to "create licensee"
so all four calls use the identical phrase and produce consistent
missing-context error messages.
|
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.