fix(discord): address code-review findings on wizard adapter (issue #112)
PR Checks / test-and-build (pull_request) Successful in 9m54s
PR Checks / test-and-build (pull_request) Successful in 9m54s
VERDICT from verifier (D:\Projects\Game\docs\review-report.md):
REQUEST_CHANGES — wizard was functionally broken at runtime.
## Critical
C-1. Choice-button customId was missing the 'choice:' segment.
ButtonCustomId emitted 'wizard:btn:<step>:<value>' but the
dispatcher's switch matches parts[1] == 'choice'. Every choice
button (D&D 5e, Pathfinder, Waitlist, Publish, Confirm) fell
into the default branch and showed 'Unknown button'.
Fix: split into 3 customId helpers:
ChoiceButtonCustomId(step, value) -> 'wizard:btn:choice:<step>:<value>'
ControlButtonCustomId(action) -> 'wizard:btn:<action>:1' (back/cancel/skip/create)
ModalTriggerButtonCustomId(modalStep) -> 'wizard:btn:modal:<modalStep>'
Bulk-rewrote all 66 Btn() call sites in DiscordWizardStep.cs.
C-2. "Другое…" modal-trigger buttons were unrouted in dispatcher.
Added 'parts[1] == "modal"' branch that opens the modal via
InteractionCallback.Modal(BuildModal(parts[2], draft.ChatId)).
C-3. DiscordWizardSubmitter was leaking ex.Message from
CreateSessionHandler to the user-visible draft embed. Postgres
exceptions expose schema/constraint names. Replaced with
generic user-facing error; full exception still logged
server-side on the existing catch block.
## I-3 — parser-roundtrip tests (the gap that let C-1/C-2 through)
Added two real behavioural tests (not string-grep) to
DiscordWizardInteractionModuleSourceTests:
- Renderer_And_Dispatcher_Agree_On_Wire_Format
- ControlButtons_Are_Parsed_As_Control_Not_Choice
These mirror NetCord's [ComponentInteraction("wizard")] prefix
strip, run the parser, and assert the dispatcher would route to
the right branch. Catches the entire class of 'renderer and
dispatcher disagree on the wire format' regressions.
## I-6 — BuildResumeRow (cascading fix from C-1)
After C-1, BuildResumeRow's ButtonCustomId('cancel', '1') would
emit the wrong format. Switched to direct format strings
('wizard:btn:cancel:1', 'wizard:btn:resume:continue', etc.) which
match the dispatcher's 'back'/'cancel'/'create'/'resume' cases
directly, not the 'choice' prefix.
## Version sync (3.8.0 -> 3.9.0)
Directory.Build.props: <Version>3.9.0</Version>
compose.yaml: all 3 image tags -> 3.9.0
Version_ShouldBeSynchronizedForDiscordFeatureRelease test now green.
## Stats
build: 0 warnings, 0 errors
format: 0 of 279 files need changes
tests: 583 passed, 2 skipped (pre-existing), 0 failed
files: 7 changed, 226 +, 79 -
This commit is contained in:
@@ -2,6 +2,7 @@ using System;
|
||||
using System.IO;
|
||||
using System.Linq;
|
||||
using System.Reflection;
|
||||
using GmRelay.DiscordBot.Features.Sessions.Wizard;
|
||||
|
||||
namespace GmRelay.Bot.Tests.Discord;
|
||||
|
||||
@@ -177,6 +178,92 @@ public sealed class DiscordWizardInteractionModuleSourceTests
|
||||
Assert.Contains("SelectedValues[0]", source, StringComparison.Ordinal);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Roundtrip the renderer output through the dispatcher's parser to
|
||||
/// prove the wire formats agree. This is a real behavioural test
|
||||
/// (not a string-grep) — it actually constructs the ButtonProperties
|
||||
/// that NetCord would send, strips the [ComponentInteraction("wizard")]
|
||||
/// prefix exactly as NetCord does, and asserts the dispatcher's
|
||||
/// switch would route the click to the right branch. Catches the
|
||||
/// class of "renderer and dispatcher disagree on the wire format"
|
||||
/// regressions that the string-grep tests above cannot detect.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void Renderer_And_Dispatcher_Agree_On_Wire_Format()
|
||||
{
|
||||
// Choice button: dispatcher expects `btn:choice:<step>:<value>`.
|
||||
var choice = DiscordWizardStep.ChoiceButtonCustomId("Type", "single");
|
||||
Assert.Equal("wizard:btn:choice:Type:single", choice);
|
||||
var choiceArgs = StripWizardPrefix(choice);
|
||||
var choiceParts = choiceArgs.Split(':', 4);
|
||||
Assert.Equal("btn", choiceParts[0]);
|
||||
Assert.Equal("choice", choiceParts[1]);
|
||||
Assert.Equal("Type", choiceParts[2]);
|
||||
Assert.Equal("single", choiceParts[3]);
|
||||
|
||||
// Control button: dispatcher expects `btn:<action>:1`.
|
||||
var cancel = DiscordWizardStep.ControlButtonCustomId("cancel");
|
||||
Assert.Equal("wizard:btn:cancel:1", cancel);
|
||||
var cancelArgs = StripWizardPrefix(cancel);
|
||||
var cancelParts = cancelArgs.Split(':', 3);
|
||||
Assert.Equal("btn", cancelParts[0]);
|
||||
Assert.Equal("cancel", cancelParts[1]);
|
||||
|
||||
// Modal trigger: dispatcher expects `btn:modal:<modalStep>`.
|
||||
var modal = DiscordWizardStep.ModalTriggerButtonCustomId("SystemFreeText");
|
||||
Assert.Equal("wizard:btn:modal:SystemFreeText", modal);
|
||||
var modalArgs = StripWizardPrefix(modal);
|
||||
var modalParts = modalArgs.Split(':', 3);
|
||||
Assert.Equal("btn", modalParts[0]);
|
||||
Assert.Equal("modal", modalParts[1]);
|
||||
Assert.Equal("SystemFreeText", modalParts[2]);
|
||||
|
||||
// All customIds must fit Discord's 100-char limit.
|
||||
Assert.All(
|
||||
new[] { choice, cancel, modal },
|
||||
cid => Assert.True(
|
||||
cid.Length <= DiscordWizardStep.MaxCustomIdLength,
|
||||
$"CustomId '{cid}' exceeds 100 chars: {cid.Length}"));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The Create/Back/Cancel/Resume control buttons in the renderer
|
||||
/// (and in BuildResumeRow) must emit the format the dispatcher's
|
||||
/// switch matches directly — NOT the choice-button format. This
|
||||
/// test parses every button's customId and asserts the dispatcher
|
||||
/// would route it to the right branch.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void ControlButtons_Are_Parsed_As_Control_Not_Choice()
|
||||
{
|
||||
// Real customIds the renderer / BuildResumeRow emit for control actions.
|
||||
var controlIds = new[]
|
||||
{
|
||||
DiscordWizardStep.ControlButtonCustomId("back"),
|
||||
DiscordWizardStep.ControlButtonCustomId("cancel"),
|
||||
"wizard:btn:create:1",
|
||||
"wizard:btn:resume:continue",
|
||||
"wizard:btn:resume:restart",
|
||||
};
|
||||
|
||||
foreach (var cid in controlIds)
|
||||
{
|
||||
var parts = StripWizardPrefix(cid).Split(':', 3);
|
||||
Assert.Equal("btn", parts[0]);
|
||||
// The dispatcher's switch matches these as parts[1] == "back"|"cancel"|"create"|"resume".
|
||||
// They must NOT be tagged as "choice" (that would route through the wizard
|
||||
// with a nonsensical step name).
|
||||
Assert.NotEqual("choice", parts[1]);
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>Mirror NetCord's [ComponentInteraction("wizard")] prefix strip.</summary>
|
||||
private static string StripWizardPrefix(string customId)
|
||||
{
|
||||
const string prefix = "wizard:";
|
||||
return customId.StartsWith(prefix, StringComparison.Ordinal) ? customId[prefix.Length..] : customId;
|
||||
}
|
||||
|
||||
private static int CountOccurrences(string haystack, string needle)
|
||||
{
|
||||
if (string.IsNullOrEmpty(needle)) return 0;
|
||||
|
||||
Reference in New Issue
Block a user