From c3535d1f91e6cdbdf0879b6df2bc62f726d2cb2a Mon Sep 17 00:00:00 2001 From: Mark Taylor <138604938+mtaylorgds@users.noreply.github.com> Date: Tue, 9 Jul 2024 17:06:01 +0100 Subject: [PATCH 1/3] Rename RootController to LegacyRootController This work is part of upgrading Mainstream Publisher to use the GOV.UK Design System, in place of the current, and outdated, bootstrap UI. This commit renames the `RootController` class to `LegacyRootController`. The next commit will create a new `RootController` class, with a feature toggle to control which is used. Some tests, which were asserting that the "index" action of the "root" controller was being redirected to, have been updated to instead assert that the `root_path` has been redirected to so that they are agnostic to this change. Renaming the legacy controller now (rather than creating a new controller for the new version) will make cleanup at the end easier when we remove the feature toggle and all legacy root code. --- .../{root_controller.rb => legacy_root_controller.rb} | 2 +- app/views/{root => legacy_root}/_amends_needed.html.erb | 0 app/views/{root => legacy_root}/_archived.html.erb | 0 app/views/{root => legacy_root}/_drafts.html.erb | 0 .../{root => legacy_root}/_fact_check_received.html.erb | 0 app/views/{root => legacy_root}/_in_review.html.erb | 0 .../{root => legacy_root}/_out_for_fact_check.html.erb | 0 app/views/{root => legacy_root}/_publication.html.erb | 0 app/views/{root => legacy_root}/_published.html.erb | 0 app/views/{root => legacy_root}/_ready.html.erb | 0 app/views/{root => legacy_root}/_reviewer.html.erb | 0 .../_scheduled_for_publishing.html.erb | 0 app/views/{root => legacy_root}/index.html.erb | 0 config/routes.rb | 2 +- test/functional/artefacts_controller_test.rb | 2 +- test/functional/downtimes_controller_test.rb | 2 +- test/functional/editions_controller_test.rb | 8 ++++---- ..._controller_test.rb => legacy_root_controller_test.rb} | 2 +- 18 files changed, 9 insertions(+), 9 deletions(-) rename app/controllers/{root_controller.rb => legacy_root_controller.rb} (97%) rename app/views/{root => legacy_root}/_amends_needed.html.erb (100%) rename app/views/{root => legacy_root}/_archived.html.erb (100%) rename app/views/{root => legacy_root}/_drafts.html.erb (100%) rename app/views/{root => legacy_root}/_fact_check_received.html.erb (100%) rename app/views/{root => legacy_root}/_in_review.html.erb (100%) rename app/views/{root => legacy_root}/_out_for_fact_check.html.erb (100%) rename app/views/{root => legacy_root}/_publication.html.erb (100%) rename app/views/{root => legacy_root}/_published.html.erb (100%) rename app/views/{root => legacy_root}/_ready.html.erb (100%) rename app/views/{root => legacy_root}/_reviewer.html.erb (100%) rename app/views/{root => legacy_root}/_scheduled_for_publishing.html.erb (100%) rename app/views/{root => legacy_root}/index.html.erb (100%) rename test/functional/{root_controller_test.rb => legacy_root_controller_test.rb} (96%) diff --git a/app/controllers/root_controller.rb b/app/controllers/legacy_root_controller.rb similarity index 97% rename from app/controllers/root_controller.rb rename to app/controllers/legacy_root_controller.rb index 4a81efb1f..a32c43467 100644 --- a/app/controllers/root_controller.rb +++ b/app/controllers/legacy_root_controller.rb @@ -1,4 +1,4 @@ -class RootController < ApplicationController +class LegacyRootController < ApplicationController respond_to :html, :json include ColumnSortable diff --git a/app/views/root/_amends_needed.html.erb b/app/views/legacy_root/_amends_needed.html.erb similarity index 100% rename from app/views/root/_amends_needed.html.erb rename to app/views/legacy_root/_amends_needed.html.erb diff --git a/app/views/root/_archived.html.erb b/app/views/legacy_root/_archived.html.erb similarity index 100% rename from app/views/root/_archived.html.erb rename to app/views/legacy_root/_archived.html.erb diff --git a/app/views/root/_drafts.html.erb b/app/views/legacy_root/_drafts.html.erb similarity index 100% rename from app/views/root/_drafts.html.erb rename to app/views/legacy_root/_drafts.html.erb diff --git a/app/views/root/_fact_check_received.html.erb b/app/views/legacy_root/_fact_check_received.html.erb similarity index 100% rename from app/views/root/_fact_check_received.html.erb rename to app/views/legacy_root/_fact_check_received.html.erb diff --git a/app/views/root/_in_review.html.erb b/app/views/legacy_root/_in_review.html.erb similarity index 100% rename from app/views/root/_in_review.html.erb rename to app/views/legacy_root/_in_review.html.erb diff --git a/app/views/root/_out_for_fact_check.html.erb b/app/views/legacy_root/_out_for_fact_check.html.erb similarity index 100% rename from app/views/root/_out_for_fact_check.html.erb rename to app/views/legacy_root/_out_for_fact_check.html.erb diff --git a/app/views/root/_publication.html.erb b/app/views/legacy_root/_publication.html.erb similarity index 100% rename from app/views/root/_publication.html.erb rename to app/views/legacy_root/_publication.html.erb diff --git a/app/views/root/_published.html.erb b/app/views/legacy_root/_published.html.erb similarity index 100% rename from app/views/root/_published.html.erb rename to app/views/legacy_root/_published.html.erb diff --git a/app/views/root/_ready.html.erb b/app/views/legacy_root/_ready.html.erb similarity index 100% rename from app/views/root/_ready.html.erb rename to app/views/legacy_root/_ready.html.erb diff --git a/app/views/root/_reviewer.html.erb b/app/views/legacy_root/_reviewer.html.erb similarity index 100% rename from app/views/root/_reviewer.html.erb rename to app/views/legacy_root/_reviewer.html.erb diff --git a/app/views/root/_scheduled_for_publishing.html.erb b/app/views/legacy_root/_scheduled_for_publishing.html.erb similarity index 100% rename from app/views/root/_scheduled_for_publishing.html.erb rename to app/views/legacy_root/_scheduled_for_publishing.html.erb diff --git a/app/views/root/index.html.erb b/app/views/legacy_root/index.html.erb similarity index 100% rename from app/views/root/index.html.erb rename to app/views/legacy_root/index.html.erb diff --git a/config/routes.rb b/config/routes.rb index 769a9a3bc..3935485c8 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -63,7 +63,7 @@ get "api/lookup-by-base-path", to: "publishing_api_proxy#lookup_by_base_path" resources :publications - root to: "root#index" + root to: "legacy_root#index" # We used to nest all URLs under /admin so we now redirect that # in case people had bookmarks set up. Using a proc as otherwise the diff --git a/test/functional/artefacts_controller_test.rb b/test/functional/artefacts_controller_test.rb index d49244437..223f990c6 100644 --- a/test/functional/artefacts_controller_test.rb +++ b/test/functional/artefacts_controller_test.rb @@ -16,7 +16,7 @@ class ArtefactsControllerTest < ActionController::TestCase get :new assert_response :redirect - assert_redirected_to controller: "root", action: "index" + assert_redirected_to root_path assert_includes flash[:danger], "do not have permission" end end diff --git a/test/functional/downtimes_controller_test.rb b/test/functional/downtimes_controller_test.rb index 11b330757..f6cb48de7 100644 --- a/test/functional/downtimes_controller_test.rb +++ b/test/functional/downtimes_controller_test.rb @@ -160,7 +160,7 @@ class DowntimesControllerTest < ActionController::TestCase get :index assert_response :redirect - assert_redirected_to controller: "root", action: "index" + assert_redirected_to root_path assert_includes flash[:danger], "do not have permission" end end diff --git a/test/functional/editions_controller_test.rb b/test/functional/editions_controller_test.rb index a84bd5b6b..a3f4c61ae 100644 --- a/test/functional/editions_controller_test.rb +++ b/test/functional/editions_controller_test.rb @@ -954,7 +954,7 @@ class EditionsControllerTest < ActionController::TestCase assert_difference("TransactionEdition.count", -1) do delete :destroy, params: { id: @transaction.id } end - assert_redirected_to(:controller => "root", "action" => "index") + assert_redirected_to root_path end should "can't destroy published transaction" do @@ -973,7 +973,7 @@ class EditionsControllerTest < ActionController::TestCase assert_difference("GuideEdition.count", -1) do delete :destroy, params: { id: @guide.id } end - assert_redirected_to(:controller => "root", "action" => "index") + assert_redirected_to root_path end should "can't destroy published guide" do @@ -1010,7 +1010,7 @@ class EditionsControllerTest < ActionController::TestCase delete :destroy, params: { id: @welsh_guide.id } end - assert_redirected_to(:controller => "root", "action" => "index") + assert_redirected_to root_path assert_equal "Edition deleted", flash[:success] end end @@ -1020,7 +1020,7 @@ class EditionsControllerTest < ActionController::TestCase should "editions index redirects to root" do get :index assert_response :redirect - assert_redirected_to(:controller => "root", "action" => "index") + assert_redirected_to root_path end end diff --git a/test/functional/root_controller_test.rb b/test/functional/legacy_root_controller_test.rb similarity index 96% rename from test/functional/root_controller_test.rb rename to test/functional/legacy_root_controller_test.rb index 7742efc15..ed8515f59 100644 --- a/test/functional/root_controller_test.rb +++ b/test/functional/legacy_root_controller_test.rb @@ -1,6 +1,6 @@ require "test_helper" -class RootControllerTest < ActionController::TestCase +class LegacyRootControllerTest < ActionController::TestCase setup do @users = FactoryBot.create_list(:user, 3) login_as_stub_user From 1bf87a4e0a84bd8d1e25031b79ad2f04d8547a98 Mon Sep 17 00:00:00 2001 From: Mark Taylor <138604938+mtaylorgds@users.noreply.github.com> Date: Thu, 11 Jul 2024 14:00:12 +0100 Subject: [PATCH 2/3] Add a new RootController Create a new `RootController` class and conditionally route to it or the `LegacyRootController` based on whether the new "design_system_publications_filter" feature is enabled (it defaults to disabled). In this commit `RootController` is an exact duplicate of `LegacyRootController`, but future commits will update this so that it uses the new, GOV.UK Design System-based, design. --- app/controllers/root_controller.rb | 72 +++++++++++++++++ app/views/root/_amends_needed.html.erb | 13 ++++ app/views/root/_archived.html.erb | 14 ++++ app/views/root/_drafts.html.erb | 13 ++++ app/views/root/_fact_check_received.html.erb | 13 ++++ app/views/root/_in_review.html.erb | 15 ++++ app/views/root/_out_for_fact_check.html.erb | 14 ++++ app/views/root/_publication.html.erb | 78 +++++++++++++++++++ app/views/root/_published.html.erb | 15 ++++ app/views/root/_ready.html.erb | 13 ++++ app/views/root/_reviewer.html.erb | 8 ++ .../root/_scheduled_for_publishing.html.erb | 14 ++++ app/views/root/index.html.erb | 66 ++++++++++++++++ config/features.rb | 4 + config/routes.rb | 6 +- test/functional/root_controller_test.rb | 64 +++++++++++++++ 16 files changed, 421 insertions(+), 1 deletion(-) create mode 100644 app/controllers/root_controller.rb create mode 100644 app/views/root/_amends_needed.html.erb create mode 100644 app/views/root/_archived.html.erb create mode 100644 app/views/root/_drafts.html.erb create mode 100644 app/views/root/_fact_check_received.html.erb create mode 100644 app/views/root/_in_review.html.erb create mode 100644 app/views/root/_out_for_fact_check.html.erb create mode 100644 app/views/root/_publication.html.erb create mode 100644 app/views/root/_published.html.erb create mode 100644 app/views/root/_ready.html.erb create mode 100644 app/views/root/_reviewer.html.erb create mode 100644 app/views/root/_scheduled_for_publishing.html.erb create mode 100644 app/views/root/index.html.erb create mode 100644 test/functional/root_controller_test.rb diff --git a/app/controllers/root_controller.rb b/app/controllers/root_controller.rb new file mode 100644 index 000000000..ed86bb232 --- /dev/null +++ b/app/controllers/root_controller.rb @@ -0,0 +1,72 @@ +# frozen_string_literal: true + +class RootController < ApplicationController + respond_to :html, :json + + include ColumnSortable + + ITEMS_PER_PAGE = 20 + + STATE_NAME_LISTS = { "draft" => "drafts", "fact_check" => "out_for_fact_check" }.freeze + + def index + user_filter = params[:user_filter] || session[:user_filter] + session[:user_filter] = user_filter + + @list = params[:list].presence || "drafts" + @presenter, @user_filter = build_presenter(user_filter, params[:page]) + + # Looking at another class, but the whole approach taken by this method and its + # associated presenter needs revisiting. + unless @presenter.acceptable_list?(@list) + render body: { "raw" => "Not Found" }, status: :not_found + return + end + + if params[:string_filter].present? + clean_string_filter = params[:string_filter] + .strip + .gsub(/\s+/, " ") + @presenter.filter_by_substring(clean_string_filter) + end + end + +private + + def format_filter + Artefact::FORMATS_BY_DEFAULT_OWNING_APP["publisher"].include?(params[:format_filter]) ? params[:format_filter] : "edition" + end + + def filtered_editions + return Edition if format_filter == "edition" + + Edition.where(_type: "#{format_filter.camelcase}Edition") + end + + def list_parameter_from_state(state) + STATE_NAME_LISTS[state] || state + end + + def build_presenter(user_filter, current_page = nil) + user_filter, user = process_user_filter(user_filter) + + editions = filtered_editions.order_by([sort_column, sort_direction]) + editions = editions.page(current_page).per(ITEMS_PER_PAGE) + editions = editions.where.not(_type: "PopularLinksEdition") + + [PrimaryListingPresenter.new(editions, user), user_filter] + end + + def process_user_filter(user_filter = nil) + if user_filter.blank? + user_filter = current_user.uid + user = current_user + elsif %w[all nobody].include?(user_filter) + user = user_filter.to_sym + else + user = User.where(uid: user_filter).first + end + + [user_filter, user] + end +end diff --git a/app/views/root/_amends_needed.html.erb b/app/views/root/_amends_needed.html.erb new file mode 100644 index 000000000..3b4c4dda1 --- /dev/null +++ b/app/views/root/_amends_needed.html.erb @@ -0,0 +1,13 @@ + + + + + + + + + + + <%= render :collection => @presenter.amends_needed, :partial => 'publication', :locals => {:tab => :amends_needed} %> + +
<%= sortable "_type", "Format" %><%= sortable "title" %><%= sortable "updated_at", "Updated" %><%= sortable "assignee", "Assigned to" %>
diff --git a/app/views/root/_archived.html.erb b/app/views/root/_archived.html.erb new file mode 100644 index 000000000..dcf9746cf --- /dev/null +++ b/app/views/root/_archived.html.erb @@ -0,0 +1,14 @@ + + + + + + + + + + + + <%= render :collection => @presenter.archived, :partial => 'publication', :locals => {:tab => :archived} %> + +
<%= sortable "_type", "Format" %><%= sortable "title" %><%= sortable "updated_at", "Updated" %><%= sortable "assignee", "Assigned to" %>Actions
diff --git a/app/views/root/_drafts.html.erb b/app/views/root/_drafts.html.erb new file mode 100644 index 000000000..8f172d9fb --- /dev/null +++ b/app/views/root/_drafts.html.erb @@ -0,0 +1,13 @@ + + + + + + + + + + + <%= render :collection => @presenter.draft, :partial => 'publication', :locals => {:tab => :draft} %> + +
<%= sortable "_type", "Format" %><%= sortable "title" %><%= sortable "updated_at", "Updated" %><%= sortable "assignee", "Assigned to" %>
diff --git a/app/views/root/_fact_check_received.html.erb b/app/views/root/_fact_check_received.html.erb new file mode 100644 index 000000000..7a74622c2 --- /dev/null +++ b/app/views/root/_fact_check_received.html.erb @@ -0,0 +1,13 @@ + + + + + + + + + + + <%= render :collection => @presenter.fact_check_received, :partial => 'publication', :locals => {:tab => :fact_check_received} %> + +
<%= sortable "_type", "Format" %><%= sortable "title" %><%= sortable "updated_at", "Updated" %><%= sortable "assignee", "Assigned to" %>
diff --git a/app/views/root/_in_review.html.erb b/app/views/root/_in_review.html.erb new file mode 100644 index 000000000..8485ab8c9 --- /dev/null +++ b/app/views/root/_in_review.html.erb @@ -0,0 +1,15 @@ + + + + + + + + + + + + + <%= render :collection => @presenter.in_review, :partial => 'publication', :locals => {:tab => :in_review} %> + +
<%= sortable "_type", "Format" %><%= sortable "title" %><%= sortable "updated_at", "Updated" %><%= sortable "assignee", "Assigned to" %><%= sortable "review_requested_at", "Awaiting review" %><%= sortable "reviewer", "Reviewer" %>
diff --git a/app/views/root/_out_for_fact_check.html.erb b/app/views/root/_out_for_fact_check.html.erb new file mode 100644 index 000000000..7449498a8 --- /dev/null +++ b/app/views/root/_out_for_fact_check.html.erb @@ -0,0 +1,14 @@ + + + + + + + + + + + + <%= render :collection => @presenter.fact_check, :partial => 'publication', :locals => {:tab => :fact_check} %> + +
<%= sortable "format" %><%= sortable "title" %><%= sortable "updated_at", "Updated" %><%= sortable "last_fact_checked_at", "Sent Out" %><%= sortable "assignee", "Assigned to" %>
diff --git a/app/views/root/_publication.html.erb b/app/views/root/_publication.html.erb new file mode 100644 index 000000000..2fbe0ded2 --- /dev/null +++ b/app/views/root/_publication.html.erb @@ -0,0 +1,78 @@ + + + <%= publication.format.underscore.humanize %> + + +

+ <%= link_to publication.admin_list_title, edition_path(publication) %> + <% if publication.in_beta? %> + beta + <% end %> +

+ + <% if publication.published? %> + <%= link_to "/#{publication.slug}", "#{Plek.website_root}/#{publication.slug}", class: 'link-muted' %> + <% elsif publication.safe_to_preview? %> + <%= link_to "/#{publication.slug}", preview_edition_path(publication), class: 'link-muted' %> + <% end %> + + · #<%= publication.version_number %> + <% if tab && (tab == :published || tab == :archived) && publication.subsequent_siblings.first.present? %> + – <%= link_to "##{publication.subsequent_siblings.first.version_number} in #{publication.subsequent_siblings.first.state.humanize.downcase}", edition_path(publication.subsequent_siblings.first), class: 'link-inherit' %> + + <% end %> + + <% if publication.important_note.present? %> + + · + + + <% end %> + + + + + <% if tab && tab == :fact_check %> + + + + <% end %> + <% if tab && tab == :scheduled_for_publishing %> + + + + <% end %> + + <%= publication.assignee %> + + <% if tab && tab == :in_review %> + + <%= time_ago_in_words(publication.review_requested_at) %> + + + <%= render partial: 'reviewer', locals: { publication: publication } %> + + <% end %> + <% if tab && tab == :published %> + + <%= publication.publisher %> + + <% end %> + <% if tab && (tab == :archived || tab == :published) %> + + <% if current_user.has_editor_permissions?(publication) %> + <% if publication.can_create_new_edition? %> + <%= link_to 'Create new edition', duplicate_edition_path(publication), class: 'btn btn-default', method: :post %> + <% elsif publication.in_progress_sibling %> + <%= link_to 'Edit newer edition', edition_path(publication.in_progress_sibling), html_options = { "class" => "btn btn-info"} %> + <% end %> + <% end %> + + <% end %> + diff --git a/app/views/root/_published.html.erb b/app/views/root/_published.html.erb new file mode 100644 index 000000000..9ba324bb5 --- /dev/null +++ b/app/views/root/_published.html.erb @@ -0,0 +1,15 @@ + + + + + + + + + + + + + <%= render :collection => @presenter.published, :partial => 'publication', :as => 'publication', :locals => {:tab => :published} %> + +
<%= sortable "_type", "Format" %><%= sortable "title" %><%= sortable "updated_at", "Updated" %><%= sortable "assignee", "Assigned to" %><%= sortable "publisher", "Published by" %>Actions
diff --git a/app/views/root/_ready.html.erb b/app/views/root/_ready.html.erb new file mode 100644 index 000000000..87acd4f07 --- /dev/null +++ b/app/views/root/_ready.html.erb @@ -0,0 +1,13 @@ + + + + + + + + + + + <%= render :collection => @presenter.ready, :partial => 'publication', :locals => {:tab => :ready} %> + +
<%= sortable "_type", "Format" %><%= sortable "title" %><%= sortable "updated_at", "Updated" %><%= sortable "assignee", "Assigned to" %>
diff --git a/app/views/root/_reviewer.html.erb b/app/views/root/_reviewer.html.erb new file mode 100644 index 000000000..ae16cbc65 --- /dev/null +++ b/app/views/root/_reviewer.html.erb @@ -0,0 +1,8 @@ +<% if publication.reviewer and publication.reviewer.present? -%> + <%= publication.reviewer %> +<% elsif current_user != publication.assigned_to && current_user.has_editor_permissions?(publication) -%> + <%= form_for :edition, url: review_edition_path(publication._id), method: :put do |f| %> + <%= f.hidden_field :reviewer, value: current_user.name %> + <%= f.submit "Claim 2i", class: "btn btn-primary" %> + <% end -%> +<% end -%> diff --git a/app/views/root/_scheduled_for_publishing.html.erb b/app/views/root/_scheduled_for_publishing.html.erb new file mode 100644 index 000000000..cd28b5178 --- /dev/null +++ b/app/views/root/_scheduled_for_publishing.html.erb @@ -0,0 +1,14 @@ + + + + + + + + + + + + <%= render :collection => @presenter.scheduled_for_publishing, :partial => 'publication', :locals => {:tab => :scheduled_for_publishing} %> + +
<%= sortable "_type", "Format" %><%= sortable "title" %><%= sortable "updated_at", "Updated" %><%= sortable "publish_at", "Scheduled" %><%= sortable "assignee", "Assigned to" %>
diff --git a/app/views/root/index.html.erb b/app/views/root/index.html.erb new file mode 100644 index 000000000..163a6b53d --- /dev/null +++ b/app/views/root/index.html.erb @@ -0,0 +1,66 @@ + +
+

+ <% if !params[:string_filter].blank? %> + Searching for “<%= params[:string_filter] -%>” in “<%= @list.humanize %>” + <% else %> + <%= @list.humanize %> + <% end %> +

+
+ + <% if flash[:notice] %> + <%= flash[:notice] %> + <% end %> + +
+ +
+ + + + +
+ + <% if params[:list] == "in_review" %> +
+

+ <%= link_to "Check Collections publisher", @presenter.step_by_step_review_url %> for step by steps that are waiting for review +

+
+ <% end %> + +
+ <%= render @list %> + <%= paginate @presenter.send(@list), theme: 'twitter-bootstrap-3' %> +
+ +
+ +<% content_for :page_title, "Publications" %> diff --git a/config/features.rb b/config/features.rb index e10ab9803..9fe4b2a30 100644 --- a/config/features.rb +++ b/config/features.rb @@ -8,4 +8,8 @@ default: true, description: "A feature only used by tests; not to be used for any actual features." end + + feature :design_system_publications_filter, + default: false, + description: "Update the publications page to use the GOV.UK Design System" end diff --git a/config/routes.rb b/config/routes.rb index 3935485c8..62e58b798 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -63,7 +63,11 @@ get "api/lookup-by-base-path", to: "publishing_api_proxy#lookup_by_base_path" resources :publications - root to: "legacy_root#index" + constraints FeatureConstraint.new("design_system_publications_filter") do + root to: "root#index" + end + # The below "as: nil" is required to avoid a name clash with the constrained route, above, which causes an error + root to: "legacy_root#index", as: nil # We used to nest all URLs under /admin so we now redirect that # in case people had bookmarks set up. Using a proc as otherwise the diff --git a/test/functional/root_controller_test.rb b/test/functional/root_controller_test.rb new file mode 100644 index 000000000..7742efc15 --- /dev/null +++ b/test/functional/root_controller_test.rb @@ -0,0 +1,64 @@ +require "test_helper" + +class RootControllerTest < ActionController::TestCase + setup do + @users = FactoryBot.create_list(:user, 3) + login_as_stub_user + session[:user_filter] = @users[0].uid + + @guide = FactoryBot.create(:guide_edition, state: "draft") + end + + test "it returns a 404 for an unknown list" do + get :index, params: { list: "draFts" } + assert response.not_found? + end + + # Most values of the list parameter match a scope on the Edition + # model, but some don't and we want to test that we allow those + # through correctly + test "it supports lists that don't match a model scope" do + get :index, params: { list: "drafts" } + assert response.ok? + end + + test "should strip leading/trailing whitespace from string_filter" do + @guide.update!(title: "Stuff") + get( + :index, + params: { + list: "drafts", + user_filter: "all", + string_filter: " stuff", + }, + ) + assert_select "td.title", /Stuff/i + end + + test "should strip excess interstitial whitespace from string_filter" do + @guide.update!(title: "Stuff and things") + get( + :index, + params: { + list: "drafts", + user_filter: "all", + string_filter: "stuff and things", + }, + ) + assert_select "td.title", /Stuff and things/i + end + + test "should search in slug with string_filter" do + @guide.update!(title: "Stuff") + @guide.update!(slug: "electric-banana") + get( + :index, + params: { + list: "drafts", + user_filter: "all", + string_filter: "electric-banana", + }, + ) + assert_select "td.title", /Stuff/i + end +end From 147e8968109659a748fd82565af39905f5bc21ee Mon Sep 17 00:00:00 2001 From: Mark Taylor <138604938+mtaylorgds@users.noreply.github.com> Date: Thu, 11 Jul 2024 14:51:06 +0100 Subject: [PATCH 3/3] Update brakeman ignore file The `LegacyRootController` is a duplicate of the `RootController` and so all of the same brakeman results (and therefore ignores) apply. --- config/brakeman.ignore | 90 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 79 insertions(+), 11 deletions(-) diff --git a/config/brakeman.ignore b/config/brakeman.ignore index f91dc5b27..e8800cd59 100644 --- a/config/brakeman.ignore +++ b/config/brakeman.ignore @@ -1,5 +1,39 @@ { "ignored_warnings": [ + { + "warning_type": "Dynamic Render Path", + "warning_code": 15, + "fingerprint": "2cee65ee3ed052192ee50380a02fcd7c1a9d9f81e3a8f0dee8f656fb473e75ff", + "check_name": "Render", + "message": "Render path contains parameter value", + "file": "app/views/legacy_root/index.html.erb", + "line": 60, + "link": "https://brakemanscanner.org/docs/warning_types/dynamic_render_path/", + "code": "render(action => (params[:list] or \"drafts\"), {})", + "render_path": [ + { + "type": "controller", + "class": "LegacyRootController", + "method": "index", + "line": 20, + "file": "app/controllers/legacy_root_controller.rb", + "rendered": { + "name": "legacy_root/index", + "file": "app/views/legacy_root/index.html.erb" + } + } + ], + "location": { + "type": "template", + "template": "legacy_root/index" + }, + "user_input": "params[:list]", + "confidence": "High", + "cwe_id": [ + 22 + ], + "note": "The params[:list] argument is already checked before the template gets rendered by @presenter.acceptable_list?(@list)." + }, { "warning_type": "Redirect", "warning_code": 18, @@ -7,7 +41,7 @@ "check_name": "Redirect", "message": "Possible unprotected redirect", "file": "app/controllers/reports_controller.rb", - "line": 21, + "line": 23, "link": "https://brakemanscanner.org/docs/warning_types/redirect/", "code": "redirect_to(Report.new(\"all_edition_churn\").url, :allow_other_host => true)", "render_path": null, @@ -38,7 +72,7 @@ "type": "controller", "class": "RootController", "method": "index", - "line": 20, + "line": 22, "file": "app/controllers/root_controller.rb", "rendered": { "name": "root/index", @@ -64,7 +98,7 @@ "check_name": "Redirect", "message": "Possible unprotected redirect", "file": "app/controllers/reports_controller.rb", - "line": 33, + "line": 35, "link": "https://brakemanscanner.org/docs/warning_types/redirect/", "code": "redirect_to(Report.new(\"all_urls\").url, :allow_other_host => true)", "render_path": null, @@ -95,7 +129,7 @@ "type": "controller", "class": "RootController", "method": "index", - "line": 20, + "line": 22, "file": "app/controllers/root_controller.rb", "rendered": { "name": "root/index", @@ -121,7 +155,7 @@ "check_name": "Redirect", "message": "Possible unprotected redirect", "file": "app/controllers/reports_controller.rb", - "line": 29, + "line": 31, "link": "https://brakemanscanner.org/docs/warning_types/redirect/", "code": "redirect_to(Report.new(\"all_content_workflow\").url, :allow_other_host => true)", "render_path": null, @@ -164,7 +198,7 @@ "check_name": "Redirect", "message": "Possible unprotected redirect", "file": "app/controllers/reports_controller.rb", - "line": 17, + "line": 19, "link": "https://brakemanscanner.org/docs/warning_types/redirect/", "code": "redirect_to(Report.new(\"edition_churn\").url, :allow_other_host => true)", "render_path": null, @@ -187,7 +221,7 @@ "check_name": "Redirect", "message": "Possible unprotected redirect", "file": "app/controllers/reports_controller.rb", - "line": 25, + "line": 27, "link": "https://brakemanscanner.org/docs/warning_types/redirect/", "code": "redirect_to(Report.new(\"content_workflow\").url, :allow_other_host => true)", "render_path": null, @@ -210,7 +244,7 @@ "check_name": "Redirect", "message": "Possible unprotected redirect", "file": "app/controllers/reports_controller.rb", - "line": 9, + "line": 11, "link": "https://brakemanscanner.org/docs/warning_types/redirect/", "code": "redirect_to(Report.new(\"editorial_progress\").url, :allow_other_host => true)", "render_path": null, @@ -233,7 +267,7 @@ "check_name": "Redirect", "message": "Possible unprotected redirect", "file": "app/controllers/reports_controller.rb", - "line": 13, + "line": 15, "link": "https://brakemanscanner.org/docs/warning_types/redirect/", "code": "redirect_to(Report.new(\"organisation_content\").url, :allow_other_host => true)", "render_path": null, @@ -248,8 +282,42 @@ 601 ], "note": "" + }, + { + "warning_type": "Dangerous Send", + "warning_code": 23, + "fingerprint": "d114390670b08cfb4120b6e65b5b2374a0f266a3ae14b5d5630cefd811e2e110", + "check_name": "Send", + "message": "User controlled method execution", + "file": "app/views/legacy_root/index.html.erb", + "line": 61, + "link": "https://brakemanscanner.org/docs/warning_types/dangerous_send/", + "code": "@presenter.send((params[:list] or \"drafts\"))", + "render_path": [ + { + "type": "controller", + "class": "LegacyRootController", + "method": "index", + "line": 20, + "file": "app/controllers/legacy_root_controller.rb", + "rendered": { + "name": "legacy_root/index", + "file": "app/views/legacy_root/index.html.erb" + } + } + ], + "location": { + "type": "template", + "template": "legacy_root/index" + }, + "user_input": "params[:list]", + "confidence": "High", + "cwe_id": [ + 77 + ], + "note": "The params[:list] argument is already checked before the template gets rendered by @presenter.acceptable_list?(@list)." } ], - "updated": "2024-01-24 11:41:24 +0000", - "brakeman_version": "6.1.1" + "updated": "2024-07-11 13:48:45 +0000", + "brakeman_version": "6.1.2" }