mirror of
https://github.com/prosolis/gogobee.git
synced 2026-07-15 16:42:41 +00:00
Review follow-ups L + I: drop dead openParty, unify the menu-index parser
L: openParty had no production callers (invite path uses joinParty -> seatLeader, which seats the leader the same way but reads the owner off the expedition row). Removed it; the tests now seat the leader through a seatLeaderFixture that wraps seatLeader in a transaction. I: promoted parseTemperIndex to parseMenuIndex and routed resolveMagicEquipReply and handleMasterworkEquipReply through it, replacing two hand-inlined copies of the same digit parser. Behaviour-preserving (the parsed flag was redundant with the idx<0 guard); the retry-on-bad-parse disagreement is left as-is since this is a pure dedup. Full plugin suite green. Claude-Session: https://claude.ai/code/session_017mEwUmmS7aQTP2NQXj6rUa
This commit is contained in:
@@ -435,18 +435,8 @@ func (p *AdventurePlugin) handleMasterworkEquipReply(ctx MessageContext, interac
|
||||
return p.SendDM(ctx.Sender, "Equip cancelled.")
|
||||
}
|
||||
|
||||
// Parse number
|
||||
idx := 0
|
||||
for _, c := range reply {
|
||||
if c >= '0' && c <= '9' {
|
||||
idx = idx*10 + int(c-'0')
|
||||
} else {
|
||||
break
|
||||
}
|
||||
}
|
||||
idx-- // 1-indexed to 0-indexed
|
||||
|
||||
if idx < 0 || idx >= len(data.Items) {
|
||||
idx, ok := parseMenuIndex(reply, len(data.Items))
|
||||
if !ok {
|
||||
return p.SendDM(ctx.Sender, "Invalid selection. Reply with a number from the list, or \"cancel\".")
|
||||
}
|
||||
|
||||
|
||||
@@ -140,8 +140,12 @@ func findTemperMaterial(items []AdvItem) (AdvItem, bool) {
|
||||
return AdvItem{}, false
|
||||
}
|
||||
|
||||
// parseTemperIndex reads a leading 1-indexed number and bounds-checks it.
|
||||
func parseTemperIndex(reply string, n int) (int, bool) {
|
||||
// parseMenuIndex reads a leading 1-indexed number off a menu reply and returns
|
||||
// the bounds-checked 0-indexed position. The single parser behind every "reply
|
||||
// with a number from the list" prompt — temper, magic-item equip, masterwork
|
||||
// equip. Leading whitespace and any trailing text after the digits are ignored;
|
||||
// a reply with no leading digit, or one outside [1, n], returns ok=false.
|
||||
func parseMenuIndex(reply string, n int) (int, bool) {
|
||||
idx, parsed := 0, false
|
||||
for _, c := range strings.TrimSpace(reply) {
|
||||
if c < '0' || c > '9' {
|
||||
@@ -214,7 +218,7 @@ func (p *AdventurePlugin) resolveTemperPick(ctx MessageContext, interaction *adv
|
||||
if strings.EqualFold(reply, "cancel") {
|
||||
return p.SendDM(ctx.Sender, "_The forge keeps burning._ Come back when you've decided.")
|
||||
}
|
||||
idx, ok := parseTemperIndex(reply, len(data.Targets))
|
||||
idx, ok := parseMenuIndex(reply, len(data.Targets))
|
||||
if !ok {
|
||||
p.pending.Store(string(ctx.Sender), interaction)
|
||||
return p.SendDM(ctx.Sender, "Reply with a number from the list, or \"cancel\".")
|
||||
|
||||
@@ -346,9 +346,9 @@ func TestParseTemperIndex(t *testing.T) {
|
||||
{"cancel", 3, 0, false},
|
||||
}
|
||||
for _, tc := range tests {
|
||||
got, ok := parseTemperIndex(tc.in, tc.n)
|
||||
got, ok := parseMenuIndex(tc.in, tc.n)
|
||||
if ok != tc.wantOK || (ok && got != tc.want) {
|
||||
t.Errorf("parseTemperIndex(%q, %d) = (%d, %v), want (%d, %v)",
|
||||
t.Errorf("parseMenuIndex(%q, %d) = (%d, %v), want (%d, %v)",
|
||||
tc.in, tc.n, got, ok, tc.want, tc.wantOK)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -115,18 +115,6 @@ func partySize(expeditionID string) (int, error) {
|
||||
return n, nil
|
||||
}
|
||||
|
||||
// openParty seats the leader, converting a solo expedition row into a party of
|
||||
// one. It is idempotent: a second call on the same expedition is a no-op, so
|
||||
// the invite path can call it unconditionally before adding a member.
|
||||
func openParty(expeditionID string, leaderID id.UserID) error {
|
||||
_, err := db.Get().Exec(`
|
||||
INSERT INTO expedition_party (expedition_id, user_id, role)
|
||||
VALUES (?, ?, 'leader')
|
||||
ON CONFLICT (expedition_id, user_id) DO NOTHING`,
|
||||
expeditionID, string(leaderID))
|
||||
return err
|
||||
}
|
||||
|
||||
// joinParty seats a member. It refuses a full roster, a duplicate, and a player
|
||||
// who already leads or rides an expedition of their own — the "one active
|
||||
// expedition per user" rule that startExpedition enforces in code, extended to
|
||||
@@ -171,9 +159,11 @@ func joinParty(expeditionID string, userID id.UserID) error {
|
||||
return tx.Commit()
|
||||
}
|
||||
|
||||
// seatLeader is openParty inside a caller's transaction: it reads the owner off
|
||||
// the expedition row rather than trusting a passed-in id, so the roster's leader
|
||||
// can never disagree with dnd_expedition.user_id.
|
||||
// seatLeader converts a solo expedition row into a party of one, inside a
|
||||
// caller's transaction. It reads the owner off the expedition row rather than
|
||||
// trusting a passed-in id, so the roster's leader can never disagree with
|
||||
// dnd_expedition.user_id. Idempotent (ON CONFLICT DO NOTHING), so the invite
|
||||
// path can call it unconditionally before adding a member.
|
||||
func seatLeader(tx *sql.Tx, expeditionID string) error {
|
||||
var owner string
|
||||
err := tx.QueryRow(
|
||||
|
||||
@@ -22,6 +22,24 @@ func seedExpedition(t *testing.T, expeditionID string, owner id.UserID, status s
|
||||
}
|
||||
}
|
||||
|
||||
// seatLeaderFixture seats the expedition's leader the way the invite path does —
|
||||
// through seatLeader in its own transaction — reading the owner off the row that
|
||||
// seedExpedition already wrote. Replaces the retired openParty helper.
|
||||
func seatLeaderFixture(t *testing.T, expeditionID string) {
|
||||
t.Helper()
|
||||
tx, err := db.Get().Begin()
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if err := seatLeader(tx, expeditionID); err != nil {
|
||||
_ = tx.Rollback()
|
||||
t.Fatalf("seatLeader %s: %v", expeditionID, err)
|
||||
}
|
||||
if err := tx.Commit(); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestParty_SoloExpeditionHasNoRoster(t *testing.T) {
|
||||
setupEmptyTestDB(t)
|
||||
owner := id.UserID("@solo:example.org")
|
||||
@@ -53,10 +71,9 @@ func TestParty_OpenIsIdempotent(t *testing.T) {
|
||||
owner := id.UserID("@lead:example.org")
|
||||
seedExpedition(t, "exp-1", owner, "active")
|
||||
|
||||
// seatLeader is idempotent — a second call on the same expedition is a no-op.
|
||||
for i := 0; i < 2; i++ {
|
||||
if err := openParty("exp-1", owner); err != nil {
|
||||
t.Fatalf("openParty #%d: %v", i+1, err)
|
||||
}
|
||||
seatLeaderFixture(t, "exp-1")
|
||||
}
|
||||
members, err := partyMembers("exp-1")
|
||||
if err != nil {
|
||||
@@ -71,9 +88,7 @@ func TestParty_JoinSeatsMembersLeaderFirst(t *testing.T) {
|
||||
setupEmptyTestDB(t)
|
||||
owner := id.UserID("@lead2:example.org")
|
||||
seedExpedition(t, "exp-2", owner, "active")
|
||||
if err := openParty("exp-2", owner); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
seatLeaderFixture(t, "exp-2")
|
||||
for _, u := range []id.UserID{"@b:x", "@c:x"} {
|
||||
if err := joinParty("exp-2", u); err != nil {
|
||||
t.Fatalf("joinParty %s: %v", u, err)
|
||||
@@ -127,9 +142,7 @@ func TestParty_JoinRefusesFullRoster(t *testing.T) {
|
||||
setupEmptyTestDB(t)
|
||||
owner := id.UserID("@lead3:example.org")
|
||||
seedExpedition(t, "exp-3", owner, "active")
|
||||
if err := openParty("exp-3", owner); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
seatLeaderFixture(t, "exp-3")
|
||||
for _, u := range []id.UserID{"@b:x", "@c:x"} {
|
||||
if err := joinParty("exp-3", u); err != nil {
|
||||
t.Fatal(err)
|
||||
@@ -145,9 +158,7 @@ func TestParty_JoinRefusesDuplicate(t *testing.T) {
|
||||
setupEmptyTestDB(t)
|
||||
owner := id.UserID("@lead4:example.org")
|
||||
seedExpedition(t, "exp-4", owner, "active")
|
||||
if err := openParty("exp-4", owner); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
seatLeaderFixture(t, "exp-4")
|
||||
if err := joinParty("exp-4", "@b:x"); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
@@ -167,12 +178,8 @@ func TestParty_JoinRefusesPlayerBusyElsewhere(t *testing.T) {
|
||||
setupEmptyTestDB(t)
|
||||
seedExpedition(t, "exp-5", "@lead5:example.org", "active")
|
||||
seedExpedition(t, "exp-6", "@lead6:example.org", "active")
|
||||
if err := openParty("exp-5", "@lead5:example.org"); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if err := openParty("exp-6", "@lead6:example.org"); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
seatLeaderFixture(t, "exp-5")
|
||||
seatLeaderFixture(t, "exp-6")
|
||||
|
||||
// Owns their own active expedition.
|
||||
seedExpedition(t, "exp-own", "@busy:example.org", "active")
|
||||
@@ -198,9 +205,7 @@ func TestParty_LeaveRemovesMemberButNotLeader(t *testing.T) {
|
||||
setupEmptyTestDB(t)
|
||||
owner := id.UserID("@lead7:example.org")
|
||||
seedExpedition(t, "exp-7", owner, "active")
|
||||
if err := openParty("exp-7", owner); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
seatLeaderFixture(t, "exp-7")
|
||||
if err := joinParty("exp-7", "@b:x"); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
@@ -217,9 +222,7 @@ func TestParty_LeaveRemovesMemberButNotLeader(t *testing.T) {
|
||||
}
|
||||
// Leaving frees the member for their own next run.
|
||||
seedExpedition(t, "exp-7b", "@lead7b:example.org", "active")
|
||||
if err := openParty("exp-7b", "@lead7b:example.org"); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
seatLeaderFixture(t, "exp-7b")
|
||||
if err := joinParty("exp-7b", "@b:x"); err != nil {
|
||||
t.Errorf("departed member could not rejoin elsewhere: %v", err)
|
||||
}
|
||||
@@ -229,9 +232,7 @@ func TestParty_DisbandFreesEveryone(t *testing.T) {
|
||||
setupEmptyTestDB(t)
|
||||
owner := id.UserID("@lead8:example.org")
|
||||
seedExpedition(t, "exp-8", owner, "complete")
|
||||
if err := openParty("exp-8", owner); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
seatLeaderFixture(t, "exp-8")
|
||||
if err := joinParty("exp-8", "@b:x"); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
@@ -242,9 +243,7 @@ func TestParty_DisbandFreesEveryone(t *testing.T) {
|
||||
t.Errorf("after disband, partySize = %d, want 1 (no rows)", n)
|
||||
}
|
||||
seedExpedition(t, "exp-8b", "@lead8b:example.org", "active")
|
||||
if err := openParty("exp-8b", "@lead8b:example.org"); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
seatLeaderFixture(t, "exp-8b")
|
||||
if err := joinParty("exp-8b", "@b:x"); err != nil {
|
||||
t.Errorf("disbanded member still looks busy: %v", err)
|
||||
}
|
||||
@@ -257,9 +256,7 @@ func TestParty_ActiveExpeditionForResolvesBothRoles(t *testing.T) {
|
||||
owner := id.UserID("@lead9:example.org")
|
||||
member := id.UserID("@member9:example.org")
|
||||
seedExpedition(t, "exp-9", owner, "active")
|
||||
if err := openParty("exp-9", owner); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
seatLeaderFixture(t, "exp-9")
|
||||
if err := joinParty("exp-9", member); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
@@ -579,18 +579,8 @@ func (p *AdventurePlugin) resolveMagicEquipReply(ctx MessageContext, interaction
|
||||
return p.SendDM(ctx.Sender, "Equip cancelled.")
|
||||
}
|
||||
|
||||
idx := 0
|
||||
parsed := false
|
||||
for _, c := range reply {
|
||||
if c >= '0' && c <= '9' {
|
||||
idx = idx*10 + int(c-'0')
|
||||
parsed = true
|
||||
} else {
|
||||
break
|
||||
}
|
||||
}
|
||||
idx-- // 1-indexed → 0-indexed
|
||||
if !parsed || idx < 0 || idx >= len(data.Items) {
|
||||
idx, ok := parseMenuIndex(reply, len(data.Items))
|
||||
if !ok {
|
||||
p.pending.Store(string(ctx.Sender), interaction)
|
||||
return p.SendDM(ctx.Sender, "Invalid selection. Reply with a number from the list, or \"cancel\".")
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user