neiam /gitgud
Git Gud
public · Issues · Pulls · Labels · Forks · Compare · Actions success · Packages
⭐
Log in to mark this repository.
Require N approving reviews to merge, with an admin override
c56c729 · Gabriel Morell · 2026-09-10 14:45
Message
{commit_body(@commit)}
Files changed
modified
lib/git_gud/pull_requests.ex
+49
−12
@@ -589,11 +589,13 @@ defmodule GitGud.PullRequests do
| 589 | 589 | ref between probe and merge, we surface a `:concurrent_update` error |
| 590 | 590 | and leave the PR untouched. |
| 591 | 591 | """ |
| 592 | − def merge!(%PullRequest{} = pr, %User{} = actor, message \\ nil) do | |
| 592 | + def merge!(%PullRequest{} = pr, %User{} = actor, opts \\ []) do | |
| 593 | + {message, opts} = pop_message(opts) | |
| 594 | + override? = Keyword.get(opts, :override, false) | |
| 593 | 595 | repo = Repositories.get_repository!(pr.repository_id) |
| 594 | 596 | path = repo.disk_path |
| 595 | 597 | |
| 596 | − with :ok <- check_review_block(repo, pr), | |
| 598 | + with :ok <- check_review_block(repo, pr, override?), | |
| 597 | 599 | {:ok, current_target_sha} <- Repositories.resolve(repo, pr.target_ref), |
| 598 | 600 | :ok <- |
| 599 | 601 | BranchProtections.check_ref_update( |
@@ -922,24 +924,59 @@ defmodule GitGud.PullRequests do
| 922 | 924 | |> Enum.filter(&(&1.state == "changes_requested")) |
| 923 | 925 | end |
| 924 | 926 | |
| 927 | + @doc "How many reviewers currently approve — latest verdict each." | |
| 928 | + def approval_count(%PullRequest{} = pr) do | |
| 929 | + pr |> latest_reviews() |> Enum.count(&(&1.state == "approved")) | |
| 930 | + end | |
| 931 | + | |
| 925 | 932 | @doc """ |
| 926 | − Whether `pr` is merge-blocked by review. Only when the repo opted in | |
| 927 | − via `block_merge_on_changes_requested`. | |
| 933 | + Every repo rule this PR currently fails, as a list of reasons: | |
| 934 | + | |
| 935 | + * `{:changes_requested, [reviewer_name]}` | |
| 936 | + * `{:insufficient_approvals, have, need}` | |
| 937 | + | |
| 938 | + Empty means the PR satisfies the repo's review rules. Both rules are | |
| 939 | + opt-in per repo and default to off. | |
| 928 | 940 | """ |
| 929 | − def review_blocked?(%Repository{block_merge_on_changes_requested: false}, _pr), do: false | |
| 941 | + def merge_blockers(%Repository{} = repo, %PullRequest{} = pr) do | |
| 942 | + changes = | |
| 943 | + if repo.block_merge_on_changes_requested and blocking_reviews(pr) != [] do | |
| 944 | + [{:changes_requested, Enum.map(blocking_reviews(pr), &reviewer_label/1)}] | |
| 945 | + else | |
| 946 | + [] | |
| 947 | + end | |
| 948 | + | |
| 949 | + approvals = | |
| 950 | + case {repo.required_approvals || 0, approval_count(pr)} do | |
| 951 | + {need, have} when need > 0 and have < need -> [{:insufficient_approvals, have, need}] | |
| 952 | + _ -> [] | |
| 953 | + end | |
| 954 | + | |
| 955 | + changes ++ approvals | |
| 956 | + end | |
| 957 | + | |
| 958 | + @doc "Whether `pr` currently fails any of the repo's review rules." | |
| 959 | + def review_blocked?(%Repository{} = repo, %PullRequest{} = pr), | |
| 960 | + do: merge_blockers(repo, pr) != [] | |
| 930 | 961 | |
| 931 | − def review_blocked?(%Repository{}, %PullRequest{} = pr), do: blocking_reviews(pr) != [] | |
| 962 | + # merge!/3 used to take a bare message string; keep that working. | |
| 963 | + defp pop_message(opts) when is_binary(opts), do: {opts, []} | |
| 964 | + defp pop_message(nil), do: {nil, []} | |
| 965 | + defp pop_message(opts) when is_list(opts), do: {Keyword.get(opts, :message), opts} | |
| 966 | + | |
| 967 | + defp reviewer_label(%{reviewer: %{handle: h}}) when is_binary(h), do: h | |
| 968 | + defp reviewer_label(_), do: "a reviewer" | |
| 932 | 969 | |
| 933 | 970 | # `merge!/3` may be handed a PR without its reviews loaded, so this |
| 934 | 971 | # reloads rather than trusting the struct it was given. |
| 935 | − defp check_review_block(%Repository{block_merge_on_changes_requested: false}, _pr), do: :ok | |
| 936 | − | |
| 937 | − defp check_review_block(%Repository{} = repo, %PullRequest{} = pr) do | |
| 972 | + defp check_review_block(%Repository{} = repo, %PullRequest{} = pr, override?) do | |
| 938 | 973 | loaded = get_pull_request!(repo, pr.number) |
| 939 | 974 | |
| 940 | − if blocking_reviews(loaded) == [], | |
| 941 | − do: :ok, | |
| 942 | − else: {:error, :changes_requested} | |
| 975 | + case merge_blockers(repo, loaded) do | |
| 976 | + [] -> :ok | |
| 977 | + _ when override? -> :ok | |
| 978 | + blockers -> {:error, {:review_rules, blockers}} | |
| 979 | + end | |
| 943 | 980 | end |
| 944 | 981 | |
| 945 | 982 | @doc """ |
modified
lib/git_gud/repositories/repository.ex
+8
−1
@@ -21,6 +21,8 @@ defmodule GitGud.Repositories.Repository do
| 21 | 21 | field :archived, :boolean, default: false |
| 22 | 22 | # When true, a standing "changes requested" review blocks merging. |
| 23 | 23 | field :block_merge_on_changes_requested, :boolean, default: false |
| 24 | + # Approving reviews needed before merging. 0 = no requirement. | |
| 25 | + field :required_approvals, :integer, default: 0 | |
| 24 | 26 | # SHA-256 of the current runner registration token, if any. |
| 25 | 27 | # Regenerating the token replaces the hash, invalidating the old |
| 26 | 28 | # token immediately. |
@@ -67,12 +69,17 @@ defmodule GitGud.Repositories.Repository do
| 67 | 69 | :visibility, |
| 68 | 70 | :archived, |
| 69 | 71 | :interaction_policy, |
| 70 | − :block_merge_on_changes_requested | |
| 72 | + :block_merge_on_changes_requested, | |
| 73 | + :required_approvals | |
| 71 | 74 | ]) |
| 72 | 75 | |> validate_inclusion(:visibility, @valid_visibilities) |
| 73 | 76 | |> validate_inclusion(:interaction_policy, @valid_interaction_policies) |
| 74 | 77 | |> validate_length(:default_branch, max: 100) |
| 75 | 78 | |> validate_length(:description, max: 1000) |
| 79 | + |> validate_number(:required_approvals, | |
| 80 | + greater_than_or_equal_to: 0, | |
| 81 | + less_than_or_equal_to: 20 | |
| 82 | + ) | |
| 76 | 83 | end |
| 77 | 84 | |
| 78 | 85 | def valid_interaction_policies, do: @valid_interaction_policies |
modified
lib/git_gud_web/components/event_components.ex
+4
−0
@@ -171,6 +171,10 @@ defmodule GitGudWeb.EventComponents do
| 171 | 171 | defp describe(%{kind: "opened"}), do: "opened this." |
| 172 | 172 | defp describe(%{kind: "closed"}), do: "closed this." |
| 173 | 173 | defp describe(%{kind: "reopened"}), do: "reopened this." |
| 174 | + | |
| 175 | + defp describe(%{kind: "merged", data: %{"override" => true}}), | |
| 176 | + do: "merged this, overriding the repo's review rules." | |
| 177 | + | |
| 174 | 178 | defp describe(%{kind: "merged"}), do: "merged this." |
| 175 | 179 | |
| 176 | 180 | defp describe(%{kind: "title_changed", data: %{"from" => from, "to" => to}}) do |
modified
lib/git_gud_web/live/pr_live/reviews.ex
+1
−1
@@ -128,7 +128,7 @@ defmodule GitGudWeb.PrLive.Reviews do
| 128 | 128 | |
| 129 | 129 | :ok = |
| 130 | 130 | Events.record(pr, "review_request_removed", viewer(socket), %{ |
| 131 | − reviewer: request.reviewer && request.reviewer.handle | |
| 131 | + reviewer: request.reviewer.handle | |
| 132 | 132 | }) |
| 133 | 133 | |
| 134 | 134 | {:noreply, reload(socket)} |
modified
lib/git_gud_web/live/pr_live/show.ex
+42
−16
@@ -134,7 +134,13 @@ defmodule GitGudWeb.PrLive.Show do
| 134 | 134 | defp blocking_reviewers(pr), |
| 135 | 135 | do: pr |> PullRequests.blocking_reviews() |> Enum.map(&reviewer_name(&1.reviewer)) |
| 136 | 136 | |
| 137 | − defp review_blocked?(repo, pr), do: PullRequests.review_blocked?(repo, pr) | |
| 137 | + defp merge_blockers(repo, pr), do: PullRequests.merge_blockers(repo, pr) | |
| 138 | + | |
| 139 | + defp blocker_text({:changes_requested, who}), | |
| 140 | + do: "#{Enum.join(who, ", ")} requested changes. Merging is blocked until that changes." | |
| 141 | + | |
| 142 | + defp blocker_text({:insufficient_approvals, have, need}), | |
| 143 | + do: "This pull request has #{have} of #{need} required approving review(s)." | |
| 138 | 144 | |
| 139 | 145 | defp review_word("approved"), do: "approved" |
| 140 | 146 | defp review_word("changes_requested"), do: "requested changes" |
@@ -356,12 +362,15 @@ defmodule GitGudWeb.PrLive.Show do
| 356 | 362 | end |
| 357 | 363 | end |
| 358 | 364 | |
| 359 | − def handle_event("merge", _params, socket) do | |
| 365 | + def handle_event("merge", params, socket) do | |
| 360 | 366 | user = socket.assigns.current_scope.user |
| 367 | + # Only an admin's override counts — the button is theirs alone, and | |
| 368 | + # the parameter isn't trusted on its own. | |
| 369 | + override? = params["override"] == "1" and socket.assigns.can_admin? | |
| 361 | 370 | |
| 362 | − case PullRequests.merge!(socket.assigns.pr, user) do | |
| 371 | + case PullRequests.merge!(socket.assigns.pr, user, override: override?) do | |
| 363 | 372 | {:ok, _pr} -> |
| 364 | − :ok = Events.record(socket.assigns.pr, "merged", user) | |
| 373 | + :ok = Events.record(socket.assigns.pr, "merged", user, %{override: override?}) | |
| 365 | 374 | |
| 366 | 375 | {:noreply, |
| 367 | 376 | socket |
@@ -369,9 +378,14 @@ defmodule GitGudWeb.PrLive.Show do
| 369 | 378 | |> assign(:event_count, Events.count_for(socket.assigns.pr)) |
| 370 | 379 | |> reload()} |
| 371 | 380 | |
| 372 | − {:error, :changes_requested} -> | |
| 381 | + {:error, {:review_rules, blockers}} -> | |
| 373 | 382 | {:noreply, |
| 374 | − put_flash(socket, :error, "A reviewer has requested changes on this pull request.")} | |
| 383 | + put_flash( | |
| 384 | + socket, | |
| 385 | + :error, | |
| 386 | + "Blocked by this repo's review rules: " <> | |
| 387 | + Enum.map_join(blockers, " ", &blocker_text/1) | |
| 388 | + )} | |
| 375 | 389 | |
| 376 | 390 | {:error, :conflict} -> |
| 377 | 391 | {:noreply, put_flash(socket, :error, "Merge conflict — cannot merge.")} |
@@ -606,17 +620,18 @@ defmodule GitGudWeb.PrLive.Show do
| 606 | 620 | <.icon name="hero-clock" class="size-4 opacity-60" /> Mergeability not yet checked. |
| 607 | 621 | <% end %> |
| 608 | 622 | </p> |
| 609 | − <p :if={blocking_reviewers(@pr) != []} class="text-sm flex items-start gap-1.5"> | |
| 623 | + <p :for={blocker <- merge_blockers(@repo, @pr)} class="text-sm flex items-start gap-1.5"> | |
| 610 | 624 | <.icon name="hero-hand-raised" class="size-4 text-error shrink-0 mt-0.5" /> |
| 625 | + <span>{blocker_text(blocker)}</span> | |
| 626 | + </p> | |
| 627 | + <p | |
| 628 | + :if={merge_blockers(@repo, @pr) == [] and blocking_reviewers(@pr) != []} | |
| 629 | + class="text-sm flex items-start gap-1.5" | |
| 630 | + > | |
| 631 | + <.icon name="hero-hand-raised" class="size-4 text-warning shrink-0 mt-0.5" /> | |
| 611 | 632 | <span> |
| 612 | − {Enum.join(blocking_reviewers(@pr), ", ")} requested changes.{if review_blocked?( | |
| 613 | − @repo, | |
| 614 | − @pr | |
| 615 | − ), | |
| 616 | − do: | |
| 617 | − " Merging is blocked until that verdict changes.", | |
| 618 | − else: | |
| 619 | − " You can still merge."} | |
| 633 | + {Enum.join(blocking_reviewers(@pr), ", ")} requested changes. This repo | |
| 634 | + doesn't enforce that, so you can still merge. | |
| 620 | 635 | </span> |
| 621 | 636 | </p> |
| 622 | 637 | <div class="flex gap-2"> |
@@ -624,10 +639,21 @@ defmodule GitGudWeb.PrLive.Show do
| 624 | 639 | type="button" |
| 625 | 640 | phx-click="merge" |
| 626 | 641 | class="btn btn-sm btn-primary" |
| 627 | − disabled={@pr.mergeable != true or review_blocked?(@repo, @pr)} | |
| 642 | + disabled={@pr.mergeable != true or merge_blockers(@repo, @pr) != []} | |
| 628 | 643 | > |
| 629 | 644 | Merge |
| 630 | 645 | </button> |
| 646 | + <button | |
| 647 | + :if={merge_blockers(@repo, @pr) != [] and @can_admin?} | |
| 648 | + type="button" | |
| 649 | + phx-click="merge" | |
| 650 | + phx-value-override="1" | |
| 651 | + class="btn btn-sm btn-outline btn-warning" | |
| 652 | + disabled={@pr.mergeable != true} | |
| 653 | + data-confirm="This pull request doesn't satisfy the repo's review rules. Merge anyway?" | |
| 654 | + > | |
| 655 | + Override and merge | |
| 656 | + </button> | |
| 631 | 657 | <button |
| 632 | 658 | :if={can_change_state?(assigns)} |
| 633 | 659 | type="button" |
modified
lib/git_gud_web/live/repo_live/settings.ex
+11
−0
@@ -201,6 +201,17 @@ defmodule GitGudWeb.RepoLive.Settings do
| 201 | 201 | A reviewer's standing "changes requested" stops the merge until they |
| 202 | 202 | change their verdict. Off by default. |
| 203 | 203 | </p> |
| 204 | + <.input | |
| 205 | + field={@form[:required_approvals]} | |
| 206 | + type="number" | |
| 207 | + min="0" | |
| 208 | + max="20" | |
| 209 | + label="Approving reviews required to merge" | |
| 210 | + /> | |
| 211 | + <p class="text-xs opacity-60 -mt-2"> | |
| 212 | + Zero means no requirement. Repo admins can override either rule when | |
| 213 | + merging. | |
| 214 | + </p> | |
| 204 | 215 | <div> |
| 205 | 216 | <button class="btn btn-primary btn-sm" type="submit">Save</button> |
| 206 | 217 | </div> |
added
priv/repo/migrations/20260910030000_add_required_approvals.exs
+13
−0
@@ -0,0 +1,13 @@
| 1 | +defmodule GitGud.Repo.Migrations.AddRequiredApprovals do | |
| 2 | + use Ecto.Migration | |
| 3 | + | |
| 4 | + # How many approving reviews a PR needs before it can merge. | |
| 5 | + # | |
| 6 | + # Zero means no requirement, which is what every existing repo gets — | |
| 7 | + # turning the rule on is each repo's decision. | |
| 8 | + def change do | |
| 9 | + alter table(:repositories) do | |
| 10 | + add :required_approvals, :integer, null: false, default: 0 | |
| 11 | + end | |
| 12 | + end | |
| 13 | +end |
added
test/git_gud_web/live/pr_live/required_approvals_test.exs
+225
−0
@@ -0,0 +1,225 @@
| 1 | +defmodule GitGudWeb.PrLive.RequiredApprovalsTest do | |
| 2 | + @moduledoc """ | |
| 3 | + The per-repo rule requiring N approving reviews before a merge, and | |
| 4 | + the admin override that gets past it. | |
| 5 | + """ | |
| 6 | + | |
| 7 | + use GitGudWeb.ConnCase, async: false | |
| 8 | + | |
| 9 | + import Phoenix.LiveViewTest | |
| 10 | + import GitGud.AccountsFixtures | |
| 11 | + import GitGud.ForgeFixtures | |
| 12 | + | |
| 13 | + alias GitGud.Events | |
| 14 | + alias GitGud.PullRequests | |
| 15 | + alias GitGud.Repositories | |
| 16 | + | |
| 17 | + defp pr_path(repo, pr) do | |
| 18 | + handle = Repositories.Storage.repo_handle(GitGud.Repo.preload(repo, [:owner, :organization])) | |
| 19 | + ~p"/r/#{handle}/#{repo.name}/pulls/#{pr.number}" | |
| 20 | + end | |
| 21 | + | |
| 22 | + defp require_approvals!(repo, n) do | |
| 23 | + {:ok, repo} = Repositories.update_repository(repo, %{"required_approvals" => n}) | |
| 24 | + repo | |
| 25 | + end | |
| 26 | + | |
| 27 | + defp approve!(conn, repo, pr, reviewer) do | |
| 28 | + {:ok, lv, _html} = live(log_in_user(conn, reviewer), pr_path(repo, pr) <> "/reviews") | |
| 29 | + | |
| 30 | + lv | |
| 31 | + |> form("#review-form", %{"pr_review" => %{"state" => "approved", "body" => ""}}) | |
| 32 | + |> render_submit() | |
| 33 | + end | |
| 34 | + | |
| 35 | + test "zero required means no rule, as before", %{conn: conn} do | |
| 36 | + {owner, repo} = repository_fixture(%{visibility: "public"}) | |
| 37 | + pr = pr_with_branches(repo, owner) | |
| 38 | + | |
| 39 | + assert repo.required_approvals == 0 | |
| 40 | + | |
| 41 | + assert PullRequests.merge_blockers(repo, PullRequests.get_pull_request!(repo, pr.number)) == | |
| 42 | + [] | |
| 43 | + | |
| 44 | + {:ok, lv, _html} = live(log_in_user(conn, owner), pr_path(repo, pr)) | |
| 45 | + refute has_element?(lv, "button", "Override and merge") | |
| 46 | + end | |
| 47 | + | |
| 48 | + test "an unmet requirement blocks and says the count", %{conn: conn} do | |
| 49 | + {owner, repo} = repository_fixture(%{visibility: "public"}) | |
| 50 | + pr = pr_with_branches(repo, owner) | |
| 51 | + repo = require_approvals!(repo, 2) | |
| 52 | + | |
| 53 | + {:ok, lv, html} = live(log_in_user(conn, owner), pr_path(repo, pr)) | |
| 54 | + | |
| 55 | + assert html =~ "has 0 of 2 required approving review(s)" | |
| 56 | + assert has_element?(lv, "button[phx-click=merge][disabled]") | |
| 57 | + end | |
| 58 | + | |
| 59 | + test "the count reflects approvals as they arrive", %{conn: conn} do | |
| 60 | + {owner, repo} = repository_fixture(%{visibility: "public"}) | |
| 61 | + pr = pr_with_branches(repo, owner) | |
| 62 | + repo = require_approvals!(repo, 2) | |
| 63 | + | |
| 64 | + _ = approve!(conn, repo, pr, user_fixture()) | |
| 65 | + | |
| 66 | + {:ok, _lv, html} = live(log_in_user(conn, owner), pr_path(repo, pr)) | |
| 67 | + assert html =~ "has 1 of 2 required approving review(s)" | |
| 68 | + end | |
| 69 | + | |
| 70 | + test "the block lifts once enough people approve", %{conn: conn} do | |
| 71 | + {owner, repo} = repository_fixture(%{visibility: "public"}) | |
| 72 | + pr = pr_with_branches(repo, owner) | |
| 73 | + repo = require_approvals!(repo, 2) | |
| 74 | + | |
| 75 | + _ = approve!(conn, repo, pr, user_fixture()) | |
| 76 | + _ = approve!(conn, repo, pr, user_fixture()) | |
| 77 | + | |
| 78 | + {:ok, _lv, html} = live(log_in_user(conn, owner), pr_path(repo, pr)) | |
| 79 | + refute html =~ "required approving review(s)" | |
| 80 | + end | |
| 81 | + | |
| 82 | + test "one reviewer approving twice still counts once", %{conn: conn} do | |
| 83 | + {owner, repo} = repository_fixture(%{visibility: "public"}) | |
| 84 | + pr = pr_with_branches(repo, owner) | |
| 85 | + repo = require_approvals!(repo, 2) | |
| 86 | + | |
| 87 | + reviewer = user_fixture() | |
| 88 | + _ = approve!(conn, repo, pr, reviewer) | |
| 89 | + _ = approve!(conn, repo, pr, reviewer) | |
| 90 | + | |
| 91 | + reloaded = PullRequests.get_pull_request!(repo, pr.number) | |
| 92 | + assert PullRequests.approval_count(reloaded) == 1 | |
| 93 | + assert [{:insufficient_approvals, 1, 2}] = PullRequests.merge_blockers(repo, reloaded) | |
| 94 | + end | |
| 95 | + | |
| 96 | + test "a reviewer who switches away from approving stops counting", %{conn: conn} do | |
| 97 | + {owner, repo} = repository_fixture(%{visibility: "public"}) | |
| 98 | + pr = pr_with_branches(repo, owner) | |
| 99 | + repo = require_approvals!(repo, 1) | |
| 100 | + | |
| 101 | + reviewer = user_fixture() | |
| 102 | + _ = approve!(conn, repo, pr, reviewer) | |
| 103 | + | |
| 104 | + {:ok, lv, _html} = live(log_in_user(conn, reviewer), pr_path(repo, pr) <> "/reviews") | |
| 105 | + | |
| 106 | + _ = | |
| 107 | + lv | |
| 108 | + |> form("#review-form", %{"pr_review" => %{"state" => "commented", "body" => "hmm"}}) | |
| 109 | + |> render_submit() | |
| 110 | + | |
| 111 | + reloaded = PullRequests.get_pull_request!(repo, pr.number) | |
| 112 | + assert PullRequests.approval_count(reloaded) == 0 | |
| 113 | + end | |
| 114 | + | |
| 115 | + test "merge!/3 refuses without the override", %{conn: conn} do | |
| 116 | + {owner, repo} = repository_fixture(%{visibility: "public"}) | |
| 117 | + pr = pr_with_branches(repo, owner) | |
| 118 | + repo = require_approvals!(repo, 1) | |
| 119 | + | |
| 120 | + reloaded = PullRequests.get_pull_request!(repo, pr.number) | |
| 121 | + | |
| 122 | + assert {:error, {:review_rules, [{:insufficient_approvals, 0, 1}]}} = | |
| 123 | + PullRequests.merge!(reloaded, owner) | |
| 124 | + | |
| 125 | + _ = conn | |
| 126 | + end | |
| 127 | + | |
| 128 | + test "merge!/3 goes through with the override", %{conn: _conn} do | |
| 129 | + {owner, repo} = repository_fixture(%{visibility: "public"}) | |
| 130 | + pr = pr_with_branches(repo, owner) | |
| 131 | + repo = require_approvals!(repo, 1) | |
| 132 | + | |
| 133 | + reloaded = PullRequests.get_pull_request!(repo, pr.number) | |
| 134 | + assert {:ok, _} = PullRequests.merge!(reloaded, owner, override: true) | |
| 135 | + assert PullRequests.get_pull_request!(repo, pr.number).state == "merged" | |
| 136 | + end | |
| 137 | + | |
| 138 | + test "an admin gets an override button and it merges", %{conn: conn} do | |
| 139 | + {owner, repo} = repository_fixture(%{visibility: "public"}) | |
| 140 | + pr = pr_with_branches(repo, owner) | |
| 141 | + repo = require_approvals!(repo, 1) | |
| 142 | + | |
| 143 | + {:ok, lv, _html} = live(log_in_user(conn, owner), pr_path(repo, pr)) | |
| 144 | + assert has_element?(lv, "button[phx-value-override='1']") | |
| 145 | + | |
| 146 | + render_hook(lv, "merge", %{"override" => "1"}) | |
| 147 | + | |
| 148 | + assert PullRequests.get_pull_request!(repo, pr.number).state == "merged" | |
| 149 | + end | |
| 150 | + | |
| 151 | + test "a non-admin can't override even by sending the flag", %{conn: conn} do | |
| 152 | + {_owner, repo} = repository_fixture(%{visibility: "public"}) | |
| 153 | + author = user_fixture() | |
| 154 | + pr = pr_with_branches(repo, author) | |
| 155 | + repo = require_approvals!(repo, 1) | |
| 156 | + | |
| 157 | + {:ok, lv, _html} = live(log_in_user(conn, author), pr_path(repo, pr)) | |
| 158 | + | |
| 159 | + refute has_element?(lv, "button[phx-value-override='1']") | |
| 160 | + render_hook(lv, "merge", %{"override" => "1"}) | |
| 161 | + | |
| 162 | + assert PullRequests.get_pull_request!(repo, pr.number).state == "open" | |
| 163 | + end | |
| 164 | + | |
| 165 | + test "an override is recorded as such in the history", %{conn: conn} do | |
| 166 | + {owner, repo} = repository_fixture(%{visibility: "public"}) | |
| 167 | + pr = pr_with_branches(repo, owner) | |
| 168 | + repo = require_approvals!(repo, 1) | |
| 169 | + | |
| 170 | + {:ok, lv, _html} = live(log_in_user(conn, owner), pr_path(repo, pr)) | |
| 171 | + render_hook(lv, "merge", %{"override" => "1"}) | |
| 172 | + | |
| 173 | + assert [%{kind: "merged", data: %{"override" => true}}] = Events.list_for(pr) | |
| 174 | + | |
| 175 | + {:ok, _hist, html} = live(log_in_user(conn, owner), pr_path(repo, pr) <> "/history") | |
| 176 | + assert html =~ "overriding the repo" | |
| 177 | + assert html =~ "review rules" | |
| 178 | + end | |
| 179 | + | |
| 180 | + test "both rules can block at once and both are named", %{conn: conn} do | |
| 181 | + {owner, repo} = repository_fixture(%{visibility: "public"}) | |
| 182 | + pr = pr_with_branches(repo, owner) | |
| 183 | + | |
| 184 | + {:ok, repo} = | |
| 185 | + Repositories.update_repository(repo, %{ | |
| 186 | + "required_approvals" => 2, | |
| 187 | + "block_merge_on_changes_requested" => true | |
| 188 | + }) | |
| 189 | + | |
| 190 | + objector = user_fixture() | |
| 191 | + {:ok, lv, _html} = live(log_in_user(conn, objector), pr_path(repo, pr) <> "/reviews") | |
| 192 | + | |
| 193 | + _ = | |
| 194 | + lv | |
| 195 | + |> form("#review-form", %{"pr_review" => %{"state" => "changes_requested", "body" => "no"}}) | |
| 196 | + |> render_submit() | |
| 197 | + | |
| 198 | + {:ok, _lv, html} = live(log_in_user(conn, owner), pr_path(repo, pr)) | |
| 199 | + | |
| 200 | + assert html =~ "requested changes" | |
| 201 | + assert html =~ "required approving review(s)" | |
| 202 | + end | |
| 203 | + | |
| 204 | + test "an admin can set the requirement from repo settings", %{conn: conn} do | |
| 205 | + {owner, repo} = repository_fixture() | |
| 206 | + handle = Repositories.Storage.repo_handle(GitGud.Repo.preload(repo, [:owner, :organization])) | |
| 207 | + | |
| 208 | + {:ok, lv, html} = live(log_in_user(conn, owner), ~p"/r/#{handle}/#{repo.name}/settings") | |
| 209 | + assert html =~ "Approving reviews required to merge" | |
| 210 | + | |
| 211 | + _ = | |
| 212 | + lv | |
| 213 | + |> form("form[phx-submit=save]", %{ | |
| 214 | + "repository" => %{ | |
| 215 | + "description" => repo.description || "", | |
| 216 | + "visibility" => repo.visibility, | |
| 217 | + "default_branch" => repo.default_branch, | |
| 218 | + "required_approvals" => "3" | |
| 219 | + } | |
| 220 | + }) | |
| 221 | + |> render_submit() | |
| 222 | + | |
| 223 | + assert Repositories.get_repository!(repo.id).required_approvals == 3 | |
| 224 | + end | |
| 225 | +end |
modified
test/git_gud_web/live/pr_live/review_block_test.exs
+5
−4
@@ -54,7 +54,7 @@ defmodule GitGudWeb.PrLive.ReviewBlockTest do
| 54 | 54 | {:ok, _lv, html} = live(log_in_user(conn, owner), pr_path(repo, pr)) |
| 55 | 55 | |
| 56 | 56 | assert html =~ "requested changes." |
| 57 | − assert html =~ "You can still merge." | |
| 57 | + assert html =~ "so you can still merge" | |
| 58 | 58 | end |
| 59 | 59 | |
| 60 | 60 | test "with the setting on, the merge button is disabled and says why", %{conn: conn} do |
@@ -67,7 +67,7 @@ defmodule GitGudWeb.PrLive.ReviewBlockTest do
| 67 | 67 | |
| 68 | 68 | {:ok, lv, html} = live(log_in_user(conn, owner), pr_path(repo, pr)) |
| 69 | 69 | |
| 70 | − assert html =~ "Merging is blocked until that verdict changes." | |
| 70 | + assert html =~ "Merging is blocked until that changes." | |
| 71 | 71 | assert has_element?(lv, "button[phx-click=merge][disabled]") |
| 72 | 72 | end |
| 73 | 73 |
@@ -82,7 +82,7 @@ defmodule GitGudWeb.PrLive.ReviewBlockTest do
| 82 | 82 | {:ok, lv, _html} = live(log_in_user(conn, owner), pr_path(repo, pr)) |
| 83 | 83 | html = render_hook(lv, "merge", %{}) |
| 84 | 84 | |
| 85 | − assert html =~ "A reviewer has requested changes" | |
| 85 | + assert html =~ "Blocked by this repo" | |
| 86 | 86 | assert PullRequests.get_pull_request!(repo, pr.number).state == "open" |
| 87 | 87 | end |
| 88 | 88 |
@@ -95,7 +95,8 @@ defmodule GitGudWeb.PrLive.ReviewBlockTest do
| 95 | 95 | repo = block!(repo) |
| 96 | 96 | |
| 97 | 97 | reloaded = PullRequests.get_pull_request!(repo, pr.number) |
| 98 | − assert {:error, :changes_requested} = PullRequests.merge!(reloaded, owner) | |
| 98 | + assert {:error, {:review_rules, [{:changes_requested, _}]}} = | |
| 99 | + PullRequests.merge!(reloaded, owner) | |
| 99 | 100 | end |
| 100 | 101 | |
| 101 | 102 | test "the block lifts when the reviewer changes their verdict", %{conn: conn} do |
Parents: f0ed5e3