Capture pull request labels before archiving a review stack

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,

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

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) ||

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

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