MPT-14937 E2E for notifications accounts categories contacts #170
Conversation
albertsola
requested review from
alephsur,
d3rky,
jentyk,
robcsegal,
ruben-sebrango and
svazquezco
as code owners
📝 WalkthroughWalkthroughRenamed resource model to Changes
Sequence Diagram(s)(omitted — changes are primarily API signature/model and tests, not multi-component control-flow additions) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mpt_api_client/resources/notifications/accounts.py (1)
11-12: Fix incorrect docstring for Contact class.The docstring says "Account resource." but this class represents a Contact, not an Account. This could confuse developers reading the code.
🔎 Proposed fix
class Contact(Model): - """Account resource.""" + """Contact resource."""
🧹 Nitpick comments (2)
tests/unit/resources/notifications/test_accounts.py (2)
19-23: Simplify test and fix misleading function name.The
@pytest.mark.parametrizedecorator is unnecessary when testing only a single method. The function nametest_sync_batches_service_methodsreferences "batches" which is misleading—the test validates theiteratemethod presence, not batch-specific functionality.🔎 Proposed refactor
-@pytest.mark.parametrize("method", ["iterate"]) -def test_sync_batches_service_methods(accounts_service, method): - result = hasattr(accounts_service, method) - - assert result is True +def test_sync_accounts_service_has_iterate(accounts_service): + assert hasattr(accounts_service, "iterate")
26-30: Simplify test and fix misleading function name.Same issue as the sync variant: the parametrize decorator is unnecessary, and the function name references "batches" instead of accurately describing what's being tested.
🔎 Proposed refactor
-@pytest.mark.parametrize("method", ["iterate"]) -def test_async_batches_service_methods(async_accounts_service, method): - result = hasattr(async_accounts_service, method) - - assert result is True +def test_async_accounts_service_has_iterate(async_accounts_service): + assert hasattr(async_accounts_service, "iterate")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
mpt_api_client/resources/notifications/accounts.py(2 hunks)tests/e2e/notifications/accounts/test_async_accounts.py(1 hunks)tests/e2e/notifications/accounts/test_sync_accounts.py(1 hunks)tests/unit/resources/notifications/test_accounts.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/accounts/test_async_accounts.pytests/unit/resources/notifications/test_accounts.pytests/e2e/notifications/accounts/test_sync_accounts.py
📚 Learning: 2025-11-27T00:05:54.701Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/accounts/accounts_users/test_async_accounts_users.py:0-0
Timestamp: 2025-11-27T00:05:54.701Z
Learning: In pytest-asyncio, async fixtures are automatically resolved before being passed to test functions (both sync and async). Test functions receive the yielded value from async fixtures, not coroutines, so fixture parameters should never be awaited. A sync test function can use async fixtures without any special handling.
Applied to files:
tests/unit/resources/notifications/test_accounts.py
🧬 Code graph analysis (3)
tests/e2e/notifications/accounts/test_async_accounts.py (2)
tests/e2e/conftest.py (2)
async_mpt_ops(36-39)account_id(122-123)tests/e2e/notifications/conftest.py (1)
category_id(5-6)
tests/e2e/notifications/accounts/test_sync_accounts.py (2)
tests/e2e/conftest.py (2)
mpt_ops(31-32)account_id(122-123)tests/e2e/notifications/conftest.py (1)
category_id(5-6)
mpt_api_client/resources/notifications/accounts.py (3)
mpt_api_client/http/mixins.py (2)
AsyncCollectionMixin(578-645)CollectionMixin(509-575)mpt_api_client/resources/notifications/contacts.py (1)
Contact(11-12)mpt_api_client/resources/accounts/account.py (3)
AccountsService(43-61)AccountsServiceConfig(33-40)AsyncAccountsService(64-82)
⏰ 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). (2)
- GitHub Check: build
- GitHub Check: danger
🔇 Additional comments (6)
tests/e2e/notifications/accounts/test_sync_accounts.py (1)
1-8: LGTM! E2E test validates the integration successfully.The test correctly verifies that the sync accounts iterator can be instantiated and consumed. While the assertion only checks the result type, this is acceptable for an e2e test focused on validating that the endpoint is reachable and returns valid data structure.
tests/e2e/notifications/accounts/test_async_accounts.py (1)
1-8: LGTM! Async e2e test validates the integration successfully.The test correctly uses async iteration patterns with async comprehension to collect results from the async iterator. The minimal type assertion is appropriate for e2e validation.
mpt_api_client/resources/notifications/accounts.py (4)
3-3: LGTM! Mixin imports added correctly.The CollectionMixin and AsyncCollectionMixin imports are necessary for the new inheritance structure and enable collection iteration functionality.
23-23: LGTM! CollectionMixin inheritance enables iteration functionality.Adding
CollectionMixin[Contact]properly enablesiterate(),fetch_page(), andfetch_one()methods for the sync service. The multiple inheritance order is correct with the mixin appearing beforeService[Contact].
27-29: LGTM! AsyncCollectionMixin inheritance enables async iteration functionality.Adding
AsyncCollectionMixin[Contact]correctly enables async iteration methods for the async service. The inheritance order follows best practices with the mixin beforeAsyncService[Contact].
18-18: This is a new endpoint in the notifications namespace, not a migration from commerce.The endpoint at line 18 was newly created in the notifications namespace. Git history shows only one commit creating this file, with no prior existence in a commerce namespace. There is no breaking change for existing code, as no old commerce endpoint implementation exists in this codebase to migrate from.
Likely an incorrect or invalid review comment.
albertsola
changed the title
albertsola
force-pushed
the
e2e/MPT-14863/Notification-categories-contacts-endpoint
branch
from
5a77946 to
597f15a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mpt_api_client/resources/notifications/accounts.py (1)
11-12: Fix docstring to match the class name.The docstring says "Account resource" but the class is named
Contact. This should be corrected for consistency.🔎 Proposed fix
class Contact(Model): - """Account resource.""" + """Contact resource."""
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
mpt_api_client/resources/notifications/accounts.pytests/e2e/notifications/accounts/test_async_accounts.pytests/e2e/notifications/accounts/test_sync_accounts.pytests/unit/resources/notifications/test_accounts.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/resources/notifications/test_accounts.py
- tests/e2e/notifications/accounts/test_async_accounts.py
- tests/e2e/notifications/accounts/test_sync_accounts.py
🧰 Additional context used
🧬 Code graph analysis (1)
mpt_api_client/resources/notifications/accounts.py (4)
mpt_api_client/resources/notifications/contacts.py (1)
Contact(11-12)mpt_api_client/resources/accounts/account.py (3)
AccountsService(43-61)AccountsServiceConfig(33-40)AsyncAccountsService(64-82)mpt_api_client/http/service.py (1)
Service(12-80)mpt_api_client/http/async_service.py (1)
AsyncService(12-80)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
mpt_api_client/resources/notifications/accounts.py (3)
3-3: LGTM! Collection mixin imports added correctly.The imports for
CollectionMixinandAsyncCollectionMixinare necessary to support the iterate pattern on both sync and async service classes.
23-29: LGTM! Collection mixins added correctly.Both
AccountsServiceandAsyncAccountsServicenow properly inherit from their respective collection mixins (CollectionMixinandAsyncCollectionMixin). This follows the established pattern in the codebase and will expose theiteratemethod for collection iteration. The inheritance order is correct for proper method resolution.
18-18: The namespace change from/public/v1/commerce/accounts/...to/public/v1/notifications/accounts/...is verified as intentional and complete. The git history confirms this was a deliberate migration (commit 597f15a for MPT-14863), all dependent code has been updated, and no stale references to the old endpoint exist. The service is properly integrated into the notifications module with corresponding tests in place.
albertsola
force-pushed
the
e2e/MPT-14863/Notification-categories-contacts-endpoint
branch
from
597f15a to
9972063
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
mpt_api_client/resources/notifications/accounts.pytests/e2e/notifications/accounts/test_async_accounts.pytests/e2e/notifications/accounts/test_sync_accounts.pytests/unit/resources/notifications/test_accounts.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/resources/notifications/test_accounts.py
- tests/e2e/notifications/accounts/test_async_accounts.py
- tests/e2e/notifications/accounts/test_sync_accounts.py
🧰 Additional context used
🧬 Code graph analysis (1)
mpt_api_client/resources/notifications/accounts.py (3)
mpt_api_client/http/mixins.py (2)
AsyncCollectionMixin(578-645)CollectionMixin(509-575)mpt_api_client/resources/accounts/account.py (3)
AccountsServiceConfig(33-40)AccountsService(43-61)AsyncAccountsService(64-82)mpt_api_client/resources/notifications/contacts.py (1)
Contact(11-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
mpt_api_client/resources/notifications/accounts.py (4)
3-3: LGTM!The import correctly adds
CollectionMixinandAsyncCollectionMixinto support the new collection iteration functionality for both sync and async service classes.
15-20: LGTM! Endpoint migrated to notifications namespace.The endpoint path change from
/public/v1/commerce/...to/public/v1/notifications/...aligns with the notifications service context. Ensure downstream consumers and documentation are updated accordingly.
23-24: LGTM!The
CollectionMixin[Contact]addition follows the established pattern (as seen inmpt_api_client/resources/accounts/account.py) and correctly positions mixins before the base service class in the MRO.
27-29: LGTM!The async service correctly mirrors the sync version's inheritance pattern, adding
AsyncCollectionMixin[Contact]to provide async collection iteration capabilities.
albertsola
force-pushed
the
e2e/MPT-14863/Notification-categories-contacts-endpoint
branch
from
9972063 to
b6de8a7
Compare
albertsola
force-pushed
the
e2e/MPT-14863/Notification-categories-contacts-endpoint
branch
from
b6de8a7 to
649c11b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/e2e/notifications/accounts/test_sync_accounts.py (1)
6-8: Consider strengthening the test assertion.The current assertion only verifies that
resultis a list, but doesn't validate the content or types of items returned by the iterator. Consider asserting that items areNotificationContactinstances or checking for expected attributes.🔎 Suggested enhancement
result = list(iterator) -assert isinstance(result, list) +assert isinstance(result, list) +if result: # Only check if non-empty + from mpt_api_client.resources.notifications.accounts import NotificationContact + assert all(isinstance(item, NotificationContact) for item in result)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
mpt_api_client/resources/notifications/accounts.pytests/e2e/notifications/accounts/test_async_accounts.pytests/e2e/notifications/accounts/test_sync_accounts.pytests/unit/resources/notifications/test_accounts.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/notifications/accounts/test_async_accounts.py
- tests/unit/resources/notifications/test_accounts.py
🧰 Additional context used
🧠 Learnings (1)
📚 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/accounts/test_sync_accounts.py
🧬 Code graph analysis (2)
mpt_api_client/resources/notifications/accounts.py (5)
mpt_api_client/http/mixins.py (2)
AsyncCollectionMixin(578-645)CollectionMixin(509-575)mpt_api_client/models/model.py (1)
Model(65-125)mpt_api_client/resources/accounts/account.py (3)
AccountsServiceConfig(33-40)AccountsService(43-61)AsyncAccountsService(64-82)mpt_api_client/http/service.py (1)
Service(12-80)mpt_api_client/http/async_service.py (1)
AsyncService(12-80)
tests/e2e/notifications/accounts/test_sync_accounts.py (2)
tests/e2e/conftest.py (2)
mpt_ops(31-32)account_id(122-123)tests/e2e/notifications/conftest.py (1)
category_id(5-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
mpt_api_client/resources/notifications/accounts.py (4)
3-3: LGTM: Collection mixin imports enable iteration functionality.The import additions correctly support the
CollectionMixinandAsyncCollectionMixinfunctionality added to the service classes below.
11-12: Good: Model rename addresses previous feedback.Renaming
ContacttoNotificationContactsuccessfully addresses the naming conflict raised in earlier reviews, making it clear this model represents contacts in the notifications context rather than general contacts.Based on learnings, this change resolves the duplicate class concern.
23-33: LGTM: Collection mixins correctly enable iteration functionality.The addition of
CollectionMixinandAsyncCollectionMixinto both service classes properly enables collection operations (iterate(),fetch_page(),fetch_one()). The type parameters correctly useNotificationContact, and the structure follows established patterns from other services likeAccountsServiceinaccounts/account.py.
18-18: Endpoint migration from commerce to notifications is complete and verified.The namespace change from
/public/v1/commerce/accounts/...to/public/v1/notifications/accounts/...has been properly completed. No references to the old commerce endpoint remain in the codebase, the new notifications endpoint is integrated into the notifications module, and the AccountsService has corresponding unit and E2E tests validating its functionality.
|
d3rky
deleted the
e2e/MPT-14863/Notification-categories-contacts-endpoint
branch
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.