From 23855ad4996f1af88347168cf3028c6fe1e22021 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 27 Oct 2026 09:40:01 -0500 Subject: [PATCH 1/4] Delete Campfire lines through their Campfire, and explain Ping refusals bc3#14171 makes the generic recording routes answer 403 to any write on a recording in a Ping. The TUI trashed whatever its Detail view, the :trash action and the Hey view held through the generic trash route, so a Campfire line opened from search went there too: in a Ping that is now a 403, and in a project Campfire it moved the line to the trash past the checks the line endpoint makes on who may delete a line and until when. Those three now go through Hub.RemoveRecording, which decides by the API's own recording type: a Chat::Lines::* recording is deleted through Campfires().DeleteLine, anything else is trashed as before. Detail passes the type and parent it loaded; :trash has only the scope's label, so the hub reads the recording first, and writes nothing when it can't tell. The confirmation and status say delete for a line, since that can't be undone. The CLI commands that send a write to one of the gated routes (recordings trash, archive, restore and visibility, the typed nouns' trash, archive and restore, subscriptions add and remove, spotlight and unspotlight) explain the refusal: bc3's 403 is empty, so on that status the recording is read back once, and when it is in a Ping the error says so and names the command that works, chat delete for a line or subscriptions subscribe/unsubscribe for the Ping's Campfire. --- internal/commands/recordings.go | 56 ++++++ internal/commands/recordings_test.go | 101 +++++++++++ internal/commands/spotlight.go | 6 + internal/commands/subscriptions.go | 3 + internal/tui/workspace/actions.go | 22 ++- internal/tui/workspace/actions_test.go | 130 +++++++++++++- internal/tui/workspace/data/hub.go | 69 +++++++- .../workspace/data/remove_recording_test.go | 162 ++++++++++++++++++ internal/tui/workspace/views/detail.go | 58 ++++++- internal/tui/workspace/views/detail_test.go | 142 +++++++++++++++ internal/tui/workspace/views/hey.go | 23 ++- internal/tui/workspace/views/hey_test.go | 47 +++++ skills/basecamp/SKILL.md | 4 + 13 files changed, 801 insertions(+), 22 deletions(-) create mode 100644 internal/tui/workspace/data/remove_recording_test.go diff --git a/internal/commands/recordings.go b/internal/commands/recordings.go index 8be034f14..0ba5244e9 100644 --- a/internal/commands/recordings.go +++ b/internal/commands/recordings.go @@ -1,7 +1,10 @@ package commands import ( + "context" + "errors" "fmt" + "net/http" "strconv" "strings" @@ -363,6 +366,9 @@ func runRecordingsStatus(cmd *cobra.Command, app *appctx.App, recordingIDStr, ne } if err != nil { + if refusal := pingWriteRefusal(cmd.Context(), app, recordingID, err); refusal != nil { + return refusal + } return convertSDKError(err) } @@ -436,6 +442,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.Context(), app, recordingID, err); refusal != nil { + return refusal + } return convertSDKError(err) } @@ -467,6 +476,53 @@ 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. 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(ctx context.Context, app *appctx.App, recordingID int64, err error) *output.Error { + var sdkErr *basecamp.Error + if !errors.As(err, &sdkErr) || sdkErr.Code != basecamp.CodeForbidden { + return nil + } + resp, readErr := app.Account().Get(ctx, fmt.Sprintf("/recordings/%d.json", recordingID)) + if readErr != nil || resp.StatusCode == http.StatusNoContent { + return nil + } + var recording 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"` + } + if resp.UnmarshalData(&recording) != nil || recording.Bucket == nil || recording.Bucket.Type != "Circle" { + return nil + } + + 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 or basecamp subscriptions unsubscribe %d", recordingID, recordingID), + 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 = fmt.Sprintf("A Ping's lines change only through its Campfire. To delete this one: basecamp chat delete %d --room %d --in %d", + recordingID, recording.Parent.ID, recording.Bucket.ID) + } + return refusal +} + // newRecordableTrashCmd creates a trash subcommand for a recordable entity. func newRecordableTrashCmd(noun string) *cobra.Command { return &cobra.Command{ diff --git a/internal/commands/recordings_test.go b/internal/commands/recordings_test.go index d9948b866..0efaa311f 100644 --- a/internal/commands/recordings_test.go +++ b/internal/commands/recordings_test.go @@ -307,3 +307,104 @@ 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() + 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}, + ) + + 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 +} + +// 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. +func TestRecordingWritesExplainAPingLine(t *testing.T) { + const line = `{"id":111,"type":"Chat::Lines::Text","parent":{"id":222,"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, line) + + 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", e.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") + }) + } + } +} diff --git a/internal/commands/spotlight.go b/internal/commands/spotlight.go index 35a6cc011..77e4189c6 100644 --- a/internal/commands/spotlight.go +++ b/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.Context(), app, 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.Context(), app, recordingID, err); refusal != nil { + return refusal + } return convertSDKError(err) } diff --git a/internal/commands/subscriptions.go b/internal/commands/subscriptions.go index cbd6d1ee7..d4dbc3f2e 100644 --- a/internal/commands/subscriptions.go +++ b/internal/commands/subscriptions.go @@ -293,6 +293,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.Context(), app, recordingID, err); refusal != nil { + return refusal + } return convertSDKError(err) } diff --git a/internal/tui/workspace/actions.go b/internal/tui/workspace/actions.go index 6cc5e1412..c87d563dd 100644 --- a/internal/tui/workspace/actions.go +++ b/internal/tui/workspace/actions.go @@ -370,8 +370,11 @@ func DefaultActions() *Registry { scope := session.Scope() hub := session.Hub() ctx := hub.ProjectContext() - return trashCmd(func() error { - return hub.TrashRecording(ctx, scope.AccountID, scope.ProjectID, scope.RecordingID) + return trashCmd(func() (bool, error) { + // The scope's RecordingType is a view's label, not the API's + // type, so the hub reads the recording to learn whether it + // is a Campfire line, which it deletes rather than trashes. + return hub.RemoveRecording(ctx, scope.AccountID, scope.RecordingID, "", 0) }) }, }) @@ -389,14 +392,21 @@ func completeCmd(completeFn func() error) tea.Cmd { } } -// trashCmd builds a tea.Cmd that calls trashFn and returns StatusMsg + NavigateBack on success. -func trashCmd(trashFn func() error) tea.Cmd { +// trashCmd builds a tea.Cmd that calls trashFn and returns StatusMsg + +// NavigateBack on success. trashFn says whether it deleted the recording, as +// it does a Campfire line, rather than trashing it. +func trashCmd(trashFn func() (deleted bool, err error)) tea.Cmd { return func() tea.Msg { - if err := trashFn(); err != nil { + deleted, err := trashFn() + if err != nil { return ErrorMsg{Err: err, Context: "trashing recording"} } + text := "Trashed" + if deleted { + text = "Deleted" + } return tea.BatchMsg{ - func() tea.Msg { return StatusMsg{Text: "Trashed"} }, + func() tea.Msg { return StatusMsg{Text: text} }, func() tea.Msg { return NavigateBackMsg{} }, } } diff --git a/internal/tui/workspace/actions_test.go b/internal/tui/workspace/actions_test.go index f648017c8..bebf6f6a7 100644 --- a/internal/tui/workspace/actions_test.go +++ b/internal/tui/workspace/actions_test.go @@ -1,10 +1,16 @@ package workspace import ( + "context" "fmt" + "io" + "net/http" + "strings" + "sync" "testing" tea "charm.land/bubbletea/v2" + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -299,7 +305,7 @@ func TestCompleteCmd_Error(t *testing.T) { } func TestTrashCmd_Success_ProducesStatusAndNavigateBack(t *testing.T) { - cmd := trashCmd(func() error { return nil }) + cmd := trashCmd(func() (bool, error) { return false, nil }) msg := cmd() batch, ok := msg.(tea.BatchMsg) @@ -317,15 +323,135 @@ func TestTrashCmd_Success_ProducesStatusAndNavigateBack(t *testing.T) { } func TestTrashCmd_Error(t *testing.T) { - cmd := trashCmd(func() error { return fmt.Errorf("boom") }) + cmd := trashCmd(func() (bool, error) { return false, fmt.Errorf("boom") }) msg := cmd() errMsg, ok := msg.(ErrorMsg) require.True(t, ok, "error should produce ErrorMsg") assert.Equal(t, "trashing recording", errMsg.Context) } +// pingLineJSON is a line in a Ping as bc3's recording JSON has it: its parent +// is the Ping's Campfire, and its bucket is the Ping, of type Circle. +const pingLineJSON = `{"id":111,"status":"active","type":"Chat::Lines::Text","title":"Lunch?",` + + `"parent":{"id":222,"title":"Ping","type":"Chat::Transcript"},` + + `"bucket":{"id":333,"name":"Jason Fried","type":"Circle"}}` + +// The palette's :trash on a Campfire line deletes it through its Campfire, +// never through the generic trash route, which bc3 refuses for anything in a +// Ping (bc3#14171). The scope's type is only a label, so the line is read to +// learn what it is: a deep link to a Ping line labels it Campfire::Line, and a +// quick jump not at all. +func TestAction_Trash_CampfireLineDeletesThroughItsCampfire(t *testing.T) { + for _, label := range []string{"Chat::Lines::Text", "Campfire::Line", ""} { + t.Run("label "+label, func(t *testing.T) { + transport := &actionTransport{routes: map[string]actionReply{ + "GET /99999/recordings/111.json": {http.StatusOK, pingLineJSON}, + "DELETE /99999/chats/222/lines/111": {http.StatusNoContent, ""}, + }} + session := actionSession(transport, Scope{AccountID: "99999", ProjectID: 333, RecordingID: 111, RecordingType: label}) + + msg := trashAction(t).Execute(session)() + + assert.Equal(t, []string{"DELETE /99999/chats/222/lines/111"}, transport.writes()) + assert.Equal(t, "Deleted", firstStatus(t, msg)) + }) + } +} + +// Anything else is trashed as before. +func TestAction_Trash_OtherRecordingTrashes(t *testing.T) { + transport := &actionTransport{routes: map[string]actionReply{ + "GET /99999/recordings/100.json": {http.StatusOK, `{"id":100,"type":"Todo","title":"Ship it"}`}, + "PUT /99999/recordings/100/status/trashed.json": {http.StatusNoContent, ""}, + }} + session := actionSession(transport, Scope{AccountID: "99999", ProjectID: 333, RecordingID: 100, RecordingType: "Todo"}) + + msg := trashAction(t).Execute(session)() + + assert.Equal(t, []string{"PUT /99999/recordings/100/status/trashed.json"}, transport.writes()) + assert.Equal(t, "Trashed", firstStatus(t, msg)) +} + // -- helpers -- +type actionReply struct { + status int + body string +} + +// actionTransport answers from a table keyed by "METHOD path" and records +// every request; anything without a route answers 404. +type actionTransport struct { + mu sync.Mutex + routes map[string]actionReply + requests []string +} + +func (at *actionTransport) RoundTrip(req *http.Request) (*http.Response, error) { + call := req.Method + " " + req.URL.Path + at.mu.Lock() + at.requests = append(at.requests, call) + at.mu.Unlock() + reply, ok := at.routes[call] + if !ok { + reply = actionReply{http.StatusNotFound, `{"error":"Not Found"}`} + } + return &http.Response{ + StatusCode: reply.status, + Header: http.Header{"Content-Type": []string{"application/json"}}, + Body: io.NopCloser(strings.NewReader(reply.body)), + Request: req, + }, nil +} + +// writes returns the requests that change something, in order. +func (at *actionTransport) writes() []string { + at.mu.Lock() + defer at.mu.Unlock() + var writes []string + for _, call := range at.requests { + if !strings.HasPrefix(call, http.MethodGet+" ") { + writes = append(writes, call) + } + } + return writes +} + +type actionTestToken struct{} + +func (actionTestToken) AccessToken(context.Context) (string, error) { return "test-token", nil } + +func actionSession(transport http.RoundTripper, scope Scope) *Session { + sdk := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, actionTestToken{}, + basecamp.WithTransport(transport), basecamp.WithMaxRetries(1)) + return NewTestSessionWithClient(sdk, scope) +} + +func trashAction(t *testing.T) Action { + t.Helper() + for _, a := range DefaultActions().All() { + if a.Name == ":trash" { + return a + } + } + require.FailNow(t, "no :trash action") + return Action{} +} + +// firstStatus returns the text of the StatusMsg a result batch starts with, +// failing on an ErrorMsg. +func firstStatus(t *testing.T, msg tea.Msg) string { + t.Helper() + if errMsg, ok := msg.(ErrorMsg); ok { + require.FailNow(t, "unexpected error", "%s: %v", errMsg.Context, errMsg.Err) + } + batch, ok := msg.(tea.BatchMsg) + require.True(t, ok, "expected a BatchMsg, got %T", msg) + status, ok := batch[0]().(StatusMsg) + require.True(t, ok) + return status.Text +} + func actionNames(actions []Action) map[string]bool { m := make(map[string]bool, len(actions)) for _, a := range actions { diff --git a/internal/tui/workspace/data/hub.go b/internal/tui/workspace/data/hub.go index b8efcf1b1..8eab9fead 100644 --- a/internal/tui/workspace/data/hub.go +++ b/internal/tui/workspace/data/hub.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "path/filepath" + "strings" "sync" "time" @@ -964,7 +965,9 @@ func (h *Hub) UncompleteTodo(ctx context.Context, accountID string, projectID, t return client.Todos().Uncomplete(ctx, todoID) } -// TrashRecording moves a recording to the trash. +// TrashRecording moves a recording to the trash. It is for views whose items +// are of a known type that is never a Campfire line; a view that can hold any +// recording uses RemoveRecording. func (h *Hub) TrashRecording(ctx context.Context, accountID string, projectID, recordingID int64) error { client := h.multi.ClientFor(accountID) if client == nil { @@ -973,6 +976,70 @@ func (h *Hub) TrashRecording(ctx context.Context, accountID string, projectID, r return client.Recordings().Trash(ctx, recordingID) } +// IsCampfireLine reports whether a recording type, as the API gives it, is a +// line in a Campfire, in a project or a Ping: one of bc3's Chat::Lines::*. +func IsCampfireLine(recordingType string) bool { + return strings.HasPrefix(recordingType, "Chat::Lines::") +} + +// RemoveRecording takes a recording away the way its type allows, and says +// whether it deleted it rather than trashing it. A Campfire line is deleted +// through its Campfire, which is how Basecamp removes one: bc3 refuses the +// generic trash route for anything in a Ping (bc3#14171), and on a line in a +// project it would skip the line endpoint's checks on who may delete a line +// and until when. Anything else is trashed. +// +// recordingType is the API's type for the recording and campfireID a line's +// Campfire, when the caller has them. A view's scope carries only a label for +// the type ("Todo", "Campfire::Line") or none, so a caller with no more than +// that passes "", and the recording is read to learn its type; a line is read +// too when its Campfire isn't given. A recording that can't be read is left +// alone rather than trashed on a guess. +func (h *Hub) RemoveRecording(ctx context.Context, accountID string, recordingID int64, recordingType string, campfireID int64) (deleted bool, err error) { + client := h.multi.ClientFor(accountID) + if client == nil { + return false, fmt.Errorf("no client for account %s", accountID) + } + if recordingType == "" || (IsCampfireLine(recordingType) && campfireID == 0) { + recordingType, campfireID, err = readRecordingKind(ctx, client, recordingID) + if err != nil { + return false, err + } + } + if !IsCampfireLine(recordingType) { + return false, client.Recordings().Trash(ctx, recordingID) + } + if campfireID == 0 { + return false, fmt.Errorf("line %d names no Campfire to delete it from", recordingID) + } + return true, client.Campfires().DeleteLine(ctx, campfireID, recordingID) +} + +// readRecordingKind reads a recording's API type and its parent's ID, which +// for a Campfire line is its Campfire. +func readRecordingKind(ctx context.Context, client *basecamp.AccountClient, recordingID int64) (recordingType string, parentID int64, err error) { + resp, err := client.Get(ctx, fmt.Sprintf("/recordings/%d.json", recordingID)) + if err != nil { + return "", 0, err + } + var recording struct { + Type string `json:"type"` + Parent *struct { + ID int64 `json:"id"` + } `json:"parent"` + } + if err := resp.UnmarshalData(&recording); err != nil { + return "", 0, fmt.Errorf("reading recording %d: %w", recordingID, err) + } + if recording.Type == "" { + return "", 0, fmt.Errorf("recording %d has no type", recordingID) + } + if recording.Parent != nil { + parentID = recording.Parent.ID + } + return recording.Type, parentID, nil +} + // CreateDocument creates a new document in a vault. func (h *Hub) CreateDocument(ctx context.Context, accountID string, projectID, vaultID int64, title string) error { client := h.multi.ClientFor(accountID) diff --git a/internal/tui/workspace/data/remove_recording_test.go b/internal/tui/workspace/data/remove_recording_test.go new file mode 100644 index 000000000..14e3e1025 --- /dev/null +++ b/internal/tui/workspace/data/remove_recording_test.go @@ -0,0 +1,162 @@ +package data + +import ( + "context" + "io" + "net/http" + "strings" + "sync" + "testing" + + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + lineInPing = `{"id":111,"type":"Chat::Lines::Text","parent":{"id":222,"type":"Chat::Transcript"},"bucket":{"id":333,"type":"Circle"}}` + deleteLine = "DELETE /99999/chats/222/lines/111" + trashLine = "PUT /99999/recordings/111/status/trashed.json" + readLine = "GET /99999/recordings/111.json" +) + +type removeReply struct { + status int + body string +} + +// removeTransport answers from a table keyed by "METHOD path" and records +// every request; anything without a route answers 404. +type removeTransport struct { + mu sync.Mutex + routes map[string]removeReply + requests []string +} + +func (rt *removeTransport) RoundTrip(req *http.Request) (*http.Response, error) { + call := req.Method + " " + req.URL.Path + rt.mu.Lock() + rt.requests = append(rt.requests, call) + rt.mu.Unlock() + reply, ok := rt.routes[call] + if !ok { + reply = removeReply{http.StatusNotFound, `{"error":"Not Found"}`} + } + return &http.Response{ + StatusCode: reply.status, + Header: http.Header{"Content-Type": []string{"application/json"}}, + Body: io.NopCloser(strings.NewReader(reply.body)), + Request: req, + }, nil +} + +func (rt *removeTransport) recorded() []string { + rt.mu.Lock() + defer rt.mu.Unlock() + return append([]string(nil), rt.requests...) +} + +func removeHub(routes map[string]removeReply) (*Hub, *removeTransport) { + transport := &removeTransport{routes: routes} + sdk := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, hubCheckinsTestTokenProvider{}, + basecamp.WithTransport(transport), basecamp.WithMaxRetries(1)) + return NewHub(NewMultiStore(sdk), ""), transport +} + +// A line whose type and Campfire the caller has is deleted from that Campfire +// without another read. +func TestRemoveRecording_KnownLineDeletes(t *testing.T) { + h, transport := removeHub(map[string]removeReply{deleteLine: {http.StatusNoContent, ""}}) + + deleted, err := h.RemoveRecording(context.Background(), "99999", 111, "Chat::Lines::RichText", 222) + + require.NoError(t, err) + assert.True(t, deleted) + assert.Equal(t, []string{deleteLine}, transport.recorded()) +} + +// A line whose Campfire the caller lacks, or a recording whose type it knows +// only by a label or not at all, is read first, and a line found that way is +// deleted, never trashed. +func TestRemoveRecording_ReadsWhatItLacks(t *testing.T) { + for _, recordingType := range []string{"Chat::Lines::Text", ""} { + t.Run("type "+recordingType, func(t *testing.T) { + h, transport := removeHub(map[string]removeReply{ + readLine: {http.StatusOK, lineInPing}, + deleteLine: {http.StatusNoContent, ""}, + }) + + deleted, err := h.RemoveRecording(context.Background(), "99999", 111, recordingType, 0) + + require.NoError(t, err) + assert.True(t, deleted) + assert.Equal(t, []string{readLine, deleteLine}, transport.recorded()) + }) + } +} + +// Anything that isn't a line is trashed: read first when its type is unknown, +// straight away when the caller has it. +func TestRemoveRecording_OtherRecordingsTrash(t *testing.T) { + const trashTodo = "PUT /99999/recordings/100/status/trashed.json" + const readTodo = "GET /99999/recordings/100.json" + cases := []struct { + recordingType string + want []string + }{ + {"Todo", []string{trashTodo}}, + {"", []string{readTodo, trashTodo}}, + } + for _, c := range cases { + t.Run("type "+c.recordingType, func(t *testing.T) { + h, transport := removeHub(map[string]removeReply{ + readTodo: {http.StatusOK, `{"id":100,"type":"Todo","parent":{"id":50,"type":"Todolist"}}`}, + trashTodo: {http.StatusNoContent, ""}, + }) + + deleted, err := h.RemoveRecording(context.Background(), "99999", 100, c.recordingType, 0) + + require.NoError(t, err) + assert.False(t, deleted) + assert.Equal(t, c.want, transport.recorded()) + }) + } +} + +// When the read can't say what the recording is, or a line names no +// Campfire, nothing is written: trashing on a guess is how a line would reach +// the generic route. +func TestRemoveRecording_WritesNothingItCantDecide(t *testing.T) { + cases := []struct { + name string + read removeReply + }{ + {"unreadable", removeReply{http.StatusNotFound, `{"error":"Not Found"}`}}, + {"no content", removeReply{http.StatusNoContent, ""}}, + {"no type", removeReply{http.StatusOK, `{"id":111}`}}, + {"line without a parent", removeReply{http.StatusOK, `{"id":111,"type":"Chat::Lines::Text"}`}}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + h, transport := removeHub(map[string]removeReply{ + readLine: c.read, + deleteLine: {http.StatusNoContent, ""}, + trashLine: {http.StatusNoContent, ""}, + }) + + _, err := h.RemoveRecording(context.Background(), "99999", 111, "", 0) + + require.Error(t, err) + assert.Equal(t, []string{readLine}, transport.recorded()) + }) + } +} + +func TestIsCampfireLine(t *testing.T) { + for _, line := range []string{"Chat::Lines::Text", "Chat::Lines::RichText", "Chat::Lines::Upload"} { + assert.True(t, IsCampfireLine(line), line) + } + for _, other := range []string{"Chat::Transcript", "Campfire::Line", "Chat", "Todo", ""} { + assert.False(t, IsCampfireLine(other), other) + } +} diff --git a/internal/tui/workspace/views/detail.go b/internal/tui/workspace/views/detail.go index 6966338c1..7c6424608 100644 --- a/internal/tui/workspace/views/detail.go +++ b/internal/tui/workspace/views/detail.go @@ -43,6 +43,8 @@ type detailBoost struct { type detailData struct { title string recordType string + apiType string // the API's type, e.g. "Kanban::Card" or "Chat::Lines::Text" + parentID int64 // the parent's ID; for a Campfire line, its Campfire content string // HTML body creator string createdAt time.Time @@ -82,7 +84,10 @@ type detailDueUpdatedMsg struct{ err error } type detailAssignResultMsg struct{ err error } type trashTimeoutMsg struct{} -type trashResultMsg struct{ err error } +type trashResultMsg struct { + deleted bool // a Campfire line, deleted rather than trashed + err error +} type editBodyResultMsg struct{ err error } type commentEditResultMsg struct{ err error } @@ -250,7 +255,7 @@ func (v *Detail) ShortHelp() []key.Binding { key.NewBinding(key.WithKeys("s"), key.WithHelp("s", "subscribe")), key.NewBinding(key.WithKeys("c"), key.WithHelp("c", "comment")), key.NewBinding(key.WithKeys("b"), key.WithHelp("b", "boost")), - key.NewBinding(key.WithKeys("t"), key.WithHelp("t", "trash")), + key.NewBinding(key.WithKeys("t"), key.WithHelp("t", v.trashVerb())), ) if v.data != nil && len(v.data.comments) > 0 { hints = append(hints, @@ -487,9 +492,17 @@ func (v *Detail) Update(msg tea.Msg) (workspace.View, tea.Cmd) { case trashResultMsg: if msg.err != nil { - return v, workspace.ReportError(msg.err, "trashing recording") + action := "trashing recording" + if v.isCampfireLine() { + action = "deleting line" + } + return v, workspace.ReportError(msg.err, action) + } + status := "Trashed" + if msg.deleted { + status = "Deleted" } - return v, tea.Batch(workspace.SetStatus("Trashed", false), workspace.NavigateBack()) + return v, tea.Batch(workspace.SetStatus(status, false), workspace.NavigateBack()) case trashTimeoutMsg: v.trashPending = false @@ -651,8 +664,12 @@ func (v *Detail) handleKey(msg tea.KeyPressMsg) tea.Cmd { return v.trashRecording() } v.trashPending = true + prompt := "Press t again to trash" + if v.isCampfireLine() { + prompt = "Press t again to delete this line for good" + } return tea.Batch( - workspace.SetStatus("Press t again to trash", false), + workspace.SetStatus(prompt, false), v.trashConfirmTimeout(), ) case "]": @@ -699,14 +716,31 @@ func (v *Detail) trashConfirmTimeout() tea.Cmd { }) } +// trashRecording trashes the recording, or deletes it if it is a Campfire +// line, by the type the API gave when it loaded. func (v *Detail) trashRecording() tea.Cmd { scope := v.session.Scope() hub := v.session.Hub() ctx := hub.ProjectContext() + recordingID, apiType, parentID := v.recordingID, v.data.apiType, v.data.parentID return func() tea.Msg { - err := hub.TrashRecording(ctx, scope.AccountID, scope.ProjectID, v.recordingID) - return trashResultMsg{err: err} + deleted, err := hub.RemoveRecording(ctx, scope.AccountID, recordingID, apiType, parentID) + return trashResultMsg{deleted: deleted, err: err} + } +} + +// isCampfireLine reports whether the loaded recording is a line in a Campfire, +// which t deletes for good rather than trashing. +func (v *Detail) isCampfireLine() bool { + return v.data != nil && data.IsCampfireLine(v.data.apiType) +} + +// trashVerb is what t does to the loaded recording. +func (v *Detail) trashVerb() string { + if v.isCampfireLine() { + return "delete" } + return "trash" } func (v *Detail) goToProject() tea.Cmd { @@ -1398,6 +1432,7 @@ func (v *Detail) fetchDetail() tea.Cmd { data = detailData{ title: todo.Content, recordType: "Todo", + apiType: "Todo", content: todo.Description, creator: creator, createdAt: todo.CreatedAt, @@ -1424,6 +1459,7 @@ func (v *Detail) fetchDetail() tea.Cmd { data = detailData{ title: msg.Subject, recordType: "Message", + apiType: "Message", content: msg.Content, creator: creator, createdAt: msg.CreatedAt, @@ -1448,6 +1484,7 @@ func (v *Detail) fetchDetail() tea.Cmd { data = detailData{ title: card.Title, recordType: "Card", + apiType: "Kanban::Card", content: card.Content, creator: creator, createdAt: card.CreatedAt, @@ -1477,6 +1514,9 @@ func (v *Detail) fetchDetail() tea.Cmd { Creator *struct { Name string `json:"name"` } `json:"creator"` + Parent *struct { + ID int64 `json:"id"` + } `json:"parent"` } if err := resp.UnmarshalData(&generic); err != nil { data = detailData{ @@ -1498,11 +1538,15 @@ func (v *Detail) fetchDetail() tea.Cmd { data = detailData{ title: title, recordType: titleCase(recordingType), + apiType: generic.Type, content: generic.Content, creator: creator, createdAt: generic.CreatedAt, appURL: generic.AppURL, } + if generic.Parent != nil { + data.parentID = generic.Parent.ID + } } } diff --git a/internal/tui/workspace/views/detail_test.go b/internal/tui/workspace/views/detail_test.go index a13b39902..2f156569c 100644 --- a/internal/tui/workspace/views/detail_test.go +++ b/internal/tui/workspace/views/detail_test.go @@ -1,6 +1,10 @@ package views import ( + "io" + "net/http" + "strings" + "sync" "testing" "time" @@ -976,3 +980,141 @@ func TestDetail_FocusMsg_NoData_ShowsSpinner(t *testing.T) { assert.True(t, v.loading, "FocusMsg with no data should set loading to true") } + +// -- Campfire line trash tests -- + +// pingLineJSON is a line in a Ping as bc3's recording JSON has it: its parent +// is the Ping's Campfire, and its bucket is the Ping, of type Circle. +const pingLineJSON = `{"id":111,"status":"active","type":"Chat::Lines::Text","title":"Lunch?",` + + `"content":"Lunch?","created_at":"2026-10-27T10:00:00Z",` + + `"parent":{"id":222,"title":"Ping","type":"Chat::Transcript"},` + + `"bucket":{"id":333,"name":"Jason Fried","type":"Circle"},"creator":{"id":1,"name":"Jason Fried"}}` + +type stubReply struct { + status int + body string +} + +// routeTransport answers from a table keyed by "METHOD path" and records +// every request. Anything without a route answers 404, so a write sent the +// wrong way fails instead of passing. +type routeTransport struct { + mu sync.Mutex + routes map[string]stubReply + requests []string +} + +func (rt *routeTransport) RoundTrip(req *http.Request) (*http.Response, error) { + call := req.Method + " " + req.URL.Path + rt.mu.Lock() + rt.requests = append(rt.requests, call) + rt.mu.Unlock() + reply, ok := rt.routes[call] + if !ok { + reply = stubReply{http.StatusNotFound, `{"error":"Not Found"}`} + } + return &http.Response{ + StatusCode: reply.status, + Header: http.Header{"Content-Type": []string{"application/json"}}, + Body: io.NopCloser(strings.NewReader(reply.body)), + Request: req, + }, nil +} + +// writes returns the requests that change something, in order. +func (rt *routeTransport) writes() []string { + rt.mu.Lock() + defer rt.mu.Unlock() + var writes []string + for _, call := range rt.requests { + if !strings.HasPrefix(call, http.MethodGet+" ") { + writes = append(writes, call) + } + } + return writes +} + +// loadedDetail opens a Detail view the way search, recents or a deep link +// does, with only the scope's label for the type, and loads it through the +// transport. +func loadedDetail(t *testing.T, transport http.RoundTripper, recordingID int64, label string) *Detail { + t.Helper() + sdk := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, starTestToken{}, + basecamp.WithTransport(transport), basecamp.WithMaxRetries(1)) + session := workspace.NewTestSessionWithClient(sdk, workspace.Scope{ + AccountID: "99999", + ProjectID: 333, + RecordingID: recordingID, + RecordingType: label, + }) + v := NewDetail(session, recordingID, label, "", "") + v.Update(v.fetchDetail()()) + require.NotNil(t, v.data, "the detail should load") + return v +} + +// statusText runs the status command a batch starts with. +func statusText(t *testing.T, cmd tea.Cmd) string { + t.Helper() + msg := cmd() + if batch, ok := msg.(tea.BatchMsg); ok { + msg = batch[0]() + } + status, ok := msg.(workspace.StatusMsg) + require.True(t, ok, "expected a StatusMsg, got %T", msg) + return status.Text +} + +// A Campfire line in Detail is deleted through its Campfire, never sent to the +// generic trash route: bc3 refuses that route for anything in a Ping +// (bc3#14171), and in a project Campfire it would skip the line endpoint's own +// checks on who may delete a line and until when. The type is the one bc3 +// returns, whatever label the view was opened with: a search hit carries the +// API's, a deep link to a Ping line says Campfire::Line, and a quick jump none. +func TestDetail_Trash_CampfireLineDeletesThroughItsCampfire(t *testing.T) { + for _, label := range []string{"Chat::Lines::Text", "Campfire::Line", ""} { + t.Run("label "+label, func(t *testing.T) { + transport := &routeTransport{routes: map[string]stubReply{ + "GET /99999/buckets/333/recordings/111.json": {http.StatusOK, pingLineJSON}, + "DELETE /99999/chats/222/lines/111": {http.StatusNoContent, ""}, + }} + v := loadedDetail(t, transport, 111, label) + + armed := v.handleKey(runeKey('t')) + require.NotNil(t, armed) + assert.Equal(t, "Press t again to delete this line for good", statusText(t, armed)) + + cmd := v.handleKey(runeKey('t')) + require.NotNil(t, cmd) + result, ok := cmd().(trashResultMsg) + require.True(t, ok) + require.NoError(t, result.err) + assert.Equal(t, []string{"DELETE /99999/chats/222/lines/111"}, transport.writes()) + + _, done := v.Update(result) + assert.Equal(t, "Deleted", statusText(t, done)) + }) + } +} + +// Anything else Detail opens through the generic recording read is trashed as +// before, decided by the type bc3 returns. +func TestDetail_Trash_OtherRecordingTrashes(t *testing.T) { + transport := &routeTransport{routes: map[string]stubReply{ + "GET /99999/buckets/333/recordings/100.json": {http.StatusOK, + `{"id":100,"type":"Document","title":"Notes","created_at":"2026-10-27T10:00:00Z"}`}, + "PUT /99999/recordings/100/status/trashed.json": {http.StatusNoContent, ""}, + }} + v := loadedDetail(t, transport, 100, "") + + armed := v.handleKey(runeKey('t')) + assert.Equal(t, "Press t again to trash", statusText(t, armed)) + + result, ok := v.handleKey(runeKey('t'))().(trashResultMsg) + require.True(t, ok) + require.NoError(t, result.err) + assert.Equal(t, []string{"PUT /99999/recordings/100/status/trashed.json"}, transport.writes()) + + _, done := v.Update(result) + assert.Equal(t, "Trashed", statusText(t, done)) +} diff --git a/internal/tui/workspace/views/hey.go b/internal/tui/workspace/views/hey.go index bc959f355..86fba5c8c 100644 --- a/internal/tui/workspace/views/hey.go +++ b/internal/tui/workspace/views/hey.go @@ -24,8 +24,9 @@ type heyCompleteResultMsg struct { err error } type heyTrashResultMsg struct { - itemID string - err error + itemID string + deleted bool // a Campfire line, deleted rather than trashed + err error } type heyTrashTimeoutMsg struct{} @@ -189,7 +190,11 @@ func (v *Hey) Update(msg tea.Msg) (workspace.View, tea.Cmd) { if snap.Usable() { v.syncEntries(snap.Data) } - return v, workspace.SetStatus("Trashed", false) + status := "Trashed" + if msg.deleted { + status = "Deleted" + } + return v, workspace.SetStatus(status, false) case heyTrashTimeoutMsg: v.trashPending = false @@ -399,14 +404,20 @@ func (v *Hey) trashSelected() tea.Cmd { ctx := hub.Global().Context() itemID := item.ID return func() tea.Msg { - err := hub.TrashRecording(ctx, meta.AccountID, meta.ProjectID, meta.ID) - return heyTrashResultMsg{itemID: itemID, err: err} + // meta.Type is the API's type, so a Campfire line would be + // deleted through its Campfire rather than trashed. + deleted, err := hub.RemoveRecording(ctx, meta.AccountID, meta.ID, meta.Type, 0) + return heyTrashResultMsg{itemID: itemID, deleted: deleted, err: err} } } v.trashPending = true v.trashPendingID = item.ID + prompt := "Press t again to trash" + if data.IsCampfireLine(meta.Type) { + prompt = "Press t again to delete this line for good" + } return tea.Batch( - workspace.SetStatus("Press t again to trash", false), + workspace.SetStatus(prompt, false), tea.Tick(3*time.Second, func(time.Time) tea.Msg { return heyTrashTimeoutMsg{} }), ) } diff --git a/internal/tui/workspace/views/hey_test.go b/internal/tui/workspace/views/hey_test.go index 1acf3f845..8a13769ec 100644 --- a/internal/tui/workspace/views/hey_test.go +++ b/internal/tui/workspace/views/hey_test.go @@ -2,10 +2,12 @@ package views import ( "context" + "net/http" "testing" "time" tea "charm.land/bubbletea/v2" + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -88,6 +90,51 @@ func TestHey_Trash_DoublePress(t *testing.T) { assert.Equal(t, "acct1:1", result.itemID) } +// heyOverTransport is testHey with an SDK client that talks to transport. +func heyOverTransport(transport http.RoundTripper, entries []data.ActivityEntryInfo) *Hey { + v := testHey(entries) + sdk := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, starTestToken{}, + basecamp.WithTransport(transport), basecamp.WithMaxRetries(1)) + v.session = workspace.NewTestSessionWithClient(sdk, workspace.Scope{AccountID: "99999"}) + return v +} + +// A message, to-do or document in Hey is trashed through the generic route. +func TestHey_Trash_TrashesARecording(t *testing.T) { + transport := &routeTransport{routes: map[string]stubReply{ + "PUT /99999/recordings/2/status/trashed.json": {http.StatusNoContent, ""}, + }} + v := heyOverTransport(transport, []data.ActivityEntryInfo{ + {ID: 2, Title: "Weekly update", Type: "Message", AccountID: "99999", ProjectID: 10, UpdatedAtTS: 100}, + }) + + assert.Equal(t, "Press t again to trash", statusText(t, v.trashSelected())) + result, ok := v.trashSelected()().(heyTrashResultMsg) + require.True(t, ok) + require.NoError(t, result.err) + assert.False(t, result.deleted) + assert.Equal(t, []string{"PUT /99999/recordings/2/status/trashed.json"}, transport.writes()) +} + +// Hey lists no Campfire lines today, but its trash decides by the entry's API +// type, so a line would be deleted through its Campfire, read to find it. +func TestHey_Trash_DeletesACampfireLine(t *testing.T) { + transport := &routeTransport{routes: map[string]stubReply{ + "GET /99999/recordings/111.json": {http.StatusOK, pingLineJSON}, + "DELETE /99999/chats/222/lines/111": {http.StatusNoContent, ""}, + }} + v := heyOverTransport(transport, []data.ActivityEntryInfo{ + {ID: 111, Title: "Lunch?", Type: "Chat::Lines::Text", AccountID: "99999", ProjectID: 333, UpdatedAtTS: 100}, + }) + + assert.Equal(t, "Press t again to delete this line for good", statusText(t, v.trashSelected())) + result, ok := v.trashSelected()().(heyTrashResultMsg) + require.True(t, ok) + require.NoError(t, result.err) + assert.True(t, result.deleted) + assert.Equal(t, []string{"DELETE /99999/chats/222/lines/111"}, transport.writes()) +} + func TestHey_FilterBlocksMutationKeys(t *testing.T) { v := testHey(testHeyEntries) v.list.StartFilter() diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index 3495c9ad7..6ef26c795 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -217,6 +217,10 @@ basecamp pings messages "Jane Smith" --agent - A message URL (`/circles/@`) works with `basecamp show` and `chat line`; edit or delete such a message with `chat update` or `chat delete` on that URL. +- Nothing in a ping changes through the generic recording commands: `recordings + trash`, `archive`, `restore` and `visibility`, `subscriptions add` and + `remove`, and `spotlight` are refused (exit 4). Only your own subscription + changes there, with `subscriptions subscribe` or `subscriptions unsubscribe`. ## Spotlights From b14f621820ed60d6637e364da96bfa404e51f8dd Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 27 Oct 2026 09:56:28 -0500 Subject: [PATCH 2/4] Have :trash refuse a Campfire line, and suggest chat delete for its reader The palette's :trash deleted a Campfire line outright, with none of the confirmation t gives in the line's view, though deleting a line can't be undone. It now reads the recording, and for a line says to press t twice there instead, writing nothing; anything else it trashes as before. The Ping-line refusal named chat delete without --force, which chat delete refuses wherever no confirmation can be shown, so an agent reading JSON could not run it. It now builds the command with deleteLineCmd, which adds --force for machine output and leaves it off for a person who will be asked. --- internal/commands/recordings.go | 50 +++++++++++++++----------- internal/commands/recordings_test.go | 30 +++++++++++++--- internal/commands/spotlight.go | 4 +-- internal/commands/subscriptions.go | 2 +- internal/tui/workspace/actions.go | 38 ++++++++++++-------- internal/tui/workspace/actions_test.go | 39 +++++++++++++------- internal/tui/workspace/data/hub.go | 14 ++++++-- 7 files changed, 121 insertions(+), 56 deletions(-) diff --git a/internal/commands/recordings.go b/internal/commands/recordings.go index 0ba5244e9..d71fe96fd 100644 --- a/internal/commands/recordings.go +++ b/internal/commands/recordings.go @@ -366,7 +366,7 @@ func runRecordingsStatus(cmd *cobra.Command, app *appctx.App, recordingIDStr, ne } if err != nil { - if refusal := pingWriteRefusal(cmd.Context(), app, recordingID, err); refusal != nil { + if refusal := pingWriteRefusal(cmd, app, recordingID, err); refusal != nil { return refusal } return convertSDKError(err) @@ -442,7 +442,7 @@ 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.Context(), app, recordingID, err); refusal != nil { + if refusal := pingWriteRefusal(cmd, app, recordingID, err); refusal != nil { return refusal } return convertSDKError(err) @@ -485,26 +485,13 @@ You can pass either an ID or a Basecamp URL: // clientsForbiddenError does for a project. 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(ctx context.Context, app *appctx.App, recordingID int64, err error) *output.Error { +func pingWriteRefusal(cmd *cobra.Command, app *appctx.App, recordingID int64, err error) *output.Error { var sdkErr *basecamp.Error if !errors.As(err, &sdkErr) || sdkErr.Code != basecamp.CodeForbidden { return nil } - resp, readErr := app.Account().Get(ctx, fmt.Sprintf("/recordings/%d.json", recordingID)) - if readErr != nil || resp.StatusCode == http.StatusNoContent { - return nil - } - var recording 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"` - } - if resp.UnmarshalData(&recording) != nil || recording.Bucket == nil || recording.Bucket.Type != "Circle" { + recording, inPing := readPingRecording(cmd.Context(), app, recordingID) + if !inPing { return nil } @@ -517,12 +504,35 @@ func pingWriteRefusal(ctx context.Context, app *appctx.App, recordingID int64, e } 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 = fmt.Sprintf("A Ping's lines change only through its Campfire. To delete this one: basecamp chat delete %d --room %d --in %d", - recordingID, recording.Parent.ID, recording.Bucket.ID) + 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)) } 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{ diff --git a/internal/commands/recordings_test.go b/internal/commands/recordings_test.go index 0efaa311f..9fdce0664 100644 --- a/internal/commands/recordings_test.go +++ b/internal/commands/recordings_test.go @@ -5,6 +5,7 @@ import ( "context" "errors" "net/http" + "strings" "testing" "github.com/spf13/cobra" @@ -335,11 +336,20 @@ var pingGatedWrites = []pingGatedWrite{ // 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. +func runPingGatedWriteIn(t *testing.T, format output.Format, w pingGatedWrite, readStatus int, readBody string) (*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{}}) err := executeRecordingCommand(w.cmd(), app, w.args...) @@ -355,24 +365,36 @@ func runPingGatedWrite(t *testing.T, w pingGatedWrite, readStatus int, readBody } } assert.Equal(t, 1, writes, "the refused write is sent once and not retried another way") - return e + 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) { - const line = `{"id":111,"type":"Chat::Lines::Text","parent":{"id":222,"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, line) + 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", e.Hint) + 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) +} + // 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"}}` diff --git a/internal/commands/spotlight.go b/internal/commands/spotlight.go index 77e4189c6..40a0a5a0a 100644 --- a/internal/commands/spotlight.go +++ b/internal/commands/spotlight.go @@ -47,7 +47,7 @@ nothing. Requires edit access to the project.`, recording, err := app.Account().Recordings().Spotlight(cmd.Context(), recordingID) if err != nil { - if refusal := pingWriteRefusal(cmd.Context(), app, recordingID, err); refusal != nil { + if refusal := pingWriteRefusal(cmd, app, recordingID, err); refusal != nil { return refusal } return spotlightError(err) @@ -100,7 +100,7 @@ nothing. Requires edit access to the project.`, } if err := app.Account().Recordings().Unspotlight(cmd.Context(), recordingID); err != nil { - if refusal := pingWriteRefusal(cmd.Context(), app, recordingID, err); refusal != nil { + if refusal := pingWriteRefusal(cmd, app, recordingID, err); refusal != nil { return refusal } return convertSDKError(err) diff --git a/internal/commands/subscriptions.go b/internal/commands/subscriptions.go index d4dbc3f2e..8ad84bb09 100644 --- a/internal/commands/subscriptions.go +++ b/internal/commands/subscriptions.go @@ -293,7 +293,7 @@ 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.Context(), app, recordingID, err); refusal != nil { + if refusal := pingWriteRefusal(cmd, app, recordingID, err); refusal != nil { return refusal } return convertSDKError(err) diff --git a/internal/tui/workspace/actions.go b/internal/tui/workspace/actions.go index c87d563dd..48f8a3381 100644 --- a/internal/tui/workspace/actions.go +++ b/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. @@ -370,11 +372,20 @@ func DefaultActions() *Registry { scope := session.Scope() hub := session.Hub() ctx := hub.ProjectContext() - return trashCmd(func() (bool, error) { + return trashCmd(func() error { // The scope's RecordingType is a view's label, not the API's - // type, so the hub reads the recording to learn whether it - // is a Campfire line, which it deletes rather than trashes. - return hub.RemoveRecording(ctx, scope.AccountID, scope.RecordingID, "", 0) + // 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.RecordingID) + if err != nil { + return err + } + if data.IsCampfireLine(recordingType) { + return errCampfireLineTrash + } + return hub.TrashRecording(ctx, scope.AccountID, scope.ProjectID, scope.RecordingID) }) }, }) @@ -392,21 +403,18 @@ func completeCmd(completeFn func() error) tea.Cmd { } } -// trashCmd builds a tea.Cmd that calls trashFn and returns StatusMsg + -// NavigateBack on success. trashFn says whether it deleted the recording, as -// it does a Campfire line, rather than trashing it. -func trashCmd(trashFn func() (deleted bool, err 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 { - deleted, err := trashFn() - if err != nil { + if err := trashFn(); err != nil { return ErrorMsg{Err: err, Context: "trashing recording"} } - text := "Trashed" - if deleted { - text = "Deleted" - } return tea.BatchMsg{ - func() tea.Msg { return StatusMsg{Text: text} }, + func() tea.Msg { return StatusMsg{Text: "Trashed"} }, func() tea.Msg { return NavigateBackMsg{} }, } } diff --git a/internal/tui/workspace/actions_test.go b/internal/tui/workspace/actions_test.go index bebf6f6a7..ef9ed21d9 100644 --- a/internal/tui/workspace/actions_test.go +++ b/internal/tui/workspace/actions_test.go @@ -305,7 +305,7 @@ func TestCompleteCmd_Error(t *testing.T) { } func TestTrashCmd_Success_ProducesStatusAndNavigateBack(t *testing.T) { - cmd := trashCmd(func() (bool, error) { return false, nil }) + cmd := trashCmd(func() error { return nil }) msg := cmd() batch, ok := msg.(tea.BatchMsg) @@ -323,7 +323,7 @@ func TestTrashCmd_Success_ProducesStatusAndNavigateBack(t *testing.T) { } func TestTrashCmd_Error(t *testing.T) { - cmd := trashCmd(func() (bool, error) { return false, fmt.Errorf("boom") }) + cmd := trashCmd(func() error { return fmt.Errorf("boom") }) msg := cmd() errMsg, ok := msg.(ErrorMsg) require.True(t, ok, "error should produce ErrorMsg") @@ -336,28 +336,43 @@ const pingLineJSON = `{"id":111,"status":"active","type":"Chat::Lines::Text","ti `"parent":{"id":222,"title":"Ping","type":"Chat::Transcript"},` + `"bucket":{"id":333,"name":"Jason Fried","type":"Circle"}}` -// The palette's :trash on a Campfire line deletes it through its Campfire, -// never through the generic trash route, which bc3 refuses for anything in a -// Ping (bc3#14171). The scope's type is only a label, so the line is read to -// learn what it is: a deep link to a Ping line labels it Campfire::Line, and a -// quick jump not at all. -func TestAction_Trash_CampfireLineDeletesThroughItsCampfire(t *testing.T) { +// The palette's :trash never sends a Campfire line to the generic trash route, +// which bc3 refuses for anything in a Ping (bc3#14171) and which in a project +// would skip the line's own delete checks. Nor does it delete the line, which +// can't be undone and which the palette would do without asking: it says to +// press t twice in the line's view, which asks first. The scope's type is only +// a label, so the line is read to learn what it is: a deep link to a Ping line +// labels it Campfire::Line, and a quick jump not at all. +func TestAction_Trash_RefusesACampfireLine(t *testing.T) { for _, label := range []string{"Chat::Lines::Text", "Campfire::Line", ""} { t.Run("label "+label, func(t *testing.T) { transport := &actionTransport{routes: map[string]actionReply{ - "GET /99999/recordings/111.json": {http.StatusOK, pingLineJSON}, - "DELETE /99999/chats/222/lines/111": {http.StatusNoContent, ""}, + "GET /99999/recordings/111.json": {http.StatusOK, pingLineJSON}, }} session := actionSession(transport, Scope{AccountID: "99999", ProjectID: 333, RecordingID: 111, RecordingType: label}) msg := trashAction(t).Execute(session)() - assert.Equal(t, []string{"DELETE /99999/chats/222/lines/111"}, transport.writes()) - assert.Equal(t, "Deleted", firstStatus(t, msg)) + assert.Empty(t, transport.writes()) + errMsg, ok := msg.(ErrorMsg) + require.True(t, ok, "expected an ErrorMsg, got %T", msg) + assert.ErrorIs(t, errMsg.Err, errCampfireLineTrash) }) } } +// When the recording can't be read, :trash writes nothing rather than guess. +func TestAction_Trash_WritesNothingWhenItCantTell(t *testing.T) { + transport := &actionTransport{routes: map[string]actionReply{}} + session := actionSession(transport, Scope{AccountID: "99999", ProjectID: 333, RecordingID: 111, RecordingType: "Message"}) + + msg := trashAction(t).Execute(session)() + + assert.Empty(t, transport.writes()) + _, ok := msg.(ErrorMsg) + assert.True(t, ok, "expected an ErrorMsg, got %T", msg) +} + // Anything else is trashed as before. func TestAction_Trash_OtherRecordingTrashes(t *testing.T) { transport := &actionTransport{routes: map[string]actionReply{ diff --git a/internal/tui/workspace/data/hub.go b/internal/tui/workspace/data/hub.go index 8eab9fead..c155053c4 100644 --- a/internal/tui/workspace/data/hub.go +++ b/internal/tui/workspace/data/hub.go @@ -1015,8 +1015,18 @@ func (h *Hub) RemoveRecording(ctx context.Context, accountID string, recordingID return true, client.Campfires().DeleteLine(ctx, campfireID, recordingID) } -// readRecordingKind reads a recording's API type and its parent's ID, which -// for a Campfire line is its Campfire. +// RecordingKind reads a recording's API type and its parent's ID, which for a +// Campfire line is its Campfire, for a caller that knows the recording only by +// a view's label. +func (h *Hub) RecordingKind(ctx context.Context, accountID string, recordingID int64) (recordingType string, parentID int64, err error) { + client := h.multi.ClientFor(accountID) + if client == nil { + return "", 0, fmt.Errorf("no client for account %s", accountID) + } + return readRecordingKind(ctx, client, recordingID) +} + +// readRecordingKind reads a recording's API type and its parent's ID. func readRecordingKind(ctx context.Context, client *basecamp.AccountClient, recordingID int64) (recordingType string, parentID int64, err error) { resp, err := client.Get(ctx, fmt.Sprintf("/recordings/%d.json", recordingID)) if err != nil { From a92a24f92120ac07f381947e5de4b7137f037c26 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 27 Oct 2026 10:07:06 -0500 Subject: [PATCH 3/4] Decide a Detail trash only by a type it read Detail read a recording's type with the rest of its fields, so one field it couldn't parse, such as a timestamp, left the type unknown: t then asked to trash, and RemoveRecording, reading the type itself, deleted the line for good. Detail now reads the type and parent on their own, and RemoveRecording takes only a type the caller has, writing nothing without one, since the caller's confirmation can't have said which would happen. Its read is left for the one thing a caller may lack, a line's Campfire. --- internal/tui/workspace/data/hub.go | 23 ++--- .../workspace/data/remove_recording_test.go | 93 ++++++++++--------- internal/tui/workspace/views/detail.go | 13 ++- internal/tui/workspace/views/detail_test.go | 18 ++++ 4 files changed, 88 insertions(+), 59 deletions(-) diff --git a/internal/tui/workspace/data/hub.go b/internal/tui/workspace/data/hub.go index c155053c4..20bf4d7e1 100644 --- a/internal/tui/workspace/data/hub.go +++ b/internal/tui/workspace/data/hub.go @@ -989,26 +989,27 @@ func IsCampfireLine(recordingType string) bool { // project it would skip the line endpoint's checks on who may delete a line // and until when. Anything else is trashed. // -// recordingType is the API's type for the recording and campfireID a line's -// Campfire, when the caller has them. A view's scope carries only a label for -// the type ("Todo", "Campfire::Line") or none, so a caller with no more than -// that passes "", and the recording is read to learn its type; a line is read -// too when its Campfire isn't given. A recording that can't be read is left -// alone rather than trashed on a guess. +// recordingType is the API's type for the recording, never a view's label +// ("Todo", "Campfire::Line"): a caller with only a label reads the type with +// RecordingKind first. Without one nothing is written, since the confirmation +// the caller showed can't have said which of the two would happen. campfireID +// is a line's Campfire, read from the line when the caller passes 0. func (h *Hub) RemoveRecording(ctx context.Context, accountID string, recordingID int64, recordingType string, campfireID int64) (deleted bool, err error) { client := h.multi.ClientFor(accountID) if client == nil { return false, fmt.Errorf("no client for account %s", accountID) } - if recordingType == "" || (IsCampfireLine(recordingType) && campfireID == 0) { - recordingType, campfireID, err = readRecordingKind(ctx, client, recordingID) - if err != nil { - return false, err - } + if recordingType == "" { + return false, fmt.Errorf("can't tell what recording %d is, so it was left alone", recordingID) } if !IsCampfireLine(recordingType) { return false, client.Recordings().Trash(ctx, recordingID) } + if campfireID == 0 { + if _, campfireID, err = readRecordingKind(ctx, client, recordingID); err != nil { + return false, err + } + } if campfireID == 0 { return false, fmt.Errorf("line %d names no Campfire to delete it from", recordingID) } diff --git a/internal/tui/workspace/data/remove_recording_test.go b/internal/tui/workspace/data/remove_recording_test.go index 14e3e1025..abc9ec993 100644 --- a/internal/tui/workspace/data/remove_recording_test.go +++ b/internal/tui/workspace/data/remove_recording_test.go @@ -75,66 +75,57 @@ func TestRemoveRecording_KnownLineDeletes(t *testing.T) { assert.Equal(t, []string{deleteLine}, transport.recorded()) } -// A line whose Campfire the caller lacks, or a recording whose type it knows -// only by a label or not at all, is read first, and a line found that way is -// deleted, never trashed. -func TestRemoveRecording_ReadsWhatItLacks(t *testing.T) { - for _, recordingType := range []string{"Chat::Lines::Text", ""} { - t.Run("type "+recordingType, func(t *testing.T) { - h, transport := removeHub(map[string]removeReply{ - readLine: {http.StatusOK, lineInPing}, - deleteLine: {http.StatusNoContent, ""}, - }) +// A line whose Campfire the caller lacks is read to find it, and deleted from +// it, never trashed. +func TestRemoveRecording_ReadsALinesCampfire(t *testing.T) { + h, transport := removeHub(map[string]removeReply{ + readLine: {http.StatusOK, lineInPing}, + deleteLine: {http.StatusNoContent, ""}, + }) - deleted, err := h.RemoveRecording(context.Background(), "99999", 111, recordingType, 0) + deleted, err := h.RemoveRecording(context.Background(), "99999", 111, "Chat::Lines::Text", 0) - require.NoError(t, err) - assert.True(t, deleted) - assert.Equal(t, []string{readLine, deleteLine}, transport.recorded()) - }) - } + require.NoError(t, err) + assert.True(t, deleted) + assert.Equal(t, []string{readLine, deleteLine}, transport.recorded()) } -// Anything that isn't a line is trashed: read first when its type is unknown, -// straight away when the caller has it. +// Anything that isn't a line is trashed, with no read. func TestRemoveRecording_OtherRecordingsTrash(t *testing.T) { const trashTodo = "PUT /99999/recordings/100/status/trashed.json" - const readTodo = "GET /99999/recordings/100.json" - cases := []struct { - recordingType string - want []string - }{ - {"Todo", []string{trashTodo}}, - {"", []string{readTodo, trashTodo}}, - } - for _, c := range cases { - t.Run("type "+c.recordingType, func(t *testing.T) { - h, transport := removeHub(map[string]removeReply{ - readTodo: {http.StatusOK, `{"id":100,"type":"Todo","parent":{"id":50,"type":"Todolist"}}`}, - trashTodo: {http.StatusNoContent, ""}, - }) + h, transport := removeHub(map[string]removeReply{trashTodo: {http.StatusNoContent, ""}}) - deleted, err := h.RemoveRecording(context.Background(), "99999", 100, c.recordingType, 0) + deleted, err := h.RemoveRecording(context.Background(), "99999", 100, "Todo", 0) - require.NoError(t, err) - assert.False(t, deleted) - assert.Equal(t, c.want, transport.recorded()) - }) - } + require.NoError(t, err) + assert.False(t, deleted) + assert.Equal(t, []string{trashTodo}, transport.recorded()) +} + +// Without the recording's type nothing is sent: the caller's confirmation +// can't have said whether this would trash it or delete it for good. +func TestRemoveRecording_WritesNothingWithoutAType(t *testing.T) { + h, transport := removeHub(map[string]removeReply{ + deleteLine: {http.StatusNoContent, ""}, + trashLine: {http.StatusNoContent, ""}, + }) + + _, err := h.RemoveRecording(context.Background(), "99999", 111, "", 0) + + require.Error(t, err) + assert.Empty(t, transport.recorded()) } -// When the read can't say what the recording is, or a line names no -// Campfire, nothing is written: trashing on a guess is how a line would reach -// the generic route. -func TestRemoveRecording_WritesNothingItCantDecide(t *testing.T) { +// When the read can't name a line's Campfire, nothing is written: trashing it +// instead is how a line would reach the generic route. +func TestRemoveRecording_WritesNothingWithoutALinesCampfire(t *testing.T) { cases := []struct { name string read removeReply }{ {"unreadable", removeReply{http.StatusNotFound, `{"error":"Not Found"}`}}, {"no content", removeReply{http.StatusNoContent, ""}}, - {"no type", removeReply{http.StatusOK, `{"id":111}`}}, - {"line without a parent", removeReply{http.StatusOK, `{"id":111,"type":"Chat::Lines::Text"}`}}, + {"no parent", removeReply{http.StatusOK, `{"id":111,"type":"Chat::Lines::Text"}`}}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { @@ -144,7 +135,7 @@ func TestRemoveRecording_WritesNothingItCantDecide(t *testing.T) { trashLine: {http.StatusNoContent, ""}, }) - _, err := h.RemoveRecording(context.Background(), "99999", 111, "", 0) + _, err := h.RemoveRecording(context.Background(), "99999", 111, "Chat::Lines::Text", 0) require.Error(t, err) assert.Equal(t, []string{readLine}, transport.recorded()) @@ -152,6 +143,18 @@ func TestRemoveRecording_WritesNothingItCantDecide(t *testing.T) { } } +// RecordingKind reads the API's type and the parent for a caller that has +// only a label. +func TestRecordingKind(t *testing.T) { + h, _ := removeHub(map[string]removeReply{readLine: {http.StatusOK, lineInPing}}) + + recordingType, parentID, err := h.RecordingKind(context.Background(), "99999", 111) + + require.NoError(t, err) + assert.Equal(t, "Chat::Lines::Text", recordingType) + assert.Equal(t, int64(222), parentID) +} + func TestIsCampfireLine(t *testing.T) { for _, line := range []string{"Chat::Lines::Text", "Chat::Lines::RichText", "Chat::Lines::Upload"} { assert.True(t, IsCampfireLine(line), line) diff --git a/internal/tui/workspace/views/detail.go b/internal/tui/workspace/views/detail.go index 7c6424608..310a19413 100644 --- a/internal/tui/workspace/views/detail.go +++ b/internal/tui/workspace/views/detail.go @@ -1514,6 +1514,11 @@ func (v *Detail) fetchDetail() tea.Cmd { Creator *struct { Name string `json:"name"` } `json:"creator"` + } + // The type and parent, which decide what t does, are read on + // their own, so a field the display can't parse doesn't lose them. + var kind struct { + Type string `json:"type"` Parent *struct { ID int64 `json:"id"` } `json:"parent"` @@ -1538,14 +1543,16 @@ func (v *Detail) fetchDetail() tea.Cmd { data = detailData{ title: title, recordType: titleCase(recordingType), - apiType: generic.Type, content: generic.Content, creator: creator, createdAt: generic.CreatedAt, appURL: generic.AppURL, } - if generic.Parent != nil { - data.parentID = generic.Parent.ID + } + if err := resp.UnmarshalData(&kind); err == nil { + data.apiType = kind.Type + if kind.Parent != nil { + data.parentID = kind.Parent.ID } } } diff --git a/internal/tui/workspace/views/detail_test.go b/internal/tui/workspace/views/detail_test.go index 2f156569c..1cdc4139c 100644 --- a/internal/tui/workspace/views/detail_test.go +++ b/internal/tui/workspace/views/detail_test.go @@ -1097,6 +1097,24 @@ func TestDetail_Trash_CampfireLineDeletesThroughItsCampfire(t *testing.T) { } } +// A field the view can't display doesn't hide what the recording is: a line +// whose timestamp won't parse is still confirmed and deleted as a line. +func TestDetail_Trash_CampfireLineWithAFieldItCantRead(t *testing.T) { + unreadable := strings.Replace(pingLineJSON, `"2026-10-27T10:00:00Z"`, `"yesterday"`, 1) + require.NotEqual(t, pingLineJSON, unreadable) + transport := &routeTransport{routes: map[string]stubReply{ + "GET /99999/buckets/333/recordings/111.json": {http.StatusOK, unreadable}, + "DELETE /99999/chats/222/lines/111": {http.StatusNoContent, ""}, + }} + v := loadedDetail(t, transport, 111, "Chat::Lines::Text") + + assert.Equal(t, "Press t again to delete this line for good", statusText(t, v.handleKey(runeKey('t')))) + result, ok := v.handleKey(runeKey('t'))().(trashResultMsg) + require.True(t, ok) + require.NoError(t, result.err) + assert.Equal(t, []string{"DELETE /99999/chats/222/lines/111"}, transport.writes()) +} + // Anything else Detail opens through the generic recording read is trashed as // before, decided by the type bc3 returns. func TestDetail_Trash_OtherRecordingTrashes(t *testing.T) { From 805c2d8845e9be9961cd6dc5816df62b4554e1d6 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 27 Oct 2026 10:24:01 -0500 Subject: [PATCH 4/4] Read a recording's kind through its bucket, and scope refusal hints The TUI read a recording's type from the bucket-less /recordings/.json, which bc3 answers with a 404 for some types, such as Kanban::Card, so :trash on a card stopped trashing it. RecordingKind and RemoveRecording now take the project and read /buckets//recordings/.json, as Detail does. The commands a Ping refusal names now carry the profile and account the refused command ran under, read before it resolved its account, as the spotlight breadcrumbs do, so pasting one into a fresh shell acts on the same account. --- internal/commands/recordings.go | 27 ++++++++++------- internal/commands/recordings_test.go | 29 +++++++++++++++++-- internal/commands/spotlight.go | 4 +-- internal/commands/subscriptions.go | 3 +- internal/tui/workspace/actions.go | 2 +- internal/tui/workspace/actions_test.go | 10 ++++--- internal/tui/workspace/data/hub.go | 19 +++++++----- .../workspace/data/remove_recording_test.go | 14 ++++----- internal/tui/workspace/views/detail.go | 2 +- internal/tui/workspace/views/hey.go | 2 +- internal/tui/workspace/views/hey_test.go | 4 +-- 11 files changed, 77 insertions(+), 39 deletions(-) diff --git a/internal/commands/recordings.go b/internal/commands/recordings.go index d71fe96fd..1a2e64b54 100644 --- a/internal/commands/recordings.go +++ b/internal/commands/recordings.go @@ -332,6 +332,7 @@ A comment's URL (…#__recording_) 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 } @@ -366,7 +367,7 @@ func runRecordingsStatus(cmd *cobra.Command, app *appctx.App, recordingIDStr, ne } if err != nil { - if refusal := pingWriteRefusal(cmd, app, recordingID, err); refusal != nil { + if refusal := pingWriteRefusal(cmd, app, scope, recordingID, err); refusal != nil { return refusal } return convertSDKError(err) @@ -414,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 @@ -442,7 +444,7 @@ 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, recordingID, err); refusal != nil { + if refusal := pingWriteRefusal(cmd, app, scope, recordingID, err); refusal != nil { return refusal } return convertSDKError(err) @@ -482,10 +484,12 @@ You can pass either an ID or a Basecamp URL: // 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. 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, recordingID int64, err error) *output.Error { +// 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 @@ -495,17 +499,20 @@ func pingWriteRefusal(cmd *cobra.Command, app *appctx.App, recordingID int64, er 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 or basecamp subscriptions unsubscribe %d", recordingID, recordingID), + 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)) + deleteLineCmd(cmd, strconv.FormatInt(recordingID, 10), strconv.FormatInt(recording.Parent.ID, 10), strconv.FormatInt(recording.Bucket.ID, 10)) + + scopeArgs } return refusal } diff --git a/internal/commands/recordings_test.go b/internal/commands/recordings_test.go index 9fdce0664..5a5ec3ffe 100644 --- a/internal/commands/recordings_test.go +++ b/internal/commands/recordings_test.go @@ -342,14 +342,17 @@ func runPingGatedWrite(t *testing.T, w pingGatedWrite, readStatus int, readBody } // runPingGatedWriteIn is runPingGatedWrite with the output in format, and -// returns the app it ran in too. -func runPingGatedWriteIn(t *testing.T, format output.Format, w pingGatedWrite, readStatus int, readBody string) (*output.Error, *appctx.App) { +// 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...) @@ -395,6 +398,28 @@ func TestRecordingWritesExplainAPingLineToAPerson(t *testing.T) { 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"}}` diff --git a/internal/commands/spotlight.go b/internal/commands/spotlight.go index 40a0a5a0a..27f2cdd1f 100644 --- a/internal/commands/spotlight.go +++ b/internal/commands/spotlight.go @@ -47,7 +47,7 @@ nothing. Requires edit access to the project.`, recording, err := app.Account().Recordings().Spotlight(cmd.Context(), recordingID) if err != nil { - if refusal := pingWriteRefusal(cmd, app, recordingID, err); refusal != nil { + if refusal := pingWriteRefusal(cmd, app, scope, recordingID, err); refusal != nil { return refusal } return spotlightError(err) @@ -100,7 +100,7 @@ nothing. Requires edit access to the project.`, } if err := app.Account().Recordings().Unspotlight(cmd.Context(), recordingID); err != nil { - if refusal := pingWriteRefusal(cmd, app, recordingID, err); refusal != nil { + if refusal := pingWriteRefusal(cmd, app, scope, recordingID, err); refusal != nil { return refusal } return convertSDKError(err) diff --git a/internal/commands/subscriptions.go b/internal/commands/subscriptions.go index 8ad84bb09..9af6c21fc 100644 --- a/internal/commands/subscriptions.go +++ b/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,7 +294,7 @@ 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, recordingID, err); refusal != nil { + if refusal := pingWriteRefusal(cmd, app, scope, recordingID, err); refusal != nil { return refusal } return convertSDKError(err) diff --git a/internal/tui/workspace/actions.go b/internal/tui/workspace/actions.go index 48f8a3381..2a3f67943 100644 --- a/internal/tui/workspace/actions.go +++ b/internal/tui/workspace/actions.go @@ -378,7 +378,7 @@ func DefaultActions() *Registry { // 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.RecordingID) + recordingType, _, err := hub.RecordingKind(ctx, scope.AccountID, scope.ProjectID, scope.RecordingID) if err != nil { return err } diff --git a/internal/tui/workspace/actions_test.go b/internal/tui/workspace/actions_test.go index ef9ed21d9..6da04c032 100644 --- a/internal/tui/workspace/actions_test.go +++ b/internal/tui/workspace/actions_test.go @@ -347,7 +347,7 @@ func TestAction_Trash_RefusesACampfireLine(t *testing.T) { for _, label := range []string{"Chat::Lines::Text", "Campfire::Line", ""} { t.Run("label "+label, func(t *testing.T) { transport := &actionTransport{routes: map[string]actionReply{ - "GET /99999/recordings/111.json": {http.StatusOK, pingLineJSON}, + "GET /99999/buckets/333/recordings/111.json": {http.StatusOK, pingLineJSON}, }} session := actionSession(transport, Scope{AccountID: "99999", ProjectID: 333, RecordingID: 111, RecordingType: label}) @@ -373,13 +373,15 @@ func TestAction_Trash_WritesNothingWhenItCantTell(t *testing.T) { assert.True(t, ok, "expected an ErrorMsg, got %T", msg) } -// Anything else is trashed as before. +// Anything else is trashed as before, a card included: its type is read +// through its bucket, since bc3's bucket-less /recordings/.json answers +// 404 for one. func TestAction_Trash_OtherRecordingTrashes(t *testing.T) { transport := &actionTransport{routes: map[string]actionReply{ - "GET /99999/recordings/100.json": {http.StatusOK, `{"id":100,"type":"Todo","title":"Ship it"}`}, + "GET /99999/buckets/333/recordings/100.json": {http.StatusOK, `{"id":100,"type":"Kanban::Card","title":"Ship it"}`}, "PUT /99999/recordings/100/status/trashed.json": {http.StatusNoContent, ""}, }} - session := actionSession(transport, Scope{AccountID: "99999", ProjectID: 333, RecordingID: 100, RecordingType: "Todo"}) + session := actionSession(transport, Scope{AccountID: "99999", ProjectID: 333, RecordingID: 100, RecordingType: "Card"}) msg := trashAction(t).Execute(session)() diff --git a/internal/tui/workspace/data/hub.go b/internal/tui/workspace/data/hub.go index 20bf4d7e1..2f9ee0244 100644 --- a/internal/tui/workspace/data/hub.go +++ b/internal/tui/workspace/data/hub.go @@ -993,8 +993,9 @@ func IsCampfireLine(recordingType string) bool { // ("Todo", "Campfire::Line"): a caller with only a label reads the type with // RecordingKind first. Without one nothing is written, since the confirmation // the caller showed can't have said which of the two would happen. campfireID -// is a line's Campfire, read from the line when the caller passes 0. -func (h *Hub) RemoveRecording(ctx context.Context, accountID string, recordingID int64, recordingType string, campfireID int64) (deleted bool, err error) { +// is a line's Campfire, read from the line in projectID, its bucket, when the +// caller passes 0. +func (h *Hub) RemoveRecording(ctx context.Context, accountID string, projectID, recordingID int64, recordingType string, campfireID int64) (deleted bool, err error) { client := h.multi.ClientFor(accountID) if client == nil { return false, fmt.Errorf("no client for account %s", accountID) @@ -1006,7 +1007,7 @@ func (h *Hub) RemoveRecording(ctx context.Context, accountID string, recordingID return false, client.Recordings().Trash(ctx, recordingID) } if campfireID == 0 { - if _, campfireID, err = readRecordingKind(ctx, client, recordingID); err != nil { + if _, campfireID, err = readRecordingKind(ctx, client, projectID, recordingID); err != nil { return false, err } } @@ -1019,17 +1020,19 @@ func (h *Hub) RemoveRecording(ctx context.Context, accountID string, recordingID // RecordingKind reads a recording's API type and its parent's ID, which for a // Campfire line is its Campfire, for a caller that knows the recording only by // a view's label. -func (h *Hub) RecordingKind(ctx context.Context, accountID string, recordingID int64) (recordingType string, parentID int64, err error) { +func (h *Hub) RecordingKind(ctx context.Context, accountID string, projectID, recordingID int64) (recordingType string, parentID int64, err error) { client := h.multi.ClientFor(accountID) if client == nil { return "", 0, fmt.Errorf("no client for account %s", accountID) } - return readRecordingKind(ctx, client, recordingID) + return readRecordingKind(ctx, client, projectID, recordingID) } -// readRecordingKind reads a recording's API type and its parent's ID. -func readRecordingKind(ctx context.Context, client *basecamp.AccountClient, recordingID int64) (recordingType string, parentID int64, err error) { - resp, err := client.Get(ctx, fmt.Sprintf("/recordings/%d.json", recordingID)) +// readRecordingKind reads a recording's API type and its parent's ID, through +// its bucket: bc3 answers the bucket-less /recordings/.json with a 404 for +// some types, such as Kanban::Card. +func readRecordingKind(ctx context.Context, client *basecamp.AccountClient, projectID, recordingID int64) (recordingType string, parentID int64, err error) { + resp, err := client.Get(ctx, fmt.Sprintf("/buckets/%d/recordings/%d.json", projectID, recordingID)) if err != nil { return "", 0, err } diff --git a/internal/tui/workspace/data/remove_recording_test.go b/internal/tui/workspace/data/remove_recording_test.go index abc9ec993..d97f2bdf1 100644 --- a/internal/tui/workspace/data/remove_recording_test.go +++ b/internal/tui/workspace/data/remove_recording_test.go @@ -17,7 +17,7 @@ const ( lineInPing = `{"id":111,"type":"Chat::Lines::Text","parent":{"id":222,"type":"Chat::Transcript"},"bucket":{"id":333,"type":"Circle"}}` deleteLine = "DELETE /99999/chats/222/lines/111" trashLine = "PUT /99999/recordings/111/status/trashed.json" - readLine = "GET /99999/recordings/111.json" + readLine = "GET /99999/buckets/333/recordings/111.json" ) type removeReply struct { @@ -68,7 +68,7 @@ func removeHub(routes map[string]removeReply) (*Hub, *removeTransport) { func TestRemoveRecording_KnownLineDeletes(t *testing.T) { h, transport := removeHub(map[string]removeReply{deleteLine: {http.StatusNoContent, ""}}) - deleted, err := h.RemoveRecording(context.Background(), "99999", 111, "Chat::Lines::RichText", 222) + deleted, err := h.RemoveRecording(context.Background(), "99999", 333, 111, "Chat::Lines::RichText", 222) require.NoError(t, err) assert.True(t, deleted) @@ -83,7 +83,7 @@ func TestRemoveRecording_ReadsALinesCampfire(t *testing.T) { deleteLine: {http.StatusNoContent, ""}, }) - deleted, err := h.RemoveRecording(context.Background(), "99999", 111, "Chat::Lines::Text", 0) + deleted, err := h.RemoveRecording(context.Background(), "99999", 333, 111, "Chat::Lines::Text", 0) require.NoError(t, err) assert.True(t, deleted) @@ -95,7 +95,7 @@ func TestRemoveRecording_OtherRecordingsTrash(t *testing.T) { const trashTodo = "PUT /99999/recordings/100/status/trashed.json" h, transport := removeHub(map[string]removeReply{trashTodo: {http.StatusNoContent, ""}}) - deleted, err := h.RemoveRecording(context.Background(), "99999", 100, "Todo", 0) + deleted, err := h.RemoveRecording(context.Background(), "99999", 333, 100, "Todo", 0) require.NoError(t, err) assert.False(t, deleted) @@ -110,7 +110,7 @@ func TestRemoveRecording_WritesNothingWithoutAType(t *testing.T) { trashLine: {http.StatusNoContent, ""}, }) - _, err := h.RemoveRecording(context.Background(), "99999", 111, "", 0) + _, err := h.RemoveRecording(context.Background(), "99999", 333, 111, "", 0) require.Error(t, err) assert.Empty(t, transport.recorded()) @@ -135,7 +135,7 @@ func TestRemoveRecording_WritesNothingWithoutALinesCampfire(t *testing.T) { trashLine: {http.StatusNoContent, ""}, }) - _, err := h.RemoveRecording(context.Background(), "99999", 111, "Chat::Lines::Text", 0) + _, err := h.RemoveRecording(context.Background(), "99999", 333, 111, "Chat::Lines::Text", 0) require.Error(t, err) assert.Equal(t, []string{readLine}, transport.recorded()) @@ -148,7 +148,7 @@ func TestRemoveRecording_WritesNothingWithoutALinesCampfire(t *testing.T) { func TestRecordingKind(t *testing.T) { h, _ := removeHub(map[string]removeReply{readLine: {http.StatusOK, lineInPing}}) - recordingType, parentID, err := h.RecordingKind(context.Background(), "99999", 111) + recordingType, parentID, err := h.RecordingKind(context.Background(), "99999", 333, 111) require.NoError(t, err) assert.Equal(t, "Chat::Lines::Text", recordingType) diff --git a/internal/tui/workspace/views/detail.go b/internal/tui/workspace/views/detail.go index 310a19413..c364fade4 100644 --- a/internal/tui/workspace/views/detail.go +++ b/internal/tui/workspace/views/detail.go @@ -724,7 +724,7 @@ func (v *Detail) trashRecording() tea.Cmd { ctx := hub.ProjectContext() recordingID, apiType, parentID := v.recordingID, v.data.apiType, v.data.parentID return func() tea.Msg { - deleted, err := hub.RemoveRecording(ctx, scope.AccountID, recordingID, apiType, parentID) + deleted, err := hub.RemoveRecording(ctx, scope.AccountID, scope.ProjectID, recordingID, apiType, parentID) return trashResultMsg{deleted: deleted, err: err} } } diff --git a/internal/tui/workspace/views/hey.go b/internal/tui/workspace/views/hey.go index 86fba5c8c..0e841cc77 100644 --- a/internal/tui/workspace/views/hey.go +++ b/internal/tui/workspace/views/hey.go @@ -406,7 +406,7 @@ func (v *Hey) trashSelected() tea.Cmd { return func() tea.Msg { // meta.Type is the API's type, so a Campfire line would be // deleted through its Campfire rather than trashed. - deleted, err := hub.RemoveRecording(ctx, meta.AccountID, meta.ID, meta.Type, 0) + deleted, err := hub.RemoveRecording(ctx, meta.AccountID, meta.ProjectID, meta.ID, meta.Type, 0) return heyTrashResultMsg{itemID: itemID, deleted: deleted, err: err} } } diff --git a/internal/tui/workspace/views/hey_test.go b/internal/tui/workspace/views/hey_test.go index 8a13769ec..13e579b4b 100644 --- a/internal/tui/workspace/views/hey_test.go +++ b/internal/tui/workspace/views/hey_test.go @@ -120,8 +120,8 @@ func TestHey_Trash_TrashesARecording(t *testing.T) { // type, so a line would be deleted through its Campfire, read to find it. func TestHey_Trash_DeletesACampfireLine(t *testing.T) { transport := &routeTransport{routes: map[string]stubReply{ - "GET /99999/recordings/111.json": {http.StatusOK, pingLineJSON}, - "DELETE /99999/chats/222/lines/111": {http.StatusNoContent, ""}, + "GET /99999/buckets/333/recordings/111.json": {http.StatusOK, pingLineJSON}, + "DELETE /99999/chats/222/lines/111": {http.StatusNoContent, ""}, }} v := heyOverTransport(transport, []data.ActivityEntryInfo{ {ID: 111, Title: "Lunch?", Type: "Chat::Lines::Text", AccountID: "99999", ProjectID: 333, UpdatedAtTS: 100},