From d9d9f25943e9c720304091dd4bc23f40b396da06 Mon Sep 17 00:00:00 2001 From: Ynda Jas Date: Thu, 18 Apr 2024 12:57:45 +0100 Subject: [PATCH 1/6] Rename republishable_documents We will soon be able to republish any document by its slug via this section of the UI. We've decided to call the tasks for "Past Prime Ministers", "How government works" etc page-specific republishing tasks The upcoming generic documents, organisations, people, and roles by slug tasks will need specific controller actions, so we needed a way of referring to these other page-specific tasks. Related code is updated here to reflect this language --- app/controllers/admin/republishing_controller.rb | 14 +++++++------- .../{confirm.html.erb => confirm_page.html.erb} | 4 ++-- app/views/admin/republishing/index.html.erb | 10 +++++----- config/routes.rb | 2 +- .../admin/republishing_controller_test.rb | 14 +++++++------- 5 files changed, 22 insertions(+), 22 deletions(-) rename app/views/admin/republishing/{confirm.html.erb => confirm_page.html.erb} (77%) diff --git a/app/controllers/admin/republishing_controller.rb b/app/controllers/admin/republishing_controller.rb index 1ef312ad9c4..faa696ff47d 100644 --- a/app/controllers/admin/republishing_controller.rb +++ b/app/controllers/admin/republishing_controller.rb @@ -2,7 +2,7 @@ class Admin::RepublishingController < Admin::BaseController before_action :enforce_permissions! def index - @republishable_documents = republishable_documents + @republishable_pages = republishable_pages end def republish_past_prime_ministers_index @@ -11,13 +11,13 @@ def republish_past_prime_ministers_index redirect_to(admin_republishing_index_path) end - def confirm - republishable_document = republishable_documents.find { |document| document[:slug] == params[:document_slug] } + def confirm_page + page_to_republish = republishable_pages.find { |page| page[:slug] == params[:page_slug] } - return render "admin/errors/not_found", status: :not_found unless republishable_document + return render "admin/errors/not_found", status: :not_found unless page_to_republish - @document_title = republishable_document[:title] - @republishing_path = republishable_document[:republishing_path] + @title = page_to_republish[:title] + @republishing_path = page_to_republish[:republishing_path] end private @@ -26,7 +26,7 @@ def enforce_permissions! enforce_permission!(:administer, :republish_documents) end - def republishable_documents + def republishable_pages historical_accounts_index_presenter = PublishingApi::HistoricalAccountsIndexPresenter.new [{ diff --git a/app/views/admin/republishing/confirm.html.erb b/app/views/admin/republishing/confirm_page.html.erb similarity index 77% rename from app/views/admin/republishing/confirm.html.erb rename to app/views/admin/republishing/confirm_page.html.erb index e66ee20b703..91f6e905918 100644 --- a/app/views/admin/republishing/confirm.html.erb +++ b/app/views/admin/republishing/confirm_page.html.erb @@ -1,5 +1,5 @@ -<% content_for :page_title, "Republish '#{@document_title}'" %> -<% content_for :title, "Are you sure you want to republish '#{@document_title}'?" %> +<% content_for :page_title, "Republish '#{@title}'" %> +<% content_for :title, "Are you sure you want to republish '#{@title}'?" %> <% content_for :title_margin_bottom, 6 %>
diff --git a/app/views/admin/republishing/index.html.erb b/app/views/admin/republishing/index.html.erb index 765e87ab64b..298f9e71d25 100644 --- a/app/views/admin/republishing/index.html.erb +++ b/app/views/admin/republishing/index.html.erb @@ -21,20 +21,20 @@ <%= render "govuk_publishing_components/components/table", { head: [ { - text: "Document", + text: "Page", }, { text: "Action", }, ], - rows: @republishable_documents.map do |document| + rows: @republishable_pages.map do |page| [ { - text: link_to(document[:title], Plek.website_root + document[:public_path], class:"govuk-link"), + text: link_to(page[:title], Plek.website_root + page[:public_path], class:"govuk-link"), }, { - text: link_to(sanitize("Republish #{tag.span('\'' + document[:title] + '\' page', class: 'govuk-visually-hidden')}"), - admin_confirm_republishing_path(document[:slug]), + text: link_to(sanitize("Republish #{tag.span('\'' + page[:title] + '\' page', class: 'govuk-visually-hidden')}"), + admin_confirm_page_republishing_path(page[:slug]), id: "republish-past-prime-ministers-index", class: "govuk-link", ), diff --git a/config/routes.rb b/config/routes.rb index bde763abc4b..816c9c03c76 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -23,7 +23,7 @@ def redirect(path, options = { prefix: Whitehall.router_prefix }) resources :users, only: %i[index show edit update] get "republishing" => "republishing#index", as: :republishing_index - get "republishing/:document_slug/confirm" => "republishing#confirm", as: :confirm_republishing + get "republishing/:page_slug/confirm" => "republishing#confirm_page", as: :confirm_page_republishing post "republishing/republish-past-prime-ministers" => "republishing#republish_past_prime_ministers_index" resources :documents, only: [] do diff --git a/test/functional/admin/republishing_controller_test.rb b/test/functional/admin/republishing_controller_test.rb index e305fb56dd5..fab934fa53c 100644 --- a/test/functional/admin/republishing_controller_test.rb +++ b/test/functional/admin/republishing_controller_test.rb @@ -8,7 +8,7 @@ class Admin::RepublishingControllerTest < ActionController::TestCase should_be_an_admin_controller - view_test "GDS Admin users should be able to acess the GET :index and see links to republishable documents" do + view_test "GDS Admin users should be able to acess the GET :index and see links to republishable pages" do get :index assert_select ".govuk-table__cell:nth-child(1) a[href='https://www.test.gov.uk/government/history/past-prime-ministers']", text: "Past Prime Ministers" @@ -23,20 +23,20 @@ class Admin::RepublishingControllerTest < ActionController::TestCase assert_response :forbidden end - test "GDS Admin users should be able to access GET :confirm with a republishable document slug" do - get :confirm, params: { document_slug: "past-prime-ministers" } + test "GDS Admin users should be able to access GET :confirm_page with a republishable page slug" do + get :confirm_page, params: { page_slug: "past-prime-ministers" } assert_response :ok end - test "GDS Admin users should see a 404 page when trying to republish a document with an unregistered document slug" do - get :confirm, params: { document_slug: "not-republishable" } + test "GDS Admin users should see a 404 page when trying to GET :confirm_page with an unregistered page slug" do + get :confirm_page, params: { page_slug: "not-republishable" } assert_response :not_found end - test "Non-GDS Admin users should not be able to access GET :confirm" do + test "Non-GDS Admin users should not be able to access GET :confirm_page" do login_as :writer - get :confirm, params: { document_slug: "past-prime-ministers" } + get :confirm_page, params: { page_slug: "past-prime-ministers" } assert_response :forbidden end From 24916128091540cd899a0e871efb01628818bde7 Mon Sep 17 00:00:00 2001 From: Ynda Jas Date: Thu, 18 Apr 2024 13:00:16 +0100 Subject: [PATCH 2/6] Fix republishing link ID We shifted to using a `.map` here but left in a page-specific ID: this makes it generic/specific to whichever item is within a given `.map` iteration --- app/views/admin/republishing/index.html.erb | 2 +- .../republishing_published_documents_steps.rb.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/admin/republishing/index.html.erb b/app/views/admin/republishing/index.html.erb index 298f9e71d25..4e81832b8ae 100644 --- a/app/views/admin/republishing/index.html.erb +++ b/app/views/admin/republishing/index.html.erb @@ -35,7 +35,7 @@ { text: link_to(sanitize("Republish #{tag.span('\'' + page[:title] + '\' page', class: 'govuk-visually-hidden')}"), admin_confirm_page_republishing_path(page[:slug]), - id: "republish-past-prime-ministers-index", + id: "republish-" + page[:slug], class: "govuk-link", ), }, diff --git a/features/step_definitions/republishing_published_documents_steps.rb.rb b/features/step_definitions/republishing_published_documents_steps.rb.rb index 7f6f4c9fe78..9df82a6ab55 100644 --- a/features/step_definitions/republishing_published_documents_steps.rb.rb +++ b/features/step_definitions/republishing_published_documents_steps.rb.rb @@ -4,7 +4,7 @@ When(/^I request a republish of the "Past prime ministers" page$/) do visit admin_republishing_index_path - find("#republish-past-prime-ministers-index").click + find("#republish-past-prime-ministers").click click_button("Confirm republishing") end From 0837fc51602fc0ec724a20f8cce4cf72d8524479 Mon Sep 17 00:00:00 2001 From: Ynda Jas Date: Wed, 17 Apr 2024 17:55:40 +0100 Subject: [PATCH 3/6] Use more generic republishing language We'll soon be able to republish organisations, people, and roles, so this makes the language more generic ('document' -> 'content') to cover all bases. 'document type' was already a little inaccurate, since we're initially focusing on individual content, not content by type --- app/controllers/admin/republishing_controller.rb | 2 +- app/views/admin/republishing/index.html.erb | 6 +++--- lib/whitehall/authority/rules/miscellaneous_rules.rb | 2 +- test/functional/admin/republishing_controller_test.rb | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/controllers/admin/republishing_controller.rb b/app/controllers/admin/republishing_controller.rb index faa696ff47d..1d77a4a71db 100644 --- a/app/controllers/admin/republishing_controller.rb +++ b/app/controllers/admin/republishing_controller.rb @@ -23,7 +23,7 @@ def confirm_page private def enforce_permissions! - enforce_permission!(:administer, :republish_documents) + enforce_permission!(:administer, :republish_content) end def republishable_pages diff --git a/app/views/admin/republishing/index.html.erb b/app/views/admin/republishing/index.html.erb index 4e81832b8ae..64064abb35b 100644 --- a/app/views/admin/republishing/index.html.erb +++ b/app/views/admin/republishing/index.html.erb @@ -1,5 +1,5 @@ -<% content_for :page_title, "Republish documents" %> -<% content_for :title, "Republish documents" %> +<% content_for :page_title, "Republish content" %> +<% content_for :title, "Republish content" %> <% content_for :title_margin_bottom, 6 %>
@@ -15,7 +15,7 @@

The following actions will allow you to schedule the republishing of content that was originally published in this application. Any linked editions will also be republished through dependency resolution. - Try to pick the document type most focused to the scope of what you need to republish to avoid unnecessary server load. + Try to pick the republishing task most focused to the scope of what you need to republish to avoid unnecessary server load.

<%= render "govuk_publishing_components/components/table", { diff --git a/lib/whitehall/authority/rules/miscellaneous_rules.rb b/lib/whitehall/authority/rules/miscellaneous_rules.rb index cf114b330da..bf97b4eeb48 100644 --- a/lib/whitehall/authority/rules/miscellaneous_rules.rb +++ b/lib/whitehall/authority/rules/miscellaneous_rules.rb @@ -8,7 +8,7 @@ def can?(action) end end - def can_for_republish_documents?(_action) + def can_for_republish_content?(_action) actor.gds_admin? end diff --git a/test/functional/admin/republishing_controller_test.rb b/test/functional/admin/republishing_controller_test.rb index fab934fa53c..a306accce34 100644 --- a/test/functional/admin/republishing_controller_test.rb +++ b/test/functional/admin/republishing_controller_test.rb @@ -8,7 +8,7 @@ class Admin::RepublishingControllerTest < ActionController::TestCase should_be_an_admin_controller - view_test "GDS Admin users should be able to acess the GET :index and see links to republishable pages" do + view_test "GDS Admin users should be able to acess the GET :index and see links to republishable content" do get :index assert_select ".govuk-table__cell:nth-child(1) a[href='https://www.test.gov.uk/government/history/past-prime-ministers']", text: "Past Prime Ministers" From c286361deb913bafbc4eff4fc618d2b7a17357ad Mon Sep 17 00:00:00 2001 From: Ynda Jas Date: Wed, 17 Apr 2024 17:15:36 +0100 Subject: [PATCH 4/6] DRY republishing action This prepares the republishing controller to handle requests for other commonly republished documents by moving towards a generic action --- app/controllers/admin/republishing_controller.rb | 14 +++++++++----- config/routes.rb | 2 +- .../admin/republishing_controller_test.rb | 15 +++++++++++---- 3 files changed, 21 insertions(+), 10 deletions(-) diff --git a/app/controllers/admin/republishing_controller.rb b/app/controllers/admin/republishing_controller.rb index 1d77a4a71db..087cdfc7ab7 100644 --- a/app/controllers/admin/republishing_controller.rb +++ b/app/controllers/admin/republishing_controller.rb @@ -5,9 +5,13 @@ def index @republishable_pages = republishable_pages end - def republish_past_prime_ministers_index - PresentPageToPublishingApiWorker.perform_async("PublishingApi::HistoricalAccountsIndexPresenter") - flash[:notice] = "'Past Prime Ministers' page has been scheduled for republishing" + def republish_page + page_to_republish = republishable_pages.find { |page| page[:slug] == params[:page_slug] } + + return render "admin/errors/not_found", status: :not_found unless page_to_republish + + PresentPageToPublishingApiWorker.perform_async(page_to_republish[:presenter]) + flash[:notice] = "'#{page_to_republish[:title]}' page has been scheduled for republishing" redirect_to(admin_republishing_index_path) end @@ -17,7 +21,7 @@ def confirm_page return render "admin/errors/not_found", status: :not_found unless page_to_republish @title = page_to_republish[:title] - @republishing_path = page_to_republish[:republishing_path] + @republishing_path = admin_republish_page_path(page_to_republish[:slug]) end private @@ -32,8 +36,8 @@ def republishable_pages [{ title: historical_accounts_index_presenter.content[:title], public_path: historical_accounts_index_presenter.base_path, - republishing_path: admin_republishing_republish_past_prime_ministers_path, slug: historical_accounts_index_presenter.base_path.split("/").last, + presenter: "PublishingApi::HistoricalAccountsIndexPresenter", }] end end diff --git a/config/routes.rb b/config/routes.rb index 816c9c03c76..7a769765f13 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -24,7 +24,7 @@ def redirect(path, options = { prefix: Whitehall.router_prefix }) get "republishing" => "republishing#index", as: :republishing_index get "republishing/:page_slug/confirm" => "republishing#confirm_page", as: :confirm_page_republishing - post "republishing/republish-past-prime-ministers" => "republishing#republish_past_prime_ministers_index" + post "republishing/:page_slug/republish" => "republishing#republish_page", as: :republish_page resources :documents, only: [] do resources :review_reminders, only: %i[new create edit update] diff --git a/test/functional/admin/republishing_controller_test.rb b/test/functional/admin/republishing_controller_test.rb index a306accce34..646de37e83d 100644 --- a/test/functional/admin/republishing_controller_test.rb +++ b/test/functional/admin/republishing_controller_test.rb @@ -40,21 +40,28 @@ class Admin::RepublishingControllerTest < ActionController::TestCase assert_response :forbidden end - test "GDS Admin users should be able to trigger the PresentPageToPublishingWorker job with the HistoricalAccountsIndexPresenter" do + test "GDS Admin users should be able to access POST :republish_page with a republishable page slug" do PresentPageToPublishingApiWorker.expects(:perform_async).with("PublishingApi::HistoricalAccountsIndexPresenter").once - post :republish_past_prime_ministers_index + post :republish_page, params: { page_slug: "past-prime-ministers" } assert_redirected_to admin_republishing_index_path assert_equal "'Past Prime Ministers' page has been scheduled for republishing", flash[:notice] end - test "Non-GDS Admin users should not be able to republish the page" do + test "GDS Admin users should see a 404 page when trying to POST :republish_page with an unregistered page slug" do + PresentPageToPublishingApiWorker.expects(:perform_async).with("PublishingApi::HistoricalAccountsIndexPresenter").never + + get :republish_page, params: { page_slug: "not-republishable" } + assert_response :not_found + end + + test "Non-GDS Admin users should not be able to access POST :republish_page" do PresentPageToPublishingApiWorker.expects(:perform_async).with("PublishingApi::HistoricalAccountsIndexPresenter").never login_as :writer - post :republish_past_prime_ministers_index + post :republish_page, params: { page_slug: "past-prime-ministers" } assert_response :forbidden end end From 8d329f9d4be7b5888573af40fd9f801f8a58ecb2 Mon Sep 17 00:00:00 2001 From: Ynda Jas Date: Wed, 17 Apr 2024 19:08:13 +0100 Subject: [PATCH 5/6] Group republishing routes This keeps these related routes tidy through nesting, and provides a more reliable visual grouping than line breaks (which are more likely to change) The names of the routes have been changed slightly to make them more similar to the way Rails generates them An additional path segment has been added to the page-related paths. This isn't strictly necessary, as the relevant routes could simply be placed last to avoid any clashes with other routes with named segments in this position, however it may help to group republishing tasks by content type --- app/controllers/admin/republishing_controller.rb | 2 +- app/views/admin/republishing/index.html.erb | 2 +- config/routes.rb | 10 +++++++--- test/functional/admin/republishing_controller_test.rb | 2 +- 4 files changed, 10 insertions(+), 6 deletions(-) diff --git a/app/controllers/admin/republishing_controller.rb b/app/controllers/admin/republishing_controller.rb index 087cdfc7ab7..ef6fa37290a 100644 --- a/app/controllers/admin/republishing_controller.rb +++ b/app/controllers/admin/republishing_controller.rb @@ -21,7 +21,7 @@ def confirm_page return render "admin/errors/not_found", status: :not_found unless page_to_republish @title = page_to_republish[:title] - @republishing_path = admin_republish_page_path(page_to_republish[:slug]) + @republishing_path = admin_republishing_page_republish_path(page_to_republish[:slug]) end private diff --git a/app/views/admin/republishing/index.html.erb b/app/views/admin/republishing/index.html.erb index 64064abb35b..de816ed0065 100644 --- a/app/views/admin/republishing/index.html.erb +++ b/app/views/admin/republishing/index.html.erb @@ -34,7 +34,7 @@ }, { text: link_to(sanitize("Republish #{tag.span('\'' + page[:title] + '\' page', class: 'govuk-visually-hidden')}"), - admin_confirm_page_republishing_path(page[:slug]), + admin_republishing_page_confirm_path(page[:slug]), id: "republish-" + page[:slug], class: "govuk-link", ), diff --git a/config/routes.rb b/config/routes.rb index 7a769765f13..1d7dc1a699c 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -22,9 +22,13 @@ def redirect(path, options = { prefix: Whitehall.router_prefix }) resources :users, only: %i[index show edit update] - get "republishing" => "republishing#index", as: :republishing_index - get "republishing/:page_slug/confirm" => "republishing#confirm_page", as: :confirm_page_republishing - post "republishing/:page_slug/republish" => "republishing#republish_page", as: :republish_page + scope :republishing do + root to: "republishing#index", as: :republishing_index, via: :get + scope :page do + get "/:page_slug/confirm" => "republishing#confirm_page", as: :republishing_page_confirm + post "/:page_slug/republish" => "republishing#republish_page", as: :republishing_page_republish + end + end resources :documents, only: [] do resources :review_reminders, only: %i[new create edit update] diff --git a/test/functional/admin/republishing_controller_test.rb b/test/functional/admin/republishing_controller_test.rb index 646de37e83d..ef76f2d6c97 100644 --- a/test/functional/admin/republishing_controller_test.rb +++ b/test/functional/admin/republishing_controller_test.rb @@ -12,7 +12,7 @@ class Admin::RepublishingControllerTest < ActionController::TestCase get :index assert_select ".govuk-table__cell:nth-child(1) a[href='https://www.test.gov.uk/government/history/past-prime-ministers']", text: "Past Prime Ministers" - assert_select ".govuk-table__cell:nth-child(2) a[href='/government/admin/republishing/past-prime-ministers/confirm']", text: "Republish 'Past Prime Ministers' page" + assert_select ".govuk-table__cell:nth-child(2) a[href='/government/admin/republishing/page/past-prime-ministers/confirm']", text: "Republish 'Past Prime Ministers' page" assert_response :ok end From 7c06a0c9886b3fa3b63fa60d6a4ca8928ae8b93a Mon Sep 17 00:00:00 2001 From: Ynda Jas Date: Thu, 18 Apr 2024 12:47:48 +0100 Subject: [PATCH 6/6] Reorder republishing controller actions This makes them more chronological --- app/controllers/admin/republishing_controller.rb | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/app/controllers/admin/republishing_controller.rb b/app/controllers/admin/republishing_controller.rb index ef6fa37290a..7aeaafa6be6 100644 --- a/app/controllers/admin/republishing_controller.rb +++ b/app/controllers/admin/republishing_controller.rb @@ -5,23 +5,23 @@ def index @republishable_pages = republishable_pages end - def republish_page + def confirm_page page_to_republish = republishable_pages.find { |page| page[:slug] == params[:page_slug] } return render "admin/errors/not_found", status: :not_found unless page_to_republish - PresentPageToPublishingApiWorker.perform_async(page_to_republish[:presenter]) - flash[:notice] = "'#{page_to_republish[:title]}' page has been scheduled for republishing" - redirect_to(admin_republishing_index_path) + @title = page_to_republish[:title] + @republishing_path = admin_republishing_page_republish_path(page_to_republish[:slug]) end - def confirm_page + def republish_page page_to_republish = republishable_pages.find { |page| page[:slug] == params[:page_slug] } return render "admin/errors/not_found", status: :not_found unless page_to_republish - @title = page_to_republish[:title] - @republishing_path = admin_republishing_page_republish_path(page_to_republish[:slug]) + PresentPageToPublishingApiWorker.perform_async(page_to_republish[:presenter]) + flash[:notice] = "'#{page_to_republish[:title]}' page has been scheduled for republishing" + redirect_to(admin_republishing_index_path) end private