From 9eed7e7902c80f409ea87c529b9887a0fdecc3ae Mon Sep 17 00:00:00 2001
From: Jeremy Fleischman <jeremyfleischman@gmail.com>
Date: Thu, 14 Jun 2018 12:50:34 -0700
Subject: [PATCH] Preserve the url hash when changing locales.

To implement this, I had to write a custom javascript handler to handle
switching locales. Previously, this was handled for us via Rails's
unobtrusive javascript, but I could not find any way to add in
additional url parameters to the PATCH request the unobtrusive
javascript was generating for us. There's actually an open PR from 2013
to implement support for this here: https://github.com/rails/jquery-ujs/pull/307.

I also rewrote the set_locale_spec to enable javascript and be more like
what I believe a feature spec should be. I actually don't understand how
the spec was even passing before, I would expect it to fail because it
was sending a GET request, and we don't have a handler for GET request
to update the locale. Maybe rspec/capybara/poltergiest has baked in
support for unobtrusive javascript so that the right thing will happen
even if you don't have javascript enabled?? That sounds crazy, but it's
the only thing I can think of. Regardless of how this was working
before, I do believe that the new specs are better.

This fixes #2962.
---
 .../app/assets/javascripts/application.js     | 20 +++++++++
 .../assets/stylesheets/navbar-static-top.scss |  2 +-
 .../app/controllers/application_controller.rb |  2 +-
 .../app/views/layouts/_navigation.html.erb    |  4 +-
 .../application_controller_spec.rb            | 12 ++++++
 WcaOnRails/spec/features/set_locale_spec.rb   | 43 +++++++++++--------
 WcaOnRails/spec/rails_helper.rb               |  6 +++
 7 files changed, 68 insertions(+), 21 deletions(-)

diff --git a/WcaOnRails/app/assets/javascripts/application.js b/WcaOnRails/app/assets/javascripts/application.js
index 5d3edf2035..03798ac552 100644
--- a/WcaOnRails/app/assets/javascripts/application.js
+++ b/WcaOnRails/app/assets/javascripts/application.js
@@ -379,3 +379,23 @@ $(function() {
     $(this).text(formatted);
   });
 });
+
+// Handler for locale changes.
+$(function() {
+  $('#locale-selector').on('click', 'a', function(e) {
+    e.preventDefault();
+    e.stopPropagation();
+
+    // More or less copied from
+    // https://github.com/rails/jquery-ujs/blob/9e805c90c8cfc57b39967052e1e9013ccb318cf8/src/rails.js#L215.
+    var csrfToken = $('meta[name=csrf-token]').attr('content');
+    var csrfParam = $('meta[name=csrf-param]').attr('content');
+    var form = $('<form method="post" action="' + this.href + '"></form>');
+    var metadataInput = '<input name="_method" value="patch" type="hidden" />';
+    metadataInput += '<input name="' + csrfParam + '" value="' + csrfToken + '" type="hidden" />';
+    metadataInput += '<input name="current_url" value="' + window.location.toString() + '" type="hidden" />';
+
+    form.hide().append(metadataInput).appendTo('body');
+    form.submit();
+  });
+});
diff --git a/WcaOnRails/app/assets/stylesheets/navbar-static-top.scss b/WcaOnRails/app/assets/stylesheets/navbar-static-top.scss
index 7eec1844e4..6f76bbe0d0 100644
--- a/WcaOnRails/app/assets/stylesheets/navbar-static-top.scss
+++ b/WcaOnRails/app/assets/stylesheets/navbar-static-top.scss
@@ -10,7 +10,7 @@
     margin-right: 10px;
   }
 
-  .dropdown-menu.countries {
+  #locale-selector {
     min-width: 0;
     li {
       text-align: left;
diff --git a/WcaOnRails/app/controllers/application_controller.rb b/WcaOnRails/app/controllers/application_controller.rb
index 97f8486025..be7d1d4d93 100644
--- a/WcaOnRails/app/controllers/application_controller.rb
+++ b/WcaOnRails/app/controllers/application_controller.rb
@@ -43,7 +43,7 @@ def update_locale
     else
       flash[:danger] = I18n.t('users.update_locale.unavailable')
     end
-    redirect_to request.referer || root_path
+    redirect_to params[:current_url] || root_path
   end
 
   # https://github.com/doorkeeper-gem/doorkeeper/wiki/Customizing-the-response-body-when-unauthorized
diff --git a/WcaOnRails/app/views/layouts/_navigation.html.erb b/WcaOnRails/app/views/layouts/_navigation.html.erb
index 561d8922da..d78737cf47 100644
--- a/WcaOnRails/app/views/layouts/_navigation.html.erb
+++ b/WcaOnRails/app/views/layouts/_navigation.html.erb
@@ -90,10 +90,10 @@
             <%= flag_icon active_locale_info[:flag_id] %> <span class="hidden-sm"><%= active_locale_info[:name] %></span>
             <span class="caret"></span>
           </a>
-          <ul class="dropdown-menu countries" role="menu">
+          <ul class="dropdown-menu" id="locale-selector" role="menu">
             <% Locales::AVAILABLE.each do |l, data| %>
               <li class="<%= l == I18n.locale ? "active" : "" %>">
-                <%= link_to update_locale_path(l), method: :patch do %>
+                <%= link_to update_locale_path(l) do %>
                   <%= flag_icon data[:flag_id] %> <%= data[:name] %>
                 <% end %>
               </li>
diff --git a/WcaOnRails/spec/controllers/application_controller_spec.rb b/WcaOnRails/spec/controllers/application_controller_spec.rb
index bdaa956b8b..1baf8f163e 100644
--- a/WcaOnRails/spec/controllers/application_controller_spec.rb
+++ b/WcaOnRails/spec/controllers/application_controller_spec.rb
@@ -15,5 +15,17 @@
       expect(user.preferred_locale).to eq "fr"
       expect(session[:locale]).to eq "fr"
     end
+
+    it "redirects to given current_url" do
+      sign_in user
+      patch :update_locale, params: { locale: :fr, current_url: "http://foo.com#bar" }
+      expect(response).to redirect_to "http://foo.com#bar"
+    end
+
+    it "redirects to root if not given current_url" do
+      sign_in user
+      patch :update_locale, params: { locale: :fr }
+      expect(response).to redirect_to root_url
+    end
   end
 end
diff --git a/WcaOnRails/spec/features/set_locale_spec.rb b/WcaOnRails/spec/features/set_locale_spec.rb
index caae43194a..c4f3c6a3e0 100644
--- a/WcaOnRails/spec/features/set_locale_spec.rb
+++ b/WcaOnRails/spec/features/set_locale_spec.rb
@@ -3,22 +3,31 @@
 require "rails_helper"
 
 RSpec.feature "Set the locale" do
-  context "As a visitor" do
-    let(:user) { FactoryBot.create :user }
-
-    scenario "visiting the home page and changing the locale" do
-      visit "/"
-      expect(I18n.locale).to eq I18n.default_locale
-      click_on "Français"
-      visit "/"
-      expect(I18n.locale).to eq :fr
-    end
-
-    scenario "signing in updates to the preferred_locale" do
-      user.update!(preferred_locale: "fr")
-      sign_in user
-      visit "/"
-      expect(I18n.locale).to eq :fr
-    end
+  scenario "visiting the home page while not signed in and changing the locale", js: true do
+    visit "/#foo"
+    expect(page).to have_content "English"
+    expect(page).not_to have_content "Français"
+
+    click_on "English" # Activate the locale selection dropdown.
+    click_on "Français"
+
+    expect(page.current_path).to eq "/"
+    expect(URI.parse(page.current_url).fragment).to eq "foo"
+
+    expect(page).not_to have_content "English"
+    expect(page).to have_content "Français"
+  end
+
+  scenario "signing in updates to the preferred_locale", js: true do
+    visit "/"
+    expect(page).to have_content "English"
+    expect(page).not_to have_content "Français"
+
+    user = FactoryBot.create :user, preferred_locale: "fr"
+    sign_in user
+    visit "/"
+
+    expect(page).not_to have_content "English"
+    expect(page).to have_content "Français"
   end
 end
diff --git a/WcaOnRails/spec/rails_helper.rb b/WcaOnRails/spec/rails_helper.rb
index 5eff7e812d..97f380e492 100644
--- a/WcaOnRails/spec/rails_helper.rb
+++ b/WcaOnRails/spec/rails_helper.rb
@@ -31,6 +31,12 @@
 # If you are not using ActiveRecord, you can remove this line.
 ActiveRecord::Migration.maintain_test_schema!
 
+# To debug feature specs using phantomjs, set `Capybara.javascript_driver = :poltergeist_debug`
+# and then call `page.driver.debug` in your feature spec.
+Capybara.register_driver :poltergeist_debug do |app|
+  Capybara::Poltergeist::Driver.new(app, inspector: true, phantomjs: Phantomjs.path, debug: true)
+end
+
 Capybara.javascript_driver = :poltergeist
 Capybara.server = :webrick