Skip to content

Show both the delayed and external-lock stack banners at once #1491

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ScottifyShopson wants to merge 1 commit into from
Closed

Show both the delayed and external-lock stack banners at once #1491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion app/views/shipit/stacks/_banners.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,9 @@
</div>
</div>
</div>
<% elsif !stack.deployment_checks_passed? %>
<% end %>

<% if !stack.deployment_checks_passed? %>
<div class="banner banner--orange">
<div class="banner__inner wrapper">
<div class="banner__content">
Expand Down
20 changes: 20 additions & 0 deletions test/controllers/stacks_controller_test.rb
View file
Open in desktop

@tdickers tdickers Jun 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Suggestion: The new test covers the important positive case where delayed CD and failing deployment checks both render. A sibling test for delayed CD with passing deployment checks would harden the opposite side of the contract: the orange external-lock banner should not render just because delayed CD is active.

Suggestion: Add a sibling test that registers a passing fake deployment check while continuous_delivery_delayed? is true, then asserts the blue 'Continuous Delivery Delayed!' banner renders and the orange 'Stack has been locked by external system!' banner has count: 0.

Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,26 @@ class StacksControllerTest < ActionController::TestCase
assert_select 'a[href="http://google.com"]'
end

test "#show renders both the delayed and external-lock banners when both apply" do
failing_checks = Class.new do

@tdickers tdickers Jun 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Improvement: Consider tightening this regression test so it verifies the specific user-facing reason that motivated the PR. The external deployment-lock reason is surfaced through Shipit.deployment_checks.message(stack) when the checker responds to message. As written, the fake checker only returns false and the assertion only checks the orange banner title, so the test would still pass if the banner appeared but the external-system reason was missing or replaced by a generic message.

Suggestion: Define self.message(_stack) on failing_checks with a distinct sentinel string, then assert that string appears inside the orange banner text after get :show. That keeps the test aligned with the deployment_checks_message contract and the user-visible reason this PR is protecting.

def self.call(_stack)
false
end
end
Shipit.deployment_checks = failing_checks
@stack.update!(continuous_deployment: true, continuous_delivery_delayed_since: Time.current)
assert @stack.continuous_delivery_delayed?
refute @stack.deployment_checks_passed?

get :show, params: { id: @stack.to_param }

assert_response :ok
assert_select '.banner .banner__title', text: 'Continuous Delivery Delayed!'
assert_select '.banner--orange .banner__title', text: 'Stack has been locked by external system!'
ensure
Shipit.deployment_checks = nil

@tdickers tdickers Jun 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 Code Style: The test mutates Shipit.deployment_checks, a module-level global. Resetting it to nil is equivalent today because the attribute has no default, but it could clobber suite-level or setup-provided configuration if that changes later. Restoring the previous value also matches the safer pattern already used for this same global elsewhere in the test suite.

Suggestion: Capture the previous value before assigning failing_checks, then restore that value in the ensure block instead of unconditionally setting Shipit.deployment_checks = nil.

Suggested change
Shipit.deployment_checks = nil
Shipit.deployment_checks = original_deployment_checks

end

test "#create creates a Stack, queues a job to setup webhooks and redirects to it" do
assert_difference "Stack.count" do
post :create, params: {
Expand Down
Loading