From 0570b427db9803119ad5919709627a3dd9eab5d7 Mon Sep 17 00:00:00 2001 From: Alexander Griffen Date: Fri, 12 Jan 2024 16:23:31 +0000 Subject: [PATCH 1/8] Refactor: Generalise login method on language This uses CSS selectors rather than labels, so it will work with the webpage being in other languages. Also needed to prefix route in an integration test to get the correct locale rendered, this is because the `signin_path` now doesn't specify the locale so the session object hasn't been set. --- spec/integration/alaveteli_dsl.rb | 8 ++++---- spec/integration/track_alerts_spec.rb | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/integration/alaveteli_dsl.rb b/spec/integration/alaveteli_dsl.rb index 02fce75112..8abccda7de 100644 --- a/spec/integration/alaveteli_dsl.rb +++ b/spec/integration/alaveteli_dsl.rb @@ -108,11 +108,11 @@ def using_pro_session(session_id) def login(user, **params) u = user.is_a?(User) ? user : users(user) alaveteli_session(u.id) do - visit signin_path(locale: 'en', **params) + visit signin_path(**params) within '#signin_form' do - fill_in "Your e-mail:", with: u.email - fill_in "Password:", with: "jonespassword" - click_button "Sign in" + fill_in "user_signin_email", with: u.email + fill_in "user_signin_password", with: "jonespassword" + find("input[name='commit']", visible: true).click end end u.id diff --git a/spec/integration/track_alerts_spec.rb b/spec/integration/track_alerts_spec.rb index 2fc84fd81f..305539edc3 100644 --- a/spec/integration/track_alerts_spec.rb +++ b/spec/integration/track_alerts_spec.rb @@ -73,7 +73,7 @@ other_user = FactoryBot.create(:user, locale: 'en') other_user_session = login(other_user) using_session(other_user_session) do - visit "request/#{info_request.url_title}/annotate" + visit "en/request/#{info_request.url_title}/annotate" fill_in "comment[body]", with: 'test comment' click_button 'Preview your annotation' click_button 'Post annotation' From df4fd71ad0d26796c537f76120dfc0fd0c0f18dd Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Mon, 4 Mar 2024 15:00:22 +0000 Subject: [PATCH 2/8] Add current_path_without_locale helper This will be used to strip the locale from the current URL so we always have clean URLs. --- app/helpers/link_to_helper.rb | 6 ++++++ spec/helpers/link_to_helper_spec.rb | 31 +++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/app/helpers/link_to_helper.rb b/app/helpers/link_to_helper.rb index 4c0f860745..585124d205 100755 --- a/app/helpers/link_to_helper.rb +++ b/app/helpers/link_to_helper.rb @@ -290,6 +290,12 @@ def unhappy_url(info_request = nil, options = {}) end end + def current_path_without_locale + unsafe_keys = %w[protocol host locale] + sanitized_params = params.reject { |k| unsafe_keys.include?(k) }.permit! + url_for(sanitized_params.merge(only_path: true)) + end + def current_path_with_locale(locale) unsafe_keys = %w[protocol host] sanitized_params = params.reject { |k| unsafe_keys.include?(k) }.permit! diff --git a/spec/helpers/link_to_helper_spec.rb b/spec/helpers/link_to_helper_spec.rb index 18f6e97d5d..6d742f75d1 100644 --- a/spec/helpers/link_to_helper_spec.rb +++ b/spec/helpers/link_to_helper_spec.rb @@ -222,6 +222,37 @@ end end + describe '#current_path_without_locale' do + before do + AlaveteliLocalization.set_locales('en cy', 'en') + end + + it 'removes locale from current path' do + allow(controller).to receive(:params).and_return( + ActionController::Parameters.new( + controller: 'public_body', action: 'show', + url_name: 'welsh_government', view: 'all', + locale: 'cy' + ) + ) + expect(current_path_without_locale). + to eq '/body/welsh_government' + end + + it 'ignores current protocol and host' do + allow(controller).to receive(:params).and_return( + ActionController::Parameters.new( + controller: 'public_body', action: 'show', + url_name: 'welsh_government', view: 'all', + protocol: 'http', host: 'example.com', + locale: 'cy' + ) + ) + expect(current_path_without_locale). + to eq '/body/welsh_government' + end + end + describe '#current_path_with_locale' do before do @was_routing_filter_active = RoutingFilter.active? From 392af7292875c577188cdbe0fd7f581a5bcdac52 Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Mon, 4 Mar 2024 18:42:27 +0000 Subject: [PATCH 3/8] Fix storing session locale When the locale changes we need to ensure the users session is updated. Normally this would happen using the session object but we have a middleware to strip "empty" sessions updates, this includes the locale key. This appears to be left over from an optimisation for Varnish cache. See [1] and [2]. Storing a different key makes the session update work. This is important for this feature as we will be removing the prefixed locale in the URL and we have to ensure this is set correctly. [1] https://github.com/mysociety/alaveteli/commit/fe895b0 [2] https://chris.heald.me/2011/rails-varnish-cookie-sessions-and-csrf-tokens/ --- app/controllers/application_controller.rb | 11 +++++++++-- spec/integration/cookie_stripping_spec.rb | 3 +++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 9fcdc26004..598fff0c1b 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -84,8 +84,15 @@ def set_gettext_locale params_locale, session[:locale], cookies[:locale], browser_locale ) - # set the current locale to the requested_locale - session[:locale] = locale + # set the current stored locale to the requested_locale + current_session_locale = session[:locale] + if current_session_locale != locale + session[:locale] = locale + + # we need to set something other than StripEmptySessions::STRIPPABLE_KEYS + # otherwise the cookie will be striped from the response + session[:previous_locale] = current_session_locale + end # ensure current user locale attribute is up-to-date current_user.update_column(:locale, locale) if current_user diff --git a/spec/integration/cookie_stripping_spec.rb b/spec/integration/cookie_stripping_spec.rb index f4298d9352..f51db82951 100644 --- a/spec/integration/cookie_stripping_spec.rb +++ b/spec/integration/cookie_stripping_spec.rb @@ -3,6 +3,9 @@ RSpec.describe 'when making stripping cookies' do it 'should not set a cookie when no significant session data is set' do + get '/' + expect(response.headers['Set-Cookie']).to_not be_blank + get '/country_message' expect(response.headers['Set-Cookie']).to be_blank end From 78a867708863d10c0a7b6171d54758a7527e9796 Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Fri, 1 Mar 2024 18:20:55 +0000 Subject: [PATCH 4/8] Disable URL locale prefix Update routing filter to never inject locale prefix, as well as updating fixtures and test examples which have hard-coded URLs to no longer have locale prefixes in them. This also disabled some integration tests where we look for localised content as these will need to be re-written once we handle locales differently in upcoming commits. --- lib/routing_filters.rb | 4 +-- .../subscription_mailer/payment_failed | 2 +- .../notification_mailer/daily-summary.txt | 34 +++++++++---------- .../notification_mailer/embargo_expiring.txt | 2 +- .../notification_mailer/expire_embargo.txt | 2 +- .../notification_mailer/new_response.txt | 2 +- .../notification_mailer/new_response_pro.txt | 2 +- .../files/notification_mailer/overdue.txt | 2 +- .../notification_mailer/very_overdue.txt | 2 +- spec/helpers/link_to_helper_spec.rb | 4 +-- spec/helpers/public_body_helper_spec.rb | 2 +- spec/integration/change_email_address_spec.rb | 2 +- spec/integration/errors_spec.rb | 2 +- spec/integration/localisation_spec.rb | 2 +- spec/integration/search_request_spec.rb | 2 +- spec/integration/signin_spec.rb | 2 +- spec/integration/track_alerts_spec.rb | 4 +-- spec/integration/view_request_spec.rb | 4 +-- spec/lib/alaveteli_localization_spec.rb | 5 --- .../info_request_batch_metrics_spec.rb | 2 +- .../info_requests/new.html.erb_spec.rb | 2 +- .../payment_failed.text.erb_spec.rb | 2 +- 22 files changed, 40 insertions(+), 47 deletions(-) diff --git a/lib/routing_filters.rb b/lib/routing_filters.rb index 540b53faf0..3204e960fb 100644 --- a/lib/routing_filters.rb +++ b/lib/routing_filters.rb @@ -3,9 +3,7 @@ class Conditionallyprependlocale < RoutingFilter::Locale # Override core Locale filter not to prepend locale path segment # when there's only one locale def prepend_locale?(locale) - locale && - AlaveteliLocalization.available_locales.length > 1 && - (self.class.include_default_locale? || !default_locale?(locale)) + false end # And override the generation logic to use FastGettext.locale diff --git a/spec/fixtures/alaveteli_pro/subscription_mailer/payment_failed b/spec/fixtures/alaveteli_pro/subscription_mailer/payment_failed index 14a4433722..2ee86c12a0 100644 --- a/spec/fixtures/alaveteli_pro/subscription_mailer/payment_failed +++ b/spec/fixtures/alaveteli_pro/subscription_mailer/payment_failed @@ -8,6 +8,6 @@ We’ll attempt to bill your card again over the coming days. If payment continues to fail, unfortunately your account will revert back to a free account, and you’ll no longer have access to Alaveteli Professional. -If you wish to preserve your access to Alaveteli Professional, please could you check that your card has sufficient funds, and that the card details are correct on your subscriptions page: http://test.host/en/profile/subscriptions +If you wish to preserve your access to Alaveteli Professional, please could you check that your card has sufficient funds, and that the card details are correct on your subscriptions page: http://test.host/profile/subscriptions -- the Alaveteli Professional team diff --git a/spec/fixtures/files/notification_mailer/daily-summary.txt b/spec/fixtures/files/notification_mailer/daily-summary.txt index 0ea9ab0450..f127dcac2e 100644 --- a/spec/fixtures/files/notification_mailer/daily-summary.txt +++ b/spec/fixtures/files/notification_mailer/daily-summary.txt @@ -8,7 +8,7 @@ You have a new response to the Freedom of Information request 'The cost of paper To view the response, click on the link below. -http://test.host/en/request/the_cost_of_paperclips?nocache=incoming-$THE_COST_OF_PAPERCLIPS_MOF_INCOMING_ID$#incoming-$THE_COST_OF_PAPERCLIPS_MOF_INCOMING_ID$ +http://test.host/request/the_cost_of_paperclips?nocache=incoming-$THE_COST_OF_PAPERCLIPS_MOF_INCOMING_ID$#incoming-$THE_COST_OF_PAPERCLIPS_MOF_INCOMING_ID$ Thefts of stationary @@ -19,11 +19,11 @@ You have a new response to the Freedom of Information request 'Thefts of station To view the response, click on the link below. -http://test.host/en/request/thefts_of_stationary?nocache=incoming-$THEFTS_OF_STATIONARY_MIQ_INCOMING_ID$#incoming-$THEFTS_OF_STATIONARY_MIQ_INCOMING_ID$ +http://test.host/request/thefts_of_stationary?nocache=incoming-$THEFTS_OF_STATIONARY_MIQ_INCOMING_ID$#incoming-$THEFTS_OF_STATIONARY_MIQ_INCOMING_ID$ This request will be made public on Alaveteli in the next week. If you do not wish this request to go public at that time, please click on the link below to keep it private for longer. -http://test.host/en/request/thefts_of_stationary +http://test.host/request/thefts_of_stationary Missing staplers @@ -32,7 +32,7 @@ Missing staplers What's happened: This request will be made public on Alaveteli in the next week. If you do not wish this request to go public at that time, please click on the link below to keep it private for longer. -http://test.host/en/request/missing_staplers +http://test.host/request/missing_staplers Late expenses claims @@ -41,7 +41,7 @@ Late expenses claims What's happened: Ministry of fact keeping have not replied to your FOI request Late expenses claims promptly, as normally required by law. Click on the link below to remind them to reply. -http://test.host/en/profile/sign_in?r=%2Fen%2Frequest%2F$LATE_EXPENSES_CLAIMS_MOF_URL_TITLE$%2Ffollowups%2Fnew +http://test.host/profile/sign_in?r=%2Frequest%2F$LATE_EXPENSES_CLAIMS_MOF_URL_TITLE$%2Ffollowups%2Fnew Extremely late expenses claims @@ -50,7 +50,7 @@ Extremely late expenses claims What's happened: Ministry of fact keeping have still not replied to your FOI request Extremely late expenses claims, as normally required by law. Click on the link below to tell them to reply. You might like to ask for an internal review, asking them to find out why their response to the request has been so slow. -http://test.host/en/profile/sign_in?r=%2Fen%2Frequest%2F$EXTREMELY_LATE_EXPENSES_CLAIMS_MOF_URL_TITLE$%2Ffollowups%2Fnew +http://test.host/profile/sign_in?r=%2Frequest%2F$EXTREMELY_LATE_EXPENSES_CLAIMS_MOF_URL_TITLE$%2Ffollowups%2Fnew Misdelivered letters @@ -59,7 +59,7 @@ Misdelivered letters What's happened: This request has been made public on Alaveteli. -http://test.host/en/request/misdelivered_letters +http://test.host/request/misdelivered_letters Zero hours employees @@ -72,8 +72,8 @@ Zero hours employees What's happened: 2 requests had new responses: You can see the responses with the following links: - Ministry of fact keeping: http://test.host/en/request/zero_hours_employees?nocache=incoming-$ZERO_HOURS_EMPLOYEES_MOF_INCOMING_ID$#incoming-$ZERO_HOURS_EMPLOYEES_MOF_INCOMING_ID$ - Minor infractions quango: http://test.host/en/request/zero_hours_employees_2?nocache=incoming-$ZERO_HOURS_EMPLOYEES_2_MIQ_INCOMING_ID$#incoming-$ZERO_HOURS_EMPLOYEES_2_MIQ_INCOMING_ID$ + Ministry of fact keeping: http://test.host/request/zero_hours_employees?nocache=incoming-$ZERO_HOURS_EMPLOYEES_MOF_INCOMING_ID$#incoming-$ZERO_HOURS_EMPLOYEES_MOF_INCOMING_ID$ + Minor infractions quango: http://test.host/request/zero_hours_employees_2?nocache=incoming-$ZERO_HOURS_EMPLOYEES_2_MIQ_INCOMING_ID$#incoming-$ZERO_HOURS_EMPLOYEES_2_MIQ_INCOMING_ID$ Employees caught stealing stationary @@ -85,8 +85,8 @@ Employees caught stealing stationary What's happened: 2 requests will be made public on Alaveteli in the next week. If you do not wish these requests to go public at that time, please click on the links below to keep them private for longer. - Ministry of fact keeping: http://test.host/en/request/employees_caught_stealing_statio - Minor infractions quango: http://test.host/en/request/employees_caught_stealing_statio_2 + Ministry of fact keeping: http://test.host/request/employees_caught_stealing_statio + Minor infractions quango: http://test.host/request/employees_caught_stealing_statio_2 Employee of the month awards @@ -98,8 +98,8 @@ Employee of the month awards What's happened: 2 requests have been made public on Alaveteli. - Ministry of fact keeping: http://test.host/en/request/employee_of_the_month_awards - Minor infractions quango: http://test.host/en/request/employee_of_the_month_awards_2 + Ministry of fact keeping: http://test.host/request/employee_of_the_month_awards + Minor infractions quango: http://test.host/request/employee_of_the_month_awards_2 Late FOI requests @@ -114,8 +114,8 @@ What's happened: You can see the requests and remind the bodies to respond with the following links: - Ministry of fact keeping: http://test.host/en/profile/sign_in?r=%2Fen%2Frequest%2F$LATE_FOI_REQUESTS_MOF_URL_TITLE$%2Ffollowups%2Fnew - Minor infractions quango: http://test.host/en/profile/sign_in?r=%2Fen%2Frequest%2F$LATE_FOI_REQUESTS_2_MIQ_URL_TITLE$%2Ffollowups%2Fnew + Ministry of fact keeping: http://test.host/profile/sign_in?r=%2Frequest%2F$LATE_FOI_REQUESTS_MOF_URL_TITLE$%2Ffollowups%2Fnew + Minor infractions quango: http://test.host/profile/sign_in?r=%2Frequest%2F$LATE_FOI_REQUESTS_2_MIQ_URL_TITLE$%2Ffollowups%2Fnew Ignored FOI requests @@ -130,8 +130,8 @@ What's happened: You can see the requests and tell the bodies to respond with the following links. You might like to ask for internal reviews, asking the bodies to find out why responses to the requests have been so slow. - Ministry of fact keeping: http://test.host/en/profile/sign_in?r=%2Fen%2Frequest%2F$IGNORED_FOI_REQUESTS_MOF_URL_TITLE$%2Ffollowups%2Fnew - Minor infractions quango: http://test.host/en/profile/sign_in?r=%2Fen%2Frequest%2F$IGNORED_FOI_REQUESTS_2_MIQ_URL_TITLE$%2Ffollowups%2Fnew + Ministry of fact keeping: http://test.host/profile/sign_in?r=%2Frequest%2F$IGNORED_FOI_REQUESTS_MOF_URL_TITLE$%2Ffollowups%2Fnew + Minor infractions quango: http://test.host/profile/sign_in?r=%2Frequest%2F$IGNORED_FOI_REQUESTS_2_MIQ_URL_TITLE$%2Ffollowups%2Fnew diff --git a/spec/fixtures/files/notification_mailer/embargo_expiring.txt b/spec/fixtures/files/notification_mailer/embargo_expiring.txt index 8b4a9e9a2e..b56d2a8144 100644 --- a/spec/fixtures/files/notification_mailer/embargo_expiring.txt +++ b/spec/fixtures/files/notification_mailer/embargo_expiring.txt @@ -1,5 +1,5 @@ The following request will be made public on Alaveteli in the next week. If you do not wish this request to go public at that time, please click on the link below to keep it private for longer. -http://test.host/en/request/here_is_a_character_that_needs_q +http://test.host/request/here_is_a_character_that_needs_q -- the Alaveteli team diff --git a/spec/fixtures/files/notification_mailer/expire_embargo.txt b/spec/fixtures/files/notification_mailer/expire_embargo.txt index 56bc6062b0..e769e613cd 100644 --- a/spec/fixtures/files/notification_mailer/expire_embargo.txt +++ b/spec/fixtures/files/notification_mailer/expire_embargo.txt @@ -1,5 +1,5 @@ The following request has been made public on Alaveteli. -http://test.host/en/request/here_is_a_character_that_needs_q +http://test.host/request/here_is_a_character_that_needs_q -- the Alaveteli team diff --git a/spec/fixtures/files/notification_mailer/new_response.txt b/spec/fixtures/files/notification_mailer/new_response.txt index 06a3968677..f6e43e4a8f 100644 --- a/spec/fixtures/files/notification_mailer/new_response.txt +++ b/spec/fixtures/files/notification_mailer/new_response.txt @@ -2,7 +2,7 @@ You have a new response to the Freedom of Information request 'Here is a charact To view the response, click on the link below. -http://test.host/en/request/here_is_a_character_that_needs_q?nocache=incoming-INCOMING_MESSAGE_ID#incoming-INCOMING_MESSAGE_ID +http://test.host/request/here_is_a_character_that_needs_q?nocache=incoming-INCOMING_MESSAGE_ID#incoming-INCOMING_MESSAGE_ID When you get there, please update the status to say if the response contains any useful information. diff --git a/spec/fixtures/files/notification_mailer/new_response_pro.txt b/spec/fixtures/files/notification_mailer/new_response_pro.txt index b7e16cdea3..e0cec82b0c 100644 --- a/spec/fixtures/files/notification_mailer/new_response_pro.txt +++ b/spec/fixtures/files/notification_mailer/new_response_pro.txt @@ -2,7 +2,7 @@ You have a new response to the Freedom of Information request 'Here is a charact To view the response, click on the link below. -http://test.host/en/profile/sign_in?r=http%3A%2F%2Ftest.host%2Fen%2Frequest%2Fhere_is_a_character_that_needs_q%3Fnocache%3Dincoming-INCOMING_MESSAGE_ID%23incoming-INCOMING_MESSAGE_ID +http://test.host/profile/sign_in?r=http%3A%2F%2Ftest.host%2Frequest%2Fhere_is_a_character_that_needs_q%3Fnocache%3Dincoming-INCOMING_MESSAGE_ID%23incoming-INCOMING_MESSAGE_ID When you get there, please update the status to say if the response contains any useful information. diff --git a/spec/fixtures/files/notification_mailer/overdue.txt b/spec/fixtures/files/notification_mailer/overdue.txt index d849554ab1..bab21877cd 100644 --- a/spec/fixtures/files/notification_mailer/overdue.txt +++ b/spec/fixtures/files/notification_mailer/overdue.txt @@ -4,7 +4,7 @@ They have not replied to your FOI request Here is a character that needs quoting Click on the link below to send a message to Test public body reminding them to reply to your request. -http://test.host/en/profile/sign_in?r=%2Fen%2Frequest%2FINFO_REQUEST_URL_TITLE%2Ffollowups%2Fnew +http://test.host/profile/sign_in?r=%2Frequest%2FINFO_REQUEST_URL_TITLE%2Ffollowups%2Fnew -- the Alaveteli team diff --git a/spec/fixtures/files/notification_mailer/very_overdue.txt b/spec/fixtures/files/notification_mailer/very_overdue.txt index 2df84ef4b8..3a2b351976 100644 --- a/spec/fixtures/files/notification_mailer/very_overdue.txt +++ b/spec/fixtures/files/notification_mailer/very_overdue.txt @@ -4,6 +4,6 @@ They have still not replied to your FOI request Here is a character that needs q Click on the link below to send a message to Test public body telling them to reply to your request. You might like to ask for an internal review, asking them to find out why their response to the request has been so slow. -http://test.host/en/profile/sign_in?r=%2Fen%2Frequest%2FINFO_REQUEST_URL_TITLE%2Ffollowups%2Fnew +http://test.host/profile/sign_in?r=%2Frequest%2FINFO_REQUEST_URL_TITLE%2Ffollowups%2Fnew -- the Alaveteli team diff --git a/spec/helpers/link_to_helper_spec.rb b/spec/helpers/link_to_helper_spec.rb index 6d742f75d1..b2ff253f64 100644 --- a/spec/helpers/link_to_helper_spec.rb +++ b/spec/helpers/link_to_helper_spec.rb @@ -272,7 +272,7 @@ url_name: 'welsh_government', view: 'all' ) ) - expect(current_path_with_locale('cy')).to eq '/cy/body/welsh_government' + expect(current_path_with_locale('cy')).to eq '/body/welsh_government' end it 'ignores current protocol and host' do @@ -283,7 +283,7 @@ protocol: 'http', host: 'example.com' ) ) - expect(current_path_with_locale('cy')).to eq '/cy/body/welsh_government' + expect(current_path_with_locale('cy')).to eq '/body/welsh_government' end end diff --git a/spec/helpers/public_body_helper_spec.rb b/spec/helpers/public_body_helper_spec.rb index 3ec6f3a062..c025fff238 100644 --- a/spec/helpers/public_body_helper_spec.rb +++ b/spec/helpers/public_body_helper_spec.rb @@ -133,7 +133,7 @@ ) public_body = FactoryBot.create(:public_body, tag_string: 'spec') - anchor = %Q(Spec category) + anchor = %Q(Spec category) AlaveteliLocalization.with_locale(:es) do expect(type_of_authority(public_body)).to eq(anchor) end diff --git a/spec/integration/change_email_address_spec.rb b/spec/integration/change_email_address_spec.rb index 58783a4c65..7a4b265e4a 100644 --- a/spec/integration/change_email_address_spec.rb +++ b/spec/integration/change_email_address_spec.rb @@ -20,7 +20,7 @@ # Check confirmation URL works visit confirmation_url_from_email - expect(page).to have_current_path("/en/user/#{user.url_name}") + expect(page).to have_current_path("/user/#{user.url_name}") expect(page).to have_content('You have now changed your email address') user.reload expect(user.email).to eq('newbob@localhost') diff --git a/spec/integration/errors_spec.rb b/spec/integration/errors_spec.rb index 7254826b68..01eaaeb40c 100644 --- a/spec/integration/errors_spec.rb +++ b/spec/integration/errors_spec.rb @@ -82,7 +82,7 @@ it 'should assign the locale for the general/exception_caught template' do allow(InfoRequest).to receive(:find_by_url_title!).and_raise("An example error") - get "/es/request/example" + get "/request/example?locale=es" expect(response).to render_template('general/exception_caught') expect(response.body).to match('Lo sentimos, hubo un problema procesando esta página') end diff --git a/spec/integration/localisation_spec.rb b/spec/integration/localisation_spec.rb index ae40329d50..0d678ad6f1 100644 --- a/spec/integration/localisation_spec.rb +++ b/spec/integration/localisation_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -RSpec.describe "when generating urls" do +RSpec.xdescribe "when generating urls" do before do @home_link_regex = /href=".*\/en\// end diff --git a/spec/integration/search_request_spec.rb b/spec/integration/search_request_spec.rb index 61425c184f..3da63b5902 100644 --- a/spec/integration/search_request_spec.rb +++ b/spec/integration/search_request_spec.rb @@ -15,7 +15,7 @@ it "should redirect requests with search in query string to URL-based page" do get '/search/all?query=bob' - expect(response).to redirect_to "/en/search/bob/all" + expect(response).to redirect_to "/search/bob/all" end it "should correctly execute simple search" do diff --git a/spec/integration/signin_spec.rb b/spec/integration/signin_spec.rb index 2ceac845ba..87589a8874 100644 --- a/spec/integration/signin_spec.rb +++ b/spec/integration/signin_spec.rb @@ -8,7 +8,7 @@ def try_login(user, options = {}) default_options = { email: user.email, password: 'jonespassword' } options = default_options.merge(options) - login_url = 'en/profile/sign_in' + login_url = '/profile/sign_in' login_url += "?r=#{options[:redirect]}" if options[:redirect] visit login_url within '#signin_form' do diff --git a/spec/integration/track_alerts_spec.rb b/spec/integration/track_alerts_spec.rb index 305539edc3..9215116832 100644 --- a/spec/integration/track_alerts_spec.rb +++ b/spec/integration/track_alerts_spec.rb @@ -21,7 +21,7 @@ other_user = FactoryBot.create(:user) other_user_session = login(other_user) using_session(other_user_session) do - visit "en/annotate/request/#{info_request.url_title}" + visit "annotate/request/#{info_request.url_title}" fill_in "comment[body]", with: 'test comment' click_button 'Preview your annotation' click_button 'Post annotation' @@ -61,7 +61,7 @@ expect(deliveries.size).to eq(0) end - it "should send localised alerts" do + xit "should send localised alerts" do info_request = FactoryBot.create(:info_request) user = FactoryBot.create(:user, last_daily_track_email: 3.days.ago, locale: 'es') diff --git a/spec/integration/view_request_spec.rb b/spec/integration/view_request_spec.rb index 0d35264dfe..daf804de07 100644 --- a/spec/integration/view_request_spec.rb +++ b/spec/integration/view_request_spec.rb @@ -17,7 +17,7 @@ it "should not make endlessly recursive JSON s" do using_session(@unregistered) do browse_request("#{@info_request.url_title}?unfold=1") - expected_link = "/en/request/#{@info_request.url_title}.json?unfold=1" + expected_link = "/request/#{@info_request.url_title}.json?unfold=1" expect(page).to have_css("head link[href='#{expected_link}']", visible: false) expect(page).not_to have_css("head link[href='#{expected_link}.json']", @@ -51,7 +51,7 @@ ) rebuild_raw_emails(info_request) - attachment_url = "/es/request/#{info_request.url_title}/response/" \ + attachment_url = "/request/#{info_request.url_title}/response/" \ "#{incoming_message.id}/attach/#{attachment.url_part_number}/" \ "#{attachment.filename}" using_session(non_owner) { visit(attachment_url) } diff --git a/spec/lib/alaveteli_localization_spec.rb b/spec/lib/alaveteli_localization_spec.rb index 0c83416456..9e7622bd26 100644 --- a/spec/lib/alaveteli_localization_spec.rb +++ b/spec/lib/alaveteli_localization_spec.rb @@ -125,11 +125,6 @@ end end - it 'sets the locales for the custom routing filter' do - expect(RoutingFilter::Conditionallyprependlocale.locales). - to eq([:en_GB, :es]) - end - it 'handles being passed a symbol as available_locales' do AlaveteliLocalization.set_locales(:es, :es) expect(AlaveteliLocalization.available_locales).to eq(['es']) diff --git a/spec/services/info_request_batch_metrics_spec.rb b/spec/services/info_request_batch_metrics_spec.rb index c7ce97b664..bf111359ec 100644 --- a/spec/services/info_request_batch_metrics_spec.rb +++ b/spec/services/info_request_batch_metrics_spec.rb @@ -8,7 +8,7 @@ subject(:metrics) { described_class.new(batch).metrics } it 'generates info request batch metrics' do - request_url = 'http://test.host/en/alaveteli_pro/info_requests/' + + request_url = 'http://test.host/alaveteli_pro/info_requests/' + request.url_title authority_name = request.public_body.name diff --git a/spec/views/alaveteli_pro/info_requests/new.html.erb_spec.rb b/spec/views/alaveteli_pro/info_requests/new.html.erb_spec.rb index b0f434305e..eb295537cb 100644 --- a/spec/views/alaveteli_pro/info_requests/new.html.erb_spec.rb +++ b/spec/views/alaveteli_pro/info_requests/new.html.erb_spec.rb @@ -36,7 +36,7 @@ def assign_variables assign_variables render expect(rendered). - to match(/data-search-url="\/en\/alaveteli_pro\/public_bodies"/) + to match(/data-search-url="\/alaveteli_pro\/public_bodies"/) end it "includes a hidden field for the body id" do diff --git a/spec/views/alaveteli_pro/subscription_mailer/payment_failed.text.erb_spec.rb b/spec/views/alaveteli_pro/subscription_mailer/payment_failed.text.erb_spec.rb index cd1e09ced5..7e6d023fe7 100644 --- a/spec/views/alaveteli_pro/subscription_mailer/payment_failed.text.erb_spec.rb +++ b/spec/views/alaveteli_pro/subscription_mailer/payment_failed.text.erb_spec.rb @@ -6,7 +6,7 @@ before do assign(:user_name, 'Paul Pro') assign(:pro_site_name, 'Alaveteli Professional') - assign(:subscriptions_url, 'http://test.host/en/profile/subscriptions') + assign(:subscriptions_url, 'http://test.host/profile/subscriptions') render end From 6d61e39519d1bcc26b0e159f2421841c60c544db Mon Sep 17 00:00:00 2001 From: Alexander Griffen Date: Fri, 12 Jan 2024 13:42:29 +0000 Subject: [PATCH 5/8] Remove locale routing filter This commit removes the RoutingFilter which injects the locale parameter into the URL. It also removes any references to this in test setup. --- Gemfile | 1 - Gemfile.lock | 4 --- config/routes.rb | 2 -- lib/alaveteli_localization.rb | 10 ------ lib/alaveteli_localization/railtie.rb | 4 --- lib/routing_filters.rb | 48 ------------------------- spec/helpers/link_to_helper_spec.rb | 15 +++----- spec/helpers/public_body_helper_spec.rb | 2 -- spec/jobs/notify_cache_job_spec.rb | 7 ---- spec/routing/redirects_spec.rb | 16 --------- spec/spec_helper.rb | 14 -------- 11 files changed, 5 insertions(+), 118 deletions(-) diff --git a/Gemfile b/Gemfile index 6378666581..66eaf511ce 100644 --- a/Gemfile +++ b/Gemfile @@ -145,7 +145,6 @@ gem 'gettext_i18n_rails', '~> 1.12.0' gem 'gettext', '~> 3.4.9' gem 'globalize', '~> 6.3.0' gem 'locale', '~> 2.1.3' -gem 'routing-filter', '~> 0.7.0' gem 'unicode', '~> 0.4.4' gem 'unidecoder', '~> 1.1.0' gem 'money', '~> 6.16.0' diff --git a/Gemfile.lock b/Gemfile.lock index fc3c8e85ab..7b8cbfd4dc 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -446,9 +446,6 @@ GEM rexml (3.2.6) rolify (6.0.1) rotp (6.2.2) - routing-filter (0.7.0) - actionpack (>= 6.1) - activesupport (>= 6.1) rspec-activemodel-mocks (1.2.0) activemodel (>= 3.0) activesupport (>= 3.0) @@ -646,7 +643,6 @@ DEPENDENCIES redcarpet (~> 3.6.0) redis (~> 4.8.1) rolify (~> 6.0.1) - routing-filter (~> 0.7.0) rspec-activemodel-mocks (~> 1.2.0) rspec-rails (~> 6.1.1) rubocop (~> 1.62.0) diff --git a/config/routes.rb b/config/routes.rb index 86db651a05..9076e52b31 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -930,6 +930,4 @@ def matches?(request) end end #### - - filter :conditionallyprependlocale end diff --git a/lib/alaveteli_localization.rb b/lib/alaveteli_localization.rb index 2c665b4065..a8372d600e 100644 --- a/lib/alaveteli_localization.rb +++ b/lib/alaveteli_localization.rb @@ -7,7 +7,6 @@ def set_locales(available_locales, default_locale) set_fast_gettext_locales(available, default) set_i18n_locales(available, default) - set_conditionally_prepend_locale_locales(available, default) end def set_default_text_domain(name, repos) @@ -15,11 +14,6 @@ def set_default_text_domain(name, repos) FastGettext.default_text_domain = name end - def set_default_locale_urls(include_default_locale_in_urls) - RoutingFilter::Locale. - include_default_locale = include_default_locale_in_urls - end - # rubocop:disable Naming/AccessorMethodName def set_default_locale(locale) locale = Locale.parse(locale) @@ -95,10 +89,6 @@ def set_i18n_locales(available, default) I18n.locale = I18n.default_locale = default.hyphenate.to_sym end - def set_conditionally_prepend_locale_locales(available, _default) - RoutingFilter::Conditionallyprependlocale.locales = available.map(&:to_s) - end - # Parse String locales to Locale instances def parse_locales(available_locales, default_locale) [available_locales.to_s.split(/ /).map { |locale| Locale.parse(locale) }, diff --git a/lib/alaveteli_localization/railtie.rb b/lib/alaveteli_localization/railtie.rb index ad54909ec7..9fc034a1d7 100644 --- a/lib/alaveteli_localization/railtie.rb +++ b/lib/alaveteli_localization/railtie.rb @@ -21,10 +21,6 @@ class Railtie < Rails::Railtie I18n::Backend::Simple.send(:include, I18n::Backend::Fallbacks) - AlaveteliLocalization.set_default_locale_urls( - AlaveteliConfiguration.include_default_locale_in_urls - ) - if Rails.version < '7.0.0' && Rails.env.development? ## # Ideally the following would only be called in the `after_initialize` diff --git a/lib/routing_filters.rb b/lib/routing_filters.rb index 3204e960fb..1d9dc3af50 100644 --- a/lib/routing_filters.rb +++ b/lib/routing_filters.rb @@ -1,51 +1,3 @@ -module RoutingFilter - class Conditionallyprependlocale < RoutingFilter::Locale - # Override core Locale filter not to prepend locale path segment - # when there's only one locale - def prepend_locale?(locale) - false - end - - # And override the generation logic to use FastGettext.locale - # rather than I18n.locale (the latter is what rails uses - # internally and may look like `en-US`, whereas the former is - # what FastGettext and other POSIX-based systems use, and will - # look like `en_US` - def around_generate(*args) - # this is because we might get a call like forum_topics_path(forum, topic, :locale => :en) - params = args.extract_options! - # extract the passed :locale option - locale = params.delete(:locale) - if locale.nil? - # default to underscore locale when locale is nil (could also be false) - locale = AlaveteliLocalization.locale - end - unless valid_locale?(locale) - # reset to no locale when locale is not valid - locale = nil - end - args << params - - yield.tap do |result| - next unless prepend_locale?(locale) - - result.update prepend_segment(result.url, locale) - end - end - - def default_locale?(locale) - AlaveteliLocalization.default_locale?(locale) - end - - # Reset the locale pattern when the locales are set. - class << self - def locales_pattern - %r(^/(#{locales.map { |l| Regexp.escape(l.to_s) }.join('|')})(?=/|$)) - end - end - end -end - ActionDispatch::Routing::RouteSet::NamedRouteCollection::UrlHelper.class_eval do def self.optimize_helper?(_route) false diff --git a/spec/helpers/link_to_helper_spec.rb b/spec/helpers/link_to_helper_spec.rb index b2ff253f64..6caf939229 100644 --- a/spec/helpers/link_to_helper_spec.rb +++ b/spec/helpers/link_to_helper_spec.rb @@ -255,24 +255,18 @@ describe '#current_path_with_locale' do before do - @was_routing_filter_active = RoutingFilter.active? - RoutingFilter.active = true - AlaveteliLocalization.set_locales('en cy', 'en') end - after do - RoutingFilter.active = @was_routing_filter_active - end - - it 'prepends current path with new locale' do + it 'adds locale parameter to current path' do allow(controller).to receive(:params).and_return( ActionController::Parameters.new( controller: 'public_body', action: 'show', url_name: 'welsh_government', view: 'all' ) ) - expect(current_path_with_locale('cy')).to eq '/body/welsh_government' + expect(current_path_with_locale('cy')). + to eq '/body/welsh_government?locale=cy' end it 'ignores current protocol and host' do @@ -283,7 +277,8 @@ protocol: 'http', host: 'example.com' ) ) - expect(current_path_with_locale('cy')).to eq '/body/welsh_government' + expect(current_path_with_locale('cy')). + to eq '/body/welsh_government?locale=cy' end end diff --git a/spec/helpers/public_body_helper_spec.rb b/spec/helpers/public_body_helper_spec.rb index c025fff238..fb6426e348 100644 --- a/spec/helpers/public_body_helper_spec.rb +++ b/spec/helpers/public_body_helper_spec.rb @@ -124,8 +124,6 @@ context 'when in a non-default locale' do it 'creates the anchor href in the correct locale' do - # Activate the routing filter, normally turned off for helper tests - RoutingFilter.active = true FactoryBot.create( :category, :public_body, category_tag: 'spec', diff --git a/spec/jobs/notify_cache_job_spec.rb b/spec/jobs/notify_cache_job_spec.rb index 2546436833..6b3b4ff3ce 100644 --- a/spec/jobs/notify_cache_job_spec.rb +++ b/spec/jobs/notify_cache_job_spec.rb @@ -15,7 +15,6 @@ before do @old_include_default_locale_in_urls = AlaveteliConfiguration.include_default_locale_in_urls - AlaveteliLocalization.set_default_locale_urls(false) allow(AlaveteliConfiguration).to receive(:varnish_hosts). and_return(['varnish']) @@ -31,12 +30,6 @@ to_return(status: 200, body: "", headers: {}) end - after do - AlaveteliLocalization.set_default_locale_urls( - @old_include_default_locale_in_urls - ) - end - context 'when called with a request' do let(:args) { [request] } diff --git a/spec/routing/redirects_spec.rb b/spec/routing/redirects_spec.rb index 95a33443ab..d73574f02c 100644 --- a/spec/routing/redirects_spec.rb +++ b/spec/routing/redirects_spec.rb @@ -11,14 +11,6 @@ expect(response).to redirect_to('/request/the_cost_of_boring.json') end - it 'redirects numerical request routes with locales' do - get('/fr/request/105') - expect(response).to redirect_to('/fr/request/the_cost_of_boring') - - get('/en_GB/request/105') - expect(response).to redirect_to('/en_GB/request/the_cost_of_boring') - end - it 'routes numerical request member routes to URL title member routes' do get('/request/105/followups/new') expect(response).to redirect_to('/request/the_cost_of_boring/followups/new') @@ -67,12 +59,4 @@ get('/categorise/request/the_cost_of_boring') expect(response).to redirect_to('/request/the_cost_of_boring/categorise') end - - it 'redirects prefixed request routes with locales' do - get('/fr/details/request/the_cost_of_boring') - expect(response).to redirect_to('/fr/request/the_cost_of_boring/details') - - get('/en_GB/details/request/the_cost_of_boring') - expect(response).to redirect_to('/en_GB/request/the_cost_of_boring/details') - end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index f2f21ef7cd..6d45cc62a4 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -125,20 +125,6 @@ AlaveteliConfiguration.default_locale) end - # Turn routing-filter off in functional and unit tests as per - # https://github.com/svenfuchs/routing-filter/blob/master/README.markdown#testing - config.before(:each) do |example| - if [:controller, :helper, :model].include? example.metadata[:type] - RoutingFilter.active = false - end - end - - config.after(:each) do |example| - if [:controller, :helper, :model].include? example.metadata[:type] - RoutingFilter.active = true - end - end - # This section makes the garbage collector run less often to speed up tests last_gc_run = Time.now From 6c6231edde4fa218869782a124efa72915996068 Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Mon, 4 Mar 2024 12:06:00 +0000 Subject: [PATCH 6/8] Remove include_default_locale_in_urls config This is now unused without the locale routing filter. The default locale has to now come after the session locale when selection which locale to use. --- app/controllers/application_controller.rb | 7 ++----- config/general.yml-example | 13 ------------- lib/configuration.rb | 1 - script/install-as-user | 1 - spec/jobs/notify_cache_job_spec.rb | 3 --- 5 files changed, 2 insertions(+), 23 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 598fff0c1b..5e74b18420 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -72,16 +72,13 @@ def long_cache def set_gettext_locale params_locale = params[:locale] - if AlaveteliConfiguration.include_default_locale_in_urls == false - params_locale ||= AlaveteliLocalization.default_locale - end - if AlaveteliConfiguration.use_default_browser_language browser_locale = request.env['HTTP_ACCEPT_LANGUAGE'] end locale = AlaveteliLocalization.set_session_locale( - params_locale, session[:locale], cookies[:locale], browser_locale + params_locale, session[:locale], cookies[:locale], browser_locale, + AlaveteliLocalization.default_locale ) # set the current stored locale to the requested_locale diff --git a/config/general.yml-example b/config/general.yml-example index 12664cc604..630a11fbe2 100644 --- a/config/general.yml-example +++ b/config/general.yml-example @@ -180,19 +180,6 @@ DEFAULT_LOCALE: 'en' # --- USE_DEFAULT_BROWSER_LANGUAGE: true -# Normally, Alaveteli will put the locale into its URLs, like this -# www.example.com/en/body/list/all. If you don't want this behaviour whenever -# the locale is the default one, set INCLUDE_DEFAULT_LOCALE_IN_URLS to false. -# -# INCLUDE_DEFAULT_LOCALE_IN_URLS: Boolean (default: true) -# -# Examples: -# -# INCLUDE_DEFAULT_LOCALE_IN_URLS: false -# -# --- -INCLUDE_DEFAULT_LOCALE_IN_URLS: true - # Are authorities required to respond by law? # # AUTHORITY_MUST_RESPOND: Boolean (default: true) diff --git a/lib/configuration.rb b/lib/configuration.rb index d97a4cb449..00f3aef75f 100644 --- a/lib/configuration.rb +++ b/lib/configuration.rb @@ -60,7 +60,6 @@ module AlaveteliConfiguration FRONTPAGE_PUBLICBODY_EXAMPLES: '', GA_CODE: '', GEOIP_DATABASE: 'vendor/data/GeoLite2-Country.mmdb', - INCLUDE_DEFAULT_LOCALE_IN_URLS: true, INCOMING_EMAIL_DOMAIN: 'localhost', INCOMING_EMAIL_PREFIX: 'foi+', INCOMING_EMAIL_SECRET: 'dummysecret', diff --git a/script/install-as-user b/script/install-as-user index 0f9654930e..5760f98f73 100755 --- a/script/install-as-user +++ b/script/install-as-user @@ -138,7 +138,6 @@ then -e "s,^( *TIME_ZONE:).*,\\1 'Europe/London'," \ -e "s,^( *BLOG_FEED:).*,\\1 ''," \ -e "s,^( *TWITTER_USERNAME:).*,\\1 ''," \ - -e "s,^( *INCLUDE_DEFAULT_LOCALE_IN_URLS:).*,\\1 false," \ -e "s,^( *INCOMING_EMAIL_DOMAIN:).*,\\1 '$HOST'," \ -e "s,^( *INCOMING_EMAIL_PREFIX:).*,\\1 'foi+'," \ -e "s,^( *INCOMING_EMAIL_SECRET:).*,\\1 '$RANDOM_EMAIL_SECRET'," \ diff --git a/spec/jobs/notify_cache_job_spec.rb b/spec/jobs/notify_cache_job_spec.rb index 6b3b4ff3ce..eeeac09ab5 100644 --- a/spec/jobs/notify_cache_job_spec.rb +++ b/spec/jobs/notify_cache_job_spec.rb @@ -13,9 +13,6 @@ let(:user) { FactoryBot.create(:user) } before do - @old_include_default_locale_in_urls = - AlaveteliConfiguration.include_default_locale_in_urls - allow(AlaveteliConfiguration).to receive(:varnish_hosts). and_return(['varnish']) From 7e517938516eefc75fa3338a70d9b0e9ba88b36d Mon Sep 17 00:00:00 2001 From: Alexander Griffen Date: Fri, 19 Jan 2024 10:56:44 +0000 Subject: [PATCH 7/8] Add locale URL redirection When processing requests for URLs where the locale is a prefix, we now: 1. Redirect to the same URL with the locale as a parameter, this is done via routing redirects. 2. Set the locale in the session. This is done in the existing `set_gettext_locale` before action callback. 3. Finally, we redirect to the same URL without the locale at all as this information is retrieved from the session or cookies. The redirect is skipped for the `PublicBodyController#show` action as there is already redirection in place there to redirect to localised version of the `PublicBody#url_name`. --- app/controllers/application_controller.rb | 10 +- app/controllers/public_body_controller.rb | 1 + config/routes/redirects.rb | 21 +++ .../admin_public_body_controller_spec.rb | 6 +- spec/controllers/general_controller_spec.rb | 2 +- spec/controllers/help_controller_spec.rb | 2 +- .../public_body_controller_spec.rb | 36 ++--- spec/integration/errors_spec.rb | 4 +- spec/integration/localisation_spec.rb | 145 +++++++----------- spec/integration/track_alerts_spec.rb | 13 +- spec/routing/redirects_spec.rb | 28 ++++ 11 files changed, 148 insertions(+), 120 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 5e74b18420..f44aa7a050 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -13,7 +13,7 @@ class PermissionDenied < StandardError class RouteNotFound < StandardError end - before_action :set_gettext_locale + before_action :set_gettext_locale, :redirect_gettext_locale before_action :collect_locales protect_from_forgery if: :authenticated?, with: :exception @@ -81,6 +81,9 @@ def set_gettext_locale AlaveteliLocalization.default_locale ) + # set response header informing the browser what language the page is in + response.headers['Content-Language'] = I18n.locale.to_s + # set the current stored locale to the requested_locale current_session_locale = session[:locale] if current_session_locale != locale @@ -95,6 +98,11 @@ def set_gettext_locale current_user.update_column(:locale, locale) if current_user end + def redirect_gettext_locale + # redirect to remove locale params if present + redirect_to current_path_without_locale if params[:locale] + end + # Help work out which request causes RAM spike. # http://www.codeweblog.com/rails-to-monitor-the-process-of-memory-leaks-skills/ # This shows the memory use increase of the Ruby process due to the request. diff --git a/app/controllers/public_body_controller.rb b/app/controllers/public_body_controller.rb index e98e33a336..d80e3bc768 100644 --- a/app/controllers/public_body_controller.rb +++ b/app/controllers/public_body_controller.rb @@ -8,6 +8,7 @@ class PublicBodyController < ApplicationController skip_before_action :html_response, only: [:show, :list_all_csv] + skip_before_action :redirect_gettext_locale, only: :show MAX_RESULTS = 500 # TODO: tidy this up with better error messages, and a more standard infrastructure for the redirect to canonical URL diff --git a/config/routes/redirects.rb b/config/routes/redirects.rb index b92a34a72a..1fa67f1fe7 100644 --- a/config/routes/redirects.rb +++ b/config/routes/redirects.rb @@ -74,3 +74,24 @@ get '/:prefix/request/:url_title', constraints: { prefix: 'categorise' }, to: info_request_redirect + +locale_constraint = ->(request) do + path_locale = request.path.split('/')[1] + begin + locale = AlaveteliLocalization::Locale.parse(path_locale).canonicalize + available = AlaveteliConfiguration.available_locales.split(' ').map do |l| + AlaveteliLocalization::Locale.parse(l).canonicalize + end + available.include?(locale) + rescue ArgumentError + false + end +end + +get ':locale', + constraints: locale_constraint, + to: redirect('?locale=%{locale}') + +get ':locale/*path', + constraints: locale_constraint, + to: redirect('%{path}?locale=%{locale}') diff --git a/spec/controllers/admin_public_body_controller_spec.rb b/spec/controllers/admin_public_body_controller_spec.rb index ac5c5288fe..c8cf516faf 100644 --- a/spec/controllers/admin_public_body_controller_spec.rb +++ b/spec/controllers/admin_public_body_controller_spec.rb @@ -14,7 +14,7 @@ end it "searches for 'humpa' in another locale" do - get :index, params: { query: "humpa", locale: "es" } + get :index, params: { query: "humpa" }, session: { locale: "es" } expect(assigns[:public_bodies]). to eq([public_bodies(:humpadink_public_body)]) end @@ -45,7 +45,7 @@ public_body.save! end sign_in admin_user - get :show, params: { id: public_body.id, locale: "es" } + get :show, params: { id: public_body.id }, session: { locale: 'es' } expect(assigns[:public_body].name).to eq 'El Public Body' end @@ -329,7 +329,7 @@ end it "edits a public body in another locale" do - get :edit, params: { id: 3, locale: :en } + get :edit, params: { id: 3 }, session: { locale: 'en' } # When editing a body, the controller returns all available translations expect(assigns[:public_body].find_translation_by_locale("es").name). diff --git a/spec/controllers/general_controller_spec.rb b/spec/controllers/general_controller_spec.rb index 24362bd10d..21867b784a 100644 --- a/spec/controllers/general_controller_spec.rb +++ b/spec/controllers/general_controller_spec.rb @@ -143,7 +143,7 @@ describe 'when using locales' do it "should use our test PO files rather than the application one" do - get :frontpage, params: { locale: 'es' } + get :frontpage, session: { locale: 'es' } expect(response.body).to match(/XOXO/) end end diff --git a/spec/controllers/help_controller_spec.rb b/spec/controllers/help_controller_spec.rb index ee85d8fd76..95881ec827 100644 --- a/spec/controllers/help_controller_spec.rb +++ b/spec/controllers/help_controller_spec.rb @@ -74,7 +74,7 @@ end it 'should render the locale-specific template if available' do - get :contact, params: { locale: 'es' } + get :contact, session: { locale: 'es' } expect(response.body).to match('contáctenos theme one') end end diff --git a/spec/controllers/public_body_controller_spec.rb b/spec/controllers/public_body_controller_spec.rb index 399b817bd0..d3ce2ee029 100644 --- a/spec/controllers/public_body_controller_spec.rb +++ b/spec/controllers/public_body_controller_spec.rb @@ -53,15 +53,15 @@ end it "should display the body using same locale as that used in url_name" do - get :show, params: { url_name: "edfh", view: 'all', locale: "es" } + get :show, params: { url_name: "edfh", view: 'all' }, + session: { locale: 'es' } expect(response.body).to have_content("Baguette") end it 'should show public body names in the selected locale language if present for a locale with underscores' do AlaveteliLocalization.set_locales('he_IL en', 'en') - get :show, params: { url_name: 'dfh', - view: 'all', - locale: 'he_IL' } + get :show, params: { url_name: 'dfh', view: 'all' }, + session: { locale: 'he_IL' } expect(response.body).to have_content('Hebrew Humpadinking') end @@ -147,7 +147,7 @@ def make_single_language_example(locale) it "with no fallback, should only return bodies from the current locale" do @english_only = make_single_language_example :en @spanish_only = make_single_language_example :es - get :list, params: { locale: 'es' } + get :list, session: { locale: 'es' } expect(assigns[:public_bodies].include?(@english_only)).to eq(false) expect(assigns[:public_bodies].include?(@spanish_only)).to eq(true) end @@ -157,7 +157,7 @@ def make_single_language_example(locale) to receive(:public_body_list_fallback_to_default_locale). and_return(true) @english_only = make_single_language_example :en - get :list, params: { locale: 'es' } + get :list, session: { locale: 'es' } expect(assigns[:public_bodies].include?(@english_only)).to eq(true) end @@ -166,7 +166,7 @@ def make_single_language_example(locale) to receive(:public_body_list_fallback_to_default_locale). and_return(true) @spanish_only = make_single_language_example :es - get :list, params: { locale: 'es' } + get :list, session: { locale: 'es' } expect(assigns[:public_bodies].include?(@spanish_only)).to eq(true) end @@ -174,14 +174,14 @@ def make_single_language_example(locale) allow(AlaveteliConfiguration). to receive(:public_body_list_fallback_to_default_locale). and_return(true) - get :list, params: { locale: 'es' } + get :list, session: { locale: 'es' } pb_ids = assigns[:public_bodies].map(&:id) unique_pb_ids = pb_ids.uniq expect(pb_ids.sort).to eq(unique_pb_ids.sort) end it 'should show public body names in the selected locale language if present' do - get :list, params: { locale: 'es' } + get :list, session: { locale: 'es' } expect(response.body).to have_content('El Department for Humpadinking') end @@ -189,13 +189,13 @@ def make_single_language_example(locale) AlaveteliLocalization.set_locales(available_locales='en en_GB', default_locale='en') @gb_only = make_single_language_example :en_GB - get :list, params: { locale: 'en_GB' } + get :list, session: { locale: 'en_GB' } expect(response.body).to have_content(@gb_only.name) end it 'should not show the internal admin authority' do PublicBody.internal_admin_body - get :list, params: { locale: 'en' } + get :list, session: { locale: 'en' } expect(response.body).not_to have_content('Internal admin authority') end @@ -206,7 +206,7 @@ def make_single_language_example(locale) allow(AlaveteliConfiguration). to receive(:public_body_list_fallback_to_default_locale). and_return(true) - get :list, params: { locale: 'es' } + get :list, session: { locale: 'es' } parsed = Nokogiri::HTML(response.body) public_body_names = parsed.xpath '//span[@class="head"]/a/text()' public_body_names = public_body_names.map(&:to_s) @@ -215,7 +215,7 @@ def make_single_language_example(locale) it 'should show public body names in the selected locale language if present for a locale with underscores' do AlaveteliLocalization.set_locales('he_IL en', 'en') - get :list, params: { locale: 'he_IL' } + get :list, session: { locale: 'he_IL' } expect(response.body).to have_content('Hebrew Humpadinking') end @@ -248,7 +248,7 @@ def make_single_language_example(locale) with(an_instance_of(String)). and_return(true) - get :list, params: { locale: 'en_GB' } + get :list, session: { locale: 'en_GB' } expect(assigns[:public_bodies].to_sql).to include('COLLATE') end @@ -261,7 +261,7 @@ def make_single_language_example(locale) with(an_instance_of(String)). and_return(false) - get :list, params: { locale: 'unknown' } + get :list, session: { locale: 'unknown' } expect(assigns[:public_bodies].to_sql).to_not include('COLLATE') end @@ -275,7 +275,7 @@ def make_single_language_example(locale) with(an_instance_of(String)). and_return(true) - get :list, params: { locale: 'en_GB' } + get :list, session: { locale: 'en_GB' } expect(assigns[:public_bodies].to_sql).to include('COLLATE') end @@ -289,7 +289,7 @@ def make_single_language_example(locale) with(an_instance_of(String)). and_return(false) - get :list, params: { locale: 'unknown' } + get :list, session: { locale: 'unknown' } expect(assigns[:public_bodies].to_sql).to_not include('COLLATE') end @@ -429,7 +429,7 @@ def make_single_language_example(locale) FactoryBot.create(:public_body, name: "Åčçèñtéd Authority") end - get :list, params: { tag: "å", locale: 'cs' } + get :list, params: { tag: "å" }, session: { locale: 'cs' } expect(response).to render_template('list') expect(assigns[:public_bodies]).to eq([ authority ]) expect(assigns[:tag]).to eq("Å") diff --git a/spec/integration/errors_spec.rb b/spec/integration/errors_spec.rb index 01eaaeb40c..6952dd4b38 100644 --- a/spec/integration/errors_spec.rb +++ b/spec/integration/errors_spec.rb @@ -82,7 +82,9 @@ it 'should assign the locale for the general/exception_caught template' do allow(InfoRequest).to receive(:find_by_url_title!).and_raise("An example error") - get "/request/example?locale=es" + get "/es" + follow_redirect! + get "/request/example" expect(response).to render_template('general/exception_caught') expect(response.body).to match('Lo sentimos, hubo un problema procesando esta página') end diff --git a/spec/integration/localisation_spec.rb b/spec/integration/localisation_spec.rb index 0d678ad6f1..f4efd7989e 100644 --- a/spec/integration/localisation_spec.rb +++ b/spec/integration/localisation_spec.rb @@ -1,44 +1,72 @@ require 'spec_helper' -RSpec.xdescribe "when generating urls" do +RSpec.describe 'when generating URLs' do before do - @home_link_regex = /href=".*\/en\// + AlaveteliLocalization.set_locales('es en en_GB', 'es') end - it "should generate URLs that include the locale when using one that includes an underscore" do - AlaveteliLocalization.set_locales('es en_GB', 'es') - get '/en_GB' - expect(response.body).to match(/href="\/en_GB\//) + it 'sets Content-Language to default locale' do + get '/' + expect(response.headers['Content-Language']).to eq('es') end - it "returns a 404 error if passed the locale with a hyphen instead of an underscore", local_requests: false do - get '/en-GB' - expect(response.status).to eq(404) + it 'redirects to locale param with old style URL locale' do + get '/es' + expect(response).to redirect_to('/?locale=es') + follow_redirect! + expect(response).to redirect_to('/') end - it "should fall back to the language if the territory is unknown" do - AlaveteliLocalization.set_locales('es en', 'en') - get '/', headers: { 'HTTP_ACCEPT_LANGUAGE' => 'en_US' } - expect(response.body).to match(/href="\/en\//) - expect(response.body).not_to match(/href="\/en_US\//) + it 'sets the correct Content-Language via locale param' do + get '/', params: { locale: 'en' } + expect(response).to redirect_to('/') + follow_redirect! + expect(response.headers['Content-Language']).to eq('en') end - it 'falls back to the default if the requested locale is unavailable' do - get '/', params: { locale: "unknown" } - expect(response.body).to match(/href="\/en\//) - expect(response.body).not_to match(/href="\/unknown\//) + it 'sets and remember the correct Content-Language via request header' do + get '/', headers: { 'HTTP_ACCEPT_LANGUAGE' => 'en' } # set + get '/' # remember + expect(response.headers['Content-Language']).to eq('en') end - it "should generate URLs without a locale prepended when there's only one locale set" do - AlaveteliLocalization.set_locales('en', 'en') - get '/' - expect(response).not_to match(/#{@home_link_regex}/) + it 'sets correct Content-Language via underscored locale param' do + get '/', params: { locale: 'en_GB' } + expect(response).to redirect_to('/') + follow_redirect! + expect(response.headers['Content-Language']).to eq('en-GB') + end + + it 'sets correct Content-Language via hyphenated locale param' do + get '/', params: { locale: 'en-GB' } + expect(response).to redirect_to('/') + follow_redirect! + expect(response.headers['Content-Language']).to eq('en-GB') + end + + it 'falls back to the language if the territory is unknown' do + get '/', params: { locale: 'en_US' } + expect(response).to redirect_to('/') + follow_redirect! + expect(response.headers['Content-Language']).to eq('en') + end + + it 'falls back to the default if the requested locale is unknown' do + get '/', params: { locale: 'unknown' } + expect(response).to redirect_to('/') + follow_redirect! + expect(response.headers['Content-Language']).to eq('es') end context 'when handling public body requests' do before do - AlaveteliLocalization.set_locales('es en', 'en') - body = FactoryBot.create(:public_body, short_name: 'english_short') + body = FactoryBot.create(:public_body) + + AlaveteliLocalization.with_locale('en') do + body.short_name = 'english_short' + body.save! + end + AlaveteliLocalization.with_locale('es') do body.short_name = 'spanish_short' body.save! @@ -47,75 +75,14 @@ it 'should redirect requests for a public body in a locale to the canonical name in that locale' do get '/es/body/english_short' - expect(response).to redirect_to "/es/body/spanish_short" + follow_redirect! + expect(response).to redirect_to '/body/spanish_short' end it 'should remember a filter view when redirecting a public body request to the canonical name' do - AlaveteliLocalization.set_locales(available_locales='es en', default_locale='en') get '/es/body/english_short/successful' - expect(response).to redirect_to "/es/body/spanish_short/successful" - end - end - - describe 'when there is more than one locale' do - before do - AlaveteliLocalization.set_locales('es en', 'en') - end - - it "should generate URLs with a locale prepended when there's more than one locale set" do - get '/' - expect(response.body).to match @home_link_regex - end - - describe 'when using the default locale' do - before do - @default_lang_home_link = /href=".*\/en\// - @old_include_default_locale_in_urls = AlaveteliConfiguration.include_default_locale_in_urls - end - - after do - AlaveteliLocalization.set_default_locale_urls(@old_include_default_locale_in_urls) - end - - describe 'when the config value INCLUDE_DEFAULT_LOCALE_IN_URLS is false' do - before do - allow(AlaveteliConfiguration).to receive(:include_default_locale_in_urls).and_return false - AlaveteliLocalization.set_default_locale_urls(false) - end - - it 'should generate URLs without a locale prepended' do - get '/' - expect(response.body).to match(/class="current-locale">English/) - expect(response.body).not_to match(/#{@default_lang_home_link}/) - end - - describe "when the default url contains an underscore" do - it "generates URLs without a locale prepended" do - AlaveteliLocalization.set_locales('en_GB es', 'en_GB') - get '/' - expect(response.body).not_to match(/href="\/en_GB\//) - end - end - - it 'should render the front page in the default language when no locale param - is present and the session locale is not the default' do - get '/', headers: { locale: 'es' } - expect(response.body).to match(/class="current-locale">English/) - end - end - - describe 'when the config value INCLUDE_DEFAULT_LOCALE_IN_URLS is true' do - before do - allow(AlaveteliConfiguration).to receive(:include_default_locale_in_urls).and_return true - AlaveteliLocalization.set_default_locale_urls(true) - end - - it 'should generate URLs with a locale prepended' do - get '/' - expect(response.body).to match(/class="current-locale">English/) - expect(response.body).to match(/#{@default_lang_home_link}/) - end - end + follow_redirect! + expect(response).to redirect_to '/body/spanish_short/successful' end end end diff --git a/spec/integration/track_alerts_spec.rb b/spec/integration/track_alerts_spec.rb index 9215116832..37fae2127a 100644 --- a/spec/integration/track_alerts_spec.rb +++ b/spec/integration/track_alerts_spec.rb @@ -61,19 +61,20 @@ expect(deliveries.size).to eq(0) end - xit "should send localised alerts" do + it "should send localised alerts" do info_request = FactoryBot.create(:info_request) - user = FactoryBot.create(:user, last_daily_track_email: 3.days.ago, - locale: 'es') + user = FactoryBot.create(:user, last_daily_track_email: 3.days.ago) user_session = login(user) using_session(user_session) do - visit "es/request/#{info_request.url_title}/track" + visit "/es" + visit "/request/#{info_request.url_title}/track" end - other_user = FactoryBot.create(:user, locale: 'en') + other_user = FactoryBot.create(:user) other_user_session = login(other_user) using_session(other_user_session) do - visit "en/request/#{info_request.url_title}/annotate" + visit "/en" + visit "/request/#{info_request.url_title}/annotate" fill_in "comment[body]", with: 'test comment' click_button 'Preview your annotation' click_button 'Post annotation' diff --git a/spec/routing/redirects_spec.rb b/spec/routing/redirects_spec.rb index d73574f02c..74f8c32446 100644 --- a/spec/routing/redirects_spec.rb +++ b/spec/routing/redirects_spec.rb @@ -59,4 +59,32 @@ get('/categorise/request/the_cost_of_boring') expect(response).to redirect_to('/request/the_cost_of_boring/categorise') end + + it 'redirects locale paths to locale parameter' do + get('/fr') + expect(response).to redirect_to('/?locale=fr') + + get('/en_GB') + expect(response).to redirect_to('/?locale=en_GB') + + get('/fr/help/about') + expect(response).to redirect_to('/help/about?locale=fr') + + get('/en_GB/help/about') + expect(response).to redirect_to('/help/about?locale=en_GB') + end + + it 'redirects to remove locale parameter' do + get('/?locale=fr') + expect(response).to redirect_to('/') + + get('/?locale=en_GB') + expect(response).to redirect_to('/') + + get('/help/about?locale=fr') + expect(response).to redirect_to('/help/about') + + get('/help/about?locale=en_GB') + expect(response).to redirect_to('/help/about') + end end From 11cbe40399ee6ce94124056815cfdfd5e860808f Mon Sep 17 00:00:00 2001 From: Alexander Griffen Date: Mon, 26 Feb 2024 14:47:32 +0000 Subject: [PATCH 8/8] Update changelog --- doc/CHANGES.md | 1 + 1 file changed, 1 insertion(+) diff --git a/doc/CHANGES.md b/doc/CHANGES.md index 5fa927762d..6cdbfa4b9e 100644 --- a/doc/CHANGES.md +++ b/doc/CHANGES.md @@ -2,6 +2,7 @@ ## Highlighted Features +* Remove locale prefixes from URLs (Alexander Griffen, Graeme Porteous) * Fix missing headers when exporting Project data (Gareth Rees) * Reduce amount of storage related background jobs (Graeme Porteous) * Add automatic parsing of emails contain Excel spreadsheets (Graeme Porteous)