Skip to content

Commit

Permalink
Preserve the url hash when changing locales.
Browse files Browse the repository at this point in the history
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 thewca#2962.
  • Loading branch information
jfly committed Jun 14, 2018
1 parent f216e7a commit afd76ad
Show file tree
Hide file tree
Showing 7 changed files with 70 additions and 21 deletions.
22 changes: 22 additions & 0 deletions WcaOnRails/app/assets/javascripts/application.js
Original file line number Diff line number Diff line change
Expand Up @@ -379,3 +379,25 @@ $(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 href = this.href;
var method = "patch";
var csrfToken = $('meta[name=csrf-token]').attr('content');
var csrfParam = $('meta[name=csrf-param]').attr('content');
var form = $('<form method="post" action="' + href + '"></form>');
var metadataInput = '<input name="_method" value="' + method + '" 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();
});
});
2 changes: 1 addition & 1 deletion WcaOnRails/app/assets/stylesheets/navbar-static-top.scss
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
margin-right: 10px;
}

.dropdown-menu.countries {
#locale-selector {
min-width: 0;
li {
text-align: left;
Expand Down
2 changes: 1 addition & 1 deletion WcaOnRails/app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions WcaOnRails/app/views/layouts/_navigation.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
12 changes: 12 additions & 0 deletions WcaOnRails/spec/controllers/application_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
43 changes: 26 additions & 17 deletions WcaOnRails/spec/features/set_locale_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
6 changes: 6 additions & 0 deletions WcaOnRails/spec/rails_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down

0 comments on commit afd76ad

Please sign in to comment.