Commit 9e99af2a11
9e99af2a1171326a949d5ec61eb7fe22fdb01dbb
parent: 1dea539c7c
Verified · cmc
cmc <hello@cleberg.net> · 2026-09-20 11:10 UTC
control: issue assign notifies only new assignees
SetIssueAssignee inserts ON CONFLICT DO NOTHING and returns nil
whether or not it inserted, so every name in --add was counted as
added. Running the same assign twice filed the inbox row twice,
mailed twice and pushed twice; a client reconciling an assignee list
by re-sending the whole set would do it on every save. The pre-update
read already carries who was on it.
Also uses issueSubject for the subject, as the other three notify
sites in this file do.
Ref #89
internal/control/issue.go
+14 −1
| @@ -445,6 +445,15 @@ func runIssueAssign(c *Ctx, args []string) int { |
| 445 | 445 | } |
| 446 | 446 | return u, -1 |
| 447 | 447 | } |
| 448 | // issue is the read from before the update, so its Assignees are who |
| 449 | // was already on it. SetIssueAssignee inserts ON CONFLICT DO NOTHING |
| 450 | // and returns nil whether or not it inserted, and the notice below is |
| 451 | // for accounts newly added: a client reconciling the list by |
| 452 | // re-sending the whole set must not notify on every save. |
| 453 | assigned := make(map[string]bool, len(issue.Assignees)) |
| 454 | for _, name := range issue.Assignees { |
| 455 | assigned[name] = true |
| 456 | } |
| 448 | 457 | var added []int64 |
| 449 | 458 | for _, name := range adds { |
| 450 | 459 | u, code := resolve(name) |
| @@ -454,6 +463,10 @@ func runIssueAssign(c *Ctx, args []string) int { |
| 454 | 463 | if err := c.Store.SetIssueAssignee(issue.ID, u.ID, true); err != nil { |
| 455 | 464 | return c.fail(protocol.ExitFailure, "%v", err) |
| 456 | 465 | } |
| 466 | if assigned[u.Username] { |
| 467 | continue |
| 468 | } |
| 469 | assigned[u.Username] = true |
| 457 | 470 | added = append(added, u.ID) |
| 458 | 471 | } |
| 459 | 472 | for _, name := range removes { |
| @@ -480,7 +493,7 @@ func runIssueAssign(c *Ctx, args []string) int { |
| 480 | 493 | // Removals file nothing, and notify drops the actor, so assigning |
| 481 | 494 | // yourself is silent. |
| 482 | 495 | notify(c, added, notice{repo: repo, kind: "issue", direct: true, |
| 483 | | subject: fmt.Sprintf("[%s] #%d: %s", repo.Path(), issue.Number, issue.Title), |
| 496 | subject: issueSubject(repo, issue.Number, issue.Title), |
| 484 | 497 | action: fmt.Sprintf("assigned you to #%d", issue.Number), |
| 485 | 498 | path: fmt.Sprintf("%s/issues/%d", repo.Path(), issue.Number)}) |
| 486 | 499 | } |
internal/control/issue_test.go
+32
| @@ -14,6 +14,38 @@ func TestIssueAssignNotifiesTheAssignee(t *testing.T) { |
| 14 | 14 | } |
| 15 | 15 | } |
| 16 | 16 | |
| 17 | // The spec files a notice for each account newly added. SetIssueAssignee |
| 18 | // inserts ON CONFLICT DO NOTHING and reports nothing either way, so a |
| 19 | // client reconciling an assignee list by re-sending the whole set would |
| 20 | // file, mail and push a row on every save. |
| 21 | func TestIssueAssignDoesNotRenotifyAnExistingAssignee(t *testing.T) { |
| 22 | c, repo, bob := testRepoWithWatcher(t) |
| 23 | c.Store.AddPushDevice(bob, "tok-b", "iphone") |
| 24 | |
| 25 | if code := runIssueAssign(c, []string{repo.Path(), "1", "--add", "bob"}); code != 0 { |
| 26 | t.Fatalf("exit %d", code) |
| 27 | } |
| 28 | if code := runIssueAssign(c, []string{repo.Path(), "1", "--add", "bob"}); code != 0 { |
| 29 | t.Fatalf("exit %d", code) |
| 30 | } |
| 31 | |
| 32 | rows, _ := c.Store.Inbox(bob, false, 20, 0) |
| 33 | if len(rows) != 1 { |
| 34 | t.Fatalf("filed %d rows for one assignment: %+v", len(rows), rows) |
| 35 | } |
| 36 | if due, _ := c.Store.DuePush(20); len(due) != 1 { |
| 37 | t.Fatalf("queued %d pushes for one assignment", len(due)) |
| 38 | } |
| 39 | // The second call still succeeds and still reports the assignee. |
| 40 | updated, err := c.Store.IssueByNumber(repo.ID, 1) |
| 41 | if err != nil { |
| 42 | t.Fatal(err) |
| 43 | } |
| 44 | if len(updated.Assignees) != 1 || updated.Assignees[0] != "bob" { |
| 45 | t.Fatalf("assignees: %+v", updated.Assignees) |
| 46 | } |
| 47 | } |
| 48 | |
| 17 | 49 | func TestIssueAssignIsSilentForTheActorAndForRemovals(t *testing.T) { |
| 18 | 50 | c, repo, bob := testRepoWithWatcher(t) |
| 19 | 51 | |