From bdf582a129cddef3c48348bcfb05c5bbe83a247e Mon Sep 17 00:00:00 2001 From: gms-gs Date: Tue, 26 Nov 2024 12:18:42 +0000 Subject: [PATCH 1/8] Remove canonical-rails gem and handroll our own --- Gemfile | 3 --- Gemfile.lock | 3 --- app/views/layouts/application.html.erb | 4 +++- app/views/layouts/find_layout.html.erb | 4 +++- 4 files changed, 6 insertions(+), 8 deletions(-) diff --git a/Gemfile b/Gemfile index aaf4848679..75cc3b3e0a 100644 --- a/Gemfile +++ b/Gemfile @@ -20,9 +20,6 @@ gem 'puma', '~> 6.5' # Reduces boot times through caching; required in config/boot.rb gem 'bootsnap', '>= 1.1.0', require: false -# Canonical meta tag -gem 'canonical-rails' - # Decorate logic to keep it out of the views and helper methods gem 'draper' diff --git a/Gemfile.lock b/Gemfile.lock index f780b6ca54..402f4220d3 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -177,8 +177,6 @@ GEM activesupport (>= 3.0.0) uniform_notifier (~> 1.11) byebug (11.1.3) - canonical-rails (0.2.16) - actionview (>= 4.1, < 7.3) capybara (3.40.0) addressable matrix @@ -756,7 +754,6 @@ DEPENDENCIES brakeman bullet byebug - canonical-rails capybara (>= 2.15) cloudfront-rails colorize diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index 13bd3b3eab..cc4da06e3a 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -4,7 +4,9 @@ <%= yield :page_title %> - Publish teacher training courses - GOV.UK <%= csrf_meta_tags %> <%= csp_meta_tag %> - <%= canonical_tag %> + + <%= tag.link(href: request.url.split('?').first, rel: "canonical") %> + <%= tag.meta(property: "og:url", content: request.url.split('?').first) %> <%= tag.meta(name: "viewport", content: "width=device-width, initial-scale=1") %> <%= tag.meta(property: "og:image", content: image_path("govuk-opengraph-image.png")) %> diff --git a/app/views/layouts/find_layout.html.erb b/app/views/layouts/find_layout.html.erb index cd85467ee6..cf62cbd058 100644 --- a/app/views/layouts/find_layout.html.erb +++ b/app/views/layouts/find_layout.html.erb @@ -4,7 +4,9 @@ <%= yield :page_title %> - <%= t("service_name.find") %> - GOV.UK <%= csrf_meta_tags %> <%= csp_meta_tag %> - <%= canonical_tag %> + + <%= tag.link(href: request.url.split('?').first, rel: "canonical") %> + <%= tag.meta(property: "og:url", content: request.url.split('?').first) %> <%= tag.meta(name: "viewport", content: "width=device-width, initial-scale=1") %> <%= tag.meta(property: "og:image", content: image_path("govuk-opengraph-image.png")) %> From 759443fe0e6660e1041b48e696a7e97d288711c7 Mon Sep 17 00:00:00 2001 From: gms-gs Date: Tue, 26 Nov 2024 12:35:45 +0000 Subject: [PATCH 2/8] Remove canonical_rails.rb file --- config/initializers/canonical_rails.rb | 30 -------------------------- 1 file changed, 30 deletions(-) delete mode 100644 config/initializers/canonical_rails.rb diff --git a/config/initializers/canonical_rails.rb b/config/initializers/canonical_rails.rb deleted file mode 100644 index 6926dc717a..0000000000 --- a/config/initializers/canonical_rails.rb +++ /dev/null @@ -1,30 +0,0 @@ -# frozen_string_literal: true - -# Do yourself a favor and set these up right when you install the engine. - -CanonicalRails.setup do |config| - # Force the protocol. If you do not specify, the protocol will be based on the incoming request's protocol. - - config.protocol = 'https://' - - # This is the main host, not just the TLD, omit slashes and protocol. If you have more than one, pick the one you want to rank in search results. - - config.host = 'find-teacher-training-courses.service.gov.uk' - config.port # = '3000' - - # http://en.wikipedia.org/wiki/URL_normalization - # Trailing slash represents semantics of a directory, ie a collection view - implying an :index get route; - # otherwise we have to assume semantics of an instance of a resource type, a member view - implying a :show get route - # - # Acts as an allowlist for routes to have trailing slashes - - config.collection_actions # = [:index] - - # Parameter spamming can cause index dilution by creating seemingly different URLs with identical or near-identical content. - # Unless allowed, these parameters will be omitted - - config.allowed_parameters # = [] - - # Output a matching OpenGraph URL meta tag (og:url) with the canonical URL, as recommended by Facebook et al - config.opengraph_url = true -end From 98cffd45b8c5639c5a6e7280e90833b9816278e1 Mon Sep 17 00:00:00 2001 From: gms-gs Date: Tue, 26 Nov 2024 12:55:38 +0000 Subject: [PATCH 3/8] Lint --- app/views/layouts/application.html.erb | 4 ++-- app/views/layouts/find_layout.html.erb | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index cc4da06e3a..8dbfb2d0ba 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -5,8 +5,8 @@ <%= csrf_meta_tags %> <%= csp_meta_tag %> - <%= tag.link(href: request.url.split('?').first, rel: "canonical") %> - <%= tag.meta(property: "og:url", content: request.url.split('?').first) %> + <%= tag.link(href: request.url.split('?').first, rel: 'canonical') %> + <%= tag.meta(property: 'og:url', content: request.url.split('?').first) %> <%= tag.meta(name: "viewport", content: "width=device-width, initial-scale=1") %> <%= tag.meta(property: "og:image", content: image_path("govuk-opengraph-image.png")) %> diff --git a/app/views/layouts/find_layout.html.erb b/app/views/layouts/find_layout.html.erb index cf62cbd058..8f0ff99221 100644 --- a/app/views/layouts/find_layout.html.erb +++ b/app/views/layouts/find_layout.html.erb @@ -5,8 +5,8 @@ <%= csrf_meta_tags %> <%= csp_meta_tag %> - <%= tag.link(href: request.url.split('?').first, rel: "canonical") %> - <%= tag.meta(property: "og:url", content: request.url.split('?').first) %> + <%= tag.link(href: request.url.split('?').first, rel: 'canonical') %> + <%= tag.meta(property: 'og:url', content: request.url.split('?').first) %> <%= tag.meta(name: "viewport", content: "width=device-width, initial-scale=1") %> <%= tag.meta(property: "og:image", content: image_path("govuk-opengraph-image.png")) %> From c3266f1e8f8c73adc68d9c54761df53fdf881dfb Mon Sep 17 00:00:00 2001 From: gms-gs Date: Tue, 26 Nov 2024 13:02:59 +0000 Subject: [PATCH 4/8] Ensure trailing slash --- app/views/layouts/application.html.erb | 5 +++-- app/views/layouts/find_layout.html.erb | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index 8dbfb2d0ba..b29c99845d 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -5,8 +5,9 @@ <%= csrf_meta_tags %> <%= csp_meta_tag %> - <%= tag.link(href: request.url.split('?').first, rel: 'canonical') %> - <%= tag.meta(property: 'og:url', content: request.url.split('?').first) %> + <% url = URI(request.url.split('?').first) %> + <%= tag.link(href: url.path.ends_with?('/') ? url.to_s : "#{url}/", rel: 'canonical') %> + <%= tag.meta(property: 'og:url', content: url.path.ends_with?('/') ? url.to_s : "#{url}/") %> <%= tag.meta(name: "viewport", content: "width=device-width, initial-scale=1") %> <%= tag.meta(property: "og:image", content: image_path("govuk-opengraph-image.png")) %> diff --git a/app/views/layouts/find_layout.html.erb b/app/views/layouts/find_layout.html.erb index 8f0ff99221..e95719629b 100644 --- a/app/views/layouts/find_layout.html.erb +++ b/app/views/layouts/find_layout.html.erb @@ -5,8 +5,9 @@ <%= csrf_meta_tags %> <%= csp_meta_tag %> - <%= tag.link(href: request.url.split('?').first, rel: 'canonical') %> - <%= tag.meta(property: 'og:url', content: request.url.split('?').first) %> + <% url = URI(request.url.split('?').first) %> + <%= tag.link(href: url.path.ends_with?('/') ? url.to_s : "#{url}/", rel: 'canonical') %> + <%= tag.meta(property: 'og:url', content: url.path.ends_with?('/') ? url.to_s : "#{url}/") %> <%= tag.meta(name: "viewport", content: "width=device-width, initial-scale=1") %> <%= tag.meta(property: "og:image", content: image_path("govuk-opengraph-image.png")) %> From 77422868ec52e5b2fe87cb1e519e9a5a80f56f41 Mon Sep 17 00:00:00 2001 From: gms-gs Date: Tue, 26 Nov 2024 13:13:12 +0000 Subject: [PATCH 5/8] Lint --- app/views/layouts/application.html.erb | 6 +++--- app/views/layouts/find_layout.html.erb | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index b29c99845d..14becbeaec 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -5,9 +5,9 @@ <%= csrf_meta_tags %> <%= csp_meta_tag %> - <% url = URI(request.url.split('?').first) %> - <%= tag.link(href: url.path.ends_with?('/') ? url.to_s : "#{url}/", rel: 'canonical') %> - <%= tag.meta(property: 'og:url', content: url.path.ends_with?('/') ? url.to_s : "#{url}/") %> + <% url = URI(request.url.split("?").first) %> + <%= tag.link(href: url.path.ends_with?("/") ? url.to_s : "#{url}/", rel: "canonical") %> + <%= tag.meta(property: "og:url", content: url.path.ends_with?("/") ? url.to_s : "#{url}/") %> <%= tag.meta(name: "viewport", content: "width=device-width, initial-scale=1") %> <%= tag.meta(property: "og:image", content: image_path("govuk-opengraph-image.png")) %> diff --git a/app/views/layouts/find_layout.html.erb b/app/views/layouts/find_layout.html.erb index e95719629b..21487970ca 100644 --- a/app/views/layouts/find_layout.html.erb +++ b/app/views/layouts/find_layout.html.erb @@ -5,9 +5,9 @@ <%= csrf_meta_tags %> <%= csp_meta_tag %> - <% url = URI(request.url.split('?').first) %> - <%= tag.link(href: url.path.ends_with?('/') ? url.to_s : "#{url}/", rel: 'canonical') %> - <%= tag.meta(property: 'og:url', content: url.path.ends_with?('/') ? url.to_s : "#{url}/") %> + <% url = URI(request.url.split("?").first) %> + <%= tag.link(href: url.path.ends_with?("/") ? url.to_s : "#{url}/", rel: "canonical") %> + <%= tag.meta(property: "og:url", content: url.path.ends_with?("/") ? url.to_s : "#{url}/") %> <%= tag.meta(name: "viewport", content: "width=device-width, initial-scale=1") %> <%= tag.meta(property: "og:image", content: image_path("govuk-opengraph-image.png")) %> From 2644e05c5aed330870aa36da3a5f064398e9d154 Mon Sep 17 00:00:00 2001 From: gms-gs Date: Wed, 27 Nov 2024 09:37:07 +0000 Subject: [PATCH 6/8] Add tests --- app/views/layouts/application.html.erb | 7 ++++--- app/views/layouts/find_layout.html.erb | 7 ++++--- spec/features/canonical_tags_spec.rb | 21 +++++++++++++++++++++ 3 files changed, 29 insertions(+), 6 deletions(-) create mode 100644 spec/features/canonical_tags_spec.rb diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index 14becbeaec..c84b0ffa92 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -5,9 +5,10 @@ <%= csrf_meta_tags %> <%= csp_meta_tag %> - <% url = URI(request.url.split("?").first) %> - <%= tag.link(href: url.path.ends_with?("/") ? url.to_s : "#{url}/", rel: "canonical") %> - <%= tag.meta(property: "og:url", content: url.path.ends_with?("/") ? url.to_s : "#{url}/") %> + <% canonical_url = "#{request.base_url}#{request.path}".chomp('/') + '/' %> + + <%= tag.link(href: canonical_url, rel: "canonical") %> + <%= tag.meta(property: "og:url", content: canonical_url) %> <%= tag.meta(name: "viewport", content: "width=device-width, initial-scale=1") %> <%= tag.meta(property: "og:image", content: image_path("govuk-opengraph-image.png")) %> diff --git a/app/views/layouts/find_layout.html.erb b/app/views/layouts/find_layout.html.erb index 21487970ca..5499fd4635 100644 --- a/app/views/layouts/find_layout.html.erb +++ b/app/views/layouts/find_layout.html.erb @@ -5,9 +5,10 @@ <%= csrf_meta_tags %> <%= csp_meta_tag %> - <% url = URI(request.url.split("?").first) %> - <%= tag.link(href: url.path.ends_with?("/") ? url.to_s : "#{url}/", rel: "canonical") %> - <%= tag.meta(property: "og:url", content: url.path.ends_with?("/") ? url.to_s : "#{url}/") %> + <% canonical_url = "#{request.base_url}#{request.path}".chomp('/') + '/' %> + + <%= tag.link(href: canonical_url, rel: "canonical") %> + <%= tag.meta(property: "og:url", content: canonical_url) %> <%= tag.meta(name: "viewport", content: "width=device-width, initial-scale=1") %> <%= tag.meta(property: "og:image", content: image_path("govuk-opengraph-image.png")) %> diff --git a/spec/features/canonical_tags_spec.rb b/spec/features/canonical_tags_spec.rb new file mode 100644 index 0000000000..3d931319af --- /dev/null +++ b/spec/features/canonical_tags_spec.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +require 'rails_helper' + +feature 'Canonical tags', :with_find_constraint do + scenario 'Find pages contain canonical tags' do + visit '/' + + expect(page).to have_css("link[rel='canonical'][href='http://www.example.com/']", visible: :all) + expect(page).to have_css("meta[property='og:url'][content='http://www.example.com/']", visible: :all) + end +end + +feature 'Canonical tags', :with_publish_constraint do + scenario 'Publish pages contain canonical tags' do + visit '/' + + expect(page).to have_css("link[rel='canonical'][href='http://www.example.com/sign-in/']", visible: :all) + expect(page).to have_css("meta[property='og:url'][content='http://www.example.com/sign-in/']", visible: :all) + end +end From eb2341a3c37073c4278b4285c5229c64d406bef7 Mon Sep 17 00:00:00 2001 From: gms-gs Date: Wed, 27 Nov 2024 09:49:54 +0000 Subject: [PATCH 7/8] fix linting --- app/views/layouts/application.html.erb | 2 +- app/views/layouts/find_layout.html.erb | 2 +- spec/features/canonical_tags_spec.rb | 24 +++++++++++++----------- 3 files changed, 15 insertions(+), 13 deletions(-) diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index c84b0ffa92..59be724184 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -5,7 +5,7 @@ <%= csrf_meta_tags %> <%= csp_meta_tag %> - <% canonical_url = "#{request.base_url}#{request.path}".chomp('/') + '/' %> + <% canonical_url = "#{request.base_url}#{request.path.chomp('/')}/" %> <%= tag.link(href: canonical_url, rel: "canonical") %> <%= tag.meta(property: "og:url", content: canonical_url) %> diff --git a/app/views/layouts/find_layout.html.erb b/app/views/layouts/find_layout.html.erb index 5499fd4635..f70ba8675c 100644 --- a/app/views/layouts/find_layout.html.erb +++ b/app/views/layouts/find_layout.html.erb @@ -5,7 +5,7 @@ <%= csrf_meta_tags %> <%= csp_meta_tag %> - <% canonical_url = "#{request.base_url}#{request.path}".chomp('/') + '/' %> + <% canonical_url = "#{request.base_url}#{request.path.chomp('/')}/" %> <%= tag.link(href: canonical_url, rel: "canonical") %> <%= tag.meta(property: "og:url", content: canonical_url) %> diff --git a/spec/features/canonical_tags_spec.rb b/spec/features/canonical_tags_spec.rb index 3d931319af..553091eaa4 100644 --- a/spec/features/canonical_tags_spec.rb +++ b/spec/features/canonical_tags_spec.rb @@ -2,20 +2,22 @@ require 'rails_helper' -feature 'Canonical tags', :with_find_constraint do - scenario 'Find pages contain canonical tags' do - visit '/' +feature 'Canonical tags' do + context 'when visiting Find pages', :with_find_constraint do + scenario 'Find pages contain canonical tags' do + visit '/' - expect(page).to have_css("link[rel='canonical'][href='http://www.example.com/']", visible: :all) - expect(page).to have_css("meta[property='og:url'][content='http://www.example.com/']", visible: :all) + expect(page).to have_css("link[rel='canonical'][href='http://www.example.com/']", visible: :all) + expect(page).to have_css("meta[property='og:url'][content='http://www.example.com/']", visible: :all) + end end -end -feature 'Canonical tags', :with_publish_constraint do - scenario 'Publish pages contain canonical tags' do - visit '/' + context 'when visiting Publish pages', :with_publish_constraint do + scenario 'Publish pages contain canonical tags' do + visit '/' - expect(page).to have_css("link[rel='canonical'][href='http://www.example.com/sign-in/']", visible: :all) - expect(page).to have_css("meta[property='og:url'][content='http://www.example.com/sign-in/']", visible: :all) + expect(page).to have_css("link[rel='canonical'][href='http://www.example.com/sign-in/']", visible: :all) + expect(page).to have_css("meta[property='og:url'][content='http://www.example.com/sign-in/']", visible: :all) + end end end From bb5cc634e3eddd54016caf2dec567881b31894c6 Mon Sep 17 00:00:00 2001 From: gms-gs Date: Wed, 27 Nov 2024 11:59:57 +0000 Subject: [PATCH 8/8] Update tets --- spec/features/canonical_tags_spec.rb | 94 ++++++++++++++++++++++++---- 1 file changed, 82 insertions(+), 12 deletions(-) diff --git a/spec/features/canonical_tags_spec.rb b/spec/features/canonical_tags_spec.rb index 553091eaa4..1b5f538525 100644 --- a/spec/features/canonical_tags_spec.rb +++ b/spec/features/canonical_tags_spec.rb @@ -2,22 +2,92 @@ require 'rails_helper' -feature 'Canonical tags' do - context 'when visiting Find pages', :with_find_constraint do - scenario 'Find pages contain canonical tags' do - visit '/' +feature 'Canonical tags', :with_find_constraint do + before do + given_i_have_courses + and_i_visit_the_home_page + end - expect(page).to have_css("link[rel='canonical'][href='http://www.example.com/']", visible: :all) - expect(page).to have_css("meta[property='og:url'][content='http://www.example.com/']", visible: :all) + describe 'Canonical tags on Find pages' do + scenario 'without query params' do + then_the_page_contains_canonical_tags_with_no_query_params + end + + scenario 'with query parameters' do + and_i_select_the_across_england_radio_button + then_the_page_contains_canonical_tags_without_query_params end - end - context 'when visiting Publish pages', :with_publish_constraint do - scenario 'Publish pages contain canonical tags' do - visit '/' + scenario 'when visiting a course' do + when_i_visit_the_course_page + then_the_page_contains_canonical_tags_for_a_course + end + + scenario 'when visiting a course and selecting an anchor tag' do + when_i_visit_the_course_page + and_i_click_an_anchor_tag + then_the_page_contains_canonical_tags_for_a_course + end + end - expect(page).to have_css("link[rel='canonical'][href='http://www.example.com/sign-in/']", visible: :all) - expect(page).to have_css("meta[property='og:url'][content='http://www.example.com/sign-in/']", visible: :all) + describe 'Canonical tags on Publish pages', :with_publish_constraint do + scenario 'Publish page contains canonical tags' do + and_i_visit_the_publish_page + then_the_publish_page_contains_canonical_tags end end + + def when_i_visit_the_course_page + visit find_course_path( + provider_code: @mathematics_course.provider.provider_code, + course_code: @mathematics_course.course_code + ) + end + + def given_i_have_courses + provider = create(:provider) + @mathematics_course = create(:course, :published_postgraduate, :secondary, provider:, name: 'Mathematics', subjects: [find_or_create(:secondary_subject, :mathematics)]) + end + + def and_i_visit_the_home_page + visit '/' + end + + def and_i_visit_the_publish_page + visit '/sign-in/' + end + + def and_i_select_the_across_england_radio_button + choose 'Across England' + click_link_or_button 'Continue' + end + + def and_i_click_an_anchor_tag + click_link_or_button 'Where you will train' + end + + def then_the_page_contains_canonical_tags_for_a_course + canonical_url = "http://www.example.com/course/#{@mathematics_course.provider.provider_code}/#{@mathematics_course.course_code}/" + + link_tag = page.find("link[rel='canonical']", visible: :all) + expect(link_tag[:href]).to eq(canonical_url) + + og_url_tag = page.find("meta[property='og:url']", visible: :all) + expect(og_url_tag[:content]).to eq(canonical_url) + end + + def then_the_page_contains_canonical_tags_with_no_query_params + expect(page).to have_css("link[rel='canonical'][href='http://www.example.com/']", visible: :all) + expect(page).to have_css("meta[property='og:url'][content='http://www.example.com/']", visible: :all) + end + + def then_the_page_contains_canonical_tags_without_query_params + expect(page).to have_css("link[rel='canonical'][href='http://www.example.com/age-groups/']", visible: :all) + expect(page).to have_css("meta[property='og:url'][content='http://www.example.com/age-groups/']", visible: :all) + end + + def then_the_publish_page_contains_canonical_tags + expect(page).to have_css("link[rel='canonical'][href='http://www.example.com/sign-in/']", visible: :all) + expect(page).to have_css("meta[property='og:url'][content='http://www.example.com/sign-in/']", visible: :all) + end end