GITGUD

neiam /gitgud

Git Gud

public · Issues · Pulls · Labels · Forks · Compare · Actions success · Packages

⭐

Log in to mark this repository.

Only record a review when the verdict actually changed

a23f026 · Gabriel Morell · 2026-09-10 14:56

6 files +352 -15
Message 
{commit_body(@commit)}

Files changed

modified lib/git_gud/pull_requests.ex
+62 −3
@@ -669,9 +669,45 @@ defmodule GitGud.PullRequests do
669 669 end
670 670
671 671 def add_review(%PullRequest{} = pr, %User{id: uid} = reviewer, attrs) do
672 − case %PrReview{pull_request_id: pr.id, reviewer_id: uid}
673 − |> PrReview.changeset(attrs)
674 − |> Repo.insert() do
672 + changeset = PrReview.changeset(%PrReview{pull_request_id: pr.id, reviewer_id: uid}, attrs)
673 +
674 + # Re-submitting the verdict you already hold isn't a new review.
675 + # Without this, pressing Update review repeatedly stacks identical
676 + # rows into the log for no reason.
677 + if unchanged_review?(pr, uid, changeset) do
678 + {:ok, latest_review_of(pr.id, uid)}
679 + else
680 + do_add_review(pr, reviewer, changeset)
681 + end
682 + end
683 +
684 + defp unchanged_review?(pr, uid, changeset) do
685 + case latest_review_of(pr.id, uid) do
686 + nil ->
687 + false
688 +
689 + previous ->
690 + Ecto.Changeset.get_field(changeset, :state) == previous.state and
691 + normalize_body(Ecto.Changeset.get_field(changeset, :body)) ==
692 + normalize_body(previous.body)
693 + end
694 + end
695 +
696 + defp normalize_body(nil), do: ""
697 + defp normalize_body(body) when is_binary(body), do: String.trim(body)
698 +
699 + defp latest_review_of(pr_id, uid) do
700 + from(r in PrReview,
701 + where: r.pull_request_id == ^pr_id and r.reviewer_id == ^uid,
702 + order_by: [desc: r.id],
703 + limit: 1,
704 + preload: [:reviewer]
705 + )
706 + |> Repo.one()
707 + end
708 +
709 + defp do_add_review(%PullRequest{} = pr, %User{} = reviewer, changeset) do
710 + case Repo.insert(changeset) do
675 711 {:ok, review} ->
676 712 # Reviewing a federated PR? Fire an outbound Like / Dislike to
677 713 # the source actor so the originating instance sees the verdict.
@@ -887,6 +923,29 @@ defmodule GitGud.PullRequests do
887 923 :ok
888 924 end
889 925
926 + @doc """
927 + People who actually review in this repo, most active first.
928 +
929 + Counts reviews left and requests fulfilled across the repo's pull
930 + requests, so the suggestions are "who reviews here" rather than "who
931 + has an account". Excludes `exclude_ids` — the PR's author and anyone
932 + already asked, since neither is a useful thing to offer.
933 + """
934 + def suggested_reviewers(%Repository{id: rid}, exclude_ids \\ [], opts \\ []) do
935 + limit = Keyword.get(opts, :limit, 6)
936 +
937 + from(rev in PrReview,
938 + join: p in assoc(rev, :pull_request),
939 + join: u in assoc(rev, :reviewer),
940 + where: p.repository_id == ^rid and rev.reviewer_id not in ^exclude_ids,
941 + group_by: u.id,
942 + order_by: [desc: count(rev.id), asc: u.id],
943 + limit: ^limit,
944 + select: {u, count(rev.id)}
945 + )
946 + |> Repo.all()
947 + end
948 +
890 949 @doc "Everyone asked to review `pr`, with `:reviewer` loaded."
891 950 def list_review_requests(%PullRequest{id: id}) do
892 951 from(r in PrReviewRequest,
modified lib/git_gud_web/live/pr_live/reviews.ex
+42 −8
@@ -31,12 +31,23 @@ defmodule GitGudWeb.PrLive.Reviews do
31 31 |> GitGudWeb.RepoLive.Header.assign_chrome(repo)
32 32 |> assign(:pr, pr)
33 33 |> assign_review_form()
34 + |> assign_suggestions()
34 35 |> assign(:page_title, "Reviews · ##{pr.number} #{pr.title}")}
35 36 end
36 37
37 38 defp reload(socket) do
38 39 pr = PullRequests.get_pull_request!(socket.assigns.repo, socket.assigns.pr.number)
39 − socket |> assign(:pr, pr) |> assign_review_form()
40 + socket |> assign(:pr, pr) |> assign_review_form() |> assign_suggestions()
41 + end
42 +
43 + # Offering the author or someone already asked would be noise, so
44 + # both are excluded from the suggestions.
45 + defp assign_suggestions(socket) do
46 + %{repo: repo, pr: pr} = socket.assigns
47 + already = Enum.map(pr.review_requests, & &1.reviewer_id)
48 + exclude = Enum.reject([pr.author_id | already], &is_nil/1)
49 +
50 + assign(socket, :suggestions, PullRequests.suggested_reviewers(repo, exclude))
40 51 end
41 52
42 53 defp viewer(socket), do: socket.assigns.current_scope && socket.assigns.current_scope.user
@@ -151,13 +162,19 @@ defmodule GitGudWeb.PrLive.Reviews do
151 162
152 163 case PullRequests.add_review(pr, user, attrs) do
153 164 {:ok, review} ->
154 − :ok =
155 − Events.record(pr, "reviewed", user, %{
156 − from: previous && previous.state,
157 − to: review.state
158 − })
159 −
160 − {:noreply, reload(socket)}
165 + # An unchanged re-submission hands back the row you already
166 + # had; there's no transition to record.
167 + if previous && previous.id == review.id do
168 + {:noreply, socket}
169 + else
170 + :ok =
171 + Events.record(pr, "reviewed", user, %{
172 + from: previous && previous.state,
173 + to: review.state
174 + })
175 +
176 + {:noreply, reload(socket)}
177 + end
161 178
162 179 {:error, cs} ->
163 180 {:noreply, assign(socket, :review_form, to_form(cs))}
@@ -230,6 +247,23 @@ defmodule GitGudWeb.PrLive.Reviews do
230 247 Nobody has been asked to review yet.
231 248 </p>
232 249
250 + <div :if={can_request_review?(assigns) and @suggestions != []} class="mb-2">
251 + <p class="text-xs opacity-60 mb-1">Reviews often in this repo</p>
252 + <div class="flex flex-wrap gap-1">
253 + <button
254 + :for={{user, count} <- @suggestions}
255 + type="button"
256 + phx-click="request_review"
257 + phx-value-handle={user.handle}
258 + class="badge badge-outline gap-1 hover:badge-primary cursor-pointer"
259 + title={"#{count} review(s) in this repo"}
260 + >
261 + {user.handle}
262 + <span class="opacity-60">{count}</span>
263 + </button>
264 + </div>
265 + </div>
266 +
233 267 <form
234 268 :if={can_request_review?(assigns)}
235 269 phx-submit="request_review"
modified lib/git_gud_web/live/pr_live/show.ex
+0 −4
@@ -142,10 +142,6 @@ defmodule GitGudWeb.PrLive.Show do
142 142 defp blocker_text({:insufficient_approvals, have, need}),
143 143 do: "This pull request has #{have} of #{need} required approving review(s)."
144 144
145 − defp review_word("approved"), do: "approved"
146 − defp review_word("changes_requested"), do: "requested changes"
147 − defp review_word(_), do: "commented"
148 −
149 145 @impl true
150 146 def handle_event("expand_diff_file", params, socket),
151 147 do: {:noreply, GitGudWeb.DiffComponents.expand(socket, params)}
added priv/repo/seeds/dev_users.exs
+84 −0
@@ -0,0 +1,84 @@
1 +# Test users for the dev server — abby, betty, carol, daria.
2 +#
3 +# mix run --no-start priv/repo/seeds/dev_users.exs
4 +#
5 +# `--no-start` because the full app binds fixed ports (SSH 2222,
6 +# metrics 9568) that a running dev server already holds; this only
7 +# needs the Repo, which it starts itself.
8 +#
9 +# Idempotent: re-running leaves existing users alone rather than
10 +# erroring, so it's safe to run whenever you want them back.
11 +#
12 +# Refuses outside :dev. These are confirmed accounts with a shared,
13 +# published password; creating them anywhere else would be handing out
14 +# four working logins.
15 +
16 +alias GitGud.Accounts
17 +alias GitGud.Accounts.User
18 +alias GitGud.Repo
19 +
20 +if Mix.env() != :dev do
21 + raise "dev_users.exs seeds accounts with a known password; refusing to run in #{Mix.env()}"
22 +end
23 +
24 +# Under --no-start nothing is running yet; bring up just the Repo.
25 +{:ok, _} = Application.ensure_all_started(:ecto_sql)
26 +{:ok, _} = Application.ensure_all_started(:postgrex)
27 +
28 +repo_started? =
29 + case GitGud.Repo.start_link() do
30 + {:ok, _pid} -> true
31 + {:error, {:already_started, _pid}} -> false
32 + end
33 +
34 +password = "devpassword123!"
35 +handles = ~w(abby betty carol daria)
36 +
37 +for handle <- handles do
38 + email = "#{handle}@example.test"
39 +
40 + case Repo.get_by(User, email: email) do
41 + %User{} = existing ->
42 + IO.puts("· #{handle} already exists (id #{existing.id})")
43 +
44 + nil ->
45 + # Registration mode may be invite-only or closed on this
46 + # instance; these are seeds, so go straight at the changeset
47 + # rather than through the invite flow.
48 + user =
49 + %User{}
50 + |> User.email_changeset(%{email: email, handle: handle})
51 + |> Repo.insert!()
52 +
53 + user =
54 + user
55 + |> User.password_changeset(%{password: password}, hash_password: true)
56 + |> Repo.update!()
57 +
58 + user =
59 + user
60 + |> User.confirm_changeset()
61 + |> Repo.update!()
62 +
63 + # The profile repo every real registration gets, so their profile
64 + # pages aren't broken. Best-effort here as it is at signup — it
65 + # writes to disk and can fail on a machine without the repo root.
66 + case GitGud.Repositories.ensure_profile_repo(user) do
67 + {:ok, _} -> :ok
68 + other -> IO.puts(" (no profile repo: #{inspect(other)})")
69 + end
70 +
71 + IO.puts("✓ #{handle} <#{email}> (id #{user.id})")
72 + end
73 +end
74 +
75 +IO.puts("""
76 +
77 +Four dev users ready. Password for all of them: #{password}
78 +Sign in at /users/log-in with the email above.
79 +
80 +Registration mode is currently #{inspect(Accounts.registration_mode())} — for
81 +testing invites, generate one from /users/settings/invites as any of them.
82 +""")
83 +
84 +if repo_started?, do: GitGud.Repo.stop()
modified test/git_gud_web/live/pr_live/review_request_test.exs
+107 −0
@@ -173,6 +173,113 @@ defmodule GitGudWeb.PrLive.ReviewRequestTest do
173 173 assert html =~ "withdrew the review request for #{reviewer.handle}."
174 174 end
175 175
176 + describe "suggestion chips" do
177 + defp review!(conn, repo, pr, reviewer, state \\ "approved") do
178 + {:ok, lv, _html} = live(log_in_user(conn, reviewer), pr_path(repo, pr))
179 +
180 + lv
181 + |> form("#review-form", %{"pr_review" => %{"state" => state, "body" => ""}})
182 + |> render_submit()
183 + end
184 +
185 + test "someone who has reviewed here is suggested with a count", %{conn: conn} do
186 + {owner, repo} = repository_fixture(%{visibility: "public"})
187 + old_pr = pr_with_branches(repo, owner, %{"title" => "Earlier"})
188 + frequent = user_fixture()
189 +
190 + _ = review!(conn, repo, old_pr, frequent)
191 +
192 + new_pr = pull_request_fixture(repo, owner, "feature", "main", %{"title" => "Later"})
193 +
194 + {:ok, lv, html} = live(log_in_user(conn, owner), pr_path(repo, new_pr))
195 +
196 + assert html =~ "Reviews often in this repo"
197 + assert has_element?(lv, "button[phx-value-handle='#{frequent.handle}']")
198 + end
199 +
200 + test "clicking a chip requests that review", %{conn: conn} do
201 + {owner, repo} = repository_fixture(%{visibility: "public"})
202 + old_pr = pr_with_branches(repo, owner)
203 + frequent = user_fixture()
204 + _ = review!(conn, repo, old_pr, frequent)
205 +
206 + new_pr = pull_request_fixture(repo, owner, "feature", "main")
207 + {:ok, lv, _html} = live(log_in_user(conn, owner), pr_path(repo, new_pr))
208 +
209 + _ = lv |> element("button[phx-value-handle='#{frequent.handle}']") |> render_click()
210 +
211 + assert [req] = PullRequests.list_review_requests(new_pr)
212 + assert req.reviewer_id == frequent.id
213 + end
214 +
215 + test "someone already asked stops being suggested", %{conn: conn} do
216 + {owner, repo} = repository_fixture(%{visibility: "public"})
217 + old_pr = pr_with_branches(repo, owner)
218 + frequent = user_fixture()
219 + _ = review!(conn, repo, old_pr, frequent)
220 +
221 + new_pr = pull_request_fixture(repo, owner, "feature", "main")
222 + {:ok, lv, _html} = live(log_in_user(conn, owner), pr_path(repo, new_pr))
223 +
224 + _ = lv |> element("button[phx-value-handle='#{frequent.handle}']") |> render_click()
225 +
226 + refute has_element?(lv, "button[phx-value-handle='#{frequent.handle}']")
227 + end
228 +
229 + test "the PR's own author is never suggested", %{conn: conn} do
230 + {owner, repo} = repository_fixture(%{visibility: "public"})
231 + first = pr_with_branches(repo, owner)
232 +
233 + # The owner reviews on one PR, then opens another.
234 + _ = review!(conn, repo, first, owner)
235 + mine = pull_request_fixture(repo, owner, "feature", "main")
236 +
237 + {:ok, lv, _html} = live(log_in_user(conn, owner), pr_path(repo, mine))
238 +
239 + refute has_element?(lv, "button[phx-value-handle='#{owner.handle}']")
240 + end
241 +
242 + test "a repo nobody has reviewed shows no chips", %{conn: conn} do
243 + {owner, repo} = repository_fixture(%{visibility: "public"})
244 + pr = pr_with_branches(repo, owner)
245 +
246 + {:ok, _lv, html} = live(log_in_user(conn, owner), pr_path(repo, pr))
247 +
248 + refute html =~ "Reviews often in this repo"
249 + end
250 +
251 + test "reviewers from another repo aren't suggested here", %{conn: conn} do
252 + {owner_a, repo_a} = repository_fixture(%{visibility: "public"})
253 + pr_a = pr_with_branches(repo_a, owner_a)
254 + elsewhere = user_fixture()
255 + _ = review!(conn, repo_a, pr_a, elsewhere)
256 +
257 + {owner_b, repo_b} = repository_fixture(%{visibility: "public"})
258 + pr_b = pr_with_branches(repo_b, owner_b)
259 +
260 + {:ok, lv, _html} = live(log_in_user(conn, owner_b), pr_path(repo_b, pr_b))
261 +
262 + refute has_element?(lv, "button[phx-value-handle='#{elsewhere.handle}']")
263 + end
264 +
265 + test "the busiest reviewer sorts first", %{conn: conn} do
266 + {owner, repo} = repository_fixture(%{visibility: "public"})
267 + one = pr_with_branches(repo, owner)
268 + two = pull_request_fixture(repo, owner, "feature", "main")
269 +
270 + busy = user_fixture()
271 + quiet = user_fixture()
272 +
273 + _ = review!(conn, repo, one, busy)
274 + _ = review!(conn, repo, two, busy)
275 + _ = review!(conn, repo, one, quiet)
276 +
277 + [{first, count} | _] = PullRequests.suggested_reviewers(repo, [])
278 + assert first.id == busy.id
279 + assert count == 2
280 + end
281 + end
282 +
176 283 describe "the dashboard panel" do
177 284 test "a requested review appears for the reviewer", %{conn: conn} do
178 285 {owner, repo} = repository_fixture(%{visibility: "public"})
modified test/git_gud_web/live/pr_live/review_test.exs
+57 −0
@@ -167,6 +167,63 @@ defmodule GitGudWeb.PrLive.ReviewTest do
167 167 assert kinds == ["reviewed", "closed"]
168 168 end
169 169
170 + test "re-submitting the same verdict adds nothing", %{conn: conn} do
171 + {owner, repo} = repository_fixture(%{visibility: "public"})
172 + pr = pr_with_branches(repo, owner)
173 +
174 + reviewer = user_fixture()
175 + {:ok, lv, _html} = live(log_in_user(conn, reviewer), pr_path(repo, pr))
176 +
177 + _ = submit_review(lv, "approved", "Looks good")
178 + _ = submit_review(lv, "approved", "Looks good")
179 + _ = submit_review(lv, "approved", "Looks good")
180 +
181 + assert length(PullRequests.get_pull_request!(repo, pr.number).reviews) == 1
182 + assert length(Events.list_for(pr)) == 1
183 + end
184 +
185 + test "whitespace-only differences don't count as a change", %{conn: conn} do
186 + {owner, repo} = repository_fixture(%{visibility: "public"})
187 + pr = pr_with_branches(repo, owner)
188 +
189 + reviewer = user_fixture()
190 + {:ok, lv, _html} = live(log_in_user(conn, reviewer), pr_path(repo, pr))
191 +
192 + _ = submit_review(lv, "approved", "Fine")
193 + _ = submit_review(lv, "approved", " Fine ")
194 +
195 + assert length(PullRequests.get_pull_request!(repo, pr.number).reviews) == 1
196 + end
197 +
198 + test "changing the body alone is a real change", %{conn: conn} do
199 + {owner, repo} = repository_fixture(%{visibility: "public"})
200 + pr = pr_with_branches(repo, owner)
201 +
202 + reviewer = user_fixture()
203 + {:ok, lv, _html} = live(log_in_user(conn, reviewer), pr_path(repo, pr))
204 +
205 + _ = submit_review(lv, "approved", "First thought")
206 + _ = submit_review(lv, "approved", "Second thought")
207 +
208 + assert length(PullRequests.get_pull_request!(repo, pr.number).reviews) == 2
209 + end
210 +
211 + test "another reviewer's identical verdict is still their own", %{conn: conn} do
212 + {owner, repo} = repository_fixture(%{visibility: "public"})
213 + pr = pr_with_branches(repo, owner)
214 +
215 + one = user_fixture()
216 + two = user_fixture()
217 +
218 + {:ok, lv1, _html} = live(log_in_user(conn, one), pr_path(repo, pr))
219 + _ = submit_review(lv1, "approved", "LGTM")
220 +
221 + {:ok, lv2, _html} = live(log_in_user(conn, two), pr_path(repo, pr))
222 + _ = submit_review(lv2, "approved", "LGTM")
223 +
224 + assert length(PullRequests.get_pull_request!(repo, pr.number).reviews) == 2
225 + end
226 +
170 227 test "the full log stays hidden while every reviewer has one review", %{conn: conn} do
171 228 {owner, repo} = repository_fixture(%{visibility: "public"})
172 229 pr = pr_with_branches(repo, owner)

Parents: 0f83059