Open
Conversation
Contributor
Reviewer's GuideRefactors bot error handling into reusable helpers, tightens telemetry serialization types, generalizes view message typing, and introduces a new in-memory, slash-command-driven guild setup/onboarding flow with verification UI and tests, replacing the older guild onboarding module. Sequence diagram for member onboarding and verification flowsequenceDiagram
actor Member
participant DiscordGuild as Discord_Guild
participant Bot as Bot_SetupCog
participant VerifyView as VerifyView
Member->>DiscordGuild: Join guild
DiscordGuild->>Bot: on_member_join(member)
Bot->>Bot: _ensure_setup(guild_id)
alt onboarding disabled or incomplete
Bot-->>DiscordGuild: Return (no onboarding)
else onboarding enabled and configured
Bot->>Bot: _mark_pending(guild_id, member_id)
Bot->>DiscordGuild: Send welcome message with VerifyView
Bot->>VerifyView: Initialize(target_user_id, on_accept, on_timeout_callback)
Bot->>Bot: _schedule_grace_period_check(guild_id, member_id)
opt welcome_dm_enabled
Bot->>Member: Send DM with onboarding hint
end
end
Member->>DiscordGuild: Click Accept Rules button
DiscordGuild->>VerifyView: accept(interaction)
alt interaction.user.id != target_user_id
VerifyView->>Member: Ephemeral error (not your button)
else valid target user
VerifyView->>Bot: _handle_accept(interaction, target_user_id)
Bot->>DiscordGuild: Lookup member and roles
Bot->>DiscordGuild: Add member_role
Bot->>Bot: Update UserOnboardingState to verified
Bot->>Bot: _cancel_grace_task(guild_id, user_id)
Bot->>Member: Ephemeral success message
Bot->>DiscordGuild: Send log message (verified)
VerifyView->>DiscordGuild: Edit message to disable button
VerifyView->>VerifyView: stop()
end
par Grace period enforcement task
Bot->>Bot: _enforce_grace_period(guild_id, user_id)
Bot->>Bot: sleep(grace_period_hours)
Bot->>Bot: Check UserOnboardingState and config
alt auto_kick_unverified and still pending
Bot->>DiscordGuild: Kick member
Bot->>DiscordGuild: Send log message (removed)
Bot->>Bot: Set status to new
else already verified or disabled
Bot-->>Bot: Do nothing
end
and Verification view timeout
VerifyView-->>Bot: on_timeout_callback(user_id)
Bot->>Bot: _mark_timed_out(guild_id, user_id)
Bot->>DiscordGuild: Send log message (timeout)
end
Class diagram for new setup onboarding cog and related schemasclassDiagram
direction LR
class Bot {
+_format_missing_permissions(permissions list~str~) str
+_get_app_command_error_message(error app_commands.AppCommandError) str
+_get_prefix_error_message(error commands.CommandError) str
+on_tree_error(interaction discord.Interaction, error app_commands.AppCommandError) None
+on_command_error(ctx commands.Context, error commands.CommandError) None
}
class Telemetry {
+_serialize_value(value object) JsonValue
}
class JsonPrimitive {
<<type_alias>>
}
class JsonValue {
<<type_alias>>
}
class BaseView {
+message discord.InteractionMessage | discord.Message | None
+on_error(interaction discord.Interaction, error Exception, item ui.Item) None
}
class VerifyView {
+target_user_id int
-_on_accept(interaction discord.Interaction, target_user_id int) Awaitable~None~
-_on_timeout_callback(target_user_id int) Awaitable~None~
+__init__(target_user_id int, on_accept Callable, on_timeout_callback Callable, timeout float) None
+accept(interaction discord.Interaction, _button ui.Button) None
+on_timeout() None
}
class Setup {
+bot commands.Bot
+setup app_commands.Group
-_setup_store dict~int, GuildSetupConfig~
-_user_state_store dict~str, UserOnboardingState~
-_grace_tasks dict~str, asyncio.Task~None~~
+__init__(bot commands.Bot) None
-_state_key(guild_id int, user_id int) str
-_ensure_setup(guild_id int) GuildSetupConfig
-_get_user_state(guild_id int, user_id int) UserOnboardingState
-_cancel_grace_task(guild_id int, user_id int) None
-_schedule_grace_period_check(guild_id int, user_id int) None
-_get_bot_member(guild discord.Guild) discord.Member
-_first_public_text_channel(guild discord.Guild) discord.TextChannel
-_format_role_mentions(guild discord.Guild, role_ids list~int~) str
-_format_channel_mention(guild discord.Guild, channel_id int) str
-_missing_items(config GuildSetupConfig) list~str~
-_build_setup_message(guild discord.Guild) str
-_parse_role_ids(raw str, guild discord.Guild) list~int~
-_send_log_message(guild discord.Guild, config GuildSetupConfig, message str) None
-_mark_pending(guild_id int, user_id int) None
-_mark_timed_out(guild_id int, user_id int) None
-_enforce_grace_period(guild_id int, user_id int) None
-_handle_accept(interaction discord.Interaction, target_user_id int) None
+on_guild_join(guild discord.Guild) None
+on_member_join(member discord.Member) None
+setup_summary(interaction discord.Interaction) None
+setup_roles(interaction discord.Interaction, admin_roles str, moderator_roles str, member_role discord.Role) None
+setup_channels(interaction discord.Interaction, log_channel discord.TextChannel, announcement_channel discord.TextChannel, welcome_channel discord.TextChannel, support_channel discord.TextChannel) None
+setup_onboarding(interaction discord.Interaction, enabled bool, welcome_dm_enabled bool, auto_kick_unverified bool, grace_period_hours int, log_events bool, rules_location str, message str) None
+setup_reset(interaction discord.Interaction) None
}
class GuildSetupConfig {
<<pydantic_model>>
+enabled bool
+admin_role_ids list~int~
+moderator_role_ids list~int~
+log_channel_id int
+announcement_channel_id int
+welcome_channel_id int
+welcome_dm_enabled bool
+auto_kick_unverified bool
+grace_period_hours int
+log_events bool
+support_channel_id int
+rules_location str
+verification_acceptance str
+member_role_id int
+onboarding_message_template str
}
class UserOnboardingState {
<<pydantic_model>>
+status str
+started_at_utc datetime
+completed_at_utc datetime
+attempts int
}
class Onboarding {
<<alias_of_Setup>>
}
class setup_function {
<<module_setup>>
+setup(bot commands.Bot) None
}
VerifyView --|> BaseView
Setup --> GuildSetupConfig : uses
Setup --> UserOnboardingState : uses
Setup --> VerifyView : creates
Onboarding --|> Setup
setup_function --> Setup
Telemetry --> JsonValue
JsonValue --> JsonPrimitive
State diagram for user onboarding lifecyclestateDiagram-v2
[*] --> new
state "new" as new
state "pending" as pending
state "verified" as verified
new --> pending: on_member_join
pending --> verified: accept_rules_button
pending --> new: view_timeout
pending --> new: grace_period_kick
pending --> new: setup_reset
verified --> new: setup_reset
new --> [*]
verified --> [*]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
Telemetry._serialize_value, the new fallbackreturn str(value)changes previous behavior for any values that were already JSON-serializable but not explicitly handled (e.g. some custom types); consider limiting stringification to known-non-JSON-serializable types or documenting this behavioral change to avoid surprising downstream consumers. - In
VerifyView.on_timeout, the buttons are not disabled or the message updated (unlike inaccept), so the UI will continue to look interactive after timeout; consider callingdisable_all_items()and editing the message to reflect the expired state before or after invoking the timeout callback. - In
test_enforce_grace_period_kicks_unverified_member, themonkeypatch.setattrtarget (capy_discord.exts.onboarding.onboarding.asyncio.sleep) does not match the new module path (capy_discord.exts.setup.setup), so the fake sleep may never be applied; update the patch target to the actual module where_enforce_grace_periodresolvesasyncio.sleep.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Telemetry._serialize_value`, the new fallback `return str(value)` changes previous behavior for any values that were already JSON-serializable but not explicitly handled (e.g. some custom types); consider limiting stringification to known-non-JSON-serializable types or documenting this behavioral change to avoid surprising downstream consumers.
- In `VerifyView.on_timeout`, the buttons are not disabled or the message updated (unlike in `accept`), so the UI will continue to look interactive after timeout; consider calling `disable_all_items()` and editing the message to reflect the expired state before or after invoking the timeout callback.
- In `test_enforce_grace_period_kicks_unverified_member`, the `monkeypatch.setattr` target (`capy_discord.exts.onboarding.onboarding.asyncio.sleep`) does not match the new module path (`capy_discord.exts.setup.setup`), so the fake sleep may never be applied; update the patch target to the actual module where `_enforce_grace_period` resolves `asyncio.sleep`.
## Individual Comments
### Comment 1
<location path="capy_discord/exts/core/telemetry.py" line_range="561-564" />
<code_context>
+ type JsonPrimitive = str | int | float | bool | None
+ type JsonValue = JsonPrimitive | list["JsonValue"] | dict[str, "JsonValue"]
+
+ def _serialize_value(self, value: object) -> JsonValue:
"""Convert complex Discord objects to simple serializable types.
</code_context>
<issue_to_address>
**issue (bug_risk):** Serializing all non-discord/collection values via `str(value)` changes types and may break telemetry consumers.
This change will now stringify primitives that previously remained JSON primitives, so downstream may receive values like "true" and "123" instead of `true` and `123`. To avoid changing telemetry semantics while still handling complex objects and keeping the new `JsonValue` typing, consider preserving native primitives first:
```python
if isinstance(value, (str, int, float, bool)) or value is None:
return value
# discord / collection / dict branches...
return str(value)
```
</issue_to_address>
### Comment 2
<location path="tests/capy_discord/exts/test_setup.py" line_range="262" />
<code_context>
+ async def fake_sleep(_seconds: float) -> None:
+ state.started_at_utc = utc_now() - timedelta(hours=2)
+
+ monkeypatch.setattr("capy_discord.exts.onboarding.onboarding.asyncio.sleep", fake_sleep)
+
+ await cog._enforce_grace_period(guild.id, member.id)
</code_context>
<issue_to_address>
**issue (testing):** The monkeypatch target for asyncio.sleep is incorrect, so this test will not behave as intended.
The implementation under test is in `capy_discord.exts.setup.setup`, but this test patches `capy_discord.exts.onboarding.onboarding.asyncio.sleep`, so the `asyncio.sleep` call in `_enforce_grace_period` is never patched. That means the test may actually sleep for up to an hour instead of simulating time passage. Please update the monkeypatch target to the module where `_enforce_grace_period` (and its `utc_now`/`asyncio.sleep` usage) is defined, e.g. `"capy_discord.exts.setup.setup.asyncio.sleep"`, so the grace-period logic is exercised deterministically.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+561
to
564
| def _serialize_value(self, value: object) -> JsonValue: | ||
| """Convert complex Discord objects to simple serializable types. | ||
|
|
||
| Args: |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Serializing all non-discord/collection values via str(value) changes types and may break telemetry consumers.
This change will now stringify primitives that previously remained JSON primitives, so downstream may receive values like "true" and "123" instead of true and 123. To avoid changing telemetry semantics while still handling complex objects and keeping the new JsonValue typing, consider preserving native primitives first:
if isinstance(value, (str, int, float, bool)) or value is None:
return value
# discord / collection / dict branches...
return str(value)| async def fake_sleep(_seconds: float) -> None: | ||
| state.started_at_utc = utc_now() - timedelta(hours=2) | ||
|
|
||
| monkeypatch.setattr("capy_discord.exts.onboarding.onboarding.asyncio.sleep", fake_sleep) |
Contributor
There was a problem hiding this comment.
issue (testing): The monkeypatch target for asyncio.sleep is incorrect, so this test will not behave as intended.
The implementation under test is in capy_discord.exts.setup.setup, but this test patches capy_discord.exts.onboarding.onboarding.asyncio.sleep, so the asyncio.sleep call in _enforce_grace_period is never patched. That means the test may actually sleep for up to an hour instead of simulating time passage. Please update the monkeypatch target to the module where _enforce_grace_period (and its utc_now/asyncio.sleep usage) is defined, e.g. "capy_discord.exts.setup.setup.asyncio.sleep", so the grace-period logic is exercised deterministically.
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.
Changed onboarding to setup + added some additional config