Revert "Feature/capr 52 dm module"#127
Merged
shamikkarkhanis merged 1 commit intodevelopfrom Mar 17, 2026
Merged
Conversation
|
Issue reopened: CAPR-52 dm module |
Contributor
Reviewer's GuideReverts the internal DM service feature and related tooling while introducing a new guild settings cog with in-memory settings, slash commands for managing guild configuration, and basic Pydantic schemas for guild settings storage/forms. Sequence diagram for guild channel settings slash commandsequenceDiagram
actor Admin
participant DiscordClient
participant Bot
participant GuildCog
participant GuildSettingsStore as GuildSettings_store
Admin->>DiscordClient: /guild channels reports=#reports announcements=#announcements
DiscordClient->>Bot: InteractionCreate
Bot->>GuildCog: guild_channels(interaction, reports, announcements, feedback)
GuildCog->>GuildCog: _ensure_settings(guild_id)
GuildCog->>GuildSettingsStore: get(guild_id)
alt settings_missing
GuildSettingsStore-->>GuildCog: None
GuildCog->>GuildSettingsStore: create GuildSettings for guild_id
else settings_exist
GuildSettingsStore-->>GuildCog: GuildSettings
end
GuildCog->>GuildSettingsStore: update reports_channel, announcements_channel, feedback_channel
GuildCog-->>Bot: send ephemeral "Channel settings saved"
Bot-->>DiscordClient: InteractionResponse
DiscordClient-->>Admin: Ephemeral confirmation message
Class diagram for new guild settings cog and schemasclassDiagram
class GuildCog {
- bot: commands.Bot
- log: logging.Logger
- _store: dict[int, GuildSettings]
+ __init__(bot: commands.Bot)
+ _ensure_settings(guild_id: int) GuildSettings
+ guild_channels(interaction: discord.Interaction, reports: discord.TextChannel, announcements: discord.TextChannel, feedback: discord.TextChannel) void
+ guild_channels_clear(interaction: discord.Interaction, target: Literal) void
+ guild_roles(interaction: discord.Interaction, admin: discord.Role, member: discord.Role) void
+ guild_roles_clear(interaction: discord.Interaction, target: Literal) void
+ guild_onboarding(interaction: discord.Interaction, message: str) void
+ guild_summary(interaction: discord.Interaction) void
}
class GuildSettings {
reports_channel: int | None
announcements_channel: int | None
feedback_channel: int | None
admin_role: str | None
member_roles: list[str]
onboarding_welcome: str | None
}
class ChannelSettingsForm {
reports: str
announcements: str
feedback: str
}
class RoleSettingsForm {
admin: str
member: str
}
class AnnouncementChannelForm {
channel: str
}
class FeedbackChannelForm {
channel: str
}
class WelcomeMessageForm {
message: str
}
GuildCog --> GuildSettings : stores_per_guild
GuildCog ..> ChannelSettingsForm : related_form_model
GuildCog ..> RoleSettingsForm : related_form_model
GuildCog ..> AnnouncementChannelForm : related_form_model
GuildCog ..> FeedbackChannelForm : related_form_model
GuildCog ..> WelcomeMessageForm : related_form_model
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:
- There are now two guild-related modules (
capy_discord/exts/guild/guild.pyandcapy_discord/guild/guild.py) that both deal with guild settings; consider consolidating these into a single cog or shared helper to avoid diverging logic and confusion over which one should be used. - The in-memory
guild_settings_storeattached to the bot will be lost on process restart and is shared across all shards/instances; if this is intended only as a temporary store, consider making that explicit in the design or wiring it through a persistence layer so settings survive restarts. - The repeated
interaction.guild is Nonechecks and error responses in eachguild_*command could be factored into a small helper or decorator to keep the cog methods focused on their core behavior and reduce duplication.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There are now two guild-related modules (`capy_discord/exts/guild/guild.py` and `capy_discord/guild/guild.py`) that both deal with guild settings; consider consolidating these into a single cog or shared helper to avoid diverging logic and confusion over which one should be used.
- The in-memory `guild_settings_store` attached to the bot will be lost on process restart and is shared across all shards/instances; if this is intended only as a temporary store, consider making that explicit in the design or wiring it through a persistence layer so settings survive restarts.
- The repeated `interaction.guild is None` checks and error responses in each `guild_*` command could be factored into a small helper or decorator to keep the cog methods focused on their core behavior and reduce duplication.
## Individual Comments
### Comment 1
<location path="capy_discord/guild/guild.py" line_range="38-47" />
<code_context>
+ @guild.command(name="channels", description="Set channel IDs in one line")
</code_context>
<issue_to_address>
**🚨 issue (security):** Guild settings commands are not permission-gated, allowing any member to mutate guild-wide configuration.
The `guild` subcommands (`channels*`, `roles*`, `onboarding`, `summary`) only use `@app_commands.guild_only()` and lack any permission/role checks, so any guild member can modify guild-wide settings. Please restrict these commands, e.g. with `@app_commands.default_permissions(manage_guild=True)` or a role-based check aligned with `admin_role`, while keeping read-only commands like `summary` more broadly accessible if desired.
</issue_to_address>
### Comment 2
<location path="capy_discord/guild/_schemas.py" line_range="23" />
<code_context>
+ member: str = Field(default="", title="Member Role", description="Role ID for general member access")
+
+
+class AnnouncementChannelForm(BaseModel):
+ """Form for setting the announcement channel."""
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider consolidating the similar channel form models and aligning ID types between forms and `GuildSettings` to reduce redundancy and conversion overhead.
You can simplify this by (1) consolidating the very-similar form models and (2) normalizing ID types with `GuildSettings` to avoid future conversion glue.
### 1. Consolidate the single-field channel forms
`AnnouncementChannelForm` and `FeedbackChannelForm` are just single-field variants of the same concept. You can express them with a reusable model instead of two separate Pydantic classes:
```py
from pydantic import BaseModel, Field
class SingleChannelForm(BaseModel):
"""Generic form for configuring a single channel destination."""
channel_id: int | None = Field(
default=None,
title="Channel",
description="Channel ID",
)
# Example specializations via helpers/factories (keeps behavior, reduces models):
def make_announcement_channel_form() -> SingleChannelForm:
return SingleChannelForm.model_construct(
channel_id=None,
__pydantic_fields__={
"channel_id": Field(
default=None,
title="Announcement Channel",
description="Channel ID for global pings",
),
},
)
def make_feedback_channel_form() -> SingleChannelForm:
return SingleChannelForm.model_construct(
channel_id=None,
__pydantic_fields__={
"channel_id": Field(
default=None,
title="Feedback Channel",
description="Channel ID for feedback routing",
),
},
)
```
This keeps the ability to have different titles/descriptions but avoids extra top-level models that look “different” but behave the same.
If you don’t need runtime factories, an even simpler variant is a single `SingleChannelForm` and you pass the label text from the UI layer instead of encoding it into the model.
### 2. Normalize ID types across forms and `GuildSettings`
Right now the forms use `str` for IDs while `GuildSettings` uses `int | None` for channels and `str` for roles. That guarantees extra parsing/branching later.
You can align all channel/role IDs to `int | None` in both the forms and `GuildSettings`:
```py
class ChannelSettingsForm(BaseModel):
"""Form for configuring guild channel destinations."""
reports: int | None = Field(
default=None,
title="Reports Channel",
description="Channel ID for bug reports",
)
announcements: int | None = Field(
default=None,
title="Announcements Channel",
description="Channel ID for announcements",
)
feedback: int | None = Field(
default=None,
title="Feedback Channel",
description="Channel ID for feedback routing",
)
class RoleSettingsForm(BaseModel):
"""Form for configuring guild role scopes."""
admin: int | None = Field(
default=None,
title="Admin Role",
description="Role ID for administrator access",
)
member: int | None = Field(
default=None,
title="Member Role",
description="Role ID for general member access",
)
class GuildSettings(BaseModel):
"""Persisted guild settings (not a form — internal state)."""
reports_channel: int | None = None
announcements_channel: int | None = None
feedback_channel: int | None = None
admin_role: int | None = None
member_roles: list[int] = Field(default_factory=list)
onboarding_welcome: str | None = None
```
This removes the type mismatch between “form” and “internal state”, so future `GuildCog` code can assign directly:
```py
settings.reports_channel = form.reports
settings.admin_role = form.admin
```
instead of carrying conversion/parsing logic or mixed `str`/`int` handling in `guild_summary`.
</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
+38
to
+47
| @guild.command(name="channels", description="Set channel IDs in one line") | ||
| @app_commands.guild_only() | ||
| @app_commands.describe( | ||
| reports="Reports channel", | ||
| announcements="Announcements channel", | ||
| feedback="Feedback channel", | ||
| ) | ||
| async def guild_channels( | ||
| self, | ||
| interaction: discord.Interaction, |
Contributor
There was a problem hiding this comment.
🚨 issue (security): Guild settings commands are not permission-gated, allowing any member to mutate guild-wide configuration.
The guild subcommands (channels*, roles*, onboarding, summary) only use @app_commands.guild_only() and lack any permission/role checks, so any guild member can modify guild-wide settings. Please restrict these commands, e.g. with @app_commands.default_permissions(manage_guild=True) or a role-based check aligned with admin_role, while keeping read-only commands like summary more broadly accessible if desired.
| member: str = Field(default="", title="Member Role", description="Role ID for general member access") | ||
|
|
||
|
|
||
| class AnnouncementChannelForm(BaseModel): |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider consolidating the similar channel form models and aligning ID types between forms and GuildSettings to reduce redundancy and conversion overhead.
You can simplify this by (1) consolidating the very-similar form models and (2) normalizing ID types with GuildSettings to avoid future conversion glue.
1. Consolidate the single-field channel forms
AnnouncementChannelForm and FeedbackChannelForm are just single-field variants of the same concept. You can express them with a reusable model instead of two separate Pydantic classes:
from pydantic import BaseModel, Field
class SingleChannelForm(BaseModel):
"""Generic form for configuring a single channel destination."""
channel_id: int | None = Field(
default=None,
title="Channel",
description="Channel ID",
)
# Example specializations via helpers/factories (keeps behavior, reduces models):
def make_announcement_channel_form() -> SingleChannelForm:
return SingleChannelForm.model_construct(
channel_id=None,
__pydantic_fields__={
"channel_id": Field(
default=None,
title="Announcement Channel",
description="Channel ID for global pings",
),
},
)
def make_feedback_channel_form() -> SingleChannelForm:
return SingleChannelForm.model_construct(
channel_id=None,
__pydantic_fields__={
"channel_id": Field(
default=None,
title="Feedback Channel",
description="Channel ID for feedback routing",
),
},
)This keeps the ability to have different titles/descriptions but avoids extra top-level models that look “different” but behave the same.
If you don’t need runtime factories, an even simpler variant is a single SingleChannelForm and you pass the label text from the UI layer instead of encoding it into the model.
2. Normalize ID types across forms and GuildSettings
Right now the forms use str for IDs while GuildSettings uses int | None for channels and str for roles. That guarantees extra parsing/branching later.
You can align all channel/role IDs to int | None in both the forms and GuildSettings:
class ChannelSettingsForm(BaseModel):
"""Form for configuring guild channel destinations."""
reports: int | None = Field(
default=None,
title="Reports Channel",
description="Channel ID for bug reports",
)
announcements: int | None = Field(
default=None,
title="Announcements Channel",
description="Channel ID for announcements",
)
feedback: int | None = Field(
default=None,
title="Feedback Channel",
description="Channel ID for feedback routing",
)
class RoleSettingsForm(BaseModel):
"""Form for configuring guild role scopes."""
admin: int | None = Field(
default=None,
title="Admin Role",
description="Role ID for administrator access",
)
member: int | None = Field(
default=None,
title="Member Role",
description="Role ID for general member access",
)
class GuildSettings(BaseModel):
"""Persisted guild settings (not a form — internal state)."""
reports_channel: int | None = None
announcements_channel: int | None = None
feedback_channel: int | None = None
admin_role: int | None = None
member_roles: list[int] = Field(default_factory=list)
onboarding_welcome: str | None = NoneThis removes the type mismatch between “form” and “internal state”, so future GuildCog code can assign directly:
settings.reports_channel = form.reports
settings.admin_role = form.admininstead of carrying conversion/parsing logic or mixed str/int handling in guild_summary.
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
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.
Reverts #126
Summary by Sourcery
Remove the internal DM services feature and introduce a new guild settings management cog with in-memory configuration storage and commands for managing guild channels, roles, onboarding message, and settings summary.
New Features:
Enhancements:
Documentation:
Chores: