From dadbf873ba96c93194b236706058fe6dd4f1345e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 31 May 2019 13:41:01 +0200 Subject: [PATCH 1/2] Order translations using ruby MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Joining the translations table caused duplicate records to appear. Ordering with SQL is simply too hard because we need to consider fallback locales. Thanks Senén for providing most of the tests in the poll spec. --- app/controllers/polls_controller.rb | 5 +- .../budget_investments_controller.rb | 4 +- app/models/budget/group.rb | 4 +- app/models/budget/heading.rb | 3 +- app/models/poll.rb | 14 ++++- app/models/user.rb | 4 +- app/views/budgets/investments/_votes.html.erb | 2 +- spec/models/budget/group_spec.rb | 38 +++++++++++++ spec/models/budget/heading_spec.rb | 6 +++ spec/models/poll/poll_spec.rb | 54 +++++++++++++++++++ spec/models/user_spec.rb | 11 +++- 11 files changed, 131 insertions(+), 14 deletions(-) diff --git a/app/controllers/polls_controller.rb b/app/controllers/polls_controller.rb index 60e1e4eff..f322379ce 100644 --- a/app/controllers/polls_controller.rb +++ b/app/controllers/polls_controller.rb @@ -12,8 +12,9 @@ class PollsController < ApplicationController ::Poll::Answer # trigger autoload def index - @polls = @polls.not_budget.public_polls.send(@current_filter).includes(:geozones) - .sort_for_list.page(params[:page]) + @polls = Kaminari.paginate_array( + @polls.public_polls.not_budget.send(@current_filter).includes(:geozones).sort_for_list + ).page(params[:page]) end def show diff --git a/app/controllers/valuation/budget_investments_controller.rb b/app/controllers/valuation/budget_investments_controller.rb index aa29120f1..a97616f41 100644 --- a/app/controllers/valuation/budget_investments_controller.rb +++ b/app/controllers/valuation/budget_investments_controller.rb @@ -75,9 +75,7 @@ class Valuation::BudgetInvestmentsController < Valuation::BaseController def heading_filters investments = @budget.investments.by_valuator(current_user.valuator.try(:id)) .visible_to_valuators.distinct - investment_headings = Budget::Heading.joins(:translations) - .where(id: investments.pluck(:heading_id).uniq) - .order(name: :asc) + investment_headings = Budget::Heading.where(id: investments.pluck(:heading_id)).sort_by(&:name) all_headings_filter = [ { diff --git a/app/models/budget/group.rb b/app/models/budget/group.rb index d77c840c0..43d28a0ae 100644 --- a/app/models/budget/group.rb +++ b/app/models/budget/group.rb @@ -28,7 +28,9 @@ class Budget validates :budget_id, presence: true validates :slug, presence: true, format: /\A[a-z0-9\-_]+\z/ - scope :sort_by_name, -> { joins(:translations).order(:name) } + def self.sort_by_name + all.sort_by(&:name) + end def single_heading_group? headings.count == 1 diff --git a/app/models/budget/heading.rb b/app/models/budget/heading.rb index 0b94aac67..7b33c4e54 100644 --- a/app/models/budget/heading.rb +++ b/app/models/budget/heading.rb @@ -40,8 +40,7 @@ class Budget delegate :budget, :budget_id, to: :group, allow_nil: true - scope :i18n, -> { joins(:translations) } - scope :allow_custom_content, -> { i18n.where(allow_custom_content: true).order("budget_heading_translations.name") } + scope :allow_custom_content, -> { where(allow_custom_content: true).sort_by(&:name) } def self.sort_by_name all.sort do |heading, other_heading| diff --git a/app/models/poll.rb b/app/models/poll.rb index 3e9ef6322..a6526f8cf 100644 --- a/app/models/poll.rb +++ b/app/models/poll.rb @@ -49,7 +49,19 @@ class Poll < ApplicationRecord scope :not_budget, -> { where(budget_id: nil) } scope :created_by_admin, -> { where(related_type: nil) } - scope :sort_for_list, -> { joins(:translations).order(:geozone_restricted, :starts_at, "poll_translations.name") } + def self.sort_for_list + all.sort do |poll, another_poll| + if poll.geozone_restricted? == another_poll.geozone_restricted? + [poll.starts_at, poll.name] <=> [another_poll.starts_at, another_poll.name] + else + if poll.geozone_restricted? + 1 + else + -1 + end + end + end + end def self.overlaping_with(poll) where("? < ends_at and ? >= starts_at", poll.starts_at.beginning_of_day, diff --git a/app/models/user.rb b/app/models/user.rb index ba9aec3d5..4e6b5f00c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -131,9 +131,7 @@ class User < ApplicationRecord end def headings_voted_within_group(group) - Budget::Heading.joins(:translations) - .order("name") - .where(id: voted_investments.by_group(group).pluck(:heading_id)) + Budget::Heading.where(id: voted_investments.by_group(group).pluck(:heading_id)) end def voted_investments diff --git a/app/views/budgets/investments/_votes.html.erb b/app/views/budgets/investments/_votes.html.erb index 8d17f696e..2923fccee 100644 --- a/app/views/budgets/investments/_votes.html.erb +++ b/app/views/budgets/investments/_votes.html.erb @@ -36,7 +36,7 @@ verify_account: link_to(t("votes.verify_account"), verification_path), signin: link_to(t("votes.signin"), new_user_session_path), signup: link_to(t("votes.signup"), new_user_registration_path), - supported_headings: (current_user && current_user.headings_voted_within_group(investment.group).map(&:name).to_sentence) + supported_headings: (current_user && current_user.headings_voted_within_group(investment.group).map(&:name).sort.to_sentence) ).html_safe %>

diff --git a/spec/models/budget/group_spec.rb b/spec/models/budget/group_spec.rb index 2ab137ba5..ce1f47563 100644 --- a/spec/models/budget/group_spec.rb +++ b/spec/models/budget/group_spec.rb @@ -32,4 +32,42 @@ describe Budget::Group do end end + + describe "#sort_by_name" do + it "returns groups sorted by name ASC" do + thinkers = create(:budget_group, name: "Mmmm...") + sleepers = create(:budget_group, name: "Zzz...") + startled = create(:budget_group, name: "Aaaaah!") + + expect(Budget::Group.sort_by_name).to eq [startled, thinkers, sleepers] + end + + it "returns groups with multiple translations only once" do + create(:budget_group, name_en: "English", name_es: "Spanish") + + expect(Budget::Group.sort_by_name.count).to eq 1 + end + + context "fallback locales" do + before do + allow(I18n.fallbacks).to receive(:[]).and_return([:es]) + Globalize.set_fallbacks_to_all_available_locales + end + + it "orders by name considering fallback locale," do + budget = create(:budget, name: "Teams") + charlie = create(:budget_group, budget: budget, name: "Charlie") + delta = create(:budget_group, budget: budget, name: "Delta") + zulu = Globalize.with_locale(:es) do + create(:budget_group, budget: budget, name: "Zulu", name_fr: "Alpha") + end + bravo = Globalize.with_locale(:es) do + create(:budget_group, budget: budget, name: "Bravo") + end + + expect(Budget::Group.sort_by_name.count).to eq 4 + expect(Budget::Group.sort_by_name).to eq [bravo, charlie, delta, zulu] + end + end + end end diff --git a/spec/models/budget/heading_spec.rb b/spec/models/budget/heading_spec.rb index 2d69839e6..b32f6aeb1 100644 --- a/spec/models/budget/heading_spec.rb +++ b/spec/models/budget/heading_spec.rb @@ -319,6 +319,12 @@ describe Budget::Heading do expect(Budget::Heading.allow_custom_content.first).to eq first_heading expect(Budget::Heading.allow_custom_content.last).to eq last_heading end + + it "returns headings with multiple translations only once" do + create(:budget_heading, allow_custom_content: true, name_en: "English", name_es: "Spanish") + + expect(Budget::Heading.allow_custom_content.count).to eq 1 + end end end diff --git a/spec/models/poll/poll_spec.rb b/spec/models/poll/poll_spec.rb index 581123997..e03d941b1 100644 --- a/spec/models/poll/poll_spec.rb +++ b/spec/models/poll/poll_spec.rb @@ -359,4 +359,58 @@ describe Poll do end + describe "#sort_for_list" do + it "returns polls sorted by name ASC" do + starts_at = Time.current + 1.day + poll1 = create(:poll, geozone_restricted: true, starts_at: starts_at, name: "Zzz...") + poll2 = create(:poll, geozone_restricted: true, starts_at: starts_at, name: "Mmmm...") + poll3 = create(:poll, geozone_restricted: true, starts_at: starts_at, name: "Aaaaah!") + + expect(Poll.sort_for_list).to eq [poll3, poll2, poll1] + end + + it "returns not geozone restricted polls first" do + starts_at = Time.current + 1.day + poll1 = create(:poll, geozone_restricted: false, starts_at: starts_at, name: "Zzz...") + poll2 = create(:poll, geozone_restricted: true, starts_at: starts_at, name: "Aaaaaah!") + + expect(Poll.sort_for_list).to eq [poll1, poll2] + end + + it "returns polls earlier to start first" do + starts_at = Time.current + 1.day + poll1 = create(:poll, geozone_restricted: false, starts_at: starts_at - 1.hour, name: "Zzz...") + poll2 = create(:poll, geozone_restricted: false, starts_at: starts_at, name: "Aaaaah!") + + expect(Poll.sort_for_list).to eq [poll1, poll2] + end + + it "returns polls with multiple translations only once" do + create(:poll, name_en: "English", name_es: "Spanish") + + expect(Poll.sort_for_list.count).to eq 1 + end + + context "fallback locales" do + before do + allow(I18n.fallbacks).to receive(:[]).and_return([:es]) + Globalize.set_fallbacks_to_all_available_locales + end + + it "orders by name considering fallback locales" do + starts_at = Time.current + 1.day + poll1 = create(:poll, starts_at: starts_at, name: "Charlie") + poll2 = create(:poll, starts_at: starts_at, name: "Delta") + poll3 = Globalize.with_locale(:es) do + create(:poll, starts_at: starts_at, name: "Zzz...", name_fr: "Aaaah!") + end + poll4 = Globalize.with_locale(:es) do + create(:poll, starts_at: starts_at, name: "Bravo") + end + + expect(Poll.sort_for_list.count).to eq 4 + expect(Poll.sort_for_list).to eq [poll4, poll1, poll2, poll3] + end + end + end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 599459324..d11df7760 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -3,7 +3,7 @@ require "rails_helper" describe User do describe "#headings_voted_within_group" do - it "returns the headings voted by a user ordered by name" do + it "returns the headings voted by a user" do user1 = create(:user) user2 = create(:user) @@ -35,6 +35,15 @@ describe User do expect(user2.headings_voted_within_group(group)).not_to include(san_franciso) expect(user2.headings_voted_within_group(group)).not_to include(another_heading) end + + it "returns headings with multiple translations only once" do + user = create(:user) + group = create(:budget_group) + heading = create(:budget_heading, group: group, name_en: "English", name_es: "Spanish") + create(:vote, votable: create(:budget_investment, heading: heading), voter: user) + + expect(user.headings_voted_within_group(group).count).to eq 1 + end end describe "#debate_votes" do From 481184e7f3e296da7165c7077a0556fd220b6bd3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 31 May 2019 13:48:27 +0200 Subject: [PATCH 2/2] Fix accidental translations join We added the code thinking we were ordering by the name of the poll, but here we're actually ordering by the name of the booth. --- app/models/poll/booth.rb | 2 +- spec/models/poll/booth_spec.rb | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/app/models/poll/booth.rb b/app/models/poll/booth.rb index d10270064..4c45b9d1a 100644 --- a/app/models/poll/booth.rb +++ b/app/models/poll/booth.rb @@ -12,7 +12,7 @@ class Poll end def self.available - where(polls: { id: Poll.current_or_recounting }).joins(polls: :translations) + where(polls: { id: Poll.current_or_recounting }).joins(:polls) end def assignment_on_poll(poll) diff --git a/spec/models/poll/booth_spec.rb b/spec/models/poll/booth_spec.rb index 8cde2263b..2a90d23c6 100644 --- a/spec/models/poll/booth_spec.rb +++ b/spec/models/poll/booth_spec.rb @@ -24,7 +24,7 @@ describe Poll::Booth do end end - describe "#available" do + describe ".available" do it "returns booths associated to current polls" do booth_for_current_poll = create(:poll_booth) @@ -40,5 +40,10 @@ describe Poll::Booth do expect(described_class.available).not_to include(booth_for_expired_poll) end + it "returns polls with multiple translations only once" do + create(:poll_booth, polls: [create(:poll, :current, name: "English", name_es: "Spanish")]) + + expect(Poll::Booth.available.count).to eq 1 + end end end