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: rails/jquery-ujs#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.

Author: jfly

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();
+  });
+});

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;

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

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>

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

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

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