feat: Docuseal member agreement webhook#69
Merged
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.
📝 WalkthroughWalkthroughAdds Docuseal webhook handling: new models, endpoint at /webhooks/docuseal, payload validation and optional template filtering, job enqueueing (idempotent) to process agreements, a CRM processor to update Espo contacts, masking utility, and accompanying tests and config entry. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant WebhookAPI as WebhookAPI
participant JobQueue as JobQueue
participant JobWorker as JobWorker
participant Processor as DocusealProcessor
participant Espo as EspoAPI
Client->>WebhookAPI: POST /webhooks/docuseal (payload + auth)
WebhookAPI->>WebhookAPI: Validate auth & parse JSON
WebhookAPI->>WebhookAPI: Validate payload model & event_type
WebhookAPI->>WebhookAPI: Check template_id against settings
alt Valid & Template Matches
WebhookAPI->>JobQueue: Enqueue process_docuseal_agreement_job (idempotency key)
JobQueue-->>WebhookAPI: Enqueue ACK (202)
JobWorker->>Processor: process_agreement(email, completed_at, submission_id)
Processor->>Espo: GET /Contact?email=...
alt Contact Found
Espo-->>Processor: Contact data (id)
Processor->>Espo: PUT /Contact/{id} (set cMemberAgreementSignedAt)
Espo-->>Processor: Success
Processor-->>JobWorker: Success payload (masked_email, contact_id, completed_at)
else Contact Not Found
Espo-->>Processor: Empty result
Processor-->>JobWorker: Failure payload (masked_email, contact_not_found)
end
else Invalid / Unmatched / Non-completed
WebhookAPI-->>Client: 400/200/401 as appropriate
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 (1)
apps/worker/src/five08/worker/crm/docuseal_processor.py (1)
70-71: Consider defensive access for contact ID.If the CRM returns a malformed response without an
idfield, this will raise an unhandledKeyError. While unlikely given theselectincludesid, defensive access improves resilience against unexpected API behavior.♻️ Suggested defensive access
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": "malformed_crm_response", + }🤖 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 pulls contact_id with contact["id"] which will KeyError if the CRM returns a contact without an id; change to defensive access by using contact.get("id") (or similar) and check for None before proceeding—if id is missing, log an error or raise a clear exception and skip processing that contact rather than allowing an unhandled KeyError; update the block around the contacts[0] assignment (variables contact, contacts, contact_id) to perform this validation and handle the missing-id path gracefully.
🤖 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 740-741: The code currently assigns and emits raw submitter.email
via the variable email; replace usage of raw email with a deterministic masked
hash (e.g., SHA-256 or HMAC with an application secret) and never log or return
submitter.email directly. Concretely: where the diff shows email =
(submitter.email or "").strip() and the subsequent logging/response code (also
at the referenced blocks around lines 767–778 and 780–785), compute a
hashed_email from submitter.email and use hashed_email in API responses and logs
instead of email, removing any occurrences that return or log the plaintext;
keep handling of empty/null the same so hashed_email is empty or a fixed
placeholder when no email is present. Ensure the hashing uses a stable method
(or HMAC with an app secret) so results are consistent but non-reversible.
- Around line 720-737: The webhook handler currently uses data.id for
idempotency and job payloads causing collisions; replace those uses with a
resolved submission_id that prefers data.submission_id and falls back to data.id
(e.g. submission_id = data.get("submission_id") or getattr(data,
"submission_id", None) or data.get("id")). Update all places that currently
reference data.id (the branches around template_filter_id / JSONResponse and the
other blocks noted) to use this submission_id when logging, returning
JSONResponse payloads, and when queuing jobs so the submitter/submitter.id
comparisons and job payloads use the correct submission identifier consistently.
Ensure you update the three occurrences mentioned (the template_filter check and
the other two blocks) to use the same submission_id resolution logic.
In `@apps/worker/src/five08/worker/jobs.py`:
- Around line 72-84: The logger in process_docuseal_agreement_job currently
emits the raw email (PII); modify it to log a non-reversible masked identifier
instead (e.g., compute a stable hash like SHA-256 of the email and log the hex
digest or a truncated prefix) and pass the raw email unchanged to
DocusealAgreementProcessor.process_agreement; update the logger.info call in
process_docuseal_agreement_job to use the hashed/masked value rather than the
plain email while keeping submission_id and other context the same.
---
Nitpick comments:
In `@apps/worker/src/five08/worker/crm/docuseal_processor.py`:
- Around line 70-71: The code pulls contact_id with contact["id"] which will
KeyError if the CRM returns a contact without an id; change to defensive access
by using contact.get("id") (or similar) and check for None before proceeding—if
id is missing, log an error or raise a clear exception and skip processing that
contact rather than allowing an unhandled KeyError; update the block around the
contacts[0] assignment (variables contact, contacts, contact_id) to perform this
validation and handle the missing-id path gracefully.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.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_worker_models.py
michaelmwu
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.env.example (1)
93-93: PlaceDOCUSEAL_MEMBER_AGREEMENT_TEMPLATE_IDin dotenv-linter key order.Line 93 currently triggers
UnorderedKey; move it beforeMAX_ATTACHMENTS_PER_CONTACTto keep lint clean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.example at line 93, The DOCUSEAL_MEMBER_AGREEMENT_TEMPLATE_ID key is out of dotenv-linter order; open .env.example and move the DOCUSEAL_MEMBER_AGREEMENT_TEMPLATE_ID entry so it appears before the MAX_ATTACHMENTS_PER_CONTACT entry (keeping any surrounding blank lines/comment grouping intact) to satisfy the linter's key order rule.tests/unit/test_worker_config.py (1)
45-53: Add a non-numeric input test for the template id.Consider adding one case like
"abc"to pin the failure contract for invalid values and prevent silent behavior drift.🤖 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 45 - 53, Add a new unit test (e.g., test_docuseal_template_id_rejects_non_numeric_string) that tries to construct WorkerSettings with docuseal_member_agreement_template_id="abc" and asserts that construction raises a ValueError (use pytest.raises(ValueError)); this pins the failure contract for non-numeric inputs and complements test_docuseal_template_id_accepts_numeric_string which checks coercion to int.
🤖 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/worker/masking.py`:
- Around line 10-15: The code currently exposes initials via masked_local =
(local[:1] if local else "*") + "***" and domain[:1], so replace character-based
masking with a deterministic hash-based mask: compute a stable hash (e.g.,
hashlib.sha256) of the identifiable input (either the whole email or
local+domain) and use a short prefix of the hex digest (e.g., first 8 chars) as
the visible token instead of local[:1]/domain[:1]; update the return expressions
that currently use masked_local and domain[:1] to use the hash prefix (for
example return f"{hash_prefix}@****..." or split into hash_local and hash_domain
prefixes if you need both parts), and ensure the hashing is consistent (use
.encode() on the string and hex digest) so tests relying on deterministic masked
values pass.
In `@tests/unit/test_docuseal_processor.py`:
- Around line 30-32: The assertion is too brittle by checking exact dict
equality on mock_api.request.call_args_list[1].args[2]; instead, assert that the
payload contains the required key and correct value (e.g., assert
"cMemberAgreementSignedAt" in payload and payload["cMemberAgreementSignedAt"] ==
"2026-02-25T12:00:00Z") or assert that a subset dict is contained (use .items()
issubset) so future CRM fields won’t break the test; update the assertion in
test_docuseal_processor.py to check for the required key/value rather than exact
dict equality.
---
Nitpick comments:
In @.env.example:
- Line 93: The DOCUSEAL_MEMBER_AGREEMENT_TEMPLATE_ID key is out of dotenv-linter
order; open .env.example and move the DOCUSEAL_MEMBER_AGREEMENT_TEMPLATE_ID
entry so it appears before the MAX_ATTACHMENTS_PER_CONTACT entry (keeping any
surrounding blank lines/comment grouping intact) to satisfy the linter's key
order rule.
In `@tests/unit/test_worker_config.py`:
- Around line 45-53: Add a new unit test (e.g.,
test_docuseal_template_id_rejects_non_numeric_string) that tries to construct
WorkerSettings with docuseal_member_agreement_template_id="abc" and asserts that
construction raises a ValueError (use pytest.raises(ValueError)); this pins the
failure contract for non-numeric inputs and complements
test_docuseal_template_id_accepts_numeric_string which checks coercion to int.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.env.exampleapps/worker/src/five08/backend/api.pyapps/worker/src/five08/worker/crm/docuseal_processor.pyapps/worker/src/five08/worker/jobs.pyapps/worker/src/five08/worker/masking.pytests/unit/test_backend_api.pytests/unit/test_docuseal_processor.pytests/unit/test_worker_config.pytests/unit/test_worker_models.py
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/worker/src/five08/backend/api.py
- apps/worker/src/five08/worker/crm/docuseal_processor.py
- tests/unit/test_worker_models.py
- tests/unit/test_backend_api.py
- apps/worker/src/five08/worker/jobs.py
Comment on lines
+10
to
+15
| masked_local = (local[:1] if local else "*") + "***" | ||
|
|
||
| if not domain: | ||
| return f"{masked_local}@****..." | ||
|
|
||
| return f"{masked_local}@{domain[:1]}****..." |
There was a problem hiding this comment.
Use hash-based masking instead of partial-character exposure.
This currently reveals email initials (local[:1], domain[:1]), which weakens PII redaction and does not match a “masked email hash” requirement.
🔧 Proposed fix
+import hashlib
+
def mask_email(email: str) -> str:
- """Return a deterministic redacted email representation for logs and responses."""
- local, at, domain = email.partition("@")
- if not at:
- return "***"
-
- masked_local = (local[:1] if local else "*") + "***"
-
- if not domain:
- return f"{masked_local}@****..."
-
- return f"{masked_local}@{domain[:1]}****..."
+ """Return deterministic non-reversible email token for logs/responses."""
+ normalized = email.strip().lower()
+ if "@" not in normalized:
+ return "email#invalid"
+ digest = hashlib.sha256(normalized.encode("utf-8")).hexdigest()[:12]
+ return f"email#{digest}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/worker/src/five08/worker/masking.py` around lines 10 - 15, The code
currently exposes initials via masked_local = (local[:1] if local else "*") +
"***" and domain[:1], so replace character-based masking with a deterministic
hash-based mask: compute a stable hash (e.g., hashlib.sha256) of the
identifiable input (either the whole email or local+domain) and use a short
prefix of the hex digest (e.g., first 8 chars) as the visible token instead of
local[:1]/domain[:1]; update the return expressions that currently use
masked_local and domain[:1] to use the hash prefix (for example return
f"{hash_prefix}@****..." or split into hash_local and hash_domain prefixes if
you need both parts), and ensure the hashing is consistent (use .encode() on the
string and hex digest) so tests relying on deterministic masked values pass.
Comment on lines
+30
to
+32
| assert mock_api.request.call_args_list[1].args[2] == { | ||
| "cMemberAgreementSignedAt": "2026-02-25T12:00:00Z", | ||
| } |
There was a problem hiding this comment.
Avoid exact payload equality in the update assertion.
This assertion is brittle and can block legitimate CRM payload expansion (e.g., additional agreement-related fields). Assert required keys instead of exact dict equality.
✅ Suggested test adjustment
- assert mock_api.request.call_args_list[1].args[2] == {
- "cMemberAgreementSignedAt": "2026-02-25T12:00:00Z",
- }
+ update_payload = mock_api.request.call_args_list[1].args[2]
+ assert update_payload["cMemberAgreementSignedAt"] == "2026-02-25T12:00:00Z"
+ # Optional when applicable:
+ # assert update_payload.get("cSignedMemberAgreement") is True📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| assert mock_api.request.call_args_list[1].args[2] == { | |
| "cMemberAgreementSignedAt": "2026-02-25T12:00:00Z", | |
| } | |
| update_payload = mock_api.request.call_args_list[1].args[2] | |
| assert update_payload["cMemberAgreementSignedAt"] == "2026-02-25T12:00:00Z" | |
| # Optional when applicable: | |
| # assert update_payload.get("cSignedMemberAgreement") is True |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/test_docuseal_processor.py` around lines 30 - 32, The assertion is
too brittle by checking exact dict equality on
mock_api.request.call_args_list[1].args[2]; instead, assert that the payload
contains the required key and correct value (e.g., assert
"cMemberAgreementSignedAt" in payload and payload["cMemberAgreementSignedAt"] ==
"2026-02-25T12:00:00Z") or assert that a subset dict is contained (use .items()
issubset) so future CRM fields won’t break the test; update the assertion in
test_docuseal_processor.py to check for the required key/value rather than exact
dict equality.
This was referenced Mar 2, 2026
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
Related Issue
How Has This Been Tested?
Summary by CodeRabbit