From f466773ff8584a9e570eea5fe0fc359c0a64b2c4 Mon Sep 17 00:00:00 2001 From: Ryan Lewis Date: Sat, 10 Oct 2026 15:11:50 +0100 Subject: [PATCH 1/2] refactor(edit): decide whether an edit is sent in one place --- cmd/things/edit.go | 30 +++++++++++++++++++++--------- cmd/things/edit_test.go | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 9 deletions(-) diff --git a/cmd/things/edit.go b/cmd/things/edit.go index 396bc3f..107c815 100644 --- a/cmd/things/edit.go +++ b/cmd/things/edit.go @@ -316,8 +316,8 @@ var ( // checklist says whether the command set a checklist flag, which only `edit` // has. params builds the update from the shared params with the item's id // and the auth token filled in, and write sends it. The update is validated -// before the tag check, and so is the auth token when the edit will be sent, -// so an edit that fails creates no tag. +// before the tag check, and so is the auth token when the edit will be sent +// (sendsEdit), so an edit that fails creates no tag. func runEdit[P interface { Validate() error ValidateSend() error @@ -398,9 +398,9 @@ func runEdit[P interface { if err := params(common).Validate(); err != nil { return err } - // Only an already-closed item with no change goes unsent (see below), - // and a tag --create-tags makes is a change. - if !already || s.Reveal || createsTags(database, s.TagFlags, f.Tags, f.AddTags) { + // The tag check below can create a tag, which is a change the edit has + // to send, so the token is checked before it whenever that happens. + if sendsEdit(already, s.Reveal, createsTags(database, s.TagFlags, f.Tags, f.AddTags)) { if err := params(common).ValidateSend(); err != nil { return err } @@ -448,16 +448,28 @@ func runEdit[P interface { } } } + if !sendsEdit(already, s.Reveal, changed) { + noteAlreadyClosed(d, task) + return printItem(d, database, task) + } if already { - if !changed && !s.Reveal { - noteAlreadyClosed(d, task) - return printItem(d, database, task) - } fmt.Fprintf(d.errOut(), "note: %q is already %s; the status is left out of the edit\n", task.Title, task.Status) } return applyEdit(d, database, task, changed, checklist, want, s.Duplicate, when, update) } +// sendsEdit reports whether runEdit sends the edit to Things: only an item +// already in the closed status asked for, with no --reveal and nothing else +// to change, goes unsent, since Things records no change for it. runEdit +// asks twice. Before the tag check, the one change it can know of is a tag +// --create-tags will make, so the auth token is checked then and a failing +// edit creates no tag. After the no-op check, changed is the whole answer. +// An edit sent for a change the first ask could not see is refused for a +// missing token by the write itself, before anything is opened. +func sendsEdit(already, reveal, changed bool) bool { + return !already || reveal || changed +} + // createsTags reports whether the tag check will create a tag: --create-tags // with a tag flag naming one Things does not have. A failed lookup reports // none, and the tag check then refuses the edit before creating anything. diff --git a/cmd/things/edit_test.go b/cmd/things/edit_test.go index 0fc1d61..a739c9e 100644 --- a/cmd/things/edit_test.go +++ b/cmd/things/edit_test.go @@ -955,6 +955,46 @@ func TestEditAlreadyClosedExistingCreateTagsNeedsNoAuthToken(t *testing.T) { } } +// An already-closed item with a field to change is sent, so with no auth +// token it is refused by the write, after the no-op check, and nothing runs. +// --reveal sends it too. +func TestEditAlreadyClosedWithChangeNeedsAuthToken(t *testing.T) { + for _, args := range [][]string{ + {"edit", "done-1", "--complete", "--title", "Renamed"}, + {"edit", "done-1", "--complete", "--reveal"}, + } { + fastVerify(t) + database, sqlDB := seedWritable(t) + if _, err := sqlDB.Exec(`DELETE FROM TMSettings`); err != nil { + t.Fatalf("clear settings: %v", err) + } + dbtest.NewFixture(t, sqlDB).Todo("done-1", "Filed", 1, dbtest.Status(model.StatusCompleted), dbtest.Anytime()) + calls := stubExecDropping(t) + _, _, err := runStreams(t, database, args...) + if err == nil || !strings.HasPrefix(err.Error(), "update: auth token is required") { + t.Errorf("%v = %v, want the auth token refusal", args, err) + } + if *calls != 0 { + t.Errorf("%v issued %d commands, want none", args, *calls) + } + } +} + +func TestSendsEdit(t *testing.T) { + for _, tc := range []struct { + already, reveal, changed, want bool + }{ + {false, false, false, true}, + {true, false, false, false}, + {true, true, false, true}, + {true, false, true, true}, + } { + if got := sendsEdit(tc.already, tc.reveal, tc.changed); got != tc.want { + t.Errorf("sendsEdit(%v, %v, %v) = %v, want %v", tc.already, tc.reveal, tc.changed, got, tc.want) + } + } +} + // An edit that will be sent with no auth token to send is refused before // --create-tags creates the unknown tag over AppleScript, with the error the // write gives: no command runs at all. An already-closed item counts, since a From 04efe71bd913518874c63d63144c1807e9bb96fa Mon Sep 17 00:00:00 2001 From: Ryan Lewis Date: Sat, 10 Oct 2026 15:15:02 +0100 Subject: [PATCH 2/2] refactor(edit): look up created tags only when the edit would go unsent --- cmd/things/edit.go | 5 +++-- cmd/things/edit_test.go | 32 +++++++++++++++++--------------- 2 files changed, 20 insertions(+), 17 deletions(-) diff --git a/cmd/things/edit.go b/cmd/things/edit.go index 107c815..140d29a 100644 --- a/cmd/things/edit.go +++ b/cmd/things/edit.go @@ -399,8 +399,9 @@ func runEdit[P interface { return err } // The tag check below can create a tag, which is a change the edit has - // to send, so the token is checked before it whenever that happens. - if sendsEdit(already, s.Reveal, createsTags(database, s.TagFlags, f.Tags, f.AddTags)) { + // to send, so the token is checked before it whenever that happens. The + // tag lookup runs only when the edit would otherwise go unsent. + if sendsEdit(already, s.Reveal, false) || createsTags(database, s.TagFlags, f.Tags, f.AddTags) { if err := params(common).ValidateSend(); err != nil { return err } diff --git a/cmd/things/edit_test.go b/cmd/things/edit_test.go index a739c9e..4f4d10d 100644 --- a/cmd/things/edit_test.go +++ b/cmd/things/edit_test.go @@ -957,26 +957,28 @@ func TestEditAlreadyClosedExistingCreateTagsNeedsNoAuthToken(t *testing.T) { // An already-closed item with a field to change is sent, so with no auth // token it is refused by the write, after the no-op check, and nothing runs. -// --reveal sends it too. +// --reveal sends it too, and is refused before the tag check. func TestEditAlreadyClosedWithChangeNeedsAuthToken(t *testing.T) { for _, args := range [][]string{ {"edit", "done-1", "--complete", "--title", "Renamed"}, {"edit", "done-1", "--complete", "--reveal"}, } { - fastVerify(t) - database, sqlDB := seedWritable(t) - if _, err := sqlDB.Exec(`DELETE FROM TMSettings`); err != nil { - t.Fatalf("clear settings: %v", err) - } - dbtest.NewFixture(t, sqlDB).Todo("done-1", "Filed", 1, dbtest.Status(model.StatusCompleted), dbtest.Anytime()) - calls := stubExecDropping(t) - _, _, err := runStreams(t, database, args...) - if err == nil || !strings.HasPrefix(err.Error(), "update: auth token is required") { - t.Errorf("%v = %v, want the auth token refusal", args, err) - } - if *calls != 0 { - t.Errorf("%v issued %d commands, want none", args, *calls) - } + t.Run(strings.Join(args[2:], " "), func(t *testing.T) { + fastVerify(t) + database, sqlDB := seedWritable(t) + if _, err := sqlDB.Exec(`DELETE FROM TMSettings`); err != nil { + t.Fatalf("clear settings: %v", err) + } + dbtest.NewFixture(t, sqlDB).Todo("done-1", "Filed", 1, dbtest.Status(model.StatusCompleted), dbtest.Anytime()) + calls := stubExecDropping(t) + _, _, err := runStreams(t, database, args...) + if err == nil || !strings.HasPrefix(err.Error(), "update: auth token is required") { + t.Errorf("%v = %v, want the auth token refusal", args, err) + } + if *calls != 0 { + t.Errorf("%v issued %d commands, want none", args, *calls) + } + }) } }