Notifications for new merge requests and comments
Extends the existing federation-grant notification list to local repo activity: opening an MR notifies the repo owner, commenting on one notifies its author. Same bell/badge/Accept-Dismiss UI already used for federated invites, branching per store.Notification.Kind (a new field, empty defaulting to the pre-existing "grant" behavior so old records are unaffected). Accepting an mr_opened/mr_comment notification just marks it seen — unlike a grant, there's nothing to pin. Never self-notifies (opening your own MR, or commenting on it, notifies nobody). Caught and fixed while building this: CreateNotification's dedup key was (Principal, FromDomain, Repo, Actor), missing the MR number, so a second comment notification on a *different* MR from the same commenter on the same repo would have silently overwritten a still-pending notification about the first MR. Added MRNumber+Kind to the match and a regression test (TestNotificationDedupKeepsDistinctMRsSeparate) that reproduces it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
7 files changed
+280 −23
M
cmd/gitfed-web/handlers_notifications.go
+41 −3
M
internal/admin/admin.go
+56 −9
M
internal/admin/admin_test.go
+81 −0
M
internal/i18n/strings_en.go
+4 −0
M
internal/i18n/strings_fr.go
+4 −0
M
internal/store/notifications.go
+44 −11
M
internal/store/notifications_test.go
+50 −0
cmd/gitfed-web/handlers_notifications.go
@@ -4,6 +4,7 @@ import (
"bytes"
"html/template"
"net/http"
+ "strconv"
"git.neuromancer.ovh/bastien-mrq/gitfed/internal/i18n"
"git.neuromancer.ovh/bastien-mrq/gitfed/internal/store"
@@ -17,11 +18,18 @@ var notificationsTpl = newTpl("notifications", `
<div class="gf-card gf-repo-list">
{{range .Notifications}}
<div class="gf-repo-row">
- <div class="gf-repo-icon">{{icon "bell"}}</div>
+ <div class="gf-repo-icon">{{icon .IconName}}</div>
<div class="gf-repo-main">
<div class="name">{{.Repo}}</div>
+ {{if eq .Kind "mr_opened"}}
+ <div class="meta">{{t $.Lang "notif.mr_opened" .Actor .MRNumber .Title}}</div>
+ {{else if eq .Kind "mr_comment"}}
+ <div class="meta">{{t $.Lang "notif.mr_comment" .Actor .MRNumber .Title}}</div>
+ {{else}}
<div class="meta">{{t $.Lang "notif.granted_by"}} {{.Actor}} · {{.FromDomain}} · {{roleLabel $.Lang .Role}}</div>
+ {{end}}
</div>
+ {{if .MRURL}}<a href="{{.MRURL}}" class="gf-btn">{{t $.Lang "notif.view"}}</a>{{end}}
{{if eq (print .Status) "pending"}}
<form class="inline" method="post" action="/notifications/accept">
<input type="hidden" name="id" value="{{.ID}}">
@@ -43,6 +51,32 @@ var notificationsTpl = newTpl("notifications", `
</div>
`)
+// notificationView adds display-only fields to a store.Notification: a
+// link to the merge request it's about (mr_opened/mr_comment only) and
+// which sprite icon fits its kind — kept out of the store type since
+// they're purely a rendering concern.
+type notificationView struct {
+ store.Notification
+}
+
+func (n notificationView) IconName() string {
+ if n.EffectiveKind() != store.NotificationGrant {
+ return "branch"
+ }
+ return "bell"
+}
+
+func (n notificationView) MRURL() string {
+ if n.EffectiveKind() == store.NotificationGrant {
+ return ""
+ }
+ // Repo names can contain "/" (owner/repo-style namespacing) and the
+ // {repo...} route wildcard expects that literally, so this isn't
+ // PathEscape'd — same convention as every repo link built directly in
+ // a template (e.g. repoTpl's "/r/{{.Repo.Name}}").
+ return "/repo-mr/" + n.Repo + "?number=" + strconv.Itoa(n.MRNumber)
+}
+
func (s *server) handleNotifications(w http.ResponseWriter, r *http.Request) {
sess, _ := s.currentSession(r)
lang := s.lang(r)
@@ -52,13 +86,17 @@ func (s *server) handleNotifications(w http.ResponseWriter, r *http.Request) {
s.serverError(w, r, err)
return
}
+ views := make([]notificationView, len(notifs))
+ for i, n := range notifs {
+ views[i] = notificationView{n}
+ }
var buf bytes.Buffer
_ = notificationsTpl.Execute(&buf, struct {
- Notifications []store.Notification
+ Notifications []notificationView
Lang string
Flash template.HTML
- }{notifs, string(lang), flash(r)})
+ }{views, string(lang), flash(r)})
s.render(w, r, i18n.T(lang, "notif.title"), "", template.HTML(buf.String()))
}
internal/admin/admin.go
@@ -587,10 +587,43 @@ func (a *Admin) ListBranches(name string) ([]string, error) {
}
func (a *Admin) CreateMergeRequest(repoName, title, description, author, sourceBranch, targetBranch string) (store.MergeRequest, error) {
- return a.Store.CreateMergeRequest(store.MergeRequest{
+ mr, err := a.Store.CreateMergeRequest(store.MergeRequest{
Repo: repoName, Title: title, Description: description, Author: author,
SourceBranch: sourceBranch, TargetBranch: targetBranch,
})
+ if err != nil {
+ return mr, err
+ }
+ // Best-effort: let the repo owner know a new MR is waiting on them.
+ // Never fails MR creation itself over a notification.
+ if repo, err := a.Store.GetRepo(repoName); err == nil {
+ a.notifyMR(repo.Owner, repoName, author, store.NotificationMROpened, mr.Number, title)
+ }
+ return mr, nil
+}
+
+// notifyMR records an mr_opened/mr_comment notification for recipient, a
+// repo's local activity rather than a federated grant (see
+// store.NotificationKind). It's a silent no-op if recipient is the same
+// principal as actor (don't notify people about their own actions) or
+// isn't a local principal — MR notifications aren't federated out to a
+// collaborator's home instance, unlike grants (see notifyGrant).
+func (a *Admin) notifyMR(recipient, repoName, actor string, kind store.NotificationKind, mrNumber int, title string) {
+ if recipient == "" || recipient == actor {
+ return
+ }
+ if _, domain, ok := splitPrincipal(recipient); !ok || domain != a.Domain {
+ return
+ }
+ _ = a.Store.CreateNotification(store.Notification{
+ Principal: recipient,
+ FromDomain: a.Domain,
+ Repo: repoName,
+ Actor: actor,
+ Kind: kind,
+ MRNumber: mrNumber,
+ Title: title,
+ })
}
func (a *Admin) ListMergeRequests(repoName string) ([]store.MergeRequest, error) {
@@ -687,7 +720,16 @@ func (a *Admin) ListMRComments(repoName string, number int) ([]store.MRComment,
}
func (a *Admin) AddMRComment(repoName string, number int, author, body string) (store.MRComment, error) {
- return a.Store.AddMRComment(store.MRComment{Repo: repoName, Number: number, Author: author, Body: body})
+ comment, err := a.Store.AddMRComment(store.MRComment{Repo: repoName, Number: number, Author: author, Body: body})
+ if err != nil {
+ return comment, err
+ }
+ // Best-effort: let the MR's author know someone commented. Never fails
+ // the comment itself over a notification.
+ if mr, err := a.Store.GetMergeRequest(repoName, number); err == nil {
+ a.notifyMR(mr.Author, repoName, author, store.NotificationMRComment, number, mr.Title)
+ }
+ return comment, nil
}
// GrantCollaborator adds/updates a collaborator's role on a repo. If the
@@ -765,11 +807,14 @@ func (a *Admin) CountPendingNotifications(principal string) (int, error) {
return a.Store.CountPendingNotifications(principal)
}
-// AcceptNotification marks a pending notification accepted and, as a
-// convenience, pins the repo it was about — the whole point of accepting
-// one is "yes, remember this for me." It grants no access: whether the
-// underlying collaborator grant is still valid is checked independently,
-// by the granting instance, whenever the repo is actually visited.
+// AcceptNotification marks a pending notification accepted and, for a
+// grant notification only, pins the repo it was about as a convenience —
+// the whole point of accepting one of those is "yes, remember this for
+// me." It grants no access: whether the underlying collaborator grant is
+// still valid is checked independently, by the granting instance, whenever
+// the repo is actually visited. For an mr_opened/mr_comment notification,
+// "accept" just means "I've seen this" — there's nothing to pin, the MR
+// itself is the thing to go look at.
func (a *Admin) AcceptNotification(principal, id string) error {
notifs, err := a.Store.ListNotifications(principal)
if err != nil {
@@ -777,8 +822,10 @@ func (a *Admin) AcceptNotification(principal, id string) error {
}
for _, n := range notifs {
if n.ID == id {
- if err := a.Store.PinRepo(principal, n.FromDomain, n.Repo, ""); err != nil {
- return err
+ if n.EffectiveKind() == store.NotificationGrant {
+ if err := a.Store.PinRepo(principal, n.FromDomain, n.Repo, ""); err != nil {
+ return err
+ }
}
break
}
internal/admin/admin_test.go
@@ -294,6 +294,87 @@ func TestReadmeLanguageVariants(t *testing.T) {
}
}
+// TestMergeRequestNotifications guards the two local-activity notification
+// triggers added alongside the notifications UI: opening an MR notifies the
+// repo owner, and commenting notifies the MR's author — but never when the
+// actor is notifying themselves, and accepting one of these must not pin a
+// repo the way accepting a federation grant notification does (there's
+// nothing to "remember" about your own repo).
+func TestMergeRequestNotifications(t *testing.T) {
+ s, err := store.Open(filepath.Join(t.TempDir(), "gitfed.db"))
+ if err != nil {
+ t.Fatalf("open store: %v", err)
+ }
+ defer s.Close()
+ a := New(s, nil, "local.test", t.TempDir(), testCA(t))
+
+ if err := a.CreateRepo("repo", "alice"); err != nil {
+ t.Fatalf("create repo: %v", err)
+ }
+ owner := "alice@local.test"
+ other := "bob@local.test"
+
+ // bob opens an MR against alice's repo — alice should be notified.
+ mr, err := a.CreateMergeRequest("repo", "Add feature", "", other, "feature", "main")
+ if err != nil {
+ t.Fatalf("CreateMergeRequest: %v", err)
+ }
+ notifs, err := s.ListNotifications(owner)
+ if err != nil {
+ t.Fatalf("ListNotifications(owner): %v", err)
+ }
+ if len(notifs) != 1 {
+ t.Fatalf("owner has %d notifications, want 1", len(notifs))
+ }
+ if notifs[0].Kind != store.NotificationMROpened || notifs[0].Actor != other || notifs[0].MRNumber != mr.Number {
+ t.Fatalf("notification = %+v, want kind=mr_opened actor=%s mr=%d", notifs[0], other, mr.Number)
+ }
+
+ // alice (the owner) opening her own MR must not notify herself.
+ if _, err := a.CreateMergeRequest("repo", "Self MR", "", owner, "feature2", "main"); err != nil {
+ t.Fatalf("CreateMergeRequest (self): %v", err)
+ }
+ notifs, _ = s.ListNotifications(owner)
+ if len(notifs) != 1 {
+ t.Fatalf("owner has %d notifications after opening their own MR, want still 1 (no self-notify)", len(notifs))
+ }
+
+ // alice comments on bob's MR — bob (the author) should be notified.
+ if _, err := a.AddMRComment("repo", mr.Number, owner, "looks good"); err != nil {
+ t.Fatalf("AddMRComment: %v", err)
+ }
+ notifs, err = s.ListNotifications(other)
+ if err != nil {
+ t.Fatalf("ListNotifications(other): %v", err)
+ }
+ if len(notifs) != 1 || notifs[0].Kind != store.NotificationMRComment || notifs[0].Actor != owner {
+ t.Fatalf("bob's notifications = %+v, want one mr_comment from %s", notifs, owner)
+ }
+
+ // bob commenting on his own MR must not notify himself.
+ if _, err := a.AddMRComment("repo", mr.Number, other, "actually let me fix one more thing"); err != nil {
+ t.Fatalf("AddMRComment (self): %v", err)
+ }
+ notifs, _ = s.ListNotifications(other)
+ if len(notifs) != 1 {
+ t.Fatalf("bob has %d notifications after commenting on his own MR, want still 1 (no self-notify)", len(notifs))
+ }
+
+ // Accepting the mr_opened notification must NOT pin the repo — unlike
+ // a grant notification, there's nothing to bookmark.
+ aliceNotifs, _ := s.ListNotifications(owner)
+ if err := a.AcceptNotification(owner, aliceNotifs[0].ID); err != nil {
+ t.Fatalf("AcceptNotification: %v", err)
+ }
+ pinned, err := s.ListPinnedRepos(owner)
+ if err != nil {
+ t.Fatalf("ListPinnedRepos: %v", err)
+ }
+ if len(pinned) != 0 {
+ t.Fatalf("pinned = %+v, want none — accepting an mr_opened notification shouldn't pin anything", pinned)
+ }
+}
+
func validTestKey(t *testing.T) string {
t.Helper()
pub, _, err := ed25519.GenerateKey(rand.Reader)
internal/i18n/strings_en.go
@@ -178,6 +178,10 @@ var en = map[string]string{
"notif.status_dismissed": "dismissed",
"notif.empty": "No notifications.",
"notif.msg_accepted": "pinned to My repos",
+ "notif.mr_opened": "%s opened merge request #%d: %s",
+ "notif.mr_comment": "%s commented on merge request #%d: %s",
+ "notif.view": "View",
+ "notif.msg_seen": "marked as seen",
// ---------- settings ----------
"settings.title": "Settings",
internal/i18n/strings_fr.go
@@ -178,6 +178,10 @@ var fr = map[string]string{
"notif.status_dismissed": "ignoré",
"notif.empty": "Aucune notification.",
"notif.msg_accepted": "épinglé dans Mes repos",
+ "notif.mr_opened": "%s a ouvert la merge request #%d : %s",
+ "notif.mr_comment": "%s a commenté la merge request #%d : %s",
+ "notif.view": "Voir",
+ "notif.msg_seen": "marqué comme vu",
// ---------- settings ----------
"settings.title": "Paramètres",
internal/store/notifications.go
@@ -17,13 +17,29 @@ const (
NotificationDismissed NotificationStatus = "dismissed"
)
-// Notification is a purely advisory record: "some other instance says you
-// were granted access to one of its repos." It is never itself a
-// credential — accepting one only adds a PinnedRepo bookmark; the access it
-// describes is (or isn't) real entirely independently of this record,
-// enforced by the granting instance's own ACL when the repo is actually
-// used. See internal/federation's notify sender/receiver for how these get
-// here and what's verified before one is ever created.
+// NotificationKind distinguishes what a Notification is about — the
+// federation trust-grant notifications this type originally existed for
+// (NotificationGrant, the zero value so old records without a Kind still
+// read as grants) versus purely-local activity on a repo the recipient
+// owns or a merge request they opened.
+type NotificationKind string
+
+const (
+ NotificationGrant NotificationKind = "grant"
+ NotificationMROpened NotificationKind = "mr_opened"
+ NotificationMRComment NotificationKind = "mr_comment"
+)
+
+// Notification is a purely advisory record — for NotificationGrant, "some
+// other instance says you were granted access to one of its repos"; for
+// NotificationMROpened/NotificationMRComment, "a merge request you care
+// about changed" on this instance. It is never itself a credential —
+// accepting a grant notification only adds a PinnedRepo bookmark, and the
+// access it describes is (or isn't) real entirely independently of this
+// record, enforced by the granting instance's own ACL when the repo is
+// actually used. See internal/federation's notify sender/receiver for how
+// grant notifications get here and what's verified before one is ever
+// created; MR notifications are created locally by internal/admin instead.
type Notification struct {
ID string `json:"id"`
Principal string `json:"principal"` // local recipient
@@ -31,10 +47,22 @@ type Notification struct {
Repo string `json:"repo"`
Role string `json:"role"`
Actor string `json:"actor"` // who granted it, e.g. "alice@chez-moi.fr"
+ Kind NotificationKind `json:"kind,omitempty"`
+ MRNumber int `json:"mr_number,omitempty"`
+ Title string `json:"title,omitempty"` // MR title, for mr_opened/mr_comment
Status NotificationStatus `json:"status"`
ReceivedAt time.Time `json:"received_at"`
}
+// EffectiveKind is Kind, defaulting to NotificationGrant for records
+// created before Kind existed.
+func (n Notification) EffectiveKind() NotificationKind {
+ if n.Kind == "" {
+ return NotificationGrant
+ }
+ return n.Kind
+}
+
func notifKey(principal, id string) string { return principal + "\x00" + id }
// MaxNotificationsPerPrincipal bounds how many notifications one recipient
@@ -48,9 +76,12 @@ const MaxNotificationsPerPrincipal = 200
// CreateNotification records n, generating an ID if it doesn't have one.
// If a pending notification already exists for the same
-// (Principal, FromDomain, Repo, Actor) — the common case being the same
-// grant notified twice, or a role change re-notified — its timestamp and
-// role are refreshed instead of creating a duplicate entry.
+// (Principal, FromDomain, Repo, Actor, Kind, MRNumber) — the common case
+// being the same grant notified twice, a role change re-notified, or
+// several comments on the same MR by the same author — its timestamp
+// (and role, for grants) is refreshed instead of creating a duplicate
+// entry. MRNumber is part of the match so two different MRs from the same
+// actor on the same repo stay as distinct notifications.
func (s *Store) CreateNotification(n Notification) error {
return s.db.Update(func(tx *bolt.Tx) error {
existing, err := listJSONPrefix[Notification](tx, bucketNotifs, n.Principal+"\x00")
@@ -58,8 +89,10 @@ func (s *Store) CreateNotification(n Notification) error {
return err
}
for _, e := range existing {
- if e.Status == NotificationPending && e.FromDomain == n.FromDomain && e.Repo == n.Repo && e.Actor == n.Actor {
+ if e.Status == NotificationPending && e.FromDomain == n.FromDomain && e.Repo == n.Repo &&
+ e.Actor == n.Actor && e.EffectiveKind() == n.EffectiveKind() && e.MRNumber == n.MRNumber {
e.Role = n.Role
+ e.Title = n.Title
e.ReceivedAt = time.Now().UTC()
return putJSON(tx, bucketNotifs, notifKey(e.Principal, e.ID), e)
}
internal/store/notifications_test.go
@@ -38,6 +38,56 @@ func TestNotificationDedup(t *testing.T) {
}
}
+// TestNotificationDedupKeepsDistinctMRsSeparate guards against a real bug
+// caught while adding mr_opened/mr_comment notifications: the original
+// dedup key (Principal, FromDomain, Repo, Actor) didn't include the MR
+// number, so a comment notification on MR #2 would have collapsed into
+// (and silently overwritten) a still-pending notification about MR #1 from
+// the same commenter on the same repo — the recipient would only ever see
+// the most recent MR, never both.
+func TestNotificationDedupKeepsDistinctMRsSeparate(t *testing.T) {
+ s, err := Open(filepath.Join(t.TempDir(), "gitfed.db"))
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer s.Close()
+
+ base := Notification{Principal: "alice@local.test", FromDomain: "local.test", Repo: "alice/repo", Actor: "bob@local.test", Kind: NotificationMRComment}
+ n1 := base
+ n1.MRNumber, n1.Title = 1, "First MR"
+ n2 := base
+ n2.MRNumber, n2.Title = 2, "Second MR"
+
+ if err := s.CreateNotification(n1); err != nil {
+ t.Fatal(err)
+ }
+ if err := s.CreateNotification(n2); err != nil {
+ t.Fatal(err)
+ }
+
+ all, err := s.ListNotifications("alice@local.test")
+ if err != nil {
+ t.Fatal(err)
+ }
+ if len(all) != 2 {
+ t.Fatalf("got %d notifications, want 2 — MR #1 and MR #2 must stay distinct", len(all))
+ }
+
+ // A second comment on the SAME MR must still collapse, same as before.
+ n1Again := n1
+ n1Again.Title = "First MR (retitled)"
+ if err := s.CreateNotification(n1Again); err != nil {
+ t.Fatal(err)
+ }
+ all, err = s.ListNotifications("alice@local.test")
+ if err != nil {
+ t.Fatal(err)
+ }
+ if len(all) != 2 {
+ t.Fatalf("got %d notifications after a second comment on MR #1, want still 2", len(all))
+ }
+}
+
// TestNotificationCapEvictsOldest reproduces the gap the audit found: with
// no cap, any instance could bloat a real user's slice of the store
// indefinitely by sending distinct fake claims (nothing about receiving a