From e6957f062e7a65296b6668a27e570126d74cba04 Mon Sep 17 00:00:00 2001 From: oswaldquek Date: Fri, 24 Mar 2017 11:44:50 +0000 Subject: [PATCH 1/4] TT-246: Fix feedback page routing when switching to welsh This commit fixes the following issue: 1. Start at verify product page 2. Click link to send feedback 3. At feedback page, change page to be in Welsh 4. Enter data and send feedback 5. User sees "Session timed out" instead of "Return to the GOV.UK product page" On investigation it appears this issue isn't just related to TT-246 but for all feedback pages in general. When the user goes to a feedback page there is a feedback source query parameter in the URL but this feedback source disappears when the language is changed. To fix this, this commit introduces a feedback_source_query_param method in application_helper which is made use of in app/views/shared/_available_languages.html.erb. This doesn't break the language link on non-feedback pages as the url_for method doesn't place a feedback-source query param if none exists. @oswaldquek --- app/helpers/application_helper.rb | 4 ++++ app/views/feedback_sent/index.html.erb | 2 +- app/views/shared/_available_languages.html.erb | 4 ++-- config/locales/cy.yml | 1 + config/locales/en.yml | 1 + 5 files changed, 9 insertions(+), 3 deletions(-) diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 4b98b9fd2..373b7a81a 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -11,6 +11,10 @@ def hide_from_search_engine? !content_for(:show_to_search_engine) end + def feedback_source_query_param + Rack::Utils.parse_nested_query(request.query_string)['feedback-source'] + end + def feedback_source content_for(:feedback_source) || "" end diff --git a/app/views/feedback_sent/index.html.erb b/app/views/feedback_sent/index.html.erb index 3b3bc8a01..b41c7795d 100644 --- a/app/views/feedback_sent/index.html.erb +++ b/app/views/feedback_sent/index.html.erb @@ -10,7 +10,7 @@

<%if @from_product_page%> - <%= link_to 'Return to the GOV.UK Verify product page', @link_back %> + <%= link_to t('hub.feedback_sent.product_page'), @link_back %> <% elsif @session_valid %> <%= link_to t('hub.feedback_sent.session_valid_link'), @link_back %> <% else %> diff --git a/app/views/shared/_available_languages.html.erb b/app/views/shared/_available_languages.html.erb index 78ad34957..62ab23817 100644 --- a/app/views/shared/_available_languages.html.erb +++ b/app/views/shared/_available_languages.html.erb @@ -5,14 +5,14 @@ <% if params[:locale] == 'en' %> English <% else %> - <%= link_to 'English', url_for(locale: 'en'), lang: 'en', hreflang: 'en' %> + <%= link_to 'English', url_for(locale: 'en', :'feedback-source' => feedback_source_query_param), lang: 'en', hreflang: 'en' %> <% end %>

  • <% if params[:locale] == 'cy' %> Cymraeg <% else %> - <%= link_to 'Cymraeg', url_for(locale: 'cy'), lang: 'cy', hreflang: 'cy' %> + <%= link_to 'Cymraeg', url_for(locale: 'cy', :'feedback-source' => feedback_source_query_param), lang: 'cy', hreflang: 'cy' %> <% end %>
    • diff --git a/config/locales/cy.yml b/config/locales/cy.yml index c92700bf5..0e74000e3 100644 --- a/config/locales/cy.yml +++ b/config/locales/cy.yml @@ -362,6 +362,7 @@ cy: session_valid_link: Dychwelyd i GOV.UK Verify session_not_valid: Mae eich sesiwn wedi’i amseru allan. session_not_valid_link: ddechrau eto + product_page: Dychwelyd i dudalen GOV.UK Verify certified_company_unavailable: title: Nid yw'r cwmni ardystiedig hwn ar gael sorry: Mae'n ddrwg gennym, nid yw %{display_name} ar gael. diff --git a/config/locales/en.yml b/config/locales/en.yml index c6bff3a0e..b37db47f6 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -356,6 +356,7 @@ en: session_valid_link: Return to GOV.UK Verify session_not_valid: Your session has timed out. session_not_valid_link: start again + product_page: Return to the GOV.UK Verify product page certified_company_unavailable: title: This certified company is unavailable sorry: Sorry, %{display_name} is not available. From a5ecdfef02a0238d3973793a37766ff24ad75907 Mon Sep 17 00:00:00 2001 From: oswaldquek Date: Tue, 28 Mar 2017 16:44:43 +0100 Subject: [PATCH 2/4] TT-246: Some fixes to the feedback page - Sanitise feedback-source query param by making sure its in the FeedbackSourceMapper - /feedback without a feedback-source query param should be allowed as user support may direct users wanting to give feedback to the /feedback link - User should be directed back to the /start page if coming from the /404 (feedback-source "ERROR_PAGE") - Add a test to give us confidence that users will still be directed back to the Verify product page if the email fails to send on first attempt. This test is important because it proves relying on Rails' flash['feedback-source'] works even if the feedback-source query parameter has disappeared. @oswaldquek @joncrawf @sgreensmith --- app/controllers/feedback_controller.rb | 9 +++++++- app/models/feedback_source_mapper.rb | 5 +++++ .../user_submits_feedback_page_spec.rb | 22 +++++++++++++++++++ .../user_visits_feedback_page_spec.rb | 6 +++++ 4 files changed, 41 insertions(+), 1 deletion(-) diff --git a/app/controllers/feedback_controller.rb b/app/controllers/feedback_controller.rb index bc309db73..6d8fbabbc 100644 --- a/app/controllers/feedback_controller.rb +++ b/app/controllers/feedback_controller.rb @@ -4,7 +4,14 @@ class FeedbackController < ApplicationController def index @form = FeedbackForm.new({}) flash['feedback_referer'] = request.referer - flash['feedback_source'] = params['feedback-source'] + feedback_source = params['feedback-source'] + if feedback_source.nil? + render + elsif FEEDBACK_SOURCE_MAPPER.is_feedback_source_valid(feedback_source) + flash['feedback_source'] = feedback_source + else + render 'errors/404', status: 400 + end end def submit diff --git a/app/models/feedback_source_mapper.rb b/app/models/feedback_source_mapper.rb index bcb166775..579d547d7 100644 --- a/app/models/feedback_source_mapper.rb +++ b/app/models/feedback_source_mapper.rb @@ -9,6 +9,7 @@ def initialize(product_page_url) 'CERTIFIED_COMPANY_UNAVAILABLE_PAGE' => 'choose_a_certified_company', 'CONFIRM_YOUR_IDENTITY' => 'confirm_your_identity', 'CONFIRMATION_PAGE' => 'confirmation', + 'ERROR_PAGE' => 'start', 'FAILED_REGISTRATION_PAGE' => 'failed_registration', 'FAILED_SIGN_IN_PAGE' => 'failed_sign_in', 'CYCLE_3_PAGE' => 'further_information', @@ -34,6 +35,10 @@ def initialize(product_page_url) }.freeze end + def is_feedback_source_valid(feedback_source) + @page_to_source_mappings[feedback_source].nil? ? false : true + end + def page_from_source(feedback_source, locale) route_name = route_name_from(feedback_source) if route_name =~ /https?:.*/ diff --git a/spec/features/user_submits_feedback_page_spec.rb b/spec/features/user_submits_feedback_page_spec.rb index f4bb6910a..52227a288 100644 --- a/spec/features/user_submits_feedback_page_spec.rb +++ b/spec/features/user_submits_feedback_page_spec.rb @@ -133,5 +133,27 @@ expect(page).to have_css 'html[lang=cy]' expect(page).to have_link I18n.t('hub.feedback_sent.session_valid_link', locale: :cy), href: select_documents_cy_path end + + it 'should be able to direct user back to the Verify product page if email could not be sent on first attempt' do + visit '/feedback?feedback-source=PRODUCT_PAGE' + + fill_in 'feedback_form_what', with: 'Verify my identity' + fill_in 'feedback_form_details', with: 'Some details' + choose 'feedback_form_reply_false', allow_label_click: true + + allow_any_instance_of(FeedbackService).to receive(:submit!).and_return(false) + + click_button I18n.t('hub.feedback.send_message') + + expect(page).to have_current_path(feedback_path) + expect(page).to have_content 'Verify my identity' + expect(page).to have_content 'Some details' + + allow_any_instance_of(FeedbackService).to receive(:submit!).and_return(true) + + click_button I18n.t('hub.feedback.send_message') + expect(page).to have_title(I18n.t('hub.feedback_sent.title')) + expect(page).to have_link 'Return to the GOV.UK Verify product page', href: 'https://govuk-verify.cloudapps.digital/' + end end end diff --git a/spec/features/user_visits_feedback_page_spec.rb b/spec/features/user_visits_feedback_page_spec.rb index 92f050759..868fab7f5 100644 --- a/spec/features/user_visits_feedback_page_spec.rb +++ b/spec/features/user_visits_feedback_page_spec.rb @@ -17,6 +17,12 @@ expect(page).to have_link 'Return to the GOV.UK Verify product page', href: 'https://govuk-verify.cloudapps.digital/' end + it 'should display and something went wrong if the feedback source is not valid' do + visit '/feedback?feedback-source=something_not_valid' + + expect(page).to have_content('This page can’t be found') + end + it 'should show errors for all input fields when missing input', js: true do visit feedback_path expect(page).to have_title(I18n.t('hub.feedback.title')) From 93bcdce2f1ebf603403ce102305a805aeba52e36 Mon Sep 17 00:00:00 2001 From: oswaldquek Date: Wed, 29 Mar 2017 17:22:04 +0100 Subject: [PATCH 3/4] TT-246: Use flash to preserve the feedback-source @oswaldquek @joncrawf --- app/controllers/feedback_controller.rb | 3 +- app/helpers/application_helper.rb | 4 -- .../shared/_available_languages.html.erb | 4 +- .../user_submits_feedback_page_spec.rb | 45 ++++++++++++------- 4 files changed, 34 insertions(+), 22 deletions(-) diff --git a/app/controllers/feedback_controller.rb b/app/controllers/feedback_controller.rb index 6d8fbabbc..adb38645e 100644 --- a/app/controllers/feedback_controller.rb +++ b/app/controllers/feedback_controller.rb @@ -4,7 +4,8 @@ class FeedbackController < ApplicationController def index @form = FeedbackForm.new({}) flash['feedback_referer'] = request.referer - feedback_source = params['feedback-source'] + feedback_source = params['feedback-source'].nil? ? flash['feedback_source'] : params['feedback-source'] + flash['feedback_source'] = feedback_source if feedback_source.nil? render elsif FEEDBACK_SOURCE_MAPPER.is_feedback_source_valid(feedback_source) diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 373b7a81a..4b98b9fd2 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -11,10 +11,6 @@ def hide_from_search_engine? !content_for(:show_to_search_engine) end - def feedback_source_query_param - Rack::Utils.parse_nested_query(request.query_string)['feedback-source'] - end - def feedback_source content_for(:feedback_source) || "" end diff --git a/app/views/shared/_available_languages.html.erb b/app/views/shared/_available_languages.html.erb index 62ab23817..78ad34957 100644 --- a/app/views/shared/_available_languages.html.erb +++ b/app/views/shared/_available_languages.html.erb @@ -5,14 +5,14 @@ <% if params[:locale] == 'en' %> English <% else %> - <%= link_to 'English', url_for(locale: 'en', :'feedback-source' => feedback_source_query_param), lang: 'en', hreflang: 'en' %> + <%= link_to 'English', url_for(locale: 'en'), lang: 'en', hreflang: 'en' %> <% end %>
  • <% if params[:locale] == 'cy' %> Cymraeg <% else %> - <%= link_to 'Cymraeg', url_for(locale: 'cy', :'feedback-source' => feedback_source_query_param), lang: 'cy', hreflang: 'cy' %> + <%= link_to 'Cymraeg', url_for(locale: 'cy'), lang: 'cy', hreflang: 'cy' %> <% end %>
    • diff --git a/spec/features/user_submits_feedback_page_spec.rb b/spec/features/user_submits_feedback_page_spec.rb index 52227a288..1f2db52bd 100644 --- a/spec/features/user_submits_feedback_page_spec.rb +++ b/spec/features/user_submits_feedback_page_spec.rb @@ -58,6 +58,28 @@ end end + it 'should be able to direct user back to the Verify product page if email could not be sent on first attempt' do + visit '/feedback?feedback-source=PRODUCT_PAGE' + + fill_in 'feedback_form_what', with: 'Verify my identity' + fill_in 'feedback_form_details', with: 'Some details' + choose 'feedback_form_reply_false', allow_label_click: true + + allow_any_instance_of(FeedbackService).to receive(:submit!).and_return(false) + + click_button I18n.t('hub.feedback.send_message') + + expect(page).to have_current_path(feedback_path) + expect(page).to have_content 'Verify my identity' + expect(page).to have_content 'Some details' + + allow_any_instance_of(FeedbackService).to receive(:submit!).and_return(true) + + click_button I18n.t('hub.feedback.send_message') + expect(page).to have_title(I18n.t('hub.feedback_sent.title')) + expect(page).to have_link 'Return to the GOV.UK Verify product page', href: 'https://govuk-verify.cloudapps.digital/' + end + context 'user session valid' do it 'should show user link back to start page' do set_session_and_session_cookies! @@ -134,26 +156,19 @@ expect(page).to have_link I18n.t('hub.feedback_sent.session_valid_link', locale: :cy), href: select_documents_cy_path end - it 'should be able to direct user back to the Verify product page if email could not be sent on first attempt' do - visit '/feedback?feedback-source=PRODUCT_PAGE' + it 'should be able to direct user back to the relevant page if user switches to Welsh on the feedback page' do + set_session_and_session_cookies! + visit '/feedback?feedback-source=START_PAGE' + + click_link 'Cymraeg' + expect(page).to have_current_path('/adborth') fill_in 'feedback_form_what', with: 'Verify my identity' fill_in 'feedback_form_details', with: 'Some details' choose 'feedback_form_reply_false', allow_label_click: true - allow_any_instance_of(FeedbackService).to receive(:submit!).and_return(false) - - click_button I18n.t('hub.feedback.send_message') - - expect(page).to have_current_path(feedback_path) - expect(page).to have_content 'Verify my identity' - expect(page).to have_content 'Some details' - - allow_any_instance_of(FeedbackService).to receive(:submit!).and_return(true) - - click_button I18n.t('hub.feedback.send_message') - expect(page).to have_title(I18n.t('hub.feedback_sent.title')) - expect(page).to have_link 'Return to the GOV.UK Verify product page', href: 'https://govuk-verify.cloudapps.digital/' + click_button I18n.t('hub.feedback.send_message', locale: :cy) + expect(page).to have_link I18n.t('hub.feedback_sent.session_valid_link', locale: :cy), href: '/dechrau' end end end From 1545641751dcee78431dc66e67897bfa1ecdef00 Mon Sep 17 00:00:00 2001 From: oswaldquek Date: Fri, 31 Mar 2017 12:48:45 +0100 Subject: [PATCH 4/4] TT-246: clean up code @oswaldquek --- app/controllers/feedback_controller.rb | 1 - app/models/feedback_source_mapper.rb | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/app/controllers/feedback_controller.rb b/app/controllers/feedback_controller.rb index adb38645e..e5dc0f9bf 100644 --- a/app/controllers/feedback_controller.rb +++ b/app/controllers/feedback_controller.rb @@ -5,7 +5,6 @@ def index @form = FeedbackForm.new({}) flash['feedback_referer'] = request.referer feedback_source = params['feedback-source'].nil? ? flash['feedback_source'] : params['feedback-source'] - flash['feedback_source'] = feedback_source if feedback_source.nil? render elsif FEEDBACK_SOURCE_MAPPER.is_feedback_source_valid(feedback_source) diff --git a/app/models/feedback_source_mapper.rb b/app/models/feedback_source_mapper.rb index 579d547d7..11109e1aa 100644 --- a/app/models/feedback_source_mapper.rb +++ b/app/models/feedback_source_mapper.rb @@ -36,7 +36,7 @@ def initialize(product_page_url) end def is_feedback_source_valid(feedback_source) - @page_to_source_mappings[feedback_source].nil? ? false : true + @page_to_source_mappings.has_key?(feedback_source) end def page_from_source(feedback_source, locale)