Move fatality notices from government frontend #4640

KludgeKML · 2025-02-12T14:07:58Z

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

What

We want to handle the rendering of fatality_notice documents from government-frontend to frontend.

Why

https://trello.com/c/2QCsgFab/492-move-fatality-notices-from-government-frontend-to-frontend

How

By moving the routes and necessary code to get the pages rendering in Frontend.

Page Examples

https://[HEROKU-APP-ID].herokuapp.com/government/fatalities/squadron-leader-patrick-marshall
https://[HEROKU-APP-ID].herokuapp.com/government/fatalities/lieutenant-philip-d-green-rn

Screenshots?

- Ran rake "consolidation:copy_translation[fatality_notice,formats.fatality_notice]" - Note that none_added isn't actually used by fatality_notice, it's used by the related field_of_operation pages, but not clear if it should be moved, since it fits with fatality notices. Audit trail: https://github.com/alphagov/government-frontend/tree/72aeb637c575c3f646096d8183a02356b9e0a3ef/config/locales

- Ran rake "consolidation:copy_translation[content_item.schema_name.fatality_notice,formats.fatality_notice]" - needed for withdrawal notices. Audit trail: https://github.com/alphagov/government-frontend/tree/8799336fd3c96dac7c7e53cce3cd8a522beb17da/config/locales

- as with other models, we don't want to include the messy Linkable concern, so inline the links_group methods (we can also simplify them because we know world_locations won't be included). Remove other presenter methods, retain field_of_operation which is clearly a model method. Audit trail: https://github.com/alphagov/government-frontend/blob/8799336fd3c96dac7c7e53cce3cd8a522beb17da/app/presenters/fatality_notice_presenter.rb https://github.com/alphagov/government-frontend/blob/8799336fd3c96dac7c7e53cce3cd8a522beb17da/app/presenters/content_item/linkable.rb

- This replaces the page_title method from Withdrawable, which feels presentational Audit trail: https://github.com/alphagov/government-frontend/blob/8799336fd3c96dac7c7e53cce3cd8a522beb17da/app/presenters/content_item/withdrawable.rb#L7-L9

- image asset included needed by view. - The only truly model method needed is the field of operations, once that's included from the presenter the rest of the methods could be done away with - the image was hardcoded, so could just be moved into the view, page_title has been replaced with the withdrawn_title method in the title_helper, title and context could be handled in the page with the heading component, and the important metadata was only ever the field of operation, so we just inline that. Audit Trail: - https://github.com/alphagov/government-frontend/blob/72aeb637c575c3f646096d8183a02356b9e0a3ef/app/assets/images/ministry-of-defence-crest.png - https://github.com/alphagov/government-frontend/blob/72aeb637c575c3f646096d8183a02356b9e0a3ef/app/presenters/fatality_notice_presenter.rb - https://github.com/alphagov/government-frontend/blob/72aeb637c575c3f646096d8183a02356b9e0a3ef/app/views/content_items/fatality_notice.html.erb

Audit trail: https://github.com/alphagov/government-frontend/blob/8799336fd3c96dac7c7e53cce3cd8a522beb17da/test/integration/fatality_notice_test.rb

deborahchua · 2025-02-14T11:35:10Z

app/helpers/title_helper.rb

@@ -0,0 +1,5 @@
+module TitleHelper
+  def withdrawable_title(content_item)


Does this mean we no longer need this method in the content item presenter?

+1 I think this is duplicating the presenter method, and it'd be better for a presenter object to be created.

I'd question this being a helper method rather than a presenter method anyway as it needs to know about the "state" and structure of the content item.

Agreed we don't need to duplicate it. I will note that I don't think "page_title" is a great method name for something that has one job (decorating a withdrawn title). I'm going to drop this commit and use the existing method, but perhaps we could revisit the name.

I don't mind the name as I think it describes what it is. What I would like to do is to move the "- GOV.UK" part into the presenter method so that we can consolidate it with the Application Helper method and then make the use of page_title consistent as there are a few places where "- GOV.UK" is being tacked onto the content item title.

leenagupte

This is a good start, but I feel that a few things have been missed. I think this has been rushed a bit, and that it could have done with sometime looking around at the other consolidated routes to see what patterns etc already exist.

The audit trail in the commits is great though and the system tests look pretty thorough.

Other comments and recommendations have been added inline.

leenagupte · 2025-02-14T11:27:17Z

app/models/fatality_notice.rb

+  end
+
+  def contributors
+    organisations_ordered_by_emphasis + links_group(%w[worldwide_organisations people speaker])


If worldwide_organisations are needed then the WorldwideOrganisations concern will need to be included.

Do fatality notices have a "speaker"?

I wonder how many content items need to access "people" in contributors. Perhaps people should be it's own model?

Do we need to start thinking about modelling Links? Not a linkable concern, but something that does the job of getting the title and base_path so we don't have to keep repeating that code?

leenagupte · 2025-02-14T11:28:33Z

app/models/fatality_notice.rb

+    content_item_links = content_store_hash["links"]["field_of_operation"]
+    if content_item_links
+      attributes = content_item_links.first
+      OpenStruct.new(title: attributes["title"], path: attributes["base_path"])


This is nice! I think we should use this pattern in other places too.

leenagupte · 2025-02-14T11:30:23Z

spec/models/fatality_notice_spec.rb

+
+  describe "#field_of_operation" do
+    it "returns the field of operation link structure" do
+      model_instance = described_class.new(content_item)


Nitpick. I'd probably have called this content_item rather than model_instance as it's being set to an instance of ContentItem.

leenagupte · 2025-02-14T11:41:51Z

app/helpers/title_helper.rb

@@ -0,0 +1,5 @@
+module TitleHelper
+  def withdrawable_title(content_item)


+1 I think this is duplicating the presenter method, and it'd be better for a presenter object to be created.

I'd question this being a helper method rather than a presenter method anyway as it needs to know about the "state" and structure of the content item.

leenagupte · 2025-02-14T11:49:18Z

app/views/fatality_notice/show.html.erb

+            <%= render "govuk_publishing_components/components/metadata", {
+              inverse: true,
+              other: {
+                t("formats.fatality_notice.field_of_operation") => link_to(content_item.field_of_operation.title, content_item.field_of_operation.path, class: "govuk-link govuk-link--inverse"),


I think you could call govuk_styled_link(content_item.field_of_operation.title, path content_item.field_of_operation.title, inverse: true) rather than link_to

leenagupte · 2025-02-14T11:50:39Z

app/views/fatality_notice/show.html.erb

+        <% end %>
+      </div>
+      <%= render "components/published_dates", {
+        published: display_date(content_item.first_public_at || content_item.first_published_at),


You could use content_item.initial_publication_date here rather than the OR.

leenagupte · 2025-02-14T11:53:53Z

app/views/fatality_notice/show.html.erb

+      </div>
+      <%= render "components/published_dates", {
+        published: display_date(content_item.first_public_at || content_item.first_published_at),
+        last_updated: display_date(content_item.public_updated_at),


I think this should be content_item.updated as that checks if there have actually been any updates. We can't trust the presence of public_updated_at as that always exists.

leenagupte · 2025-02-14T11:57:07Z

spec/requests/fatality_notice_spec.rb

+  end
+
+  describe "GET show" do
+    it "returns 200" do


I've asked this in the speech PR too, but it'd be nice if this also checked the template being rendered.

leenagupte · 2025-02-14T11:59:17Z

spec/models/fatality_notice_spec.rb

@@ -0,0 +1,43 @@
+RSpec.describe FatalityNotice do


This tests should also include the shared examples "it has updates" and "it has no updates"

govuk-ci temporarily deployed to govuk-frontend-app-pr-4640 February 12, 2025 14:08 Inactive

govuk-ci temporarily deployed to govuk-frontend-app-pr-4640 February 12, 2025 14:10 Inactive

KludgeKML force-pushed the move-fatality-notices-from-government-frontend branch from 7d78f9a to 1391d9a Compare February 12, 2025 14:12

govuk-ci temporarily deployed to govuk-frontend-app-pr-4640 February 12, 2025 14:13 Inactive

KludgeKML marked this pull request as draft February 12, 2025 14:14

govuk-ci temporarily deployed to govuk-frontend-app-pr-4640 February 12, 2025 17:04 Inactive

KludgeKML force-pushed the move-fatality-notices-from-government-frontend branch from 4c38d5f to 98c043b Compare February 13, 2025 10:18

govuk-ci temporarily deployed to govuk-frontend-app-pr-4640 February 13, 2025 10:18 Inactive

govuk-ci temporarily deployed to govuk-frontend-app-pr-4640 February 13, 2025 10:35 Inactive

govuk-ci temporarily deployed to govuk-frontend-app-pr-4640 February 13, 2025 11:30 Inactive

KludgeKML force-pushed the move-fatality-notices-from-government-frontend branch from bdb431b to cb37db2 Compare February 13, 2025 11:40

govuk-ci temporarily deployed to govuk-frontend-app-pr-4640 February 13, 2025 11:41 Inactive

KludgeKML force-pushed the move-fatality-notices-from-government-frontend branch from cb37db2 to c9b6726 Compare February 13, 2025 11:41

govuk-ci temporarily deployed to govuk-frontend-app-pr-4640 February 13, 2025 11:41 Inactive

KludgeKML force-pushed the move-fatality-notices-from-government-frontend branch from c9b6726 to 18a9576 Compare February 13, 2025 11:51

govuk-ci temporarily deployed to govuk-frontend-app-pr-4640 February 13, 2025 11:51 Inactive

Update README and examples config

e50adca

KludgeKML force-pushed the move-fatality-notices-from-government-frontend branch from 18a9576 to bf89a36 Compare February 13, 2025 11:55

govuk-ci temporarily deployed to govuk-frontend-app-pr-4640 February 13, 2025 11:56 Inactive

KludgeKML force-pushed the move-fatality-notices-from-government-frontend branch from bf89a36 to 909f4b1 Compare February 13, 2025 11:58

govuk-ci temporarily deployed to govuk-frontend-app-pr-4640 February 13, 2025 11:58 Inactive

KludgeKML added 4 commits February 13, 2025 12:00

Add withdrawable_title helper method

ba5c664

- This replaces the page_title method from Withdrawable, which feels presentational Audit trail: https://github.com/alphagov/government-frontend/blob/8799336fd3c96dac7c7e53cce3cd8a522beb17da/app/presenters/content_item/withdrawable.rb#L7-L9

KludgeKML force-pushed the move-fatality-notices-from-government-frontend branch from 909f4b1 to c809a60 Compare February 13, 2025 12:07

govuk-ci temporarily deployed to govuk-frontend-app-pr-4640 February 13, 2025 12:07 Inactive

govuk-ci temporarily deployed to govuk-frontend-app-pr-4640 February 13, 2025 12:12 Inactive

KludgeKML added 2 commits February 13, 2025 12:12

Add inverse-background style helper

98eeac6

Add request and system tests for fatality_notice

db94142

Audit trail: https://github.com/alphagov/government-frontend/blob/8799336fd3c96dac7c7e53cce3cd8a522beb17da/test/integration/fatality_notice_test.rb

KludgeKML force-pushed the move-fatality-notices-from-government-frontend branch from e35fd92 to db94142 Compare February 13, 2025 12:12

govuk-ci deployed to govuk-frontend-app-pr-4640 February 13, 2025 12:12 View deployment

KludgeKML marked this pull request as ready for review February 13, 2025 14:37

KludgeKML requested review from leenagupte, deborahchua and sairamya93 February 13, 2025 14:38

deborahchua reviewed Feb 14, 2025

View reviewed changes

leenagupte reviewed Feb 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move fatality notices from government frontend #4640

Move fatality notices from government frontend #4640

KludgeKML commented Feb 12, 2025 •

edited

Loading

deborahchua Feb 14, 2025

leenagupte Feb 14, 2025

KludgeKML Feb 17, 2025

leenagupte Feb 17, 2025

leenagupte left a comment

leenagupte Feb 14, 2025

leenagupte Feb 14, 2025

leenagupte Feb 14, 2025

leenagupte Feb 14, 2025

leenagupte Feb 14, 2025

leenagupte Feb 14, 2025

leenagupte Feb 14, 2025

leenagupte Feb 14, 2025

leenagupte Feb 14, 2025

leenagupte Feb 14, 2025

leenagupte Feb 14, 2025

		@@ -0,0 +1,5 @@
		module TitleHelper
		def withdrawable_title(content_item)

Move fatality notices from government frontend #4640

Are you sure you want to change the base?

Move fatality notices from government frontend #4640

Conversation

KludgeKML commented Feb 12, 2025 • edited Loading

What

Why

How

Page Examples

Screenshots?

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

Choose a reason for hiding this comment

KludgeKML commented Feb 12, 2025 •

edited

Loading