Delete Campfire lines through their Campfire, and explain Ping refusals

internal/commands/recordings.go

@@ -1,7 +1,10 @@
 package commands
 
 import (
+	"context"
+	"errors"
 	"fmt"
+	"net/http"
 	"strconv"
 	"strings"
 
@@ -329,6 +332,7 @@ A comment's URL (…#__recording_<id>) restores that comment.`,
 // recordings and comments, false for a typed noun such as cards, which acts
 // on the item the link names, as that noun's show does.
 func runRecordingsStatus(cmd *cobra.Command, app *appctx.App, recordingIDStr, newStatus string, followComment bool) error {
+	scope := newFollowUpScope(app)
 	if err := ensureAccount(cmd, app); err != nil {
 		return err
 	}
@@ -363,6 +367,9 @@ func runRecordingsStatus(cmd *cobra.Command, app *appctx.App, recordingIDStr, ne
 	}
 
 	if err != nil {
+		if refusal := pingWriteRefusal(cmd, app, scope, recordingID, err); refusal != nil {
+			return refusal
+		}
 		return convertSDKError(err)
 	}
 
@@ -408,6 +415,7 @@ You can pass either an ID or a Basecamp URL:
 		Args: cobra.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
 			app := appctx.FromContext(cmd.Context())
+			scope := newFollowUpScope(app)
 
 			if err := ensureAccount(cmd, app); err != nil {
 				return err
@@ -436,6 +444,9 @@ You can pass either an ID or a Basecamp URL:
 
 			recording, err := app.Account().Recordings().SetClientVisibility(cmd.Context(), recordingID, isVisible)
 			if err != nil {
+				if refusal := pingWriteRefusal(cmd, app, scope, recordingID, err); refusal != nil {
+					return refusal
+				}
 				return convertSDKError(err)
 			}
 
@@ -467,6 +478,68 @@ You can pass either an ID or a Basecamp URL:
 	return cmd
 }
 
+// pingWriteRefusal explains a 403 that bc3 gave a generic recording route
+// because the recording is in a Ping. bc3 takes no write to a Ping's Campfire
+// or its lines through those routes, but subscribing or unsubscribing yourself
+// and bubbling a line up (bc3#14171): the lines change through their Campfire.
+// Its 403 is empty, the same one a person without permission gets, so on that
+// status the recording is read back once to tell the two apart, as
+// clientsForbiddenError does for a project. The commands it names carry the
+// profile and account this one ran under, from scope, read before the account
+// was resolved. It returns nil when the error is anything else, or the
+// recording isn't in a Ping or can't be read, leaving the caller's own
+// conversion to stand.
+func pingWriteRefusal(cmd *cobra.Command, app *appctx.App, scope followUpScope, recordingID int64, err error) *output.Error {
+	var sdkErr *basecamp.Error
+	if !errors.As(err, &sdkErr) || sdkErr.Code != basecamp.CodeForbidden {
+		return nil
+	}
+	recording, inPing := readPingRecording(cmd.Context(), app, recordingID)
+	if !inPing {
+		return nil
+	}
+
+	scopeArgs := scope.args(app.Config.AccountID)
+	refusal := &output.Error{
+		Code:    output.CodeForbidden,
+		Message: "Basecamp doesn't change a Ping's Campfire through this command",
+		Hint: fmt.Sprintf("In a Ping you change only your own subscription: basecamp subscriptions subscribe %d%s or basecamp subscriptions unsubscribe %d%s",
+			recordingID, scopeArgs, recordingID, scopeArgs),
+		HTTPStatus: sdkErr.HTTPStatus,
+		Cause:      sdkErr,
+	}
+	if strings.HasPrefix(recording.Type, "Chat::Lines::") && recording.Parent != nil {
+		refusal.Message = "Basecamp doesn't change a line in a Ping through this command"
+		refusal.Hint = "A Ping's lines change only through its Campfire. To delete this one: " +
+			deleteLineCmd(cmd, strconv.FormatInt(recordingID, 10), strconv.FormatInt(recording.Parent.ID, 10), strconv.FormatInt(recording.Bucket.ID, 10)) +
+			scopeArgs
+	}
+	return refusal
+}
+
+// pingRecording is what pingWriteRefusal reads of a recording: its type, its
+// parent (for a line, its Campfire), and its bucket.
+type pingRecording struct {
+	Type   string `json:"type"`
+	Parent *struct {
+		ID int64 `json:"id"`
+	} `json:"parent"`
+	Bucket *struct {
+		ID   int64  `json:"id"`
+		Type string `json:"type"`
+	} `json:"bucket"`
+}
+
+// readPingRecording reads a recording and reports whether it is in a Ping,
+// whose bucket is a Circle. A recording that can't be read is not known to be.
+func readPingRecording(ctx context.Context, app *appctx.App, recordingID int64) (recording pingRecording, inPing bool) {
+	resp, err := app.Account().Get(ctx, fmt.Sprintf("/recordings/%d.json", recordingID))
+	if err == nil && resp.StatusCode != http.StatusNoContent && resp.UnmarshalData(&recording) == nil {
+		inPing = recording.Bucket != nil && recording.Bucket.Type == "Circle"
+	}
+	return recording, inPing
+}
+
 // newRecordableTrashCmd creates a trash subcommand for a recordable entity.
 func newRecordableTrashCmd(noun string) *cobra.Command {
 	return &cobra.Command{

internal/commands/recordings_test.go

@@ -5,6 +5,7 @@ import (
 	"context"
 	"errors"
 	"net/http"
+	"strings"
 	"testing"
 
 	"github.com/spf13/cobra"
@@ -307,3 +308,150 @@ func TestRecordingsStatusProjectURLIsRefused(t *testing.T) {
 		})
 	}
 }
+
+// pingGatedWrite is a command that sends a write to a generic recording route
+// bc3 closes to a Ping (bc3#14171), and the request it sends.
+type pingGatedWrite struct {
+	name   string
+	cmd    func() *cobra.Command
+	args   []string
+	method string
+	path   string
+}
+
+// pingGatedWrites are those commands, for recording 111. bc3 answers each of
+// their writes to a Ping with an empty 403.
+var pingGatedWrites = []pingGatedWrite{
+	{"recordings trash", NewRecordingsCmd, []string{"trash", "111"}, http.MethodPut, "/99999/recordings/111/status/trashed.json"},
+	{"recordings archive", NewRecordingsCmd, []string{"archive", "111"}, http.MethodPut, "/99999/recordings/111/status/archived.json"},
+	{"recordings restore", NewRecordingsCmd, []string{"restore", "111"}, http.MethodPut, "/99999/recordings/111/status/active.json"},
+	{"todos trash", NewTodosCmd, []string{"trash", "111"}, http.MethodPut, "/99999/recordings/111/status/trashed.json"},
+	{"recordings visibility", NewRecordingsCmd, []string{"visibility", "111", "--hidden"}, http.MethodPut, "/99999/recordings/111/client_visibility.json"},
+	{"subscriptions add", NewSubscriptionsCmd, []string{"add", "111", "--people", "5"}, http.MethodPut, "/99999/recordings/111/subscription.json"},
+	{"subscriptions remove", NewSubscriptionsCmd, []string{"remove", "111", "--people", "5"}, http.MethodPut, "/99999/recordings/111/subscription.json"},
+	{"spotlight", NewSpotlightCmd, []string{"111"}, http.MethodPost, "/99999/recordings/111/spotlight.json"},
+	{"unspotlight", NewUnspotlightCmd, []string{"111"}, http.MethodDelete, "/99999/recordings/111/spotlight.json"},
+}
+
+// runPingGatedWrite runs one gated write against a 403 and the given read of
+// recording 111, and returns the error it ends in.
+func runPingGatedWrite(t *testing.T, w pingGatedWrite, readStatus int, readBody string) *output.Error {
+	t.Helper()
+	e, _ := runPingGatedWriteIn(t, output.FormatJSON, w, readStatus, readBody)
+	return e
+}
+
+// runPingGatedWriteIn is runPingGatedWrite with the output in format, and
+// returns the app it ran in too. setup, when given, adjusts the app first.
+func runPingGatedWriteIn(t *testing.T, format output.Format, w pingGatedWrite, readStatus int, readBody string, setup ...func(*appctx.App)) (*output.Error, *appctx.App) {
+	t.Helper()
+	app, transport := setupRecordingTestApp(t,
+		stubRoute{method: w.method, path: w.path, status: http.StatusForbidden},
+		stubRoute{method: http.MethodGet, path: "/99999/recordings/111.json", status: readStatus, body: readBody},
+	)
+	app.Output = output.New(output.Options{Format: format, Writer: &bytes.Buffer{}})
+	for _, adjust := range setup {
+		adjust(app)
+	}
+
+	err := executeRecordingCommand(w.cmd(), app, w.args...)
+
+	require.Error(t, err)
+	var e *output.Error
+	require.True(t, errors.As(err, &e), "expected *output.Error, got %T: %v", err, err)
+	assert.Equal(t, output.CodeForbidden, e.Code)
+	assert.Equal(t, http.StatusForbidden, e.HTTPStatus)
+	writes := 0
+	for _, call := range transport.recorded() {
+		if call.Method != http.MethodGet {
+			writes++
+		}
+	}
+	assert.Equal(t, 1, writes, "the refused write is sent once and not retried another way")
+	return e, app
+}
+
+const pingLine = `{"id":111,"type":"Chat::Lines::Text","parent":{"id":222,"type":"Chat::Transcript"},"bucket":{"id":333,"type":"Circle"}}`
+
+// A 403 on a line in a Ping says why and names the command that does delete
+// the line, with its Campfire and its Ping: bc3's 403 is empty, the same one a
+// person without permission gets, so the line is read back to tell them apart.
+// Read as JSON, the command carries --force, without which chat delete refuses
+// to run where no confirmation can be shown.
+func TestRecordingWritesExplainAPingLine(t *testing.T) {
+	for _, w := range pingGatedWrites {
+		t.Run(w.name, func(t *testing.T) {
+			e, app := runPingGatedWriteIn(t, output.FormatJSON, w, http.StatusOK, pingLine)
+
+			assert.Equal(t, "Basecamp doesn't change a line in a Ping through this command", e.Message)
+			assert.Equal(t, "A Ping's lines change only through its Campfire. To delete this one: basecamp chat delete 111 --room 222 --in 333 --force", e.Hint)
+			require.NoError(t, ensureDeleteConfirmable(deleteGuard(app), strings.Contains(e.Hint, "--force")),
+				"the hint %q is refused by the guard it would hit", e.Hint)
+		})
+	}
+}
+
+// Read by a person, the command leaves --force off: chat delete will ask them.
+func TestRecordingWritesExplainAPingLineToAPerson(t *testing.T) {
+	e, _ := runPingGatedWriteIn(t, output.FormatStyled, pingGatedWrites[0], http.StatusOK, pingLine)
+
+	assert.Equal(t, "A Ping's lines change only through its Campfire. To delete this one: basecamp chat delete 111 --room 222 --in 333", e.Hint)
+}
+
+// The commands a refusal names carry the profile and the account it ran under,
+// so pasting one into a fresh shell acts on the same account.
+func TestRecordingWritesNameTheirScopeInAPingRefusal(t *testing.T) {
+	const campfire = `{"id":111,"type":"Chat::Transcript","bucket":{"id":333,"type":"Circle"}}`
+	scoped := func(app *appctx.App) {
+		app.Config.ActiveProfile = "work"
+		app.Config.Sources = map[string]string{"account_id": string(config.SourceFlag)}
+	}
+	for _, w := range pingGatedWrites {
+		t.Run(w.name, func(t *testing.T) {
+			line, _ := runPingGatedWriteIn(t, output.FormatJSON, w, http.StatusOK, pingLine, scoped)
+			assert.Equal(t, "A Ping's lines change only through its Campfire. To delete this one: "+
+				"basecamp chat delete 111 --room 222 --in 333 --force --profile work --account 99999", line.Hint)
+
+			ping, _ := runPingGatedWriteIn(t, output.FormatJSON, w, http.StatusOK, campfire, scoped)
+			assert.Equal(t, "In a Ping you change only your own subscription: "+
+				"basecamp subscriptions subscribe 111 --profile work --account 99999 or "+
+				"basecamp subscriptions unsubscribe 111 --profile work --account 99999", ping.Hint)
+		})
+	}
+}
+
+// A 403 on a Ping's Campfire says that only your own subscription changes there.
+func TestRecordingWritesExplainAPingCampfire(t *testing.T) {
+	const campfire = `{"id":111,"type":"Chat::Transcript","bucket":{"id":333,"type":"Circle"}}`
+	for _, w := range pingGatedWrites {
+		t.Run(w.name, func(t *testing.T) {
+			e := runPingGatedWrite(t, w, http.StatusOK, campfire)
+
+			assert.Equal(t, "Basecamp doesn't change a Ping's Campfire through this command", e.Message)
+			assert.Equal(t, "In a Ping you change only your own subscription: basecamp subscriptions subscribe 111 or basecamp subscriptions unsubscribe 111", e.Hint)
+		})
+	}
+}
+
+// A 403 on anything outside a Ping, or on a recording that can't be read back,
+// is the plain refusal it was.
+func TestRecordingWritesLeaveOtherRefusalsAlone(t *testing.T) {
+	cases := []struct {
+		name, body string
+		status     int
+	}{
+		{"project recording", `{"id":111,"type":"Todo","bucket":{"id":333,"type":"Project"}}`, http.StatusOK},
+		{"project line", `{"id":111,"type":"Chat::Lines::Text","parent":{"id":222},"bucket":{"id":333,"type":"Project"}}`, http.StatusOK},
+		{"unreadable", `{"error":"Not Found"}`, http.StatusNotFound},
+	}
+	for _, c := range cases {
+		for _, w := range pingGatedWrites {
+			t.Run(c.name+" "+w.name, func(t *testing.T) {
+				e := runPingGatedWrite(t, w, c.status, c.body)
+
+				assert.NotContains(t, e.Message, "Ping")
+				assert.NotContains(t, e.Hint, "Ping")
+			})
+		}
+	}
+}

internal/commands/spotlight.go

@@ -47,6 +47,9 @@ nothing. Requires edit access to the project.`,
 
 			recording, err := app.Account().Recordings().Spotlight(cmd.Context(), recordingID)
 			if err != nil {
+				if refusal := pingWriteRefusal(cmd, app, scope, recordingID, err); refusal != nil {
+					return refusal
+				}
 				return spotlightError(err)
 			}
 
@@ -97,6 +100,9 @@ nothing. Requires edit access to the project.`,
 			}
 
 			if err := app.Account().Recordings().Unspotlight(cmd.Context(), recordingID); err != nil {
+				if refusal := pingWriteRefusal(cmd, app, scope, recordingID, err); refusal != nil {
+					return refusal
+				}
 				return convertSDKError(err)
 			}
 

internal/commands/subscriptions.go

@@ -244,6 +244,7 @@ You can pass either an ID or a Basecamp URL:
 
 func runSubscriptionsUpdate(cmd *cobra.Command, args []string, peopleIDs, mode string) error {
 	app := appctx.FromContext(cmd.Context())
+	scope := newFollowUpScope(app)
 
 	if err := ensureAccount(cmd, app); err != nil {
 		return err
@@ -293,6 +294,9 @@ func runSubscriptionsUpdate(cmd *cobra.Command, args []string, peopleIDs, mode s
 
 	subscription, err := app.Account().Subscriptions().Update(cmd.Context(), recordingID, req)
 	if err != nil {
+		if refusal := pingWriteRefusal(cmd, app, scope, recordingID, err); refusal != nil {
+			return refusal
+		}
 		return convertSDKError(err)
 	}
 

internal/tui/workspace/actions.go

@@ -1,12 +1,14 @@
 package workspace
 
 import (
+	"errors"
 	"fmt"
 	"strings"
 
 	tea "charm.land/bubbletea/v2"
 
 	"github.com/basecamp/basecamp-cli/internal/hostutil"
+	"github.com/basecamp/basecamp-cli/internal/tui/workspace/data"
 )
 
 // ScopeRequirement defines what scope context an action needs.
@@ -371,6 +373,18 @@ func DefaultActions() *Registry {
 			hub := session.Hub()
 			ctx := hub.ProjectContext()
 			return trashCmd(func() error {
+				// The scope's RecordingType is a view's label, not the API's
+				// type, so the recording is read to learn whether it is a
+				// Campfire line. A line can't be trashed, only deleted for
+				// good, which the palette doesn't do unasked; t in the line's
+				// view asks first.
+				recordingType, _, err := hub.RecordingKind(ctx, scope.AccountID, scope.ProjectID, scope.RecordingID)
+				if err != nil {
+					return err
+				}
+				if data.IsCampfireLine(recordingType) {
+					return errCampfireLineTrash
+				}
 				return hub.TrashRecording(ctx, scope.AccountID, scope.ProjectID, scope.RecordingID)
 			})
 		},
@@ -389,6 +403,10 @@ func completeCmd(completeFn func() error) tea.Cmd {
 	}
 }
 
+// errCampfireLineTrash is :trash's answer for a Campfire line, which Basecamp
+// deletes rather than trashes.
+var errCampfireLineTrash = errors.New("a Campfire line can't be trashed: press t twice in its view to delete it for good")
+
 // trashCmd builds a tea.Cmd that calls trashFn and returns StatusMsg + NavigateBack on success.
 func trashCmd(trashFn func() error) tea.Cmd {
 	return func() tea.Msg {