Add Emergency Banner handling #22

KludgeKML · 2024-11-28T15:44:19Z

Adds the ability for apps with the gem included to locally handle emergency banners. This will be necessary for any apps moving away from Slimmer/Static. They will need to be given access to the Redis cluster that's currently shared by Static and Whitehall, details in commits.

Note that this is slightly more involved than setting up recruitment banner code (you also have to specify a redis cluster in a Rails.application.config key), but only slightly, and that lets us move away from the slightly awkward class variables that Static uses for caching the client connection.

When this is merged it won't be included directly in the existing apps until they're switched over to no-slimmer versions, so we won't need to add anything to those apps immediately.

https://trello.com/c/1REVdLAr/247-add-emergency-banner-handling-to-govukwebbanners

leenagupte

It's looking good. I have a few questions and comments inline.

The homepage variant is something that needs to be addressed before this can be merged.

leenagupte · 2024-12-10T09:52:25Z

README.md

+```
+<%= render "govuk_publishing_components/components/layout_for_public", {
+  draft_watermark: draft_environment,
+  emergency_banner: render("govuk_web_banners/emergency_banner"), # <-- Add this line


Ok, so layout_for_public just renders whatever is passed as the emergency banner?

It feels like we want to get to a place where apps only need to layout_for_public, and it does the rendering of the banner without needing anything extra passed to it.

Could you add a comment about this to the commit message?

Notes about this added to the commit

leenagupte · 2024-12-10T09:56:52Z

app/views/govuk_web_banners/_emergency_banner.html.erb

+      link: emergency_banner.link,
+      link_text: emergency_banner.link_text,
+      short_description: emergency_banner.short_description,
+      homepage: false,


The Homepage case will need to be handled it can't always be hard-coded to false, though it could have a default of false. This is something that should be added to the EmergencyBanner class, or passed in some other way.

Otherwise we're saying that the banner will never render the larger homepage version.

Oh yeah, I had a mental todo there but no actual todo. Done now.

leenagupte · 2024-12-10T09:58:18Z

app/views/govuk_web_banners/_emergency_banner.html.erb

@@ -0,0 +1,11 @@
+<% emergency_banner = GovukWebBanners::EmergencyBanner.new(redis_client: Rails.application.config.emergency_banner_redis_client) %>


Does the redis client need to be passed in? Could the EmergencyBanner class not pick it up from the config itself?

Hmm. I'm in two minds about this, because it doesn't make any difference to the end user, and the utility is debatable for the dev (to me, it makes the partial code 1% nicer, the model code 1% nicer, and the unit tests 25% less readable). But I'm not necessarily the person who's going to do the next change to this code, so if you prefer it I'm okay with adding it. For the moment I've added it in a standalone commit (the last one) so that you can see the changes involved. If we accept it, I'll squash it into the previous commit.

I'll have a look at the commit, but in general I'd hide away something so very backend from the frontend bits.

leenagupte · 2024-12-10T10:00:08Z

lib/govuk_web_banners/emergency_banner.rb

+    attr_reader :campaign_class, :heading, :short_description, :link, :link_text
+
+    def initialize(redis_client:)
+      content = content_from_redis(redis_client)


As commented in the view, I think that as there's only one place that the EmergencyBanner class is getting the banner information, I don't think the Redis client needs to be passed in, could just be a method in this class.

spec/units/emergency_banner_spec.rb

leenagupte · 2024-12-10T12:26:28Z

spec/units/emergency_banner_spec.rb

+  subject(:emergency_banner) { GovukWebBanners::EmergencyBanner.new }
+
+  before do
+    Rails.application.config.emergency_banner_redis_client = double(hgetall: {})


I think another way to do this would be to make the redis_client method in the class public and stub that. allow .... to receive etc.

I'm sure we can play around with nicer ways to write this.

But I do prefer this commit.

I think probably (previous comments notwithstanding) we shouldn't tinker with the model too much just to make the tests nicer. If you think this is understandable, I'm okay with it.

I think we can play around with the stubbing and expectations to make those a bit nicer to read.

- Ultimately, we probably just want the layout component to handle the emergency banner itself, because we're just pushing in the results of the render, and we have to let the emergency banner partial know whether we're on the homepage or not (which we also have to tell the layout for public component). We don't want to do that just now, though, because it potentially introduces a circular dependency, and because we don't want to have to deal with disabling it in apps that still use slimmer.

- So we don't have to spec out redis for the existing tests, we rename the dummy app routes to something relevant to the banners they're testing.

- needed by the emergency banner system

- The banner is intended to be called as a parameter for the layout_for_public component, so we call it that way in the dummy app. This means we need to include a bunch of dependencies for that component (it wants an application js/css file, and updates to the manifest) - We also add the initializer that sets the redis client to be used. - Add a / (equivalent to the homepage) to check the homepage local variable causes the homepage variant to be rendered.

- We remove the odd handling of link_text (where it's blanked if the link is blank), because the component can handle this already. - In `static`, the library class handling the banner used a class variable to memoize the redis client. Because that's a bit of a code smell (and makes testing awkward), we replace that with a required config value in Rails.application.config (as mentioned in the documentation). - We use the rails cache to cache the call to redis for one minute, which seems like a reasonable compromise between traffic to the redis cluster and the banner being available (one minute means that we're more responsive than the 5 minute cache, giving us a bit of flexibility in cache busting to test when the banner comes up). Audit Trail: - https://github.com/alphagov/static/blob/bb53325994ebb23476f577f8dbb8bc78d04ceac7/lib/emergency_banner/display.rb - https://github.com/alphagov/static/blob/bb53325994ebb23476f577f8dbb8bc78d04ceac7/app/views/root/_gem_base.html.erb#L21-L44

leenagupte

Looks good! 👍

leenagupte · 2024-12-13T10:57:34Z

app/views/govuk_web_banners/_emergency_banner.html.erb

+      link: emergency_banner.link,
+      link_text: emergency_banner.link_text,
+      short_description: emergency_banner.short_description,
+      homepage: local_assigns[:homepage] || false,


leenagupte · 2024-12-13T11:01:34Z

spec/requests/emergency_banner_spec.rb

@@ -0,0 +1,38 @@
+RSpec.describe "Emergency Banners" do


These tests are great!

KludgeKML force-pushed the emergency-banner branch 4 times, most recently from 8d2a183 to ce4807f Compare December 4, 2024 13:57

KludgeKML marked this pull request as ready for review December 4, 2024 13:59

KludgeKML force-pushed the emergency-banner branch from ce4807f to a48c950 Compare December 10, 2024 09:23

KludgeKML requested a review from leenagupte December 10, 2024 09:24

leenagupte reviewed Dec 10, 2024

View reviewed changes

KludgeKML force-pushed the emergency-banner branch 3 times, most recently from 9c772c6 to fa4bbf8 Compare December 10, 2024 10:44

leenagupte reviewed Dec 10, 2024

View reviewed changes

KludgeKML force-pushed the emergency-banner branch from fa4bbf8 to 9350b01 Compare December 10, 2024 16:53

KludgeKML added 4 commits December 12, 2024 14:52

Prepare existing tests for emergency banners

15ae21e

- So we don't have to spec out redis for the existing tests, we rename the dummy app routes to something relevant to the banners they're testing.

Add redis and govuk_app_config gems

4dbf754

- needed by the emergency banner system

KludgeKML force-pushed the emergency-banner branch 2 times, most recently from 89c31bb to 95830f8 Compare December 12, 2024 15:10

KludgeKML force-pushed the emergency-banner branch from 95830f8 to f7e971e Compare December 12, 2024 15:21

leenagupte approved these changes Dec 13, 2024

View reviewed changes

KludgeKML merged commit 82d8118 into main Dec 16, 2024
10 checks passed

hannako mentioned this pull request Dec 20, 2024

Release 0.3.0 #38

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Emergency Banner handling #22

Add Emergency Banner handling #22

KludgeKML commented Nov 28, 2024 •

edited

Loading

leenagupte left a comment

leenagupte Dec 10, 2024

KludgeKML Dec 10, 2024

leenagupte Dec 10, 2024

KludgeKML Dec 10, 2024

leenagupte Dec 10, 2024

KludgeKML Dec 10, 2024

leenagupte Dec 10, 2024

leenagupte Dec 10, 2024

leenagupte Dec 10, 2024

KludgeKML Dec 10, 2024

leenagupte Dec 10, 2024 •

edited

Loading

leenagupte left a comment

leenagupte Dec 13, 2024

leenagupte Dec 13, 2024

		@@ -0,0 +1,11 @@
		<% emergency_banner = GovukWebBanners::EmergencyBanner.new(redis_client: Rails.application.config.emergency_banner_redis_client) %>

Add Emergency Banner handling #22

Add Emergency Banner handling #22

Conversation

KludgeKML commented Nov 28, 2024 • edited Loading

leenagupte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leenagupte Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

leenagupte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KludgeKML commented Nov 28, 2024 •

edited

Loading

leenagupte Dec 10, 2024 •

edited

Loading