[MPT-16315] Updated account seeding and tests #155
Conversation
robcsegal
requested review from
albertsola,
alephsur,
d3rky,
jentyk,
ruben-sebrango and
svazquezco
as code owners
WalkthroughThis PR refactors account seeding by introducing a unified env-var-based account initializer, replacing specific get/init patterns with direct creation functions across modules, renaming test fixtures from buyer_account_id to client_account_id, and updating init_resource to handle both string and object return types. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25–35 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
✅ Found Jira issue key in the title: MPT-16315 Tests mirroring check (created files only)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/seed/accounts/test_buyer.py (1)
15-38: Test will fail due to key casing mismatch.The expected data uses
"lastName"(camelCase), but according to the implementation inseed/accounts/buyer.py(lines 22-24), the actual key is"LastName"(PascalCase with capital L):"contact": { "firstName": "first", "LastName": "last", # Note: capital L ... }This test will fail with an assertion error.
Apply this diff to fix the expected key:
"contact": { "firstName": "first", - "lastName": "last", + "LastName": "last", "email": "created.buyer@example.com", },Alternatively, if
lastNameis the intended API contract, the fix should be applied toseed/accounts/buyer.pyinstead.
🧹 Nitpick comments (6)
seed/accounts/account.py (3)
15-21: Remove unused noqa directive and refactor error message.Two minor issues flagged by static analysis:
- The
noqa: RUF029directive on line 15 is unused and should be removed- The error message on line 19 should be extracted to follow TRY003 guidelines
Apply this diff:
@inject -async def init_vendor_account_id(context: Context = Provide[Container.context]) -> Account: # noqa: RUF029 +async def init_vendor_account_id(context: Context = Provide[Container.context]) -> Account: """Initialize account ID in context.""" account_id = os.getenv("VENDOR_ACCOUNT_ID") if not account_id: - raise ValueError("VENDOR_ACCOUNT_ID environment variable is required") + msg = "VENDOR_ACCOUNT_ID environment variable is required" + raise ValueError(msg) context["accounts.account.id"] = account_id return Account({"id": account_id})
25-31: Remove unused noqa directive and refactor error message.Same issues as
init_vendor_account_id:
- The
noqa: RUF029directive on line 25 is unused and should be removed- The error message on line 29 should be extracted to follow TRY003 guidelines
Apply this diff:
@inject -async def init_client_account_id(context: Context = Provide[Container.context]) -> Account: # noqa: RUF029 +async def init_client_account_id(context: Context = Provide[Container.context]) -> Account: """Initialize client account ID in context.""" account_id = os.getenv("CLIENT_ACCOUNT_ID") if not account_id: - raise ValueError("CLIENT_ACCOUNT_ID environment variable is required") + msg = "CLIENT_ACCOUNT_ID environment variable is required" + raise ValueError(msg) context["accounts.client_account.id"] = account_id return Account({"id": account_id})
34-39: Clarify the docstring.The docstring says "Seed account" but the function seeds both vendor and client account IDs. Consider updating for clarity.
Apply this diff:
async def seed_account_ids() -> None: - """Seed account.""" + """Seed account IDs.""" logger.debug("Seeding vendor account ...") await init_resource("accounts.account.id", init_vendor_account_id) await init_resource("accounts.client_account.id", init_client_account_id) logger.debug("Seeding account completed.")seed/accounts/licensee.py (1)
16-45: Remove unused noqa directive; refactoring looks good.Static analysis flags the
noqa: WPS238directive on line 16 as unused (unknown rule). The refactored function correctly usesrequire_context_idfor context-based ID retrieval, which is a clean improvement.Apply this diff to remove the unused directive:
@inject -def build_licensee_data( # noqa: WPS238 +def build_licensee_data( context: Context = Provide[Container.context], ) -> dict[str, object]:tests/seed/accounts/test_seller.py (1)
7-9: Unused fixturecontext_without_data.This fixture is defined but not used in any test within this file. Consider removing it unless it's intended for future use.
-@pytest.fixture -def context_without_data() -> Context: - return Context() - -tests/seed/accounts/test_licensee.py (1)
62-69: Tighten assertion oninit_resourcecall intest_seed_licenseeRight now the test only checks that
init_resourcewas awaited once. Asserting the exact namespace and factory would better lock in the new seeding contract:- mock_init_resource.assert_awaited_once() + mock_init_resource.assert_awaited_once_with("accounts.licensee.id", create_licensee)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
e2e_config.test.json(1 hunks)mpt_api_client/resources/accounts/account.py(2 hunks)mpt_api_client/resources/accounts/buyers.py(2 hunks)mpt_api_client/resources/accounts/licensees.py(2 hunks)seed/accounts/account.py(1 hunks)seed/accounts/accounts.py(2 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)tests/e2e/accounts/buyers/conftest.py(1 hunks)tests/e2e/accounts/buyers/test_async_buyers.py(3 hunks)tests/e2e/accounts/buyers/test_sync_buyers.py(2 hunks)tests/e2e/accounts/licensees/conftest.py(2 hunks)tests/e2e/catalog/pricing_policies/conftest.py(1 hunks)tests/e2e/conftest.py(1 hunks)tests/seed/accounts/test_account.py(1 hunks)tests/seed/accounts/test_accounts.py(2 hunks)tests/seed/accounts/test_api_token.py(1 hunks)tests/seed/accounts/test_buyer.py(2 hunks)tests/seed/accounts/test_licensee.py(2 hunks)tests/seed/accounts/test_module.py(3 hunks)tests/seed/accounts/test_seller.py(2 hunks)tests/seed/accounts/test_user_group.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-27T00:05:54.701Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/accounts/accounts_users/test_async_accounts_users.py:0-0
Timestamp: 2025-11-27T00:05:54.701Z
Learning: In pytest-asyncio, async fixtures are automatically resolved before being passed to test functions (both sync and async). Test functions receive the yielded value from async fixtures, not coroutines, so fixture parameters should never be awaited. A sync test function can use async fixtures without any special handling.
Applied to files:
tests/e2e/accounts/buyers/test_async_buyers.py
🧬 Code graph analysis (20)
mpt_api_client/resources/accounts/account.py (1)
mpt_api_client/resources/accounts/mixins.py (6)
ActivatableMixin(4-27)EnablableMixin(30-53)ValidateMixin(56-68)AsyncActivatableMixin(134-159)AsyncEnablableMixin(162-185)AsyncValidateMixin(188-200)
seed/accounts/api_tokens.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/api_tokens.py (1)
ApiToken(12-13)
tests/seed/accounts/test_account.py (4)
seed/accounts/account.py (3)
init_client_account_id(25-31)init_vendor_account_id(15-21)seed_account_ids(34-39)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)seed/helper.py (1)
init_resource(44-84)
tests/e2e/accounts/licensees/conftest.py (2)
tests/e2e/conftest.py (1)
client_account_id(127-128)tests/e2e/accounts/conftest.py (1)
user_group_factory(45-60)
seed/accounts/module.py (4)
seed/helper.py (1)
init_resource(44-84)seed/context.py (1)
Context(12-33)mpt_api_client/resources/accounts/modules.py (1)
Module(11-12)mpt_api_client/models/model.py (1)
id(119-121)
mpt_api_client/resources/accounts/buyers.py (3)
mpt_api_client/http/mixins.py (4)
CreateFileMixin(115-144)UpdateFileMixin(147-184)AsyncCreateFileMixin(187-216)AsyncUpdateFileMixin(219-256)mpt_api_client/resources/accounts/mixins.py (6)
ActivatableMixin(4-27)EnablableMixin(30-53)ValidateMixin(56-68)AsyncActivatableMixin(134-159)AsyncEnablableMixin(162-185)AsyncValidateMixin(188-200)mpt_api_client/resources/catalog/mixins.py (2)
ActivatableMixin(109-132)AsyncActivatableMixin(135-160)
mpt_api_client/resources/accounts/licensees.py (2)
mpt_api_client/http/mixins.py (4)
CreateFileMixin(115-144)UpdateFileMixin(147-184)AsyncCreateFileMixin(187-216)AsyncUpdateFileMixin(219-256)mpt_api_client/resources/accounts/mixins.py (2)
EnablableMixin(30-53)AsyncEnablableMixin(162-185)
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)
tests/seed/accounts/test_licensee.py (4)
seed/accounts/licensee.py (3)
build_licensee_data(16-45)create_licensee(49-57)seed_licensee(60-64)seed/context.py (1)
Context(12-33)tests/seed/accounts/test_module.py (1)
context_with_data(23-26)tests/seed/accounts/test_user_group.py (1)
context_with_data(12-16)
seed/accounts/licensee.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/licensees.py (1)
Licensee(21-22)
tests/e2e/accounts/buyers/conftest.py (1)
tests/e2e/conftest.py (2)
client_account_id(127-128)account_id(122-123)
tests/e2e/catalog/pricing_policies/conftest.py (1)
tests/e2e/conftest.py (2)
client_account_id(127-128)product_id(96-97)
seed/accounts/accounts.py (2)
seed/accounts/account.py (1)
seed_account_ids(34-39)tests/e2e/conftest.py (1)
logger(63-64)
tests/seed/accounts/test_api_token.py (4)
seed/accounts/api_tokens.py (3)
create_api_token(31-38)seed_api_token(41-45)build_api_token_data(15-27)tests/seed/conftest.py (2)
context(14-15)operations_client(24-25)seed/context.py (1)
Context(12-33)tests/seed/accounts/test_module.py (1)
context_with_data(23-26)
tests/seed/accounts/test_seller.py (3)
seed/accounts/seller.py (3)
build_seller_data(14-29)create_seller(33-38)seed_seller(41-45)tests/seed/conftest.py (2)
context(14-15)operations_client(24-25)seed/context.py (1)
Context(12-33)
tests/seed/accounts/test_buyer.py (2)
seed/accounts/buyer.py (3)
build_buyer_data(16-37)create_buyer(41-49)seed_buyer(52-56)seed/context.py (1)
Context(12-33)
tests/e2e/accounts/buyers/test_sync_buyers.py (3)
tests/e2e/conftest.py (3)
mpt_ops(31-32)client_account_id(127-128)account_id(122-123)tests/e2e/accounts/buyers/conftest.py (2)
buyer_factory(10-34)invalid_buyer_id(5-6)tests/e2e/accounts/conftest.py (1)
account_icon(20-21)
tests/seed/accounts/test_user_group.py (2)
seed/accounts/user_group.py (3)
create_user_group(33-39)seed_user_group(42-46)build_user_group_data(15-29)seed/context.py (1)
Context(12-33)
tests/seed/accounts/test_module.py (4)
seed/accounts/module.py (2)
refresh_module(16-31)seed_module(34-38)tests/seed/accounts/test_user_group.py (1)
context_with_data(12-16)seed/context.py (1)
Context(12-33)mpt_api_client/resources/accounts/modules.py (1)
Module(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)
🪛 Ruff (0.14.8)
seed/accounts/account.py
15-15: Unused noqa directive (non-enabled: RUF029)
Remove unused noqa directive
(RUF100)
19-19: Avoid specifying long messages outside the exception class
(TRY003)
25-25: Unused noqa directive (non-enabled: RUF029)
Remove unused noqa directive
(RUF100)
29-29: Avoid specifying long messages outside the exception class
(TRY003)
tests/seed/accounts/test_licensee.py
43-43: Unused noqa directive (unknown: WPS218)
Remove unused noqa directive
(RUF100)
seed/accounts/licensee.py
16-16: Unused noqa directive (unknown: WPS238)
Remove unused noqa directive
(RUF100)
seed/accounts/accounts.py
14-14: Unused noqa directive (unknown: WPS217, WPS213)
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 (53)
e2e_config.test.json (1)
4-4: LGTM! Configuration key updated consistently.The account identifier has been successfully renamed to
accounts.client_account.id, aligning with the broader refactoring effort across the codebase.seed/accounts/module.py (2)
10-10: LGTM! Import added for new seeding pattern.The
init_resourceimport supports the refactored seeding approach and is consistent with changes across other seed modules.
16-31: LGTM! Simplified module fetching logic.The refactored
refresh_modulefunction correctly:
- Fetches the "Access Management" module directly
- Validates the module type
- Stores the module ID in context
- Handles edge cases with appropriate warnings
mpt_api_client/resources/accounts/account.py (1)
42-46: LGTM! Type parameters tightened for improved type safety.The mixin type parameters have been correctly updated from
ModeltoAccountfor both synchronous and asynchronous service classes. This change improves type safety without affecting runtime behavior and is consistent with similar updates across other account service modules.Also applies to: 62-66
seed/accounts/licensee.py (3)
9-9: LGTM! Imports added for context-based ID handling.The new imports support the refactored seeding pattern that uses
require_context_idfor ID validation andinit_resourcefor resource initialization.
49-57: LGTM! Clean licensee creation implementation.The
create_licenseefunction follows the consistent pattern established across other seed modules, building the licensee data from context and creating it via the API.
60-64: LGTM! Seeding flow updated to use init_resource pattern.The refactored
seed_licenseefunction correctly uses the standardizedinit_resourcehelper with thecreate_licenseefactory.seed/accounts/seller.py (1)
9-9: LGTM! Seller seeding refactored consistently.The changes successfully refactor the seller seeding to follow the standardized pattern:
- Added
init_resourceimport- Renamed
init_sellertocreate_sellerwith simplified implementation- Updated
seed_sellerto useinit_resourcewith thecreate_sellerfactoryThese changes are consistent with the refactoring across other seed modules.
Also applies to: 33-38, 41-45
mpt_api_client/resources/accounts/licensees.py (1)
36-38: LGTM! Type parameters tightened for improved type safety.The mixin type parameters have been correctly updated from
ModeltoLicenseefor both synchronous and asynchronous service classes. This enhances type safety and is consistent with the refactoring pattern applied across other account service modules.Also applies to: 49-51
mpt_api_client/resources/accounts/buyers.py (1)
41-45: LGTM! Type parameters tightened for improved type safety.The mixin type parameters have been correctly updated from
ModeltoBuyerfor both synchronous and asynchronous service classes. This enhances type safety and completes the consistent refactoring pattern applied across all account service modules.Also applies to: 74-78
tests/seed/accounts/test_accounts.py (1)
5-7: LGTM!The test correctly mocks
seed_account_idsas anAsyncMockand includes it in the assertion list to verify it's called exactly once during the seeding process. This aligns with the implementation inseed/accounts/accounts.py.Also applies to: 27-35
tests/e2e/conftest.py (1)
126-129: LGTM!The new
client_account_idfixture follows the established pattern for ID fixtures in this file and aligns with the broader refactor to standardize onclient_accountterminology.tests/e2e/catalog/pricing_policies/conftest.py (1)
7-11: LGTM!The parameter rename from
buyer_account_idtoclient_account_idis consistent with the fixture changes intests/e2e/conftest.py. The"client"field in the payload semantically aligns with the new naming convention.tests/e2e/accounts/buyers/conftest.py (1)
10-14: LGTM!The
buyer_factoryfixture correctly usesclient_account_idas both the fixture dependency and the default value for theaccount_idparameter, maintaining consistency with the naming convention changes across the test suite.seed/accounts/accounts.py (2)
3-3: LGTM!The addition of
seed_account_ids()as the first step in the seeding sequence is correct—account IDs must be initialized before dependent resources (seller, buyer, etc.) are seeded.Also applies to: 17-18
14-14: Remove this suggestion—thenoqadirectives are necessary.The project uses wemake-python-styleguide, which defines WPS217 and WPS213. These noqa directives are valid and should be retained.
tests/seed/accounts/test_account.py (3)
7-12: LGTM!The
context_with_datafixture provides a pre-populated context for testing the init functions, allowing verification that the environment values correctly overwrite existing context entries.
15-21: LGTM!Both tests correctly verify:
- The returned
Accountobject has the expected ID from the environment variable- The context is mutated with the new ID value
This provides good coverage for the init functions' dual responsibilities.
Also applies to: 24-30
33-42: LGTM!The test correctly mocks
init_resourceand verifies it's called with the expected namespace keys and init functions for both vendor and client accounts.tests/e2e/accounts/buyers/test_sync_buyers.py (1)
10-10: LGTM!All references to
buyer_account_idhave been consistently replaced withclient_account_idacross fixtures and test functions, aligning with the broader refactor.Also applies to: 14-14, 78-78, 80-80, 88-88, 91-91
tests/e2e/accounts/licensees/conftest.py (1)
15-15: LGTM!The
licensee_factorycorrectly usesclient_account_idfor both the user group creation and the licensee's account reference, maintaining consistency with the broader test suite refactor.Also applies to: 20-20, 34-34
seed/accounts/api_tokens.py (4)
9-9: LGTM!The new imports for
init_resourceandrequire_context_idfromseed.helperalign with the refactored seeding pattern used across other account modules.
14-27: LGTM!The refactor to use
require_context_idfor fetchingaccount_idandmodule_idprovides better error handling with clear context about which resource is missing and for what action. This is consistent with the pattern used inbuild_buyer_dataandbuild_user_group_data.
30-38: LGTM!The rename from
init_api_tokentocreate_api_tokenwith create-only semantics is a good simplification. The function now has a single responsibility and delegates the "get-or-create" logic toinit_resource.
41-45: LGTM!The
seed_api_tokenfunction now uses theinit_resourcepattern, which handles caching the resource ID in the context and only callscreate_api_tokenwhen needed. This is consistent with other seed functions in the codebase.tests/seed/accounts/test_seller.py (2)
32-43: LGTM!The test correctly validates the
create_sellerflow by mocking the API call, invoking the function, and asserting both the return value and the payload structure.
46-53: LGTM!The test correctly patches
init_resourceand verifies it's awaited once whenseed_selleris called, aligning with the new seeding pattern.tests/e2e/accounts/buyers/test_async_buyers.py (3)
9-14: LGTM!The rename from
buyer_account_idtoclient_account_idin theasync_created_buyerfixture is consistent with the broader PR refactoring and is properly propagated to thebuyer_factorycall.
80-84: LGTM!The parameter rename is correctly applied to
test_update_buyer.
93-97: LGTM!The parameter rename is correctly applied to
test_update_buyer_not_found.tests/seed/accounts/test_module.py (4)
4-4: LGTM!The import is updated to reflect the removal of
get_moduleand the new seeding flow that usesrefresh_moduleandseed_module.
22-26: LGTM!The
context_with_datafixture provides the pre-populatedaccounts.module.idneeded for testing the module refresh flow.
43-55: LGTM!The test correctly uses the new
context_with_datafixture and validates thatrefresh_modulereturns the expected module when the API call is mocked.
58-65: LGTM!The test correctly patches
init_resourceand verifies it's awaited once whenseed_moduleis called.tests/seed/accounts/test_buyer.py (2)
41-55: LGTM!The test correctly validates the
create_buyerflow by mocking the API call and verifying both the return value and critical payload fields.
58-65: LGTM!The test correctly patches
init_resourceand verifies it's awaited once whenseed_buyeris called.tests/seed/accounts/test_user_group.py (5)
5-5: LGTM!The import is updated to use
create_user_groupinstead of the removedinit_user_group, aligning with the new create-focused seeding flow.
11-16: LGTM!The
context_with_datafixture provides the requiredaccounts.client_account.idandaccounts.module.idvalues for testingbuild_user_group_dataandcreate_user_group.
19-31: LGTM!The test correctly validates that
build_user_group_dataproduces the expected payload using IDs from the context fixture.
34-48: LGTM!The test validates the
create_user_groupflow by mocking the API call and verifying both the return value and the payload structure.
51-58: LGTM!The test correctly patches
init_resourceand verifies it's awaited once whenseed_user_groupis called.seed/accounts/user_group.py (4)
9-9: LGTM!The new imports for
init_resourceandrequire_context_idfromseed.helperalign with the standardized seeding pattern used across other account modules.
14-29: LGTM!The refactor to use
require_context_idfor fetchingaccount_idandmodule_idprovides consistent error handling with clear context about which resource is missing. This matches the pattern inbuild_api_token_dataandbuild_buyer_data.
32-39: LGTM!The rename from
init_user_grouptocreate_user_groupwith a simplified return type (UserGroupinstead ofUserGroup | None) provides clearer semantics. The "get-or-create" logic is now handled byinit_resource.
42-46: LGTM!The
seed_user_groupfunction now uses theinit_resourcepattern, consistent with other seed functions likeseed_api_token,seed_buyer, andseed_seller.tests/seed/accounts/test_licensee.py (2)
8-14: Fixture and data-shape expectations look consistent with licensee builderThe
context_with_datafixture andtest_build_licensee_dataexpectations line up withbuild_licensee_data’s use ofaccounts.client_account.id,accounts.seller.id,accounts.buyer.id, andaccounts.user_group.id. This should give good confidence the new context-driven wiring behaves as intended.Also applies to: 17-40
43-59: Remove unused# noqa: WPS218to satisfy RuffRuff reports the
# noqa: WPS218on Line 43 as unused. Since this rule is no longer being triggered fortest_create_licensee, the directive should be dropped.-async def test_create_licensee(mocker, client_client, context_with_data): # noqa: WPS218 +async def test_create_licensee(mocker, client_client, context_with_data):⛔ Skipped due to learnings
Learnt from: jentyk Repo: softwareone-platform/mpt-api-python-client PR: 136 File: tests/e2e/notifications/subscribers/conftest.py:14-25 Timestamp: 2025-12-01T10:48:52.586Z Learning: The repository softwareone-platform/mpt-api-python-client uses wemake-python-styleguide linter, so noqa directives for WPS rules should not be flagged as unused.tests/seed/accounts/test_api_token.py (3)
12-28: Context fixture andtest_build_api_token_datacorrectly reflect new context-based IDsThe
context_with_datafixture andexpected_dataintest_build_api_token_dataalign withbuild_api_token_data’s use ofaccounts.client_account.idandaccounts.module.id. This looks correct and keeps the tests close to the seeding contract.
33-47:test_create_api_tokenthoroughly validates the create payloadThe AsyncMock wiring and payload assertions ensure
create_api_tokenpasses the expected account and module IDs intompt_ops.accounts.api_tokens.create, which should catch regressions in the new create-based flow.
49-56:test_seed_api_tokencorrectly asserts init_resource wiringPatching
seed.accounts.api_tokens.init_resourceand asserting it is called once with"accounts.api_token.id"andcreate_api_tokenneatly validates the newinit_resource-based seeding behavior.seed/accounts/buyer.py (3)
9-9: Context-driven buyer data construction looks correctSwitching
build_buyer_datato userequire_context_idforaccounts.client_account.idandaccounts.seller.idmatches the new seeding convention and yields a payload shape consistent with the Buyer API.Also applies to: 16-37
40-50:create_buyerimplementation matches the new create+ICON patternUsing
build_buyer_data(context=context)with the injectedContextandmpt_operations, and passingICON.open("rb")intobuyers.create, is consistent with the other account seeding flows and keeps the creation path straightforward.
52-56:seed_buyercorrectly delegates toinit_resource
seed_buyernow defers toinit_resource("accounts.buyer.id", create_buyer), which aligns with the shared seeding helper and should ensure the buyer is created on demand and cached in the context.
robcsegal
force-pushed
the
MPT-16315-update-account-seeding-to-latest-convention
branch
from
22c94cf to
54a00af
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tests/seed/accounts/test_buyer.py (1)
58-65: Consider verifyinginit_resourcearguments for more robust test coverage.The test only asserts that
init_resourcewas awaited once but doesn't verify the namespace or factory function passed. This could miss bugs where the wrong namespace or factory is used.async def test_seed_buyer(mocker): mock_init_resource = mocker.patch( "seed.accounts.buyer.init_resource", new_callable=mocker.AsyncMock ) await seed_buyer() - mock_init_resource.assert_awaited_once() + mock_init_resource.assert_awaited_once_with("accounts.buyer.id", create_buyer)seed/accounts/licensee.py (1)
16-16: Remove unusednoqadirective.Static analysis (Ruff) indicates the
noqa: WPS238directive is no longer needed.-def build_licensee_data( # noqa: WPS238 +def build_licensee_data( context: Context = Provide[Container.context], ) -> dict[str, object]:tests/seed/accounts/test_licensee.py (1)
62-69: Consider validatinginit_resourcearguments.Unlike similar tests in
test_api_token.pyandtest_user_group.py, this test only verifies thatinit_resourcewas called but doesn't validate the arguments (namespace and factory function). Consider adding argument validation for consistency.await seed_licensee() - mock_init_resource.assert_awaited_once() + mock_init_resource.assert_awaited_once_with("accounts.licensee.id", create_licensee)tests/seed/accounts/test_user_group.py (1)
51-58: Consider validatinginit_resourcearguments.Similar to
test_seed_licensee, this test only verifies thatinit_resourcewas called but doesn't validate the arguments. For consistency withtest_seed_api_token, consider adding argument validation.await seed_user_group() - mock_init_resource.assert_awaited_once() + mock_init_resource.assert_awaited_once_with("accounts.user_group.id", create_user_group)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
e2e_config.test.json(1 hunks)mpt_api_client/resources/accounts/account.py(2 hunks)mpt_api_client/resources/accounts/buyers.py(2 hunks)mpt_api_client/resources/accounts/licensees.py(2 hunks)seed/accounts/account.py(1 hunks)seed/accounts/accounts.py(2 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)tests/e2e/accounts/buyers/conftest.py(1 hunks)tests/e2e/accounts/buyers/test_async_buyers.py(3 hunks)tests/e2e/accounts/buyers/test_sync_buyers.py(2 hunks)tests/e2e/accounts/licensees/conftest.py(2 hunks)tests/e2e/catalog/pricing_policies/conftest.py(1 hunks)tests/e2e/conftest.py(1 hunks)tests/seed/accounts/test_account.py(1 hunks)tests/seed/accounts/test_accounts.py(2 hunks)tests/seed/accounts/test_api_token.py(1 hunks)tests/seed/accounts/test_buyer.py(2 hunks)tests/seed/accounts/test_licensee.py(2 hunks)tests/seed/accounts/test_module.py(3 hunks)tests/seed/accounts/test_seller.py(2 hunks)tests/seed/accounts/test_user_group.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- e2e_config.test.json
- tests/e2e/catalog/pricing_policies/conftest.py
- tests/e2e/conftest.py
- mpt_api_client/resources/accounts/account.py
- tests/e2e/accounts/buyers/conftest.py
- tests/seed/accounts/test_account.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-27T00:05:54.701Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/accounts/accounts_users/test_async_accounts_users.py:0-0
Timestamp: 2025-11-27T00:05:54.701Z
Learning: In pytest-asyncio, async fixtures are automatically resolved before being passed to test functions (both sync and async). Test functions receive the yielded value from async fixtures, not coroutines, so fixture parameters should never be awaited. A sync test function can use async fixtures without any special handling.
Applied to files:
tests/e2e/accounts/buyers/test_async_buyers.py
🧬 Code graph analysis (15)
tests/seed/accounts/test_user_group.py (3)
seed/accounts/user_group.py (2)
create_user_group(33-39)build_user_group_data(15-29)seed/context.py (1)
Context(12-33)tests/seed/accounts/test_module.py (1)
context_with_data(23-26)
seed/accounts/seller.py (2)
seed/helper.py (1)
init_resource(44-84)mpt_api_client/resources/accounts/sellers.py (1)
Seller(13-14)
tests/e2e/accounts/licensees/conftest.py (2)
tests/e2e/conftest.py (3)
seller_id(132-133)buyer_id(137-138)client_account_id(127-128)tests/e2e/accounts/conftest.py (1)
user_group_factory(45-60)
tests/seed/accounts/test_seller.py (3)
seed/accounts/seller.py (3)
build_seller_data(14-29)create_seller(33-38)seed_seller(41-45)tests/seed/conftest.py (2)
context(14-15)operations_client(24-25)seed/context.py (1)
Context(12-33)
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)
mpt_api_client/resources/accounts/buyers.py (2)
mpt_api_client/http/mixins.py (4)
CreateFileMixin(115-144)UpdateFileMixin(147-184)AsyncCreateFileMixin(187-216)AsyncUpdateFileMixin(219-256)mpt_api_client/resources/catalog/mixins.py (2)
ActivatableMixin(109-132)AsyncActivatableMixin(135-160)
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)
tests/seed/accounts/test_api_token.py (2)
seed/accounts/api_tokens.py (3)
create_api_token(31-38)seed_api_token(41-45)build_api_token_data(15-27)seed/context.py (1)
Context(12-33)
seed/accounts/module.py (4)
seed/helper.py (1)
init_resource(44-84)seed/context.py (1)
Context(12-33)seed/container.py (1)
Container(7-30)mpt_api_client/resources/accounts/modules.py (1)
Module(11-12)
tests/e2e/accounts/buyers/test_sync_buyers.py (3)
tests/e2e/conftest.py (2)
client_account_id(127-128)account_id(122-123)tests/e2e/accounts/buyers/conftest.py (2)
buyer_factory(10-34)invalid_buyer_id(5-6)tests/e2e/accounts/conftest.py (1)
account_icon(20-21)
seed/accounts/buyer.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/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/accounts.py (1)
seed/accounts/account.py (1)
seed_account_ids(34-39)
mpt_api_client/resources/accounts/licensees.py (2)
mpt_api_client/http/mixins.py (4)
CreateFileMixin(115-144)UpdateFileMixin(147-184)AsyncCreateFileMixin(187-216)AsyncUpdateFileMixin(219-256)mpt_api_client/resources/accounts/mixins.py (2)
EnablableMixin(30-53)AsyncEnablableMixin(162-185)
seed/accounts/account.py (4)
mpt_api_client/resources/accounts/account.py (1)
Account(27-28)seed/container.py (1)
Container(7-30)seed/context.py (1)
Context(12-33)seed/helper.py (1)
init_resource(44-84)
🪛 Ruff (0.14.8)
tests/seed/accounts/test_licensee.py
43-43: Unused noqa directive (unknown: WPS218)
Remove unused noqa directive
(RUF100)
seed/accounts/licensee.py
16-16: Unused noqa directive (unknown: WPS238)
Remove unused noqa directive
(RUF100)
seed/accounts/accounts.py
14-14: Unused noqa directive (unknown: WPS217, WPS213)
Remove unused noqa directive
(RUF100)
seed/accounts/account.py
15-15: Unused noqa directive (non-enabled: RUF029)
Remove unused noqa directive
(RUF100)
19-19: Avoid specifying long messages outside the exception class
(TRY003)
25-25: Unused noqa directive (non-enabled: RUF029)
Remove unused noqa directive
(RUF100)
29-29: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (38)
mpt_api_client/resources/accounts/licensees.py (2)
36-38: LGTM! Type parameter tightening improves type safety.Narrowing the generic type parameters from
ModeltoLicenseeforCreateFileMixin,UpdateFileMixin, andEnablableMixinprovides better type inference for callers. Methods likecreate(),update(),enable(), anddisable()will now returnLicenseeinstead of the more genericModel, improving IDE support and type checking.
49-51: LGTM! Async mixins updated consistently.The async service mixins mirror the synchronous service changes, ensuring consistent type safety across both synchronous and asynchronous API surfaces. This completes the pattern by aligning all mixins with the specific
Licenseetype.tests/e2e/accounts/licensees/conftest.py (1)
15-34: LGTM! Consistent naming refactor.The parameter rename from
licensee_account_idtoclient_account_idis consistently applied throughout the fixture and aligns with the broader PR changes.tests/seed/accounts/test_accounts.py (1)
5-7: LGTM! Proper test coverage for new seed function.The test correctly patches and asserts
seed_account_idsis invoked during theseed_accounts()flow, following the same pattern as the existing seed function mocks.Also applies to: 28-28
tests/seed/accounts/test_seller.py (1)
3-3: LGTM! Tests properly validate the refactored seller seeding.The tests correctly validate both the
create_sellerfunction (payload structure and API call) and theseed_sellerorchestration throughinit_resource. The newcontext_without_datafixture is defined but unused in the current tests—this is acceptable if it's intended for future use or other test modules.Also applies to: 8-9, 32-53
tests/seed/accounts/test_module.py (1)
4-4: LGTM! Tests properly validate the refactored module seeding.The addition of
context_with_datafixture and the updates to both test functions correctly validate the refactored module seeding flow that relies on context-based IDs andinit_resourceorchestration.Also applies to: 22-27, 43-44, 54-54, 58-65
seed/accounts/module.py (1)
10-10: LGTM! Refactoring addresses previous feedback.The module seeding has been properly refactored:
refresh_modulenow directly filters and fetches the module, storing its ID in contextseed_modulereturn type has been corrected to-> None(addressing the previous review comment)- The seed orchestration properly uses
init_resourceAlso applies to: 16-31, 34-38
seed/accounts/account.py (1)
34-39: LGTM! Proper account ID seeding orchestration.The function correctly orchestrates the initialization of both vendor and client account IDs through
init_resource, with appropriate logging.seed/accounts/user_group.py (1)
9-9: LGTM! Consistent refactor to context-based ID sourcing.The refactoring properly transitions from environment-based to context-based ID retrieval using
require_context_id. The function rename frominit_user_grouptocreate_user_groupbetter reflects its creation-only semantics, and the tightened return types (UserGroupinstead ofUserGroup | None) improve type safety. These changes align with the broader seeding refactor pattern across the PR.Also applies to: 19-20, 33-39, 42-46
seed/accounts/accounts.py (1)
3-3: LGTM! The noqa directive is necessary.The addition of
seed_account_ids()at the start of the seeding flow is appropriate, ensuring account IDs are available for dependent seeds.The
noqa: WPS217, WPS213directive on line 14 is correctly used. The repository uses wemake-python-styleguide, and these WPS rules are active for this file (they are not disabled in the per-file-ignores configuration forseed/accounts/*.py). The directive should be kept as-is.tests/seed/accounts/test_buyer.py (2)
7-12: LGTM!The
context_with_datafixture correctly populates the required context keys (accounts.client_account.idandaccounts.seller.id) needed bybuild_buyer_data.
41-55: LGTM!The test properly mocks the create method, invokes
create_buyer, and verifies both the return value and key payload fields.seed/accounts/licensee.py (2)
20-23: LGTM!The use of
require_context_idfor all four dependencies (client_account,seller,buyer,user_group) ensures clear error messages if any prerequisite resource is missing.
48-57: LGTM!The
create_licenseefunction follows the same pattern as other create functions, properly building data and creating the resource with an icon file.tests/e2e/accounts/buyers/test_sync_buyers.py (2)
9-14: LGTM!The fixture correctly uses the renamed
client_account_idparameter, aligning with the broader refactor of account naming conventions.
78-91: LGTM!The test functions consistently use
client_account_idthroughout, maintaining alignment with the renamed fixtures and the PR's naming convention changes.seed/accounts/seller.py (2)
32-38: LGTM!The
create_sellerfunction is correctly simplified since sellers don't depend on other seeded resources. The self-contained data generation viabuild_seller_data()with UUID-basedexternalIdis appropriate.
41-45: LGTM!The
seed_sellerfunction correctly usesinit_resourcewith the appropriate namespace, following the new seeding pattern.seed/accounts/buyer.py (3)
15-37: LGTM!The
build_buyer_datafunction correctly usesrequire_context_idto fetch bothclient_account_idandseller_idfrom context, ensuring proper error handling if prerequisites are missing.
40-49: LGTM!The
create_buyerfunction follows the established pattern: building data with context, then creating via API with the icon file.
52-56: LGTM!The
seed_buyerfunction correctly delegates toinit_resourcewith theaccounts.buyer.idnamespace.mpt_api_client/resources/accounts/buyers.py (2)
40-51: LGTM!Tightening the generic type parameters from
ModeltoBuyerimproves type safety. Methods from these mixins will now correctly returnBuyerinstances instead of the genericModeltype.
73-84: LGTM!The async service class receives the same type parameter improvements, ensuring consistent type safety across both sync and async APIs.
tests/seed/accounts/test_api_token.py (4)
11-16: LGTM!The context fixture provides appropriate test data for validating the API token creation flow.
19-30: LGTM!The test correctly validates that
build_api_token_dataconstructs the expected payload structure with IDs from context.
33-47: LGTM!The test thoroughly validates both the return value and the payload structure sent to the API, ensuring that IDs are correctly sourced from the context.
49-56: LGTM!The test correctly validates that
seed_api_tokendelegates toinit_resourcewith the appropriate namespace and factory function.tests/seed/accounts/test_licensee.py (3)
7-14: LGTM!The fixture provides comprehensive test data for licensee creation, including all required account-related IDs.
17-40: LGTM!The test thoroughly validates the licensee payload structure, ensuring all fields including seller, buyer, account, and groups are correctly populated from context.
44-59: LGTM!The test comprehensively validates the licensee creation, checking both the return value and the payload structure with all required IDs.
tests/e2e/accounts/buyers/test_async_buyers.py (1)
10-14: LGTM!The rename from
buyer_account_idtoclient_account_idis consistently applied across all occurrences, aligning with the PR's objective to standardize account identifier naming.Also applies to: 81-84, 94-97
tests/seed/accounts/test_user_group.py (3)
12-16: LGTM!The context fixture appropriately provides test data for user group creation validation.
19-31: LGTM!The test correctly validates that
build_user_group_dataconstructs the expected payload with IDs sourced from context.
34-49: LGTM!The test comprehensively validates both the return value and the payload structure, ensuring IDs are correctly sourced from context.
seed/accounts/api_tokens.py (4)
9-9: LGTM!The imports correctly bring in the helper functions needed for the new context-based resource initialization pattern.
14-27: LGTM!The function correctly uses
require_context_idto ensure required IDs are present in context, providing clear error messages if dependencies are missing. The key names align with the PR's standardization of account identifier naming.
30-38: LGTM!The function has a clean implementation following the create-only pattern. It correctly delegates payload construction to
build_api_token_dataand returns the created API token.
40-45: LGTM!The seeding function correctly uses
init_resourceto manage resource creation and ID caching, ensuring the API token is created only when needed.
robcsegal
force-pushed
the
MPT-16315-update-account-seeding-to-latest-convention
branch
2 times, most recently
from
01597b4 to
b3543dd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
seed/accounts/account.py (1)
14-20: Cleanup unusedcontextparameter /noqaand align with Ruff
init_account_id()never usescontext, so Ruff reportsARG001and the# noqa: RUF029is also flagged as unused (RUF100). Either start usingcontexthere, or remove the parameter and thenoqato keep the signature minimal and satisfy linting. If your local Ruff config still requiresRUF029suppression while CI doesn’t, it’d be good to reconcile the configs so this pattern is consistent across the repo.
🧹 Nitpick comments (9)
seed/accounts/module.py (1)
23-27: Redundant context assignment.
refresh_modulestores the module ID incontext["accounts.module.id"]at line 26, butinit_resource(fromseed/helper.pylines 79-80) also stores the returned value under the same namespace. This results in the ID being written twice.Consider returning just the module and letting
init_resourcehandle the context storage:if modules: first_module = modules[0] if isinstance(first_module, Module): - context["accounts.module.id"] = first_module.id return first_module logger.warning("First module is not a Module instance.") return Nonetests/seed/accounts/test_buyer.py (1)
58-65: Consider verifyinginit_resourcearguments.The test only asserts that
init_resourcewas awaited once but doesn't verify the namespace or factory function. This could miss regressions if the namespace changes.await seed_buyer() - mock_init_resource.assert_awaited_once() + mock_init_resource.assert_awaited_once_with("accounts.buyer.id", create_buyer)tests/seed/accounts/test_licensee.py (1)
62-69: Consider verifyinginit_resourcearguments for consistency.Similar to the buyer test, verifying the namespace and factory function would make the test more robust.
await seed_licensee() - mock_init_resource.assert_awaited_once() + mock_init_resource.assert_awaited_once_with("accounts.licensee.id", create_licensee)seed/accounts/licensee.py (1)
62-64: Inconsistent logging level.
seed_licenseeuseslogger.infofor completion (line 64) while other seed functions in this PR uselogger.debugfor both start and completion messages. Consider aligning for consistency.logger.debug("Seeding licensee ...") await init_resource("accounts.licensee.id", create_licensee) - logger.info("Seeding licensee completed.") + logger.debug("Seeding licensee completed.")seed/accounts/seller.py (1)
32-38: Confirm create-only seller seeding semantics
create_seller()is now a pure create factory andseed_seller()delegates toinit_resource("accounts.seller.id", create_seller), so each time the seeding context is fresh you’ll create a new Seller (no lookup byexternalId). That’s fine if the target environment is ephemeral, but in a long-lived environment this can accumulate many seeded sellers. If you want idempotent behavior per external ID, consider aget-or-createpath keyed byexternalIdinstead.Also applies to: 41-45
seed/accounts/account.py (1)
23-38: Minor polish for seeding log message and duplication
seed_account_ids()seeds three IDs (vendor, client, operations) but the log message on Line 25 only mentions “Seeding vendor account ...”. Consider either broadening the wording (e.g., “Seeding account IDs ...”) or adding separate log lines per ID. You could also DRY the threeinit_resourcecalls via a small loop over(namespace, env_var)pairs to reduce repetition.tests/seed/accounts/test_module.py (2)
22-27: Strengthenrefresh_moduletest by asserting context mutation
refresh_module()’s main responsibility is to both return the module and set"accounts.module.id"in the providedContext.test_refresh_module()currently only asserts the return value. Consider also asserting thatcontext_with_data["accounts.module.id"]is updated tomodule.idafter the call, so regressions in the context‑write behavior are caught. The mockedmodules_service.refreshassignment is also unused and can be removed.Also applies to: 43-55
58-65: Tightenseed_moduletest to verifyinit_resourcearguments
test_seed_module()only checks thatinit_resourcewas awaited once. To better guard against regressions, assert the exact call, e.g.mock_init_resource.assert_awaited_once_with("accounts.module.id", refresh_module)so a wrong namespace or factory won’t silently pass.seed/accounts/api_tokens.py (1)
30-45: Confirm create-only API token seeding behavior
create_api_token()is a pure create factory andseed_api_token()now delegates toinit_resource("accounts.api_token.id", create_api_token). As with the seller flow, this will create a new token whenever seeding runs with a freshContext; over time that can accumulate many API tokens in a persistent environment. If you expect to rerun seeding regularly against the same account/module, consider whether aget-or-createstrategy (or periodic cleanup) is preferable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
e2e_config.test.json(1 hunks)mpt_api_client/resources/accounts/account.py(2 hunks)mpt_api_client/resources/accounts/buyers.py(2 hunks)mpt_api_client/resources/accounts/licensees.py(2 hunks)seed/accounts/account.py(1 hunks)seed/accounts/accounts.py(2 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/helper.py(1 hunks)tests/e2e/accounts/buyers/conftest.py(1 hunks)tests/e2e/accounts/buyers/test_async_buyers.py(3 hunks)tests/e2e/accounts/buyers/test_sync_buyers.py(2 hunks)tests/e2e/accounts/licensees/conftest.py(2 hunks)tests/e2e/catalog/pricing_policies/conftest.py(1 hunks)tests/e2e/conftest.py(1 hunks)tests/seed/accounts/test_account.py(1 hunks)tests/seed/accounts/test_accounts.py(2 hunks)tests/seed/accounts/test_api_token.py(1 hunks)tests/seed/accounts/test_buyer.py(2 hunks)tests/seed/accounts/test_licensee.py(2 hunks)tests/seed/accounts/test_module.py(3 hunks)tests/seed/accounts/test_seller.py(2 hunks)tests/seed/accounts/test_user_group.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- mpt_api_client/resources/accounts/account.py
- tests/seed/accounts/test_user_group.py
- tests/e2e/conftest.py
- seed/accounts/buyer.py
- tests/seed/accounts/test_api_token.py
- tests/e2e/accounts/buyers/conftest.py
- tests/e2e/accounts/buyers/test_sync_buyers.py
- tests/e2e/accounts/buyers/test_async_buyers.py
- tests/seed/accounts/test_accounts.py
- mpt_api_client/resources/accounts/licensees.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_licensee.py
🧬 Code graph analysis (9)
tests/e2e/accounts/licensees/conftest.py (2)
tests/e2e/conftest.py (3)
seller_id(132-133)buyer_id(137-138)client_account_id(127-128)tests/e2e/accounts/conftest.py (1)
user_group_factory(45-60)
seed/helper.py (1)
mpt_api_client/models/model.py (1)
id(119-121)
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)mpt_api_client/resources/accounts/user_groups.py (1)
UserGroup(11-12)
seed/accounts/module.py (4)
seed/helper.py (1)
init_resource(44-84)seed/context.py (1)
Context(12-33)mpt_api_client/resources/accounts/modules.py (1)
Module(11-12)mpt_api_client/models/model.py (1)
id(119-121)
tests/seed/accounts/test_module.py (4)
seed/accounts/module.py (2)
refresh_module(16-31)seed_module(34-38)tests/seed/accounts/test_user_group.py (1)
context_with_data(12-16)seed/context.py (1)
Context(12-33)tests/seed/conftest.py (2)
operations_client(24-25)context(14-15)
tests/e2e/catalog/pricing_policies/conftest.py (1)
tests/e2e/conftest.py (2)
client_account_id(127-128)product_id(96-97)
mpt_api_client/resources/accounts/buyers.py (2)
mpt_api_client/resources/catalog/mixins.py (2)
ActivatableMixin(109-132)AsyncActivatableMixin(135-160)mpt_api_client/resources/accounts/mixins.py (6)
ActivatableMixin(4-27)EnablableMixin(30-53)ValidateMixin(56-68)AsyncActivatableMixin(134-159)AsyncEnablableMixin(162-185)AsyncValidateMixin(188-200)
tests/seed/accounts/test_account.py (5)
seed/accounts/account.py (2)
init_account_id(15-20)seed_account_ids(23-38)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)tests/seed/accounts/test_seller.py (1)
context_without_data(8-9)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)
🪛 Ruff (0.14.8)
seed/accounts/accounts.py
14-14: Unused noqa directive (unknown: WPS217, WPS213)
Remove unused noqa directive
(RUF100)
tests/seed/accounts/test_licensee.py
43-43: Unused noqa directive (unknown: WPS218)
Remove unused noqa directive
(RUF100)
seed/accounts/account.py
15-15: Unused function argument: context
(ARG001)
15-15: Unused noqa directive (non-enabled: RUF029)
Remove unused noqa directive
(RUF100)
19-19: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/licensee.py
16-16: Unused noqa directive (unknown: WPS238)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (22)
e2e_config.test.json (1)
4-4: LGTM - Config rename aligns with client_account refactoring.The key rename from
accounts.buyer.account.idtoaccounts.client_account.idis consistent with the broader refactoring to standardize account identification terminology across the codebase.tests/e2e/catalog/pricing_policies/conftest.py (1)
7-11: LGTM - Parameter rename consistent with refactoring.The parameter rename from
buyer_account_idtoclient_account_idcorrectly propagates through the fixture signature and payload construction. The fixture dependency is properly defined intests/e2e/conftest.py.tests/e2e/accounts/licensees/conftest.py (1)
15-34: LGTM - Consistent licensee fixture refactoring.The fixture correctly replaces
licensee_account_idwithclient_account_idthroughout the function signature,user_group_factorycall, and account payload. All references are properly updated.mpt_api_client/resources/accounts/buyers.py (2)
41-45: LGTM - Improved type safety with concrete Buyer type.Tightening the mixin type parameters from
ModeltoBuyerimproves type safety and provides better IDE support. The mixins'_resource_actioncalls will now return the more specificBuyertype instead of the genericModel.
74-78: LGTM - Async mixins consistently typed.The async service mixins are correctly updated with the same type parameter changes as the sync version, maintaining consistency across the codebase.
seed/helper.py (1)
82-82: LGTM - Flexible resource ID extraction.The enhancement to handle both string IDs and objects with an
idattribute is a sensible generalization that maintains backward compatibility while adding flexibility. Theisinstancecheck correctly handles both cases, and the docstring appropriately documents the expected behavior.tests/seed/accounts/test_account.py (3)
7-9: LGTM - Clean test fixture.The
context_without_datafixture provides a cleanContextinstance for testing account initialization in isolation.
12-23: LGTM - Comprehensive environment variable testing.The parametrized test correctly verifies that
init_account_idretrieves account IDs from environment variables for all three account types (VENDOR, CLIENT, OPERATIONS). The test uses appropriate pytest patterns.
26-36: LGTM - Proper seeding verification.The test correctly mocks
init_resourceand verifies thatseed_account_idsinitializes all three account ID types. The use ofassert_any_callappropriately validates the expected namespace arguments.seed/accounts/accounts.py (4)
3-3: LGTM - Import correctly added.The import for
seed_account_idsis properly added and used at line 18.
14-14: Static analysis false positive - noqa codes are valid for wemake-python-styleguide.The Ruff warning about unknown noqa codes is a false positive. The WPS217 and WPS213 codes are from the wemake-python-styleguide linter, not Ruff. If your project uses wemake, these directives are appropriate.
18-18: LGTM - Account IDs seeded first.Calling
seed_account_ids()before other seeding functions is correct, as downstream entities depend on these account IDs being available in the context.
27-27: LGTM - Completion logging added.The completion log message provides clear feedback that the seeding process has finished, complementing the opening log at line 16.
tests/seed/accounts/test_seller.py (4)
3-9: LGTM - Imports and fixture updated for new API.The imports correctly reflect the new
create_sellerAPI surface, and thecontext_without_datafixture provides a clean testing environment consistent with other test files.
12-29: LGTM - Existing test unchanged.The
test_build_seller_datatest correctly validates the seller data payload structure and remains unchanged.
32-43: LGTM - Comprehensive create_seller test.The test properly validates both the return value and payload structure of
create_seller. The mock assertion correctly extracts and verifies that the payload contains the expected seller name.
46-53: LGTM - Seeding test updated for init_resource pattern.The test correctly verifies that
seed_sellerdelegates toinit_resource, aligning with the new generalized seeding approach. The async mock and assertion are properly configured.tests/seed/accounts/test_licensee.py (1)
43-43:noqa: WPS218is intentional.Based on learnings, the repository uses wemake-python-styleguide, and WPS218 suppresses "too many assert statements". Ruff doesn't recognize WPS-specific directives, making this a false positive.
seed/accounts/user_group.py (1)
14-46: Clean refactor to create-only flow.The changes correctly implement the unified seeding pattern:
require_context_idvalidates required dependencies upfrontcreate_user_grouphas clear create-only semanticsseed_user_groupproperly delegates toinit_resourceThis aligns well with the broader PR pattern.
seed/accounts/licensee.py (2)
16-16:noqa: WPS238directive.Based on learnings, the repository uses wemake-python-styleguide. WPS238 flags "too many expressions" in a function. Given the function builds a large dictionary, this suppression is likely intentional.
48-57: LGTM on create_licensee implementation.Clean implementation using context manager for the icon file and properly delegating data building to
build_licensee_data.seed/accounts/api_tokens.py (1)
14-27: Context‑driven API token payload looks solidUsing
require_context_idfor bothaccounts.client_account.idandaccounts.module.idgives clear, early failures if prerequisites weren’t seeded, and keepsbuild_api_token_data()focused on payload construction. This aligns well with the new context‑first seeding approach.
robcsegal
force-pushed
the
MPT-16315-update-account-seeding-to-latest-convention
branch
from
b3543dd to
6b782e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
seed/accounts/account.py (2)
14-20: Consider usingcontextor removing it if unnecessary.The
contextparameter is injected but never used in the function body. If it's intentionally unused (kept for DI wiring consistency), consider prefixing with underscore (_context) to indicate intent and silence ARG001.@inject -async def init_account_id(env_var: str, context: Context = Provide[Container.context]) -> str: # noqa: RUF029 +async def init_account_id(env_var: str, _context: Context = Provide[Container.context]) -> str: # noqa: RUF029
23-38: Consider improving log message clarity.The initial log says "Seeding vendor account" but the function seeds vendor, client, and operations accounts. Consider updating to reflect the full scope:
async def seed_account_ids() -> None: - """Seed account.""" - logger.debug("Seeding vendor account ...") + """Seed account IDs from environment variables.""" + logger.debug("Seeding account IDs ...")tests/seed/accounts/test_user_group.py (1)
12-16: Solid coverage of user group build/create/seed flow; consider tightening the seed assertionThe fixture and tests nicely validate that:
build_user_group_datapulls IDs fromcontext_with_data.create_user_groupforwards those IDs into the payload sent toaccounts.user_groups.create.seed_user_groupdelegates toinit_resource.If you want stricter guarantees, you could also assert the exact arguments used when seeding:
mock_init_resource.assert_awaited_once_with( "accounts.user_group.id", create_user_group, )Also applies to: 19-31, 34-49, 52-58
tests/seed/accounts/test_module.py (1)
22-27: Tests exercise the new module flow; you can simplify mocks and strengthen assertionsThe updated tests correctly:
- Use
context_with_datawhen callingrefresh_module.- Mock the modules filter/iterate chain to return a
Module.- Verify that
seed_moduleawaitsinit_resource.Two small optional tweaks:
- In
test_refresh_module,modules_service.refreshis mocked but never used byrefresh_module; you can drop that mock to reduce noise.- You might also assert that
context_with_data["accounts.module.id"]was updated, to validate the side effect.- In
test_seed_module, consider asserting the arguments as well:mock_init_resource.assert_awaited_once_with( "accounts.module.id", refresh_module, )Also applies to: 43-55, 58-65
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
e2e_config.test.json(1 hunks)seed/accounts/account.py(1 hunks)seed/accounts/accounts.py(2 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/helper.py(1 hunks)tests/e2e/accounts/buyers/conftest.py(1 hunks)tests/e2e/accounts/buyers/test_async_buyers.py(3 hunks)tests/e2e/accounts/buyers/test_sync_buyers.py(2 hunks)tests/e2e/accounts/licensees/conftest.py(2 hunks)tests/e2e/catalog/pricing_policies/conftest.py(1 hunks)tests/e2e/conftest.py(1 hunks)tests/seed/accounts/test_account.py(1 hunks)tests/seed/accounts/test_accounts.py(2 hunks)tests/seed/accounts/test_api_token.py(1 hunks)tests/seed/accounts/test_buyer.py(2 hunks)tests/seed/accounts/test_licensee.py(2 hunks)tests/seed/accounts/test_module.py(3 hunks)tests/seed/accounts/test_seller.py(2 hunks)tests/seed/accounts/test_user_group.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- e2e_config.test.json
- tests/seed/accounts/test_account.py
- tests/e2e/catalog/pricing_policies/conftest.py
- tests/e2e/accounts/licensees/conftest.py
- tests/seed/accounts/test_accounts.py
- tests/e2e/accounts/buyers/test_async_buyers.py
- tests/e2e/conftest.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-01T10:48:52.586Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 136
File: tests/e2e/notifications/subscribers/conftest.py:14-25
Timestamp: 2025-12-01T10:48:52.586Z
Learning: The repository softwareone-platform/mpt-api-python-client uses wemake-python-styleguide linter, so noqa directives for WPS rules should not be flagged as unused.
Applied to files:
tests/seed/accounts/test_licensee.py
🧬 Code graph analysis (11)
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/e2e/accounts/buyers/conftest.py (1)
tests/e2e/conftest.py (2)
client_account_id(127-128)account_id(122-123)
tests/e2e/accounts/buyers/test_sync_buyers.py (3)
tests/e2e/conftest.py (2)
client_account_id(127-128)account_id(122-123)tests/e2e/accounts/buyers/conftest.py (1)
buyer_factory(10-34)tests/e2e/accounts/conftest.py (1)
account_icon(20-21)
tests/seed/accounts/test_buyer.py (7)
seed/accounts/buyer.py (3)
build_buyer_data(16-37)create_buyer(41-49)seed_buyer(52-56)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)tests/seed/accounts/test_module.py (1)
context_with_data(23-26)tests/seed/accounts/test_user_group.py (1)
context_with_data(12-16)tests/e2e/accounts/buyers/test_async_buyers.py (1)
test_create_buyer(64-67)tests/e2e/accounts/buyers/test_sync_buyers.py (1)
test_create_buyer(62-65)
seed/accounts/buyer.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/buyers.py (1)
Buyer(26-27)
tests/seed/accounts/test_seller.py (3)
seed/accounts/seller.py (3)
build_seller_data(14-29)create_seller(33-38)seed_seller(41-45)tests/seed/conftest.py (2)
context(14-15)operations_client(24-25)seed/context.py (1)
Context(12-33)
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_user_group.py (2)
seed/accounts/user_group.py (3)
create_user_group(33-39)seed_user_group(42-46)build_user_group_data(15-29)seed/context.py (1)
Context(12-33)
seed/helper.py (1)
mpt_api_client/models/model.py (1)
id(119-121)
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/account.py (4)
seed/container.py (1)
Container(7-30)tests/seed/conftest.py (1)
context(14-15)seed/context.py (1)
Context(12-33)seed/helper.py (1)
init_resource(44-84)
🪛 Ruff (0.14.8)
tests/seed/accounts/test_licensee.py
43-43: Unused noqa directive (unknown: WPS218)
Remove unused noqa directive
(RUF100)
seed/accounts/licensee.py
16-16: Unused noqa directive (unknown: WPS238)
Remove unused noqa directive
(RUF100)
seed/accounts/account.py
15-15: Unused function argument: context
(ARG001)
15-15: Unused noqa directive (non-enabled: RUF029)
Remove unused noqa directive
(RUF100)
19-19: Avoid specifying long messages outside the exception class
(TRY003)
seed/accounts/accounts.py
14-14: Unused noqa directive (unknown: WPS217, WPS213)
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 (20)
tests/e2e/accounts/buyers/conftest.py (1)
10-13: LGTM! Clean parameter rename to standardize naming conventions.The fixture parameter rename from
buyer_account_idtoclient_account_idis consistent with the fixture defined intests/e2e/conftest.pyand aligns with the broader PR objective of standardizing account naming conventions across the test suite.tests/e2e/accounts/buyers/test_sync_buyers.py (3)
10-14: LGTM! Fixture parameter consistently updated.The
created_buyerfixture correctly uses the renamedclient_account_idparameter, maintaining consistency with the updatedbuyer_factoryfixture.
78-80: LGTM! Test function properly updated.The test function signature and
buyer_factorycall are correctly updated to useclient_account_id.
87-91: LGTM! Test function properly updated.The test function signature and
buyer_factorycall are correctly updated to useclient_account_id.seed/helper.py (1)
81-83: LGTM! Clean dual-type handling for resource factory returns.The
isinstancecheck appropriately handles both string IDs (from environment variable lookups) and Model objects (from API calls), enabling unified usage ofinit_resourceacross the seeding workflow.seed/accounts/buyer.py (2)
16-37: LGTM! Clean context-driven data construction.The use of
require_context_idfor bothclient_account_idandseller_idensures proper dependency ordering and clear error messages if prerequisites are missing.
40-56: LGTM! Simplified creation flow.The
create_buyerfunction cleanly encapsulates data building and API call with icon upload. Theseed_buyerfunction properly delegates toinit_resourcefor consistent resource initialization.seed/accounts/api_tokens.py (2)
14-27: LGTM! Consistent context-driven data construction.The
build_api_token_datafunction properly usesrequire_context_idfor bothaccount_idandmodule_id, ensuring dependencies are validated before token creation.
30-45: LGTM! Clean API token creation flow.The
create_api_tokenfunction follows the same pattern as other seed modules, andseed_api_tokenproperly delegates toinit_resource.seed/accounts/accounts.py (1)
14-27: LGTM! Correct seeding orchestration with proper dependency ordering.The
seed_account_ids()call at the beginning ensures account IDs are populated before dependent resources are seeded. The sequence correctly respects dependencies: accounts → seller → buyer → module → api_token → user_group → licensee.tests/seed/accounts/test_seller.py (1)
32-53: LGTM! Good test coverage for the refactored seller flow.The
test_create_sellerproperly verifies the payload structure, andtest_seed_sellercorrectly validates thatinit_resourceis invoked during seeding.tests/seed/accounts/test_licensee.py (2)
7-14: LGTM! Comprehensive fixture setup.The
context_with_datafixture correctly populates all required IDs (client_account, seller, buyer, user_group) needed for licensee creation.
43-69: LGTM! Thorough test coverage for licensee creation.The
test_create_licenseeproperly validates all key payload fields, andtest_seed_licenseecorrectly verifies theinit_resourceinvocation. Based on learnings, thenoqa: WPS218directive is required for wemake-python-styleguide compliance.tests/seed/accounts/test_buyer.py (2)
7-38: LGTM! Well-structured fixture and data test.The
context_with_datafixture correctly sets up required IDs, andtest_build_buyer_datavalidates the expected payload structure with the newclient_account_idfield.
41-65: LGTM! Comprehensive test coverage for buyer creation flow.The
test_create_buyerproperly validates payload fields including account and seller IDs, andtest_seed_buyercorrectly verifies theinit_resourceinvocation pattern.seed/accounts/licensee.py (1)
20-23: Context-based IDs andinit_resourcewiring look consistentUsing
require_context_idfor all dependent IDs and seeding viaawait init_resource("accounts.licensee.id", create_licensee)matches the new convention used by other account seeders. The async factory signature and payload (includinggroups: [{"id": group_id}]and file upload viaICON) look correct.Also applies to: 41-45, 49-57, 60-63
seed/accounts/module.py (1)
34-38:seed_module’s use ofinit_resourcematches the shared seeding patternChanging
seed_moduleto returnNoneand delegate toawait init_resource("accounts.module.id", refresh_module)keeps it consistent with the other seeders and centralizes caching behavior in the helper.seed/accounts/seller.py (1)
33-38: Seller creation and seeding refactor looks correct
create_sellercleanly wrapsbuild_seller_dataandmpt_operations.accounts.sellers.create, andseed_sellernow follows the sharedinit_resource("accounts.seller.id", create_seller)pattern. The zero-arg async factory signature aligns withinit_resourceexpectations.Also applies to: 41-45
seed/accounts/user_group.py (1)
15-29: User group seeding now consistently uses context-based IDs andinit_resourceThe switch to
require_context_idfor"accounts.client_account.id"and"accounts.module.id", plus thecreate_user_group+init_resource("accounts.user_group.id", create_user_group)wiring, aligns well with the new seeding conventions and the updated tests. No issues from a correctness standpoint.Also applies to: 33-40, 42-45
tests/seed/accounts/test_api_token.py (1)
12-16: API token tests cleanly validate the new builder, factory, and seeding behaviorThe
context_with_datafixture, payload comparison intest_build_api_token_data, argument inspection intest_create_api_token, and theseed_api_tokencheck againstinit_resource("accounts.api_token.id", create_api_token)together give good confidence in the new flow. No changes needed here.Also applies to: 19-31, 33-47, 50-56
| @pytest.fixture | ||
| def seller(): | ||
| return Seller({"id": "SEL-123", "name": "Test Seller"}) | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def sellers_service(mocker): | ||
| return mocker.Mock(spec=AsyncSellersService) | ||
|
|
||
|
|
||
| async def test_get_seller(context: Context, operations_client, seller, sellers_service): | ||
| context["accounts.seller.id"] = seller.id | ||
| sellers_service.get.return_value = seller | ||
| operations_client.accounts.sellers = sellers_service | ||
|
|
||
| result = await get_seller(context=context, mpt_operations=operations_client) | ||
|
|
||
| assert result == seller | ||
| assert context.get_resource("accounts.seller", seller.id) == seller | ||
|
|
||
|
|
||
| async def test_get_seller_without_id(context: Context): | ||
| result = await get_seller(context=context) | ||
| assert result is None | ||
| def context_without_data() -> Context: | ||
| return Context() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "context_without_data" tests/Repository: softwareone-platform/mpt-api-python-client
Length of output: 477
Remove unused fixture context_without_data.
The context_without_data fixture defined at lines 7-9 is not used by any test function in this file. It should be removed.
🤖 Prompt for AI Agents
In tests/seed/accounts/test_seller.py around lines 7 to 9, the fixture named
context_without_data is defined but unused; remove the entire fixture definition
(the @pytest.fixture decorator and the def context_without_data() -> Context:
return Context() body) from the file to eliminate dead code and any linter
warnings.
|
d3rky
deleted the
MPT-16315-update-account-seeding-to-latest-convention
branch
Updated account seeding and tests
https://softwareone.atlassian.net/browse/MPT-16315
Summary by CodeRabbit
Release Notes
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.