✨ (merge-requests): Add line-level review comments on the diff
Changes
10 files changed, +829 -24
MODIFY
README.md
+1 -0
@@ -22,6 +22,7 @@
22
22
- Merge requests move through the lifecycle Open → Merged / Closed; the repo navigation and shared tab bar show the open merge-request count, and merged/closed ones collapse into an "Archive" section on the list page (same pattern as issues)
23
23
- The merge request detail page renders the live diff of the source branch relative to the merge base with the target (three-dot diff), file by file with per-line add/delete coloring and a changed-files / +additions / −deletions summary — always computed live from git, never duplicated into the database
24
24
- The owner can Merge or Close an open merge request from the detail page; merging runs entirely in-core against the bare repository (no working tree), fast-forwarding when possible or else recording a two-parent merge commit authored by the acting user and advancing the target branch ref. An automatic merge that would conflict is rejected; a source branch already contained in the target is treated as already merged
25
+- Line-level review comments on a merge request's diff: any authenticated user who can read the repository can comment on a specific diff line (added, deleted, or context) from the merge-request detail page; comments render inline beneath the line they anchor to. A comment can be deleted by its author or by the repository owner. Comments are anchored to a file plus the diff line's old/new line numbers and must land on a line that's part of the current diff. The comment form is a progressive-enhancement `<details>` disclosure and works without JavaScript
25
26
- OIDC login (authorization code flow) via `GET /login`; on first login the user account is created without a username and the browser is redirected to `/onboarding`, where the user picks a URL-safe handle (`^[a-z0-9][a-z0-9-]{0,38}$`, unique). The chosen handle — not the OIDC `preferred_username` claim (which is an SPN form in kanidm and not URL-safe) — is used in all repo, SSH, ActivityPub, and webfinger URLs. The `name` claim becomes an editable display name; both can be changed later at `/settings/profile`. A request filter blocks all app pages until a handle is chosen. Logout is local-session only via `POST /logout` (the kanidm provider advertises no `end_session_endpoint`, so RP-Initiated Logout is disabled)
26
27
- Single access policy on all paths: owner read/write, public world-readable, private owner-only
27
28
- **Federation (ForgeFed / ActivityPub)** — *opt-in, off by default.* Public repositories are
MODIFY
src/main/java/de/workaround/git/GitMergeService.java
+27 -3
@@ -8,6 +8,8 @@
8
8
import java.util.ArrayList;
9
9
import java.util.List;
10
10
import java.util.Optional;
11
+import java.util.regex.Matcher;
12
+import java.util.regex.Pattern;
11
13
12
14
import org.eclipse.jgit.diff.DiffEntry;
13
15
import org.eclipse.jgit.diff.DiffFormatter;
@@ -40,8 +42,16 @@
40
42
@ApplicationScoped
41
43
public class GitMergeService
42
44
{
43
- /** A single line of a unified diff, tagged so the UI can colour it without re-parsing the patch. */
44
- public record DiffLine(String type, String text)
45
+ /** Matches a unified-diff hunk header, capturing the 1-based old and new starting line numbers. */
46
+ private static final Pattern HUNK_HEADER = Pattern.compile("^@@ -(\\d+)(?:,\\d+)? \\+(\\d+)(?:,\\d+)? @@");
47
+
48
+ /**
49
+ * A single line of a unified diff, tagged so the UI can colour it without re-parsing the patch. {@code oldLine}
50
+ * and {@code newLine} are the 1-based line numbers on the old and new side, or {@code -1} where the line has no
51
+ * counterpart (added lines have no old number, deleted lines no new number, meta/hunk lines have neither). The
52
+ * {@code (oldLine, newLine)} pair uniquely anchors a comment to a line within a file.
53
+ */
54
+ public record DiffLine(String type, String text, int oldLine, int newLine)
45
55
{
46
56
}
47
57
@@ -194,6 +204,8 @@
194
204
List<DiffLine> lines = new ArrayList<>();
195
205
int additions = 0;
196
206
int deletions = 0;
207
+ int oldLn = 0;
208
+ int newLn = 0;
197
209
String[] raw = patch.split("\n", -1);
198
210
for (int i = 0; i < raw.length; i++)
199
211
{
@@ -204,9 +216,17 @@
204
216
break;
205
217
}
206
218
String type;
219
+ int oldLine = -1;
220
+ int newLine = -1;
207
221
if (line.startsWith("@@"))
208
222
{
209
223
type = "hunk";
224
+ Matcher matcher = HUNK_HEADER.matcher(line);
225
+ if (matcher.find())
226
+ {
227
+ oldLn = Integer.parseInt(matcher.group(1));
228
+ newLn = Integer.parseInt(matcher.group(2));
229
+ }
210
230
}
211
231
else if (line.startsWith("+++") || line.startsWith("---") || line.startsWith("diff ")
212
232
|| line.startsWith("index ") || line.startsWith("new file") || line.startsWith("deleted file")
@@ -219,17 +239,21 @@
219
239
{
220
240
type = "add";
221
241
additions++;
242
+ newLine = newLn++;
222
243
}
223
244
else if (line.startsWith("-"))
224
245
{
225
246
type = "del";
226
247
deletions++;
248
+ oldLine = oldLn++;
227
249
}
228
250
else
229
251
{
230
252
type = "context";
253
+ oldLine = oldLn++;
254
+ newLine = newLn++;
231
255
}
232
- lines.add(new DiffLine(type, line));
256
+ lines.add(new DiffLine(type, line, oldLine, newLine));
233
257
}
234
258
String path = entry.getChangeType() == DiffEntry.ChangeType.DELETE ? entry.getOldPath() : entry.getNewPath();
235
259
return new FileDiff(path, entry.getChangeType().name(), lines, additions, deletions);
ADD
src/main/java/de/workaround/git/MergeRequestCommentService.java
+106 -0
@@ -0,0 +1,106 @@
1
+package de.workaround.git;
2
+
3
+import java.util.List;
4
+
5
+import de.workaround.model.MergeRequest;
6
+import de.workaround.model.MergeRequestComment;
7
+import de.workaround.model.User;
8
+import jakarta.enterprise.context.ApplicationScoped;
9
+import jakarta.inject.Inject;
10
+import jakarta.transaction.Transactional;
11
+
12
+/**
13
+ * Manages line-level review comments on merge-request diffs. Any authenticated user who can read the repository
14
+ * may comment; a comment can be removed by its author or by the repository owner. Comments must anchor to a line
15
+ * that is actually part of the merge request's current diff, so they cannot be orphaned on creation.
16
+ */
17
+@ApplicationScoped
18
+public class MergeRequestCommentService
19
+{
20
+ @Inject
21
+ MergeRequestComment.Repo comments;
22
+
23
+ @Inject
24
+ AccessPolicy accessPolicy;
25
+
26
+ @Inject
27
+ MergeRequestService mergeRequests;
28
+
29
+ @Transactional
30
+ public MergeRequestComment add(User actor, MergeRequest mr, String filePath, int oldLine, int newLine, String body)
31
+ {
32
+ requireReader(actor, mr);
33
+ String trimmedBody = body == null ? "" : body.strip();
34
+ if (trimmedBody.isEmpty())
35
+ {
36
+ throw new InvalidMergeRequestException("Comment must not be empty");
37
+ }
38
+ if (filePath == null || filePath.isBlank())
39
+ {
40
+ throw new InvalidMergeRequestException("Comment must reference a file");
41
+ }
42
+ if (!anchorExists(mr, filePath, oldLine, newLine))
43
+ {
44
+ throw new InvalidMergeRequestException("Comment does not anchor to a line in the diff");
45
+ }
46
+ MergeRequestComment comment = new MergeRequestComment();
47
+ comment.mergeRequest = mr;
48
+ comment.author = actor;
49
+ comment.filePath = filePath;
50
+ comment.oldLine = oldLine;
51
+ comment.newLine = newLine;
52
+ comment.body = trimmedBody;
53
+ comment.persist();
54
+ return comment;
55
+ }
56
+
57
+ public List<MergeRequestComment> list(MergeRequest mr)
58
+ {
59
+ return comments.findByMergeRequest(mr);
60
+ }
61
+
62
+ @Transactional
63
+ public void delete(User actor, MergeRequestComment comment)
64
+ {
65
+ if (actor == null)
66
+ {
67
+ throw new ForbiddenOperationException("Authentication required");
68
+ }
69
+ MergeRequestComment managed = comments.findById(comment.id);
70
+ if (managed == null)
71
+ {
72
+ return;
73
+ }
74
+ boolean isAuthor = managed.author.id.equals(actor.id);
75
+ boolean isOwner = accessPolicy.canWrite(actor, managed.mergeRequest.repository);
76
+ if (!isAuthor && !isOwner)
77
+ {
78
+ throw new ForbiddenOperationException("Only the comment author or the repository owner can delete it");
79
+ }
80
+ comments.deleteById(managed.id);
81
+ }
82
+
83
+ private void requireReader(User actor, MergeRequest mr)
84
+ {
85
+ if (actor == null)
86
+ {
87
+ throw new ForbiddenOperationException("Authentication required");
88
+ }
89
+ if (!accessPolicy.canRead(actor, mr.repository))
90
+ {
91
+ throw new ForbiddenOperationException("You cannot comment on this merge request");
92
+ }
93
+ }
94
+
95
+ private boolean anchorExists(MergeRequest mr, String filePath, int oldLine, int newLine)
96
+ {
97
+ return mergeRequests.diff(mr)
98
+ .map(diff -> diff.files().stream()
99
+ .filter(file -> file.path().equals(filePath))
100
+ .flatMap(file -> file.lines().stream())
101
+ .anyMatch(line -> line.oldLine() == oldLine && line.newLine() == newLine
102
+ && ("add".equals(line.type()) || "del".equals(line.type()) || "context".equals(line.type()))))
103
+ .orElse(false);
104
+ }
105
+
106
+}
ADD
src/main/java/de/workaround/model/MergeRequestComment.java
+56 -0
@@ -0,0 +1,56 @@
1
+package de.workaround.model;
2
+
3
+import java.time.Instant;
4
+import java.util.List;
5
+import java.util.UUID;
6
+
7
+import org.hibernate.annotations.processing.HQL;
8
+
9
+import io.quarkus.hibernate.panache.PanacheEntity;
10
+import io.quarkus.hibernate.panache.PanacheRepository;
11
+import jakarta.persistence.Entity;
12
+import jakarta.persistence.GeneratedValue;
13
+import jakarta.persistence.GenerationType;
14
+import jakarta.persistence.Id;
15
+import jakarta.persistence.ManyToOne;
16
+import jakarta.persistence.Table;
17
+
18
+/**
19
+ * A line-level review comment on a {@link MergeRequest}'s diff. The comment is anchored to a file
20
+ * ({@link #filePath}) and a specific diff line via the {@code (oldLine, newLine)} pair — {@code -1} on a side
21
+ * means the line has no counterpart there (added lines have no old number, deleted lines no new number). Removed
22
+ * with its merge request (and thus its repository) via DB-level ON DELETE CASCADE.
23
+ */
24
+@Entity
25
+@Table(name = "merge_request_comments")
26
+public class MergeRequestComment implements PanacheEntity.Managed
27
+{
28
+ @Id
29
+ @GeneratedValue(strategy = GenerationType.UUID)
30
+ public UUID id;
31
+
32
+ @ManyToOne(optional = false)
33
+ public MergeRequest mergeRequest;
34
+
35
+ @ManyToOne(optional = false)
36
+ public User author;
37
+
38
+ public String filePath;
39
+
40
+ /** 1-based line number on the old side of the diff, or -1 if the anchored line is an addition. */
41
+ public int oldLine;
42
+
43
+ /** 1-based line number on the new side of the diff, or -1 if the anchored line is a deletion. */
44
+ public int newLine;
45
+
46
+ public String body;
47
+
48
+ public Instant createdAt = Instant.now();
49
+
50
+ public interface Repo extends PanacheRepository.Managed<MergeRequestComment, UUID>
51
+ {
52
+ @HQL("select c from MergeRequestComment c join fetch c.author where c.mergeRequest = :mergeRequest order by c.createdAt")
53
+ List<MergeRequestComment> findByMergeRequest(MergeRequest mergeRequest);
54
+ }
55
+
56
+}
MODIFY
src/main/java/de/workaround/web/MergeRequestResource.java
+88 -3
@@ -2,6 +2,7 @@
2
2
3
3
import java.net.URI;
4
4
import java.nio.file.Path;
5
+import java.util.ArrayList;
5
6
import java.util.List;
6
7
import java.util.UUID;
7
8
@@ -11,14 +12,17 @@
11
12
import de.workaround.git.GitBrowseService;
12
13
import de.workaround.git.GitMergeService;
13
14
import de.workaround.git.GitRepositoryService;
15
+import de.workaround.git.MergeRequestCommentService;
14
16
import de.workaround.git.MergeRequestService;
15
17
import de.workaround.model.MergeRequest;
18
+import de.workaround.model.MergeRequestComment;
16
19
import de.workaround.model.Repository;
17
20
import de.workaround.model.User;
18
21
import io.quarkus.qute.CheckedTemplate;
19
22
import io.quarkus.qute.TemplateInstance;
20
23
import jakarta.inject.Inject;
21
24
import jakarta.ws.rs.Consumes;
25
+import jakarta.ws.rs.DefaultValue;
22
26
import jakarta.ws.rs.FormParam;
23
27
import jakarta.ws.rs.GET;
24
28
import jakarta.ws.rs.NotFoundException;
@@ -40,8 +44,18 @@
40
44
41
45
static native TemplateInstance newMergeRequest(Repository repo, List<String> branches, String defaultBranch);
42
46
43
- static native TemplateInstance mergeRequest(Repository repo, boolean owner, MergeRequest mr,
44
- GitMergeService.DiffView diff);
47
+ static native TemplateInstance mergeRequest(Repository repo, boolean owner, boolean loggedIn,
48
+ UUID currentUserId, MergeRequest mr, List<FileDiffView> files, int additions, int deletions);
49
+ }
50
+
51
+ /** A diff line paired with whether it accepts comments and the comments already anchored to it. */
52
+ public record DiffLineView(GitMergeService.DiffLine line, boolean commentable, List<MergeRequestComment> comments)
53
+ {
54
+ }
55
+
56
+ /** A changed file's diff lines augmented with per-line comment state, for the merge-request detail view. */
57
+ public record FileDiffView(String path, String changeType, int additions, int deletions, List<DiffLineView> lines)
58
+ {
45
59
}
46
60
47
61
@Inject
@@ -59,6 +73,12 @@
59
73
@Inject
60
74
MergeRequestService mergeRequestService;
61
75
76
+ @Inject
77
+ MergeRequestCommentService commentService;
78
+
79
+ @Inject
80
+ MergeRequestComment.Repo commentRepo;
81
+
62
82
@GET
63
83
public TemplateInstance list(@PathParam("owner") String owner, @PathParam("name") String name)
64
84
{
@@ -104,7 +124,72 @@
104
124
Repository repo = requireReadable(owner, name);
105
125
MergeRequest mr = mergeRequestService.find(repo, parseId(id)).orElseThrow(NotFoundException::new);
106
126
GitMergeService.DiffView diff = mergeRequestService.diff(mr).orElse(null);
107
- return Templates.mergeRequest(repo, isOwner(repo), mr, diff);
127
+ List<MergeRequestComment> comments = commentService.list(mr);
128
+ User user = currentUser.get();
129
+ boolean loggedIn = user != null;
130
+ UUID currentUserId = user == null ? null : user.id;
131
+
132
+ List<FileDiffView> files = new ArrayList<>();
133
+ int additions = 0;
134
+ int deletions = 0;
135
+ if (diff != null)
136
+ {
137
+ for (GitMergeService.FileDiff file : diff.files())
138
+ {
139
+ List<DiffLineView> lines = new ArrayList<>();
140
+ for (GitMergeService.DiffLine line : file.lines())
141
+ {
142
+ boolean commentable = isContent(line.type());
143
+ List<MergeRequestComment> lineComments = commentable
144
+ ? comments.stream()
145
+ .filter(c -> c.filePath.equals(file.path()) && c.oldLine == line.oldLine()
146
+ && c.newLine == line.newLine())
147
+ .toList()
148
+ : List.of();
149
+ lines.add(new DiffLineView(line, commentable && loggedIn, lineComments));
150
+ }
151
+ files.add(new FileDiffView(file.path(), file.changeType(), file.additions(), file.deletions(), lines));
152
+ additions += file.additions();
153
+ deletions += file.deletions();
154
+ }
155
+ }
156
+ return Templates.mergeRequest(repo, isOwner(repo), loggedIn, currentUserId, mr, files, additions, deletions);
157
+ }
158
+
159
+ @POST
160
+ @jakarta.ws.rs.Path("{id}/comments")
161
+ @Consumes(MediaType.APPLICATION_FORM_URLENCODED)
162
+ public Response comment(@PathParam("owner") String owner, @PathParam("name") String name,
163
+ @PathParam("id") String id, @FormParam("filePath") String filePath,
164
+ @FormParam("oldLine") @DefaultValue("-1") int oldLine, @FormParam("newLine") @DefaultValue("-1") int newLine,
165
+ @FormParam("body") String body)
166
+ {
167
+ Repository repo = requireReadable(owner, name);
168
+ MergeRequest mr = mergeRequestService.find(repo, parseId(id)).orElseThrow(NotFoundException::new);
169
+ commentService.add(currentUser.require(), mr, filePath, oldLine, newLine, body);
170
+ return Response.seeOther(detailUri(repo, mr.id)).build();
171
+ }
172
+
173
+ @POST
174
+ @jakarta.ws.rs.Path("{id}/comments/{commentId}/delete")
175
+ @Consumes(MediaType.APPLICATION_FORM_URLENCODED)
176
+ public Response deleteComment(@PathParam("owner") String owner, @PathParam("name") String name,
177
+ @PathParam("id") String id, @PathParam("commentId") String commentId)
178
+ {
179
+ Repository repo = requireReadable(owner, name);
180
+ MergeRequest mr = mergeRequestService.find(repo, parseId(id)).orElseThrow(NotFoundException::new);
181
+ MergeRequestComment comment = commentRepo.findById(parseId(commentId));
182
+ if (comment == null || !comment.mergeRequest.id.equals(mr.id))
183
+ {
184
+ throw new NotFoundException();
185
+ }
186
+ commentService.delete(currentUser.require(), comment);
187
+ return Response.seeOther(detailUri(repo, mr.id)).build();
188
+ }
189
+
190
+ private static boolean isContent(String lineType)
191
+ {
192
+ return "add".equals(lineType) || "del".equals(lineType) || "context".equals(lineType);
108
193
}
109
194
110
195
@POST
MODIFY
src/main/resources/META-INF/resources/shark.css
+123 -12
@@ -935,38 +935,149 @@
935
935
background: var(--danger-bg);
936
936
}
937
937
938
-pre.diff {
939
- margin: 0;
940
- padding: 0;
938
+.diff-body {
941
939
overflow-x: auto;
942
- font: 12.5px/1.5 var(--mono);
940
+ font: 12.5px/1.6 var(--mono);
943
941
background: var(--surface);
944
942
}
945
943
946
-pre.diff .dl {
947
- display: block;
948
- padding: 0 var(--s3);
949
- white-space: pre;
944
+.diff-body .dl {
950
945
color: var(--ink);
946
+ border-bottom: 1px solid var(--border-soft);
951
947
}
952
948
953
-pre.diff .dl.add {
949
+.diff-body .dl:last-child {
950
+ border-bottom: none;
951
+}
952
+
953
+.dl-line {
954
+ display: flex;
955
+ align-items: flex-start;
956
+ min-width: max-content;
957
+}
958
+
959
+.dl-line .ln {
960
+ flex: 0 0 auto;
961
+ width: 44px;
962
+ padding: 0 8px;
963
+ text-align: right;
964
+ color: var(--faint);
965
+ background: var(--border-soft);
966
+ border-right: 1px solid var(--border);
967
+ -webkit-user-select: none;
968
+ user-select: none;
969
+}
970
+
971
+.dl-line .dl-text {
972
+ flex: 1 1 auto;
973
+ padding: 0 var(--s3);
974
+ white-space: pre;
975
+}
976
+
977
+.diff-body .dl.add .dl-line {
954
978
background: oklch(0.95 0.05 150);
955
979
}
956
980
957
-pre.diff .dl.del {
981
+.diff-body .dl.add .ln {
982
+ background: oklch(0.9 0.06 150);
983
+}
984
+
985
+.diff-body .dl.del .dl-line {
958
986
background: oklch(0.95 0.05 27);
959
987
}
960
988
961
-pre.diff .dl.hunk {
989
+.diff-body .dl.del .ln {
990
+ background: oklch(0.9 0.06 27);
991
+}
992
+
993
+.diff-body .dl.hunk .dl-line {
962
994
color: var(--accent-deep);
963
995
background: var(--accent-soft);
964
996
}
965
997
966
-pre.diff .dl.meta {
998
+.diff-body .dl.hunk .ln {
999
+ background: var(--accent-soft);
1000
+}
1001
+
1002
+.diff-body .dl.meta .dl-line {
967
1003
color: var(--faint);
968
1004
}
969
1005
1006
+/* per-line comment affordance and threads */
1007
+
1008
+.dl-add-comment > summary {
1009
+ cursor: pointer;
1010
+ list-style: none;
1011
+ padding: 2px var(--s3) 2px 96px;
1012
+ font: 500 11px/1.4 var(--font);
1013
+ color: var(--accent-deep);
1014
+ background: var(--accent-soft);
1015
+ opacity: 0;
1016
+ -webkit-user-select: none;
1017
+ user-select: none;
1018
+}
1019
+
1020
+.dl:hover .dl-add-comment > summary,
1021
+.dl-add-comment[open] > summary {
1022
+ opacity: 1;
1023
+}
1024
+
1025
+.dl-add-comment > summary::-webkit-details-marker {
1026
+ display: none;
1027
+}
1028
+
1029
+.dl-add-comment form {
1030
+ display: flex;
1031
+ flex-direction: column;
1032
+ gap: var(--s2);
1033
+ max-width: 560px;
1034
+ padding: var(--s2) var(--s3) var(--s3) 96px;
1035
+ background: var(--surface);
1036
+}
1037
+
1038
+.dl-comment-row {
1039
+ padding: var(--s2) var(--s3) var(--s2) 96px;
1040
+ background: var(--surface);
1041
+ border-top: 1px dashed var(--border);
1042
+ font-family: var(--font);
1043
+}
1044
+
1045
+.comment {
1046
+ max-width: 640px;
1047
+}
1048
+
1049
+.comment-head {
1050
+ display: flex;
1051
+ align-items: center;
1052
+ gap: var(--s2);
1053
+ margin-bottom: 2px;
1054
+}
1055
+
1056
+.comment-head .who {
1057
+ font-weight: 600;
1058
+ font-size: 13px;
1059
+}
1060
+
1061
+.comment-del {
1062
+ border: none;
1063
+ background: none;
1064
+ padding: 0;
1065
+ cursor: pointer;
1066
+ color: var(--danger);
1067
+ font: 500 12px/1 var(--font);
1068
+}
1069
+
1070
+.comment-del:hover {
1071
+ text-decoration: underline;
1072
+}
1073
+
1074
+.comment-body {
1075
+ white-space: pre-wrap;
1076
+ word-break: break-word;
1077
+ font-size: 13.5px;
1078
+ color: var(--ink);
1079
+}
1080
+
970
1081
/* dashboard */
971
1082
972
1083
.dashboard-section {
ADD
src/main/resources/db/migration/V8__merge_request_comments.sql
+15 -0
@@ -0,0 +1,15 @@
1
+-- Line-level review comments on a merge request's diff. A comment is anchored to a file and a diff line by the
2
+-- (old_line, new_line) pair (-1 where the line has no counterpart on that side, e.g. an added or deleted line).
3
+create table merge_request_comments
4
+(
5
+ id uuid primary key,
6
+ merge_request_id uuid not null references merge_requests (id) on delete cascade,
7
+ author_id uuid not null references users (id) on delete cascade,
8
+ file_path text not null,
9
+ old_line integer not null default -1,
10
+ new_line integer not null default -1,
11
+ body text not null,
12
+ created_at timestamptz not null default now()
13
+);
14
+
15
+create index merge_request_comments_mr_idx on merge_request_comments (merge_request_id);
MODIFY
src/main/resources/templates/MergeRequestResource/mergeRequest.html
+47 -6
@@ -23,22 +23,63 @@
23
23
</div>
24
24
{/if}
25
25
<h3>Changes</h3>
26
-{#if diff == null || diff.files.isEmpty()}
26
+{#if files.isEmpty()}
27
27
<p class="muted">No changes to show. The source branch has nothing new over the target.</p>
28
28
{#else}
29
29
<p class="diffstat-summary">
30
- <b>{diff.files.size()}</b> file{#if diff.files.size() != 1}s{/if} changed,
31
- <span class="add">+{diff.additions}</span> <span class="del">-{diff.deletions}</span>
30
+ <b>{files.size()}</b> file{#if files.size() != 1}s{/if} changed,
31
+ <span class="add">+{additions}</span> <span class="del">-{deletions}</span>
32
+ {#if !loggedIn}<span class="muted">· log in to comment on a line</span>{/if}
32
33
</p>
33
-{#for file in diff.files}
34
+{#for file in files}
34
35
<div class="diff-file">
35
36
<div class="diff-file-head">
36
37
<span class="badge ct-{file.changeType}">{file.changeType}</span>
37
38
<code>{file.path}</code>
38
39
<span class="diffstat"><span class="add">+{file.additions}</span> <span class="del">-{file.deletions}</span></span>
39
40
</div>
40
- <pre class="diff">{#for line in file.lines}<span class="dl {line.type}">{line.text}</span>
41
-{/for}</pre>
41
+ <div class="diff-body">
42
+ {#for lv in file.lines}
43
+ <div class="dl {lv.line.type}">
44
+ <div class="dl-line">
45
+ <span class="ln">{#if lv.line.oldLine != -1}{lv.line.oldLine}{/if}</span>
46
+ <span class="ln">{#if lv.line.newLine != -1}{lv.line.newLine}{/if}</span>
47
+ <span class="dl-text">{lv.line.text}</span>
48
+ </div>
49
+ {#if lv.commentable}
50
+ <details class="dl-add-comment">
51
+ <summary title="Comment on this line" aria-label="Comment on this line">+ Comment on this line</summary>
52
+ <form method="post" action="/repos/{repo.owner.username}/{repo.name}/merge-requests/{mr.id}/comments">
53
+ <input type="hidden" name="filePath" value="{file.path}">
54
+ <input type="hidden" name="oldLine" value="{lv.line.oldLine}">
55
+ <input type="hidden" name="newLine" value="{lv.line.newLine}">
56
+ <textarea name="body" rows="3" placeholder="Comment on this line" required autocomplete="off"></textarea>
57
+ <div class="form-actions">
58
+ <button type="submit" class="btn btn-primary btn-sm">Comment</button>
59
+ </div>
60
+ </form>
61
+ </details>
62
+ {/if}
63
+ </div>
64
+ {#for c in lv.comments}
65
+ <div class="dl-comment-row">
66
+ <div class="comment">
67
+ <div class="comment-head">
68
+ <span class="who">{c.author.username}</span>
69
+ {#if owner || c.author.id == currentUserId}
70
+ <form class="inline" method="post"
71
+ action="/repos/{repo.owner.username}/{repo.name}/merge-requests/{mr.id}/comments/{c.id}/delete"
72
+ onsubmit="return confirm('Delete this comment?')">
73
+ <button type="submit" class="comment-del">Delete</button>
74
+ </form>
75
+ {/if}
76
+ </div>
77
+ <div class="comment-body">{c.body}</div>
78
+ </div>
79
+ </div>
80
+ {/for}
81
+ {/for}
82
+ </div>
42
83
</div>
43
84
{/for}
44
85
{/if}
ADD
src/test/java/de/workaround/git/MergeRequestCommentServiceTest.java
+212 -0
@@ -0,0 +1,212 @@
1
+package de.workaround.git;
2
+
3
+import java.nio.charset.StandardCharsets;
4
+import java.nio.file.Files;
5
+import java.nio.file.Path;
6
+import java.util.Map;
7
+import java.util.UUID;
8
+
9
+import org.eclipse.jgit.api.Git;
10
+import org.eclipse.jgit.transport.RefSpec;
11
+import org.junit.jupiter.api.Test;
12
+
13
+import de.workaround.model.MergeRequest;
14
+import de.workaround.model.MergeRequestComment;
15
+import de.workaround.model.Repository;
16
+import de.workaround.model.User;
17
+import io.quarkus.test.junit.QuarkusTest;
18
+import jakarta.inject.Inject;
19
+import jakarta.transaction.Transactional;
20
+
21
+import static org.junit.jupiter.api.Assertions.assertEquals;
22
+import static org.junit.jupiter.api.Assertions.assertNull;
23
+import static org.junit.jupiter.api.Assertions.assertThrows;
24
+import static org.junit.jupiter.api.Assertions.assertTrue;
25
+
26
+@QuarkusTest
27
+class MergeRequestCommentServiceTest
28
+{
29
+ @Inject
30
+ GitRepositoryService service;
31
+
32
+ @Inject
33
+ MergeRequestService mergeRequests;
34
+
35
+ @Inject
36
+ MergeRequestCommentService comments;
37
+
38
+ @Inject
39
+ MergeRequestComment.Repo commentRepo;
40
+
41
+ @Inject
42
+ MergeRequest.Repo mrRepo;
43
+
44
+ @Inject
45
+ User.Repo userRepo;
46
+
47
+ @Test
48
+ void addStoresACommentAnchoredToADiffLine() throws Exception
49
+ {
50
+ User owner = persistUser("cm-alice");
51
+ Repository repository = seed(owner, "cma");
52
+ MergeRequest mr = mergeRequests.create(owner, repository, "MR", null, "feature", "main");
53
+
54
+ // feature.txt is a newly added file, so its single line is an addition on the new side (line 1)
55
+ MergeRequestComment comment = comments.add(owner, mr, "feature.txt", -1, 1, "looks good");
56
+
57
+ assertEquals("feature.txt", comment.filePath);
58
+ assertEquals(-1, comment.oldLine);
59
+ assertEquals(1, comment.newLine);
60
+ assertEquals("looks good", comment.body);
61
+ assertEquals(owner.id, comment.author.id);
62
+ assertEquals(mr.id, comment.mergeRequest.id);
63
+ }
64
+
65
+ @Test
66
+ void addRejectsBlankBody() throws Exception
67
+ {
68
+ User owner = persistUser("cm-bob");
69
+ Repository repository = seed(owner, "cmb");
70
+ MergeRequest mr = mergeRequests.create(owner, repository, "MR", null, "feature", "main");
71
+
72
+ assertThrows(InvalidMergeRequestException.class, () -> comments.add(owner, mr, "feature.txt", -1, 1, " "));
73
+ }
74
+
75
+ @Test
76
+ void addRejectsALineThatIsNotPartOfTheDiff() throws Exception
77
+ {
78
+ User owner = persistUser("cm-cara");
79
+ Repository repository = seed(owner, "cmc");
80
+ MergeRequest mr = mergeRequests.create(owner, repository, "MR", null, "feature", "main");
81
+
82
+ assertThrows(InvalidMergeRequestException.class,
83
+ () -> comments.add(owner, mr, "does-not-exist.txt", -1, 99, "huh"));
84
+ }
85
+
86
+ @Test
87
+ void anonymousCannotComment() throws Exception
88
+ {
89
+ User owner = persistUser("cm-dan");
90
+ Repository repository = seed(owner, "cmd");
91
+ MergeRequest mr = mergeRequests.create(owner, repository, "MR", null, "feature", "main");
92
+
93
+ assertThrows(ForbiddenOperationException.class, () -> comments.add(null, mr, "feature.txt", -1, 1, "hi"));
94
+ }
95
+
96
+ @Test
97
+ void anyReaderOfAPublicRepositoryCanComment() throws Exception
98
+ {
99
+ User owner = persistUser("cm-erin");
100
+ User reader = persistUser("cm-frank");
101
+ Repository repository = seed(owner, "cme");
102
+ MergeRequest mr = mergeRequests.create(owner, repository, "MR", null, "feature", "main");
103
+
104
+ MergeRequestComment comment = comments.add(reader, mr, "feature.txt", -1, 1, "drive-by review");
105
+
106
+ assertEquals(reader.id, comment.author.id);
107
+ }
108
+
109
+ @Test
110
+ void strangerCannotCommentOnAPrivateRepository() throws Exception
111
+ {
112
+ User owner = persistUser("cm-gwen");
113
+ User stranger = persistUser("cm-hugo");
114
+ Repository repository = seedPrivate(owner, "cmg");
115
+ MergeRequest mr = mergeRequests.create(owner, repository, "MR", null, "feature", "main");
116
+
117
+ assertThrows(ForbiddenOperationException.class,
118
+ () -> comments.add(stranger, mr, "feature.txt", -1, 1, "sneaky"));
119
+ }
120
+
121
+ @Test
122
+ void authorOrOwnerCanDeleteButOthersCannot() throws Exception
123
+ {
124
+ User owner = persistUser("cm-ivy");
125
+ User reader = persistUser("cm-jack");
126
+ User other = persistUser("cm-kate");
127
+ Repository repository = seed(owner, "cmi");
128
+ MergeRequest mr = mergeRequests.create(owner, repository, "MR", null, "feature", "main");
129
+ MergeRequestComment byReader = comments.add(reader, mr, "feature.txt", -1, 1, "reader note");
130
+
131
+ assertThrows(ForbiddenOperationException.class, () -> comments.delete(other, byReader));
132
+
133
+ // the author can delete their own comment
134
+ comments.delete(reader, byReader);
135
+ assertNull(commentRepo.findById(byReader.id));
136
+
137
+ // the repo owner can delete anyone's comment
138
+ MergeRequestComment byReader2 = comments.add(reader, mr, "feature.txt", -1, 1, "another");
139
+ comments.delete(owner, byReader2);
140
+ assertNull(commentRepo.findById(byReader2.id));
141
+ }
142
+
143
+ @Test
144
+ void listReturnsCommentsForTheMergeRequest() throws Exception
145
+ {
146
+ User owner = persistUser("cm-liam");
147
+ Repository repository = seed(owner, "cml");
148
+ MergeRequest mr = mergeRequests.create(owner, repository, "MR", null, "feature", "main");
149
+ comments.add(owner, mr, "feature.txt", -1, 1, "one");
150
+ comments.add(owner, mr, "feature.txt", -1, 1, "two");
151
+
152
+ assertEquals(2, comments.list(mr).size());
153
+ }
154
+
155
+ @Test
156
+ void deletingARepositoryRemovesItsMergeRequestComments() throws Exception
157
+ {
158
+ User owner = persistUser("cm-mia");
159
+ Repository repository = seed(owner, "cmm");
160
+ MergeRequest mr = mergeRequests.create(owner, repository, "MR", null, "feature", "main");
161
+ MergeRequestComment comment = comments.add(owner, mr, "feature.txt", -1, 1, "bye");
162
+ UUID commentId = comment.id;
163
+
164
+ service.delete(owner, repository);
165
+
166
+ assertNull(commentRepo.findById(commentId), "comment must cascade-delete with its repository");
167
+ }
168
+
169
+ private Repository seed(User owner, String name) throws Exception
170
+ {
171
+ return seed(owner, name, Repository.Visibility.PUBLIC);
172
+ }
173
+
174
+ private Repository seedPrivate(User owner, String name) throws Exception
175
+ {
176
+ return seed(owner, name, Repository.Visibility.PRIVATE);
177
+ }
178
+
179
+ /** Repo with a base commit on main and a divergent 'feature' branch adding feature.txt. */
180
+ private Repository seed(User owner, String name, Repository.Visibility visibility) throws Exception
181
+ {
182
+ Repository repository = service.create(owner, name, visibility, null);
183
+ Path bare = service.repositoryPath(repository);
184
+ GitTestSeeder.seed(bare, Map.of("base.txt", "base\n".getBytes(StandardCharsets.UTF_8)));
185
+ Path work = Files.createTempDirectory("cmseed");
186
+ try (Git git = Git.cloneRepository().setURI(bare.toUri().toString()).setDirectory(work.toFile()).call())
187
+ {
188
+ git.checkout().setCreateBranch(true).setName("feature").call();
189
+ Files.writeString(work.resolve("feature.txt"), "feature\n");
190
+ git.add().addFilepattern(".").call();
191
+ git.commit().setMessage("feature work").setSign(false)
192
+ .setAuthor("dev", "dev@example.com").setCommitter("dev", "dev@example.com").call();
193
+ git.push().setRefSpecs(new RefSpec("feature:refs/heads/feature")).call();
194
+ }
195
+ return repository;
196
+ }
197
+
198
+ @Transactional
199
+ User persistUser(String name)
200
+ {
201
+ User existing = userRepo.findByOidcSubOptional(name).orElse(null);
202
+ if (existing != null)
203
+ {
204
+ return existing;
205
+ }
206
+ User user = new User();
207
+ user.oidcSub = name;
208
+ user.username = name;
209
+ user.persist();
210
+ return user;
211
+ }
212
+}
ADD
src/test/java/de/workaround/web/MergeRequestCommentUiTest.java
+154 -0
@@ -0,0 +1,154 @@
1
+package de.workaround.web;
2
+
3
+import java.nio.charset.StandardCharsets;
4
+import java.nio.file.Files;
5
+import java.nio.file.Path;
6
+import java.util.Map;
7
+import java.util.UUID;
8
+
9
+import org.eclipse.jgit.api.Git;
10
+import org.eclipse.jgit.transport.RefSpec;
11
+import org.junit.jupiter.api.Test;
12
+
13
+import de.workaround.git.GitRepositoryService;
14
+import de.workaround.git.GitTestSeeder;
15
+import de.workaround.git.MergeRequestCommentService;
16
+import de.workaround.git.MergeRequestService;
17
+import de.workaround.model.MergeRequest;
18
+import de.workaround.model.MergeRequestComment;
19
+import de.workaround.model.Repository;
20
+import de.workaround.model.User;
21
+import io.quarkus.test.junit.QuarkusTest;
22
+import io.quarkus.test.security.TestSecurity;
23
+import jakarta.inject.Inject;
24
+import jakarta.transaction.Transactional;
25
+
26
+import static io.restassured.RestAssured.given;
27
+import static org.hamcrest.CoreMatchers.containsString;
28
+import static org.hamcrest.CoreMatchers.not;
29
+
30
+@QuarkusTest
31
+class MergeRequestCommentUiTest
32
+{
33
+ @Inject
34
+ GitRepositoryService service;
35
+
36
+ @Inject
37
+ MergeRequestService mergeRequests;
38
+
39
+ @Inject
40
+ MergeRequestCommentService comments;
41
+
42
+ @Inject
43
+ User.Repo userRepo;
44
+
45
+ @Test
46
+ @TestSecurity(user = "cmu-owner")
47
+ void ownerCanCommentOnADiffLineAndSeeItInline()
48
+ {
49
+ User owner = persistUser("cmu-owner");
50
+ MergeRequest mr = seededMr(owner, "board");
51
+ String detail = "/repos/" + owner.username + "/board/merge-requests/" + mr.id;
52
+
53
+ given().redirects().follow(false).contentType("application/x-www-form-urlencoded")
54
+ .formParam("filePath", "feature.txt").formParam("oldLine", "-1").formParam("newLine", "1")
55
+ .formParam("body", "please rename this")
56
+ .when().post(detail + "/comments")
57
+ .then().statusCode(303);
58
+
59
+ given().when().get(detail)
60
+ .then().statusCode(200)
61
+ .body(containsString("please rename this"))
62
+ .body(containsString("feature.txt"));
63
+ }
64
+
65
+ @Test
66
+ @TestSecurity(user = "cmu-reader")
67
+ void aReaderWhoIsNotTheOwnerCanCommentOnAPublicMergeRequest()
68
+ {
69
+ persistUser("cmu-reader");
70
+ User owner = persistUser("cmu-owner2-" + UUID.randomUUID().toString().substring(0, 8));
71
+ MergeRequest mr = seededMr(owner, "board");
72
+ String detail = "/repos/" + owner.username + "/board/merge-requests/" + mr.id;
73
+
74
+ given().redirects().follow(false).contentType("application/x-www-form-urlencoded")
75
+ .formParam("filePath", "feature.txt").formParam("oldLine", "-1").formParam("newLine", "1")
76
+ .formParam("body", "outside review")
77
+ .when().post(detail + "/comments")
78
+ .then().statusCode(303);
79
+
80
+ given().when().get(detail).then().statusCode(200).body(containsString("outside review"));
81
+ }
82
+
83
+ @Test
84
+ void anonymousCannotComment()
85
+ {
86
+ User owner = persistUser("cmu-anon-" + UUID.randomUUID().toString().substring(0, 8));
87
+ MergeRequest mr = seededMr(owner, "board");
88
+ String detail = "/repos/" + owner.username + "/board/merge-requests/" + mr.id;
89
+
90
+ given().contentType("application/x-www-form-urlencoded")
91
+ .formParam("filePath", "feature.txt").formParam("oldLine", "-1").formParam("newLine", "1")
92
+ .formParam("body", "sneaky")
93
+ .when().post(detail + "/comments")
94
+ .then().statusCode(403);
95
+ }
96
+
97
+ @Test
98
+ @TestSecurity(user = "cmu-del")
99
+ void authorCanDeleteTheirComment()
100
+ {
101
+ User owner = persistUser("cmu-del");
102
+ MergeRequest mr = seededMr(owner, "board");
103
+ MergeRequestComment comment = comments.add(owner, mr, "feature.txt", -1, 1, "temporary note");
104
+ String detail = "/repos/" + owner.username + "/board/merge-requests/" + mr.id;
105
+
106
+ given().when().get(detail).then().statusCode(200).body(containsString("temporary note"));
107
+
108
+ given().redirects().follow(false).contentType("application/x-www-form-urlencoded")
109
+ .when().post(detail + "/comments/" + comment.id + "/delete")
110
+ .then().statusCode(303);
111
+
112
+ given().when().get(detail).then().statusCode(200).body(not(containsString("temporary note")));
113
+ }
114
+
115
+ private MergeRequest seededMr(User owner, String name)
116
+ {
117
+ try
118
+ {
119
+ Repository repo = service.create(owner, name, Repository.Visibility.PUBLIC, null);
120
+ Path bare = service.repositoryPath(repo);
121
+ GitTestSeeder.seed(bare, Map.of("base.txt", "base\n".getBytes(StandardCharsets.UTF_8)));
122
+ Path work = Files.createTempDirectory("cmuseed");
123
+ try (Git git = Git.cloneRepository().setURI(bare.toUri().toString()).setDirectory(work.toFile()).call())
124
+ {
125
+ git.checkout().setCreateBranch(true).setName("feature").call();
126
+ Files.writeString(work.resolve("feature.txt"), "feature\n");
127
+ git.add().addFilepattern(".").call();
128
+ git.commit().setMessage("feature work").setSign(false)
129
+ .setAuthor("dev", "dev@example.com").setCommitter("dev", "dev@example.com").call();
130
+ git.push().setRefSpecs(new RefSpec("feature:refs/heads/feature")).call();
131
+ }
132
+ return mergeRequests.create(owner, repo, "Review me", null, "feature", "main");
133
+ }
134
+ catch (Exception e)
135
+ {
136
+ throw new RuntimeException(e);
137
+ }
138
+ }
139
+
140
+ @Transactional
141
+ User persistUser(String name)
142
+ {
143
+ User existing = userRepo.findByOidcSubOptional(name).orElse(null);
144
+ if (existing != null)
145
+ {
146
+ return existing;
147
+ }
148
+ User user = new User();
149
+ user.oidcSub = name;
150
+ user.username = name;
151
+ user.persist();
152
+ return user;
153
+ }
154
+}