From c79c69a2ad55d8ed21e3953fae9661fa3fb80ad2 Mon Sep 17 00:00:00 2001 From: Marco Mondini <10319061+mondial7@users.noreply.github.com> Date: Sat, 11 Jul 2026 20:26:30 +0200 Subject: [PATCH] feat(v1.2.0): self-nomination (#41) + batch user lookup (#33 follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - #41 Members can request a feedback round on themselves (My Feedback → Request feedback): they pick reviewers, and the round is created as a draft owned by a manager (their team admin, else a global admin) — NEVER the subject — so the requester can never de-anonymize their reviewers. One open request at a time; the owner reviews and starts it. Owner≠subject is asserted in tests and verified end-to-end (subject can't see raw submissions of their own request). - Non-admin Rounds now also lists rounds you own, so a team admin can see and manage rounds they created (fixes a pre-existing gap; also surfaces self-nominated rounds to the team-admin owner). - #33 follow-up: paginated round lists resolve only the current page's users via Users.FindByIDs (ANY($1::uuid[]), gateway-tested) instead of a full-table load. gosec 0 / govulncheck 0; full suite incl. Postgres gateway green. --- CHANGELOG.md | 17 ++++ internal/handlers/handlers_test.go | 78 +++++++++++++++ internal/handlers/myfeedback.go | 9 +- internal/handlers/presenters.go | 50 ++++++++-- internal/handlers/rounds.go | 15 +-- internal/handlers/routes.go | 4 + internal/handlers/selfnominate.go | 141 ++++++++++++++++++++++++++++ internal/models/audit.go | 1 + internal/repo/fakes.go | 12 +++ internal/repo/gateway_pg_test.go | 25 +++++ internal/repo/pg_users.go | 8 ++ internal/repo/repo.go | 1 + sonar-project.properties | 2 +- web/templates/my_feedback.html | 5 +- web/templates/request_feedback.html | 35 +++++++ 15 files changed, 387 insertions(+), 16 deletions(-) create mode 100644 internal/handlers/selfnominate.go create mode 100644 web/templates/request_feedback.html diff --git a/CHANGELOG.md b/CHANGELOG.md index 103af43..6867328 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,23 @@ and the project follows [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## [Unreleased] +## [1.2.0] - 2026-07-11 + +### Added + +- **Self-nomination** (#41) — a member can request a 360 round on themselves + from **My Feedback → Request feedback**: they pick their reviewers, and the + round is created as a draft **owned by a manager (their team admin, else a + global admin) — never the subject**, so the requester can never de-anonymize + their reviewers. One open request at a time. The owner reviews and starts it. + +### Changed + +- Non-admin **Rounds** now also lists the rounds you own, so a team admin sees + and can manage the rounds they created (including self-nominated ones). +- Performance (#33 follow-up): paginated round lists resolve only the users on + the current page via a batch lookup, instead of loading the whole user table. + ## [1.1.0] - 2026-07-11 ### Added diff --git a/internal/handlers/handlers_test.go b/internal/handlers/handlers_test.go index 4cd46c1..75e5a04 100644 --- a/internal/handlers/handlers_test.go +++ b/internal/handlers/handlers_test.go @@ -6,6 +6,8 @@ import ( "net/http" "net/http/cookiejar" "net/http/httptest" + "net/url" + "regexp" "strings" "testing" @@ -64,6 +66,82 @@ func get(t *testing.T, c *http.Client, url string) (int, string) { return resp.StatusCode, string(body) } +// csrfToken pulls the per-session CSRF token from a rendered page's meta tag. +func csrfToken(t *testing.T, c *http.Client, base string) string { + t.Helper() + _, body := get(t, c, base+"/my-feedback") + m := regexp.MustCompile(`name="csrf-token" content="([^"]*)"`).FindStringSubmatch(body) + if m == nil { + t.Fatal("no csrf-token meta on page") + } + return m[1] +} + +// postForm submits a form with the CSRF token in the header (as htmx does). +func postForm(t *testing.T, c *http.Client, base, path, token string, form url.Values) (int, string) { + t.Helper() + req, _ := http.NewRequest(http.MethodPost, base+path, strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req.Header.Set("X-CSRF-Token", token) + resp, err := c.Do(req) + if err != nil { + t.Fatalf("POST %s: %v", path, err) + } + defer resp.Body.Close() + body, _ := io.ReadAll(resp.Body) + return resp.StatusCode, string(body) +} + +func TestSelfNominationOwnedByManagerNotSubject(t *testing.T) { + srv, admin, repos := newTestServer(t) + ctx := t.Context() + // admin@example.com matches ADMIN_EMAIL → the eligible owner. + _, _ = get(t, admin, srv.URL+"/auth/dev-login?email=admin@example.com") + adminUser, _ := repos.Users.FindByEmail(ctx, "admin@example.com") + + // A member requests feedback on themselves. + jar, _ := cookiejar.New(nil) + member := &http.Client{Jar: jar} + member.CheckRedirect = func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse } + _, _ = get(t, member, srv.URL+"/auth/dev-login?email=mia@example.com") + memberUser, _ := repos.Users.FindByEmail(ctx, "mia@example.com") + token := csrfToken(t, member, srv.URL) + + code, _ := postForm(t, member, srv.URL, "/request-feedback", token, url.Values{"reviewer_ids": {adminUser.ID}}) + if code != http.StatusSeeOther { + t.Fatalf("expected 303 after request, got %d", code) + } + + rounds, _ := repos.Rounds.FindBySubjectID(ctx, memberUser.ID) + if len(rounds) != 1 { + t.Fatalf("expected exactly one requested round, got %d", len(rounds)) + } + rd := rounds[0] + if rd.SubjectID != memberUser.ID { + t.Fatalf("subject should be the member") + } + // The invariant: the owner (creator, who gets raw-submission access) is NOT + // the subject — otherwise the member could de-anonymize their reviewers. + if rd.CreatedByID == memberUser.ID { + t.Fatal("SECURITY: self-nominated round must not be owned by its subject") + } + if rd.CreatedByID != adminUser.ID { + t.Fatalf("expected the admin to own the round, got %q", rd.CreatedByID) + } + if rd.Status != models.RoundDraft { + t.Fatalf("expected draft (awaiting owner approval), got %q", rd.Status) + } + + // A second request is blocked while one is pending. + code, _ = postForm(t, member, srv.URL, "/request-feedback", token, url.Values{"reviewer_ids": {adminUser.ID}}) + if code != http.StatusSeeOther { + t.Fatalf("expected redirect on duplicate request, got %d", code) + } + if again, _ := repos.Rounds.FindBySubjectID(ctx, memberUser.ID); len(again) != 1 { + t.Fatalf("duplicate request should not create a second round; got %d", len(again)) + } +} + func TestUnauthenticatedRedirectsToLogin(t *testing.T) { srv, client, _ := newTestServer(t) // Don't follow redirects, so we can assert the 303 → /login. diff --git a/internal/handlers/myfeedback.go b/internal/handlers/myfeedback.go index 84c948a..8edb152 100644 --- a/internal/handlers/myfeedback.go +++ b/internal/handlers/myfeedback.go @@ -42,7 +42,14 @@ func (h *Handlers) MyFeedback(w http.ResponseWriter, r *http.Request) { "Radar": radar, "CanCompare": len(cons) >= 2, } - h.View.Page(w, http.StatusOK, h.page(r, "My feedback", "my-feedback", "my_feedback_content", data)) + pd := h.page(r, "My feedback", "my-feedback", "my_feedback_content", data) + switch { + case r.URL.Query().Get("requested") == "1": + pd.Flash = "Your feedback request was sent — a manager will review and start it." + case r.URL.Query().Get("pending") == "1": + pd.Flash = "You already have a feedback request awaiting a manager's approval." + } + h.View.Page(w, http.StatusOK, pd) } func deltaLen(d *models.SelfVsOthersDelta) int { diff --git a/internal/handlers/presenters.go b/internal/handlers/presenters.go index 060235b..c256dbb 100644 --- a/internal/handlers/presenters.go +++ b/internal/handlers/presenters.go @@ -24,20 +24,31 @@ func (h *Handlers) roundsForMe(ctx context.Context, userID string) ([]models.Fee if err != nil { return nil, err } + asOwner, err := h.Repos.Rounds.FindByCreatedByID(ctx, userID) + if err != nil { + return nil, err + } asSubject, err := h.Repos.Rounds.FindBySubjectID(ctx, userID) if err != nil { return nil, err } - seen := make(map[string]bool, len(asReviewer)) - out := make([]models.FeedbackRound, 0, len(asReviewer)+len(asSubject)) - for _, r := range asReviewer { - if !seen[r.ID] { - seen[r.ID] = true - out = append(out, r) + seen := make(map[string]bool, len(asReviewer)+len(asOwner)) + out := make([]models.FeedbackRound, 0, len(asReviewer)+len(asOwner)+len(asSubject)) + add := func(rounds []models.FeedbackRound) { + for _, r := range rounds { + if !seen[r.ID] { + seen[r.ID] = true + out = append(out, r) + } } } + // Rounds I review, plus every round I own (a team admin manages their own + // rounds, including ones members self-nominated to them). + add(asReviewer) + add(asOwner) + // As the subject, I only participate while the round is collecting feedback + // (self-assessment); I never gain owner access to my own rounds. for _, r := range asSubject { - // The subject participates only while the round is collecting feedback. if r.Status == models.RoundActive && !seen[r.ID] { seen[r.ID] = true out = append(out, r) @@ -74,6 +85,31 @@ func (h *Handlers) allUsersIndex(ctx context.Context) (map[string]models.User, e return userMap(users), nil } +// usersForRounds resolves just the users a set of rounds reference (subject + +// creator) in a single batch query, rather than loading the whole user table. +func (h *Handlers) usersForRounds(ctx context.Context, rounds []models.FeedbackRound) (map[string]models.User, error) { + seen := make(map[string]struct{}, len(rounds)*2) + ids := make([]string, 0, len(rounds)*2) + add := func(id string) { + if id == "" { + return + } + if _, ok := seen[id]; !ok { + seen[id] = struct{}{} + ids = append(ids, id) + } + } + for _, rd := range rounds { + add(rd.SubjectID) + add(rd.CreatedByID) + } + users, err := h.Repos.Users.FindByIDs(ctx, ids) + if err != nil { + return nil, err + } + return userMap(users), nil +} + // canSeeManagerOnlyChannel reports whether the caller may see the private // manager-only synthesis: global admins and the round creator, never the subject. func canSeeManagerOnlyChannel(user *models.User, round models.FeedbackRound) bool { diff --git a/internal/handlers/rounds.go b/internal/handlers/rounds.go index 1632726..a051672 100644 --- a/internal/handlers/rounds.go +++ b/internal/handlers/rounds.go @@ -66,14 +66,9 @@ func (h *Handlers) RoundsList(w http.ResponseWriter, r *http.Request) { ctx := r.Context() u := h.user(r) - users, err := h.allUsersIndex(ctx) - if err != nil { - serverError(w, err) - return - } - var rounds []models.FeedbackRound var nav pageNav // zero value renders no controls (both HasPrev/HasNext false) + var err error if u.Role == models.RoleAdmin { page := pageParam(r) paged, perr := h.Repos.Rounds.FindPaged(ctx, pageSize+1, (page-1)*pageSize) @@ -92,6 +87,14 @@ func (h *Handlers) RoundsList(w http.ResponseWriter, r *http.Request) { return } + // Resolve only the users referenced by this page (subject + creator), not + // the whole table — keeps the per-page cost bounded as the org grows. + users, err := h.usersForRounds(ctx, rounds) + if err != nil { + serverError(w, err) + return + } + cards := make([]RoundCard, 0, len(rounds)) for _, rd := range rounds { cards = append(cards, h.toCard(ctx, rd, u.ID, users)) diff --git a/internal/handlers/routes.go b/internal/handlers/routes.go index 15536d3..14a9e11 100644 --- a/internal/handlers/routes.go +++ b/internal/handlers/routes.go @@ -13,6 +13,10 @@ func (h *Handlers) MountAppRoutes(r chi.Router, submitLimit func(http.Handler) h // Onboarding r.Post("/onboarding/complete", h.CompleteOnboarding) + // Self-nomination: any member can request a feedback round on themselves. + r.Get("/request-feedback", h.RequestFeedbackForm) + r.Post("/request-feedback", h.CreateFeedbackRequest) + // Rounds r.Get("/rounds", h.RoundsList) r.With(h.Auth.RequireTeamAdminOrAdmin).Get("/rounds/new", h.NewRoundForm) diff --git a/internal/handlers/selfnominate.go b/internal/handlers/selfnominate.go new file mode 100644 index 0000000..7ab9511 --- /dev/null +++ b/internal/handlers/selfnominate.go @@ -0,0 +1,141 @@ +package handlers + +import ( + "context" + "errors" + "net/http" + "strings" + + "github.com/mondial7/smart-360/internal/models" +) + +// RequestFeedbackForm lets a member ask for a feedback round on themselves. +func (h *Handlers) RequestFeedbackForm(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() + u := h.user(r) + + // One open request at a time: if the member already has a draft round as the + // subject, send them to it rather than letting requests pile up. + if pending, _ := h.pendingRequestFor(ctx, u.ID); pending != nil { + http.Redirect(w, r, "/my-feedback?pending=1", http.StatusSeeOther) + return + } + + // Everyone except the member can be a reviewer. + all, err := h.Repos.Users.FindAll(ctx) + if err != nil { + serverError(w, err) + return + } + reviewers := make([]models.User, 0, len(all)) + for _, usr := range all { + if usr.ID != u.ID { + reviewers = append(reviewers, usr) + } + } + templates, err := h.Repos.Templates.FindAll(ctx) + if err != nil { + serverError(w, err) + return + } + data := map[string]any{"Reviewers": reviewers, "Templates": templates} + h.View.Page(w, http.StatusOK, h.page(r, "Request feedback", "my-feedback", "request_feedback_content", data)) +} + +// CreateFeedbackRequest creates a self-nominated round. The round is owned by a +// manager who is NOT the subject (the member's team admin, else a global admin), +// so the subject never gains the owner's raw-submission access. It starts as a +// draft for the owner to review and start. +func (h *Handlers) CreateFeedbackRequest(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() + u := h.user(r) + + if pending, _ := h.pendingRequestFor(ctx, u.ID); pending != nil { + http.Redirect(w, r, "/my-feedback?pending=1", http.StatusSeeOther) + return + } + + owner, err := h.resolveOwnerFor(ctx, u) + if err != nil { + http.Error(w, "No manager is available to receive your request. Ask an admin to run a round for you.", http.StatusConflict) + return + } + if err := r.ParseForm(); err != nil { + http.Error(w, "bad form", http.StatusBadRequest) + return + } + + var templateID *string + if t := r.FormValue("template_id"); t != "" { + templateID = &t + } + round := &models.FeedbackRound{ + SubjectID: u.ID, + CreatedByID: owner.ID, // owner ≠ subject: preserves reviewer anonymity + TemplateID: templateID, + Deadline: parseDate(r.FormValue("deadline")), + Status: models.RoundDraft, + } + if err := h.Repos.Rounds.Create(ctx, round); err != nil { + serverError(w, err) + return + } + for _, reviewerID := range r.Form["reviewer_ids"] { + if reviewerID != "" && reviewerID != u.ID { + _ = h.Repos.Rounds.AddReviewer(ctx, round.ID, models.RoundReviewer{ReviewerID: reviewerID}) + } + } + // Actor is the requester; the round is attributed to them in the trail. + h.audit(ctx, auditParams{Action: models.AuditRoundRequested, Actor: u, RoundID: round.ID, + RoundSubject: u.Name, Description: "Requested a feedback round (owner: " + owner.Name + ")"}) + + http.Redirect(w, r, "/my-feedback?requested=1", http.StatusSeeOther) +} + +// pendingRequestFor returns the member's existing draft round-as-subject, if any. +func (h *Handlers) pendingRequestFor(ctx context.Context, memberID string) (*models.FeedbackRound, error) { + rounds, err := h.Repos.Rounds.FindBySubjectID(ctx, memberID) + if err != nil { + return nil, err + } + for i := range rounds { + if rounds[i].Status == models.RoundDraft { + return &rounds[i], nil + } + } + return nil, nil +} + +// resolveOwnerFor picks a manager to own a member's self-nominated round — +// never the member themselves. Prefers the member's team admin, then the +// configured ADMIN_EMAIL admin, then any other admin. +func (h *Handlers) resolveOwnerFor(ctx context.Context, member *models.User) (*models.User, error) { + if member.TeamID != nil { + if team, err := h.Repos.Teams.FindByID(ctx, *member.TeamID); err == nil && + team.TeamAdminID != "" && team.TeamAdminID != member.ID { + if admin, err := h.Repos.Users.FindByID(ctx, team.TeamAdminID); err == nil { + return admin, nil + } + } + } + users, err := h.Repos.Users.FindAll(ctx) + if err != nil { + return nil, err + } + var fallback *models.User + for i := range users { + if users[i].Role != models.RoleAdmin || users[i].ID == member.ID { + continue + } + if h.Cfg.AdminEmail != "" && strings.EqualFold(users[i].Email, h.Cfg.AdminEmail) { + return &users[i], nil // prefer the bootstrap owner + } + if fallback == nil { + fallback = &users[i] + } + } + if fallback != nil { + return fallback, nil + } + return nil, errors.New("no eligible manager") +} diff --git a/internal/models/audit.go b/internal/models/audit.go index 84b7991..ca1fcd5 100644 --- a/internal/models/audit.go +++ b/internal/models/audit.go @@ -7,6 +7,7 @@ type AuditAction string const ( // Round lifecycle AuditRoundCreated AuditAction = "round.created" + AuditRoundRequested AuditAction = "round.requested" AuditRoundStatusChanged AuditAction = "round.status_changed" AuditRoundSubjectChanged AuditAction = "round.subject_changed" AuditRoundDeadlineChanged AuditAction = "round.deadline_changed" diff --git a/internal/repo/fakes.go b/internal/repo/fakes.go index d79b333..a4351a1 100644 --- a/internal/repo/fakes.go +++ b/internal/repo/fakes.go @@ -65,6 +65,18 @@ func (f *FakeUsers) FindByID(_ context.Context, id string) (*models.User, error) return nil, ErrNotFound } +func (f *FakeUsers) FindByIDs(_ context.Context, ids []string) ([]models.User, error) { + f.mu.Lock() + defer f.mu.Unlock() + var out []models.User + for _, id := range ids { + if u, ok := f.data[id]; ok { + out = append(out, u) + } + } + return out, nil +} + func (f *FakeUsers) FindByEmail(_ context.Context, email string) (*models.User, error) { f.mu.Lock() defer f.mu.Unlock() diff --git a/internal/repo/gateway_pg_test.go b/internal/repo/gateway_pg_test.go index 2cf978a..5bfbffc 100644 --- a/internal/repo/gateway_pg_test.go +++ b/internal/repo/gateway_pg_test.go @@ -37,6 +37,31 @@ func TestUsers_CreateAndLookup(t *testing.T) { } } +func TestUsers_FindByIDs(t *testing.T) { + r := gateway(t) + ctx := context.Background() + a := makeUser(t, r, "a") + b := makeUser(t, r, "b") + _ = makeUser(t, r, "c") // not requested + + got, err := r.Users.FindByIDs(ctx, []string{a, b, "" + a}) // dup a is fine + if err != nil { + t.Fatalf("find by ids: %v", err) + } + ids := map[string]bool{} + for _, u := range got { + ids[u.ID] = true + } + if !ids[a] || !ids[b] || len(ids) != 2 { + t.Fatalf("expected exactly {a,b}, got %v", ids) + } + + // Empty input is a no-op (no query). + if out, err := r.Users.FindByIDs(ctx, nil); err != nil || len(out) != 0 { + t.Fatalf("expected empty result for nil ids, got %v (err %v)", out, err) + } +} + func TestTeams_MembershipJoinTable(t *testing.T) { r := gateway(t) ctx := context.Background() diff --git a/internal/repo/pg_users.go b/internal/repo/pg_users.go index 765e94c..e96c809 100644 --- a/internal/repo/pg_users.go +++ b/internal/repo/pg_users.go @@ -27,6 +27,14 @@ func (r *pgUsers) FindByEmail(ctx context.Context, email string) (*models.User, return scanUser(r.q.QueryRow(ctx, `SELECT `+userColumns+` FROM users WHERE email = $1`, email)) } +// FindByIDs fetches the users for the given ids in one query (order unspecified). +func (r *pgUsers) FindByIDs(ctx context.Context, ids []string) ([]models.User, error) { + if len(ids) == 0 { + return nil, nil + } + return r.queryUsers(ctx, `SELECT `+userColumns+` FROM users WHERE id = ANY($1::uuid[])`, ids) +} + func (r *pgUsers) Create(ctx context.Context, u *models.User) error { if u.Role == "" { u.Role = models.RoleMember diff --git a/internal/repo/repo.go b/internal/repo/repo.go index 1024cf9..65f2ac8 100644 --- a/internal/repo/repo.go +++ b/internal/repo/repo.go @@ -11,6 +11,7 @@ import ( type UserRepository interface { FindByID(ctx context.Context, id string) (*models.User, error) + FindByIDs(ctx context.Context, ids []string) ([]models.User, error) FindByEmail(ctx context.Context, email string) (*models.User, error) Create(ctx context.Context, user *models.User) error UpdateRole(ctx context.Context, id string, role models.UserRole) error diff --git a/sonar-project.properties b/sonar-project.properties index 2a568a8..69f2415 100644 --- a/sonar-project.properties +++ b/sonar-project.properties @@ -10,7 +10,7 @@ # --- Project identity --------------------------------------------------------- sonar.projectKey=mondial7_smart-360 sonar.projectName=Smart 360 Feedback -sonar.projectVersion=1.1.0 +sonar.projectVersion=1.2.0 # If you use SonarCloud, also set: # sonar.organization= diff --git a/web/templates/my_feedback.html b/web/templates/my_feedback.html index 7d7bff7..67acb1e 100644 --- a/web/templates/my_feedback.html +++ b/web/templates/my_feedback.html @@ -1,7 +1,10 @@ {{define "my_feedback_content"}}

My feedback

Consolidations shared with you.

- {{if .CanCompare}}See your growth over time{{end}} +
+ Request feedback + {{if .CanCompare}}See your growth over time{{end}} +
{{if .Consolidations}} diff --git a/web/templates/request_feedback.html b/web/templates/request_feedback.html new file mode 100644 index 0000000..bd7b468 --- /dev/null +++ b/web/templates/request_feedback.html @@ -0,0 +1,35 @@ +{{define "request_feedback_content"}} +
+

Request feedback

+

Ask for a 360 round on yourself. A manager reviews and starts it; + your reviewers stay anonymous to you.

+ Cancel +
+ +
+
+ + {{if .Reviewers}} +
+ {{range .Reviewers}} + + {{end}} +
+ {{else}}

No one else is set up yet — ask an admin.

{{end}} +
+
+ + +
+
+ + +
+
+ + Cancel +
+
+{{end}}