MPT-14936 add e2e test suite for notification subscribers handling #136
Conversation
|
Warning Rate limit exceeded@albertsola has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 46 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (14)
WalkthroughAdds explicit Enable/Disable mixins (sync and async), replaces legacy Enablable/AsyncEnablable usages across multiple account and subscriber resources, updates HTTP mixins (with a duplicated block), adds unit tests for enable/disable, and introduces new e2e subscriber fixtures and sync/async e2e tests plus config entry. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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-14936 |
jentyk
force-pushed
the
feat/MPT-14936
branch
9 times, most recently
from
0b29daf to
c24da6a
Compare
jentyk
requested review from
albertsola,
alephsur,
d3rky,
robcsegal,
ruben-sebrango and
svazquezco
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/e2e/notifications/subscribers/conftest.py (1)
4-6: Inconsistent dictionary access pattern.This fixture uses
.get()which returnsNoneif the key is missing, while other e2e config fixtures (e.g.,user_id,user_group_id,account_idintests/e2e/conftest.py) use direct dictionary access that raisesKeyErroron missing keys. Direct access is preferable as it fails fast during test setup if configuration is incomplete.Apply this diff to match the pattern used by other fixtures:
@pytest.fixture def subscriber_id(e2e_config): - return e2e_config.get("notifications.subscriber.id") + return e2e_config["notifications.subscriber.id"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e_config.test.json(1 hunks)tests/e2e/accounts/conftest.py(0 hunks)tests/e2e/conftest.py(1 hunks)tests/e2e/notifications/subscribers/conftest.py(1 hunks)tests/e2e/notifications/subscribers/test_async_subscribers.py(1 hunks)tests/e2e/notifications/subscribers/test_sync_subscribers.py(1 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/accounts/conftest.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-27T00:05:36.332Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/conftest.py:0-0
Timestamp: 2025-11-27T00:05:36.332Z
Learning: When reviewing PRs that add fixtures to a parent conftest that may duplicate fixtures in child conftest files, do not suggest removing the duplicates from child conftest files that are outside the scope of the PR's changes.
Applied to files:
tests/e2e/notifications/subscribers/conftest.py
🧬 Code graph analysis (2)
tests/e2e/notifications/subscribers/test_async_subscribers.py (5)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/conftest.py (2)
async_mpt_client(48-51)user_group_id(122-123)tests/e2e/notifications/subscribers/conftest.py (4)
subscriber_id(5-6)invalid_subscriber_id(10-11)subscriber_factory(29-39)recipients_factory(15-25)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/notifications/subscribers/conftest.py (1)
tests/e2e/conftest.py (3)
e2e_config(89-92)user_id(117-118)user_group_id(122-123)
🪛 Ruff (0.14.6)
tests/e2e/notifications/subscribers/test_async_subscribers.py
44-44: Unused function argument: user_group_id
(ARG001)
tests/e2e/notifications/subscribers/test_sync_subscribers.py
42-42: Unused function argument: user_group_id
(ARG001)
tests/e2e/notifications/subscribers/conftest.py
16-16: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
30-30: Unused noqa directive (unknown: WPS430)
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 (11)
e2e_config.test.json (1)
23-24: LGTM!The new subscriber ID configuration is properly added alongside existing configuration keys, and the JSON structure is valid.
tests/e2e/conftest.py (1)
121-128: LGTM!The fixtures follow the established pattern and properly centralize shared configuration access. The reorganization from
tests/e2e/accounts/conftest.pyto the root-levelconftest.pyimproves fixture reusability across test modules.tests/e2e/notifications/subscribers/test_async_subscribers.py (5)
9-12: LGTM!The test appropriately verifies subscriber retrieval functionality.
15-20: LGTM!The pagination test correctly verifies that the endpoint returns results.
23-25: LGTM!The test properly validates the 404 error response for non-existent subscribers.
43-51: LGTM!The test properly validates the error case for updating a non-existent subscriber. The
user_group_idfixture is used indirectly throughrecipients_factory, so the static analysis hint is a false positive.
54-75: LGTM!The enable/disable tests provide comprehensive coverage of both success and error scenarios, with appropriate status validation.
tests/e2e/notifications/subscribers/test_sync_subscribers.py (3)
9-26: LGTM!The tests properly cover basic retrieval, listing, and error handling scenarios for the subscribers API.
41-48: LGTM!The test properly validates the error case for updating a non-existent subscriber. The
user_group_idfixture is used indirectly throughrecipients_factory, so the static analysis hint is a false positive.
50-71: LGTM!The enable/disable tests provide comprehensive coverage of both success and error scenarios, with appropriate status validation.
tests/e2e/notifications/subscribers/conftest.py (1)
9-11: LGTM!The hardcoded invalid ID is appropriate for testing error scenarios.
jentyk
force-pushed
the
feat/MPT-14936
branch
3 times, most recently
from
0b8fd8c to
8d16088
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/e2e/notifications/subscribers/test_sync_subscribers.py (1)
39-45: Remove unused fixture parameter.The
user_group_idparameter is not used in the test body. Sincerecipients_factoryalready depends onuser_group_id, pytest will automatically inject it when needed—the test doesn't need to explicitly request it.Apply this diff:
def test_update_subscriber_not_found( - mpt_ops, subscriber_factory, invalid_subscriber_id, user_group_id, recipients_factory + mpt_ops, subscriber_factory, invalid_subscriber_id, recipients_factory ):tests/e2e/notifications/subscribers/test_async_subscribers.py (1)
41-49: Remove unused fixture parameter.The
user_group_idparameter is not used in the test body. Sincerecipients_factoryalready depends onuser_group_id, pytest will automatically inject it when needed.Apply this diff:
async def test_update_subscriber_not_found( - async_mpt_client, subscriber_factory, invalid_subscriber_id, user_group_id, recipients_factory + async_mpt_client, subscriber_factory, invalid_subscriber_id, recipients_factory ):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e_config.test.json(1 hunks)tests/e2e/accounts/conftest.py(0 hunks)tests/e2e/conftest.py(1 hunks)tests/e2e/notifications/subscribers/conftest.py(1 hunks)tests/e2e/notifications/subscribers/test_async_subscribers.py(1 hunks)tests/e2e/notifications/subscribers/test_sync_subscribers.py(1 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/accounts/conftest.py
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e_config.test.json
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-27T00:05:36.332Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/conftest.py:0-0
Timestamp: 2025-11-27T00:05:36.332Z
Learning: When reviewing PRs that add fixtures to a parent conftest that may duplicate fixtures in child conftest files, do not suggest removing the duplicates from child conftest files that are outside the scope of the PR's changes.
Applied to files:
tests/e2e/conftest.pytests/e2e/notifications/subscribers/conftest.py
📚 Learning: 2025-12-01T10:48:52.572Z
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.572Z
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/e2e/notifications/subscribers/conftest.py
🧬 Code graph analysis (3)
tests/e2e/notifications/subscribers/conftest.py (1)
tests/e2e/conftest.py (3)
e2e_config(89-92)user_id(117-118)user_group_id(122-123)
tests/e2e/notifications/subscribers/test_async_subscribers.py (4)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/conftest.py (1)
async_mpt_client(48-51)tests/e2e/notifications/subscribers/conftest.py (4)
subscriber_id(5-6)invalid_subscriber_id(10-11)subscriber_factory(29-39)recipients_factory(15-25)
tests/e2e/notifications/subscribers/test_sync_subscribers.py (4)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/conftest.py (3)
mpt_client(43-44)mpt_ops(31-32)user_group_id(122-123)tests/e2e/notifications/subscribers/conftest.py (4)
subscriber_id(5-6)invalid_subscriber_id(10-11)subscriber_factory(29-39)recipients_factory(15-25)
🪛 Ruff (0.14.6)
tests/e2e/notifications/subscribers/conftest.py
16-16: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
30-30: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
tests/e2e/notifications/subscribers/test_async_subscribers.py
42-42: Unused function argument: user_group_id
(ARG001)
tests/e2e/notifications/subscribers/test_sync_subscribers.py
40-40: Unused function argument: user_group_id
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (8)
tests/e2e/conftest.py (1)
121-128: LGTM!The new fixtures follow the established pattern and correctly expose account-related identifiers from the e2e configuration for use across test modules.
tests/e2e/notifications/subscribers/test_sync_subscribers.py (3)
9-25: LGTM!The GET, list, and 404 error handling tests are well-structured and verify the expected behavior correctly.
28-36: LGTM!The filter test correctly validates that results are returned and demonstrates proper use of RQLQuery for filtering and field selection.
48-69: LGTM!The disable and enable tests comprehensively verify both successful status transitions and proper 404 error handling.
tests/e2e/notifications/subscribers/test_async_subscribers.py (3)
9-25: LGTM!The async GET, list, and 404 error handling tests correctly use async/await and verify the expected behavior.
28-38: LGTM!The async filter test correctly demonstrates async iteration and validates non-empty results with proper field selection.
52-73: LGTM!The async disable and enable tests comprehensively verify status transitions and error handling with proper async/await patterns.
tests/e2e/notifications/subscribers/conftest.py (1)
4-39: LGTM!The fixture definitions are well-structured and provide the necessary test data factories for the subscriber e2e tests. The fixtures correctly leverage dependency injection and follow pytest conventions.
Based on learnings, the
noqa: WPS430directives are appropriate for the wemake-python-styleguide linter in use.
jentyk
force-pushed
the
feat/MPT-14936
branch
from
8d16088 to
a0d688a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/e2e/notifications/subscribers/test_sync_subscribers.py (1)
28-36: Consider verifying filter and select behavior.The test now correctly asserts non-empty results, but doesn't verify that the filter and select operations worked as expected. Consider adding assertions to confirm:
- The returned subscriber(s) actually match the filter criteria (e.g.,
result[0].id == subscriber_id)- The
configfield was excluded by the select operationApply this diff to add verification:
result = list(filtered_subscribers.iterate()) assert len(result) > 0 + # Verify filter worked + assert all(subscriber.id == subscriber_id for subscriber in result) + # Verify select exclusion worked + assert all(not hasattr(subscriber, 'config') for subscriber in result)tests/e2e/notifications/subscribers/test_async_subscribers.py (1)
28-38: Consider verifying filter and select behavior.The test now correctly asserts non-empty results, but doesn't verify that the filter and select operations worked as expected. Consider adding assertions to confirm:
- The returned subscriber(s) actually match the filter criteria (e.g.,
result[0].id == subscriber_id)- The
configfield was excluded by the select operationApply this diff to add verification:
] assert len(result) > 0 + # Verify filter worked + assert all(subscriber.id == subscriber_id for subscriber in result) + # Verify select exclusion worked + assert all(not hasattr(subscriber, 'config') for subscriber in result)tests/e2e/notifications/subscribers/conftest.py (1)
4-6: Consider using direct dictionary access for consistency.The fixture uses
.get()which returnsNoneif the key is missing, whereas other fixtures in the parentconftest.pyuse direct dictionary access (e.g.,e2e_config["accounts.user.id"]). Using direct access would raise a clearerKeyErrorimmediately if the configuration is missing.Apply this diff for consistency:
@pytest.fixture def subscriber_id(e2e_config): - return e2e_config.get("notifications.subscriber.id") + return e2e_config["notifications.subscriber.id"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e_config.test.json(1 hunks)tests/e2e/accounts/conftest.py(0 hunks)tests/e2e/conftest.py(1 hunks)tests/e2e/notifications/subscribers/conftest.py(1 hunks)tests/e2e/notifications/subscribers/test_async_subscribers.py(1 hunks)tests/e2e/notifications/subscribers/test_sync_subscribers.py(1 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/accounts/conftest.py
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e_config.test.json
- tests/e2e/conftest.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-27T00:05:36.332Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/conftest.py:0-0
Timestamp: 2025-11-27T00:05:36.332Z
Learning: When reviewing PRs that add fixtures to a parent conftest that may duplicate fixtures in child conftest files, do not suggest removing the duplicates from child conftest files that are outside the scope of the PR's changes.
Applied to files:
tests/e2e/notifications/subscribers/conftest.py
📚 Learning: 2025-12-01T10:48:52.572Z
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.572Z
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/e2e/notifications/subscribers/conftest.py
🧬 Code graph analysis (2)
tests/e2e/notifications/subscribers/conftest.py (1)
tests/e2e/conftest.py (3)
e2e_config(89-92)user_id(117-118)user_group_id(122-123)
tests/e2e/notifications/subscribers/test_async_subscribers.py (4)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/conftest.py (2)
async_mpt_client(48-51)user_group_id(122-123)tests/e2e/notifications/subscribers/conftest.py (4)
subscriber_id(5-6)invalid_subscriber_id(10-11)subscriber_factory(29-39)recipients_factory(15-25)
🪛 Ruff (0.14.6)
tests/e2e/notifications/subscribers/conftest.py
16-16: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
30-30: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
tests/e2e/notifications/subscribers/test_sync_subscribers.py
40-40: Unused function argument: user_group_id
(ARG001)
tests/e2e/notifications/subscribers/test_async_subscribers.py
42-42: Unused function argument: user_group_id
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (8)
tests/e2e/notifications/subscribers/test_sync_subscribers.py (3)
15-20: LGTM!The test correctly validates that the list endpoint returns results.
23-25: LGTM!The error handling test correctly validates 404 responses.
48-73: LGTM!The skipped tests are appropriately marked with clear TODO comments and will provide good coverage once the
disableandenableactions are implemented.tests/e2e/notifications/subscribers/test_async_subscribers.py (3)
15-20: LGTM!The async test correctly validates that the list endpoint returns results.
23-25: LGTM!The error handling test correctly validates 404 responses.
52-77: LGTM!The skipped tests are appropriately marked with clear TODO comments and will provide good coverage once the
disableandenableactions are implemented.tests/e2e/notifications/subscribers/conftest.py (2)
9-11: LGTM!The fixture provides a clearly invalid ID for negative testing.
14-39: LGTM!The factory fixtures are well-structured and provide flexible test data generation. The type annotations and
noqadirectives were correctly addressed in previous reviews.Based on learnings, the
noqa: WPS430directives are required for the wemake-python-styleguide linter.
jentyk
force-pushed
the
feat/MPT-14936
branch
from
a0d688a to
c23107c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/e2e/notifications/subscribers/conftest.py (1)
4-11: Consider aligningsubscriber_idaccess with other ID fixturesEverywhere else (e.g.,
product_id,user_id) you usee2e_config["…"], whilesubscriber_iduses.get, returningNoneif the key is missing. Ifnotifications.subscriber.idis required like the others, you may want direct indexing here as well to fail fast on a misconfigured e2e config:-@pytest.fixture -def subscriber_id(e2e_config): - return e2e_config.get("notifications.subscriber.id") +@pytest.fixture +def subscriber_id(e2e_config): + return e2e_config["notifications.subscriber.id"]If you intentionally treat this ID as optional or want backward compatibility with older configs, the current implementation is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e_config.test.json(1 hunks)tests/e2e/accounts/conftest.py(0 hunks)tests/e2e/conftest.py(1 hunks)tests/e2e/notifications/subscribers/conftest.py(1 hunks)tests/e2e/notifications/subscribers/test_async_subscribers.py(1 hunks)tests/e2e/notifications/subscribers/test_sync_subscribers.py(1 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/accounts/conftest.py
🚧 Files skipped from review as they are similar to previous changes (3)
- e2e_config.test.json
- tests/e2e/notifications/subscribers/test_sync_subscribers.py
- tests/e2e/notifications/subscribers/test_async_subscribers.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-27T00:05:36.332Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/conftest.py:0-0
Timestamp: 2025-11-27T00:05:36.332Z
Learning: When reviewing PRs that add fixtures to a parent conftest that may duplicate fixtures in child conftest files, do not suggest removing the duplicates from child conftest files that are outside the scope of the PR's changes.
Applied to files:
tests/e2e/notifications/subscribers/conftest.py
📚 Learning: 2025-12-01T10:48:52.572Z
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.572Z
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/e2e/notifications/subscribers/conftest.py
🧬 Code graph analysis (1)
tests/e2e/notifications/subscribers/conftest.py (1)
tests/e2e/conftest.py (3)
e2e_config(89-92)user_id(117-118)user_group_id(122-123)
🪛 Ruff (0.14.6)
tests/e2e/notifications/subscribers/conftest.py
16-16: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
30-30: Unused noqa directive (unknown: WPS430)
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 (2)
tests/e2e/conftest.py (1)
121-128: New account/user-group ID fixtures are consistent and correct
user_group_idandaccount_idfollow the same pattern as existing ID fixtures (e.g.,user_id,product_id), pulling frome2e_configwith direct indexing so misconfigurations fail fast. This centralizes commonly used IDs in the root conftest without changing existing behavior.tests/e2e/notifications/subscribers/conftest.py (1)
14-39: Recipients and subscriber factories are coherent and reusable
recipients_factoryandsubscriber_factorygenerate payloads with consistent shapes (users/userGroupslists of{"id": …}and anotefield) and allow easy overriding of defaults via parameters. The genericlist/dictannotations keep the two factories type-consistent, and the nested helpers plus# noqa: WPS430match the project’s wemake-python-styleguide setup. Based on learnings, keeping the WPS430noqadirectives here is appropriate.
jentyk
force-pushed
the
feat/MPT-14936
branch
from
c23107c to
3da4c77
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/e2e/notifications/subscribers/conftest.py (1)
4-39: Fixtures and factories are consistent and ready for reuse
subscriber_id,invalid_subscriber_id,recipients_factory, andsubscriber_factorycompose cleanly and produce the expected subscriber payloads for tests. Keeping the nested helper functions with# noqa: WPS430is appropriate given wemake-python-styleguide; Ruff’s RUF100 warning for that code can be safely ignored or suppressed in tooling config.Based on learnings, ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e_config.test.json(1 hunks)tests/e2e/accounts/conftest.py(0 hunks)tests/e2e/conftest.py(1 hunks)tests/e2e/notifications/subscribers/conftest.py(1 hunks)tests/e2e/notifications/subscribers/test_async_subscribers.py(1 hunks)tests/e2e/notifications/subscribers/test_sync_subscribers.py(1 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/accounts/conftest.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/notifications/subscribers/test_sync_subscribers.py
- e2e_config.test.json
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-27T00:05:36.332Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/conftest.py:0-0
Timestamp: 2025-11-27T00:05:36.332Z
Learning: When reviewing PRs that add fixtures to a parent conftest that may duplicate fixtures in child conftest files, do not suggest removing the duplicates from child conftest files that are outside the scope of the PR's changes.
Applied to files:
tests/e2e/notifications/subscribers/conftest.py
📚 Learning: 2025-12-01T10:48:52.572Z
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.572Z
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/e2e/notifications/subscribers/conftest.py
🧬 Code graph analysis (1)
tests/e2e/notifications/subscribers/test_async_subscribers.py (4)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/notifications/subscribers/test_sync_subscribers.py (9)
test_get_subscriber_by_id(9-12)test_list_subscribers(15-20)test_get_subscriber_by_id_not_found(23-25)test_filter_subscribers(28-36)test_update_subscriber_not_found(39-45)test_disable_subscriber(49-53)test_disable_subscriber_not_found(57-59)test_enable_subscriber(63-67)test_enable_subscriber_not_found(71-73)tests/e2e/conftest.py (1)
async_mpt_client(48-51)tests/e2e/notifications/subscribers/conftest.py (4)
subscriber_id(5-6)invalid_subscriber_id(10-11)subscriber_factory(29-39)recipients_factory(15-25)
🪛 Ruff (0.14.7)
tests/e2e/notifications/subscribers/conftest.py
16-16: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
30-30: Unused noqa directive (unknown: WPS430)
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 (2)
tests/e2e/conftest.py (1)
117-128: Centralizing account/user-group fixtures at root e2e conftest looks goodThe new
user_group_idandaccount_idfixtures follow the existinge2e_configpattern and should make these IDs reusable across e2e suites without surprises.tests/e2e/notifications/subscribers/test_async_subscribers.py (1)
1-77: Async subscriber e2e coverage and async patterns look solidThe async tests exercise happy paths, listing, filtering, and error cases consistently using
MPTAPIError, with correctawaitusage and async iteration; the skipped enable/disable tests are clearly flagged for future implementation.
jentyk
force-pushed
the
feat/MPT-14936
branch
from
3da4c77 to
64c45ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/e2e/notifications/subscribers/conftest.py (2)
4-7: Prefer direct key access to fail fast on missing config
subscriber_iduses.get(...), which will quietly returnNoneif"notifications.subscriber.id"is missing or misspelled, whereas other fixtures likeuser_id/user_group_idindex directly intoe2e_config. For a required config value, using[]keeps behavior consistent across fixtures and surfaces misconfiguration as a clearKeyError.@pytest.fixture def subscriber_id(e2e_config): - return e2e_config.get("notifications.subscriber.id") + return e2e_config["notifications.subscriber.id"]
14-39: Be aware oforsemantics for empty recipients;noqa: WPS430usage is appropriateBoth
_recipientsand_subscriberuse thevalue or defaultpattern. This means explicitly passing empty containers (e.g.users=[],user_groups=[], orrecipients={}) will be replaced by the defaults, so you currently cannot distinguish “no recipients” from “use default recipients”. If you ever need that distinction in tests, consider switching to explicitis Nonechecks:def recipients_factory(user_id, user_group_id): def _recipients( # noqa: WPS430 - users: list | None = None, - user_groups: list | None = None, - ) -> dict: - return { - "users": users or [{"id": user_id}], - "userGroups": user_groups or [{"id": user_group_id}], - } + users: list | None = None, + user_groups: list | None = None, + ) -> dict: + return { + "users": [{"id": user_id}] if users is None else users, + "userGroups": [{"id": user_group_id}] if user_groups is None else user_groups, + } return _recipients @pytest.fixture def subscriber_factory(recipients_factory): def _subscriber( # noqa: WPS430 - recipients: dict | None = None, + recipients: dict | None = None, note: str = "Test note", ) -> dict: - return { - "recipients": recipients or recipients_factory(), - "note": note, - } + return { + "recipients": recipients_factory() if recipients is None else recipients, + "note": note, + }The nested helper functions plus
# noqa: WPS430are consistent with using wemake‑python‑styleguide in this repo; Ruff’s “unknown WPS430” (RUF100) can be safely ignored here. Based on learnings, ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e_config.test.json(1 hunks)tests/e2e/accounts/conftest.py(0 hunks)tests/e2e/conftest.py(1 hunks)tests/e2e/notifications/subscribers/conftest.py(1 hunks)tests/e2e/notifications/subscribers/test_async_subscribers.py(1 hunks)tests/e2e/notifications/subscribers/test_sync_subscribers.py(1 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/accounts/conftest.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/e2e/notifications/subscribers/test_async_subscribers.py
- tests/e2e/conftest.py
- e2e_config.test.json
- tests/e2e/notifications/subscribers/test_sync_subscribers.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-27T00:05:36.332Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/conftest.py:0-0
Timestamp: 2025-11-27T00:05:36.332Z
Learning: When reviewing PRs that add fixtures to a parent conftest that may duplicate fixtures in child conftest files, do not suggest removing the duplicates from child conftest files that are outside the scope of the PR's changes.
Applied to files:
tests/e2e/notifications/subscribers/conftest.py
📚 Learning: 2025-12-01T10:48:52.572Z
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.572Z
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/e2e/notifications/subscribers/conftest.py
🪛 Ruff (0.14.7)
tests/e2e/notifications/subscribers/conftest.py
16-16: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
30-30: Unused noqa directive (unknown: WPS430)
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 (1)
tests/e2e/notifications/subscribers/conftest.py (1)
9-11:invalid_subscriber_idfixture is clear and sufficientThe hard‑coded invalid ID is explicit and well‑scoped for negative tests; no issues here.
albertsola
force-pushed
the
feat/MPT-14936
branch
from
64c45ea to
da23aeb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/e2e/notifications/subscribers/conftest.py (1)
4-6: Consider using direct dictionary access for consistency.This fixture uses
.get()which returnsNoneif the key is missing, potentially causing obscure test failures. Other ID fixtures intests/e2e/conftest.py(e.g.,user_idat line 117) use direct dictionary access with[]to fail fast with a clearKeyErrorif the configuration is incomplete.Apply this diff for consistency:
@pytest.fixture def subscriber_id(e2e_config): - return e2e_config.get("notifications.subscriber.id") + return e2e_config["notifications.subscriber.id"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
e2e_config.test.json(1 hunks)tests/e2e/conftest.py(1 hunks)tests/e2e/notifications/subscribers/conftest.py(1 hunks)tests/e2e/notifications/subscribers/test_async_subscribers.py(1 hunks)tests/e2e/notifications/subscribers/test_sync_subscribers.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/notifications/subscribers/test_async_subscribers.py
- tests/e2e/notifications/subscribers/test_sync_subscribers.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-27T00:05:36.356Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/conftest.py:0-0
Timestamp: 2025-11-27T00:05:36.356Z
Learning: When reviewing PRs that add fixtures to a parent conftest that may duplicate fixtures in child conftest files, do not suggest removing the duplicates from child conftest files that are outside the scope of the PR's changes.
Applied to files:
tests/e2e/notifications/subscribers/conftest.py
📚 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/e2e/notifications/subscribers/conftest.py
🧬 Code graph analysis (2)
tests/e2e/conftest.py (1)
tests/e2e/accounts/conftest.py (1)
user_group_id(30-31)
tests/e2e/notifications/subscribers/conftest.py (1)
tests/e2e/conftest.py (3)
e2e_config(89-92)user_id(117-118)user_group_id(156-157)
🪛 GitHub Actions: PR build and merge
tests/e2e/conftest.py
[error] 1-1: Code formatting check failed. The file would be reformatted by the formatter (would reformat: tests/e2e/conftest.py). Run the formatter with --write to fix code style issues.
🪛 Ruff (0.14.8)
tests/e2e/conftest.py
161-161: Redefinition of unused account_id from line 122
(F811)
tests/e2e/notifications/subscribers/conftest.py
16-16: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
30-30: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (5)
e2e_config.test.json (1)
24-24: LGTM! Required configuration for subscriber tests.The new
notifications.subscriber.idis properly added to support the end-to-end subscriber test suite.tests/e2e/notifications/subscribers/conftest.py (3)
9-11: LGTM!The invalid subscriber ID follows a sensible pattern for testing error scenarios.
14-25: LGTM! Factory pattern properly implemented.The recipients factory correctly provides defaults while allowing customization for test scenarios. The
noqa: WPS430directive is appropriate for wemake-python-styleguide.Based on learnings, the repository uses wemake-python-styleguide linter.
28-39: LGTM! Subscriber factory follows best practices.The fixture properly composes with
recipients_factoryand provides sensible defaults. Thenoqa: WPS430directive is appropriate for wemake-python-styleguide.Based on learnings, the repository uses wemake-python-styleguide linter.
tests/e2e/conftest.py (1)
155-157: LGTM! Fixture added for broader test coverage.The
user_group_idfixture is properly implemented and extends the available e2e fixtures.
albertsola
force-pushed
the
feat/MPT-14936
branch
3 times, most recently
from
28e15aa to
7858d0b
Compare
albertsola
force-pushed
the
feat/MPT-14936
branch
from
7858d0b to
34c0038
Compare
albertsola
force-pushed
the
feat/MPT-14936
branch
from
34c0038 to
df7c8de
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tests/unit/http/test_mixins.py (1)
74-90: Fix typo in class names: "Sercice" → "Service".Both test service class names have a typo.
-class EnableDisableSercice( +class EnableDisableService( EnableMixin[DummyModel], DisableMixin[DummyModel], Service[DummyModel], ): _endpoint = "/public/v1/dummy" _model_class = DummyModel -class AsyncEnableDisableSercice( +class AsyncEnableDisableService( AsyncEnableMixin[DummyModel], AsyncDisableMixin[DummyModel], AsyncService[DummyModel], ): _endpoint = "/public/v1/dummy" _model_class = DummyModelAlso update the usages in the test functions below (lines 1305, 1320, 1335, 1350).
tests/e2e/notifications/subscribers/conftest.py (3)
16-27: Be careful usingorwith list defaults inrecipients_factoryUsing
users or [{"id": user_id}](and similarly foruser_groups) means an explicitly passed empty list will be replaced by the default, making it impossible to express “no users / no groups” distinctly from “use defaults”.If you ever need to pass empty lists intentionally, consider treating
Noneas the only sentinel:def recipients_factory(user_id, user_group_id): def _recipients( # noqa: WPS430 - users: list | None = None, - user_groups: list | None = None, - ) -> dict: - return { - "users": users or [{"id": user_id}], - "userGroups": user_groups or [{"id": user_group_id}], - } + users: list | None = None, + user_groups: list | None = None, + ) -> dict: + if users is None: + users = [{"id": user_id}] + if user_groups is None: + user_groups = [{"id": user_group_id}] + return { + "users": users, + "userGroups": user_groups, + }Keeping the nested helper with
# noqa: WPS430is appropriate here given the wemake-python-styleguide usage. Based on learnings, …
30-41: Sameor-vs-Nonenuance insubscriber_factory
recipients or recipients_factory()will also replace any falsyrecipients(e.g.,{}) with the default payload, which may or may not be what you want.For clearer semantics—“use defaults only when argument is omitted”—you could mirror the
Nonecheck pattern:@pytest.fixture def subscriber_factory(recipients_factory): def _subscriber( # noqa: WPS430 - recipients: dict | None = None, - note: str = "Test note", - ) -> dict: - return { - "recipients": recipients or recipients_factory(), - "note": note, - } + recipients: dict | None = None, + note: str = "Test note", + ) -> dict: + if recipients is None: + recipients = recipients_factory() + return { + "recipients": recipients, + "note": note, + }Keeping the WPS430 suppression for this nested helper also aligns with your wemake setup. Based on learnings, …
44-56: Teardown behavior fordisabled_subscriber_idThe fixture correctly ensures the subscriber is disabled before yielding and attempts to re‑enable it afterward while swallowing
MPTAPIError, which is a good pattern for not breaking tests on teardown.One nuance: if the subscriber started out already
"Disabled", this fixture will still try to enable it in teardown, so the post‑test state may differ from the pre‑test state. If preserving the original status ever matters, you’d need to track the initialsubscriber.statusand restore it accordingly; otherwise the current “always re‑enable” behavior is fine for a dedicated test subscriber. The# noqa: WPS421is consistent with the wemake configuration here. Based on learnings, …
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
e2e_config.test.json(1 hunks)mpt_api_client/http/mixins.py(1 hunks)mpt_api_client/resources/notifications/subscribers.py(3 hunks)tests/e2e/conftest.py(1 hunks)tests/e2e/notifications/subscribers/conftest.py(1 hunks)tests/e2e/notifications/subscribers/test_async_subscribers.py(1 hunks)tests/e2e/notifications/subscribers/test_sync_subscribers.py(1 hunks)tests/unit/http/test_mixins.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/notifications/subscribers/test_sync_subscribers.py
- tests/e2e/conftest.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-12T15:02:20.732Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 160
File: tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py:55-58
Timestamp: 2025-12-12T15:02:20.732Z
Learning: In pytest with pytest-asyncio, if a test function uses async fixtures but contains no await, declare the test function as def (synchronous) instead of async def. Pytest-asyncio will resolve the async fixtures automatically; this avoids linter complaints about unnecessary async functions. This pattern applies to any test file under the tests/ directory that uses such fixtures.
Applied to files:
tests/unit/http/test_mixins.pytests/e2e/notifications/subscribers/test_async_subscribers.pytests/e2e/notifications/subscribers/conftest.py
📚 Learning: 2025-11-27T00:05:36.356Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/conftest.py:0-0
Timestamp: 2025-11-27T00:05:36.356Z
Learning: When reviewing PRs that add fixtures to a parent conftest that may duplicate fixtures in child conftest files, do not suggest removing the duplicates from child conftest files that are outside the scope of the PR's changes.
Applied to files:
tests/e2e/notifications/subscribers/conftest.py
📚 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/e2e/notifications/subscribers/conftest.py
🧬 Code graph analysis (2)
mpt_api_client/resources/notifications/subscribers.py (1)
mpt_api_client/http/mixins.py (5)
AsyncDisableMixin(418-425)AsyncEnableMixin(398-405)AsyncManagedResourceMixin(662-665)DisableMixin(428-435)EnableMixin(408-415)
mpt_api_client/http/mixins.py (4)
mpt_api_client/models/model.py (1)
Model(65-125)mpt_api_client/resources/accounts/mixins.py (4)
enable(33-42)enable(165-174)disable(44-53)disable(176-185)mpt_api_client/http/service.py (1)
_resource_action(55-80)mpt_api_client/http/async_service.py (1)
_resource_action(55-80)
🪛 Ruff (0.14.8)
tests/unit/http/test_mixins.py
11-11: Unused noqa directive (unknown: WPS235)
Remove unused noqa directive
(RUF100)
tests/e2e/notifications/subscribers/conftest.py
18-18: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
32-32: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
55-55: Unused noqa directive (unknown: WPS421)
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 (6)
e2e_config.test.json (1)
45-45: LGTM!The new configuration key follows the established pattern and provides the subscriber ID needed for the e2e test fixtures.
tests/unit/http/test_mixins.py (1)
1304-1361: LGTM!The enable/disable mixin tests properly verify the POST requests to the correct endpoints and validate response parsing. The test structure follows established patterns in the file.
tests/e2e/notifications/subscribers/test_async_subscribers.py (1)
1-77: LGTM!The e2e test suite provides comprehensive coverage for subscriber operations including get, list, filter, update, enable, and disable with appropriate error handling tests. The use of
pytest.mark.flakyis appropriate for e2e tests that interact with external services.mpt_api_client/resources/notifications/subscribers.py (1)
27-46: LGTM!The integration of Enable/Disable mixins into both sync and async subscriber services is correct. The mixin order in the inheritance chain ensures proper method resolution, placing Enable/Disable mixins before ManagedResourceMixin.
tests/e2e/notifications/subscribers/conftest.py (2)
6-13: Config‑backed ID fixtures look good
subscriber_idandinvalid_subscriber_idare simple, readable fixtures that centralize the valid/invalid IDs for tests; no issues from a correctness or style standpoint.
58-64:active_subscriber_idfixture is straightforward and consistentThis fixture mirrors the disabled variant, ensuring the subscriber is
"Active"before use and returning its id. The pattern is clear and matches the intended enable/disable semantics introduced in the client; no changes needed.
| from mpt_api_client.exceptions import MPTAPIError | ||
| from mpt_api_client.http import AsyncService, Service | ||
| from mpt_api_client.http.mixins import ( | ||
| from mpt_api_client.http.mixins import ( # noqa: WPS235 |
There was a problem hiding this comment.
Remove unused noqa directive.
Per static analysis, the noqa: WPS235 directive is unused and should be removed.
-from mpt_api_client.http.mixins import ( # noqa: WPS235
+from mpt_api_client.http.mixins import (📝 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.
| from mpt_api_client.http.mixins import ( # noqa: WPS235 | |
| from mpt_api_client.http.mixins import ( |
🧰 Tools
🪛 Ruff (0.14.8)
11-11: Unused noqa directive (unknown: WPS235)
Remove unused noqa directive
(RUF100)
🤖 Prompt for AI Agents
In tests/unit/http/test_mixins.py line 11, remove the unused noqa directive from
the import line: delete " # noqa: WPS235" so the import reads normally; run
linters/tests to confirm no warning/violation appears after the change.
df7c8de to
b1f30b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/unit/http/test_mixins.py (1)
11-11: Remove unusednoqadirective.Per static analysis (RUF100), the
noqa: WPS235directive is unused and should be removed.-from mpt_api_client.http.mixins import ( # noqa: WPS235 +from mpt_api_client.http.mixins import (mpt_api_client/http/mixins.py (1)
428-435: Fix typo: extra space in docstring.Line 432 has an extra space before the period.
class DisableMixin[Model: BaseModel]: """Disable resource mixin.""" def disable(self, resource_id: str, resource_data: ResourceData | None = None) -> Model: - """Disable a specific resource .""" + """Disable a specific resource.""" return self._resource_action( # type: ignore[attr-defined, no-any-return] resource_id=resource_id, method="POST", action="disable", json=resource_data )
🧹 Nitpick comments (3)
tests/e2e/notifications/subscribers/test_async_subscribers.py (1)
59-63: Consider using the fixture value in the error match.The hardcoded ID
'NTS-0000-0000-0000'in the regex match duplicates the value frominvalid_subscriber_idfixture. Consider using an f-string or raw string interpolation for maintainability:async def test_disable_subscriber_not_found(async_mpt_client, invalid_subscriber_id): with pytest.raises( - MPTAPIError, match=r"400 Bad Request - Subscriber with id 'NTS-0000-0000-0000' not found" + MPTAPIError, match=rf"400 Bad Request - Subscriber with id '{invalid_subscriber_id}' not found" ): await async_mpt_client.notifications.subscribers.disable(invalid_subscriber_id)This applies to
test_enable_subscriber_not_foundas well (line 75).tests/e2e/notifications/subscribers/conftest.py (2)
44-55: Consider logging the exception details in teardown.The teardown correctly handles
MPTAPIErrorto prevent test failures from masking actual test results. However, the print statement discards valuable debugging information.try: mpt_client.notifications.subscribers.enable(subscriber_id) - except MPTAPIError: - print(f"TEARDOWN - Unable to re-enable subscriber {subscriber_id=}") # noqa: WPS421 + except MPTAPIError as exc: + print(f"TEARDOWN - Unable to re-enable subscriber {subscriber_id=}: {exc}") # noqa: WPS421
58-64: Consider adding teardown to restore original state.Unlike
disabled_subscriber_id, this fixture doesn't restore the subscriber's original state after the test. If a test using this fixture fails or leaves the subscriber in an unexpected state, subsequent tests could be affected.If state restoration is desired for test isolation:
@pytest.fixture def active_subscriber_id(mpt_client, subscriber_id): subscriber = mpt_client.notifications.subscribers.get(subscriber_id) + original_status = subscriber.status if subscriber.status != "Active": subscriber = mpt_client.notifications.subscribers.enable(subscriber_id) - return subscriber.id + yield subscriber.id + + if original_status == "Disabled": + try: + mpt_client.notifications.subscribers.disable(subscriber_id) + except MPTAPIError as exc: + print(f"TEARDOWN - Unable to restore subscriber {subscriber_id=} to Disabled: {exc}") # noqa: WPS421
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
e2e_config.test.json(1 hunks)mpt_api_client/http/mixins.py(1 hunks)mpt_api_client/resources/accounts/account.py(3 hunks)mpt_api_client/resources/accounts/api_tokens.py(3 hunks)mpt_api_client/resources/accounts/buyers.py(3 hunks)mpt_api_client/resources/accounts/licensees.py(3 hunks)mpt_api_client/resources/accounts/mixins.py(0 hunks)mpt_api_client/resources/notifications/subscribers.py(3 hunks)tests/e2e/conftest.py(1 hunks)tests/e2e/notifications/subscribers/conftest.py(1 hunks)tests/e2e/notifications/subscribers/test_async_subscribers.py(1 hunks)tests/e2e/notifications/subscribers/test_sync_subscribers.py(1 hunks)tests/unit/http/test_mixins.py(3 hunks)tests/unit/resources/accounts/test_mixins.py(0 hunks)
💤 Files with no reviewable changes (2)
- mpt_api_client/resources/accounts/mixins.py
- tests/unit/resources/accounts/test_mixins.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/e2e/notifications/subscribers/test_sync_subscribers.py
- tests/e2e/conftest.py
- e2e_config.test.json
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-12T15:02:20.732Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 160
File: tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py:55-58
Timestamp: 2025-12-12T15:02:20.732Z
Learning: In pytest with pytest-asyncio, if a test function uses async fixtures but contains no await, declare the test function as def (synchronous) instead of async def. Pytest-asyncio will resolve the async fixtures automatically; this avoids linter complaints about unnecessary async functions. This pattern applies to any test file under the tests/ directory that uses such fixtures.
Applied to files:
tests/e2e/notifications/subscribers/test_async_subscribers.pytests/e2e/notifications/subscribers/conftest.pytests/unit/http/test_mixins.py
📚 Learning: 2025-11-27T00:05:36.356Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/conftest.py:0-0
Timestamp: 2025-11-27T00:05:36.356Z
Learning: When reviewing PRs that add fixtures to a parent conftest that may duplicate fixtures in child conftest files, do not suggest removing the duplicates from child conftest files that are outside the scope of the PR's changes.
Applied to files:
tests/e2e/notifications/subscribers/conftest.py
📚 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/e2e/notifications/subscribers/conftest.pytests/unit/http/test_mixins.py
🧬 Code graph analysis (9)
mpt_api_client/resources/notifications/subscribers.py (1)
mpt_api_client/http/mixins.py (4)
AsyncDisableMixin(418-425)AsyncEnableMixin(398-405)DisableMixin(428-435)EnableMixin(408-415)
tests/e2e/notifications/subscribers/test_async_subscribers.py (6)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/conftest.py (1)
async_mpt_client(48-51)tests/e2e/notifications/subscribers/conftest.py (6)
subscriber_id(7-8)invalid_subscriber_id(12-13)subscriber_factory(31-41)recipients_factory(17-27)active_subscriber_id(59-64)disabled_subscriber_id(45-55)mpt_api_client/http/mixins.py (12)
get(364-377)get(383-395)fetch_page(512-519)fetch_page(581-588)filter(460-474)select(476-495)iterate(542-563)iterate(611-633)disable(421-425)disable(431-435)enable(401-405)enable(411-415)mpt_api_client/models/model.py (1)
id(119-121)
mpt_api_client/resources/accounts/buyers.py (1)
mpt_api_client/http/mixins.py (4)
AsyncDisableMixin(418-425)AsyncEnableMixin(398-405)DisableMixin(428-435)EnableMixin(408-415)
mpt_api_client/resources/accounts/licensees.py (1)
mpt_api_client/http/mixins.py (4)
AsyncDisableMixin(418-425)AsyncEnableMixin(398-405)DisableMixin(428-435)EnableMixin(408-415)
mpt_api_client/resources/accounts/api_tokens.py (1)
mpt_api_client/http/mixins.py (6)
AsyncDisableMixin(418-425)AsyncEnableMixin(398-405)AsyncManagedResourceMixin(662-665)CollectionMixin(509-575)DisableMixin(428-435)EnableMixin(408-415)
mpt_api_client/http/mixins.py (4)
mpt_api_client/models/model.py (1)
Model(65-125)mpt_api_client/http/async_service.py (1)
_resource_action(55-80)mpt_api_client/http/service.py (1)
_resource_action(55-80)mpt_api_client/http/types.py (1)
json(40-42)
mpt_api_client/resources/accounts/account.py (1)
mpt_api_client/http/mixins.py (8)
AsyncDisableMixin(418-425)AsyncEnableMixin(398-405)AsyncGetMixin(380-395)AsyncUpdateFileMixin(219-256)CollectionMixin(509-575)CreateFileMixin(115-144)DisableMixin(428-435)EnableMixin(408-415)
tests/e2e/notifications/subscribers/conftest.py (2)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/http/mixins.py (6)
get(364-377)get(383-395)disable(421-425)disable(431-435)enable(401-405)enable(411-415)
tests/unit/http/test_mixins.py (1)
mpt_api_client/http/mixins.py (4)
AsyncDisableMixin(418-425)AsyncEnableMixin(398-405)DisableMixin(428-435)EnableMixin(408-415)
🪛 Ruff (0.14.8)
tests/e2e/notifications/subscribers/conftest.py
18-18: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
32-32: Unused noqa directive (unknown: WPS430)
Remove unused noqa directive
(RUF100)
55-55: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/unit/http/test_mixins.py
11-11: Unused noqa directive (unknown: WPS235)
Remove unused noqa directive
(RUF100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (14)
tests/unit/http/test_mixins.py (1)
1314-1433: Well-structured tests for enable/disable mixins.Good use of parametrized tests to cover both
enableanddisableactions with and without request data. The assertions comprehensively verify:
- HTTP POST method and correct endpoint
- Request content serialization
- Response parsing and model type
mpt_api_client/http/mixins.py (1)
398-416: Clean implementation of Enable/Disable mixins.The new mixins follow the established patterns in this file, correctly delegating to
_resource_actionwith the appropriate HTTP method and action. Good separation of enable and disable into distinct mixins for flexible composition.mpt_api_client/resources/accounts/account.py (2)
43-54: Clean refactor to explicit Enable/Disable mixins.The change from the combined
EnablableMixinto separateEnableMixinandDisableMixinimproves clarity and follows the single-responsibility principle for mixin composition.
64-75: Async service updated consistently.The
AsyncAccountsServicecorrectly mirrors the sync service changes, usingAsyncEnableMixinandAsyncDisableMixin.tests/e2e/notifications/subscribers/test_async_subscribers.py (2)
1-6: Good e2e test setup for async subscriber operations.The
pytest.mark.flakymarker is appropriate for e2e tests that may have intermittent failures due to external dependencies.
52-70: Well-designed enable/disable state tests.Good use of
active_subscriber_idanddisabled_subscriber_idfixtures to ensure the subscriber is in the correct state before testing the opposite action. The assertions verify both the response existence and the expected status value.mpt_api_client/resources/accounts/api_tokens.py (1)
27-45: Consistent refactor for API tokens service.The changes follow the same pattern as other account services, replacing the combined
EnablableMixinwith explicitEnableMixinandDisableMixin. Both sync and async services are updated consistently.mpt_api_client/resources/accounts/licensees.py (1)
6-7: LGTM! Mixin migration is consistent.The replacement of
EnablableMixin/AsyncEnablableMixinwith separateEnableMixin/DisableMixinpairs follows the established pattern across the codebase. Import ordering and type parameterization withLicenseeare correct.Also applies to: 13-14, 38-39, 52-53
mpt_api_client/resources/notifications/subscribers.py (1)
4-5: LGTM! Subscriber services correctly adopt Enable/Disable mixins.The mixin additions are consistent with the repository-wide migration. Placing
EnableMixin/DisableMixinbeforeManagedResourceMixinensures proper MRO for method resolution.Also applies to: 8-9, 28-29, 39-40
mpt_api_client/resources/accounts/buyers.py (1)
6-7: LGTM! Buyers services correctly updated with Enable/Disable mixins.The mixin migration is consistent with the pattern applied across other account resources. The custom
synchronizeandtransfermethods remain unaffected.Also applies to: 13-14, 46-47, 80-81
tests/e2e/notifications/subscribers/conftest.py (4)
6-8: LGTM!Simple fixture that correctly retrieves the subscriber ID from e2e configuration.
11-13: LGTM!Clear fixture providing a well-formed but invalid ID for negative test cases.
16-27: LGTM!The
noqa: WPS430directive is correctly retained for wemake-python-styleguide's nested function warning. Type annotations were previously addressed and are now consistent. Based on learnings, WPS noqa directives should not be flagged as unused.
30-41: LGTM!Factory pattern is clean and consistent with
recipients_factory. Thenoqa: WPS430directive is valid per wemake-python-styleguide requirements.
- Add enable/disable actions for notifications subscriber - Add e2e test suite for notification subscribers
albertsola
force-pushed
the
feat/MPT-14936
branch
from
b1f30b2 to
b022e54
Compare
|
Summary by CodeRabbit
New Features
Tests
Configuration
✏️ Tip: You can customize this high-level summary in your review settings.