fix: handle DocuSeal submitter list response#255
Conversation
📝 WalkthroughWalkthroughThe Docuseal client now accepts any JSON from responses: ChangesDocuSeal Response Type Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates the shared DocuSeal client to tolerate the alternate submission-create response shape described in the PR: a submitter array instead of a submission object. It keeps the Discord CRM member-agreement flow working by normalizing that array back to the {"id": ...} shape the command currently expects.
Changes:
- Relax the low-level DocuSeal request helper so submission creation can accept non-object JSON responses.
- Normalize DocuSeal submitter-array responses into a stable submission object containing
id. - Add unit coverage for both submitter-array and object submission-create responses.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
packages/shared/src/five08/clients/docuseal.py |
Updates submission creation to normalize DocuSeal array responses back into the CRM-compatible submission shape. |
tests/unit/test_docuseal_client.py |
Adjusts existing expectations and adds regression tests for both array and object response bodies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/test_docuseal_client.py (1)
105-152: 💤 Low valueConsider adding error-case tests for normalization edge cases.
The new happy-path tests are solid. For completeness, you could add tests verifying that
DocusealAPIErroris raised for:
- Empty list response:
[]- Missing
submission_id:[{"id": 1}]- Non-dict submitter element:
[123]This would ensure the error handling in
_normalize_submission_submitters_responsehas explicit coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_docuseal_client.py` around lines 105 - 152, Add unit tests that assert DocusealAPIError is raised for edge-case submitter responses handled by _normalize_submission_submitters_response: add three tests calling create_member_agreement_submission (patched requests.request) that return status_code 201 but content/json of (1) an empty list [], (2) a list with a dict missing submission_id like [{"id": 1}], and (3) a list with a non-dict element like [123]; each test should confirm create_member_agreement_submission propagates/raises DocusealAPIError for these invalid normalization inputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/test_docuseal_client.py`:
- Around line 105-152: Add unit tests that assert DocusealAPIError is raised for
edge-case submitter responses handled by
_normalize_submission_submitters_response: add three tests calling
create_member_agreement_submission (patched requests.request) that return
status_code 201 but content/json of (1) an empty list [], (2) a list with a dict
missing submission_id like [{"id": 1}], and (3) a list with a non-dict element
like [123]; each test should confirm create_member_agreement_submission
propagates/raises DocusealAPIError for these invalid normalization inputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 254c2618-9ae8-4b35-b64d-430e67fa32fa
📒 Files selected for processing (2)
packages/shared/src/five08/clients/docuseal.pytests/unit/test_docuseal_client.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/test_docuseal_client.py (1)
105-224: ⚡ Quick winMissing test branch: valid JSON that is neither
dictnorlistThe
create_submissionimplementation has three branches (from the snippet atpackages/shared/src/five08/clients/docuseal.py:90-93):
isinstance(response_payload, dict)→ return as-is ✅ covered bytest_create_member_agreement_submission_keeps_object_responseisinstance(response_payload, list)→ normalize ✅ covered by the new list-shape tests- neither →
raise DocusealAPIError("API response is not valid JSON for a submission")❌ not coveredA response body like
42,true, or"ok"(valid JSON, but not dict/list) would hit this path silently in CI. Adding one test case closes the gap cheaply.✅ Suggested test to add
def test_create_member_agreement_submission_raises_on_scalar_json_response() -> None: """2xx responses with a scalar JSON value should raise DocusealAPIError.""" mock_response = Mock() mock_response.status_code = 200 mock_response.content = b"42" mock_response.json.return_value = 42 with patch( "five08.clients.docuseal.requests.request", return_value=mock_response, ): with pytest.raises( DocusealAPIError, match="API response is not valid JSON for a submission", ): create_member_agreement_submission( base_url="https://docuseal.example.com", api_key="secret", template_id=1000001, submitter_name="Jane Doe", submitter_email="jane@example.com", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_docuseal_client.py` around lines 105 - 224, Add a test that covers the missing branch where the DocuSeal response JSON is a scalar (neither dict nor list): mock requests.request to return a 2xx response whose .json() is a scalar (e.g., 42 or "ok"), then call create_member_agreement_submission and assert it raises DocusealAPIError with the message "API response is not valid JSON for a submission"; this ensures the branch in create_member_agreement_submission that raises DocusealAPIError for non-dict/non-list JSON is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/test_docuseal_client.py`:
- Around line 105-224: Add a test that covers the missing branch where the
DocuSeal response JSON is a scalar (neither dict nor list): mock
requests.request to return a 2xx response whose .json() is a scalar (e.g., 42 or
"ok"), then call create_member_agreement_submission and assert it raises
DocusealAPIError with the message "API response is not valid JSON for a
submission"; this ensures the branch in create_member_agreement_submission that
raises DocusealAPIError for non-dict/non-list JSON is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 139d88cb-9194-42a9-9603-8aadda92cfef
📒 Files selected for processing (1)
tests/unit/test_docuseal_client.py
Description
Handle DocuSeal submission create responses that return a submitter array instead of a JSON object. Normalize that response back to the submission ID shape expected by the Discord CRM flow, and add regression coverage for both array and object responses.
Related Issue
No matching issue found.
How Has This Been Tested?
uv run pytest tests/unit/test_docuseal_client.pyuv run pytest tests/unit/test_crm.py -k send_member_agreementSummary by CodeRabbit
Bug Fixes
Tests