From 7115d5f0cf6f65ac5a95bedf0252de927cb99468 Mon Sep 17 00:00:00 2001 From: Alan Gabbianelli Date: Mon, 15 Apr 2019 19:50:20 +0100 Subject: [PATCH] Allow download of links with specific statuses The previous behaviour of clicking a link to download broken links has been modified so that the user can select specific statuses for which they want to download links. Only links with the selected status will be present in the downloaded csv file. --- app/assets/javascripts/local_authorities.js | 15 +++ .../local_authorities_controller.rb | 6 +- .../shared/_local_authority_details.html.erb | 21 +++- config/initializers/assets.rb | 1 + config/routes.rb | 2 +- .../export/links_exporter.rb | 71 ++++++------ .../local_authorities_controller_spec.rb | 14 ++- .../local_authority_show_spec.rb | 108 ++++++++++++------ .../export/fixtures/bad_links_url_status.csv | 2 +- .../export/links_exporter_spec.rb | 68 +++++++---- 10 files changed, 211 insertions(+), 97 deletions(-) create mode 100644 app/assets/javascripts/local_authorities.js diff --git a/app/assets/javascripts/local_authorities.js b/app/assets/javascripts/local_authorities.js new file mode 100644 index 000000000..3dd300871 --- /dev/null +++ b/app/assets/javascripts/local_authorities.js @@ -0,0 +1,15 @@ +document.addEventListener("DOMContentLoaded", function(event) { + var links_download_button = document.getElementById('links_download_button'); + var href = links_download_button.href; + + links_download_button.addEventListener('mouseenter', function() { + var checkboxes = document.getElementsByClassName('link_status_checkbox'); + var params = []; + for (var i = 0; i < checkboxes.length; i++) { + if (checkboxes[i].checked) { + params.push(checkboxes[i].name + '=' + checkboxes[i].value + '&') + } + } + links_download_button.href = href + '?' + params.join('') + }); +}); diff --git a/app/controllers/local_authorities_controller.rb b/app/controllers/local_authorities_controller.rb index 30e1076e2..7d5b63c09 100644 --- a/app/controllers/local_authorities_controller.rb +++ b/app/controllers/local_authorities_controller.rb @@ -17,11 +17,11 @@ def show @link_count = links_for_authority.count end - def broken_links_csv + def links_csv @authority = LocalAuthority.find_by_slug!(params[:local_authority_slug]) authority_name = @authority.name.parameterize.underscore - data = LocalLinksManager::Export::LinksExporter.new.export_broken_links(@authority) - send_data data, filename: "#{authority_name}_broken_links.csv" + data = LocalLinksManager::Export::LinksExporter.new.export_links(@authority.id, params) + send_data data, filename: "#{authority_name}_links.csv" end def bad_homepage_url_and_status_csv diff --git a/app/views/shared/_local_authority_details.html.erb b/app/views/shared/_local_authority_details.html.erb index 05a68522b..dee4e4443 100644 --- a/app/views/shared/_local_authority_details.html.erb +++ b/app/views/shared/_local_authority_details.html.erb @@ -1,9 +1,28 @@ +<%= javascript_include_tag params[:controller] %> +

<%= authority.name %>

Homepage <%= link_to_if(authority.homepage_url, nil, authority.homepage_url) %>
<%= authority.homepage_status %> <%= authority.homepage_link_last_checked %> -

<%= link_to('Download broken links', broken_links_csv_local_authority_path, class: "btn btn-default btn-s") %>

+

+ <%= check_box_tag(:ok, :ok, true, class: 'link_status_checkbox') %> + <%= label_tag(:ok, 'OK') %> + <%= check_box_tag(:broken, :broken, true, class: 'link_status_checkbox') %> + <%= label_tag(:broken, 'Broken') %> + <%= check_box_tag(:caution, :caution, true, class: 'link_status_checkbox') %> + <%= label_tag(:caution, 'Caution') %> + <%= check_box_tag(:missing, :missing, true, class: 'link_status_checkbox') %> + <%= label_tag(:missing, 'Missing') %> + <%= check_box_tag(:pending, :pending, true, class: 'link_status_checkbox') %> + <%= label_tag(:pending, 'Pending') %> + <%= link_to( + 'Download links', + links_csv_local_authority_path, + class: "btn btn-default btn-s", + id: "links_download_button" + ) %> +

diff --git a/config/initializers/assets.rb b/config/initializers/assets.rb index 01ef3e663..b36a7ab3c 100644 --- a/config/initializers/assets.rb +++ b/config/initializers/assets.rb @@ -9,3 +9,4 @@ # Precompile additional assets. # application.js, application.css, and all non-JS/CSS in app/assets folder are already added. # Rails.application.config.assets.precompile += %w( search.js ) +Rails.application.config.assets.precompile += %w( local_authorities.js ) diff --git a/config/routes.rb b/config/routes.rb index 052ec412d..6c55e3859 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -5,7 +5,7 @@ resources 'local_authorities', only: [:index, :show], param: :local_authority_slug do member do - get 'broken_links_csv' + get 'links_csv' end end diff --git a/lib/local-links-manager/export/links_exporter.rb b/lib/local-links-manager/export/links_exporter.rb index ddfd5b344..920ccb132 100644 --- a/lib/local-links-manager/export/links_exporter.rb +++ b/lib/local-links-manager/export/links_exporter.rb @@ -3,9 +3,29 @@ module LocalLinksManager module Export class LinksExporter - HEADINGS = ["Authority Name", "SNAC", "GSS", "Description", "LGSL", "LGIL", "URL"].freeze - ALL_LINKS_HEADINGS = ["Supported by GOV.UK"].freeze - BROKEN_LINKS_HEADINGS = ["New URL"].freeze + SELECTION = [ + "local_authorities.name", + :snac, + :gss, + "services.label as service_label", + "interactions.label as interaction_label", + "links.status as status", + :lgsl_code, + :lgil_code, + :url, + :enabled + ].freeze + COMMON_HEADINGS = [ + "Authority Name", + "SNAC", + "GSS", + "Description", + "LGSL", + "LGIL", + "URL", + "Supported by GOV.UK" + ].freeze + EXTRA_HEADINGS = ["Status", "New URL"].freeze def self.export_links path = Rails.root.join("public", "data", 'links_to_services_provided_by_local_authorities.csv') @@ -17,54 +37,40 @@ def self.export_links def export(io) output = CSV.generate do |csv| - csv << HEADINGS + ALL_LINKS_HEADINGS + csv << COMMON_HEADINGS records.each do |record| - csv << format(record).push(record.enabled) + csv << format(record) end end io.write(output) end - def export_broken_links(local_authority_id) + def export_links(local_authority_id, params) + statuses = params.slice('ok', 'broken', 'caution', 'missing', 'pending').keys CSV.generate do |csv| - csv << HEADINGS + BROKEN_LINKS_HEADINGS - broken_links(local_authority_id).each do |link| - csv << format(link) + csv << COMMON_HEADINGS + EXTRA_HEADINGS + statuses.each do |status| + links(local_authority_id, status).each do |link| + csv << format(link).push(link.status) + end end end end def records Link.with_url.joins(:local_authority, :service, :interaction) - .select( - "local_authorities.name", - :snac, - :gss, - "services.label as service_label", - "interactions.label as interaction_label", - :lgsl_code, - :lgil_code, - :url, - :enabled - ).order("local_authorities.name", "services.lgsl_code", "interactions.lgil_code").all + .select(*SELECTION) + .order("local_authorities.name", "services.lgsl_code", "interactions.lgil_code").all end private - def broken_links(local_authority_id) - Link.enabled_links.broken + def links(local_authority_id, status) + Link.enabled_links.public_send(status) .where(local_authority_id: local_authority_id) .joins(:local_authority, :service, :interaction) - .select( - "local_authorities.name", - :snac, - :gss, - "services.label as service_label", - "interactions.label as interaction_label", - :lgsl_code, - :lgil_code, - :url, - ).order("services.lgsl_code", "interactions.lgil_code").all + .select(*SELECTION) + .order("services.lgsl_code", "interactions.lgil_code").all end def format(record) @@ -76,6 +82,7 @@ def format(record) record.lgsl_code, record.lgil_code, record.url, + record.enabled ] end diff --git a/spec/controllers/local_authorities_controller_spec.rb b/spec/controllers/local_authorities_controller_spec.rb index 9d4fd0bae..ab5e434e1 100644 --- a/spec/controllers/local_authorities_controller_spec.rb +++ b/spec/controllers/local_authorities_controller_spec.rb @@ -41,14 +41,24 @@ end end - describe "GET broken_links_csv" do + describe "GET links_csv" do before do @local_authority = create(:local_authority) end it "retrieves HTTP success" do login_as_stub_user - get :broken_links_csv, params: { local_authority_slug: @local_authority.slug } + get( + :links_csv, + params: { + local_authority_slug: @local_authority.slug, + ok: 'ok', + broken: 'broken', + caution: 'caution', + missing: 'missing', + pending: 'pending' + } + ) expect(response).to have_http_status(200) expect(response.headers["Content-Type"]).to eq("text/csv") end diff --git a/spec/features/local_authorities/local_authority_show_spec.rb b/spec/features/local_authorities/local_authority_show_spec.rb index 269d0c39f..13b390318 100644 --- a/spec/features/local_authorities/local_authority_show_spec.rb +++ b/spec/features/local_authorities/local_authority_show_spec.rb @@ -1,16 +1,17 @@ require 'rails_helper' feature "The local authority show page" do + let!(:local_authority) { create(:district_council) } + before do User.create(email: 'user@example.com', name: 'Test User', permissions: %w[signin]) - @local_authority = create(:district_council) - visit local_authority_path(local_authority_slug: @local_authority.slug) + visit local_authority_path(local_authority_slug: local_authority.slug) end it 'has a list of breadcrumbs pointing back to the authority that lead us here' do within '.breadcrumb' do expect(page).to have_link 'Local links', href: root_path - expect(page).to have_text @local_authority.name + expect(page).to have_text local_authority.name end end @@ -26,18 +27,18 @@ end it "displays 'No link'" do - @local_authority.homepage_url = nil - @local_authority.save - visit local_authority_path(local_authority_slug: @local_authority.slug) + local_authority.homepage_url = nil + local_authority.save + visit local_authority_path(local_authority_slug: local_authority.slug) within(:css, ".page-title") do expect(page).to have_content('No link') end end it "does not display 'Link not checked'" do - @local_authority.homepage_url = nil - @local_authority.save - visit local_authority_path(local_authority_slug: @local_authority.slug) + local_authority.homepage_url = nil + local_authority.save + visit local_authority_path(local_authority_slug: local_authority.slug) within(:css, ".page-title") do expect(page).not_to have_content('Link not checked') end @@ -45,14 +46,15 @@ end describe "with services present" do + let(:service) { create(:service, :all_tiers) } + let(:disabled_service) { create(:disabled_service) } + let!(:ok_link) { create_service_interaction_link(service, status: :ok) } + let!(:disabled_link) { create_service_interaction_link(disabled_service, status: :ok) } + let!(:broken_link) { create_service_interaction_link(service, status: :broken) } + let!(:missing_link) { create_missing_link(service) } + before do - @service = create(:service, :all_tiers) - @disabled_service = create(:disabled_service) - @good_link = create_service_interaction_link(@service, status: :ok) - @disabled_link = create_service_interaction_link(@disabled_service, status: :ok) - @broken_link = create_service_interaction_link(@service, status: :broken) - @missing_link = create_missing_link(@service) - visit local_authority_path(@local_authority) + visit local_authority_path(local_authority) end let(:http_status) { 200 } @@ -77,11 +79,11 @@ it "shows only the enabled services provided by the authority according to its tier with links to their individual pages" do expect(page).to have_content 'Services and links' - expect(page).to have_text(@good_link.service.label) + expect(page).to have_text(ok_link.service.label) end it "does not show the disabled service interaction" do - expect(page).not_to have_content(@disabled_service.label) + expect(page).not_to have_content(disabled_service.label) end it "shows missing links" do @@ -90,28 +92,68 @@ it "shows each service's LGSL codes in the table" do expect(page).to have_content 'Code' - expect(page).to have_css('td.lgsl', text: @good_link.service.lgsl_code) + expect(page).to have_css('td.lgsl', text: ok_link.service.lgsl_code) end it 'shows the link status as Good Link when the status is 200' do - within(:css, "tr[data-interaction-id=\"#{@good_link.interaction.id}\"]") do + within(:css, "tr[data-interaction-id=\"#{ok_link.interaction.id}\"]") do expect(page).to have_text 'Good' end end it 'shows the link last checked details' do - expect(page).to have_text @good_link.link_last_checked + expect(page).to have_text ok_link.link_last_checked end it 'should have a link to Edit Link' do - expect(page).to have_link 'Edit link', href: edit_link_path(@local_authority, @service, @good_link.interaction) + expect(page).to have_link 'Edit link', href: edit_link_path(local_authority, service, ok_link.interaction) end - it "allows user to download a CSV of broken links for the local authority" do - click_on "Download broken links" - expect(page.response_headers["Content-Type"]).to eq("text/csv") - csv = page.body.chomp.split(",") - expect(csv).to include(@broken_link.url) + describe "CSV download" do + let(:status_checkboxes) { %w(ok broken caution missing pending) } + let(:url_regex) { /http:\/\/.+\/local_authorities\/.+\/links_csv/ } + + it "downloads a CSV" do + click_on "Download links" + + expect(page.response_headers["Content-Type"]).to eq("text/csv") + end + + context "when user leaves all link status checkboxes selected (by default)", js: true do + it "passes all statuses in params" do + submit_button = find("a", text: "Download links") + expect(submit_button['href']).to match(url_regex) + + submit_button.hover + params = submit_button['href'].split('?')[-1].split('&') + + status_checkboxes.each do |status| + expect(params).to include("#{status}=#{status}") + end + end + end + + context "when user deselects some link status checkboxes", js: true do + let(:unchecked_status_checkboxes) { %w(ok caution pending) } + let(:checked_status_checkboxes) { status_checkboxes - unchecked_status_checkboxes } + + it "passes all statuses in params, except the unchecked ones" do + submit_button = find("a", text: "Download links") + expect(submit_button['href']).to match(url_regex) + + unchecked_status_checkboxes.each { |status_checkbox| uncheck status_checkbox } + submit_button.hover + params = submit_button['href'].split('?')[-1].split('&') + + checked_status_checkboxes.each do |status| + expect(params).to include("#{status}=#{status}") + end + + unchecked_status_checkboxes.each do |status| + expect(params).to_not include("#{status}=#{status}") + end + end + end end context 'editing a link' do @@ -120,14 +162,14 @@ fill_in('link_url', with: 'http://angus.example.com/link-to-change') click_on('Update') - expect(page.current_path).to eq(local_authority_path(@local_authority)) + expect(page.current_path).to eq(local_authority_path(local_authority)) end it 'returns you to the correct page after cancelling the editing of a link' do within('.table') { click_on('Edit link', match: :first) } click_on('Cancel') - expect(page.current_path).to eq(local_authority_path(@local_authority)) + expect(page.current_path).to eq(local_authority_path(local_authority)) end end @@ -141,11 +183,11 @@ end it 'shows non-200 status links' do - expect(page).to have_link @broken_link.url + expect(page).to have_link broken_link.url end it 'doesn\'t show 200 status links' do - expect(page).not_to have_link @good_link.url + expect(page).not_to have_link ok_link.url end it 'shows missing links' do @@ -159,7 +201,7 @@ def create_service_interaction_link(service, status:) create( :link, - local_authority: @local_authority, + local_authority: local_authority, service_interaction: service_interaction, status: status ) @@ -170,7 +212,7 @@ def create_missing_link(service) create( :missing_link, - local_authority: @local_authority, + local_authority: local_authority, service_interaction: service_interaction, status: "missing" ) diff --git a/spec/lib/local-links-manager/export/fixtures/bad_links_url_status.csv b/spec/lib/local-links-manager/export/fixtures/bad_links_url_status.csv index 91d5a93fe..1c3d3f4d8 100644 --- a/spec/lib/local-links-manager/export/fixtures/bad_links_url_status.csv +++ b/spec/lib/local-links-manager/export/fixtures/bad_links_url_status.csv @@ -1,5 +1,5 @@ ga:dimension36,ga:dimension37 http://www.carmarthenshire.gov.uk/Cymraeg/addysg/childrens-services/Pages/fostering.aspx,Page not found -http://www.warwickshire.gov.uk/azrecycling,Website unavailable http://www.southoxon.gov.uk/dogwardens,Page requires login +http://www.warwickshire.gov.uk/azrecycling,Website unavailable https://portal.southtyneside.info/eservices/frmHomepage.aspx?FunctionId=79&ignore=0,Security Error diff --git a/spec/lib/local-links-manager/export/links_exporter_spec.rb b/spec/lib/local-links-manager/export/links_exporter_spec.rb index 90d035210..a12e723af 100644 --- a/spec/lib/local-links-manager/export/links_exporter_spec.rb +++ b/spec/lib/local-links-manager/export/links_exporter_spec.rb @@ -8,7 +8,7 @@ def fixture_file(file) let(:exporter) { LocalLinksManager::Export::LinksExporter.new } - describe '#export_links' do + describe '#export' do def test_url(local_authority, interaction) base_url = "http://www.example.com" "#{base_url}/#{local_authority.gss}/#{interaction.lgil_code}" @@ -61,30 +61,50 @@ def test_url(local_authority, interaction) end end - describe "#export_broken_links" do + describe "#export_links" do + let(:headings) { (LocalLinksManager::Export::LinksExporter::COMMON_HEADINGS + LocalLinksManager::Export::LinksExporter::EXTRA_HEADINGS).join(",") } let(:la) { create(:local_authority) } - let(:service) { create(:service) } - let(:disabled_service) { create(:disabled_service) } - let(:interaction_1) { create(:interaction) } - let(:interaction_2) { create(:interaction) } - let(:service_interaction_1) { create(:service_interaction, service: service, interaction: interaction_1) } - let(:service_interaction_2) { create(:service_interaction, service: service, interaction: interaction_2) } - let(:service_interaction_3) { create(:service_interaction, service: disabled_service, interaction: interaction_1) } - let(:broken_link) { create(:broken_link, local_authority: la, url: "http://www.diagonalley.gov.uk/broken-link", service_interaction: service_interaction_1) } - let(:ok_link) { create(:link, local_authority: la, url: "http://www.diagonalley.gov.uk/ok-link", status: "ok", service_interaction: service_interaction_2) } - let(:disabled_link) { create(:broken_link, local_authority: la, url: "http://www.diagonalley.gov.uk/ok-link", service_interaction: service_interaction_3) } - - it "exports broken links for enabled services for a given local authority to CSV format with headings" do - broken_link_data = "#{la.name},#{la.snac},#{la.gss},#{service.label}: #{interaction_1.label},#{service.lgsl_code},#{interaction_1.lgil_code},#{broken_link.url}" - ok_link_data = "#{la.name},#{la.snac},#{la.gss},#{service.label}: #{interaction_2.label},#{service.lgsl_code},#{interaction_2.lgil_code},#{ok_link.url}" - disabled_link_data = "#{la.name},#{la.snac},#{la.gss},#{disabled_service.label}: #{interaction_1.label},#{disabled_service.lgsl_code},#{interaction_1.lgil_code},#{disabled_link.url}" - headings = (LocalLinksManager::Export::LinksExporter::HEADINGS + LocalLinksManager::Export::LinksExporter::BROKEN_LINKS_HEADINGS).join(",") - csv = exporter.export_broken_links(la.id).split("\n") - - expect(csv).to include(broken_link_data) - expect(csv).not_to include(ok_link_data) - expect(csv).not_to include(disabled_link_data) - expect(csv).to include(headings) + let(:ok_link) { create(:ok_link, local_authority: la) } + let(:broken_link) { create(:broken_link, local_authority: la) } + let(:caution_link) { create(:caution_link, local_authority: la) } + let(:missing_link) { create(:missing_link, local_authority: la) } + let(:pending_link) { create(:pending_link, local_authority: la) } + let(:disabled_link) { create(:link_for_disabled_service, local_authority: la) } + let!(:links) do + { + 'ok' => ok_link, + 'broken' => broken_link, + 'caution' => caution_link, + 'missing' => missing_link, + 'pending' => pending_link, + 'disabled' => disabled_link + } + end + + %w(ok broken caution missing pending).each do |status_in_params| + context "when params is {'#{status_in_params}' => '#{status_in_params}'}" do + let(:params) { { status_in_params => status_in_params } } + let(:csv) { exporter.export_links(la.id, params) } + + it "exports #{status_in_params} links for enabled services for a given local authority to CSV format with headings" do + expect(csv).to include(headings) + links.slice(status_in_params).values.each do |link| + expect(csv).to include("#{la.name},#{la.snac},#{la.gss},#{link.service.label}: #{link.interaction.label},#{link.service.lgsl_code},#{link.interaction.lgil_code},#{link.url},#{link.service.enabled},#{link.status}") + end + end + + it "does not export links for disabled services" do + expect(csv).to_not include("#{la.name},#{la.snac},#{la.gss},#{disabled_link.service.label}: #{disabled_link.interaction.label},#{disabled_link.service.lgsl_code},#{disabled_link.interaction.lgil_code},#{disabled_link.url},#{disabled_link.service.enabled},#{disabled_link.status}") + end + + (%w(ok broken caution missing pending) - [status_in_params]).each do |status_not_in_params| + it "does not export #{status_not_in_params} links" do + links.except(status_in_params).values.each do |link| + expect(csv).to_not include("#{la.name},#{la.snac},#{la.gss},#{link.service.label}: #{link.interaction.label},#{link.service.lgsl_code},#{link.interaction.lgil_code},#{link.url},#{link.service.enabled},#{link.status}") + end + end + end + end end end end