From c4edb4c7f3afc6415195de33b5939b59c783b3a4 Mon Sep 17 00:00:00 2001 From: Ryan Brown Date: Fri, 30 Aug 2024 14:14:42 +0100 Subject: [PATCH] Revert "Add an interface for GDS editors and admins to override the government associated with an edition" This reverts commit 22c5e18f3e34086929c7e73006666026ce705b4f. Because we weren't rendering the hidden `political` field for non-managing editor users, they were creating editions with null values in the political column. This then failed Publishing API validation. --- .../editions/history_mode_form_controls.erb | 31 ---------- .../editions/history_mode_form_controls.rb | 32 ---------- .../admin/editions/_standard_fields.html.erb | 24 ++++++- config/features.rb | 1 - .../authority/rules/edition_rules.rb | 4 +- .../history_mode_form_controls_test.rb | 62 ------------------- .../political_document_test.rb | 18 +++++- 7 files changed, 40 insertions(+), 132 deletions(-) delete mode 100644 app/components/admin/editions/history_mode_form_controls.erb delete mode 100644 app/components/admin/editions/history_mode_form_controls.rb delete mode 100644 test/components/admin/editions/history_mode_form_controls_test.rb diff --git a/app/components/admin/editions/history_mode_form_controls.erb b/app/components/admin/editions/history_mode_form_controls.erb deleted file mode 100644 index 85fb580870e..00000000000 --- a/app/components/admin/editions/history_mode_form_controls.erb +++ /dev/null @@ -1,31 +0,0 @@ -
- <%= render "govuk_publishing_components/components/input", { - name: "edition[political]", - type: "hidden", - value: "", - } %> - - <%= render "govuk_publishing_components/components/checkboxes", { - name: "edition[political]", - id: "edition_political", - heading: "Political", - heading_size: "l", - no_hint_text: true, - items: [ - { - label: "Associate with government of the time (currently set to #{@edition.government&.name}).", - value: "1", - checked: @edition.political, - conditional: renders_government_selector? ? (render "govuk_publishing_components/components/select", { - name: "edition[government_id]", - label: "Or, associate this document with a different government", - id: "edition_government_id", - options:, - }) : "", - }, - ], - } %> - -

<%= link_to "Read the history mode guidance", "https://www.gov.uk/guidance/how-to-publish-on-gov-uk/creating-and-updating-pages#history-mode", class: "govuk-link" %> - for more information as to what this means.

-
diff --git a/app/components/admin/editions/history_mode_form_controls.rb b/app/components/admin/editions/history_mode_form_controls.rb deleted file mode 100644 index 97d7aa64fe5..00000000000 --- a/app/components/admin/editions/history_mode_form_controls.rb +++ /dev/null @@ -1,32 +0,0 @@ -class Admin::Editions::HistoryModeFormControls < ViewComponent::Base - def initialize(edition, current_user) - @edition = edition - @enforcer = Whitehall::Authority::Enforcer.new(current_user, edition) - end - - def render? - @edition.document&.live? && @edition.can_be_marked_political? && @enforcer.can?(:mark_political) - end - - def renders_government_selector? - Flipflop.override_government? && @enforcer.can?(:select_government_for_history_mode) - end - - # noinspection RubyMismatchedArgumentType - def options - [ - { - text: "Associate with default government", - value: "", - }, - ].tap do |options| - Government.find_each do |government| - options << { - text: government.name, - value: government.id, - selected: government.id == @edition.government_id, - } - end - end - end -end diff --git a/app/views/admin/editions/_standard_fields.html.erb b/app/views/admin/editions/_standard_fields.html.erb index 7245e9a9e5e..20578017639 100644 --- a/app/views/admin/editions/_standard_fields.html.erb +++ b/app/views/admin/editions/_standard_fields.html.erb @@ -123,7 +123,29 @@ <%= render "additional_significant_fields", form: form, edition: form.object %> - <%= render Admin::Editions::HistoryModeFormControls.new(@edition, current_user) %> + <% if edition.document&.live? && edition.can_be_marked_political? && can?(:mark_political, edition) %> +
+ <%= form.hidden_field :political, value: "0" %> + + <%= render "govuk_publishing_components/components/checkboxes", { + name: "edition[political]", + id: "edition_political", + heading: "Political", + heading_size: "l", + no_hint_text: true, + error: errors_for_input(edition.errors, :political), + items: [ + { + label: "Associate with government of the time (currently set to #{edition.government&.name}).", + value: "1", + checked: edition.political, + }, + ], + } %> + +

<%= link_to "Read the history mode guidance", "https://www.gov.uk/guidance/how-to-publish-on-gov-uk/creating-and-updating-pages#history-mode", class: "govuk-link" %> for more information as to what this means.

+
+ <% end %> <%= render Admin::Editions::FirstPublishedAtComponent.new( diff --git a/config/features.rb b/config/features.rb index 36efc44e1da..56955683d82 100644 --- a/config/features.rb +++ b/config/features.rb @@ -23,5 +23,4 @@ # description: "Take over the world." feature :govspeak_visual_editor, description: "Enables a visual editor for Govspeak fields", default: false feature :content_object_store, description: "Enables the object store for sharable content", default: Rails.env.development? - feature :override_government, description: "Enables GDS Editors and Admins to override the government associated with a document", default: false end diff --git a/lib/whitehall/authority/rules/edition_rules.rb b/lib/whitehall/authority/rules/edition_rules.rb index 23fbd03b3c3..8d92336a85a 100644 --- a/lib/whitehall/authority/rules/edition_rules.rb +++ b/lib/whitehall/authority/rules/edition_rules.rb @@ -19,7 +19,6 @@ def self.actions review_editorial_remark review_fact_check see - select_government_for_history_mode unpublish unwithdraw update @@ -172,7 +171,7 @@ def departmental_editor_can?(action) can_publish? when :force_publish can_force_publish? - when :unpublish, :mark_political, :perform_administrative_tasks, :select_government_for_history_mode + when :unpublish, :mark_political, :perform_administrative_tasks false else true @@ -201,7 +200,6 @@ def departmental_writer_can?(action) force_publish reject mark_political - select_government_for_history_mode perform_administrative_tasks ] diff --git a/test/components/admin/editions/history_mode_form_controls_test.rb b/test/components/admin/editions/history_mode_form_controls_test.rb deleted file mode 100644 index 92526c951d1..00000000000 --- a/test/components/admin/editions/history_mode_form_controls_test.rb +++ /dev/null @@ -1,62 +0,0 @@ -require "test_helper" - -class Admin::Editions::HistoryModeFormControlsTest < ViewComponent::TestCase - setup do - @test_strategy = Flipflop::FeatureSet.current.test! - end - - test "hides the government selector for GDS editor users when the feature flag is disabled" do - @test_strategy.switch!(:override_government, false) - government = create(:current_government) - published_edition = create(:published_news_article, government_id: government.id) - new_draft = create(:news_article, document: published_edition.document, government_id: government.id) - render_inline(Admin::Editions::HistoryModeFormControls.new(new_draft, create(:gds_editor))) - assert_selector "select#edition_government_id", count: 0 - end - - test "conditionally displays the government selector for gds editor users" do - @test_strategy.switch!(:override_government, true) - previous_government = create(:previous_government) - governments = [create(:current_government), previous_government] - published_edition = create(:published_news_article, government_id: previous_government.id) - new_draft = create(:news_article, document: published_edition.document, government_id: previous_government.id) - render_inline(Admin::Editions::HistoryModeFormControls.new(new_draft, create(:gds_editor))) - assert_selector "#edition_political-0-conditional-0 select#edition_government_id" - assert_selector "option[value='']" - governments.each do |government| - assert_selector "option[value='#{government.id}']" - end - assert_selector "option[value='#{previous_government.id}'][selected='selected']" - end - - test "displays the political checkbox for managing editor users " do - @test_strategy.switch!(:override_government, true) - published_edition = create(:published_news_article) - new_draft = create(:news_article, document: published_edition.document) - render_inline(Admin::Editions::HistoryModeFormControls.new(new_draft, create(:managing_editor))) - assert_selector "#edition_political" - end - - test "doesn't display the government selector for managing editor users " do - @test_strategy.switch!(:override_government, true) - published_edition = create(:published_news_article) - new_draft = create(:news_article, document: published_edition.document) - render_inline(Admin::Editions::HistoryModeFormControls.new(new_draft, create(:managing_editor))) - assert_selector "select#edition_government_id", count: 0 - end - - test "doesn't display the political checkbox for writer users " do - @test_strategy.switch!(:override_government, true) - published_edition = create(:published_news_article) - new_draft = create(:news_article, document: published_edition.document) - render_inline(Admin::Editions::HistoryModeFormControls.new(new_draft, create(:writer))) - assert_selector "#edition_political", count: 0 - end - - test "doesn't display the political checkbox on creation" do - @test_strategy.switch!(:override_government, true) - new_draft = create(:news_article) - render_inline(Admin::Editions::HistoryModeFormControls.new(new_draft, create(:managing_editor))) - assert_selector "#edition_political", count: 0 - end -end diff --git a/test/functional/admin/generic_editions_controller_tests/political_document_test.rb b/test/functional/admin/generic_editions_controller_tests/political_document_test.rb index 56292d83703..c545eef2ee9 100644 --- a/test/functional/admin/generic_editions_controller_tests/political_document_test.rb +++ b/test/functional/admin/generic_editions_controller_tests/political_document_test.rb @@ -31,12 +31,26 @@ class Admin::GenericEditionsController::PolticalDocumentsTest < ActionController assert_equal previous_government, new_draft.reload.government end - view_test "displays the history mode form controls for privileged users " do + view_test "displays the political checkbox for privileged users " do + create(:current_government) login_as :managing_editor + published_edition = create(:published_news_article, first_published_at: 2.days.ago) + new_draft = create(:news_article, document: published_edition.document, first_published_at: 2.days.ago) + get :edit, params: { id: new_draft } + assert_select "#edition_political" + end + + view_test "doesn't display the political checkbox for non-privileged users " do published_edition = create(:published_news_article) new_draft = create(:news_article, document: published_edition.document) get :edit, params: { id: new_draft } - assert_select "#edition_political" + assert_select "#edition_political", count: 0 + end + + view_test "doesn't display the political checkbox on creation" do + login_as :managing_editor + get :new + assert_select "#edition_political", count: 0 end def edit_historic_document