diff --git a/app/controllers/legacy_root_controller.rb b/app/controllers/legacy_root_controller.rb new file mode 100644 index 000000000..a32c43467 --- /dev/null +++ b/app/controllers/legacy_root_controller.rb @@ -0,0 +1,70 @@ +class LegacyRootController < 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/controllers/root_controller.rb b/app/controllers/root_controller.rb index 4a81efb1f..ed86bb232 100644 --- a/app/controllers/root_controller.rb +++ b/app/controllers/root_controller.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + class RootController < ApplicationController respond_to :html, :json diff --git a/app/views/legacy_root/_amends_needed.html.erb b/app/views/legacy_root/_amends_needed.html.erb new file mode 100644 index 000000000..3b4c4dda1 --- /dev/null +++ b/app/views/legacy_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/legacy_root/_archived.html.erb b/app/views/legacy_root/_archived.html.erb new file mode 100644 index 000000000..dcf9746cf --- /dev/null +++ b/app/views/legacy_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/legacy_root/_drafts.html.erb b/app/views/legacy_root/_drafts.html.erb new file mode 100644 index 000000000..8f172d9fb --- /dev/null +++ b/app/views/legacy_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/legacy_root/_fact_check_received.html.erb b/app/views/legacy_root/_fact_check_received.html.erb new file mode 100644 index 000000000..7a74622c2 --- /dev/null +++ b/app/views/legacy_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/legacy_root/_in_review.html.erb b/app/views/legacy_root/_in_review.html.erb new file mode 100644 index 000000000..8485ab8c9 --- /dev/null +++ b/app/views/legacy_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/legacy_root/_out_for_fact_check.html.erb b/app/views/legacy_root/_out_for_fact_check.html.erb new file mode 100644 index 000000000..7449498a8 --- /dev/null +++ b/app/views/legacy_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/legacy_root/_publication.html.erb b/app/views/legacy_root/_publication.html.erb new file mode 100644 index 000000000..2fbe0ded2 --- /dev/null +++ b/app/views/legacy_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/legacy_root/_published.html.erb b/app/views/legacy_root/_published.html.erb new file mode 100644 index 000000000..9ba324bb5 --- /dev/null +++ b/app/views/legacy_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/legacy_root/_ready.html.erb b/app/views/legacy_root/_ready.html.erb new file mode 100644 index 000000000..87acd4f07 --- /dev/null +++ b/app/views/legacy_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/legacy_root/_reviewer.html.erb b/app/views/legacy_root/_reviewer.html.erb new file mode 100644 index 000000000..ae16cbc65 --- /dev/null +++ b/app/views/legacy_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/legacy_root/_scheduled_for_publishing.html.erb b/app/views/legacy_root/_scheduled_for_publishing.html.erb new file mode 100644 index 000000000..cd28b5178 --- /dev/null +++ b/app/views/legacy_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/legacy_root/index.html.erb b/app/views/legacy_root/index.html.erb new file mode 100644 index 000000000..163a6b53d --- /dev/null +++ b/app/views/legacy_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 %> + +
+ +
+ +
+
    +
  • Filter by Status
    • + <%= publication_tab_list(@presenter, current: @list) %> +
+
+ +
+
+ <%= hidden_field_tag :list, params[:list] %> + <%= hidden_field_tag :string_filter, params[:string_filter] %> + + <%= + select_tag("user_filter", options_for_select( + [["All", "all"], ["Nobody", "nobody"]] + + User.enabled.alphabetized.map{ |u| [u.name, u.uid] }, @user_filter + ), class: 'form-control js-user-filter', "data-module" => 'assignee-select') + %> + + <%= text_field_tag "string_filter", params[:string_filter], class: 'form-control', type: "search" %> + + + <%= select_tag("format_filter", options_for_select( + format_filter_selection_options, + params[:format_filter] + ), class: 'form-control') %> + +
+
+
+ + <% 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/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" } 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 769a9a3bc..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: "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/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/legacy_root_controller_test.rb b/test/functional/legacy_root_controller_test.rb new file mode 100644 index 000000000..ed8515f59 --- /dev/null +++ b/test/functional/legacy_root_controller_test.rb @@ -0,0 +1,64 @@ +require "test_helper" + +class LegacyRootControllerTest < 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