Add member agreement Docuseal flow and role grant#215
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRenamed Discord admin env var to Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Worker as Worker<br/>(Docuseal Processor)
participant CRM as CRM<br/>(Espo)
participant DiscordBot as Discord Bot<br/>(Internal API)
participant Discord as Discord<br/>API
User->>Worker: POST /webhooks/docuseal (agreement signed)
Worker->>CRM: GET contact (include Discord fields)
CRM-->>Worker: contact data
Worker->>CRM: PUT contact (set cMemberAgreementSignedAt)
CRM-->>Worker: success
alt Discord user linked
Worker->>DiscordBot: POST /internal/member-agreements/member-role<br/>(X-API-Secret, payload)
DiscordBot->>DiscordBot: validate secret, resolve guild & role & member
DiscordBot->>Discord: add "Member" role
Discord-->>DiscordBot: success / error
DiscordBot-->>Worker: {status: "applied"/"already_present"/"error"}
else Not linked / missing config
Worker->>Worker: set member_role.status ("not_linked" / "bot_endpoint_not_configured" / "api_secret_not_configured")
end
Worker-->>User: {success: true, member_role: {...}}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
Pull request overview
Adds a Docuseal-backed member agreement flow that, upon signature completion, updates the CRM timestamp and then makes a best-effort call into the Discord bot to grant the Member role for linked contacts. It also refactors the Discord bot’s HTTP server to host both health and authenticated internal automation routes, and renames the guild setting to DISCORD_SERVER_ID.
Changes:
- Introduce shared Discord bot internal API client + worker-side best-effort role grant on Docuseal completion.
- Add Discord bot authenticated internal route for
Memberrole granting and refactor healthcheck into shared HTTP server wiring. - Rename
DISCORD_ADMIN_GUILD_ID→DISCORD_SERVER_IDacross config/docs/examples; addDISCORD_BOT_INTERNAL_BASE_URLsetting.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_worker_config.py | Adds coverage for worker default bot internal base URL. |
| tests/unit/test_internal_api.py | New unit tests for bot internal automation routes (auth + role grant behavior). |
| tests/unit/test_healthcheck.py | Updates tests to reflect refactor from server class to route collection. |
| tests/unit/test_docuseal_processor.py | Extends processor tests to validate CRM update + best-effort Discord role grant results. |
| tests/unit/test_discord_bot_client.py | New tests for shared Discord bot internal API client helper. |
| tests/unit/test_bot_http.py | New tests for bot HTTP server wiring/start-stop behavior. |
| packages/shared/src/five08/clients/discord_bot.py | Adds shared client for authenticated bot internal endpoints + helper function. |
| packages/shared/src/five08/clients/init.py | Exposes the new discord_bot client module. |
| apps/worker/src/five08/worker/crm/docuseal_processor.py | After CRM update, best-effort call to bot internal API to grant Member role. |
| apps/worker/src/five08/worker/config.py | Adds discord_bot_internal_base_url; renames guild id setting to discord_server_id. |
| apps/worker/README.md | Documents best-effort Discord role grant after Docuseal signature processing. |
| apps/discord_bot/src/five08/discord_bot/utils/internal_api.py | Implements authenticated internal endpoint to grant Member role. |
| apps/discord_bot/src/five08/discord_bot/utils/healthcheck.py | Refactors healthcheck into route registration helper. |
| apps/discord_bot/src/five08/discord_bot/utils/bot_http.py | New shared aiohttp server hosting health + internal routes. |
| apps/discord_bot/src/five08/discord_bot/config.py | Adds discord_server_id setting for selecting target guild. |
| apps/discord_bot/src/five08/discord_bot/bot.py | Switches from healthcheck server to shared bot HTTP server lifecycle. |
| apps/api/src/five08/backend/auth.py | Updates Discord API fallback to use discord_server_id. |
| README.md | Updates environment variable docs; documents DISCORD_BOT_INTERNAL_BASE_URL. |
| ENVIRONMENT.md | Renames documented guild env var to DISCORD_SERVER_ID. |
| .env.example | Renames env var and adds DISCORD_BOT_INTERNAL_BASE_URL example. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| base_url = settings.discord_bot_internal_base_url.strip() | ||
| if not base_url: | ||
| return {"status": "bot_endpoint_not_configured"} | ||
|
|
||
| api_secret = str(settings.api_shared_secret or "").strip() | ||
| if not api_secret: | ||
| return {"status": "api_secret_not_configured"} | ||
|
|
There was a problem hiding this comment.
The new _grant_member_role branches for missing bot base URL / missing API secret return distinct statuses, but there are no unit tests covering these paths. Consider adding tests that assert the returned member_role status (and that the bot client is not called) so misconfiguration handling doesn’t regress.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
apps/worker/src/five08/worker/crm/docuseal_processor.py (1)
74-85: Preserve best-effort semantics for unexpected client exceptions.At
Line 74, onlyDiscordBotAPIErroris handled. If any other exception escapes, agreement processing can fail after the CRM write. Consider a final broad exception guard that still returns an"error"member-role status.🛡️ Suggested defensive catch-all
except DiscordBotAPIError as exc: logger.warning( "Best-effort Member role assignment failed contact_id=%s discord_user_id=%s: %s", contact_id, normalized_user_id, exc, ) return { "status": "error", "discord_user_id": normalized_user_id, "error": str(exc), } + except Exception as exc: + logger.warning( + "Best-effort Member role assignment failed unexpectedly contact_id=%s discord_user_id=%s: %s", + contact_id, + normalized_user_id, + exc, + ) + return { + "status": "error", + "discord_user_id": normalized_user_id, + "error": f"unexpected_error: {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 74 - 85, The current except block only catches DiscordBotAPIError in the member-role assignment within the docuseal processing flow (see the except DiscordBotAPIError handling); add a final broad except Exception as exc immediately after it to preserve best-effort semantics so unexpected client exceptions don't break agreement processing after CRM write: log the exception (using logger.warning or logger.exception with context similar to the existing message) and return the same error payload shape ({"status": "error", "discord_user_id": normalized_user_id, "error": str(exc)}) instead of allowing the exception to propagate..env.example (1)
79-80: Optional: reorder worker env keys to satisfy dotenv-linter.
Line 80currently triggers the reportedUnorderedKeywarning; placing it beforeWORKER_API_BASE_URLshould clear the lint warning.♻️ Suggested ordering tweak
WORKER_NAME=worker -WORKER_API_BASE_URL=http://api:8090 DISCORD_BOT_INTERNAL_BASE_URL=http://discord_bot:3000 +WORKER_API_BASE_URL=http://api:8090 WORKER_QUEUE_NAMES=jobs.default🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.example around lines 79 - 80, UnorderedKey lint warning is caused by environment variable ordering; move DISCORD_BOT_INTERNAL_BASE_URL to be listed before WORKER_API_BASE_URL in the .env.example file so the sequence places DISCORD_BOT_INTERNAL_BASE_URL above WORKER_API_BASE_URL, satisfying dotenv-linter ordering rules.apps/discord_bot/src/five08/discord_bot/utils/internal_api.py (2)
56-66: Consider fetching guild when not in cache.
bot.get_guild()only checks the internal cache. If the guild isn't cached (e.g., after bot reconnect), this returnsNoneeven though the bot is a member. Consider usingawait bot.fetch_guild(int(configured_guild_id))as a fallback, similar to howfetch_memberis used later.♻️ Optional: Add fetch fallback for guild resolution
def _resolve_target_guild(self) -> discord.Guild | None: configured_guild_id = str(settings.discord_server_id or "").strip() if configured_guild_id: try: - return self.bot.get_guild(int(configured_guild_id)) + guild = self.bot.get_guild(int(configured_guild_id)) + if guild is None: + # Guild not in cache; attempt fetch + # Note: This would require making method async + pass + return guild except ValueError: return NoneNote: This would require making
_resolve_target_guildasync. Given that the bot likely has the guild cached in normal operation, this is optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/discord_bot/src/five08/discord_bot/utils/internal_api.py` around lines 56 - 66, _resolve_target_guild currently uses bot.get_guild which only checks the cache and can return None if the guild isn't cached; change _resolve_target_guild to be async, try bot.get_guild(int(configured_guild_id)) first and if that returns None call await bot.fetch_guild(int(configured_guild_id)) as a fallback (handle discord.NotFound/HTTPException similarly), and keep the existing fallback of using self.bot.guilds[0] when no configured_guild_id; update any callers to await _resolve_target_guild and mirror how fetch_member is used elsewhere.
94-97: Consider logging the exception for debugging.The broad
except Exceptioncatch is reasonable here (treating all fetch failures as "member not found"), but logging the actual exception would help diagnose issues like rate limits or API errors.♻️ Log the exception before discarding
try: member = await guild.fetch_member(discord_user_id) - except Exception: + except Exception as exc: + logger.debug( + "fetch_member failed for user_id=%s: %s", discord_user_id, exc + ) member = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/discord_bot/src/five08/discord_bot/utils/internal_api.py` around lines 94 - 97, The except block that swallows errors after calling guild.fetch_member(discord_user_id) should capture the exception as e and log it before setting member = None; update the try/except to use "except Exception as e:" and call the module or existing logger (e.g., logger.exception or logger.warning with exception info) including context such as discord_user_id and guild.id to aid debugging, then keep member = None so behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.env.example:
- Around line 79-80: UnorderedKey lint warning is caused by environment variable
ordering; move DISCORD_BOT_INTERNAL_BASE_URL to be listed before
WORKER_API_BASE_URL in the .env.example file so the sequence places
DISCORD_BOT_INTERNAL_BASE_URL above WORKER_API_BASE_URL, satisfying
dotenv-linter ordering rules.
In `@apps/discord_bot/src/five08/discord_bot/utils/internal_api.py`:
- Around line 56-66: _resolve_target_guild currently uses bot.get_guild which
only checks the cache and can return None if the guild isn't cached; change
_resolve_target_guild to be async, try bot.get_guild(int(configured_guild_id))
first and if that returns None call await
bot.fetch_guild(int(configured_guild_id)) as a fallback (handle
discord.NotFound/HTTPException similarly), and keep the existing fallback of
using self.bot.guilds[0] when no configured_guild_id; update any callers to
await _resolve_target_guild and mirror how fetch_member is used elsewhere.
- Around line 94-97: The except block that swallows errors after calling
guild.fetch_member(discord_user_id) should capture the exception as e and log it
before setting member = None; update the try/except to use "except Exception as
e:" and call the module or existing logger (e.g., logger.exception or
logger.warning with exception info) including context such as discord_user_id
and guild.id to aid debugging, then keep member = None so behavior is unchanged.
In `@apps/worker/src/five08/worker/crm/docuseal_processor.py`:
- Around line 74-85: The current except block only catches DiscordBotAPIError in
the member-role assignment within the docuseal processing flow (see the except
DiscordBotAPIError handling); add a final broad except Exception as exc
immediately after it to preserve best-effort semantics so unexpected client
exceptions don't break agreement processing after CRM write: log the exception
(using logger.warning or logger.exception with context similar to the existing
message) and return the same error payload shape ({"status": "error",
"discord_user_id": normalized_user_id, "error": str(exc)}) instead of allowing
the exception to propagate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 21bfb57c-24bb-4cb3-9d53-afb6238a07ba
📒 Files selected for processing (20)
.env.exampleENVIRONMENT.mdREADME.mdapps/api/src/five08/backend/auth.pyapps/discord_bot/src/five08/discord_bot/bot.pyapps/discord_bot/src/five08/discord_bot/config.pyapps/discord_bot/src/five08/discord_bot/utils/bot_http.pyapps/discord_bot/src/five08/discord_bot/utils/healthcheck.pyapps/discord_bot/src/five08/discord_bot/utils/internal_api.pyapps/worker/README.mdapps/worker/src/five08/worker/config.pyapps/worker/src/five08/worker/crm/docuseal_processor.pypackages/shared/src/five08/clients/__init__.pypackages/shared/src/five08/clients/discord_bot.pytests/unit/test_bot_http.pytests/unit/test_discord_bot_client.pytests/unit/test_docuseal_processor.pytests/unit/test_healthcheck.pytests/unit/test_internal_api.pytests/unit/test_worker_config.py
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3ea56bef9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| contact = contacts[0] | ||
| contact_id = contact["id"] | ||
| contact_name = str(contact.get("name") or "").strip() or None | ||
| discord_user_id = str(contact.get("cDiscordUserID") or "").strip() or None |
There was a problem hiding this comment.
The new role-grant path only reads cDiscordUserID from the CRM contact, so linked users stored under other aliases (e.g. cDiscordUserId/discordUserId) are treated as unlinked and never get the Member role after signing. This is a real regression because the worker already documents/handles those aliases in people_sync.py (_discord_user_id checks multiple keys), which indicates production payloads can use more than one key shape.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f46aa84a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for key in _DISCORD_USER_ID_FIELDS: | ||
| candidate = str(contact.get(key) or "").strip() | ||
| if candidate: | ||
| return candidate | ||
| return None |
There was a problem hiding this comment.
The new Docuseal role-grant path treats some already-linked contacts as unlinked because _contact_discord_user_id only checks explicit ID fields and then returns None. Contacts that store linkage only in cDiscordUsername (for example mention-style values) will skip Member-role assignment after signing, even though the existing people sync logic (people_sync._discord_user_id) still resolves those records as linked. This creates inconsistent behavior for the same CRM data shape and silently drops role grants.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a Codex generated review. Please validate.
Addressed in 0030d6d. Please validate that DocusealAgreementProcessor._contact_discord_user_id now mirrors the existing people-sync behavior by parsing mention-style IDs from cDiscordUsername, and that the added unit test covers this fallback.
| if self.bot.guilds: | ||
| return self.bot.guilds[0] |
There was a problem hiding this comment.
When DISCORD_SERVER_ID is not configured, _resolve_target_guild silently picks self.bot.guilds[0], which makes role assignment nondeterministic for bots connected to multiple guilds. In that scenario the worker can grant Member in the wrong server (or fail against an unrelated guild) based purely on guild ordering, so Docuseal completion behavior becomes environment-dependent. The route should fail closed unless the target guild is unambiguous.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a Codex generated review. Please validate.
Addressed in 0030d6d. Please validate that internal role grants now fail closed unless DISCORD_SERVER_ID is configured or the bot is connected to exactly one guild, and that the updated tests cover both the unambiguous and ambiguous cases.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/test_docuseal_processor.py (1)
182-203: Assert that the CRM lookup selects the alias under test.The mock returns
field_nameregardless of the request, so this test would still pass if Line 135-138 stopped selecting that alias from Espo. Pin the firstmock_api.request()call as part of this test.Suggested change
assert result["success"] is True assert result["discord_user_id"] == field_value assert result["member_role"]["status"] == "applied" assert mock_grant_role.call_args.kwargs["discord_user_id"] == field_value + search_params = mock_api.request.call_args_list[0].args[2] + assert field_name in search_params["select"].split(",")🤖 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 182 - 203, The test currently lets mock_api.request return the same alias regardless of which CRM request is made, so update the test to pin the first Espo API call explicitly: configure mock_api.request.side_effect with a first return value that contains the specific alias/field_name expected for the CRM lookup and subsequent return values for other calls (or raise if unexpected), then call DocusealAgreementProcessor().process_agreement and assert mock_api.request.call_args_list[0] (or rely on the pinned side_effect) corresponds to the alias lookup and that grant_member_role_for_signed_agreement received that alias via mock_grant_role.call_args.kwargs["discord_user_id"]; reference the mock_api object, its request method, and DocusealAgreementProcessor.process_agreement when making the change.
🤖 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/crm/docuseal_processor.py`:
- Around line 91-96: The warning log currently emits the raw Discord identifier
(normalized_user_id) which must be redacted; modify the logger.warning call in
the same block so it does not include normalized_user_id directly—either replace
it with a constant placeholder like "<redacted>" or call an existing masking
helper (e.g., mask_user_id(normalized_user_id)) before logging; keep contact_id
and exc as-is and ensure the logged message and format string reflect the
removed/masked identifier (update the format args to match).
- Around line 72-78: The early-return branches in process_agreement() that check
settings.discord_bot_internal_base_url and settings.api_shared_secret currently
return statuses without any observability; before returning {"status":
"bot_endpoint_not_configured"} or {"status": "api_secret_not_configured"} add a
warning log and/or increment a metric so missing configuration is observable
(e.g., use the existing process_logger.warn/error or a metrics counter used
elsewhere in docuseal_processor.py) — update the checks around base_url =
settings.discord_bot_internal_base_url.strip() and api_secret =
str(settings.api_shared_secret or "").strip() to emit the warning/metric and
include contextual info (function name and which config is missing) before
returning.
---
Nitpick comments:
In `@tests/unit/test_docuseal_processor.py`:
- Around line 182-203: The test currently lets mock_api.request return the same
alias regardless of which CRM request is made, so update the test to pin the
first Espo API call explicitly: configure mock_api.request.side_effect with a
first return value that contains the specific alias/field_name expected for the
CRM lookup and subsequent return values for other calls (or raise if
unexpected), then call DocusealAgreementProcessor().process_agreement and assert
mock_api.request.call_args_list[0] (or rely on the pinned side_effect)
corresponds to the alias lookup and that grant_member_role_for_signed_agreement
received that alias via mock_grant_role.call_args.kwargs["discord_user_id"];
reference the mock_api object, its request method, and
DocusealAgreementProcessor.process_agreement when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7be8b651-d81b-4def-bb25-e8e0fd3e5727
📒 Files selected for processing (2)
apps/worker/src/five08/worker/crm/docuseal_processor.pytests/unit/test_docuseal_processor.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except Exception: | ||
| member = None |
There was a problem hiding this comment.
guild.fetch_member() exceptions are currently swallowed and treated as if the member simply isn't in the guild, which can misclassify Discord API failures (e.g., Forbidden / transient HTTPException) as a 404 member_not_in_guild. Handle the known discord.py exceptions explicitly (NotFound vs Forbidden vs HTTPException) and return an appropriate status/error so callers can distinguish real absence from operational failures.
| except Exception: | |
| member = None | |
| except discord.NotFound: | |
| # User is not a member of this guild; handled below as 404. | |
| member = None | |
| except discord.Forbidden: | |
| logger.warning( | |
| "Bot lacks permission to fetch member %s in guild %s", | |
| discord_user_id, | |
| guild.id, | |
| ) | |
| return { | |
| "error": "fetch_member_forbidden", | |
| "guild_id": str(guild.id), | |
| "discord_user_id": str(payload.discord_user_id), | |
| }, 403 | |
| except discord.HTTPException as exc: | |
| logger.error( | |
| "Discord HTTPException while fetching member %s in guild %s: %s", | |
| discord_user_id, | |
| guild.id, | |
| exc, | |
| ) | |
| return { | |
| "error": "discord_api_error", | |
| "guild_id": str(guild.id), | |
| "discord_user_id": str(payload.discord_user_id), | |
| }, 503 |
There was a problem hiding this comment.
This is a Codex generated review. Please validate.
Addressed in 094974a. Please validate that InternalAPIRoutes._grant_member_role now distinguishes discord.NotFound, discord.Forbidden, and discord.HTTPException, and that transient or permission failures no longer fall through to the member_not_in_guild 404 path.
| response = requests.request( | ||
| method.upper(), | ||
| url, | ||
| headers=headers, | ||
| json=payload, | ||
| timeout=self.timeout_seconds, | ||
| ) |
There was a problem hiding this comment.
DiscordBotClient.request() accepts payload: ... | None, but still passes json=payload to requests.request(). When payload is None, this sends a JSON null body, which is surprising and can break endpoints that expect an object or no body. Consider only passing the json argument when a payload is provided.
| response = requests.request( | |
| method.upper(), | |
| url, | |
| headers=headers, | |
| json=payload, | |
| timeout=self.timeout_seconds, | |
| ) | |
| request_kwargs: dict[str, Any] = { | |
| "method": method.upper(), | |
| "url": url, | |
| "headers": headers, | |
| "timeout": self.timeout_seconds, | |
| } | |
| if payload is not None: | |
| request_kwargs["json"] = payload | |
| response = requests.request(**request_kwargs) |
There was a problem hiding this comment.
This is a Codex generated review. Please validate.
Addressed in 094974a. Please validate that DiscordBotClient.request() now omits the json argument when payload is None, and that the new unit test covers the no-body request shape.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/test_docuseal_processor.py (1)
26-35: Pindiscord_bot_internal_base_urlin these unit tests.These cases all rely on the ambient default for
settings.discord_bot_internal_base_url. If that default or the CI env changes, several tests will start returning"bot_endpoint_not_configured"instead of the branch you're trying to exercise, without any regression inDocusealAgreementProcessor.Example tweak
with ( patch("five08.worker.crm.docuseal_processor.EspoClient", return_value=mock_api), + patch( + "five08.worker.crm.docuseal_processor.settings.discord_bot_internal_base_url", + "http://discord_bot:3000", + ), patch( "five08.worker.crm.docuseal_processor.settings.api_shared_secret", "top-secret", ),Also applies to: 76-80, 105-118, 145-154, 194-203, 233-242, 357-366
🤖 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 26 - 35, Tests rely on the ambient settings.discord_bot_internal_base_url which can change CI/defaults and cause branches to return "bot_endpoint_not_configured"; update each affected test block (the patch(...) calls in tests/unit/test_docuseal_processor.py that set EspoClient, settings.api_shared_secret, grant_member_role_for_signed_agreement, etc.) to also patch "five08.worker.crm.docuseal_processor.settings.discord_bot_internal_base_url" to a fixed URL (e.g. "https://internal-bot.local") so the DocusealAgreementProcessor code path that requires a configured bot endpoint is exercised deterministically; apply the same patch to all listed ranges (around lines 26-35, 76-80, 105-118, 145-154, 194-203, 233-242, 357-366) where discord_bot_internal_base_url is relied upon.
🤖 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/discord_bot/src/five08/discord_bot/utils/internal_api.py`:
- Around line 92-103: The current member lookup swallows all exceptions from
guild.fetch_member and always returns a 404 "member_not_in_guild"; change this
to catch and handle discord.NotFound (return the 404 with the
"member_not_in_guild" payload referencing guild.id and payload.discord_user_id),
catch discord.Forbidden (return a 403 indicating lack of permission), catch
discord.HTTPException and other transient API errors (return a 502/temporary
failure), and only treat a genuine NotFound as the 404 case; update the error
responses where guild.get_member and guild.fetch_member are used so transient
failures are not converted into permanent "member_not_in_guild" misses.
---
Nitpick comments:
In `@tests/unit/test_docuseal_processor.py`:
- Around line 26-35: Tests rely on the ambient
settings.discord_bot_internal_base_url which can change CI/defaults and cause
branches to return "bot_endpoint_not_configured"; update each affected test
block (the patch(...) calls in tests/unit/test_docuseal_processor.py that set
EspoClient, settings.api_shared_secret, grant_member_role_for_signed_agreement,
etc.) to also patch
"five08.worker.crm.docuseal_processor.settings.discord_bot_internal_base_url" to
a fixed URL (e.g. "https://internal-bot.local") so the
DocusealAgreementProcessor code path that requires a configured bot endpoint is
exercised deterministically; apply the same patch to all listed ranges (around
lines 26-35, 76-80, 105-118, 145-154, 194-203, 233-242, 357-366) where
discord_bot_internal_base_url is relied upon.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5fba8fde-d001-4edf-985d-3f9dbffabdec
📒 Files selected for processing (4)
apps/discord_bot/src/five08/discord_bot/utils/internal_api.pyapps/worker/src/five08/worker/crm/docuseal_processor.pytests/unit/test_docuseal_processor.pytests/unit/test_internal_api.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/test_internal_api.py
Description
Adds the
/send-member-agreementcommand flow for Docuseal-backed member agreement delivery and keeps the existing send path wired to the shared Docuseal client and webhook processor.On Docuseal completion, the worker now marks the CRM agreement timestamp and makes a best-effort call to the Discord bot to grant the
Memberrole when the contact has a linked Discord user.It also refactors the bot HTTP surface so health routes stay separate from authenticated internal automation routes, and renames the guild setting to
DISCORD_SERVER_ID.Related Issue
N/A
How Has This Been Tested?
./scripts/lint.shuv run pytest tests/unit/test_discord_bot_client.py tests/unit/test_docuseal_processor.py tests/unit/test_healthcheck.py tests/unit/test_worker_config.pyuv run pytest tests/unit/test_crm.py -k send_member_agreementuv run pytest tests/unit/test_backend_api.py -k docusealuv run pytest tests/unit/test_healthcheck.py tests/unit/test_internal_api.py tests/unit/test_bot_http.pyuv run pytest tests/unit/test_internal_api.py tests/unit/test_discord_bot_client.py tests/unit/test_docuseal_processor.pyuv run pytest tests/unit/test_internal_api.py tests/unit/test_docuseal_processor.py tests/unit/test_backend_api.py -k "docuseal or auth or discord" tests/unit/test_worker_config.pySummary by CodeRabbit
New Features
Configuration Updates
Documentation
Tests