Enhance Discord subscription filters and status rule formatting - #2
Conversation
This aligns slash-command raid selection with RaidHub manifest choices, sends typed rule filters to the API, and standardizes subscription status rule display per player/clan entry for clearer UX. Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
Enhances Discord slash-command subscriptions by adding optional raid filter choices sourced from the RaidHub manifest, passing rule filter options through subscribe operations, and standardizing how subscription status rules are rendered.
Changes:
- Add manifest-sourced raid dropdown choices for
/subscribe(player/clan) and thread them into command sync. - Consolidate unsubscribe UX into
/unsubscribesubcommands (delete/player/clan) and centralize common message titles/copy. - Standardize subscription status rendering to include explicit rule tokens per player/clan and add a “Rule Filters” summary field.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_sync_commands.py | Adds unit coverage for raid-choice extraction ordering/format. |
| tests/test_subscription_helpers.py | Updates status embed expectations for new “Rule Filters” field and rule token formatting. |
| tests/test_manifest_commands.py | Updates manifest expectations for new /subscribe options and unified /unsubscribe subcommands. |
| src/sync_commands.py | Fetches RaidHub manifest to populate raid dropdown choices during Discord command sync. |
| src/manifest/schema.py | Adds DTO support for Discord option choices serialization. |
| src/manifest/builders.py | Adds raid filter options (with choices) to /subscribe and restructures /unsubscribe into subcommands. |
| src/manifest/init.py | Plumbs optional raid choice data into manifest generation. |
| src/commands/unsubscribe.py | Routes /unsubscribe by subcommand and unifies message copy; adds richer success embeds. |
| src/commands/subscription_messages.py | Centralizes common subscription/unsubscribe message titles and shared descriptions. |
| src/commands/subscription_helpers.py | Standardizes per-rule token rendering and adds “Rule Filters” summary to status embeds. |
| src/commands/subscription.py | Switches to shared message titles for consistent UX. |
| src/commands/subscribe.py | Accepts and forwards rule filters (fresh/completed/raid) and unifies success/error copy. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await patch_discord_followup_best_effort( | ||
| app_id, | ||
| token, | ||
| error_embed("Player Not Found", "Missing membership id for that player."), | ||
| error_embed( | ||
| PLAYER_NOT_FOUND_TITLE, | ||
| "Missing membership id for that player.", | ||
| ), |
There was a problem hiding this comment.
The "Missing membership id" branch here assumes resolved_id normalization always succeeds, but resolved_id = str(int(str(raw_mid).strip())) just above can raise ValueError before this check runs (e.g., if membershipId is non-numeric/empty). Consider switching to resolve_player_membership_id(...) (or the existing _norm_membership_id) so invalid IDs are handled as a normal control-flow case instead of an exception.
| outcome = "completed" | ||
| try: | ||
| data = interaction.get("data") or {} | ||
| top_opts = data.get("options") or [] | ||
| sub = "" | ||
| if top_opts and isinstance(top_opts[0], dict): | ||
| sub = str(top_opts[0].get("name") or "").strip().lower() | ||
| if not sub: | ||
| sub = "delete" | ||
|
|
||
| if sub == "player": | ||
| await run_unsubscribe_player_deferred(interaction, raidhub, settings) | ||
| return | ||
| if sub == "clan": | ||
| await run_unsubscribe_clan_deferred(interaction, raidhub, settings) | ||
| return |
There was a problem hiding this comment.
run_unsubscribe_deferred delegates to run_unsubscribe_player_deferred / run_unsubscribe_clan_deferred and then returns, but the finally block still records observe_deferred_completion(command="unsubscribe", ...). This will double-count metrics for /unsubscribe player|clan invocations (and may mark the wrapper as completed even if the delegated handler errors). Consider either (a) moving the dispatching logic outside the try/finally metrics block, (b) skipping wrapper metrics when delegating, or (c) recording a single metric with a subcommand dimension instead of recording both wrapper and sub-handler metrics.
| outcome = "completed" | |
| try: | |
| data = interaction.get("data") or {} | |
| top_opts = data.get("options") or [] | |
| sub = "" | |
| if top_opts and isinstance(top_opts[0], dict): | |
| sub = str(top_opts[0].get("name") or "").strip().lower() | |
| if not sub: | |
| sub = "delete" | |
| if sub == "player": | |
| await run_unsubscribe_player_deferred(interaction, raidhub, settings) | |
| return | |
| if sub == "clan": | |
| await run_unsubscribe_clan_deferred(interaction, raidhub, settings) | |
| return | |
| data = interaction.get("data") or {} | |
| top_opts = data.get("options") or [] | |
| sub = "" | |
| if top_opts and isinstance(top_opts[0], dict): | |
| sub = str(top_opts[0].get("name") or "").strip().lower() | |
| if not sub: | |
| sub = "delete" | |
| if sub == "player": | |
| await run_unsubscribe_player_deferred(interaction, raidhub, settings) | |
| return | |
| if sub == "clan": | |
| await run_unsubscribe_clan_deferred(interaction, raidhub, settings) | |
| return | |
| outcome = "completed" | |
| try: |
| raw_mid = prow.get("membershipId") | ||
| resolved_id = str(int(str(raw_mid).strip())) if raw_mid is not None else "" | ||
| if not resolved_id or not resolved_id.isdigit(): |
There was a problem hiding this comment.
resolved_id = str(int(str(raw_mid).strip())) can raise ValueError if membershipId is non-numeric/empty, which bypasses the intended "Missing membership id" handling and falls into the generic exception path. Consider using the existing normalization helper in subscribe_resolution (e.g., _norm_membership_id) or resolve_player_membership_id(...) to safely normalize/validate without throwing.
This avoids wrapper double-counting for delegated unsubscribe subcommands and prevents ValueError paths when membership ids are malformed in subscribe/unsubscribe player flows. Made-with: Cursor
…ode. This keeps `/unsubscribe` as the full remove-all command, restores dedicated `/unsubscribe-player` and `/unsubscribe-clan` commands, and limits `/subscription` to status-only behavior. Made-with: Cursor
This switches `/unsubscribe` to all/player/clan subcommands and updates subscription status/unsubscribe wording to alert-focused language without internal webhook terminology. Made-with: Cursor
Remove temporary raid option wiring from subscribe commands, keep display compatibility for raidIds, and leave explicit TODOs for a future multi-select-friendly design. Made-with: Cursor
Drop unused raid choice construction now that raid filter inputs are deferred, while keeping status formatting compatible with raidIds payloads. Made-with: Cursor
Reuse shared membership-id normalization for unsubscribe player flow so invalid or malformed IDs follow expected user-facing handling instead of exception fallback. Made-with: Cursor
Rename the player search command to search and remove the instance command from the manifest to reduce command clutter. Made-with: Cursor
Summary
/subscriberaid filter choices sourced from RaidHub manifest sync data while keeping user-facing raid namesTest plan
python3 -m unittest tests.test_manifest_commands tests.test_sync_commandsraidhub-discordtest suite in environment withjwtdependency installedMade with Cursor