MPT-20245 update fixtures and tests for chat participants, unskip tests#303
Merged
MPT-20245 update fixtures and tests for chat participants, unskip tests#303
Conversation
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdded Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
robcsegal
approved these changes
Apr 16, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/helpdesk/chats/participants/conftest.py`:
- Around line 25-31: Fixtures created_chat_participant and
async_created_chat_participant create remote participants but don't clean them
up; convert them to yield-style fixtures that create the participant, yield the
created participant(s) to the test, and then delete them in the teardown step.
Specifically, change created_chat_participant to a pytest fixture that calls
chat_participants_service.create([chat_participant_data]), yields the created
object(s), and then calls chat_participants_service.delete(...) (using the
returned id(s) from create) in the finally/teardown; do the analogous change for
async_created_chat_participant using
async_chat_participants_service.create([...]), async yield, and
async_chat_participants_service.delete(...) to remove created participant(s)
after the test.
- Around line 25-31: The fixture functions created_chat_participant and
async_created_chat_participant are passing a list into create(), causing
CreateMixin.create()/AsyncCreateMixin.create() to treat the payload as a batch
and return a ModelCollection; change the calls to pass the single dict directly
by replacing chat_participants_service.create([chat_participant_data]) with
chat_participants_service.create(chat_participant_data) and likewise await
async_chat_participants_service.create(chat_participant_data) in
async_created_chat_participant; after this change update any tests that
currently assert a ModelCollection to expect a ChatParticipant (or implement a
dedicated batch-create method if true batch semantics are required).
In `@tests/e2e/helpdesk/chats/participants/test_async_participants.py`:
- Around line 19-22: The nested any(... to_list()) in
test_create_chat_participant causes a WPS221 violation; refactor by extracting
the generator expression result into a small local variable or helper inside the
test (e.g., found_contact = any(cp["contact"]["id"] == contact_id for cp in
async_created_chat_participant.to_list()) ) and then assert found_contact;
update test_create_chat_participant to keep the three asserts but replace the
nested expression with that local variable to satisfy flake8/WPS221 while
preserving the same logic.
- Around line 25-32: The test_update_chat_participant currently mutates
async_created_chat_participant[0] directly which can pick the wrong participant;
instead locate the correct participant from the created collection
(async_created_chat_participant) by matching the expected contact id or other
unique key, convert that matched item to dict, set its "status" to "Active",
then call async_chat_participants_service.update with that matched participant's
id and payload (use identifiers like async_created_chat_participant,
test_update_chat_participant, and async_chat_participants_service to find where
to change).
In `@tests/e2e/helpdesk/chats/participants/test_sync_participants.py`:
- Around line 19-22: The assertion that checks for the contact id should be
simplified to avoid the WPS221 flake8 error: in test_create_chat_participant use
created_chat_participant (ModelCollection) and extract the contact ids first
(e.g., map or list comprehension over created_chat_participant/to_list() pulling
each cp["contact"]["id"]) and then assert that contact_id is in that extracted
list (use membership assertion) so the check is a single simple expression;
locate this change in test_create_chat_participant where
created_chat_participant and ChatParticipant are used.
- Around line 25-31: The test_update_chat_participant currently updates
created_chat_participant[0] directly which is order-dependent; instead locate
the correct participant by matching contact_id in the created_chat_participant
collection, select that participant (e.g., by filtering created_chat_participant
for item["contact_id"] == expected_contact_id), modify its status and call
chat_participants_service.update(participant["id"], participant); then assert
the returned ChatParticipant has the same id/contact_id and the updated status
to ensure the right entity was updated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: bec7eb96-9ec8-4114-8183-74ba78ce3427
📒 Files selected for processing (4)
e2e_config.test.jsontests/e2e/helpdesk/chats/participants/conftest.pytests/e2e/helpdesk/chats/participants/test_async_participants.pytests/e2e/helpdesk/chats/participants/test_sync_participants.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests/e2e/helpdesk/chats/participants/test_sync_participants.py (1)
26-32:⚠️ Potential issue | 🟠 MajorSync update test still mutates index
0, which is brittle.This remains order-dependent and may update the wrong participant while still passing type-only assertions. Select by
contact_idand validate updated fields.🎯 Suggested fix
-def test_update_chat_participant(chat_participants_service, created_chat_participant): - chat_participant = created_chat_participant[0].to_dict() +def test_update_chat_participant(chat_participants_service, created_chat_participant, contact_id): + chat_participant = next( + participant.to_dict() + for participant in created_chat_participant + if participant["contact"]["id"] == contact_id + ) chat_participant["status"] = "Active" result = chat_participants_service.update(chat_participant["id"], chat_participant) assert isinstance(result, ChatParticipant) + assert result["id"] == chat_participant["id"] + assert result["status"] == "Active"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/helpdesk/chats/participants/test_sync_participants.py` around lines 26 - 32, The test_update_chat_participant is brittle because it mutates index 0 of created_chat_participant rather than locating the participant by contact_id; update the test to find the correct participant from created_chat_participant by matching contact_id, modify its status to "Active", call chat_participants_service.update(participant_id, payload) using that participant's id, and assert both the returned type is ChatParticipant and that the returned object's fields (e.g., status and contact_id) match the expected updated values to ensure the right participant was updated.tests/e2e/helpdesk/chats/participants/conftest.py (1)
24-31:⚠️ Potential issue | 🟠 MajorCreated participant fixtures still leak remote resources (no teardown).
Both fixtures create API data and return immediately, so create/update tests can leave participants behind. Please switch to
yieldfixtures and delete created participant IDs in teardown (keep broadMPTAPIErrorcatch + diagnostic print).♻️ Suggested fix
+from mpt_api_client.exceptions import MPTAPIError import pytest @@ `@pytest.fixture` def created_chat_participant(chat_participants_service, chat_participant_data): - return chat_participants_service.create([chat_participant_data]) + created = chat_participants_service.create([chat_participant_data]) + yield created + for participant in created.to_list(): + try: + chat_participants_service.delete(participant["id"]) + except MPTAPIError as error: + print(f"TEARDOWN - Unable to delete resource {participant}: {error.title}") # noqa: WPS421 @@ `@pytest.fixture` async def async_created_chat_participant(async_chat_participants_service, chat_participant_data): - return await async_chat_participants_service.create([chat_participant_data]) + created = await async_chat_participants_service.create([chat_participant_data]) + yield created + for participant in created.to_list(): + try: + await async_chat_participants_service.delete(participant["id"]) + except MPTAPIError as error: + print(f"TEARDOWN - Unable to delete resource {participant}: {error.title}") # noqa: WPS421Based on learnings, "In end-to-end tests (e.g., tests/e2e/...), reuse existing API resources for read-only operations and safe mutations to speed up test execution. Reserve isolated fixtures ... for destructive tests that require per-test creation and cleanup."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/helpdesk/chats/participants/conftest.py` around lines 24 - 31, The fixtures created_chat_participant and async_created_chat_participant currently create remote participants and leak them; change both to yield-style fixtures that create the participant(s), yield the created result to tests, and in the teardown block delete the created participant IDs using chat_participants_service.delete (and async_chat_participants_service.delete for the async fixture), wrapping deletes in a broad MPTAPIError except block that prints diagnostic info; ensure you reference the original create calls (create([chat_participant_data])) and remove any returned IDs from the yielded result during teardown.tests/e2e/helpdesk/chats/participants/test_async_participants.py (1)
26-33:⚠️ Potential issue | 🟠 MajorUpdate test is still order-dependent and can target the wrong participant.
Selecting
async_created_chat_participant[0]can update an unrelated item if ordering changes. Pick bycontact_id(same strategy as delete) and assert the updated entity identity/state.🎯 Suggested fix
async def test_update_chat_participant( - async_chat_participants_service, async_created_chat_participant + async_chat_participants_service, async_created_chat_participant, contact_id ): - chat_participant = async_created_chat_participant[0].to_dict() + chat_participant = next( + participant.to_dict() + for participant in async_created_chat_participant + if participant["contact"]["id"] == contact_id + ) chat_participant["status"] = "Active" result = await async_chat_participants_service.update(chat_participant["id"], chat_participant) assert isinstance(result, ChatParticipant) + assert result["id"] == chat_participant["id"] + assert result["status"] == "Active"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/helpdesk/chats/participants/test_async_participants.py` around lines 26 - 33, The test_update_chat_participant currently uses async_created_chat_participant[0] which is order-dependent; instead locate the participant by its contact_id (like the delete test) from async_created_chat_participant list, set its "status" to "Active", call async_chat_participants_service.update(participant["id"], participant), then assert the returned or fetched participant has the same id and the updated status to ensure identity and state are correct; update references to async_created_chat_participant and the assertions accordingly in the test_update_chat_participant function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/helpdesk/chats/participants/conftest.py`:
- Around line 15-16: The contact_id fixture currently uses
e2e_config.get("notifications.contact.id") which can return None and cause later
cryptic failures; update the contact_id function to explicitly retrieve and
validate the value from e2e_config (using e2e_config["notifications.contact.id"]
or checking for None) and raise a clear exception (e.g., ValueError or
RuntimeError) with a descriptive message when the key is missing or empty so the
test run fails fast and points to the misconfiguration.
---
Duplicate comments:
In `@tests/e2e/helpdesk/chats/participants/conftest.py`:
- Around line 24-31: The fixtures created_chat_participant and
async_created_chat_participant currently create remote participants and leak
them; change both to yield-style fixtures that create the participant(s), yield
the created result to tests, and in the teardown block delete the created
participant IDs using chat_participants_service.delete (and
async_chat_participants_service.delete for the async fixture), wrapping deletes
in a broad MPTAPIError except block that prints diagnostic info; ensure you
reference the original create calls (create([chat_participant_data])) and remove
any returned IDs from the yielded result during teardown.
In `@tests/e2e/helpdesk/chats/participants/test_async_participants.py`:
- Around line 26-33: The test_update_chat_participant currently uses
async_created_chat_participant[0] which is order-dependent; instead locate the
participant by its contact_id (like the delete test) from
async_created_chat_participant list, set its "status" to "Active", call
async_chat_participants_service.update(participant["id"], participant), then
assert the returned or fetched participant has the same id and the updated
status to ensure identity and state are correct; update references to
async_created_chat_participant and the assertions accordingly in the
test_update_chat_participant function.
In `@tests/e2e/helpdesk/chats/participants/test_sync_participants.py`:
- Around line 26-32: The test_update_chat_participant is brittle because it
mutates index 0 of created_chat_participant rather than locating the participant
by contact_id; update the test to find the correct participant from
created_chat_participant by matching contact_id, modify its status to "Active",
call chat_participants_service.update(participant_id, payload) using that
participant's id, and assert both the returned type is ChatParticipant and that
the returned object's fields (e.g., status and contact_id) match the expected
updated values to ensure the right participant was updated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: eeb5ff37-8e99-4d6e-adf6-40c639c2701d
📒 Files selected for processing (4)
e2e_config.test.jsontests/e2e/helpdesk/chats/participants/conftest.pytests/e2e/helpdesk/chats/participants/test_async_participants.pytests/e2e/helpdesk/chats/participants/test_sync_participants.py
✅ Files skipped from review due to trivial changes (1)
- e2e_config.test.json
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/e2e/helpdesk/chats/participants/test_sync_participants.py (1)
26-32:⚠️ Potential issue | 🟡 MinorAssert the update result, not just its type.
Even if updating the first participant is intentional here, this test can still pass when the server ignores the
"status": "Active"change. Add a postcondition on the returnedid/statusso it proves the update succeeded.Suggested assertion tightening
result = chat_participants_service.update(chat_participant["id"], chat_participant) assert isinstance(result, ChatParticipant) + assert result["id"] == chat_participant["id"] + assert result["status"] == "Active"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/helpdesk/chats/participants/test_sync_participants.py` around lines 26 - 32, The test_update_chat_participant currently only asserts the returned type; change it to verify the update actually applied by asserting the returned ChatParticipant's id matches chat_participant["id"] and its status equals "Active" (e.g., assert result.id == chat_participant["id"] and assert result.status == "Active"); if the returned object is a dict use its keys (id/status) or call to_dict() on result accordingly; keep using chat_participants_service and created_chat_participant as the source of truth for expected values.tests/e2e/helpdesk/chats/participants/test_async_participants.py (1)
29-35:⚠️ Potential issue | 🟡 MinorAssert the update actually changed the participant.
This still passes if the API returns a
ChatParticipantwithout applying"status": "Active". Please assert the returnedid/statusso the test verifies the mutation, not just the response type.Suggested assertion tightening
result = await async_chat_participants_service.update(chat_participant["id"], chat_participant) assert isinstance(result, ChatParticipant) + assert result["id"] == chat_participant["id"] + assert result["status"] == "Active"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/helpdesk/chats/participants/test_async_participants.py` around lines 29 - 35, The test only asserts the return type after calling async_chat_participants_service.update, but not that the mutation actually applied; update the assertions to verify the returned object's id and status match the requested change (use async_created_chat_participant[0].to_dict() to get the original id, assert result.id equals that id and assert result.status == "Active", or compare result.to_dict()["status"] == "Active") so the test confirms the update took effect rather than just returning a ChatParticipant instance.
🧹 Nitpick comments (1)
tests/e2e/helpdesk/chats/participants/conftest.py (1)
24-31: Make the fixture contract explicit.These fixtures now return a collection, but their singular names still read like they yield a single
ChatParticipant. Renaming them to plural, or at least adding explicit return annotations, would make the new shape much harder to misuse in callers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/helpdesk/chats/participants/conftest.py` around lines 24 - 31, The fixtures created_chat_participant and async_created_chat_participant currently return the result of chat_participants_service.create / async_chat_participants_service.create (a collection) but their singular names imply a single ChatParticipant; either rename them to created_chat_participants / async_created_chat_participants or add explicit return type annotations (e.g., List[ChatParticipant]) and update docstrings to reflect the collection shape so callers won't misuse them; change the function names or add the typing on the fixtures and update any tests that import these fixtures to match the new names/types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/e2e/helpdesk/chats/participants/test_async_participants.py`:
- Around line 29-35: The test only asserts the return type after calling
async_chat_participants_service.update, but not that the mutation actually
applied; update the assertions to verify the returned object's id and status
match the requested change (use async_created_chat_participant[0].to_dict() to
get the original id, assert result.id equals that id and assert result.status ==
"Active", or compare result.to_dict()["status"] == "Active") so the test
confirms the update took effect rather than just returning a ChatParticipant
instance.
In `@tests/e2e/helpdesk/chats/participants/test_sync_participants.py`:
- Around line 26-32: The test_update_chat_participant currently only asserts the
returned type; change it to verify the update actually applied by asserting the
returned ChatParticipant's id matches chat_participant["id"] and its status
equals "Active" (e.g., assert result.id == chat_participant["id"] and assert
result.status == "Active"); if the returned object is a dict use its keys
(id/status) or call to_dict() on result accordingly; keep using
chat_participants_service and created_chat_participant as the source of truth
for expected values.
---
Nitpick comments:
In `@tests/e2e/helpdesk/chats/participants/conftest.py`:
- Around line 24-31: The fixtures created_chat_participant and
async_created_chat_participant currently return the result of
chat_participants_service.create / async_chat_participants_service.create (a
collection) but their singular names imply a single ChatParticipant; either
rename them to created_chat_participants / async_created_chat_participants or
add explicit return type annotations (e.g., List[ChatParticipant]) and update
docstrings to reflect the collection shape so callers won't misuse them; change
the function names or add the typing on the fixtures and update any tests that
import these fixtures to match the new names/types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: a1d9e267-a6d5-4022-be51-51c6994db373
📒 Files selected for processing (4)
e2e_config.test.jsontests/e2e/helpdesk/chats/participants/conftest.pytests/e2e/helpdesk/chats/participants/test_async_participants.pytests/e2e/helpdesk/chats/participants/test_sync_participants.py
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e_config.test.json
alephsur
approved these changes
Apr 16, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mpt_api_client/models/model.py`:
- Around line 223-227: The constructor change renamed the parameter
resource_data to resource in Model.__init__, breaking callers using
resource_data; update Model.__init__ to accept both names (e.g., keep parameter
resource: Resource | None = None and also accept resource_data as an alias via
kwargs or a second parameter) and ensure the implementation uses the provided
value (prefer resource if both supplied, otherwise fall back to resource_data)
when building data (the dict(resource or {} ) logic and data.update(kwargs)).
Adjust any references in Model.__init__ so existing callers using resource_data
continue to populate self.meta and data unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 25b007cf-6f0a-4322-80d4-ee8b3bc7f87c
📒 Files selected for processing (5)
e2e_config.test.jsonmpt_api_client/models/model.pytests/e2e/helpdesk/chats/participants/conftest.pytests/e2e/helpdesk/chats/participants/test_async_participants.pytests/e2e/helpdesk/chats/participants/test_sync_participants.py
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e_config.test.json
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes MPT-20245