refactor(edit): decide whether an edit is sent in one place

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,10 @@ 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. 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
 		}
@@ -448,16 +449,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.

cmd/things/edit_test.go

@@ -955,6 +955,48 @@ 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, 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"},
+	} {
+		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)
+			}
+		})
+	}
+}
+
+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