From e18725fd44b17142471e6cd176e710e471b5dd25 Mon Sep 17 00:00:00 2001 From: Daniel Karaj Date: Mon, 4 Nov 2024 14:02:46 +0000 Subject: [PATCH 1/7] Add summary card for facets Display facets on the finder admin page. --- app/views/admin/_facets_summary_card.html.erb | 19 +++++++++++++++++++ .../admin/_metadata_summary_card.html.erb | 1 + app/views/admin/summary.erb | 10 ++++++++++ .../editing_the_cma_case_finder_spec.rb | 8 ++++++-- 4 files changed, 36 insertions(+), 2 deletions(-) create mode 100644 app/views/admin/_facets_summary_card.html.erb diff --git a/app/views/admin/_facets_summary_card.html.erb b/app/views/admin/_facets_summary_card.html.erb new file mode 100644 index 000000000..5cd5f9e88 --- /dev/null +++ b/app/views/admin/_facets_summary_card.html.erb @@ -0,0 +1,19 @@ +<% + summary_card_actions ||= [] + + summary_rows = schema.facets.map do |facet| + { + key: facet["name"], + value: facet["allowed_values"] ? + facet["allowed_values"].first(3).map { |value| value["label"] }.join(", ") + (facet["allowed_values"].length > 3 ? "…" : "") + : facet["type"].humanize, + } + end +%> +<%= render "govuk_publishing_components/components/summary_card", { + id: "facets_summary_card", + title: "Filters and options", + rows: summary_rows, + summary_card_actions:, + margin_top: 6, +} %> diff --git a/app/views/admin/_metadata_summary_card.html.erb b/app/views/admin/_metadata_summary_card.html.erb index 94057c043..fd01a6021 100644 --- a/app/views/admin/_metadata_summary_card.html.erb +++ b/app/views/admin/_metadata_summary_card.html.erb @@ -14,6 +14,7 @@ <% end %> <%= render "govuk_publishing_components/components/summary_card", { + id: "metadata_summary_card", title: "Finder details", rows: [ ({ diff --git a/app/views/admin/summary.erb b/app/views/admin/summary.erb index c62001c34..1eb714e91 100644 --- a/app/views/admin/summary.erb +++ b/app/views/admin/summary.erb @@ -67,5 +67,15 @@ }, ], } %> + + <%= render "facets_summary_card", { + schema: current_format.finder_schema, + summary_card_actions: [ + { + label: "Request changes", + href: "/admin/facets/#{current_format.admin_slug}" + }, + ], + } %> diff --git a/spec/features/editing_the_cma_case_finder_spec.rb b/spec/features/editing_the_cma_case_finder_spec.rb index 1989d945d..ae0c0b8b4 100644 --- a/spec/features/editing_the_cma_case_finder_spec.rb +++ b/spec/features/editing_the_cma_case_finder_spec.rb @@ -19,7 +19,9 @@ scenario "changing all fields" do visit "admin/cma-cases" - click_link "Request changes" + within "#metadata_summary_card" do + click_link "Request changes" + end expect(page).to have_selector("span", text: "CMA Case finder") expect(page).to have_selector("h1", text: "Request change: Finder details") @@ -53,7 +55,9 @@ scenario "deleting all fields" do visit "admin/cma-cases" - click_link "Request changes" + within "#metadata_summary_card" do + click_link "Request changes" + end fill_in "name", with: "" fill_in "base_path", with: "" From 51d2fcd2bc3fc972f8627d8aba1f4930982e0ef0 Mon Sep 17 00:00:00 2001 From: Daniel Karaj Date: Mon, 4 Nov 2024 14:03:22 +0000 Subject: [PATCH 2/7] Add page to edit facets Introduce route and form to modify the facets for a finder. We did attempt to have a nice "conditional reveal" on the filter options for multi/single select, but that doesn't play nicely with the "Add another" component, so we had to revert it. The code where we tried it out is in commit: fcd2e8f45161af9e21618e14f169c906c283b9d8 The other area where the form differs from the design is in the handling of the 'keys'/'values' (as opposed to 'labels') of the filter options. The result is the least worst solution to the problem of: "how do we retain the original filter option key for _existing_ filter options?". The design called for a simple textarea comma-separated list of filter option _labels_ (i.e. "Commercial - fixed wing"), with nowhere to specify the _key_ / _value_ ("commercial-fixed-wing"). Whilst in that example we can easily convert the label into the key, there are plenty of examples of existing filter options where the key is markedly different from the label, and thus submitting the form would be "lossy" and lead to a larger and unwanted diff. (Moreover it could lead to real problems if a dev were to go ahead and merge the diff). I did explore exposing the key / value pairs in dedicated input fields, and in theory could then turn the key into an `input type=hidden` so that it's not exposed to the user. This was explored in e8810b6a2dc77dbf685aff65bf92f7b11fe6bb36 but the component doesn't currently allow nesting, so this isn't a viable way forward for now. What I've arrived at is I think a good compromise: it is the label that appears first (followed by the key), and there is no expectation for the user to edit the key nor to provide a key for any new filter options they add. The controller automatically defaults to a kebab-cased key derived from the label, if one is not provided up front. (Side note: I couldn't find a Rails native kebabcase method, to my surprise, so took the regex from https://jonathanyeong.com/changing-to-kebabcase/). --- app/assets/javascripts/application.js | 1 + app/assets/stylesheets/application.scss | 2 + .../stylesheets/components/_add-another.scss | 4 + app/controllers/admin_controller.rb | 2 + app/policies/document_policy.rb | 1 + app/views/admin/_facet_fields.html.erb | 110 ++++++++++++++++++ app/views/admin/edit_facets.html.erb | 53 +++++++++ config/routes.rb | 1 + spec/controllers/admin_controller_spec.rb | 8 ++ 9 files changed, 182 insertions(+) create mode 100644 app/assets/stylesheets/components/_add-another.scss create mode 100644 app/views/admin/_facet_fields.html.erb create mode 100644 app/views/admin/edit_facets.html.erb diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js index 6d55f303c..587c471cc 100644 --- a/app/assets/javascripts/application.js +++ b/app/assets/javascripts/application.js @@ -1,5 +1,6 @@ //= require govuk_publishing_components/dependencies //= require govuk_publishing_components/lib +//= require govuk_publishing_components/components/add-another //= require govuk_publishing_components/components/copy-to-clipboard //= require components/autocomplete diff --git a/app/assets/stylesheets/application.scss b/app/assets/stylesheets/application.scss index 2a83a6adc..4ceda2a1e 100644 --- a/app/assets/stylesheets/application.scss +++ b/app/assets/stylesheets/application.scss @@ -2,6 +2,7 @@ $govuk-page-width: 1140px; @import 'govuk_publishing_components/govuk_frontend_support'; @import 'govuk_publishing_components/component_support'; +@import 'govuk_publishing_components/components/add-another'; @import 'govuk_publishing_components/components/breadcrumbs'; @import 'govuk_publishing_components/components/button'; @import 'govuk_publishing_components/components/checkboxes'; @@ -24,4 +25,5 @@ $govuk-page-width: 1140px; @import 'govuk_publishing_components/components/textarea'; @import 'govuk_publishing_components/components/title'; +@import "./components/add-another"; @import "./components/autocomplete"; diff --git a/app/assets/stylesheets/components/_add-another.scss b/app/assets/stylesheets/components/_add-another.scss new file mode 100644 index 000000000..2b0c8657b --- /dev/null +++ b/app/assets/stylesheets/components/_add-another.scss @@ -0,0 +1,4 @@ +.js-add-another__fieldset { + background-color: govuk-colour("light-grey"); + padding: 1em; +} diff --git a/app/controllers/admin_controller.rb b/app/controllers/admin_controller.rb index dd9d972de..b3f467824 100644 --- a/app/controllers/admin_controller.rb +++ b/app/controllers/admin_controller.rb @@ -5,6 +5,8 @@ class AdminController < ApplicationController def summary; end + def edit_facets; end + def edit_metadata; end def confirm_metadata diff --git a/app/policies/document_policy.rb b/app/policies/document_policy.rb index 8734ed25b..d00ffec19 100644 --- a/app/policies/document_policy.rb +++ b/app/policies/document_policy.rb @@ -16,6 +16,7 @@ def can_request_edits_to_finder? publish? end alias_method :summary?, :can_request_edits_to_finder? + alias_method :edit_facets?, :can_request_edits_to_finder? alias_method :edit_metadata?, :can_request_edits_to_finder? alias_method :confirm_metadata?, :can_request_edits_to_finder? alias_method :zendesk?, :can_request_edits_to_finder? diff --git a/app/views/admin/_facet_fields.html.erb b/app/views/admin/_facet_fields.html.erb new file mode 100644 index 000000000..8d2b3717a --- /dev/null +++ b/app/views/admin/_facet_fields.html.erb @@ -0,0 +1,110 @@ +<% facet ||= {} %> +"> + +<%= render "govuk_publishing_components/components/input", { + label: { + text: "Filter", + heading_size: "s", + }, + name: "facets[#{index}][name]", + value: facet["name"], + hint: "Example: Disease control zone type" +} %> + +<%= render "govuk_publishing_components/components/input", { + label: { + text: "Filter short name (optional)", + heading_size: "s", + }, + name: "facets[#{index}][short_name]", + value: facet["short_name"], + hint: "Example: Control zone type" +} %> + +<%= render "govuk_publishing_components/components/select", { + id: "facets-#{index}-type", + name: "facets[#{index}][type]", + label: "Type", + options: [ + { + text: "Multiple select", + value: "enum_text_multiple", # Temporary value for the form only. It ends up as "text" in the schema, and "allowed_values" is retained + selected: facet.dig("specialist_publisher_properties", "select") == "multiple", + }, + { + text: "One option", + value: "enum_text_single", # Temporary value for the form only. It ends up as "text" in the schema, and "allowed_values" is retained + selected: facet.dig("specialist_publisher_properties", "select") == "one", + }, + { + text: "Free text", + value: "text", # Ends up as "text" in the schema, and deletes any submitted "allowed_values" value + selected: facet["type"] == "text" && facet["allowed_values"].nil?, + }, + { + text: "Date", + value: "date", + selected: facet["type"] == "date", + } + ] +} %> + +<%= render "govuk_publishing_components/components/textarea", { + label: { + text: "Filter options ('Multiple select' or 'One option' only)", + heading_size: "s", + }, + hint: sanitize("Put each option on a new line. The underlying name for existing (live) options will appear in curly braces: please don't edit these, and don't add them for new options (they'll be created automatically later on in the process). Example:
Pre-existing value {pre-existing-value}
New value"), + name: "facets[#{index}][allowed_values]", + rows: 5, + value: facet["allowed_values"]&.map { |opt| "#{opt["label"]} {#{opt["value"]}}" }&.join("\n"), +} %> + +<%= render "govuk_publishing_components/components/radio", { + heading: "Can users use this filter when searching for content items?", + heading_size: "s", + name: "facets[#{index}][filterable]", + inline: true, + items: [ + { + value: "true", + text: "Yes", + checked: facet["filterable"] + }, + { + value: "false", + text: "No", + checked: !facet["filterable"] + } + ] +} %> + +<%= render "govuk_publishing_components/components/radio", { + heading: "Show as information under content item?", + heading_size: "s", + hint: "Example: Air Accidents Investigation Branch reports displays ‘Aircraft category: Commercial - fixed wing’ under relevant content items", + name: "facets[#{index}][display_as_result_metadata]", + inline: true, + items: [ + { + value: "true", + text: "Yes", + checked: facet["display_as_result_metadata"] + }, + { + value: "false", + text: "No", + checked: !facet["display_as_result_metadata"] + } + ] +} %> + +<%= render "govuk_publishing_components/components/input", { + label: { + text: "Preposition (to be displayed when filter option is selected)", + heading_size: "s", + }, + name: "facets[#{index}][preposition]", + value: facet["preposition"], + hint: "Example: In Find funding for land or farms, selecting an option in the filter ‘Area of interest’ displays the preposition ‘With’ before the selected option" +} %> diff --git a/app/views/admin/edit_facets.html.erb b/app/views/admin/edit_facets.html.erb new file mode 100644 index 000000000..ec36aabfe --- /dev/null +++ b/app/views/admin/edit_facets.html.erb @@ -0,0 +1,53 @@ +<% content_for :breadcrumbs do %> + <%= render "govuk_publishing_components/components/breadcrumbs", { + collapse_on_mobile: true, + breadcrumbs: [ + { + title: "All finders", + url: root_path + }, + { + title: "#{current_format.title} finder", + url: "/admin/#{current_format.admin_slug}" + }, + { + title: "Request change", + url: request.original_url + } + ] + } %> +<% end %> +<% content_for :page_title, "Edit #{current_format.title} finder" %> +<% content_for :title, "Request change: Filters and options" %> +<% content_for :context, "#{current_format.title} finder" %> + + +
+
+ <%= form_tag do %> +

+ Filters and its options are to help users find desired content on a specialist finder. These are selected by you for each specialist content item created in the Specialist Publisher. These are shown in the blue box on the documents and elsewhere. +

+ + <%= render "govuk_publishing_components/components/add_another", { + fieldset_legend: "Filter", + add_button_text: "Add another filter", + items: current_format.finder_schema.facets.each_with_index.map do |facet, index| + { + fields: render(partial: "facet_fields", locals: { facet:, index: }), + destroy_checkbox: render("govuk_publishing_components/components/checkboxes", { name: "facets[#{index}][_destroy]", items: [{label: "Delete", value: "1" }]}) + } + end, + empty: render(partial: "facet_fields", locals: { index: current_format.finder_schema.facets.length }), + } %> + +
+ <%= render "govuk_publishing_components/components/button", { + text: "Submit changes", + } %> + + <%= link_to("Cancel", "/admin/#{current_format.admin_slug}", class: "govuk-link govuk-link--no-visited-state") %> +
+ <% end %> +
+
diff --git a/config/routes.rb b/config/routes.rb index 58da14cee..23d16d51b 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -16,6 +16,7 @@ resources :document_list_export_request, path: "/export/:document_type_slug", param: :export_id, only: [:show] get "/admin/:document_type_slug", to: "admin#summary" + get "/admin/facets/:document_type_slug", to: "admin#edit_facets" get "/admin/metadata/:document_type_slug", to: "admin#edit_metadata" post "/admin/metadata/:document_type_slug", to: "admin#confirm_metadata" post "/admin/zendesk/:document_type_slug", to: "admin#zendesk" diff --git a/spec/controllers/admin_controller_spec.rb b/spec/controllers/admin_controller_spec.rb index f9a2f0486..0acb6dbec 100644 --- a/spec/controllers/admin_controller_spec.rb +++ b/spec/controllers/admin_controller_spec.rb @@ -28,6 +28,14 @@ end end + describe "GET edit facets" do + it "responds successfully" do + stub_publishing_api_has_content([], hash_including(document_type: Organisation.document_type)) + get :edit_facets, params: { document_type_slug: "asylum-support-decisions" } + expect(response.status).to eq(200) + end + end + describe "POST edit metadata" do it "responds successfully" do stub_publishing_api_has_content([], hash_including(document_type: Organisation.document_type)) From 2a6eb9b9d4814da57dd9263c2d7df1bc9425b4be Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Fri, 3 Jan 2025 09:13:34 +0000 Subject: [PATCH 3/7] Add Diffy dependency and styles This uses the same dependency, and [copies the same CSS](https://github.com/alphagov/travel-advice-publisher/blob/767e4308ff2be37ca3497efd43eeb2105699561a/app/assets/stylesheets/diff.scss), as is used in Travel Advice Publisher. This will be used on the form confirmation screen in one of the subsequent commits. --- Gemfile | 1 + Gemfile.lock | 2 + app/assets/stylesheets/application.scss | 1 + app/assets/stylesheets/components/_diff.scss | 83 ++++++++++++++++++++ 4 files changed, 87 insertions(+) create mode 100644 app/assets/stylesheets/components/_diff.scss diff --git a/Gemfile b/Gemfile index 1653684b3..f3e212a76 100644 --- a/Gemfile +++ b/Gemfile @@ -6,6 +6,7 @@ gem "aws-sdk-s3" gem "bootsnap", require: false gem "bootstrap-kaminari-views" gem "dartsass-rails" +gem "diffy" gem "gds-api-adapters" gem "gds-sso" gem "govspeak" diff --git a/Gemfile.lock b/Gemfile.lock index bc16a054e..bdef66ccb 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -144,6 +144,7 @@ GEM date (3.4.1) debug_inspector (1.2.0) diff-lcs (1.5.1) + diffy (3.4.3) docile (1.4.0) domain_name (0.6.20240107) drb (2.2.1) @@ -811,6 +812,7 @@ DEPENDENCIES capybara-select-2 dartsass-rails database_cleaner-mongoid + diffy factory_bot gds-api-adapters gds-sso diff --git a/app/assets/stylesheets/application.scss b/app/assets/stylesheets/application.scss index 4ceda2a1e..5d0935e21 100644 --- a/app/assets/stylesheets/application.scss +++ b/app/assets/stylesheets/application.scss @@ -27,3 +27,4 @@ $govuk-page-width: 1140px; @import "./components/add-another"; @import "./components/autocomplete"; +@import "./components/diff"; diff --git a/app/assets/stylesheets/components/_diff.scss b/app/assets/stylesheets/components/_diff.scss new file mode 100644 index 000000000..eab6079f8 --- /dev/null +++ b/app/assets/stylesheets/components/_diff.scss @@ -0,0 +1,83 @@ +// stylelint-disable max-nesting-depth + +// Diff of two editions + +$added-color: #ddffdd; +$strong-added-color: #77f177; +$removed-color: #ffdddd; +$strong-removed-color: #ffaaaa; +$gray-lighter: govuk-colour("light-grey"); +$state-danger-text: govuk-colour("red"); +$state-success-text: govuk-colour("green"); + +.diff { + border: 1px solid $gray-lighter; + border-left: 40px solid $gray-lighter; + border-radius: 3px; + padding: 15px; + + ul { + padding-left: 0; + + li { + min-height: 24px; + margin: 0 -15px; + padding: 0 15px; + word-wrap: break-word; + list-style: none; + position: relative; + + del, + ins { + text-decoration: none; + } + } + + .del, + .ins { + padding-top: 2px; + } + + .del { + background-color: $removed-color; + + strong { + font-weight: normal; + background-color: $strong-removed-color; + } + } + + .ins { + background-color: $added-color; + + strong { + font-weight: normal; + background-color: $strong-added-color; + } + } + + .del::before, + .ins::before { + position: absolute; + font-weight: bold; + margin-left: -55px; + width: 40px; + text-align: center; + min-height: 24px; + top: 0; + bottom: 0; + } + + .del::before { + color: $state-danger-text; + background-color: $removed-color; + content: "-"; + } + + .ins::before { + color: $state-success-text; + background-color: $added-color; + content: "+"; + } + } +} From cb08bdeca06839f7674cd950c6ba1b08ead2cdd5 Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Fri, 3 Jan 2025 14:35:16 +0000 Subject: [PATCH 4/7] Added Facet model to keep logic outside of AdminController Following the same pattern as the EmailAlert model introduced in ae63aa74d931388dd8b874f4461d18c977ff2c61, we've added a Facet model which provides a translation layer between the form inputs we expect to add in subsequent commits, and the eventual representation of the facet in the finder schema JSON. This logic started off life inline in the AdminController, which added bloat and complexity to the controller and would have been extremely difficult to test. The logic is now instead encapsulated in "Facet" and has an accompanying test suite, so we can be confident that form inputs are being correctly converted to their finder schema JSON form. See next commit for where this class is actually used. --- app/models/facet.rb | 92 +++++++++++++++ spec/models/facet_spec.rb | 231 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 323 insertions(+) create mode 100644 app/models/facet.rb create mode 100644 spec/models/facet_spec.rb diff --git a/app/models/facet.rb b/app/models/facet.rb new file mode 100644 index 000000000..f9b929580 --- /dev/null +++ b/app/models/facet.rb @@ -0,0 +1,92 @@ +class Facet + include ActiveModel::Model + + attr_accessor( + :key, + :name, + :short_name, + :type, + :preposition, + :display_as_result_metadata, + :filterable, + :allowed_values, + :specialist_publisher_properties, + ) + + def to_finder_schema_attributes + { + key:, + name:, + short_name:, + type:, + preposition:, + display_as_result_metadata:, + filterable:, + allowed_values:, + specialist_publisher_properties:, + }.compact + end + + class << self + def from_finder_admin_form_params(params) + facet = new + facet.key = facet_key(params["key"], params["name"]) + facet.name = params["name"] + facet.short_name = nil_if_blank(params["short_name"]) + facet.type = facet_type(params["type"]) + facet.preposition = nil_if_blank(params["preposition"]) + facet.display_as_result_metadata = str_to_bool(params["display_as_result_metadata"]) + facet.filterable = str_to_bool(params["filterable"]) + facet.allowed_values = facet_allowed_values(params["allowed_values"], params["type"]) + facet.specialist_publisher_properties = facet_specialist_publisher_properties(params["type"]) + facet + end + + private + + def facet_key(key, name) + key.presence || name&.gsub(" ", "")&.underscore + end + + def nil_if_blank(str) + str.presence + end + + def str_to_bool(str) + if str == "true" + true + elsif str == "false" + false + end + end + + def facet_type(type) + facet_types_that_allow_enum_values.include?(type) ? "text" : type + end + + def facet_allowed_values(values, type) + return nil if values.nil? || facet_types_that_allow_enum_values.exclude?(type) + + values.split("\n").map do |str| + label = str.match(/^(.+){/) + label = label.nil? ? str.strip : label[1].strip + value = str.match(/{(.+)}/) + value = value.nil? ? str.strip.downcase.gsub(/[^\w\d\s]/, "").gsub(/\s/u, "-") : value[1].strip + { label:, value: } + end + end + + def facet_specialist_publisher_properties(type) + case type + when "enum_text_multiple" + { select: "multiple" } + when "enum_text_single" + { select: "one" } + end + end + + def facet_types_that_allow_enum_values + %w[enum_text_multiple enum_text_single] + end + end +end diff --git a/spec/models/facet_spec.rb b/spec/models/facet_spec.rb new file mode 100644 index 000000000..0cedfa550 --- /dev/null +++ b/spec/models/facet_spec.rb @@ -0,0 +1,231 @@ +require "spec_helper" + +RSpec.describe "Facet" do + describe "#from_finder_admin_form_params" do + it "builds a facet for a finder schema" do + params = { + "key" => "foo", + "name" => "Foo", + "short_name" => "f", + "type" => "text", + "preposition" => "sector", + "display_as_result_metadata" => "false", + "filterable" => "true", + "allowed_values" => "existing value {existing-value}\nnew value", + } + facet = Facet.from_finder_admin_form_params(params) + expect(facet.key).to eq("foo") + expect(facet.name).to eq("Foo") + expect(facet.short_name).to eq("f") + expect(facet.type).to eq("text") + expect(facet.preposition).to eq("sector") + expect(facet.display_as_result_metadata).to eq(false) + expect(facet.filterable).to eq(true) + end + + it "derives the key from the name if no key provided" do + params = { "name" => "Foo Bar Baz" } + facet = Facet.from_finder_admin_form_params(params) + expect(facet.key).to eq("foo_bar_baz") + end + + it "returns nil for 'short_name' if not provided/blank" do + facet = Facet.from_finder_admin_form_params({ "short_name" => "" }) + expect(facet.short_name).to eq(nil) + end + + it "returns nil for 'preposition' if not provided/blank" do + facet = Facet.from_finder_admin_form_params({ "preposition" => "" }) + expect(facet.preposition).to eq(nil) + end + + describe "inferring the boolean value, or absence of a value, for 'display_as_result_metadata' and 'filterable'" do + it "converts 'true' to true for 'display_as_result_metadata'" do + facet = Facet.from_finder_admin_form_params({ "display_as_result_metadata" => "true" }) + expect(facet.display_as_result_metadata).to eq(true) + end + + it "converts 'false' to false for 'display_as_result_metadata'" do + facet = Facet.from_finder_admin_form_params({ "display_as_result_metadata" => "false" }) + expect(facet.display_as_result_metadata).to eq(false) + end + + it "sets 'display_as_result_metadata' to 'nil' for any other values" do + facet = Facet.from_finder_admin_form_params({ "display_as_result_metadata" => "" }) + expect(facet.display_as_result_metadata).to eq(nil) + end + + it "converts 'true' to true for 'filterable'" do + facet = Facet.from_finder_admin_form_params({ "filterable" => "true" }) + expect(facet.filterable).to eq(true) + end + + it "converts 'false' to false for 'filterable'" do + facet = Facet.from_finder_admin_form_params({ "filterable" => "false" }) + expect(facet.filterable).to eq(false) + end + + it "sets 'filterable' to 'nil' for any other values" do + facet = Facet.from_finder_admin_form_params({ "filterable" => "" }) + expect(facet.filterable).to eq(nil) + end + end + + describe "constructing the allowed_values" do + it "uses any pre-supplied keys in curly brackets" do + params = { "type" => "enum_text_multiple", "allowed_values" => "Foo {food}\nBart {bar}" } + facet = Facet.from_finder_admin_form_params(params) + expect(facet.allowed_values).to eq([ + { label: "Foo", value: "food" }, + { label: "Bart", value: "bar" }, + ]) + end + + it "also works for the enum_text_single 'type'" do + params = { "type" => "enum_text_single", "allowed_values" => "Foo {food}\nBart {bar}" } + facet = Facet.from_finder_admin_form_params(params) + expect(facet.allowed_values).to eq([ + { label: "Foo", value: "food" }, + { label: "Bart", value: "bar" }, + ]) + end + + it "derives the key, in kebab-case, from the label if no key provided" do + params = { "type" => "enum_text_multiple", "allowed_values" => "Foo\nSome Long Name" } + facet = Facet.from_finder_admin_form_params(params) + expect(facet.allowed_values).to eq([ + { label: "Foo", value: "foo" }, + { label: "Some Long Name", value: "some-long-name" }, + ]) + end + + it "strips any extraneous spaces" do + params = { "type" => "enum_text_multiple", "allowed_values" => "Foo { bar }" } + facet = Facet.from_finder_admin_form_params(params) + expect(facet.allowed_values).to eq([ + { label: "Foo", value: "bar" }, + ]) + end + + it "returns empty array if no values provided" do + params = { "type" => "enum_text_multiple", "allowed_values" => "" } + facet = Facet.from_finder_admin_form_params(params) + expect(facet.allowed_values).to eq([]) + end + + it "returns nil if not of an enum type, even if allowed_values have been specified" do + params = { "type" => "text", "allowed_values" => "foo {foo}\nbar" } + facet = Facet.from_finder_admin_form_params(params) + expect(facet.allowed_values).to eq(nil) + + params = { "type" => "date", "allowed_values" => "foo {foo}\nbar" } + facet = Facet.from_finder_admin_form_params(params) + expect(facet.allowed_values).to eq(nil) + end + end + + describe "converting the facet 'type' and setting the corresponding specialist_publisher_properties" do + it "converts 'enum_text_multiple' type to 'text', and sets specialist_publisher_properties to select multiple" do + params = { "type" => "enum_text_multiple" } + facet = Facet.from_finder_admin_form_params(params) + expect(facet.type).to eq("text") + expect(facet.specialist_publisher_properties).to eq({ select: "multiple" }) + end + + it "converts 'enum_text_single' type to 'text', and sets specialist_publisher_properties to select one" do + params = { "type" => "enum_text_single" } + facet = Facet.from_finder_admin_form_params(params) + expect(facet.type).to eq("text") + expect(facet.specialist_publisher_properties).to eq({ select: "one" }) + end + + it "leaves type 'text' as-is, and sets specialist_publisher_properties to nil" do + params = { "type" => "text" } + facet = Facet.from_finder_admin_form_params(params) + expect(facet.type).to eq("text") + expect(facet.specialist_publisher_properties).to eq(nil) + end + + it "leaves type 'date' as-is, and sets specialist_publisher_properties to nil" do + params = { "type" => "date" } + facet = Facet.from_finder_admin_form_params(params) + expect(facet.type).to eq("date") + expect(facet.specialist_publisher_properties).to eq(nil) + end + end + end + + describe "#to_finder_schema_attributes" do + it "transforms the attributes of the facet to a hash of finder schema attributes" do + facet = Facet.new + facet.key = "foo" + facet.name = "Foo" + facet.short_name = "f" + facet.type = "text" + facet.preposition = "sector" + facet.display_as_result_metadata = false + facet.filterable = true + facet.allowed_values = [ + { label: "existing value", value: "existing-value" }, + { label: "new value", value: "new-value" }, + ] + facet.specialist_publisher_properties = { select: "one" } + + expect(facet.to_finder_schema_attributes).to eq({ + key: "foo", + name: "Foo", + short_name: "f", + type: "text", + preposition: "sector", + display_as_result_metadata: false, + filterable: true, + allowed_values: [ + { label: "existing value", value: "existing-value" }, + { label: "new value", value: "new-value" }, + ], + specialist_publisher_properties: { select: "one" }, + }) + end + + it "omits any nil values from the hash" do + facet = Facet.new + facet.key = "foo" + facet.name = "Foo" + facet.type = "text" + + expect(facet.to_finder_schema_attributes).to eq({ + key: "foo", + name: "Foo", + type: "text", + }) + end + + it "returns the hash keys in a specific order (to minimise the eventual diff)" do + facet = Facet.new + facet.key = "foo" + facet.name = "Foo" + facet.short_name = "f" + facet.type = "text" + facet.preposition = "sector" + facet.display_as_result_metadata = false + facet.filterable = true + facet.allowed_values = [ + { label: "existing value", value: "existing-value" }, + { label: "new value", value: "new-value" }, + ] + facet.specialist_publisher_properties = { select: "one" } + + expect(facet.to_finder_schema_attributes.keys).to eq(%i[ + key + name + short_name + type + preposition + display_as_result_metadata + filterable + allowed_values + specialist_publisher_properties + ]) + end + end +end From c142f2dc59e3192d3f9d145b48f375e7211c3764 Mon Sep 17 00:00:00 2001 From: Daniel Karaj Date: Mon, 4 Nov 2024 15:40:59 +0000 Subject: [PATCH 5/7] Add confirmation page for finder facets Introduces page to confirm changes to the facets of a finder: - Hides diff / generated schema in a details component. - Have a shortened summary, don't bother attempting to summarise for humans, they can always click on view diff if needed. - Minimises the diff by sorting the properties as per the most common ordering in the existing JSON files (we don't want any diffs if someone submits without making any changes!) - Has summary list facets in the order they're defined (with the exception of deleted facets, which are a bit fiddly to slot into the correct spot. Jotting them onto the end is sufficient). --- app/controllers/admin_controller.rb | 32 +++++++++ app/policies/document_policy.rb | 1 + app/views/admin/_facets_summary_card.html.erb | 32 +++++++++ app/views/admin/confirm_facets.html.erb | 67 +++++++++++++++++++ config/routes.rb | 1 + spec/controllers/admin_controller_spec.rb | 8 +++ 6 files changed, 141 insertions(+) create mode 100644 app/views/admin/confirm_facets.html.erb diff --git a/app/controllers/admin_controller.rb b/app/controllers/admin_controller.rb index b3f467824..899211ee1 100644 --- a/app/controllers/admin_controller.rb +++ b/app/controllers/admin_controller.rb @@ -9,6 +9,21 @@ def edit_facets; end def edit_metadata; end + def confirm_facets + @params = facets_params + @params["facets"] = @params["facets"].values.map { |facet_params| + next if facet_params["_destroy"] == "1" + + Facet.from_finder_admin_form_params(facet_params) + .to_finder_schema_attributes + }.compact + + @proposed_schema = FinderSchema.new(@current_format.finder_schema.attributes) + @proposed_schema.update(@params) + + render :confirm_facets + end + def confirm_metadata @params = params.permit( :name, @@ -77,4 +92,21 @@ def email_alert_params :signup_link, ) end + + def facets_params + allowed_facet_params = %i[ + key + name + short_name + type + preposition + display_as_result_metadata + filterable + allowed_values + _destroy + ] + params.permit( + facets: allowed_facet_params, + ) + end end diff --git a/app/policies/document_policy.rb b/app/policies/document_policy.rb index d00ffec19..eaedcf15f 100644 --- a/app/policies/document_policy.rb +++ b/app/policies/document_policy.rb @@ -17,6 +17,7 @@ def can_request_edits_to_finder? end alias_method :summary?, :can_request_edits_to_finder? alias_method :edit_facets?, :can_request_edits_to_finder? + alias_method :confirm_facets?, :can_request_edits_to_finder? alias_method :edit_metadata?, :can_request_edits_to_finder? alias_method :confirm_metadata?, :can_request_edits_to_finder? alias_method :zendesk?, :can_request_edits_to_finder? diff --git a/app/views/admin/_facets_summary_card.html.erb b/app/views/admin/_facets_summary_card.html.erb index 5cd5f9e88..35d6a74a8 100644 --- a/app/views/admin/_facets_summary_card.html.erb +++ b/app/views/admin/_facets_summary_card.html.erb @@ -1,6 +1,7 @@ <% summary_card_actions ||= [] + # Value for the summary page, where we only want to summarise the current facets summary_rows = schema.facets.map do |facet| { key: facet["name"], @@ -9,6 +10,37 @@ : facet["type"].humanize, } end + + if (defined? previous_schema) + # We're on the edit page, so want to summarise the changes. So overriding `summary_rows`. + removed_facets = previous_schema.facets.map { |facet| facet["name"] } - schema.facets.map { |facet| facet["name"] } + previous_schema_facet_names = previous_schema.facets.map { |facet| facet["name"] } + proposed_schema_facet_names = schema.facets.map { |facet| facet["name"] } + + summary_rows = proposed_schema_facet_names.map do |facet_name| + if (previous_schema_facet_names.include?(facet_name)) + old_facet_config = previous_schema.facets.find { |f| f["name"] == facet_name } + new_facet_config = schema.facets.find { |f| f["name"] == facet_name } + updated = !facet_name.in?(removed_facets) && !old_facet_config.nil? && old_facet_config != new_facet_config + + { + key: facet_name, + value: raw("Updated (click on 'View diff' for details)"), + } if updated + else + { + key: facet_name, + value: raw("Added (click on 'View diff' for details)"), + } + end + end + summary_rows = summary_rows.compact.concat(removed_facets.map do |facet_name| + { + key: facet_name, + value: raw("Deleted"), + } + end) + end %> <%= render "govuk_publishing_components/components/summary_card", { id: "facets_summary_card", diff --git a/app/views/admin/confirm_facets.html.erb b/app/views/admin/confirm_facets.html.erb new file mode 100644 index 000000000..72da6c0c6 --- /dev/null +++ b/app/views/admin/confirm_facets.html.erb @@ -0,0 +1,67 @@ +<% content_for :breadcrumbs do %> + <%= render "govuk_publishing_components/components/breadcrumbs", { + collapse_on_mobile: true, + breadcrumbs: [ + { + title: "All finders", + url: root_path + }, + { + title: "#{current_format.title} finder", + url: "/admin/#{current_format.admin_slug}" + }, + { + title: "Request change", + url: "/admin/facets/#{current_format.admin_slug}" + }, + { + title: "Check changes", + url: request.original_url + } + ] + } %> +<% end %> +<% content_for :page_title, "Edit #{current_format.title} finder" %> +<% content_for :title, "Check your changes before submitting" %> +<% content_for :context, "#{current_format.title} finder" %> + + +
+
+ <%= render "facets_summary_card", { + schema: @proposed_schema, + previous_schema: @current_format.finder_schema, + } %> + +
+ View diff + <%= + Diffy::Diff.new( + JSON.pretty_generate(@current_format.finder_schema.as_json).to_s, + JSON.pretty_generate(@proposed_schema.as_json).to_s, + allow_empty_diff: false, + ).to_s(:html).html_safe + %> +
+ +
+ View generated schema + 
<%= JSON.pretty_generate(@proposed_schema.as_json) %>
+
+ +

+ By submitting you are confirming that these changes are required to the specialist finder. +

+ +
+ <%= form_tag "/admin/zendesk/#{current_format.admin_slug}", method: 'post' do %> + <%= hidden_field_tag :proposed_schema, JSON.pretty_generate(@proposed_schema) %> + <%= render "govuk_publishing_components/components/button", { + text: "Submit changes" + } %> + <% end %> + + <%= link_to("Cancel", "/admin/#{current_format.admin_slug}", class: "govuk-link govuk-link--no-visited-state") %> +
+
+
diff --git a/config/routes.rb b/config/routes.rb index 23d16d51b..523ad179e 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -17,6 +17,7 @@ get "/admin/:document_type_slug", to: "admin#summary" get "/admin/facets/:document_type_slug", to: "admin#edit_facets" + post "/admin/facets/:document_type_slug", to: "admin#confirm_facets" get "/admin/metadata/:document_type_slug", to: "admin#edit_metadata" post "/admin/metadata/:document_type_slug", to: "admin#confirm_metadata" post "/admin/zendesk/:document_type_slug", to: "admin#zendesk" diff --git a/spec/controllers/admin_controller_spec.rb b/spec/controllers/admin_controller_spec.rb index 0acb6dbec..1bdd40c46 100644 --- a/spec/controllers/admin_controller_spec.rb +++ b/spec/controllers/admin_controller_spec.rb @@ -44,6 +44,14 @@ end end + describe "POST edit facets" do + it "responds successfully" do + stub_publishing_api_has_content([], hash_including(document_type: Organisation.document_type)) + post :edit_facets, params: { document_type_slug: "asylum-support-decisions" } + expect(response.status).to eq(200) + end + end + describe "POST zendesk" do it "responds successfully, calling support api" do stub_post = stub_support_api_valid_raise_support_ticket(hash_including({ From 910c706cd86e797b088d0a0f2e6ad25c6304b7aa Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Fri, 3 Jan 2025 16:00:08 +0000 Subject: [PATCH 6/7] Add feature test for the filters and options form This integration test checks that the following things work: - Deleting a facet - Editing an existing facet - Adding a new facet ...and that a Zendesk ticket is created. --- ...g_the_cma_case_filters_and_options_spec.rb | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 spec/features/editing_the_cma_case_filters_and_options_spec.rb diff --git a/spec/features/editing_the_cma_case_filters_and_options_spec.rb b/spec/features/editing_the_cma_case_filters_and_options_spec.rb new file mode 100644 index 000000000..02b830a64 --- /dev/null +++ b/spec/features/editing_the_cma_case_filters_and_options_spec.rb @@ -0,0 +1,67 @@ +require "spec_helper" +require "gds_api/test_helpers/support_api" + +RSpec.feature "Editing the CMA case finder filters and options", type: :feature do + include GdsApi::TestHelpers::SupportApi + + let(:organisations) do + [ + { "content_id" => "957eb4ec-089b-4f71-ba2a-dc69ac8919ea", "title" => "Competition and Markets Authority" }, + ] + end + + before do + Capybara.current_driver = Capybara.javascript_driver + log_in_as_editor(:cma_editor) + stub_publishing_api_has_content([], hash_including(document_type: CmaCase.document_type)) + stub_publishing_api_has_content(organisations, hash_including(document_type: Organisation.document_type)) + stub_any_support_api_call + end + + scenario "editing, deleting and adding new filters" do + visit "admin/cma-cases" + within "#facets_summary_card" do + click_link "Request changes" + end + + expect(page).to have_selector("span", text: "CMA Case finder") + expect(page).to have_selector("h1", text: "Request change: Filters and options") + + # Delete all but the first filter + all(".js-add-another__remove-button").last.click while all(".js-add-another__remove-button").count > 1 + + # Edit one of the fields of the first filter + fill_in "facets[0][preposition]", with: "foo" + + # Create a new filter, using all of the inputs + click_button "Add another filter" + # The new facet inserts at "previous count -1 + 1 == 6", + # rather than "current total -1 + 1 == 1" as might be expected. + # This is because the previous facets are still there in the DOM, + # just hidden and with `_destroy=1` + inserted_facet = "facets[6]" + fill_in "#{inserted_facet}[name]", with: "New filter" + fill_in "#{inserted_facet}[short_name]", with: "short name" + select "One option", from: "#{inserted_facet}[type]" + fill_in "#{inserted_facet}[allowed_values]", with: "foo\nbar" + # TODO: unsure why the 'visible: false' is necessary below. The radio buttons should be (and are) visible. + choose "#{inserted_facet}[filterable]", option: "false", visible: false + choose "#{inserted_facet}[display_as_result_metadata]", option: "false", visible: false + fill_in "#{inserted_facet}[preposition]", with: "of type NEW" + + click_button "Submit changes" + + expect(page).to have_selector(".govuk-summary-list__row", text: "Case type Updated (click on 'View diff' for details)") + expect(page).to have_selector(".govuk-summary-list__row", text: "New filter Added (click on 'View diff' for details)") + expect(page).to have_selector(".govuk-summary-list__row", text: "Case state Deleted") + expect(page).to have_selector(".govuk-summary-list__row", text: "Market sector Deleted") + expect(page).to have_selector(".govuk-summary-list__row", text: "Outcome Deleted") + expect(page).to have_selector(".govuk-summary-list__row", text: "Opened Deleted") + expect(page).to have_selector(".govuk-summary-list__row", text: "Closed Deleted") + expect(page).to have_selector("details summary", text: "View diff") + expect(page).to have_selector("details summary", text: "View generated schema") + + click_button "Submit changes" + expect(page).to have_selector(".gem-c-success-alert__message", text: "Your changes have been submitted and Zendesk ticket created.") + end +end From 2901c3b7abc074c20d0a3e8b63c88a0cc46351b2 Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Mon, 6 Jan 2025 14:29:54 +0000 Subject: [PATCH 7/7] Use ActiveRecord to do the boolean casting As suggested in https://github.com/alphagov/specialist-publisher/pull/2929#discussion_r1901992443 Follows the same pattern as in FinderSchema --- app/models/facet.rb | 33 ++++++++++++--------------------- 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/app/models/facet.rb b/app/models/facet.rb index f9b929580..96dedbc00 100644 --- a/app/models/facet.rb +++ b/app/models/facet.rb @@ -1,17 +1,16 @@ class Facet include ActiveModel::Model + include ActiveModel::Attributes - attr_accessor( - :key, - :name, - :short_name, - :type, - :preposition, - :display_as_result_metadata, - :filterable, - :allowed_values, - :specialist_publisher_properties, - ) + attribute :key + attribute :name + attribute :short_name + attribute :type + attribute :preposition + attribute :display_as_result_metadata, :boolean + attribute :filterable, :boolean + attribute :allowed_values + attribute :specialist_publisher_properties def to_finder_schema_attributes { @@ -35,8 +34,8 @@ def from_finder_admin_form_params(params) facet.short_name = nil_if_blank(params["short_name"]) facet.type = facet_type(params["type"]) facet.preposition = nil_if_blank(params["preposition"]) - facet.display_as_result_metadata = str_to_bool(params["display_as_result_metadata"]) - facet.filterable = str_to_bool(params["filterable"]) + facet.display_as_result_metadata = params["display_as_result_metadata"] + facet.filterable = params["filterable"] facet.allowed_values = facet_allowed_values(params["allowed_values"], params["type"]) facet.specialist_publisher_properties = facet_specialist_publisher_properties(params["type"]) facet @@ -52,14 +51,6 @@ def nil_if_blank(str) str.presence end - def str_to_bool(str) - if str == "true" - true - elsif str == "false" - false - end - end - def facet_type(type) facet_types_that_allow_enum_values.include?(type) ? "text" : type end