test: add docuseal webhook coverage#70
Closed
michaelmwu wants to merge 5 commits into
Closed
Conversation
Add /webhooks/docuseal endpoint that receives Docuseal form.completed events, looks up the signer by email in EspoCRM, and updates the cMemberAgreementSignedAt field on the contact. NOTE: The CRM field name cMemberAgreementSignedAt is a placeholder following the existing c-prefix convention. The client should confirm the actual field name and adjust in docuseal_processor.py if needed.
|
Warning Rate limit exceeded
⌛ 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 (3)
📝 WalkthroughWalkthroughAdds Docuseal webhook support: new /webhooks/docuseal API, payload models, job and actor registration, worker settings and env var, EspoCRM processor to record signed member agreements, and accompanying unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Docuseal as Docuseal Service
participant Webhook as API Webhook
participant Queue as Job Queue
participant Worker as Worker Process
participant CRM as EspoCRM API
Docuseal->>Webhook: POST form.completed event
activate Webhook
Webhook->>Webhook: Validate authorization
Webhook->>Webhook: Parse & validate payload
Webhook->>Webhook: Filter by event_type and template ID
Webhook->>Queue: Enqueue process_docuseal_agreement_job(email, completed_at, submission_id)
deactivate Webhook
Webhook-->>Docuseal: 202 Accepted / 200 Ignored
Queue->>Worker: Deliver job
activate Worker
Worker->>CRM: Search Contact by email
CRM-->>Worker: Return contact (or not)
alt contact found
Worker->>CRM: PUT update cMemberAgreementSignedAt, cSignedMemberAgreement
CRM-->>Worker: Success
else contact not found or error
CRM-->>Worker: Error / not found
end
deactivate Worker
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
apps/worker/src/five08/worker/config.py (1)
105-110: Normalize malformed numeric-string errors to one explicit message.
int(normalized)currently leaks Python’s raw parsing error text. Wrapping that into one stable setting-specific message improves operator UX.♻️ Suggested refinement
if isinstance(value, str): normalized = value.strip() if not normalized: return None - return int(normalized) + try: + return int(normalized) + except ValueError as exc: + raise ValueError( + "DOCUSEAL_MEMBER_AGREEMENT_TEMPLATE_ID must be an integer" + ) from exc raise TypeError("DOCUSEAL_MEMBER_AGREEMENT_TEMPLATE_ID must be an integer")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/worker/src/five08/worker/config.py` around lines 105 - 110, The parsing of DOCUSEAL_MEMBER_AGREEMENT_TEMPLATE_ID currently calls int(normalized) which can raise a raw ValueError; catch ValueError (and optionally TypeError) around int(normalized) and re-raise a single stable TypeError with the message "DOCUSEAL_MEMBER_AGREEMENT_TEMPLATE_ID must be an integer" so callers only see that explicit setting-specific error; update the block that handles str inputs (where normalized is computed) to perform this try/except and raise the unified TypeError.tests/unit/test_worker_config.py (1)
34-53: Add a negative-case test for invalid template-id strings.The new tests cover valid normalization, but a non-numeric input case (e.g.
"abc") should be pinned to prevent silent regression of validation behavior.✅ Suggested test addition
+def test_docuseal_template_id_rejects_non_numeric_string() -> None: + with pytest.raises(ValidationError): + WorkerSettings( + espo_base_url="https://crm.test.com", + espo_api_key="test-key", + docuseal_member_agreement_template_id="abc", + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_worker_config.py` around lines 34 - 53, Add a negative-case unit test named test_docuseal_template_id_rejects_non_numeric_string that constructs WorkerSettings with docuseal_member_agreement_template_id="abc" and asserts that creating the settings raises ValueError (i.e., pin the current validation behavior); reference the WorkerSettings class and the docuseal_member_agreement_template_id field so the test fails if non-numeric strings stop being rejected.tests/unit/test_backend_api.py (2)
751-770: Variable shadowing:payloadis reassigned.Line 766 reassigns
payloadtoresponse.json(), shadowing the request payload dict from line 751. While this doesn't affect test correctness, it reduces readability.♻️ Suggested rename for clarity
- payload = { + request_payload = { **_DOCUSEAL_PAYLOAD, "data": { **_DOCUSEAL_PAYLOAD["data"], "template": None, }, } with patch("five08.backend.api.enqueue_job") as mock_enqueue: mock_enqueue.return_value = Mock(id="job-ds-3") response = client.post( "/webhooks/docuseal", - json=payload, + json=request_payload, headers=auth_headers, ) - payload = response.json() + response_payload = response.json() assert response.status_code == 202 - assert payload["status"] == "queued" - assert payload["source"] == "docuseal" - assert payload["job_id"] == "job-ds-3" + assert response_payload["status"] == "queued" + assert response_payload["source"] == "docuseal" + assert response_payload["job_id"] == "job-ds-3"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_backend_api.py` around lines 751 - 770, The test reuses the variable name payload for both the request body and the response, causing variable shadowing and reduced readability; rename the request payload (the dict built from _DOCUSEAL_PAYLOAD) to something like request_payload (or ds_payload) and update the client.post call to use that name, leaving the response variable as response and keeping response.json() assigned to payload or response_payload to avoid confusion; update any subsequent assertions to reference the new variable names (references: the request payload variable originally named payload, the client.post call, and the response.json() assignment).
618-627: Clarify the submission_id source in the response.The test asserts
submission_id == 42, which matchesdata["id"]in the fixture, notdata["submission_id"](which is4200). This suggests the webhook handler usesdata.idas the submission identifier. The naming is potentially confusing since the fixture contains both fields with different values.Consider adding a brief comment in the test or fixture to clarify that
submission_idin the response corresponds todata.id(the submitter record ID), notdata.submission_id.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_backend_api.py` around lines 618 - 627, Clarify that the test's asserted submission_id comes from the fixture's data["id"] (submitter record ID) rather than data["submission_id"] (external submission number): update the test around the payload assertions (payload["submission_id"]) or the fixture to add a one-line comment stating "submission_id in the webhook response maps to data['id'] (submitter record ID), not data['submission_id'] which is 4200" so future readers of the webhook handler and tests (the code referencing response, payload, and the webhook handler that maps data.id) are not confused.apps/worker/src/five08/worker/crm/docuseal_processor.py (1)
70-71: Add defensive check for missing contact ID.If the CRM returns a contact without an
idfield (unexpected but possible), this will raise aKeyErrorthat bypasses the structured error handling.🛡️ Suggested defensive check
contact = contacts[0] - contact_id = contact["id"] + contact_id = contact.get("id") + if not contact_id: + logger.error( + "CRM returned contact without id for masked_email=%s", + masked_email, + ) + return { + "success": False, + "masked_email": masked_email, + "error": "contact_missing_id", + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/worker/src/five08/worker/crm/docuseal_processor.py` around lines 70 - 71, The code assumes contacts[0] contains an "id" and will KeyError if missing; update the docuseal_processor logic that sets contact = contacts[0] and contact_id = contact["id"] to defensively handle a missing id: check for the "id" key on the contact (or that contact.get("id") is truthy), and if absent either log an error via the existing logger/exception handling path and skip/raise a controlled exception (rather than letting a KeyError bubble), or provide a clear fallback path; update any surrounding error handling in the same function so missing-contact-id cases produce a structured error message referencing contact and contacts for debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/worker/src/five08/backend/api.py`:
- Around line 767-787: Logs and responses are exposing raw PII (the variable
email) in logger.exception, logger.info and the JSONResponse payload; update the
enqueue flow that returns/prints job info (references: logger.exception,
logger.info, JSONResponse and the variables email and submitter.id) to redact or
pseudonymize the email before using it in logs or the response (e.g., compute
email_masked or email_hash and use that value in both logger calls and the
returned JSON instead of the raw email).
- Around line 720-765: The job enqueue call uses submitter.id for both the job
args and idempotency key but the payload exposes a distinct submission_id;
change the enqueue_job invocation (the args tuple passed to
process_docuseal_agreement_job and the idempotency_key value) to use
submission_id when present (e.g., use submitter.submission_id if
non-empty/falsy-checked, falling back to submitter.id) so the downstream job and
the idempotency key consistently reference the payload's submission identifier
instead of always using submitter.id.
In `@apps/worker/src/five08/worker/jobs.py`:
- Around line 72-84: The logger in process_docuseal_agreement_job is emitting
raw PII (the email); change it to log a masked identifier instead: obtain a
masked value (reuse DocusealAgreementProcessor._masked_email or extract its
masking logic into a module-level utility) and log that masked_email and
submission_id in the logger.info call; ensure the function still passes the
original email to DocusealAgreementProcessor().process_agreement but never
writes the raw email to logs.
---
Nitpick comments:
In `@apps/worker/src/five08/worker/config.py`:
- Around line 105-110: The parsing of DOCUSEAL_MEMBER_AGREEMENT_TEMPLATE_ID
currently calls int(normalized) which can raise a raw ValueError; catch
ValueError (and optionally TypeError) around int(normalized) and re-raise a
single stable TypeError with the message "DOCUSEAL_MEMBER_AGREEMENT_TEMPLATE_ID
must be an integer" so callers only see that explicit setting-specific error;
update the block that handles str inputs (where normalized is computed) to
perform this try/except and raise the unified TypeError.
In `@apps/worker/src/five08/worker/crm/docuseal_processor.py`:
- Around line 70-71: The code assumes contacts[0] contains an "id" and will
KeyError if missing; update the docuseal_processor logic that sets contact =
contacts[0] and contact_id = contact["id"] to defensively handle a missing id:
check for the "id" key on the contact (or that contact.get("id") is truthy), and
if absent either log an error via the existing logger/exception handling path
and skip/raise a controlled exception (rather than letting a KeyError bubble),
or provide a clear fallback path; update any surrounding error handling in the
same function so missing-contact-id cases produce a structured error message
referencing contact and contacts for debugging.
In `@tests/unit/test_backend_api.py`:
- Around line 751-770: The test reuses the variable name payload for both the
request body and the response, causing variable shadowing and reduced
readability; rename the request payload (the dict built from _DOCUSEAL_PAYLOAD)
to something like request_payload (or ds_payload) and update the client.post
call to use that name, leaving the response variable as response and keeping
response.json() assigned to payload or response_payload to avoid confusion;
update any subsequent assertions to reference the new variable names
(references: the request payload variable originally named payload, the
client.post call, and the response.json() assignment).
- Around line 618-627: Clarify that the test's asserted submission_id comes from
the fixture's data["id"] (submitter record ID) rather than data["submission_id"]
(external submission number): update the test around the payload assertions
(payload["submission_id"]) or the fixture to add a one-line comment stating
"submission_id in the webhook response maps to data['id'] (submitter record ID),
not data['submission_id'] which is 4200" so future readers of the webhook
handler and tests (the code referencing response, payload, and the webhook
handler that maps data.id) are not confused.
In `@tests/unit/test_worker_config.py`:
- Around line 34-53: Add a negative-case unit test named
test_docuseal_template_id_rejects_non_numeric_string that constructs
WorkerSettings with docuseal_member_agreement_template_id="abc" and asserts that
creating the settings raises ValueError (i.e., pin the current validation
behavior); reference the WorkerSettings class and the
docuseal_member_agreement_template_id field so the test fails if non-numeric
strings stop being rejected.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.env.exampleapps/worker/src/five08/backend/api.pyapps/worker/src/five08/worker/actors.pyapps/worker/src/five08/worker/config.pyapps/worker/src/five08/worker/crm/docuseal_processor.pyapps/worker/src/five08/worker/jobs.pyapps/worker/src/five08/worker/models.pytests/unit/test_backend_api.pytests/unit/test_docuseal_processor.pytests/unit/test_worker_config.pytests/unit/test_worker_models.py
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/unit/test_backend_api.py (3)
599-600: Consider extracting shared test helper to avoid duplication.This helper is duplicated in
tests/unit/test_docuseal_processor.py(lines 9-10). Consider moving it to a shared location liketests/conftest.pyor atests/helpers.pymodule.♻️ Example extraction to conftest.py
In
tests/conftest.py:import hashlib def expected_masked_email(email: str) -> str: return hashlib.sha256(email.encode("utf-8")).hexdigest()[:12]Then import in both test files:
-def _expected_masked_email(email: str) -> str: - return hashlib.sha256(email.encode("utf-8")).hexdigest()[:12] +from tests.conftest import expected_masked_email🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_backend_api.py` around lines 599 - 600, The helper function _expected_masked_email is duplicated across tests; extract it into a shared test helper (e.g., define expected_masked_email(email: str) in tests/conftest.py or tests/helpers.py) and update both callers (references to _expected_masked_email in test_backend_api.py and the duplicate in test_docuseal_processor.py) to import and call the shared expected_masked_email function instead of keeping file-local duplicates.
750-776: Variable shadowing reduces readability.The variable
payloadis reused on line 772 to hold the response, shadowing the request payload defined on line 757. Consider using a distinct name likeresponse_bodyorresultfor clarity.♻️ Suggested rename
- payload = response.json() - assert response.status_code == 202 - assert payload["status"] == "queued" - assert payload["source"] == "docuseal" - assert payload["job_id"] == "job-ds-3" + result = response.json() + assert response.status_code == 202 + assert result["status"] == "queued" + assert result["source"] == "docuseal" + assert result["job_id"] == "job-ds-3"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_backend_api.py` around lines 750 - 776, In test_docuseal_webhook_processes_without_template_filter, the local variable payload is reused (first for the request payload, then for the response JSON) which causes shadowing and hurts readability; rename the second usage to a distinct identifier (e.g., response_body or result) and update the subsequent assertions to reference that new name (change the line that assigns response.json() and the assertions that follow), keeping the original request payload variable unchanged and ensuring all references use the updated symbol.
779-810: Same variable shadowing issue.Same pattern as above—
payloadis reused on line 801. Renaming toresultorresponse_bodywould improve clarity.♻️ Suggested rename
- payload = response.json() - assert response.status_code == 202 - assert payload["status"] == "queued" - assert payload["source"] == "docuseal" - assert payload["job_id"] == "job-ds-4" - assert payload["masked_email"] == _expected_masked_email("member@508.dev") - assert payload["submission_id"] == 42 + result = response.json() + assert response.status_code == 202 + assert result["status"] == "queued" + assert result["source"] == "docuseal" + assert result["job_id"] == "job-ds-4" + assert result["masked_email"] == _expected_masked_email("member@508.dev") + assert result["submission_id"] == 42🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_backend_api.py` around lines 779 - 810, In test_docuseal_webhook_uses_submitter_id_when_submission_id_missing rename the second reuse of the variable payload (the value returned by response.json()) to a distinct name like result or response_body to avoid shadowing the request payload; update subsequent references (assert payload["status"], payload["source"], payload["job_id"], payload["masked_email"], payload["submission_id"]) to use the new name and leave the original request payload variable intact, and ensure the call to mock_enqueue.call_args.kwargs remains unchanged (still checking idempotency_key == "docuseal-agreement:42").
🤖 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_backend_api.py`:
- Around line 599-600: The helper function _expected_masked_email is duplicated
across tests; extract it into a shared test helper (e.g., define
expected_masked_email(email: str) in tests/conftest.py or tests/helpers.py) and
update both callers (references to _expected_masked_email in test_backend_api.py
and the duplicate in test_docuseal_processor.py) to import and call the shared
expected_masked_email function instead of keeping file-local duplicates.
- Around line 750-776: In
test_docuseal_webhook_processes_without_template_filter, the local variable
payload is reused (first for the request payload, then for the response JSON)
which causes shadowing and hurts readability; rename the second usage to a
distinct identifier (e.g., response_body or result) and update the subsequent
assertions to reference that new name (change the line that assigns
response.json() and the assertions that follow), keeping the original request
payload variable unchanged and ensuring all references use the updated symbol.
- Around line 779-810: In
test_docuseal_webhook_uses_submitter_id_when_submission_id_missing rename the
second reuse of the variable payload (the value returned by response.json()) to
a distinct name like result or response_body to avoid shadowing the request
payload; update subsequent references (assert payload["status"],
payload["source"], payload["job_id"], payload["masked_email"],
payload["submission_id"]) to use the new name and leave the original request
payload variable intact, and ensure the call to mock_enqueue.call_args.kwargs
remains unchanged (still checking idempotency_key == "docuseal-agreement:42").
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/worker/src/five08/backend/api.pyapps/worker/src/five08/worker/jobs.pytests/unit/test_backend_api.py
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/worker/src/five08/worker/jobs.py
- apps/worker/src/five08/backend/api.py
Member
Author
|
#69 covers everything here |
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
2 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.
Description
Add /webhooks/docuseal endpoint that receives Docuseal form.completed events, looks up the signer by email in EspoCRM, and updates the cMemberAgreementSignedAt field on the contact.
NOTE: The CRM field name cMemberAgreementSignedAt is a placeholder following the existing c-prefix convention, and the client should confirm the actual field name and adjust in docuseal_processor.py if needed.
This implementation normalizes and validates payload fields, skips non-matching templates when DOCUSEAL_MEMBER_AGREEMENT_TEMPLATE_ID is set, and enqueues idempotent
docuseal-agreementjobs for processing.The Docuseal processor path now performs CRM contact lookup plus member agreement updates with safer logging that avoids writing raw email values.
Regression tests were added/expanded across
test_backend_api.py,test_worker_config.py,test_worker_models.py, andtest_docuseal_processor.pyfor webhook validation, template handling, and CRM processor success/error paths.Related Issue
N/A
How Has This Been Tested?
Not run in this environment per request; tests were added and are ready to run in CI or locally.
Summary by CodeRabbit
New Features
Tests