From 3b20a789ff15d379a529d9f7d662254969d348ad Mon Sep 17 00:00:00 2001 From: syed-ali-tw Date: Wed, 9 Oct 2024 14:31:51 +0100 Subject: [PATCH 01/11] Add route and controller method for publish and confirm-unpublish co-authored-by: Helen Pickavance --- app/controllers/editions_controller.rb | 4 ++++ config/routes.rb | 2 ++ 2 files changed, 6 insertions(+) diff --git a/app/controllers/editions_controller.rb b/app/controllers/editions_controller.rb index 349b791db..5d9b26359 100644 --- a/app/controllers/editions_controller.rb +++ b/app/controllers/editions_controller.rb @@ -38,6 +38,10 @@ def unpublish render action: "show" end + def confirm_unpublish + render "editions/secondary_nav_tabs/confirm_unpublish" + end + protected def setup_view_paths diff --git a/config/routes.rb b/config/routes.rb index 7e286645a..4b7e8ca9f 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -25,6 +25,8 @@ get "related_external_links", to: "editions#linking" get "tagging", to: "editions#linking" get "unpublish" + get "unpublish/confirm-unpublish", to: "editions#confirm_unpublish", as: "confirm_unpublish" + post "process_unpublish" end end end From bf4b7a04b4e126130951532f487d3219649aba6e Mon Sep 17 00:00:00 2001 From: syed-ali-tw Date: Wed, 9 Oct 2024 14:37:04 +0100 Subject: [PATCH 02/11] Move resuable components to common helper Co-authored-by: Helen Pickavance --- app/helpers/common_components_helper.rb | 36 +++++++++++++++++++++++++ app/helpers/popular_links_helper.rb | 27 ------------------- 2 files changed, 36 insertions(+), 27 deletions(-) create mode 100644 app/helpers/common_components_helper.rb diff --git a/app/helpers/common_components_helper.rb b/app/helpers/common_components_helper.rb new file mode 100644 index 000000000..35e63a300 --- /dev/null +++ b/app/helpers/common_components_helper.rb @@ -0,0 +1,36 @@ +module CommonComponentsHelper + def header_for(tab_name) + render "govuk_publishing_components/components/heading", { + text: tab_name, + heading_level: 2, + margin_bottom: 5, + } + end + + def primary_button_for(model, url, text) + form_for model, url:, method: :post do + render "govuk_publishing_components/components/button", { + text:, + margin_bottom: 3, + } + end + end + + def secondary_button_for(model, url, text) + form_for model, url:, method: :post do + render "govuk_publishing_components/components/button", { + text:, + margin_bottom: 3, + secondary_solid: true, + } + end + end + + def primary_link_button_for(url, text) + render "govuk_publishing_components/components/button", { + text:, + href: url, + margin_bottom: 3, + } + end +end diff --git a/app/helpers/popular_links_helper.rb b/app/helpers/popular_links_helper.rb index 0bc89bb71..6c19a09ef 100644 --- a/app/helpers/popular_links_helper.rb +++ b/app/helpers/popular_links_helper.rb @@ -5,31 +5,4 @@ def popular_link_rows(item) rows << { key: "URL", value: item[:url] } rows.compact end - - def primary_button_for(model, url, text) - form_for model, url:, method: :post do - render "govuk_publishing_components/components/button", { - text:, - margin_bottom: 3, - } - end - end - - def secondary_button_for(model, url, text) - form_for model, url:, method: :post do - render "govuk_publishing_components/components/button", { - text:, - margin_bottom: 3, - secondary_solid: true, - } - end - end - - def primary_link_button_for(url, text) - render "govuk_publishing_components/components/button", { - text:, - href: url, - margin_bottom: 3, - } - end end From 744920e71cebeff873ab8efdc28ee2c8c27c656d Mon Sep 17 00:00:00 2001 From: syed-ali-tw Date: Wed, 9 Oct 2024 14:38:32 +0100 Subject: [PATCH 03/11] Add views for unpublish and confirm-unpublish page Co-authored-by: Helen Pickavance --- app/assets/stylesheets/application.scss | 1 + app/helpers/tabbed_nav_helper.rb | 10 ++- .../secondary_nav_tabs/_metadata.html.erb | 6 +- .../secondary_nav_tabs/_unpublish.html.erb | 24 +++++++ .../confirm_unpublish.html.erb | 20 ++++++ test/integration/edition_edit_test.rb | 72 ++++++++++++++++--- 6 files changed, 116 insertions(+), 17 deletions(-) create mode 100644 app/views/editions/secondary_nav_tabs/_unpublish.html.erb create mode 100644 app/views/editions/secondary_nav_tabs/confirm_unpublish.html.erb diff --git a/app/assets/stylesheets/application.scss b/app/assets/stylesheets/application.scss index f084f9bd0..c14949e5f 100644 --- a/app/assets/stylesheets/application.scss +++ b/app/assets/stylesheets/application.scss @@ -16,6 +16,7 @@ $govuk-page-width: 1140px; @import 'govuk_publishing_components/components/heading'; @import 'govuk_publishing_components/components/hint'; @import 'govuk_publishing_components/components/input'; +@import 'govuk_publishing_components/components/inset-text'; @import 'govuk_publishing_components/components/label'; @import 'govuk_publishing_components/components/layout-footer'; @import 'govuk_publishing_components/components/layout-for-admin'; diff --git a/app/helpers/tabbed_nav_helper.rb b/app/helpers/tabbed_nav_helper.rb index 13d95b6e8..e63c539d1 100644 --- a/app/helpers/tabbed_nav_helper.rb +++ b/app/helpers/tabbed_nav_helper.rb @@ -31,7 +31,15 @@ def edit_nav_item(label, href, current) def current_tab_name current_tab = (request.path.split("/") & all_tab_names).first - current_tab == "metadata" ? "metadata" : "temp_nav_text" + + case current_tab + when "metadata" + "metadata" + when "unpublish" + "unpublish" + else + "temp_nav_text" + end end private diff --git a/app/views/editions/secondary_nav_tabs/_metadata.html.erb b/app/views/editions/secondary_nav_tabs/_metadata.html.erb index 65c972f9b..50b56e9bc 100644 --- a/app/views/editions/secondary_nav_tabs/_metadata.html.erb +++ b/app/views/editions/secondary_nav_tabs/_metadata.html.erb @@ -1,10 +1,6 @@
- <%= render "govuk_publishing_components/components/heading", { - text: "Metadata", - heading_level: 2, - margin_bottom: 5, - } %> + <%= header_for("Metadata") %> <% if Edition::PUBLISHING_API_DRAFT_STATES.include? publication.state %> <%= form_for(@artefact, :html => { :class => "artefact", :id => "edit_artefact" }) do |f| %> diff --git a/app/views/editions/secondary_nav_tabs/_unpublish.html.erb b/app/views/editions/secondary_nav_tabs/_unpublish.html.erb new file mode 100644 index 000000000..ce07e2961 --- /dev/null +++ b/app/views/editions/secondary_nav_tabs/_unpublish.html.erb @@ -0,0 +1,24 @@ +<%= header_for("Unpublish") %> + +<%= render "govuk_publishing_components/components/inset_text", { + text: "If you unpublish a page from GOV.UK it cannot be undone." +} %> + +<%= form_for @edition, url: confirm_unpublish_edition_path, method: "get" do |f| %> + <%= render "govuk_publishing_components/components/input", { + label: { + text: "Redirect to URL", + }, + name: "unpublish[redirect-url]", + hint:"For example: https://www.gov.uk/redirect-to-replacement-page", + heading_level: 3, + heading_size: "s", + } %> +
+ <%= render("govuk_publishing_components/components/button", { + text: "Continue", + type: "submit", + }) %> +
+<% end %> + diff --git a/app/views/editions/secondary_nav_tabs/confirm_unpublish.html.erb b/app/views/editions/secondary_nav_tabs/confirm_unpublish.html.erb new file mode 100644 index 000000000..c707738ee --- /dev/null +++ b/app/views/editions/secondary_nav_tabs/confirm_unpublish.html.erb @@ -0,0 +1,20 @@ +<% content_for :title_context, @resource.title %> +<% content_for :page_title, "Unpublish" %> +<% content_for :title, "Unpublish" %> + +<%= render "govuk_publishing_components/components/inset_text", { + text: "If you unpublish a page from GOV.UK it cannot be undone." +} %> + +
+ <%= form_tag process_unpublish_edition_path(@resource), method: :post do %> +

Are you sure you want to unpublish this document?

+
+ <%= render "govuk_publishing_components/components/button", { + text: "Unpublish document", + destructive: true, + } %> + <%= link_to("Cancel", unpublish_edition_path, class: "govuk-link govuk-link--no-visited-state") %> +
+ <% end %> +
diff --git a/test/integration/edition_edit_test.rb b/test/integration/edition_edit_test.rb index 0d06654f2..846e4d6c5 100644 --- a/test/integration/edition_edit_test.rb +++ b/test/integration/edition_edit_test.rb @@ -67,19 +67,20 @@ class EditionEditTest < IntegrationTest end context "when edition is published" do + setup do + @edition = FactoryBot.create( + :completed_transaction_edition, + panopticon_id: FactoryBot.create( + :artefact, + slug: "can-i-get-a-driving-licence", + ).id, + state: "published", + slug: "can-i-get-a-driving-licence", + ) + visit edition_path(@edition) + end context "metadata tab" do setup do - edition = FactoryBot.create( - :completed_transaction_edition, - panopticon_id: FactoryBot.create( - :artefact, - slug: "can-i-get-a-driving-licence", - ).id, - state: "published", - slug: "can-i-get-a-driving-licence", - ) - - visit edition_path(edition) click_link("Metadata") end @@ -93,5 +94,54 @@ class EditionEditTest < IntegrationTest assert page.has_text?(/English/) end end + + context "unpublish tab" do + setup do + click_link("Unpublish") + end + + should "show 'Unpublish' header and 'Continue' button" do + within :css, ".gem-c-heading" do + assert page.has_text?("Unpublish") + end + assert page.has_button?("Continue") + end + + should "show 'cannot be undone' banner" do + assert page.has_text?("If you unpublish a page from GOV.UK it cannot be undone.") + end + + should "show 'Redirect to URL' text, input box and example text" do + assert page.has_text?("Redirect to URL") + assert page.has_text?("For example: https://www.gov.uk/redirect-to-replacement-page") + assert page.has_css?(".govuk-input", count: 1) + end + + should "navigate to 'confirm-unpublish' page when clicked on 'Continue' button" do + click_button("Continue") + assert_equal(page.current_path, "/editions/#{@edition.id}/unpublish/confirm-unpublish") + end + end + + context "confirm unpublish" do + setup do + click_link("Unpublish") + click_button("Continue") + end + + should "show 'Unpublish' header and document title" do + assert page.has_text?("Unpublish") + assert page.has_text?(@edition.title.to_s) + end + + should "show 'cannot be undone' banner" do + assert page.has_text?("If you unpublish a page from GOV.UK it cannot be undone.") + end + + should "show 'Unpublish document' button and 'Cancel' link" do + assert page.has_button?("Unpublish document") + assert page.has_link?("Cancel") + end + end end end From ec3e125021b860cb1c537c91c0b9f86362fba836 Mon Sep 17 00:00:00 2001 From: syed-ali-tw Date: Mon, 14 Oct 2024 17:33:18 +0100 Subject: [PATCH 04/11] Add process unpublish --- app/controllers/editions_controller.rb | 54 +++++++++++- app/helpers/tabbed_nav_helper.rb | 2 + .../secondary_nav_tabs/_unpublish.html.erb | 6 +- .../confirm_unpublish.html.erb | 21 ++++- app/views/editions/show.html.erb | 15 ++++ test/functional/editions_controller_test.rb | 87 +++++++++++++++++++ test/integration/edition_edit_test.rb | 16 +++- 7 files changed, 193 insertions(+), 8 deletions(-) diff --git a/app/controllers/editions_controller.rb b/app/controllers/editions_controller.rb index 5d9b26359..2422d220e 100644 --- a/app/controllers/editions_controller.rb +++ b/app/controllers/editions_controller.rb @@ -3,10 +3,14 @@ class EditionsController < InheritedResources::Base layout "design_system" defaults resource_class: Edition, collection_name: "editions", instance_name: "resource" + before_action :setup_view_paths, except: %i[index] before_action except: %i[index] do require_user_accessibility_to_edition(@resource) end + before_action only: %i[unpublish confirm_unpublish process_unpublish] do + require_govuk_editor(redirect_path: edition_path(resource)) + end helper_method :locale_to_language @@ -21,6 +25,7 @@ def show end alias_method :metadata, :show + alias_method :unpublish, :show def history render action: "show" @@ -34,12 +39,32 @@ def linking render action: "show" end - def unpublish - render action: "show" + def confirm_unpublish + if redirect_url.blank? || validate_redirect(redirect_url) + render "secondary_nav_tabs/confirm_unpublish" + else + error_message = "Redirect path is invalid. #{description(resource)} can not be unpublished." + @resource.errors.add(:redirect_url, error_message) + render "show" + end end - def confirm_unpublish - render "editions/secondary_nav_tabs/confirm_unpublish" + def process_unpublish + artefact = @resource.artefact + + success = params["redirect_url"].strip.empty? ? UnpublishService.call(artefact, current_user) : UnpublishService.call(artefact, current_user, redirect_url) + if success + notice = "Content unpublished" + notice << " and redirected" if redirect_url.present? + flash[:success] = notice + redirect_to root_path + else + @resource.errors.add(:unpublish, downstream_error_message) + render "secondary_nav_tabs/confirm_unpublish" + end + rescue StandardError + @resource.errors.add(:unpublish, downstream_error_message) + render "secondary_nav_tabs/confirm_unpublish" end protected @@ -50,6 +75,10 @@ def setup_view_paths private + def downstream_error_message + "Due to a service problem, the edition couldn't be unpublished" + end + def setup_view_paths_for(publication) prepend_view_path "app/views/editions" prepend_view_path template_folder_for(publication) @@ -65,4 +94,21 @@ def locale_to_language(locale) "" end end + + def validate_redirect(redirect_url) + regex = /(\/([a-z0-9]+-)*[a-z0-9]+)+/ + redirect_url =~ regex + end + + def make_govuk_url_relative(url = "") + url.sub(%r{^(https?://)?(www\.)?gov\.uk/}, "/") + end + + def redirect_url + make_govuk_url_relative params["redirect_url"] + end + + def description(resource) + resource.format.underscore.humanize + end end diff --git a/app/helpers/tabbed_nav_helper.rb b/app/helpers/tabbed_nav_helper.rb index e63c539d1..25b9ca863 100644 --- a/app/helpers/tabbed_nav_helper.rb +++ b/app/helpers/tabbed_nav_helper.rb @@ -3,6 +3,8 @@ def edition_nav_items(edition) nav_items = [] all_tab_names.each do |item| + next if edition.state == "draft" && item == "unpublish" + nav_items << standard_nav_items(item, edition) end diff --git a/app/views/editions/secondary_nav_tabs/_unpublish.html.erb b/app/views/editions/secondary_nav_tabs/_unpublish.html.erb index ce07e2961..24f55d7ba 100644 --- a/app/views/editions/secondary_nav_tabs/_unpublish.html.erb +++ b/app/views/editions/secondary_nav_tabs/_unpublish.html.erb @@ -1,3 +1,4 @@ +<%@edition= @resource %> <%= header_for("Unpublish") %> <%= render "govuk_publishing_components/components/inset_text", { @@ -9,10 +10,13 @@ label: { text: "Redirect to URL", }, - name: "unpublish[redirect-url]", + id: "redirect_url", + name: "redirect_url", + value: params[:redirect_url], hint:"For example: https://www.gov.uk/redirect-to-replacement-page", heading_level: 3, heading_size: "s", + error_items: errors_for(f.object.errors,:redirect_url, use_full_message: false) } %>
<%= render("govuk_publishing_components/components/button", { diff --git a/app/views/editions/secondary_nav_tabs/confirm_unpublish.html.erb b/app/views/editions/secondary_nav_tabs/confirm_unpublish.html.erb index c707738ee..3b470126f 100644 --- a/app/views/editions/secondary_nav_tabs/confirm_unpublish.html.erb +++ b/app/views/editions/secondary_nav_tabs/confirm_unpublish.html.erb @@ -1,13 +1,30 @@ -<% content_for :title_context, @resource.title %> +<%@edition = @resource %> +<% content_for :title_context, @edition.title %> <% content_for :page_title, "Unpublish" %> <% content_for :title, "Unpublish" %> +<% unless @edition.errors.empty? %> + <% content_for :error_summary do %> + <%= render("govuk_publishing_components/components/error_summary", { + id: "error-summary", + title: "There is a problem", + items: @edition.errors.map do |error| + { + text: error.message, + href: "##{error.attribute.to_s}", + } + end, + }) %> + <% end %> +<% end %> + <%= render "govuk_publishing_components/components/inset_text", { text: "If you unpublish a page from GOV.UK it cannot be undone." } %>
- <%= form_tag process_unpublish_edition_path(@resource), method: :post do %> + <%= form_for @edition, url: process_unpublish_edition_path(@edition), method: :post do %> + <%= hidden_field_tag :redirect_url, params[:redirect_url]%>

Are you sure you want to unpublish this document?

<%= render "govuk_publishing_components/components/button", { diff --git a/app/views/editions/show.html.erb b/app/views/editions/show.html.erb index 0d1bfc54e..cf004a0ca 100644 --- a/app/views/editions/show.html.erb +++ b/app/views/editions/show.html.erb @@ -3,6 +3,21 @@ <% content_for :page_title, @resource.title %> <% content_for :title, @resource.title %> +<% unless @edition.errors.empty? %> + <% content_for :error_summary do %> + <%= render("govuk_publishing_components/components/error_summary", { + id: "error-summary", + title: "There is a problem", + items: @edition.errors.map do |error| + { + text: error.message, + href: "##{error.attribute.to_s}", + } + end, + }) %> + <% end %> +<% end %> +
<%= render "govuk_publishing_components/components/summary_list", { diff --git a/test/functional/editions_controller_test.rb b/test/functional/editions_controller_test.rb index a9791b4cf..5a801f3ac 100644 --- a/test/functional/editions_controller_test.rb +++ b/test/functional/editions_controller_test.rb @@ -113,4 +113,91 @@ class EditionsControllerTest < ActionController::TestCase end end end + + context "#unpublish" do + setup do + artefact = FactoryBot.create( + :artefact, + slug: "test2", + kind: "guide", + name: "test", + owning_app: "publisher", + ) + @guide = GuideEdition.create!(title: "test", slug: "test2", panopticon_id: artefact.id) + end + + should "redirect to edition_path when user does not have govuk-editor permission" do + user = FactoryBot.create(:user, :welsh_editor, name: "Stub User") + login_as(user) + get :unpublish, params: { id: @guide.id } + + assert_redirected_to edition_path(@guide) + end + + context "#confirm_unpublish" do + should "redirect to edition_path when user does not have govuk-editor permission" do + user = FactoryBot.create(:user, :welsh_editor, name: "Stub User") + login_as(user) + get :confirm_unpublish, params: { id: @guide.id } + + assert_redirected_to edition_path(@guide) + end + + should "render 'confirm_unpublish' template if redirect url is blank" do + get :confirm_unpublish, params: { id: @guide.id, redirect_url: "" } + + assert_template "secondary_nav_tabs/confirm_unpublish" + end + + should "render 'confirm_unpublish' template if redirect url is a valid url" do + get :confirm_unpublish, params: { id: @guide.id, redirect_url: "https://www.gov.uk/redirect-to-replacement-page" } + + assert_template "secondary_nav_tabs/confirm_unpublish" + end + + should "render show template with error message when redirect url is not valid" do + get :confirm_unpublish, params: { id: @guide.id, redirect_url: "bob" } + + assert_select ".gem-c-error-summary__list-item", "Redirect path is invalid. Guide can not be unpublished." + assert_template "show" + end + end + + context "#process_unpublish" do + should "redirect to edition_path when user does not have govuk-editor permission" do + user = FactoryBot.create(:user, :welsh_editor, name: "Stub User") + login_as(user) + get :confirm_unpublish, params: { id: @guide.id, redirect_url: nil } + + assert_redirected_to edition_path(@guide) + end + + should "show success message and redirect to root path when unpublished successfully with redirect url" do + get :process_unpublish, params: { id: @guide.id, redirect_url: "https://www.gov.uk/redirect-to-replacement-page" } + + assert_equal "Content unpublished and redirected", flash[:success] + end + + should "show success message and redirect to root path when unpublished successfully without redirect url" do + UnpublishService.stubs(:call).returns(true) + get :process_unpublish, params: { id: @guide.id, redirect_url: nil } + + assert_equal "Content unpublished", flash[:success] + end + + should "show error message when unpublish is unsuccessful" do + UnpublishService.stubs(:call).returns(nil) + get :process_unpublish, params: { id: @guide.id, redirect_url: nil } + + assert_select ".gem-c-error-summary__list-item", "Due to a service problem, the edition couldn't be unpublished" + end + + should "show error message when unpublish service returns an error" do + UnpublishService.stubs(:call).raises(StandardError) + get :process_unpublish, params: { id: @guide.id, redirect_url: nil } + + assert_select ".gem-c-error-summary__list-item", "Due to a service problem, the edition couldn't be unpublished" + end + end + end end diff --git a/test/integration/edition_edit_test.rb b/test/integration/edition_edit_test.rb index 846e4d6c5..5b25d7fde 100644 --- a/test/integration/edition_edit_test.rb +++ b/test/integration/edition_edit_test.rb @@ -33,7 +33,10 @@ class EditionEditTest < IntegrationTest assert page.has_text?("History and notes") assert page.has_text?("Admin") assert page.has_text?("Related external links") - assert page.has_text?("Unpublish") + end + + should "not show unpublish tab" do + assert page.has_no_text?("Unpublish") end context "metadata tab" do @@ -79,6 +82,17 @@ class EditionEditTest < IntegrationTest ) visit edition_path(@edition) end + + should "show all the tabs for the published edition" do + assert page.has_text?("Edit") + assert page.has_text?("Tagging") + assert page.has_text?("Metadata") + assert page.has_text?("History and notes") + assert page.has_text?("Admin") + assert page.has_text?("Related external links") + assert page.has_text?("Unpublish") + end + context "metadata tab" do setup do click_link("Metadata") From 291f6df8fd7939902d697fc08270865e6db25739 Mon Sep 17 00:00:00 2001 From: Helen Pickavance Date: Tue, 15 Oct 2024 12:09:17 +0100 Subject: [PATCH 05/11] Resolve test issues and layout error - Published and draft tests separated for TabbedNavHelperTest - Unpublish tab removed from editions_controller_test - awaiting investigation - Add div class to fix layout to bottom tab section - Correct linting/rubocop errors --- .../secondary_nav_tabs/_unpublish.html.erb | 51 ++++++++++--------- .../confirm_unpublish.html.erb | 6 +-- test/functional/editions_controller_test.rb | 2 +- .../helpers/admin/tabbed_nav_helper_test.rb | 41 ++++++++++++++- 4 files changed, 70 insertions(+), 30 deletions(-) diff --git a/app/views/editions/secondary_nav_tabs/_unpublish.html.erb b/app/views/editions/secondary_nav_tabs/_unpublish.html.erb index 24f55d7ba..47850881e 100644 --- a/app/views/editions/secondary_nav_tabs/_unpublish.html.erb +++ b/app/views/editions/secondary_nav_tabs/_unpublish.html.erb @@ -1,28 +1,29 @@ -<%@edition= @resource %> -<%= header_for("Unpublish") %> +
+ <% @edition = @resource %> + <%= header_for("Unpublish") %> -<%= render "govuk_publishing_components/components/inset_text", { - text: "If you unpublish a page from GOV.UK it cannot be undone." -} %> - -<%= form_for @edition, url: confirm_unpublish_edition_path, method: "get" do |f| %> - <%= render "govuk_publishing_components/components/input", { - label: { - text: "Redirect to URL", - }, - id: "redirect_url", - name: "redirect_url", - value: params[:redirect_url], - hint:"For example: https://www.gov.uk/redirect-to-replacement-page", - heading_level: 3, - heading_size: "s", - error_items: errors_for(f.object.errors,:redirect_url, use_full_message: false) + <%= render "govuk_publishing_components/components/inset_text", { + text: "If you unpublish a page from GOV.UK it cannot be undone.", } %> -
- <%= render("govuk_publishing_components/components/button", { - text: "Continue", - type: "submit", - }) %> -
-<% end %> + <%= form_for @edition, url: confirm_unpublish_edition_path, method: "get" do |f| %> + <%= render "govuk_publishing_components/components/input", { + label: { + text: "Redirect to URL", + }, + id: "redirect_url", + name: "redirect_url", + value: params[:redirect_url], + hint: "For example: https://www.gov.uk/redirect-to-replacement-page", + heading_level: 3, + heading_size: "s", + error_items: errors_for(f.object.errors, :redirect_url, use_full_message: false), + } %> +
+ <%= render("govuk_publishing_components/components/button", { + text: "Continue", + type: "submit", + }) %> +
+ <% end %> +
diff --git a/app/views/editions/secondary_nav_tabs/confirm_unpublish.html.erb b/app/views/editions/secondary_nav_tabs/confirm_unpublish.html.erb index 3b470126f..cbba584d0 100644 --- a/app/views/editions/secondary_nav_tabs/confirm_unpublish.html.erb +++ b/app/views/editions/secondary_nav_tabs/confirm_unpublish.html.erb @@ -1,4 +1,4 @@ -<%@edition = @resource %> +<% @edition = @resource %> <% content_for :title_context, @edition.title %> <% content_for :page_title, "Unpublish" %> <% content_for :title, "Unpublish" %> @@ -19,12 +19,12 @@ <% end %> <%= render "govuk_publishing_components/components/inset_text", { - text: "If you unpublish a page from GOV.UK it cannot be undone." + text: "If you unpublish a page from GOV.UK it cannot be undone.", } %>
<%= form_for @edition, url: process_unpublish_edition_path(@edition), method: :post do %> - <%= hidden_field_tag :redirect_url, params[:redirect_url]%> + <%= hidden_field_tag :redirect_url, params[:redirect_url] %>

Are you sure you want to unpublish this document?

<%= render "govuk_publishing_components/components/button", { diff --git a/test/functional/editions_controller_test.rb b/test/functional/editions_controller_test.rb index 5a801f3ac..54dbce91b 100644 --- a/test/functional/editions_controller_test.rb +++ b/test/functional/editions_controller_test.rb @@ -69,7 +69,7 @@ class EditionsControllerTest < ActionController::TestCase end context "when 'restrict_access_by_org' feature toggle is disabled" do - %i[show metadata history admin linking unpublish].each do |action| + %i[show metadata history admin linking].each do |action| context "##{action}" do setup do @edition = FactoryBot.create(:edition, owning_org_content_ids: %w[org-two]) diff --git a/test/unit/helpers/admin/tabbed_nav_helper_test.rb b/test/unit/helpers/admin/tabbed_nav_helper_test.rb index 36381bd57..d73449e89 100644 --- a/test/unit/helpers/admin/tabbed_nav_helper_test.rb +++ b/test/unit/helpers/admin/tabbed_nav_helper_test.rb @@ -1,9 +1,48 @@ require "test_helper" class TabbedNavHelperTest < ActionView::TestCase - test "#secondary_navigation_tabs_items for edit edition page" do + test "#secondary_navigation_tabs_items for draft edition edit page" do resource = FactoryBot.create(:guide_edition, title: "Edit page title", state: "draft") + expected_output = [ + { + label: "Edit", + href: "/editions/#{resource.id}", + current: false, + }, + { + label: "Tagging", + href: "/editions/#{resource.id}/tagging", + current: false, + }, + { + label: "Metadata", + href: "/editions/#{resource.id}/metadata", + current: false, + }, + { + label: "History and notes", + href: "/editions/#{resource.id}/history", + current: false, + }, + { + label: "Admin", + href: "/editions/#{resource.id}/admin", + current: false, + }, + { + label: "Related external links", + href: "/editions/#{resource.id}/related_external_links", + current: false, + }, + ] + + assert_equal expected_output, edition_nav_items(resource) + end + + test "#secondary_navigation_tabs_items for published edition edit page" do + resource = FactoryBot.create(:guide_edition, title: "Edit page title", state: "published") + expected_output = [ { label: "Edit", From 7e69d41cc590b2ae22769dd3e2aa7567807174f6 Mon Sep 17 00:00:00 2001 From: Helen Pickavance Date: Wed, 16 Oct 2024 12:16:37 +0100 Subject: [PATCH 06/11] Change the order of array comparision for allowed keys --- app/models/edition.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/app/models/edition.rb b/app/models/edition.rb index 5816bf858..ca584a9aa 100644 --- a/app/models/edition.rb +++ b/app/models/edition.rb @@ -343,7 +343,7 @@ def cloning_between_parted_types?(new_edition) def self.find_or_create_from_panopticon_data(panopticon_id, importing_user) existing_publication = Edition.where(panopticon_id:) - .order_by(version_number: :desc).first + .order_by(version_number: :desc).first return existing_publication if existing_publication metadata = Artefact.find(panopticon_id) @@ -422,8 +422,9 @@ def check_for_archived_artefact a = Artefact.find(panopticon_id) if (a.state == "archived") && changes.any? # If we're only changing the state to archived, that's ok - # Any other changes are not allowed - allowed_keys = %w[state updated_at] + # We have to add owning_org_content_ids as Mongo changes the + # value from nil to an empty array on save - reads as a change + allowed_keys = %w[state updated_at owning_org_content_ids] unless (changes.keys - allowed_keys).empty? && (state == "archived") raise "Editing of an edition with an Archived artefact is not allowed" end From a08b68fe965e8df3cd05e59664ea856ce644f8d1 Mon Sep 17 00:00:00 2001 From: syed-ali-tw Date: Wed, 16 Oct 2024 23:52:03 +0100 Subject: [PATCH 07/11] Fix govuk grid --- .../secondary_nav_tabs/_unpublish.html.erb | 54 +++++++++-------- .../confirm_unpublish.html.erb | 59 ++++++++++--------- 2 files changed, 58 insertions(+), 55 deletions(-) diff --git a/app/views/editions/secondary_nav_tabs/_unpublish.html.erb b/app/views/editions/secondary_nav_tabs/_unpublish.html.erb index 47850881e..cd89c2f71 100644 --- a/app/views/editions/secondary_nav_tabs/_unpublish.html.erb +++ b/app/views/editions/secondary_nav_tabs/_unpublish.html.erb @@ -1,29 +1,31 @@ -
- <% @edition = @resource %> - <%= header_for("Unpublish") %> +
+
+ <% @edition = @resource %> + <%= header_for("Unpublish") %> - <%= render "govuk_publishing_components/components/inset_text", { - text: "If you unpublish a page from GOV.UK it cannot be undone.", - } %> - - <%= form_for @edition, url: confirm_unpublish_edition_path, method: "get" do |f| %> - <%= render "govuk_publishing_components/components/input", { - label: { - text: "Redirect to URL", - }, - id: "redirect_url", - name: "redirect_url", - value: params[:redirect_url], - hint: "For example: https://www.gov.uk/redirect-to-replacement-page", - heading_level: 3, - heading_size: "s", - error_items: errors_for(f.object.errors, :redirect_url, use_full_message: false), + <%= render "govuk_publishing_components/components/inset_text", { + text: "If you unpublish a page from GOV.UK it cannot be undone.", } %> -
- <%= render("govuk_publishing_components/components/button", { - text: "Continue", - type: "submit", - }) %> -
- <% end %> + + <%= form_for @edition, url: confirm_unpublish_edition_path, method: "get" do |f| %> + <%= render "govuk_publishing_components/components/input", { + label: { + text: "Redirect to URL", + }, + id: "redirect_url", + name: "redirect_url", + value: params[:redirect_url], + hint: "For example: https://www.gov.uk/redirect-to-replacement-page", + heading_level: 3, + heading_size: "s", + error_items: errors_for(f.object.errors, :redirect_url, use_full_message: false), + } %> +
+ <%= render("govuk_publishing_components/components/button", { + text: "Continue", + type: "submit", + }) %> +
+ <% end %> +
diff --git a/app/views/editions/secondary_nav_tabs/confirm_unpublish.html.erb b/app/views/editions/secondary_nav_tabs/confirm_unpublish.html.erb index cbba584d0..802eaffac 100644 --- a/app/views/editions/secondary_nav_tabs/confirm_unpublish.html.erb +++ b/app/views/editions/secondary_nav_tabs/confirm_unpublish.html.erb @@ -2,36 +2,37 @@ <% content_for :title_context, @edition.title %> <% content_for :page_title, "Unpublish" %> <% content_for :title, "Unpublish" %> - -<% unless @edition.errors.empty? %> - <% content_for :error_summary do %> - <%= render("govuk_publishing_components/components/error_summary", { - id: "error-summary", - title: "There is a problem", - items: @edition.errors.map do |error| - { - text: error.message, - href: "##{error.attribute.to_s}", - } - end, - }) %> +
+ <% unless @edition.errors.empty? %> + <% content_for :error_summary do %> + <%= render("govuk_publishing_components/components/error_summary", { + id: "error-summary", + title: "There is a problem", + items: @edition.errors.map do |error| + { + text: error.message, + href: "##{error.attribute.to_s}", + } + end, + }) %> + <% end %> <% end %> -<% end %> -<%= render "govuk_publishing_components/components/inset_text", { - text: "If you unpublish a page from GOV.UK it cannot be undone.", -} %> + <%= render "govuk_publishing_components/components/inset_text", { + text: "If you unpublish a page from GOV.UK it cannot be undone.", + } %> -
- <%= form_for @edition, url: process_unpublish_edition_path(@edition), method: :post do %> - <%= hidden_field_tag :redirect_url, params[:redirect_url] %> -

Are you sure you want to unpublish this document?

-
- <%= render "govuk_publishing_components/components/button", { - text: "Unpublish document", - destructive: true, - } %> - <%= link_to("Cancel", unpublish_edition_path, class: "govuk-link govuk-link--no-visited-state") %> -
- <% end %> +
+ <%= form_for @edition, url: process_unpublish_edition_path(@edition), method: :post do %> + <%= hidden_field_tag :redirect_url, params[:redirect_url] %> +

Are you sure you want to unpublish this document?

+
+ <%= render "govuk_publishing_components/components/button", { + text: "Unpublish document", + destructive: true, + } %> + <%= link_to("Cancel", unpublish_edition_path, class: "govuk-link govuk-link--no-visited-state") %> +
+ <% end %> +
From 7bfdced7cc4d2d3161888017d3a389d5900d00be Mon Sep 17 00:00:00 2001 From: syed-ali-tw Date: Thu, 17 Oct 2024 10:55:54 +0100 Subject: [PATCH 08/11] Remove unpublish tab for when edition is not published --- app/helpers/tabbed_nav_helper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/helpers/tabbed_nav_helper.rb b/app/helpers/tabbed_nav_helper.rb index 25b9ca863..a174cf782 100644 --- a/app/helpers/tabbed_nav_helper.rb +++ b/app/helpers/tabbed_nav_helper.rb @@ -3,7 +3,7 @@ def edition_nav_items(edition) nav_items = [] all_tab_names.each do |item| - next if edition.state == "draft" && item == "unpublish" + next if !edition.state.eql?("published") && item == "unpublish" nav_items << standard_nav_items(item, edition) end From dad4b1132e207b09f832864d5515c79fff49b9a8 Mon Sep 17 00:00:00 2001 From: syed-ali-tw Date: Thu, 17 Oct 2024 11:57:53 +0100 Subject: [PATCH 09/11] Add govuk editor permission to test to prevent redirections for unpublish --- test/functional/editions_controller_test.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/functional/editions_controller_test.rb b/test/functional/editions_controller_test.rb index 54dbce91b..e9edb2977 100644 --- a/test/functional/editions_controller_test.rb +++ b/test/functional/editions_controller_test.rb @@ -69,14 +69,14 @@ class EditionsControllerTest < ActionController::TestCase end context "when 'restrict_access_by_org' feature toggle is disabled" do - %i[show metadata history admin linking].each do |action| + %i[show metadata history admin linking unpublish].each do |action| context "##{action}" do setup do @edition = FactoryBot.create(:edition, owning_org_content_ids: %w[org-two]) end should "return a 200 when requesting an edition owned by a different organisation" do - login_as(FactoryBot.create(:user, organisation_content_id: "org-one")) + login_as(FactoryBot.create(:user, :govuk_editor, organisation_content_id: "org-one")) get action, params: { id: @edition.id } From d20ad3e61adee054a478b5ed33fbb8a156a57580 Mon Sep 17 00:00:00 2001 From: syed-ali-tw Date: Thu, 17 Oct 2024 14:43:37 +0100 Subject: [PATCH 10/11] Move confirm unpublish banner to inside govuk grid --- .../confirm_unpublish.html.erb | 32 ++++++++++--------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/app/views/editions/secondary_nav_tabs/confirm_unpublish.html.erb b/app/views/editions/secondary_nav_tabs/confirm_unpublish.html.erb index 802eaffac..28d3da0db 100644 --- a/app/views/editions/secondary_nav_tabs/confirm_unpublish.html.erb +++ b/app/views/editions/secondary_nav_tabs/confirm_unpublish.html.erb @@ -18,21 +18,23 @@ <% end %> <% end %> - <%= render "govuk_publishing_components/components/inset_text", { - text: "If you unpublish a page from GOV.UK it cannot be undone.", - } %> +
+
+ <%= render "govuk_publishing_components/components/inset_text", { + text: "If you unpublish a page from GOV.UK it cannot be undone.", + } %> -
- <%= form_for @edition, url: process_unpublish_edition_path(@edition), method: :post do %> - <%= hidden_field_tag :redirect_url, params[:redirect_url] %> -

Are you sure you want to unpublish this document?

-
- <%= render "govuk_publishing_components/components/button", { - text: "Unpublish document", - destructive: true, - } %> - <%= link_to("Cancel", unpublish_edition_path, class: "govuk-link govuk-link--no-visited-state") %> -
- <% end %> + <%= form_for @edition, url: process_unpublish_edition_path(@edition), method: :post do %> + <%= hidden_field_tag :redirect_url, params[:redirect_url] %> +

Are you sure you want to unpublish this document?

+
+ <%= render "govuk_publishing_components/components/button", { + text: "Unpublish document", + destructive: true, + } %> + <%= link_to("Cancel", unpublish_edition_path, class: "govuk-link govuk-link--no-visited-state") %> +
+ <% end %> +
From d3cea9fd5b0475a04cac0222628b57831670143b Mon Sep 17 00:00:00 2001 From: syed-ali-tw Date: Fri, 18 Oct 2024 08:17:56 +0100 Subject: [PATCH 11/11] Refactor controller to improve readability --- app/controllers/editions_controller.rb | 17 +++++++--- .../confirm_unpublish.html.erb | 33 +++++++++---------- 2 files changed, 28 insertions(+), 22 deletions(-) diff --git a/app/controllers/editions_controller.rb b/app/controllers/editions_controller.rb index 2422d220e..a52958e33 100644 --- a/app/controllers/editions_controller.rb +++ b/app/controllers/editions_controller.rb @@ -52,19 +52,17 @@ def confirm_unpublish def process_unpublish artefact = @resource.artefact - success = params["redirect_url"].strip.empty? ? UnpublishService.call(artefact, current_user) : UnpublishService.call(artefact, current_user, redirect_url) + success = unpublish_edition(artefact) if success notice = "Content unpublished" notice << " and redirected" if redirect_url.present? flash[:success] = notice redirect_to root_path else - @resource.errors.add(:unpublish, downstream_error_message) - render "secondary_nav_tabs/confirm_unpublish" + render_confirm_page_with_error end rescue StandardError - @resource.errors.add(:unpublish, downstream_error_message) - render "secondary_nav_tabs/confirm_unpublish" + render_confirm_page_with_error end protected @@ -75,6 +73,15 @@ def setup_view_paths private + def unpublish_edition(artefact) + params["redirect_url"].strip.empty? ? UnpublishService.call(artefact, current_user) : UnpublishService.call(artefact, current_user, redirect_url) + end + + def render_confirm_page_with_error + @resource.errors.add(:unpublish, downstream_error_message) + render "secondary_nav_tabs/confirm_unpublish" + end + def downstream_error_message "Due to a service problem, the edition couldn't be unpublished" end diff --git a/app/views/editions/secondary_nav_tabs/confirm_unpublish.html.erb b/app/views/editions/secondary_nav_tabs/confirm_unpublish.html.erb index 28d3da0db..006172cb9 100644 --- a/app/views/editions/secondary_nav_tabs/confirm_unpublish.html.erb +++ b/app/views/editions/secondary_nav_tabs/confirm_unpublish.html.erb @@ -18,23 +18,22 @@ <% end %> <% end %> -
-
- <%= render "govuk_publishing_components/components/inset_text", { - text: "If you unpublish a page from GOV.UK it cannot be undone.", - } %> +
+ <%= render "govuk_publishing_components/components/inset_text", { + text: "If you unpublish a page from GOV.UK it cannot be undone.", + } %> - <%= form_for @edition, url: process_unpublish_edition_path(@edition), method: :post do %> - <%= hidden_field_tag :redirect_url, params[:redirect_url] %> -

Are you sure you want to unpublish this document?

-
- <%= render "govuk_publishing_components/components/button", { - text: "Unpublish document", - destructive: true, - } %> - <%= link_to("Cancel", unpublish_edition_path, class: "govuk-link govuk-link--no-visited-state") %> -
- <% end %> -
+ <%= form_for @edition, url: process_unpublish_edition_path(@edition), method: :post do %> + <%= hidden_field_tag :redirect_url, params[:redirect_url] %> +

Are you sure you want to unpublish this document?

+
+ <%= render "govuk_publishing_components/components/button", { + text: "Unpublish document", + destructive: true, + } %> + <%= link_to("Cancel", unpublish_edition_path, class: "govuk-link govuk-link--no-visited-state") %> +
+ <% end %>
+