diff --git a/CHANGELOG.md b/CHANGELOG.md index db1ed26b2..cee408bde 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,10 @@ # Unreleased +* (bugfix) Capture a pull request's labels before a label change archives its review stack. + `LabelCapturingHandler` is registered after `LabeledHandler`/`UnlabeledHandler` and skips + archived stacks, so the label event that that archived the stack was never captured, + and the stack kept the label that tore it down. `LabeledHandler` and `UnlabeledHandler` + now capture the payload's labels before archiving. Already-archived stacks and the unarchive path + are unchanged. * (bugfix) Fix task output flickering and freezing on long logs. Clusterize sized its virtual-scroll spacers with an inline style attribute produced by `outerHTML`, which a `style-src` Content Security Policy without `'unsafe-inline'` refuses to apply. The spacers collapsed to zero height, diff --git a/app/models/shipit/webhooks/handlers/pull_request/labeled_handler.rb b/app/models/shipit/webhooks/handlers/pull_request/labeled_handler.rb index 7bfa5ce61..141aced0e 100644 --- a/app/models/shipit/webhooks/handlers/pull_request/labeled_handler.rb +++ b/app/models/shipit/webhooks/handlers/pull_request/labeled_handler.rb @@ -48,6 +48,7 @@ def process def handle if archive? + capture_labels stack.archive! elsif unarchive? stack.unarchive! @@ -56,6 +57,16 @@ def handle stack end + def capture_labels + review_stack = stack.stack + return if review_stack.blank? || review_stack.archived? + + persisted_pull_request = review_stack.pull_request + return if persisted_pull_request.blank? + + persisted_pull_request.update!(labels: pull_request_label_names) + end + def stack @stack ||= Shipit::Webhooks::Handlers::PullRequest::ReviewStackAdapter diff --git a/app/models/shipit/webhooks/handlers/pull_request/unlabeled_handler.rb b/app/models/shipit/webhooks/handlers/pull_request/unlabeled_handler.rb index 42767ae98..de7b6961d 100644 --- a/app/models/shipit/webhooks/handlers/pull_request/unlabeled_handler.rb +++ b/app/models/shipit/webhooks/handlers/pull_request/unlabeled_handler.rb @@ -48,6 +48,7 @@ def process def handle if archive? + capture_labels stack.archive! elsif unarchive? stack.unarchive! @@ -56,6 +57,16 @@ def handle stack end + def capture_labels + review_stack = stack.stack + return if review_stack.blank? || review_stack.archived? + + persisted_pull_request = review_stack.pull_request + return if persisted_pull_request.blank? + + persisted_pull_request.update!(labels: pull_request_label_names) + end + def repository @repository ||= Shipit::Repository.from_github_repo_name(params.repository.full_name) || diff --git a/test/models/shipit/webhooks/handlers/pull_request/labeled_handler_test.rb b/test/models/shipit/webhooks/handlers/pull_request/labeled_handler_test.rb index 03b72eb4a..489c5edcd 100644 --- a/test/models/shipit/webhooks/handlers/pull_request/labeled_handler_test.rb +++ b/test/models/shipit/webhooks/handlers/pull_request/labeled_handler_test.rb @@ -245,6 +245,64 @@ class LabeledHandlerTest < ActiveSupport::TestCase LabeledHandler.new(payload).process end + test "captures the PullRequest labels when adding the provisioning label archives the stack" do + stack = create_stack + repository = shipit_repositories(:shipit) + configure_provisioning_behavior( + repository:, + behavior: :prevent_with_label, + label: "pull-requests-label" + ) + payload = payload_parsed(:pull_request_labeled) + payload["pull_request"]["labels"] = [ + { "name" => "pull-requests-label" }, + { "name" => "ready-for-review" } + ] + + LabeledHandler.new(payload).process + + stack.reload + assert stack.archived?, "Expected stack to be archived" + assert_equal(["pull-requests-label", "ready-for-review"], stack.pull_request.labels) + end + + test "does not capture the PullRequest labels when the stack is already archived" do + stack = create_archived_stack + repository = shipit_repositories(:shipit) + configure_provisioning_behavior( + repository:, + behavior: :prevent_with_label, + label: "pull-requests-label" + ) + payload = payload_parsed(:pull_request_labeled) + payload["pull_request"]["labels"] = [ + { "name" => "pull-requests-label" }, + { "name" => "ready-for-review" } + ] + + LabeledHandler.new(payload).process + + assert_equal(["pull-requests-label"], stack.reload.pull_request.labels) + end + + test "does not capture the PullRequest labels when unarchiving an existing review stack" do + stack = create_archived_stack + repository = shipit_repositories(:shipit) + configure_provisioning_behavior( + repository:, + behavior: :prevent_with_label, + label: "pull-requests-label" + ) + payload = payload_parsed(:pull_request_labeled) + payload["pull_request"]["labels"] = [] + + LabeledHandler.new(payload).process + + stack.reload + assert_not stack.archived?, "Expected stack to NOT be archived" + assert_equal(["pull-requests-label"], stack.pull_request.labels) + end + def configure_provisioning_behavior(repository:, provisioning_enabled: true, behavior: :allow_all, label: nil) repository.review_stacks_enabled = provisioning_enabled repository.provisioning_behavior = behavior diff --git a/test/models/shipit/webhooks/handlers/pull_request/unlabeled_handler_test.rb b/test/models/shipit/webhooks/handlers/pull_request/unlabeled_handler_test.rb index 19fe3a607..1e6827cc1 100644 --- a/test/models/shipit/webhooks/handlers/pull_request/unlabeled_handler_test.rb +++ b/test/models/shipit/webhooks/handlers/pull_request/unlabeled_handler_test.rb @@ -239,6 +239,97 @@ class UnlabeledHandlerTest < ActiveSupport::TestCase UnlabeledHandler.new(payload).process end + test "captures the PullRequest labels when removing the provisioning label archives the stack" do + stack = create_stack + repository = shipit_repositories(:shipit) + configure_provisioning_behavior( + repository:, + behavior: :allow_with_label, + label: "pull-requests-label" + ) + payload = payload_parsed(:pull_request_unlabeled) + payload["pull_request"]["labels"] = [{ "name" => "ready-for-review" }] + + UnlabeledHandler.new(payload).process + + stack.reload + assert stack.archived?, "Expected stack to be archived" + assert_equal(["ready-for-review"], stack.pull_request.labels) + end + + test "captures an empty label list when removing the last label archives the stack" do + stack = create_stack + repository = shipit_repositories(:shipit) + configure_provisioning_behavior( + repository:, + behavior: :allow_with_label, + label: "pull-requests-label" + ) + payload = payload_parsed(:pull_request_unlabeled) + payload["pull_request"]["labels"] = [] + + UnlabeledHandler.new(payload).process + + stack.reload + assert stack.archived?, "Expected stack to be archived" + assert_empty(stack.pull_request.labels) + end + + test "does not capture the PullRequest labels when the stack is already archived" do + stack = create_archived_stack + repository = shipit_repositories(:shipit) + configure_provisioning_behavior( + repository:, + behavior: :allow_with_label, + label: "pull-requests-label" + ) + payload = payload_parsed(:pull_request_unlabeled) + payload["pull_request"]["labels"] = [{ "name" => "ready-for-review" }] + + UnlabeledHandler.new(payload).process + + assert_equal(["pull-requests-label"], stack.reload.pull_request.labels) + end + + test "does not capture the PullRequest labels when unarchiving an existing review stack" do + stack = create_archived_stack + repository = shipit_repositories(:shipit) + configure_provisioning_behavior( + repository:, + behavior: :allow_with_label, + label: "pull-requests-label" + ) + payload = payload_parsed(:pull_request_unlabeled) + payload["pull_request"]["labels"] = [ + { "name" => "pull-requests-label" }, + { "name" => "ready-for-review" } + ] + + UnlabeledHandler.new(payload).process + + stack.reload + assert_not stack.archived?, "Expected stack to NOT be archived" + assert_equal(["pull-requests-label"], stack.pull_request.labels) + end + + test "the removed provisioning label does not survive the whole pull_request handler chain" do + stack = create_stack + repository = shipit_repositories(:shipit) + configure_provisioning_behavior( + repository:, + behavior: :allow_with_label, + label: "pull-requests-label" + ) + payload = payload_parsed(:pull_request_unlabeled) + payload["pull_request"]["labels"] = [{ "name" => "ready-for-review" }] + + Shipit::Webhooks.for_event("pull_request").each { |handler| handler.call(payload) } + + stack.reload + assert stack.archived?, "Expected stack to be archived" + assert_equal(["ready-for-review"], stack.pull_request.labels) + end + def configure_provisioning_behavior(repository:, provisioning_enabled: true, behavior: :allow_all, label: nil) repository.review_stacks_enabled = provisioning_enabled repository.provisioning_behavior = behavior