Merge pull request 'Music: only remove and move are commands' (#35) from fix/music-only-remove-move into dev

Reviewed-on: #35

docs/commands.md

@@ -35,7 +35,7 @@ Anyone can use these. `/rps`, `/rpsstats` and `/rpsleaderboard` work in servers
 
 | Command | What it does |
 | --- | --- |
-| `/help` | DMs you a command reference |
+| `/help` | DMs you a command reference, or shows it to you in the channel if DMs are off |
 
 ## Leveling
 
@@ -60,14 +60,10 @@ Anyone can use these. `/rps`, `/rpsstats` and `/rpsleaderboard` work in servers
 | `/queue` | | **DJ** role | Shows the queue |
 | `/volume` | `value`* (0 to 150) | **DJ** role | Sets the volume |
 | `/music toggle` | | Manage Server | Turns music on or off in this server |
-| `/music skip` | | **DJ** role | Skips the current song |
-| `/music pause`, `/music resume` | | **DJ** role | Pauses or resumes |
-| `/music stop` | | **DJ** role | Stops and clears the queue |
-| `/music repeat` | `mode`* (off, song, queue) | **DJ** role | Repeats the song, the queue, or nothing |
 | `/music remove` | `position`* (its number in `/queue`) | **DJ** role | Removes a queued song |
 | `/music move` | `from`*, `to`* | **DJ** role | Moves a queued song |
 
-Requires Lavalink. See [Getting started](getting-started.md#optional-music-with-lavalink).
+Pause, skip, repeat and stop are buttons on the now-playing message. Requires Lavalink. See [Getting started](getting-started.md#optional-music-with-lavalink).
 
 ## Meeting rooms
 

docs/features.md

@@ -68,7 +68,7 @@ Lavalink-powered playback. Code: `internal/music`, commands in `internal/command
 
 - `/play` takes a name or a URL, `/queue` shows the queue, `/volume` sets 0 to 150 percent. These need a role called **DJ**.
 - The now-playing message has buttons for playback and repeat. Buttons also check for the DJ role.
-- `/music toggle` (Manage Server) turns music on or off in a server. `/music skip`, `pause`, `resume`, `stop`, `repeat`, `remove` and `move` control playback and the queue (DJ role).
+- `/music toggle` (Manage Server) turns music on or off in a server. `/music remove` and `/music move` edit the queue (DJ role).
 - The queue and now-playing state are kept in the database, and playback can also be controlled by writing to the `music_commands` table. The hosted Velox dashboard uses this (see [Database](database.md)); you never need it, the commands above do the same.
 - Music needs a Lavalink server. Without `LAVALINK_HOST` the rest of the bot works and music does not.
 
@@ -117,4 +117,4 @@ When the bot is added to a server, it applies safe defaults (modules off) and DM
 
 ## Help
 
-`/help` DMs a command reference. The text is hand-written in `internal/commands/help/help.go`, so update it when you add or change a command.
+`/help` DMs a command reference. If the member has DMs off, or Discord refuses the DM, it shows the same help as a message only they can see. The text is hand-written in `internal/commands/help/help.go`, so update it when you add or change a command.

internal/commands/help/help.go

@@ -1,6 +1,7 @@
 package help
 
 import (
+	"fmt"
 	"log"
 	"unicode/utf8"
 
@@ -43,7 +44,7 @@ func fields() []*discordgo.MessageEmbedField {
 		col("Fun",
 			"`/ping` `/joke` `/coinflip`", "`/dice` `/rps` `/rpsstats`", "`/rpsleaderboard`"),
 		col("Music (DJ role)",
-			"`/play` `/queue` `/volume`", "`/music skip` `pause` `resume`", "`/music stop` `repeat`", "`/music remove` `move`", "`/music toggle` (Manage Server)"),
+			"`/play` `/queue` `/volume`", "`/music remove` `move`", "`/music toggle` (Manage Server)"),
 		col("Sessions and time",
 			"`/schedule create`", "`/schedule list`", "`/schedule reschedule`", "`/timezone set` `show`"),
 		col("Tickets",
@@ -97,39 +98,85 @@ func embeds(author *discordgo.MessageEmbedAuthor) []*discordgo.MessageEmbed {
 	return append(out, cur)
 }
 
+// HelpHandler sends the help by direct message. If that isn't possible (the
+// member has DMs off, or shares no server with the bot), it shows the help to
+// them in the channel instead, as a message only they can see.
 func HelpHandler(s *discordgo.Session, i *discordgo.InteractionCreate) {
-	s.InteractionRespond(i.Interaction, &discordgo.InteractionResponse{
-		Type: discordgo.InteractionResponseChannelMessageWithSource,
-		Data: &discordgo.InteractionResponseData{
-			Content: "Check your DMs for help.",
-			Flags:   discordgo.MessageFlagsEphemeral,
-		},
-	})
-
-	userID := ""
-
-	// check if command is run in a guild
-	if i.Member != nil {
-		userID = i.Member.User.ID
-	} else {
-		userID = i.Interaction.User.ID
-	}
-
-	dmChannel, err := s.UserChannelCreate(userID)
-	if err != nil {
-		log.Printf("Error creating DM channel: %v", err)
-		return
-	}
-
 	author := discordgo.MessageEmbedAuthor{
 		Name: "Velox Bot",
 		URL:  "https://gitea.fernandovideira.com/FernandoJVideira/velox-bot",
 	}
+	list := embeds(&author)
 
-	for _, e := range embeds(&author) {
-		if _, err := s.ChannelMessageSendEmbed(dmChannel.ID, e); err != nil {
-			log.Printf("Error sending help: %v", err)
-			return
+	// Used in a DM with the bot, there is nowhere else to send it.
+	if i.GuildID == "" {
+		_ = s.InteractionRespond(i.Interaction, &discordgo.InteractionResponse{
+			Type: discordgo.InteractionResponseChannelMessageWithSource,
+			Data: &discordgo.InteractionResponseData{Embeds: list},
+		})
+		return
+	}
+
+	// Answer first (sending the DM can take a moment), and edit the answer
+	// once we know where the help went.
+	if err := s.InteractionRespond(i.Interaction, &discordgo.InteractionResponse{
+		Type: discordgo.InteractionResponseDeferredChannelMessageWithSource,
+		Data: &discordgo.InteractionResponseData{Flags: discordgo.MessageFlagsEphemeral},
+	}); err != nil {
+		log.Printf("help: cannot answer: %v", err)
+		return
+	}
+
+	userID := ""
+	if i.Member != nil && i.Member.User != nil {
+		userID = i.Member.User.ID
+	} else if i.User != nil {
+		userID = i.User.ID
+	}
+
+	if err := sendByDM(s, userID, list); err != nil {
+		// Typically "Cannot send messages to this user": DMs are off.
+		// The context says where the command came from, which is what to look at
+		// when "no mutual guilds" appears for someone who is in a server with
+		// the bot: a command run through a user install, or in a server the bot
+		// isn't actually a member of, has no shared server for Discord to see.
+		log.Printf("help: DM not possible, showing it here (guild %q, context %d, installs %v, %s): %v",
+			i.GuildID, i.Context, i.AuthorizingIntegrationOwners, botMembership(s, i.GuildID), err)
+		note := "I couldn't DM you (your DMs may be off), so here it is."
+		_, _ = s.InteractionResponseEdit(i.Interaction, &discordgo.WebhookEdit{Content: &note, Embeds: &list})
+		return
+	}
+	done := "Check your DMs for help."
+	_, _ = s.InteractionResponseEdit(i.Interaction, &discordgo.WebhookEdit{Content: &done})
+}
+
+func sendByDM(s *discordgo.Session, userID string, list []*discordgo.MessageEmbed) error {
+	dm, err := s.UserChannelCreate(userID)
+	if err != nil {
+		return err
+	}
+	for _, e := range list {
+		if _, err := s.ChannelMessageSendEmbed(dm.ID, e); err != nil {
+			return err
 		}
 	}
+	return nil
+}
+
+// botMembership says whether the bot is really a member of the server, which
+// is what Discord means by "mutual guilds". A server where the app was added
+// with only the applications.commands scope runs slash commands fine but has
+// no bot user in it.
+func botMembership(s *discordgo.Session, guildID string) string {
+	if s == nil || s.State == nil || s.State.User == nil {
+		return "bot user unknown"
+	}
+	inCache := false
+	if g, err := s.State.Guild(guildID); err == nil && g != nil {
+		inCache = true
+	}
+	if _, err := s.GuildMember(guildID, s.State.User.ID); err != nil {
+		return fmt.Sprintf("bot is NOT a member of that server (in gateway cache: %v): %v", inCache, err)
+	}
+	return fmt.Sprintf("bot is a member of that server (in gateway cache: %v)", inCache)
 }

internal/commands/help/help_test.go

@@ -1,6 +1,9 @@
 package help
 
 import (
+	"io"
+	"net/http"
+	"strings"
 	"testing"
 	"unicode/utf8"
 
@@ -39,6 +42,78 @@ func TestHelpFitsDiscordsLimits(t *testing.T) {
 	}
 }
 
+// fakeDiscord answers the calls /help makes and records what the member was
+// shown. dm decides whether the direct message works.
+type fakeDiscord struct {
+	dm        bool
+	edited    []byte
+	callbacks int
+	sent      int
+}
+
+func (f *fakeDiscord) RoundTrip(r *http.Request) (*http.Response, error) {
+	reply := func(code int, body string) (*http.Response, error) {
+		return &http.Response{StatusCode: code, Body: io.NopCloser(strings.NewReader(body)), Header: http.Header{"Content-Type": {"application/json"}}, Request: r}, nil
+	}
+	switch {
+	case strings.HasSuffix(r.URL.Path, "/callback"):
+		f.callbacks++
+		return reply(204, "")
+	case strings.HasSuffix(r.URL.Path, "/users/@me/channels"):
+		if !f.dm {
+			return reply(403, `{"message":"Cannot send messages to this user","code":50007}`)
+		}
+		return reply(200, `{"id":"900","type":1}`)
+	case strings.Contains(r.URL.Path, "/channels/900/messages"):
+		f.sent++
+		return reply(200, `{"id":"1"}`)
+	case strings.Contains(r.URL.Path, "/messages/@original"):
+		f.edited, _ = io.ReadAll(r.Body)
+		return reply(200, `{"id":"2"}`)
+	}
+	return reply(404, `{"message":"unexpected `+r.URL.Path+`"}`)
+}
+
+func run(t *testing.T, f *fakeDiscord, guild string) {
+	t.Helper()
+	s, err := discordgo.New("Bot x")
+	if err != nil {
+		t.Fatal(err)
+	}
+	s.Client = &http.Client{Transport: f}
+	s.MaxRestRetries = 0
+	i := &discordgo.InteractionCreate{Interaction: &discordgo.Interaction{
+		ID: "1", Token: "tok", AppID: "100", GuildID: guild,
+		Member: &discordgo.Member{User: &discordgo.User{ID: "42"}},
+	}}
+	HelpHandler(s, i)
+}
+
+func TestHelpGoesToDMs(t *testing.T) {
+	f := &fakeDiscord{dm: true}
+	run(t, f, "5")
+	if f.sent == 0 || !strings.Contains(string(f.edited), "Check your DMs") || strings.Contains(string(f.edited), `"embeds"`) {
+		t.Errorf("sent=%d edited=%s", f.sent, f.edited)
+	}
+}
+
+func TestHelpFallsBackToTheChannelWhenDMsAreOff(t *testing.T) {
+	f := &fakeDiscord{dm: false}
+	run(t, f, "5")
+	got := string(f.edited)
+	if f.sent != 0 || !strings.Contains(got, "couldn't DM you") || !strings.Contains(got, `"embeds"`) || !strings.Contains(got, "Velox Bot Help") {
+		t.Errorf("the help should be shown in the reply: sent=%d edited=%s", f.sent, got)
+	}
+}
+
+func TestHelpInADMIsShownDirectly(t *testing.T) {
+	f := &fakeDiscord{dm: true}
+	run(t, f, "")
+	if f.callbacks != 1 || f.sent != 0 || f.edited != nil {
+		t.Errorf("one direct answer, no DM: %+v", f)
+	}
+}
+
 // The help is meant to be wide and short: inline fields, so Discord puts
 // three side by side, and it should fit in one message.
 func TestHelpIsCompactAndInline(t *testing.T) {

internal/commands/music/public/player.go

@@ -10,10 +10,10 @@ import (
 	"github.com/bwmarrin/discordgo"
 )
 
-// The player controls. Everything a dashboard can do to the queue (skip, pause,
-// resume, stop, repeat, remove and move a track) is also a command, so a
-// server never needs one. They need the DJ role, like /play, /queue and
-// /volume. Turning music on or off needs Manage Server.
+// Queue editing. Pausing, skipping, repeating and stopping are buttons on the
+// now-playing message, so the only queue controls that need commands are
+// removing a song and moving one. They need the DJ role, like /play, /queue
+// and /volume. Turning music on or off needs Manage Server.
 
 func positionOption(name, desc string) *discordgo.ApplicationCommandOption {
 	return &discordgo.ApplicationCommandOption{
@@ -27,15 +27,6 @@ func playerOptions() []*discordgo.ApplicationCommandOption {
 		return &discordgo.ApplicationCommandOption{Type: discordgo.ApplicationCommandOptionSubCommand, Name: name, Description: desc, Options: o}
 	}
 	return []*discordgo.ApplicationCommandOption{
-		sub("skip", "Skip the current song (DJ)"),
-		sub("pause", "Pause playback (DJ)"),
-		sub("resume", "Resume playback (DJ)"),
-		sub("stop", "Stop playback and clear the queue (DJ)"),
-		sub("repeat", "Repeat the current song, the whole queue, or nothing (DJ)",
-			&discordgo.ApplicationCommandOption{
-				Type: discordgo.ApplicationCommandOptionString, Name: "mode", Description: "What repeats", Required: true,
-				Choices: []*discordgo.ApplicationCommandOptionChoice{{Name: "off", Value: "off"}, {Name: "song", Value: "song"}, {Name: "queue", Value: "queue"}},
-			}),
 		sub("remove", "Remove a song from the queue (DJ)", positionOption("position", "Its number in /queue")),
 		sub("move", "Move a song to another place in the queue (DJ)",
 			positionOption("from", "Its number in /queue"), positionOption("to", "The number it should have")),
@@ -57,44 +48,19 @@ func title(e *music.TrackEntry) string {
 	return strings.TrimSpace(e.Track.Info.Title)
 }
 
-// handlePlayer runs a player subcommand. The caller has checked that music is
-// on and that the member has the DJ role.
+// handlePlayer runs a queue subcommand. The caller has checked that music is
+// on and that the member has the DJ role. Pause, skip, repeat and stop are
+// buttons on the now-playing message, so only what has no button is a command.
 func handlePlayer(s *discordgo.Session, i *discordgo.InteractionCreate, sub *discordgo.ApplicationCommandInteractionDataOption) {
 	g := i.GuildID
 	args := map[string]*discordgo.ApplicationCommandInteractionDataOption{}
 	for _, o := range sub.Options {
 		args[o.Name] = o
 	}
-	current, _ := music.GetQueue(g)
-	needsPlayer := sub.Name != "repeat" && sub.Name != "remove" && sub.Name != "move"
-	if needsPlayer && current == nil {
-		shared.RespondEphemeral(s, i, "Nothing is playing.")
-		return
-	}
 
 	var err error
 	var msg string
 	switch sub.Name {
-	case "skip":
-		err, msg = music.Skip(g), "Skipped."
-	case "pause":
-		err, msg = music.Pause(g, true), "Paused. `/music resume` continues."
-	case "resume":
-		err, msg = music.Pause(g, false), "Resumed."
-	case "stop":
-		err, msg = music.Stop(g), "Stopped, and the queue is cleared."
-	case "repeat":
-		switch args["mode"].StringValue() {
-		case "song":
-			music.SetRepeat(g, true, false)
-			msg = "The current song repeats."
-		case "queue":
-			music.SetRepeat(g, false, true)
-			msg = "The whole queue repeats."
-		default:
-			music.SetRepeat(g, false, false)
-			msg = "Nothing repeats."
-		}
 	case "remove":
 		n := int(args["position"].IntValue())
 		e, size := queueEntry(g, n)

internal/music/manager.go

@@ -686,23 +686,6 @@ func ToggleRepeatQueue(guildID string) (bool, bool) {
 	return repeatSong, repeatQueue
 }
 
-// RepeatFlags reports whether the current song or the whole queue repeats.
-func RepeatFlags(guildID string) (song, queue bool) { return repeatFlags(guildID) }
-
-// SetRepeat sets the repeat mode directly. At most one of the two applies;
-// asking for both repeats the song.
-func SetRepeat(guildID string, song, queue bool) {
-	if manager == nil {
-		return
-	}
-	gp := getOrCreateGuildPlayer(guildID)
-	manager.mu.Lock()
-	gp.RepeatSong = song
-	gp.RepeatQueue = queue && !song
-	manager.mu.Unlock()
-	syncState(guildID)
-}
-
 func repeatFlags(guildID string) (bool, bool) {
 	if manager == nil {
 		return false, false