Uh oh!
There was an error while loading. Please reload this page.
fix: normalize DocuSeal completed_at queue contract - #78
Merged
Conversation
📝 WalkthroughWalkthroughAdded webhook endpoint documentation and implemented UTC timestamp normalization for DocuSeal contract completion timestamps across the API layer, worker processor, and supporting tests. Includes validation logic to handle invalid timestamp inputs gracefully. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/worker/src/five08/worker/jobs.py (1)
16-17: Use one shared source for the DocuSeal datetime format across modules.Line 16 defines the contract format, but the same literal is still hardcoded in
apps/worker/src/five08/backend/api.py(Line 133) andapps/worker/src/five08/worker/crm/docuseal_processor.py(Line 29). Centralizing this in a neutral shared module would reduce contract drift risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/worker/src/five08/worker/jobs.py` around lines 16 - 17, Replace the duplicated DocuSeal datetime literal with a single shared constant: move DOCUSEAL_COMPLETED_AT_UTC_FORMAT into a neutral shared module (e.g., a new constant in five08 package like docuseal_constants or constants) and update all call sites to import that constant instead of hardcoding the string; specifically, replace literal occurrences in backend/api.py (where the format is used around the DocuSeal handling) and worker/crm/docuseal_processor.py to import DOCUSEAL_COMPLETED_AT_UTC_FORMAT, remove the duplicate literals, and run tests to ensure no import errors.apps/worker/src/five08/worker/crm/docuseal_processor.py (1)
84-100: Validatecompleted_atbefore CRM lookup to fail fast.Right now Line 85 runs after the CRM search request, so malformed timestamps still incur external I/O and can be masked by earlier
contact_not_foundpaths. Moving normalization ahead of the first API call would enforce the worker boundary contract more consistently.♻️ Suggested refactor sketch
def process_agreement(...): masked_email = mask_email(email) + try:+ crm_completed_at = self._normalize_completed_at(completed_at)+ except ValueError as exc:+ logger.error(+ "CRM update failed for invalid datetime=%s: %s",+ completed_at,+ exc,+ )+ return {+ "success": False,+ "masked_email": masked_email,+ "submission_id": submission_id,+ "error": f"invalid_completed_at: {exc}",+ }+ try: result = self.api.request("GET", "Contact", {...}) except EspoAPIError as exc: ... -- try:- crm_completed_at = self._normalize_completed_at(completed_at)- except ValueError as exc:- ...🤖 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 84 - 100, Move the completed_at normalization so it runs before any CRM lookup: call self._normalize_completed_at(completed_at) at the start of the processing flow (before the CRM search/lookup call that uses contact_id), catch ValueError and return the same error payload (including masked_email, submission_id, contact_id and error=f"invalid_completed_at: {exc}") and log via logger.error as already done; update the method containing the CRM search to remove the current try/except around _normalize_completed_at that occurs after the CRM call so malformed timestamps fail fast and avoid performing external I/O.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/worker/src/five08/worker/crm/docuseal_processor.py`:
- Around line 84-100: Move the completed_at normalization so it runs before any
CRM lookup: call self._normalize_completed_at(completed_at) at the start of the
processing flow (before the CRM search/lookup call that uses contact_id), catch
ValueError and return the same error payload (including masked_email,
submission_id, contact_id and error=f"invalid_completed_at: {exc}") and log via
logger.error as already done; update the method containing the CRM search to
remove the current try/except around _normalize_completed_at that occurs after
the CRM call so malformed timestamps fail fast and avoid performing external
I/O.
In `@apps/worker/src/five08/worker/jobs.py`:
- Around line 16-17: Replace the duplicated DocuSeal datetime literal with a
single shared constant: move DOCUSEAL_COMPLETED_AT_UTC_FORMAT into a neutral
shared module (e.g., a new constant in five08 package like docuseal_constants or
constants) and update all call sites to import that constant instead of
hardcoding the string; specifically, replace literal occurrences in
backend/api.py (where the format is used around the DocuSeal handling) and
worker/crm/docuseal_processor.py to import DOCUSEAL_COMPLETED_AT_UTC_FORMAT,
remove the duplicate literals, and run tests to ensure no import errors.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.mdapps/worker/README.mdapps/worker/src/five08/backend/api.pyapps/worker/src/five08/worker/crm/docuseal_processor.pyapps/worker/src/five08/worker/jobs.pytests/unit/test_backend_api.pytests/unit/test_docuseal_processor.py
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
Normalize DocuSeal webhook agreement timestamps into an explicit UTC queue contract (
YYYY-MM-DD HH:mm:ss) before enqueuing jobs and preserve the same contract through worker processing to prevent CRM validation failures.The webhook handler now normalizes
completed_atfrom raw ISO input (including offsets) into UTC before queueingprocess_docuseal_agreement_job.process_docuseal_agreement_jobandDocusealAgreementProcessorare documented to treatcompleted_atas a UTC string for JSON-serializable queue payloads.I added a worker-scoped docs file (
apps/worker/README.md) and pointed the root README webhook list to this contract.Regression coverage was added for webhook enqueue contract normalization and processor-side datetime formatting plus invalid timestamp handling.
Related Issue
None.
How Has This Been Tested?
Unit tests for
backend/api.pyanddocuseal_processor.pywere updated to assert UTC string conversion and persisted argument format.Summary by CodeRabbit
Documentation
Bug Fixes