krz/gitbay

web: review, resolve, merge, and close on the merge request page !66

merged merged by cmc on 2026-08-26 02:42 UTC · krz/gitbay:mr-web-actions into main

8 files changed, +369 −3

Layout: unified · split

e2e/mrweb_test.go added +157
@@ -0,0 +1,157 @@
1package e2e
2
3import (
4 "encoding/json"
5 "net/http"
6 "net/url"
7 "os"
8 "path/filepath"
9 "regexp"
10 "strings"
11 "testing"
12)
13
14// login returns a browser holding a session for the given key's account.
15func (i *instance) login(t *testing.T, key string) *http.Client {
16 t.Helper()
17 out, errOut, code := i.ssh(t, key, "", "web", "login", "--json")
18 if code != 0 {
19 t.Fatalf("web login: %s", errOut)
20 }
21 var env struct {
22 Data struct {
23 URL string `json:"url"`
24 } `json:"data"`
25 }
26 json.Unmarshal([]byte(out), &env)
27 c := newBrowser(t)
28 path := env.Data.URL[strings.Index(env.Data.URL, "/login"):]
29 if status, _ := browserGet(t, c, i.base()+path); status != 200 {
30 t.Fatalf("login landed: %d", status)
31 }
32 return c
33}
34
35// TestMRWebReviewLoop drives review, thread resolution, and merge from the
36// browser. Every action runs the same control command the CLI runs, so the
37// test also proves the merge gates apply to web merges.
38func TestMRWebReviewLoop(t *testing.T) {
39 inst := startInstanceWith(t, "[web]\nmode = \"accounts\"\n")
40 aliceKey := inst.newKey(t, "alice")
41 bobKey := inst.newKey(t, "bob")
42 inst.admin(t, "admin", "user", "create", "alice",
43 "--key", aliceKey+".pub", "--email", "alice@example.test", "--verified")
44 inst.admin(t, "admin", "user", "create", "bob",
45 "--key", bobKey+".pub", "--email", "bob@example.test", "--verified")
46
47 if _, errOut, code := inst.ssh(t, aliceKey, "", "repo", "create", "alice/lib"); code != 0 {
48 t.Fatalf("repo create: %s", errOut)
49 }
50 if _, errOut, code := inst.ssh(t, aliceKey, "", "repo", "access", "grant", "alice/lib", "bob", "write"); code != 0 {
51 t.Fatalf("grant: %s", errOut)
52 }
53 // Unresolved review threads block merges, so the gate is observable.
54 if _, errOut, code := inst.ssh(t, aliceKey, "", "repo", "settings", "require-resolved", "alice/lib", "on"); code != 0 {
55 t.Fatalf("require-resolved: %s", errOut)
56 }
57
58 env := inst.gitEnv(aliceKey)
59 work := t.TempDir()
60 mustGit(t, work, env, "clone", inst.sshURL("alice/lib"), "w")
61 dir := filepath.Join(work, "w")
62 os.WriteFile(filepath.Join(dir, "lib.txt"), []byte("v1\n"), 0o644)
63 mustGit(t, dir, env, "checkout", "-q", "-b", "main")
64 mustGit(t, dir, env, "add", ".")
65 mustGit(t, dir, env, "commit", "-q", "-m", "base")
66 mustGit(t, dir, env, "push", "-q", "origin", "main")
67
68 // Bob proposes a change and leaves a review thread on it.
69 bobEnv := inst.gitEnv(bobKey)
70 bobWork := t.TempDir()
71 mustGit(t, bobWork, bobEnv, "clone", inst.sshURL("alice/lib"), "w")
72 bobDir := filepath.Join(bobWork, "w")
73 mustGit(t, bobDir, bobEnv, "checkout", "-q", "-b", "feature", "origin/main")
74 os.WriteFile(filepath.Join(bobDir, "feature.txt"), []byte("bob's work\n"), 0o644)
75 mustGit(t, bobDir, bobEnv, "add", ".")
76 mustGit(t, bobDir, bobEnv, "commit", "-q", "-m", "add feature")
77 mustGit(t, bobDir, bobEnv, "push", "-q", "origin", "feature")
78 if _, errOut, code := inst.ssh(t, bobKey, "", "mr", "create", "alice/lib",
79 "--source", "feature", "--target", "main", "--title", "'add feature'"); code != 0 {
80 t.Fatalf("mr create: %s", errOut)
81 }
82 if _, errOut, code := inst.ssh(t, bobKey, "", "mr", "diff-comment", "alice/lib", "1",
83 "--path", "feature.txt", "--line", "1", "--message", "'is this right?'"); code != 0 {
84 t.Fatalf("diff-comment: %s", errOut)
85 }
86
87 mrURL := inst.base() + "/alice/lib/mrs/1"
88 alice := inst.login(t, aliceKey)
89
90 // The controls are on the page, and carry the thread to resolve.
91 _, body := browserGet(t, alice, mrURL)
92 for _, want := range []string{`value="approve"`, `action="/alice/lib/mrs/1/merge"`} {
93 if !strings.Contains(body, want) {
94 t.Fatalf("MR page missing %q", want)
95 }
96 }
97 // Review threads live on the diff view, where their lines are.
98 _, diffBody := browserGet(t, alice, mrURL+"?view=diff")
99 m := regexp.MustCompile(`name="thread" value="(\d+)"`).FindStringSubmatch(diffBody)
100 if m == nil {
101 t.Fatalf("no thread control on the diff view:\n%s", diffBody)
102 }
103 threadID := m[1]
104
105 // Approve from the browser; the CLI sees the review.
106 if status, _ := browserPost(t, alice, mrURL+"/review", url.Values{"verdict": {"approve"}}); status != 200 {
107 t.Fatalf("review post: %d", status)
108 }
109 show := inst.mrShow(t, aliceKey, "alice/lib", "1")
110 if len(show.Reviews) != 1 || show.Reviews[0].Reviewer != "alice" || show.Reviews[0].Verdict != "approve" {
111 t.Fatalf("review not recorded: %+v", show.Reviews)
112 }
113
114 // Merging is refused while the thread is open, and the page says why.
115 _, body = browserPost(t, alice, mrURL+"/merge", url.Values{"strategy": {"auto"}})
116 if !strings.Contains(body, "unresolved") {
117 t.Fatalf("merge gate not surfaced:\n%s", body)
118 }
119 if st := inst.mrShow(t, aliceKey, "alice/lib", "1").State; st != "open" {
120 t.Fatalf("blocked merge changed state to %s", st)
121 }
122
123 // Resolve the thread, then merge.
124 if status, _ := browserPost(t, alice, mrURL+"/thread",
125 url.Values{"thread": {threadID}, "action": {"resolve"}}); status != 200 {
126 t.Fatalf("resolve post: %d", status)
127 }
128 out, _, _ := inst.ssh(t, aliceKey, "", "mr", "threads", "alice/lib", "1")
129 if !strings.Contains(out, "resolved") {
130 t.Fatalf("thread not resolved:\n%s", out)
131 }
132 if status, _ := browserPost(t, alice, mrURL+"/merge", url.Values{"strategy": {"auto"}}); status != 200 {
133 t.Fatalf("merge post: %d", status)
134 }
135 if st := inst.mrShow(t, aliceKey, "alice/lib", "1").State; st != "merged" {
136 t.Fatalf("MR state after web merge: %s", st)
137 }
138 mustGit(t, dir, env, "pull", "-q", "origin", "main")
139 if _, err := os.Stat(filepath.Join(dir, "feature.txt")); err != nil {
140 t.Fatal("merged content missing from main")
141 }
142
143 // Readers get no controls, and a forged POST is refused by the command.
144 _, anon := browserGet(t, newBrowser(t), mrURL)
145 if strings.Contains(anon, `value="approve"`) {
146 t.Fatal("anonymous visitor sees review controls")
147 }
148 carol := inst.newKey(t, "carol")
149 inst.admin(t, "admin", "user", "create", "carol", "--key", carol+".pub")
150 if _, errOut, code := inst.ssh(t, carol, "", "repo", "create", "carol/own"); code != 0 {
151 t.Fatalf("carol repo: %s", errOut)
152 }
153 _, denied := browserPost(t, inst.login(t, carol), mrURL+"/close", url.Values{})
154 if !strings.Contains(denied, `class="error"`) || !strings.Contains(denied, "write access") {
155 t.Fatalf("reader was not refused:\n%s", denied)
156 }
157}
internal/httpd/control.go added +38
@@ -0,0 +1,38 @@
1package httpd
2
3import (
4 "bytes"
5 "strings"
6
7 "gitbay.org/gitbay/internal/control"
8 "gitbay.org/gitbay/internal/protocol"
9 "gitbay.org/gitbay/internal/store"
10)
11
12// runControl executes a control command as the browser session's user,
13// through the same registry the CLI and the JSON API reach. Web writes
14// never reimplement command logic — merge gates, review rules, and audit
15// entries stay in one place — so the surfaces cannot drift apart.
16//
17// ViaAPI is set, which refuses SSHOnly commands: anything whose input is a
18// credential (secrets, mirror tokens, session minting) stays on SSH.
19func (s *Server) runControl(u store.User, argv []string) (out string, msg string, ok bool) {
20 var stdout, stderr bytes.Buffer
21 ctx := &control.Ctx{
22 User: u,
23 Source: "web",
24 Scope: "full",
25 Store: s.st,
26 Cfg: s.cfg,
27 Stdin: strings.NewReader(""),
28 Stdout: &stdout,
29 Stderr: &stderr,
30 ViaAPI: true,
31 }
32 code := control.Dispatch(ctx, argv)
33 m := strings.TrimSpace(stderr.String())
34 if m == "" {
35 m = strings.TrimSpace(stdout.String())
36 }
37 return stdout.String(), m, code == protocol.ExitOK
38}
internal/httpd/mractions.go added +91
@@ -0,0 +1,91 @@
1package httpd
2
3import (
4 "fmt"
5 "net/http"
6 "net/url"
7 "strconv"
8 "strings"
9
10 "gitbay.org/gitbay/internal/store"
11)
12
13// Merge request actions. Each one runs the control command the CLI runs,
14// so review rules, merge gates, and audit entries have a single
15// implementation; the browser only chooses arguments and shows the
16// result.
17
18// mrRedirect returns to the merge request, carrying a failure message the
19// page renders as a banner.
20func (s *Server) mrRedirect(w http.ResponseWriter, r *http.Request, msg string) {
21 dest := fmt.Sprintf("/%s/%s/mrs/%s",
22 r.PathValue("owner"), r.PathValue("repo"), r.PathValue("n"))
23 if msg != "" {
24 if len(msg) > 300 {
25 msg = msg[:300]
26 }
27 dest += "?e=" + url.QueryEscape(msg)
28 }
29 http.Redirect(w, r, dest, http.StatusSeeOther)
30}
31
32// mrArgs builds "<verb> owner/name <n>" for the mr command family.
33func mrArgs(r *http.Request, verb string, extra ...string) []string {
34 repo := r.PathValue("owner") + "/" + r.PathValue("repo")
35 return append([]string{"mr", verb, repo, r.PathValue("n")}, extra...)
36}
37
38func (s *Server) mrReviewSubmit(w http.ResponseWriter, r *http.Request, u store.User) {
39 flag := map[string]string{
40 "approve": "--approve",
41 "request-changes": "--request-changes",
42 "comment": "--comment",
43 }[r.FormValue("verdict")]
44 if flag == "" {
45 s.mrRedirect(w, r, "pick approve, request changes, or comment")
46 return
47 }
48 _, msg, ok := s.runControl(u, mrArgs(r, "review", flag))
49 if ok {
50 msg = ""
51 }
52 s.mrRedirect(w, r, msg)
53}
54
55func (s *Server) mrMergeSubmit(w http.ResponseWriter, r *http.Request, u store.User) {
56 args := []string{}
57 if st := strings.TrimSpace(r.FormValue("strategy")); st != "" && st != "auto" {
58 args = append(args, "--strategy", st)
59 }
60 _, msg, ok := s.runControl(u, mrArgs(r, "merge", args...))
61 if ok {
62 msg = ""
63 }
64 s.mrRedirect(w, r, msg)
65}
66
67func (s *Server) mrCloseSubmit(w http.ResponseWriter, r *http.Request, u store.User) {
68 _, msg, ok := s.runControl(u, mrArgs(r, "close"))
69 if ok {
70 msg = ""
71 }
72 s.mrRedirect(w, r, msg)
73}
74
75// mrThreadSubmit resolves or reopens one review thread.
76func (s *Server) mrThreadSubmit(w http.ResponseWriter, r *http.Request, u store.User) {
77 verb := "resolve"
78 if r.FormValue("action") == "unresolve" {
79 verb = "unresolve"
80 }
81 id := strings.TrimSpace(r.FormValue("thread"))
82 if _, err := strconv.ParseInt(id, 10, 64); err != nil {
83 s.mrRedirect(w, r, "bad thread id")
84 return
85 }
86 _, msg, ok := s.runControl(u, mrArgs(r, verb, id))
87 if ok {
88 msg = ""
89 }
90 s.mrRedirect(w, r, msg)
91}
internal/httpd/routes.go +9
@@ -114,6 +114,15 @@ func (s *Server) Routes() []Route {
114114 Handler: s.checkOrigin(s.requireUser(s.mrEditSubmit))},
115115 Route{Method: "POST", Pattern: "/{owner}/{repo}/mrs/{n}/comment", Mutating: true,
116116 Handler: s.checkOrigin(s.requireUser(s.mrCommentSubmit))},
117 // Review loop: each runs the matching mr command.
118 Route{Method: "POST", Pattern: "/{owner}/{repo}/mrs/{n}/review", Mutating: true,
119 Handler: s.checkOrigin(s.requireUser(s.mrReviewSubmit))},
120 Route{Method: "POST", Pattern: "/{owner}/{repo}/mrs/{n}/merge", Mutating: true,
121 Handler: s.checkOrigin(s.requireUser(s.mrMergeSubmit))},
122 Route{Method: "POST", Pattern: "/{owner}/{repo}/mrs/{n}/close", Mutating: true,
123 Handler: s.checkOrigin(s.requireUser(s.mrCloseSubmit))},
124 Route{Method: "POST", Pattern: "/{owner}/{repo}/mrs/{n}/thread", Mutating: true,
125 Handler: s.checkOrigin(s.requireUser(s.mrThreadSubmit))},
117126 Route{Method: "GET", Pattern: "/{owner}/{repo}/edit/{ref}/{path...}",
118127 Handler: s.requireUser(s.editForm)},
119128 Route{Method: "POST", Pattern: "/{owner}/{repo}/edit/{ref}/{path...}", Mutating: true,
internal/httpd/web.go +20 −1
@@ -1361,6 +1361,20 @@ func (s *Server) issue(w http.ResponseWriter, r *http.Request) {
13611361}
13621362
13631363// canEditItem: the author or anyone with write access may edit.
1364// canWriteRepo reports whether the browser session may push to the repo,
1365// which is what gates the review and merge controls.
1366func (s *Server) canWriteRepo(r *http.Request, repo store.Repo) bool {
1367 if s.cfg.Web.Mode != "accounts" {
1368 return false
1369 }
1370 u := s.viewer(r)
1371 if u.ID == 0 {
1372 return false
1373 }
1374 grant, _ := s.st.AccessRole(repo.ID, u.ID)
1375 return policy.CanWrite(u, repo, grant)
1376}
1377
13641378func (s *Server) canEditItem(r *http.Request, repo store.Repo, author string) bool {
13651379 if s.cfg.Web.Mode != "accounts" {
13661380 return false
@@ -1480,6 +1494,7 @@ func (s *Server) mr(w http.ResponseWriter, r *http.Request) {
14801494 // The diff is the reason most people open a merge request, so it gets
14811495 // its own view rather than a fold at the foot of the conversation.
14821496 // A query parameter keeps this working without JavaScript.
1497 unresolved, _ := s.st.UnresolvedThreadCount(m.ID)
14831498 view := r.URL.Query().Get("view")
14841499 if view != "commits" && view != "diff" {
14851500 view = "conversation"
@@ -1497,9 +1512,13 @@ func (s *Server) mr(w http.ResponseWriter, r *http.Request) {
14971512 Stat diffStat
14981513 Commits []commitRow
14991514 CanEdit bool
1515 CanWrite bool
1516 Unresolved int
1517 Notice string
15001518 DetachedThreads []diffThread
15011519 }{p, m, view, md(m.Body), checks, store.CombinedStatus(checks), renderComments(comments, md),
1502 reviews, lines, stat, commits, s.canEditItem(r, p.Repo, m.Author), detachedThreads})
1520 reviews, lines, stat, commits, s.canEditItem(r, p.Repo, m.Author),
1521 s.canWriteRepo(r, p.Repo), unresolved, r.URL.Query().Get("e"), detachedThreads})
15031522}
15041523
15051524func (s *Server) refs(w http.ResponseWriter, r *http.Request) {
internal/web/static/style.css +11
@@ -540,6 +540,17 @@ code.fullsha { color: var(--muted); overflow-wrap: anywhere; }
540540 padding: var(--sp-2) var(--sp-3);
541541 margin-bottom: var(--sp-4);
542542}
543/* merge request actions in the aside: stacked controls, full width */
544.aside form.actions {
545 display: flex;
546 flex-wrap: wrap;
547 gap: var(--sp-2);
548 margin-top: var(--sp-2);
549}
550.aside form.actions button { flex: 1 1 auto; }
551.aside form.actions select { width: 100%; }
552form.threadact { margin: var(--sp-1) 0 0; padding: 0 var(--sp-3) var(--sp-2); }
553
543554.meta { color: var(--muted); font-size: var(--fs-1); }
544555svg.icon { vertical-align: -0.125em; }
545556.lede { font-size: var(--fs-3); margin: var(--sp-2) 0; }
internal/web/templates/mr.html +33 −2
@@ -5,6 +5,8 @@
55<p class="issuemeta"><span class="chip chip-{{.MR.State}}">{{.MR.State}}</span>
66<a href="/{{.MR.Author}}">{{.MR.Author}}</a> wants to merge <code>{{if .MR.SourcePath}}{{.MR.SourcePath}}:{{end}}{{.MR.SourceRef}}</code> into <code>{{.MR.TargetRef}}</code></p>
77
8{{if .Notice}}<p class="error" role="alert">{{.Notice}}</p>{{end}}
9
810<div class="withaside">
911<div class="mainside">
1012
@@ -33,7 +35,7 @@
3335</article>{{end}}
3436{{end}}
3537{{if .DetachedThreads}}<h2>Threads on earlier revisions</h2>
36{{range .DetachedThreads}}<div class="thread stale"><p class="threadstate">{{if .Stale}}stale{{end}}{{if .Resolved}}{{if .Stale}} · {{end}}resolved by {{.Resolved}}{{end}}</p>{{range .Comments}}<p class="commenthead"><strong>{{.Author}}</strong> <span class="when">{{when .CreatedAt}}</span></p><div class="rendered">{{.BodyHTML}}</div>{{end}}</div>{{end}}{{end}}
38{{range .DetachedThreads}}<div class="thread stale"><p class="threadstate">{{if .Stale}}stale{{end}}{{if .Resolved}}{{if .Stale}} · {{end}}resolved by {{.Resolved}}{{end}}</p>{{range .Comments}}<p class="commenthead"><strong>{{.Author}}</strong> <span class="when">{{when .CreatedAt}}</span></p><div class="rendered">{{.BodyHTML}}</div>{{end}}{{template "threadact" dict "ID" .ID "Resolved" .Resolved "Base" $base "Can" $.CanWrite}}</div>{{end}}{{end}}
3739{{if .Viewer}}
3840<form method="post" action="{{$base}}/comment" class="commentform">
3941<p><textarea name="body" aria-label="Comment" rows="4" placeholder="Comment as {{.Viewer}}"></textarea></p>
@@ -60,12 +62,39 @@
6062{{else}}
6163<p class="diffstat">{{.Stat.Files}} file{{if ne .Stat.Files 1}}s{{end}} changed, <span class="add">+{{.Stat.Adds}}</span> <span class="del">−{{.Stat.Dels}}</span></p>
6264<pre class="diff">{{range .DiffLines}}<span class="{{.Class}}">{{.Text}}</span>
63{{range .Threads}}</pre><div class="thread{{if .Resolved}} resolved{{end}}">{{if .Resolved}}<p class="threadstate">resolved by {{.Resolved}}</p>{{end}}{{range .Comments}}<p class="commenthead"><strong>{{.Author}}</strong> <span class="when">{{when .CreatedAt}}</span></p><div class="rendered">{{.BodyHTML}}</div>{{end}}</div><pre class="diff">{{end}}{{end}}</pre>
65{{range .Threads}}</pre><div class="thread{{if .Resolved}} resolved{{end}}">{{if .Resolved}}<p class="threadstate">resolved by {{.Resolved}}</p>{{end}}{{range .Comments}}<p class="commenthead"><strong>{{.Author}}</strong> <span class="when">{{when .CreatedAt}}</span></p><div class="rendered">{{.BodyHTML}}</div>{{end}}{{template "threadact" dict "ID" .ID "Resolved" .Resolved "Base" $base "Can" $.CanWrite}}</div><pre class="diff">{{end}}{{end}}</pre>
6466{{end}}
6567
6668</div>
6769
6870<aside class="aside">
71 {{if and .CanWrite (or (eq .MR.State "open") (eq .MR.State "source_gone"))}}
72 <div class="grp">
73 <h2>Review</h2>
74 <form method="post" action="{{$base}}/review" class="actions">
75 <button type="submit" name="verdict" value="approve">Approve</button>
76 <button type="submit" name="verdict" value="request-changes">Request changes</button>
77 </form>
78 </div>
79 <div class="grp">
80 <h2>Merge</h2>
81 {{if .Unresolved}}<p class="row none">{{.Unresolved}} unresolved thread{{if ne .Unresolved 1}}s{{end}}</p>{{end}}
82 <form method="post" action="{{$base}}/merge" class="actions">
83 <label class="none" for="strategy">Strategy</label>
84 <select id="strategy" name="strategy">
85 <option value="auto">Automatic</option>
86 <option value="ff">Fast-forward</option>
87 <option value="merge">Merge commit</option>
88 <option value="squash">Squash</option>
89 <option value="rebase">Rebase</option>
90 </select>
91 <button type="submit" class="primary">Merge</button>
92 </form>
93 <form method="post" action="{{$base}}/close" class="actions">
94 <button type="submit">Close without merging</button>
95 </form>
96 </div>
97 {{end}}
6998 <div class="grp">
7099 <h2>Reviews</h2>
71100 {{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}}</p>
@@ -89,3 +118,5 @@
89118</aside>
90119</div>
91120{{end}}
121
122{{define "threadact"}}{{if .Can}}<form method="post" action="{{.Base}}/thread" class="threadact"><input type="hidden" name="thread" value="{{.ID}}"><button type="submit" name="action" value="{{if .Resolved}}unresolve{{else}}resolve{{end}}" class="linklike">{{if .Resolved}}Reopen thread{{else}}Resolve thread{{end}}</button></form>{{end}}{{end}}
internal/web/web.go +10
@@ -76,6 +76,16 @@ var funcs = template.FuncMap{
7676 }
7777 return s
7878 },
79 // dict builds a map for {{template}} calls that need several values.
80 "dict": func(pairs ...any) map[string]any {
81 m := map[string]any{}
82 for i := 0; i+1 < len(pairs); i += 2 {
83 if k, ok := pairs[i].(string); ok {
84 m[k] = pairs[i+1]
85 }
86 }
87 return m
88 },
7989 "add": func(a, b int) int { return a + b },
8090 "sub": func(a, b int) int { return a - b },
8191 // topTab maps a page's Tab to the repo header tab that should read as