krz/gitbay

A CLI-first git forge. cli forge git self-hosted https://gitbay.org

Commit a2d1580d3d

a2d1580d3dbd22814b96515833e024d03b854c4a

parent: f994d5a21a

Verified · cmc ci/build: success ci/sonar: success ci/test: success ci/vuln: success

cmc <hello@cleberg.net> · 2026-09-05 22:09 UTC 

control, store, web: ask a specific person for a review

mr review request <owner/name> <n> [--add <user>]... [--remove <user>]...
mirrors issue assign: it records requests in a new mr_review_requests
table (migration 0042, with the per-user index issue_assignees needed in
0035), notifies the people added, and refuses a request for someone who
cannot read the repository.

The review queue gains a second query, driven from mr_review_requests the
way AssignedIssues drives from issue_assignees, so a requested reviewer
sees the merge request regardless of involvement. It uses the same
head-sha check reviewQueueQuery does, so the queue still empties once the
current head is reviewed and repopulates on a new push. mr ready now also
notifies whoever has been asked.

The merge request page shows who has been asked and, for a collaborator,
a form to ask more.

Closes #145

Layout: unified · split

cmd/gitbay/main.go +3 −1
@@ -524,6 +524,8 @@ func milestoneCmd() *cobra.Command {
524} 524}
525 525
526func mrCmd() *cobra.Command { 526func mrCmd() *cobra.Command {
527 review := pass("review", "submit a review: --approve|--request-changes|--comment, or --discard a pending batch", passOpts{server: []string{"mr", "review"}, needsRepo: true})
528 review.AddCommand(pass("request", "ask specific people for review: [--add <u>]... [--remove <u>]...", passOpts{server: []string{"mr", "review", "request"}, needsRepo: true}))
527 return group("mr", "merge requests", 529 return group("mr", "merge requests",
528 pass("create", "open a merge request: --source <branch> --target <branch> --title <t>", 530 pass("create", "open a merge request: --source <branch> --target <branch> --title <t>",
529 passOpts{server: []string{"mr", "create"}, needsRepo: true, stdinOK: true, editor: "merge request", inferSource: true}), 531 passOpts{server: []string{"mr", "create"}, needsRepo: true, stdinOK: true, editor: "merge request", inferSource: true}),
@@ -536,7 +538,7 @@ func mrCmd() *cobra.Command {
536 pass("threads", "review threads on an MR", passOpts{server: []string{"mr", "threads"}, needsRepo: true}), 538 pass("threads", "review threads on an MR", passOpts{server: []string{"mr", "threads"}, needsRepo: true}),
537 pass("resolve", "resolve a review thread: <n> <thread-id>", passOpts{server: []string{"mr", "resolve"}, needsRepo: true}), 539 pass("resolve", "resolve a review thread: <n> <thread-id>", passOpts{server: []string{"mr", "resolve"}, needsRepo: true}),
538 pass("unresolve", "reopen a review thread: <n> <thread-id>", passOpts{server: []string{"mr", "unresolve"}, needsRepo: true}), 540 pass("unresolve", "reopen a review thread: <n> <thread-id>", passOpts{server: []string{"mr", "unresolve"}, needsRepo: true}),
539 pass("review", "submit a review: --approve|--request-changes|--comment, or --discard a pending batch", passOpts{server: []string{"mr", "review"}, needsRepo: true}), 541 review,
540 pass("merge", "merge: [--strategy ff|merge|squash|rebase]", passOpts{server: []string{"mr", "merge"}, needsRepo: true}), 542 pass("merge", "merge: [--strategy ff|merge|squash|rebase]", passOpts{server: []string{"mr", "merge"}, needsRepo: true}),
541 pass("close", "close without merging", passOpts{server: []string{"mr", "close"}, needsRepo: true}), 543 pass("close", "close without merging", passOpts{server: []string{"mr", "close"}, needsRepo: true}),
542 pass("revisions", "the heads this merge request has had", passOpts{server: []string{"mr", "revisions"}, needsRepo: true}), 544 pass("revisions", "the heads this merge request has had", passOpts{server: []string{"mr", "revisions"}, needsRepo: true}),
e2e/reviewloop_test.go +6 −5
@@ -87,12 +87,13 @@ func TestTwoAccountReviewLoop(t *testing.T) {
87 if q := reviewQueue(t, inst, reviewerKey); len(q) != 1 || q[0] != 1 { 87 if q := reviewQueue(t, inst, reviewerKey); len(q) != 1 || q[0] != 1 {
88 t.Fatalf("review queue = %v, want !1", q) 88 t.Fatalf("review queue = %v, want !1", q)
89 } 89 }
90 // The queue is how a reviewer finds out, and the only way: there is 90 // The queue is how this reviewer finds out: nobody ran "mr review
91 // no "request review from <user>", so nothing is pushed to someone 91 // request" for them, so being an owner or already in the thread is
92 // who is neither an owner nor already in the thread. Writing this 92 // the only other way in, and this reviewer is neither. Writing this
93 // test is what surfaced that — see #145. 93 // test is what surfaced the gap — see #145; TestMRReviewRequest covers
94 // the case where someone has been asked directly.
94 if got := inbox(t, inst, reviewerKey); strings.Contains(got, "ready for review") { 95 if got := inbox(t, inst, reviewerKey); strings.Contains(got, "ready for review") {
95 t.Fatalf("a reviewer is notified after all; #145 and this comment are stale:\n%s", got) 96 t.Fatalf("a reviewer is notified without being asked or involved:\n%s", got)
96 } 97 }
97 98
98 // The author cannot approve their own work past the gate. 99 // The author cannot approve their own work past the gate.
e2e/reviewrequest_test.go added +128
@@ -0,0 +1,128 @@
1package e2e
2
3import (
4 "os"
5 "path/filepath"
6 "strings"
7 "testing"
8)
9
10// TestMRReviewRequest drives "mr review request" (#145) end to end: a
11// requested reviewer reaches the queue without being otherwise involved,
12// drops out once they review the current head, comes back on a new push,
13// and --remove takes them out outright. A separate, private repository
14// checks that requesting someone who cannot read it is refused.
15func TestMRReviewRequest(t *testing.T) {
16 inst := startInstance(t)
17 aliceKey := inst.newKey(t, "alice")
18 bobKey := inst.newKey(t, "bob")
19 carolKey := inst.newKey(t, "carol")
20 inst.admin(t, "admin", "user", "create", "alice", "--key", aliceKey+".pub")
21 inst.admin(t, "admin", "user", "create", "bob", "--key", bobKey+".pub")
22 inst.admin(t, "admin", "user", "create", "carol", "--key", carolKey+".pub")
23
24 // Public, and bob is granted nothing: any involvement he has in the
25 // queue can only come from being asked directly.
26 if _, errOut, code := inst.ssh(t, aliceKey, "", "repo", "create", "alice/app"); code != 0 {
27 t.Fatalf("repo create: %s", errOut)
28 }
29
30 env := inst.gitEnv(aliceKey)
31 work := t.TempDir()
32 mustGit(t, work, env, "clone", inst.sshURL("alice/app"), "w")
33 dir := filepath.Join(work, "w")
34 os.WriteFile(filepath.Join(dir, "a.txt"), []byte("a\n"), 0o644)
35 mustGit(t, dir, env, "checkout", "-q", "-b", "main")
36 mustGit(t, dir, env, "add", ".")
37 mustGit(t, dir, env, "commit", "-q", "-m", "base")
38 mustGit(t, dir, env, "push", "-q", "origin", "main")
39 mustGit(t, dir, env, "checkout", "-q", "-b", "feat")
40 os.WriteFile(filepath.Join(dir, "a.txt"), []byte("a\nb\n"), 0o644)
41 mustGit(t, dir, env, "add", ".")
42 mustGit(t, dir, env, "commit", "-q", "-m", "feat")
43 mustGit(t, dir, env, "push", "-q", "origin", "feat")
44
45 if _, errOut, code := inst.ssh(t, aliceKey, "", "mr", "create", "alice/app",
46 "--source", "feat", "--target", "main", "--title", "'add b'", "--draft"); code != 0 {
47 t.Fatalf("mr create: %s", errOut)
48 }
49
50 // Asking while a draft does not put it in bob's queue: draft merge
51 // requests stay out of the review queue for everyone, requested or
52 // not.
53 if _, errOut, code := inst.ssh(t, aliceKey, "", "mr", "review", "request", "alice/app", "1", "--add", "bob"); code != 0 {
54 t.Fatalf("review request: %s", errOut)
55 }
56 if q := reviewQueue(t, inst, bobKey); len(q) != 0 {
57 t.Fatalf("a draft is waiting on the requested reviewer: %v", q)
58 }
59
60 // Ready: the request now surfaces, and it notified bob at the same
61 // moment it notified everyone else.
62 if _, errOut, code := inst.ssh(t, aliceKey, "", "mr", "ready", "alice/app", "1"); code != 0 {
63 t.Fatalf("mr ready: %s", errOut)
64 }
65 if !strings.Contains(inbox(t, inst, bobKey), "ready for review") {
66 t.Fatalf("requested reviewer not notified by mr ready:\n%s", inbox(t, inst, bobKey))
67 }
68 if q := reviewQueue(t, inst, bobKey); len(q) != 1 || q[0] != 1 {
69 t.Fatalf("requested reviewer not in queue: %v", q)
70 }
71
72 // Reviewing the current head empties the queue, the same rule an
73 // involved reviewer follows.
74 if _, errOut, code := inst.ssh(t, bobKey, "", "mr", "review", "alice/app", "1", "--approve"); code != 0 {
75 t.Fatalf("review: %s", errOut)
76 }
77 if q := reviewQueue(t, inst, bobKey); len(q) != 0 {
78 t.Fatalf("queue did not empty after reviewing the head: %v", q)
79 }
80
81 // A new push moves the head, so the review no longer covers it: back
82 // in the queue.
83 os.WriteFile(filepath.Join(dir, "a.txt"), []byte("a\nb\nc\n"), 0o644)
84 mustGit(t, dir, env, "add", ".")
85 mustGit(t, dir, env, "commit", "-q", "-m", "more")
86 mustGit(t, dir, env, "push", "-q", "origin", "feat")
87 if q := reviewQueue(t, inst, bobKey); len(q) != 1 || q[0] != 1 {
88 t.Fatalf("new head did not bring the request back: %v", q)
89 }
90
91 // --remove takes it out outright.
92 if _, errOut, code := inst.ssh(t, aliceKey, "", "mr", "review", "request", "alice/app", "1", "--remove", "bob"); code != 0 {
93 t.Fatalf("review request --remove: %s", errOut)
94 }
95 if q := reviewQueue(t, inst, bobKey); len(q) != 0 {
96 t.Fatalf("queue after --remove: %v", q)
97 }
98 if _, _, code := inst.ssh(t, aliceKey, "", "mr", "review", "request", "alice/app", "1", "--remove", "bob"); code != 3 {
99 t.Fatalf("removing an absent reviewer should 404, got %d", code)
100 }
101
102 // A private repository where carol has no access at all: asking her
103 // for a review is refused, not silently recorded.
104 if _, errOut, code := inst.ssh(t, aliceKey, "", "repo", "create", "alice/secret", "--private"); code != 0 {
105 t.Fatalf("repo create: %s", errOut)
106 }
107 work2 := t.TempDir()
108 mustGit(t, work2, env, "clone", inst.sshURL("alice/secret"), "w")
109 dir2 := filepath.Join(work2, "w")
110 os.WriteFile(filepath.Join(dir2, "a.txt"), []byte("a\n"), 0o644)
111 mustGit(t, dir2, env, "checkout", "-q", "-b", "main")
112 mustGit(t, dir2, env, "add", ".")
113 mustGit(t, dir2, env, "commit", "-q", "-m", "base")
114 mustGit(t, dir2, env, "push", "-q", "origin", "main")
115 mustGit(t, dir2, env, "checkout", "-q", "-b", "feat")
116 os.WriteFile(filepath.Join(dir2, "a.txt"), []byte("a\nb\n"), 0o644)
117 mustGit(t, dir2, env, "add", ".")
118 mustGit(t, dir2, env, "commit", "-q", "-m", "feat")
119 mustGit(t, dir2, env, "push", "-q", "origin", "feat")
120 if _, errOut, code := inst.ssh(t, aliceKey, "", "mr", "create", "alice/secret",
121 "--source", "feat", "--target", "main", "--title", "'private'"); code != 0 {
122 t.Fatalf("mr create: %s", errOut)
123 }
124 _, errOut, code := inst.ssh(t, aliceKey, "", "mr", "review", "request", "alice/secret", "1", "--add", "carol")
125 if code != 4 || !strings.Contains(errOut, "cannot read") {
126 t.Fatalf("requesting a review from someone with no access should be refused: exit %d, %s", code, errOut)
127 }
128}
internal/control/events.go +1
@@ -36,6 +36,7 @@ var EventKinds = []string{
36 "mr.merged", 36 "mr.merged",
37 "mr.milestoned", 37 "mr.milestoned",
38 "mr.retargeted", 38 "mr.retargeted",
39 "mr.review_requested",
39 "mr.reviewed", 40 "mr.reviewed",
40 "push", 41 "push",
41 "release.created", 42 "release.created",
internal/control/events_test.go +1 −1
@@ -12,7 +12,7 @@ import (
12// recordEventKind pulls the kind out of a RecordEvent call. The kind is 12// recordEventKind pulls the kind out of a RecordEvent call. The kind is
13// either a literal or a literal prefix concatenated with a variable, and 13// either a literal or a literal prefix concatenated with a variable, and
14// the second shape is why this reads source rather than trusting a list. 14// the second shape is why this reads source rather than trusting a list.
15var recordEventKind = regexp.MustCompile(`RecordEvent\([^,]+,\s*[^,]+,\s*"([a-z.]+)"`) 15var recordEventKind = regexp.MustCompile(`RecordEvent\([^,]+,\s*[^,]+,\s*"([a-z._]+)"`)
16 16
17// TestEventKindsAreRecorded keeps the published list and the code 17// TestEventKindsAreRecorded keeps the published list and the code
18// together: an event added without documenting it, or documented without 18// together: an event added without documenting it, or documented without
internal/control/mr.go +102 −3
@@ -76,6 +76,9 @@ func init() {
76 register(Command{Path: []string{"mr", "review"}, 76 register(Command{Path: []string{"mr", "review"},
77 Summary: "review", 77 Summary: "review",
78 Usage: "mr review <owner/name> <n> --approve|--request-changes|--comment|--discard", Run: runMRReview}) 78 Usage: "mr review <owner/name> <n> --approve|--request-changes|--comment|--discard", Run: runMRReview})
79 register(Command{Path: []string{"mr", "review", "request"},
80 Summary: "ask specific people for a review",
81 Usage: "mr review request <owner/name> <n> [--add <user>]... [--remove <user>]...", Run: runMRReviewRequest})
79 register(Command{Path: []string{"mr", "merge"}, 82 register(Command{Path: []string{"mr", "merge"},
80 Summary: "merge", 83 Summary: "merge",
81 Usage: "mr merge <owner/name> <n> [--strategy ff|merge|squash|rebase]", Run: runMRMerge}) 84 Usage: "mr merge <owner/name> <n> [--strategy ff|merge|squash|rebase]", Run: runMRMerge})
@@ -339,6 +342,8 @@ type mrOut struct {
339 Body string `json:"body,omitempty"` 342 Body string `json:"body,omitempty"`
340 BodyFormat string `json:"body_format,omitempty"` 343 BodyFormat string `json:"body_format,omitempty"`
341 Milestone string `json:"milestone,omitempty"` 344 Milestone string `json:"milestone,omitempty"`
345 // ReviewRequests is who has been asked, directly, for a review.
346 ReviewRequests []string `json:"review_requests,omitempty"`
342 // StackedOn is the open merge request whose source branch this one 347 // StackedOn is the open merge request whose source branch this one
343 // targets; Stacked are the open ones targeting this one's source. 348 // targets; Stacked are the open ones targeting this one's source.
344 StackedOn *stackRef `json:"stacked_on,omitempty"` 349 StackedOn *stackRef `json:"stacked_on,omitempty"`
@@ -391,7 +396,8 @@ func mrToOut(repo store.Repo, m store.MR, withBody bool) mrOut {
391 } 396 }
392 o := mrOut{Number: m.Number, Title: m.Title, State: m.State, Draft: m.Draft, Author: m.Author, 397 o := mrOut{Number: m.Number, Title: m.Title, State: m.State, Draft: m.Draft, Author: m.Author,
393 Source: src, TargetRef: m.TargetRef, HeadSHA: m.HeadSHA, Milestone: m.Milestone, 398 Source: src, TargetRef: m.TargetRef, HeadSHA: m.HeadSHA, Milestone: m.Milestone,
394 CreatedAt: m.CreatedAt, MergedAt: m.MergedAt, MergedBy: m.MergedBy, 399 ReviewRequests: m.ReviewRequests,
400 CreatedAt: m.CreatedAt, MergedAt: m.MergedAt, MergedBy: m.MergedBy,
395 ClosedAt: m.ClosedAt, ClosedBy: m.ClosedBy} 401 ClosedAt: m.ClosedAt, ClosedBy: m.ClosedBy}
396 if withBody { 402 if withBody {
397 o.Body = m.Body 403 o.Body = m.Body
@@ -540,6 +546,9 @@ func runMRShow(c *Ctx, args []string) int {
540 state = "draft" 546 state = "draft"
541 } 547 }
542 fmt.Fprintf(w, "!%d %s [%s] by %s\n%s -> %s @ %.10s\n", d.Number, d.Title, state, d.Author, d.Source, d.TargetRef, d.HeadSHA) 548 fmt.Fprintf(w, "!%d %s [%s] by %s\n%s -> %s @ %.10s\n", d.Number, d.Title, state, d.Author, d.Source, d.TargetRef, d.HeadSHA)
549 if len(d.ReviewRequests) > 0 {
550 fmt.Fprintf(w, "reviewers: %s\n", strings.Join(d.ReviewRequests, ", "))
551 }
543 if d.StackedOn != nil { 552 if d.StackedOn != nil {
544 fmt.Fprintf(w, "stacked on !%d %s\n", d.StackedOn.Number, d.StackedOn.Title) 553 fmt.Fprintf(w, "stacked on !%d %s\n", d.StackedOn.Number, d.StackedOn.Title)
545 } 554 }
@@ -773,6 +782,93 @@ func runMRReview(c *Ctx, args []string) int {
773 }) 782 })
774} 783}
775 784
785// runMRReviewRequest is issue assign's counterpart for merge requests: it
786// pushes a merge request into a specific person's review queue and inbox
787// directly, rather than waiting for them to be otherwise involved (#145).
788func runMRReviewRequest(c *Ctx, args []string) int {
789 rest, adds, removes, err := addRemoveFlags(args)
790 if err != nil {
791 return c.failErr(err)
792 }
793 if len(adds)+len(removes) == 0 {
794 return c.fail(protocol.ExitUsage, "usage: mr review request <owner/name> <n> [--add <user>]... [--remove <user>]...")
795 }
796 repo, mr, code := mrRef(c, rest, policy.CanWrite)
797 if code >= 0 {
798 return code
799 }
800 if code := refuseArchived(c, repo); code >= 0 {
801 return code
802 }
803 resolve := func(name string) (store.User, int) {
804 u, err := c.Store.UserByUsername(name)
805 if errors.Is(err, store.ErrNotFound) {
806 return u, c.fail(protocol.ExitNotFound, "no such user %q", name)
807 }
808 if err != nil {
809 return u, c.fail(protocol.ExitFailure, "%v", err)
810 }
811 return u, -1
812 }
813 // Notified on every return, not just success: a name later in --add
814 // that fails to resolve or lacks access must not silence the people
815 // already added earlier in the same call.
816 var added []store.User
817 defer func() {
818 if len(added) == 0 {
819 return
820 }
821 ids := make([]int64, len(added))
822 for i, u := range added {
823 ids[i] = u.ID
824 }
825 notify(c, ids, notice{repo: repo, kind: "mr",
826 subject: mrSubject(repo, mr.Number, mr.Title),
827 action: fmt.Sprintf("asked for a review on !%d", mr.Number),
828 path: fmt.Sprintf("%s/mrs/%d", repo.Path(), mr.Number)})
829 }()
830 for _, name := range adds {
831 u, code := resolve(name)
832 if code >= 0 {
833 return code
834 }
835 // A review request that lands nowhere the recipient can see it is
836 // worse than useless: it looks like the ask went through.
837 grant, err := c.Store.AccessRole(repo.ID, u.ID)
838 if err != nil {
839 return c.fail(protocol.ExitFailure, "%v", err)
840 }
841 if !policy.CanRead(u, repo, grant) {
842 return c.fail(protocol.ExitDenied, "%s cannot read %s", name, repo.Path())
843 }
844 if err := c.Store.SetMRReviewRequest(mr.ID, u.ID, true); err != nil {
845 return c.fail(protocol.ExitFailure, "%v", err)
846 }
847 added = append(added, u)
848 }
849 for _, name := range removes {
850 u, code := resolve(name)
851 if code >= 0 {
852 return code
853 }
854 if err := c.Store.SetMRReviewRequest(mr.ID, u.ID, false); err != nil {
855 if errors.Is(err, store.ErrNotFound) {
856 return c.fail(protocol.ExitNotFound, "%s is not a requested reviewer", name)
857 }
858 return c.fail(protocol.ExitFailure, "%v", err)
859 }
860 }
861 updated, err := c.Store.MRByNumber(repo.ID, mr.Number)
862 if err != nil {
863 return c.fail(protocol.ExitFailure, "%v", err)
864 }
865 c.Store.RecordEvent(repo.ID, c.User.ID, "mr.review_requested",
866 fmt.Sprintf(`{"number":%d,"reviewers":%s}`, mr.Number, jsonStrings(updated.ReviewRequests)))
867 return c.emit(map[string]any{"number": mr.Number, "reviewers": updated.ReviewRequests}, func(w io.Writer) {
868 fmt.Fprintf(w, "requested reviewers on %s!%d: %s\n", repo.Path(), mr.Number, strings.Join(updated.ReviewRequests, ", "))
869 })
870}
871
776func runMRMerge(c *Ctx, args []string) int { 872func runMRMerge(c *Ctx, args []string) int {
777 f, err := parseFlags(args, flagSpec{Values: []string{"--strategy"}, MaxPos: -1, Usage: "mr merge <owner/name> <n> [--strategy ff|merge|squash|rebase]"}) 873 f, err := parseFlags(args, flagSpec{Values: []string{"--strategy"}, MaxPos: -1, Usage: "mr merge <owner/name> <n> [--strategy ff|merge|squash|rebase]"})
778 if err != nil { 874 if err != nil {
@@ -1269,11 +1365,14 @@ func setMRDraft(c *Ctx, args []string, draft bool) int {
1269 // author, who is the actor and excluded — so notifying participants 1365 // author, who is the actor and excluded — so notifying participants
1270 // here reaches nobody, which is exactly what opening it as a draft 1366 // here reaches nobody, which is exactly what opening it as a draft
1271 // and then marking it ready would do. Opening a merge request tells 1367 // and then marking it ready would do. Opening a merge request tells
1272 // the repository; so does saying it is finally asking. 1368 // the repository; so does saying it is finally asking. A review
1369 // request made before ready — or on an earlier revision — reaches its
1370 // target here too: they are exactly who else is being asked.
1273 if !draft { 1371 if !draft {
1274 if targets, err := c.Store.RepoNotifyTargets(repo); err == nil { 1372 if targets, err := c.Store.RepoNotifyTargets(repo); err == nil {
1275 parts, _ := c.Store.MRParticipants(mr.ID) 1373 parts, _ := c.Store.MRParticipants(mr.ID)
1276 notify(c, append(targets, parts...), notice{repo: repo, kind: "mr", 1374 reviewers, _ := c.Store.MRReviewRequestIDs(mr.ID)
1375 notify(c, append(append(targets, parts...), reviewers...), notice{repo: repo, kind: "mr",
1277 subject: mrSubject(repo, mr.Number, mr.Title), 1376 subject: mrSubject(repo, mr.Number, mr.Title),
1278 action: fmt.Sprintf("marked !%d ready for review", mr.Number), 1377 action: fmt.Sprintf("marked !%d ready for review", mr.Number),
1279 path: fmt.Sprintf("%s/mrs/%d", repo.Path(), mr.Number)}) 1378 path: fmt.Sprintf("%s/mrs/%d", repo.Path(), mr.Number)})
internal/httpd/mractions.go +11
@@ -54,6 +54,17 @@ func (s *Server) mrReviewSubmit(w http.ResponseWriter, r *http.Request, u store.
54 s.done(w, r, code, msg, s.mrRedirect) 54 s.done(w, r, code, msg, s.mrRedirect)
55} 55}
56 56
57func (s *Server) mrReviewRequestSubmit(w http.ResponseWriter, r *http.Request, u store.User) {
58 args := append(fieldArgs("--add", r.FormValue("add")), fieldArgs("--remove", r.FormValue("remove"))...)
59 if len(args) == 0 {
60 s.mrRedirect(w, r, "name at least one person")
61 return
62 }
63 repo := r.PathValue("owner") + "/" + r.PathValue("repo")
64 _, msg, code := s.runControlCode(u, append([]string{"mr", "review", "request", repo, r.PathValue("n")}, args...))
65 s.done(w, r, code, msg, s.mrRedirect)
66}
67
57func (s *Server) mrMergeSubmit(w http.ResponseWriter, r *http.Request, u store.User) { 68func (s *Server) mrMergeSubmit(w http.ResponseWriter, r *http.Request, u store.User) {
58 args := []string{} 69 args := []string{}
59 if st := strings.TrimSpace(r.FormValue("strategy")); st != "" && st != "auto" { 70 if st := strings.TrimSpace(r.FormValue("strategy")); st != "" && st != "auto" {
internal/httpd/routes.go +2
@@ -167,6 +167,8 @@ func (s *Server) Routes() []Route {
167 // Review loop: each runs the matching mr command. 167 // Review loop: each runs the matching mr command.
168 Route{Method: "POST", Pattern: "/{owner}/{repo}/mrs/{n}/review", Mutating: true, 168 Route{Method: "POST", Pattern: "/{owner}/{repo}/mrs/{n}/review", Mutating: true,
169 Handler: s.checkOrigin(s.requireUser(s.mrReviewSubmit))}, 169 Handler: s.checkOrigin(s.requireUser(s.mrReviewSubmit))},
170 Route{Method: "POST", Pattern: "/{owner}/{repo}/mrs/{n}/review-request", Mutating: true,
171 Handler: s.checkOrigin(s.requireUser(s.mrReviewRequestSubmit))},
170 Route{Method: "POST", Pattern: "/{owner}/{repo}/mrs/{n}/merge", Mutating: true, 172 Route{Method: "POST", Pattern: "/{owner}/{repo}/mrs/{n}/merge", Mutating: true,
171 Handler: s.checkOrigin(s.requireUser(s.mrMergeSubmit))}, 173 Handler: s.checkOrigin(s.requireUser(s.mrMergeSubmit))},
172 Route{Method: "POST", Pattern: "/{owner}/{repo}/mrs/{n}/close", Mutating: true, 174 Route{Method: "POST", Pattern: "/{owner}/{repo}/mrs/{n}/close", Mutating: true,
internal/store/dashboard.go +65 −4
@@ -1,5 +1,7 @@
1package store 1package store
2 2
3import "sort"
4
3// DashboardItem is one open issue or MR row on the logged-in homepage. 5// DashboardItem is one open issue or MR row on the logged-in homepage.
4type DashboardItem struct { 6type DashboardItem struct {
5 RepoPath string 7 RepoPath string
@@ -127,9 +129,9 @@ func (s *Store) PinnedRepos(userID int64) ([]Repo, error) {
127 return out, rows.Err() 129 return out, rows.Err()
128} 130}
129 131
130// ReviewQueue returns open merge requests the user is involved in, has not 132// reviewQueueQuery is ReviewQueue's involved half: open merge requests the
131// authored, and has not reviewed at the current head — what the rail shows 133// user is involved in, has not authored, and has not reviewed at the
132// as waiting on them. Ordered most recently touched first. 134// current head.
133const reviewQueueQuery = ` 135const reviewQueueQuery = `
134 SELECT COALESCE(u.username, o.name) || '/' || r.name, 136 SELECT COALESCE(u.username, o.name) || '/' || r.name,
135 x.number, x.title, au.username, x.state, x.updated_at 137 x.number, x.title, au.username, x.state, x.updated_at
@@ -147,8 +149,67 @@ const reviewQueueQuery = `
147 AND ` + involvedCond + ` 149 AND ` + involvedCond + `
148 ORDER BY x.updated_at DESC LIMIT 8` 150 ORDER BY x.updated_at DESC LIMIT 8`
149 151
152// requestedReviewsQuery is ReviewQueue's other half: MRs where the user was
153// asked directly, regardless of involvement — the same exemption
154// AssignedIssues gives assignment, and for the same reason (dashboard.go
155// above). It drives from mr_review_requests rather than testing EXISTS
156// against every merge request: one user's requests are a handful, the
157// merge_requests table is the whole instance.
158const requestedReviewsQuery = `
159 SELECT COALESCE(u.username, o.name) || '/' || r.name,
160 x.number, x.title, au.username, x.state, x.updated_at
161 FROM mr_review_requests rr
162 JOIN merge_requests x ON x.id = rr.mr_id
163 JOIN repos r ON r.id = x.repo_id
164 LEFT JOIN users u ON r.owner_kind = 'user' AND u.id = r.owner_id
165 LEFT JOIN orgs o ON r.owner_kind = 'org' AND o.id = r.owner_id
166 JOIN users au ON au.id = x.author_id
167 WHERE rr.user_id = ?1
168 AND x.state IN ('open', 'source_gone')
169 AND x.draft = 0
170 AND x.author_id <> ?1
171 AND NOT EXISTS (SELECT 1 FROM mr_reviews rv
172 WHERE rv.mr_id = x.id AND rv.reviewer_id = ?1
173 AND rv.head_sha = x.head_sha)
174 ORDER BY x.updated_at DESC LIMIT 8`
175
176// ReviewQueue returns open merge requests the user is involved in, has not
177// authored, and has not reviewed at the current head — what the rail shows
178// as waiting on them — unioned with merge requests where they were asked
179// directly. Both halves drop an MR once its current head has been
180// reviewed, so a requested reviewer's queue empties the same way an
181// involved one's does. Ordered most recently touched first.
150func (s *Store) ReviewQueue(userID int64) ([]DashboardItem, error) { 182func (s *Store) ReviewQueue(userID int64) ([]DashboardItem, error) {
151 return s.dashboardQuery(reviewQueueQuery, userID) 183 involved, err := s.dashboardQuery(reviewQueueQuery, userID)
184 if err != nil {
185 return nil, err
186 }
187 requested, err := s.dashboardQuery(requestedReviewsQuery, userID)
188 if err != nil {
189 return nil, err
190 }
191 type key struct {
192 repo string
193 n int64
194 }
195 seen := make(map[key]bool, len(involved))
196 out := make([]DashboardItem, 0, len(involved)+len(requested))
197 for _, d := range involved {
198 seen[key{d.RepoPath, d.Number}] = true
199 out = append(out, d)
200 }
201 for _, d := range requested {
202 k := key{d.RepoPath, d.Number}
203 if !seen[k] {
204 seen[k] = true
205 out = append(out, d)
206 }
207 }
208 sort.SliceStable(out, func(i, j int) bool { return out[i].UpdatedAt > out[j].UpdatedAt })
209 if len(out) > 8 {
210 out = out[:8]
211 }
212 return out, nil
152} 213}
153 214
154// OpenCounts returns the repo's open issue and open merge request counts, 215// OpenCounts returns the repo's open issue and open merge request counts,
internal/store/dashboardplan_test.go +1
@@ -54,6 +54,7 @@ func TestDashboardQueriesUseIndexes(t *testing.T) {
54 {"DashboardMRs", queryPlan(t, s, dashboardMRsQuery, int64(1)), "merge_requests_recent", true}, 54 {"DashboardMRs", queryPlan(t, s, dashboardMRsQuery, int64(1)), "merge_requests_recent", true},
55 {"ReviewQueue", queryPlan(t, s, reviewQueueQuery, int64(1)), "merge_requests_recent", true}, 55 {"ReviewQueue", queryPlan(t, s, reviewQueueQuery, int64(1)), "merge_requests_recent", true},
56 {"AssignedIssues", queryPlan(t, s, assignedIssuesQuery, int64(1)), "issue_assignees_user", false}, 56 {"AssignedIssues", queryPlan(t, s, assignedIssuesQuery, int64(1)), "issue_assignees_user", false},
57 {"RequestedReviews", queryPlan(t, s, requestedReviewsQuery, int64(1)), "mr_review_requests_user", false},
57 } 58 }
58 for _, tc := range cases { 59 for _, tc := range cases {
59 if !strings.Contains(tc.plan, tc.want) { 60 if !strings.Contains(tc.plan, tc.want) {
internal/store/migrations/0042_mr_review_requests.down.sql added +2
@@ -0,0 +1,2 @@
1DROP INDEX mr_review_requests_user;
2DROP TABLE mr_review_requests;
internal/store/migrations/0042_mr_review_requests.up.sql added +11
@@ -0,0 +1,11 @@
1-- Requesting a review is a direct push to a specific person, the merge
2-- request counterpart of issue_assignees (0001).
3CREATE TABLE mr_review_requests (
4 mr_id INTEGER NOT NULL REFERENCES merge_requests(id) ON DELETE CASCADE,
5 user_id INTEGER NOT NULL REFERENCES users(id) ON DELETE CASCADE,
6 PRIMARY KEY (mr_id, user_id)
7);
8
9-- Same problem as issue_assignees (0035): the primary key leads with
10-- mr_id, so the review queue had no way in by user.
11CREATE INDEX mr_review_requests_user ON mr_review_requests(user_id);
internal/store/mrs.go +35
@@ -31,6 +31,9 @@ type MR struct {
31 ClosedBy string 31 ClosedBy string
32 CreatedAt string 32 CreatedAt string
33 UpdatedAt string 33 UpdatedAt string
34 // ReviewRequests is who has been asked, directly, for a review — the
35 // mr review request counterpart of Issue.Assignees.
36 ReviewRequests []string
34} 37}
35 38
36type MRReview struct { 39type MRReview struct {
@@ -112,9 +115,41 @@ func (s *Store) MRByNumber(repoID, number int64) (MR, error) {
112 if errors.Is(err, sql.ErrNoRows) { 115 if errors.Is(err, sql.ErrNoRows) {
113 return m, ErrNotFound 116 return m, ErrNotFound
114 } 117 }
118 if err != nil {
119 return m, err
120 }
121 m.ReviewRequests, err = s.issueStrings(m.ID, `
122 SELECT u.username FROM mr_review_requests rr JOIN users u ON u.id = rr.user_id
123 WHERE rr.mr_id = ? ORDER BY u.username`)
115 return m, err 124 return m, err
116} 125}
117 126
127// SetMRReviewRequest adds or removes a review request by user id — the
128// mr review request counterpart of SetIssueAssignee.
129func (s *Store) SetMRReviewRequest(mrID, userID int64, add bool) error {
130 if add {
131 _, err := s.DB.Exec(
132 "INSERT INTO mr_review_requests (mr_id, user_id) VALUES (?, ?) ON CONFLICT DO NOTHING",
133 mrID, userID)
134 return err
135 }
136 res, err := s.DB.Exec(
137 "DELETE FROM mr_review_requests WHERE mr_id = ? AND user_id = ?", mrID, userID)
138 if err != nil {
139 return err
140 }
141 if n, _ := res.RowsAffected(); n == 0 {
142 return ErrNotFound
143 }
144 return nil
145}
146
147// MRReviewRequestIDs returns who has been asked for a review, by id — for
148// notifying them without a username round trip.
149func (s *Store) MRReviewRequestIDs(mrID int64) ([]int64, error) {
150 return s.idQuery("SELECT user_id FROM mr_review_requests WHERE mr_id = ?", mrID)
151}
152
118// ListMRs returns merge requests for a repo. limit 0 means everything; 153// ListMRs returns merge requests for a repo. limit 0 means everything;
119// before (an MR number) starts the page strictly below it, matching the 154// before (an MR number) starts the page strictly below it, matching the
120// number-descending order. 155// number-descending order.
internal/store/reviewrequests_test.go added +73
@@ -0,0 +1,73 @@
1package store
2
3import "testing"
4
5// A requested reviewer sees the merge request in their queue even with no
6// other tie to the repository, and the queue empties once they have
7// reviewed the current head — the two rules #145 requires together. A new
8// push brings it back, and --remove drops it outright.
9func TestReviewQueueRequestedReviewer(t *testing.T) {
10 s, repoID, _ := mrFixture(t)
11 mr, err := s.MRByNumber(repoID, 1)
12 if err != nil {
13 t.Fatal(err)
14 }
15 reviewerID, err := s.CreateUser("dana", false)
16 if err != nil {
17 t.Fatal(err)
18 }
19
20 if q, err := s.ReviewQueue(reviewerID); err != nil || len(q) != 0 {
21 t.Fatalf("queue before any request: %+v, %v", q, err)
22 }
23
24 if err := s.SetMRReviewRequest(mr.ID, reviewerID, true); err != nil {
25 t.Fatal(err)
26 }
27 q, err := s.ReviewQueue(reviewerID)
28 if err != nil || len(q) != 1 || q[0].Number != mr.Number {
29 t.Fatalf("requested reviewer not in queue: %+v, %v", q, err)
30 }
31
32 if err := s.AddMRReview(mr.ID, reviewerID, "approve", mr.HeadSHA); err != nil {
33 t.Fatal(err)
34 }
35 if q, err := s.ReviewQueue(reviewerID); err != nil || len(q) != 0 {
36 t.Fatalf("queue after reviewing the current head: %+v, %v", q, err)
37 }
38
39 if err := s.UpdateMRHead(mr.ID, "def456", ""); err != nil {
40 t.Fatal(err)
41 }
42 if q, err := s.ReviewQueue(reviewerID); err != nil || len(q) != 1 {
43 t.Fatalf("queue after a new head: %+v, %v", q, err)
44 }
45
46 if err := s.SetMRReviewRequest(mr.ID, reviewerID, false); err != nil {
47 t.Fatal(err)
48 }
49 if q, err := s.ReviewQueue(reviewerID); err != nil || len(q) != 0 {
50 t.Fatalf("queue after --remove: %+v, %v", q, err)
51 }
52 if err := s.SetMRReviewRequest(mr.ID, reviewerID, false); err != ErrNotFound {
53 t.Fatalf("removing an absent request: %v", err)
54 }
55}
56
57// The involved half of the queue never shows an author their own merge
58// request (reviewQueueQuery's author_id <> ?1); the requested half must
59// hold the same line even if the author is somehow added as a requested
60// reviewer on their own MR.
61func TestReviewQueueExcludesAuthor(t *testing.T) {
62 s, repoID, authorID := mrFixture(t)
63 mr, err := s.MRByNumber(repoID, 1)
64 if err != nil {
65 t.Fatal(err)
66 }
67 if err := s.SetMRReviewRequest(mr.ID, authorID, true); err != nil {
68 t.Fatal(err)
69 }
70 if q, err := s.ReviewQueue(authorID); err != nil || len(q) != 0 {
71 t.Fatalf("author requested on their own MR should not see it in queue: %+v, %v", q, err)
72 }
73}
internal/web/templates/mr.html +12
@@ -126,6 +126,18 @@
126 <p class="row none">Retargeting stales existing reviews.</p> 126 <p class="row none">Retargeting stales existing reviews.</p>
127 </div> 127 </div>
128 {{end}} 128 {{end}}
129 <div class="grp">
130 <h2>Reviewers</h2>
131 {{if .MR.ReviewRequests}}<p class="row">{{range .MR.ReviewRequests}}<a href="/{{.}}">{{.}}</a> {{end}}</p>
132 {{else}}<p class="none">Nobody asked yet</p>{{end}}
133 {{if and .CanWrite (or (eq .MR.State "open") (eq .MR.State "source_gone"))}}
134 <form method="post" action="{{$base}}/review-request" class="actions">
135 <input type="text" name="add" aria-label="Add reviewers" placeholder="add, space-separated">
136 <input type="text" name="remove" aria-label="Remove reviewers" placeholder="remove">
137 <button type="submit">Apply</button>
138 </form>
139 {{end}}
140 </div>
129 <div class="grp"> 141 <div class="grp">
130 <h2>Reviews</h2> 142 <h2>Reviews</h2>
131 {{range .Reviews}}<p class="row"><span class="dot {{if eq .Verdict "approve"}}ok{{else}}pend{{end}}"></span><a href="/{{.Reviewer}}">{{.Reviewer}}</a> {{.Verdict}}{{if .Stale}} <span class="chip chip-stale">stale</span>{{end}}{{if not .Counts}} <span class="chip chip-neutral" title="This reviewer has no write access, so the merge gates do not count it">advisory</span>{{end}}<span class="sub">{{when .CreatedAt}}</span></p> 143 {{range .Reviews}}<p class="row"><span class="dot {{if eq .Verdict "approve"}}ok{{else}}pend{{end}}"></span><a href="/{{.Reviewer}}">{{.Reviewer}}</a> {{.Verdict}}{{if .Stale}} <span class="chip chip-stale">stale</span>{{end}}{{if not .Counts}} <span class="chip chip-neutral" title="This reviewer has no write access, so the merge gates do not count it">advisory</span>{{end}}<span class="sub">{{when .CreatedAt}}</span></p>