feat: add Docuseal webhook endpoint for member agreement (#48)#67
Closed
happysmile001 wants to merge 1 commit into
Closed
feat: add Docuseal webhook endpoint for member agreement (#48)#67happysmile001 wants to merge 1 commit into
happysmile001 wants to merge 1 commit into
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.
📝 WalkthroughWalkthroughIntroduces a complete Docuseal webhook integration system. An API endpoint receives and validates webhook payloads, enqueues background jobs for form completion events, and processes them asynchronously to update CRM contact records with signed agreement timestamps. Changes
Sequence DiagramsequenceDiagram
actor Client as Docuseal<br/>(Client)
participant API as API<br/>Endpoint
participant Queue as Job<br/>Queue
participant Worker as Background<br/>Worker
participant CRM as EspoAPI<br/>(CRM)
Client->>API: POST /webhooks/docuseal<br/>(form.completed event)
API->>API: Validate auth & payload
API->>API: Filter event type
API->>Queue: enqueue_job<br/>(process_docuseal_agreement_job)
Queue-->>API: Job enqueued
API-->>Client: 202 Accepted
Worker->>Queue: Pick up job
Worker->>Worker: DocusealAgreementProcessor<br/>instantiate
Worker->>CRM: GET /Contact<br/>(search by email)
CRM-->>Worker: Contact found<br/>(id, details)
Worker->>CRM: PUT /Contact/{id}<br/>(set cMemberAgreementSignedAt)
CRM-->>Worker: Update successful
Worker->>Worker: Log completion<br/>success: true
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 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: 2
🧹 Nitpick comments (1)
tests/unit/test_backend_api.py (1)
603-681: Add regression tests for blank/whitespace email and timestamp inputs.Current tests don’t cover empty-string edge cases for
data.emailandcompleted_at/timestamp, which are critical for robust ingest validation.🧪 Suggested additional tests
+def test_docuseal_webhook_rejects_blank_email( + client: TestClient, + auth_headers: dict[str, str], +) -> None: + payload = { + **_DOCUSEAL_PAYLOAD, + "data": {**_DOCUSEAL_PAYLOAD["data"], "email": " "}, + } + response = client.post("/webhooks/docuseal", json=payload, headers=auth_headers) + assert response.status_code == 400 + assert response.json()["error"] == "invalid_payload" + + +def test_docuseal_webhook_rejects_blank_completed_timestamp( + client: TestClient, + auth_headers: dict[str, str], +) -> None: + payload = { + **_DOCUSEAL_PAYLOAD, + "timestamp": " ", + "data": {**_DOCUSEAL_PAYLOAD["data"], "completed_at": " "}, + } + response = client.post("/webhooks/docuseal", json=payload, headers=auth_headers) + assert response.status_code == 400 + assert response.json()["error"] == "invalid_payload"🤖 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 603 - 681, Add regression tests to tests/unit/test_backend_api.py to cover blank/whitespace edge cases for the Docuseal webhook: create two new tests (e.g., test_docuseal_webhook_rejects_blank_email and test_docuseal_webhook_rejects_blank_timestamp) that POST to "/webhooks/docuseal" using payloads based on _DOCUSEAL_PAYLOAD but with data.email set to "" and " " (whitespace) and completed_at/timestamp set to "" and " " respectively, and assert the handler returns a 400 with error "invalid_payload" (mirror the style of test_docuseal_webhook_rejects_invalid_payload). Ensure the tests use the TestClient and auth_headers fixtures and maintain naming/structure consistent with existing tests so they exercise the same validation logic in the webhook handler.
🤖 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 718-729: Normalize and validate the Docuseal fields before calling
enqueue_job: compute email = (submitter.email or "").strip() and completed_at =
submitter.completed_at or payload.timestamp (normalize/validate if it's a
string), then if email == "" or completed_at is falsy raise/return a 400 HTTP
error (e.g., HTTPException) and do not call enqueue_job; otherwise pass the
normalized email and completed_at into enqueue_job (keeping
idempotency_key=f"docuseal-agreement:{submitter.id}" and using
process_docuseal_agreement_job) so only non-blank values are enqueued.
In `@apps/worker/src/five08/worker/crm/docuseal_processor.py`:
- Around line 43-77: Replace raw email logging to avoid PII: update the logger
calls in docuseal_processor.py (the logger.error in the CRM search exception,
the logger.warning when no contact is found, the logger.error in the
EspoAPIError except block, and the final logger.info) to omit or mask the
variable email and instead log contact_id or submission_id (or a deterministic
masked/hash of email). Specifically, change references that pass email into
logger.* (e.g., the "CRM search failed for email=%s", "No CRM contact found for
email=%s", "CRM update failed for contact_id=%s: %s" and "Marked member
agreement signed contact_id=%s email=%s") so they only interpolate
contact_id/submission_id or a masked_email variable (created by hashing or
redacting) and ensure error payloads returned in dicts do not expose the raw
email either.
---
Nitpick comments:
In `@tests/unit/test_backend_api.py`:
- Around line 603-681: Add regression tests to tests/unit/test_backend_api.py to
cover blank/whitespace edge cases for the Docuseal webhook: create two new tests
(e.g., test_docuseal_webhook_rejects_blank_email and
test_docuseal_webhook_rejects_blank_timestamp) that POST to "/webhooks/docuseal"
using payloads based on _DOCUSEAL_PAYLOAD but with data.email set to "" and "
" (whitespace) and completed_at/timestamp set to "" and " " respectively, and
assert the handler returns a 400 with error "invalid_payload" (mirror the style
of test_docuseal_webhook_rejects_invalid_payload). Ensure the tests use the
TestClient and auth_headers fixtures and maintain naming/structure consistent
with existing tests so they exercise the same validation logic in the webhook
handler.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/worker/src/five08/backend/api.pyapps/worker/src/five08/worker/actors.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_worker_models.py
Comment on lines
+718
to
+729
| submitter = payload.data | ||
| completed_at = submitter.completed_at or payload.timestamp | ||
| queue = request.app.state.queue | ||
| try: | ||
| job: EnqueuedJob = await asyncio.to_thread( | ||
| enqueue_job, | ||
| queue=queue, | ||
| fn=process_docuseal_agreement_job, | ||
| args=(submitter.email, completed_at, submitter.id), | ||
| settings=settings, | ||
| idempotency_key=f"docuseal-agreement:{submitter.id}", | ||
| ) |
There was a problem hiding this comment.
Normalize and reject blank Docuseal identity/timestamp fields before enqueue.
submitter.email and completed_at/timestamp are used directly; whitespace/empty strings can still be queued. Add strip + non-empty validation before enqueue_job.
✅ Suggested validation hardening diff
submitter = payload.data
- completed_at = submitter.completed_at or payload.timestamp
+ email = submitter.email.strip().lower()
+ if not email:
+ return JSONResponse(
+ {"error": "invalid_payload", "detail": "data.email is required"},
+ status_code=400,
+ )
+
+ completed_at = (submitter.completed_at or payload.timestamp).strip()
+ if not completed_at:
+ return JSONResponse(
+ {"error": "invalid_payload", "detail": "completed_at/timestamp is required"},
+ status_code=400,
+ )
queue = request.app.state.queue
try:
job: EnqueuedJob = await asyncio.to_thread(
enqueue_job,
queue=queue,
fn=process_docuseal_agreement_job,
- args=(submitter.email, completed_at, submitter.id),
+ args=(email, completed_at, submitter.id),
settings=settings,
idempotency_key=f"docuseal-agreement:{submitter.id}",
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/worker/src/five08/backend/api.py` around lines 718 - 729, Normalize and
validate the Docuseal fields before calling enqueue_job: compute email =
(submitter.email or "").strip() and completed_at = submitter.completed_at or
payload.timestamp (normalize/validate if it's a string), then if email == "" or
completed_at is falsy raise/return a 400 HTTP error (e.g., HTTPException) and do
not call enqueue_job; otherwise pass the normalized email and completed_at into
enqueue_job (keeping idempotency_key=f"docuseal-agreement:{submitter.id}" and
using process_docuseal_agreement_job) so only non-blank values are enqueued.
Comment on lines
+43
to
+77
| logger.error("CRM search failed for email=%s: %s", email, exc) | ||
| return { | ||
| "success": False, | ||
| "email": email, | ||
| "error": f"CRM search failed: {exc}", | ||
| } | ||
|
|
||
| contacts = result.get("list", []) | ||
| if not contacts: | ||
| logger.warning("No CRM contact found for email=%s", email) | ||
| return {"success": False, "email": email, "error": "contact_not_found"} | ||
|
|
||
| contact = contacts[0] | ||
| contact_id = contact["id"] | ||
|
|
||
| try: | ||
| self.api.request( | ||
| "PUT", | ||
| f"Contact/{contact_id}", | ||
| {"cMemberAgreementSignedAt": completed_at}, | ||
| ) | ||
| except EspoAPIError as exc: | ||
| logger.error("CRM update failed for contact_id=%s: %s", contact_id, exc) | ||
| return { | ||
| "success": False, | ||
| "email": email, | ||
| "contact_id": contact_id, | ||
| "error": f"CRM update failed: {exc}", | ||
| } | ||
|
|
||
| logger.info( | ||
| "Marked member agreement signed contact_id=%s email=%s", | ||
| contact_id, | ||
| email, | ||
| ) |
There was a problem hiding this comment.
Redact signer email from logs to avoid PII leakage.
The new log lines emit raw email addresses. Please log submission_id/contact_id (or a masked hash) instead of full email.
🔒 Suggested log-hardening diff
- logger.error("CRM search failed for email=%s: %s", email, exc)
+ logger.error("CRM search failed submission_id=%s: %s", submission_id, exc)
...
- logger.warning("No CRM contact found for email=%s", email)
+ logger.warning("No CRM contact found submission_id=%s", submission_id)
...
- "Marked member agreement signed contact_id=%s email=%s",
+ "Marked member agreement signed contact_id=%s submission_id=%s",
contact_id,
- email,
+ submission_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 43 -
77, Replace raw email logging to avoid PII: update the logger calls in
docuseal_processor.py (the logger.error in the CRM search exception, the
logger.warning when no contact is found, the logger.error in the EspoAPIError
except block, and the final logger.info) to omit or mask the variable email and
instead log contact_id or submission_id (or a deterministic masked/hash of
email). Specifically, change references that pass email into logger.* (e.g., the
"CRM search failed for email=%s", "No CRM contact found for email=%s", "CRM
update failed for contact_id=%s: %s" and "Marked member agreement signed
contact_id=%s email=%s") so they only interpolate contact_id/submission_id or a
masked_email variable (created by hashing or redacting) and ensure error
payloads returned in dicts do not expose the raw email either.
Member
|
Great, thanks for this, I'll take it from here. We should be filtering on the template being the member agreement. The documentation of what a webhook looks like is: |
Member
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.
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.
Summary by CodeRabbit
New Features
Tests