From 7c425c00aa175e0ee7436e2a37ff4aedf6971dc5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 12 Jul 2020 22:24:09 +0200 Subject: [PATCH 01/15] Remove unnecessary condition The ballot is used in the previous lines, so there's no point checking whether it's present. --- app/models/budget/investment.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index fc4d31ff7..b11af9e25 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -268,7 +268,7 @@ class Budget return :not_selected unless selected? return :no_ballots_allowed unless budget.balloting? return :different_heading_assigned unless ballot.valid_heading?(heading) - return :not_enough_money if ballot.present? && !enough_money?(ballot) + return :not_enough_money unless enough_money?(ballot) return :casted_offline if ballot.casted_offline? end From 7ce2d8b7eb546c311098536de9b0f454eaf043d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 12 Jul 2020 22:53:02 +0200 Subject: [PATCH 02/15] Remove unused methods One method was calling `reason_for_not_being_ballotable_by` passing just one parameter instead of two. The other method was calling the method `amount_spent`, which does not exist in the Budget class. So both methods would make the application crash if they were called. Luckily, they aren't, so the application doesn't crash. --- app/models/budget.rb | 4 ---- app/models/budget/investment.rb | 4 ---- 2 files changed, 8 deletions(-) diff --git a/app/models/budget.rb b/app/models/budget.rb index 26489419c..8d2a6ef57 100644 --- a/app/models/budget.rb +++ b/app/models/budget.rb @@ -163,10 +163,6 @@ class Budget < ApplicationRecord formatted_amount(heading_price(heading)) end - def formatted_heading_amount_spent(heading) - formatted_amount(amount_spent(heading)) - end - def investments_orders case phase when "accepting", "reviewing" diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index b11af9e25..61d31b625 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -301,10 +301,6 @@ class Budget user.headings_voted_within_group(group).where(id: heading.id).exists? end - def ballotable_by?(user) - reason_for_not_being_ballotable_by(user).blank? - end - def enough_money?(ballot) available_money = ballot.amount_available(heading) price.to_i <= available_money From aff213b0ef5f8725925a49572e7c6d9e662eaffe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 12 Jul 2020 23:05:39 +0200 Subject: [PATCH 03/15] Remove redundant calls to load resources We already load the budget and the ballot in `before_action` calls, so we don't have to load them again. --- app/controllers/budgets/ballot/lines_controller.rb | 4 ++-- app/controllers/budgets/ballots_controller.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/budgets/ballot/lines_controller.rb b/app/controllers/budgets/ballot/lines_controller.rb index 124bfba83..e9df6ab2c 100644 --- a/app/controllers/budgets/ballot/lines_controller.rb +++ b/app/controllers/budgets/ballot/lines_controller.rb @@ -9,8 +9,8 @@ module Budgets before_action :load_investments before_action :load_ballot_referer - load_and_authorize_resource :budget - load_and_authorize_resource :ballot, class: "Budget::Ballot", through: :budget + authorize_resource :budget + authorize_resource :ballot load_and_authorize_resource :line, through: :ballot, find_by: :investment_id, class: "Budget::Ballot::Line" def create diff --git a/app/controllers/budgets/ballots_controller.rb b/app/controllers/budgets/ballots_controller.rb index 7015c8285..52b68c630 100644 --- a/app/controllers/budgets/ballots_controller.rb +++ b/app/controllers/budgets/ballots_controller.rb @@ -2,7 +2,7 @@ module Budgets class BallotsController < ApplicationController before_action :authenticate_user! before_action :load_budget - load_and_authorize_resource :budget + authorize_resource :budget before_action :load_ballot after_action :store_referer, only: [:show] From a32c0f8154728c4e60a38db9f9a898d8fe56910b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 12 Jul 2020 23:12:39 +0200 Subject: [PATCH 04/15] Remove unused parameter The `refresh_ballots` partial ignores the `investment` parameter completely; instead, it iterates over the investments in the `@investments` instance variable. --- app/views/budgets/ballot/lines/create.js.erb | 1 - app/views/budgets/ballot/lines/destroy.js.erb | 1 - 2 files changed, 2 deletions(-) diff --git a/app/views/budgets/ballot/lines/create.js.erb b/app/views/budgets/ballot/lines/create.js.erb index 24e9a5aef..22617ef02 100644 --- a/app/views/budgets/ballot/lines/create.js.erb +++ b/app/views/budgets/ballot/lines/create.js.erb @@ -6,7 +6,6 @@ $("#<%= dom_id(@investment) %>_ballot").html("<%= j render("/budgets/investments ballot: @ballot) %>"); <%= render "refresh_ballots", - investment: @investment, investment_ids: @investment_ids, ballot: @ballot %> diff --git a/app/views/budgets/ballot/lines/destroy.js.erb b/app/views/budgets/ballot/lines/destroy.js.erb index 0503609e1..078fae9a9 100644 --- a/app/views/budgets/ballot/lines/destroy.js.erb +++ b/app/views/budgets/ballot/lines/destroy.js.erb @@ -7,7 +7,6 @@ $("#<%= dom_id(@investment) %>_ballot").html("<%= j render("/budgets/investments investment_ids: @investment_ids, ballot: @ballot) %>"); <%= render "refresh_ballots", - investment: @investment, investment_ids: @investment_ids, ballot: @ballot %> From a9900e3f2757d8d8d522b6d71588e6ac1ad2531c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 12 Jul 2020 23:39:01 +0200 Subject: [PATCH 05/15] Remove duplication calculating insufficient funds We were using the same logic twice. I've moved the logic to the Ballot model, which to me is a more natural place to calculate whether there's enough money left than the Investment model. After all, the remaining money is in the ballot, and not in the investment. --- app/models/budget/ballot.rb | 4 ++++ app/models/budget/ballot/line.rb | 2 +- app/models/budget/investment.rb | 7 +------ 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/app/models/budget/ballot.rb b/app/models/budget/ballot.rb index ccdf1b289..70f59fc05 100644 --- a/app/models/budget/ballot.rb +++ b/app/models/budget/ballot.rb @@ -33,6 +33,10 @@ class Budget budget.formatted_amount(amount_available(heading)) end + def enough_money?(investment) + investment.price.to_i <= amount_available(investment.heading) + end + def has_lines_in_group?(group) groups.include?(group) end diff --git a/app/models/budget/ballot/line.rb b/app/models/budget/ballot/line.rb index 091ceaea7..9d9805781 100644 --- a/app/models/budget/ballot/line.rb +++ b/app/models/budget/ballot/line.rb @@ -20,7 +20,7 @@ class Budget def check_sufficient_funds ballot.lock! - errors.add(:money, "insufficient funds") if ballot.amount_available(investment.heading) < investment.price.to_i + errors.add(:money, "insufficient funds") unless ballot.enough_money?(investment) end def check_valid_heading diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index 61d31b625..63f03441a 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -268,7 +268,7 @@ class Budget return :not_selected unless selected? return :no_ballots_allowed unless budget.balloting? return :different_heading_assigned unless ballot.valid_heading?(heading) - return :not_enough_money unless enough_money?(ballot) + return :not_enough_money unless ballot.enough_money?(self) return :casted_offline if ballot.casted_offline? end @@ -301,11 +301,6 @@ class Budget user.headings_voted_within_group(group).where(id: heading.id).exists? end - def enough_money?(ballot) - available_money = ballot.amount_available(heading) - price.to_i <= available_money - end - def register_selection(user) vote_by(voter: user, vote: "yes") if selectable_by?(user) end From a38cdb4df33eaefb9f30f23ce7ced06743b94dbc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 12 Jul 2020 23:41:02 +0200 Subject: [PATCH 06/15] Remove unnecessary safe navigation operator The heading is used with `find_by_slug_or_id`, which raises an exception if it isn't found, so executing `@heading.group` after it does not need the safe navigation operator. --- app/controllers/budgets/investments_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/budgets/investments_controller.rb b/app/controllers/budgets/investments_controller.rb index 9728c8ef3..541f40e44 100644 --- a/app/controllers/budgets/investments_controller.rb +++ b/app/controllers/budgets/investments_controller.rb @@ -148,7 +148,7 @@ module Budgets def load_heading if params[:heading_id].present? @heading = @budget.headings.find_by_slug_or_id! params[:heading_id] - @assigned_heading = @ballot&.heading_for_group(@heading&.group) + @assigned_heading = @ballot&.heading_for_group(@heading.group) load_map end end From c22e800329d4f690e2380d57671c31cfe8b835eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 12 Jul 2020 23:44:31 +0200 Subject: [PATCH 07/15] Remove code duplication We were calling the same method three times. --- app/views/budgets/ballot/_ballot.html.erb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/app/views/budgets/ballot/_ballot.html.erb b/app/views/budgets/ballot/_ballot.html.erb index cd70aea12..795246d47 100644 --- a/app/views/budgets/ballot/_ballot.html.erb +++ b/app/views/budgets/ballot/_ballot.html.erb @@ -19,21 +19,22 @@
<% ballot_groups = @ballot.groups.sort_by_name %> <% ballot_groups.each do |group| %> + <% heading = @ballot.heading_for_group(group) %>

- <%= group.name %> - <%= @ballot.heading_for_group(group).name %> + <%= group.name %> - <%= heading.name %>

<%= link_to sanitize(t("budgets.ballots.show.remaining", - amount: @ballot.formatted_amount_available(@ballot.heading_for_group(group)))), + amount: @ballot.formatted_amount_available(heading))), budget_group_path(@budget, group) %>
<% if @ballot.has_lines_in_group?(group) %>

<%= t("budgets.ballots.show.amount_spent") %> - <%= @ballot.formatted_amount_spent(@ballot.heading_for_group(group)) %> + <%= @ballot.formatted_amount_spent(heading) %>

<% else %> From ad094e5063cc63503958727980928258eb87891d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 12 Jul 2020 23:59:41 +0200 Subject: [PATCH 08/15] Extract class to handle voting style logic Since we're going to introduce a new voting style which will not be based on money, we're extracting the logic specific to the current voting style to a new class. This way adding new voting styles will be easier. --- app/models/budget/ballot.rb | 28 +++++++++------------ app/models/budget/voting_styles/base.rb | 7 ++++++ app/models/budget/voting_styles/knapsack.rb | 17 +++++++++++++ 3 files changed, 36 insertions(+), 16 deletions(-) create mode 100644 app/models/budget/voting_styles/base.rb create mode 100644 app/models/budget/voting_styles/knapsack.rb diff --git a/app/models/budget/ballot.rb b/app/models/budget/ballot.rb index 70f59fc05..4c4b2370c 100644 --- a/app/models/budget/ballot.rb +++ b/app/models/budget/ballot.rb @@ -17,26 +17,10 @@ class Budget investments.sum(:price).to_i end - def amount_spent(heading) - investments.by_heading(heading.id).sum(:price).to_i - end - def formatted_amount_spent(heading) budget.formatted_amount(amount_spent(heading)) end - def amount_available(heading) - budget.heading_price(heading) - amount_spent(heading) - end - - def formatted_amount_available(heading) - budget.formatted_amount(amount_available(heading)) - end - - def enough_money?(investment) - investment.price.to_i <= amount_available(investment.heading) - end - def has_lines_in_group?(group) groups.include?(group) end @@ -79,5 +63,17 @@ class Budget def casted_offline? budget.poll&.voted_by?(user) end + + def voting_style + @voting_style ||= voting_style_class.new(self) + end + delegate :amount_available, :amount_spent, :enough_money?, :formatted_amount_available, + to: :voting_style + + private + + def voting_style_class + Budget::VotingStyles::Knapsack + end end end diff --git a/app/models/budget/voting_styles/base.rb b/app/models/budget/voting_styles/base.rb new file mode 100644 index 000000000..48de3a032 --- /dev/null +++ b/app/models/budget/voting_styles/base.rb @@ -0,0 +1,7 @@ +class Budget::VotingStyles::Base + attr_reader :ballot + + def initialize(ballot) + @ballot = ballot + end +end diff --git a/app/models/budget/voting_styles/knapsack.rb b/app/models/budget/voting_styles/knapsack.rb new file mode 100644 index 000000000..6c056115e --- /dev/null +++ b/app/models/budget/voting_styles/knapsack.rb @@ -0,0 +1,17 @@ +class Budget::VotingStyles::Knapsack < Budget::VotingStyles::Base + def enough_money?(investment) + investment.price.to_i <= amount_available(investment.heading) + end + + def amount_available(heading) + ballot.budget.heading_price(heading) - amount_spent(heading) + end + + def amount_spent(heading) + ballot.investments.by_heading(heading.id).sum(:price).to_i + end + + def formatted_amount_available(heading) + ballot.budget.formatted_amount(amount_available(heading)) + end +end From ad6d830c1f98b4252496d8ce82dac2cd7aca23f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 13 Jul 2020 02:04:41 +0200 Subject: [PATCH 09/15] Make translations more consistent We're passing the amount as a paramenter to the "remaining" text, so it makes sense to pass it to the "amount spent" text as well. Here we're also changing the I18n key to the text saying users can change their vote, so it's easier to note the text is about changing their vote, and not about the projects they have voted so far. --- app/views/budgets/ballot/_ballot.html.erb | 6 ++---- app/views/budgets/investments/_sidebar.html.erb | 4 ++-- config/locales/en/budgets.yml | 6 +++--- config/locales/es/budgets.yml | 6 +++--- 4 files changed, 10 insertions(+), 12 deletions(-) diff --git a/app/views/budgets/ballot/_ballot.html.erb b/app/views/budgets/ballot/_ballot.html.erb index 795246d47..e288111a9 100644 --- a/app/views/budgets/ballot/_ballot.html.erb +++ b/app/views/budgets/ballot/_ballot.html.erb @@ -32,10 +32,8 @@
<% if @ballot.has_lines_in_group?(group) %>

- <%= t("budgets.ballots.show.amount_spent") %> - - <%= @ballot.formatted_amount_spent(heading) %> - + <%= sanitize(t("budgets.ballots.show.amount_spent", + amount: @ballot.formatted_amount_spent(heading))) %>

<% else %>

diff --git a/app/views/budgets/investments/_sidebar.html.erb b/app/views/budgets/investments/_sidebar.html.erb index 823c23a89..2a7070670 100644 --- a/app/views/budgets/investments/_sidebar.html.erb +++ b/app/views/budgets/investments/_sidebar.html.erb @@ -14,8 +14,8 @@ <% if @heading && can?(:show, @ballot) %>

- <%= sanitize(t("budgets.investments.index.sidebar.voted_info", - link: link_to(t("budgets.investments.index.sidebar.voted_info_link"), + <%= sanitize(t("budgets.investments.index.sidebar.change_vote_info", + link: link_to(t("budgets.investments.index.sidebar.change_vote_link"), budget_ballot_path(@budget)))) %>

<% end %> diff --git a/config/locales/en/budgets.yml b/config/locales/en/budgets.yml index f3c5713b5..6dad9ade6 100644 --- a/config/locales/en/budgets.yml +++ b/config/locales/en/budgets.yml @@ -3,7 +3,7 @@ en: ballots: show: title: Your ballot - amount_spent: Amount spent + amount_spent: "Amount spent %{amount}" remaining: "You still have %{amount} to invest." no_balloted_group_yet: "You have not voted on this group yet, go vote!" remove: Remove vote @@ -87,8 +87,8 @@ en: voted: one: "You voted one proposal with a cost of %{amount_spent}" other: "You voted %{count} proposals with a cost of %{amount_spent}" - voted_info: You can %{link} at any time until the close of this phase. No need to spend all the money available. - voted_info_link: change your vote + change_vote_info: "You can %{link} at any time until the close of this phase. No need to spend all the money available." + change_vote_link: "change your vote" different_heading_assigned: "You have active votes in another heading: %{heading_link}" change_ballot: "If your change your mind you can remove your votes in %{check_ballot} and start again." check_ballot_link: "check and confirm my ballot" diff --git a/config/locales/es/budgets.yml b/config/locales/es/budgets.yml index e096dcc3b..5b0c3c918 100644 --- a/config/locales/es/budgets.yml +++ b/config/locales/es/budgets.yml @@ -3,7 +3,7 @@ es: ballots: show: title: Mis votos - amount_spent: Coste total + amount_spent: "Coste total %{amount}" remaining: "Te quedan %{amount} para invertir" no_balloted_group_yet: "Todavía no has votado proyectos de este grupo, ¡vota!" remove: Quitar voto @@ -87,8 +87,8 @@ es: voted: one: "Has votado un proyecto por un valor de %{amount_spent}" other: "Has votado %{count} proyectos por un valor de %{amount_spent}" - voted_info: Puedes %{link} en cualquier momento hasta el cierre de esta fase. No hace falta que gastes todo el dinero disponible. - voted_info_link: cambiar tus votos + change_vote_info: "Puedes %{link} en cualquier momento hasta el cierre de esta fase. No hace falta que gastes todo el dinero disponible." + change_vote_link: "cambiar tus votos" different_heading_assigned: "Ya apoyaste proyectos de otra sección del presupuesto: %{heading_link}" change_ballot: "Si cambias de opinión puedes borrar tus votos en %{check_ballot} y volver a empezar." check_ballot_link: "revisar y confirmar mis votos" From 5f726df8be50f64e9851a9d1e7fd222aa60b47e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 13 Jul 2020 02:11:24 +0200 Subject: [PATCH 10/15] Extract methods showing voting style information The idea is that different voting styles will display different information messages. --- app/models/budget/ballot.rb | 8 ++-- app/models/budget/voting_styles/base.rb | 39 +++++++++++++++++++ app/models/budget/voting_styles/knapsack.rb | 22 +++++++++-- app/views/budgets/ballot/_ballot.html.erb | 6 +-- .../budgets/ballot/_progress_bar.html.erb | 2 +- .../budgets/investments/_sidebar.html.erb | 8 +--- config/i18n-tasks.yml | 1 + config/locales/en/budgets.yml | 18 ++++++--- config/locales/es/budgets.yml | 18 ++++++--- 9 files changed, 91 insertions(+), 31 deletions(-) diff --git a/app/models/budget/ballot.rb b/app/models/budget/ballot.rb index 4c4b2370c..074bf6a8f 100644 --- a/app/models/budget/ballot.rb +++ b/app/models/budget/ballot.rb @@ -17,10 +17,6 @@ class Budget investments.sum(:price).to_i end - def formatted_amount_spent(heading) - budget.formatted_amount(amount_spent(heading)) - end - def has_lines_in_group?(group) groups.include?(group) end @@ -67,7 +63,9 @@ class Budget def voting_style @voting_style ||= voting_style_class.new(self) end - delegate :amount_available, :amount_spent, :enough_money?, :formatted_amount_available, + delegate :amount_available, :amount_available_info, :amount_spent, :amount_spent_info, + :amount_limit_info, :change_vote_info, :enough_money?, :formatted_amount_available, + :formatted_amount_limit, :formatted_amount_spent, :voted_info, to: :voting_style private diff --git a/app/models/budget/voting_styles/base.rb b/app/models/budget/voting_styles/base.rb index 48de3a032..e407641db 100644 --- a/app/models/budget/voting_styles/base.rb +++ b/app/models/budget/voting_styles/base.rb @@ -4,4 +4,43 @@ class Budget::VotingStyles::Base def initialize(ballot) @ballot = ballot end + + def name + self.class.name.split("::").last.underscore + end + + def change_vote_info(link:) + I18n.t("budgets.investments.index.sidebar.change_vote_info.#{name}", link: link) + end + + def voted_info(heading) + I18n.t("budgets.investments.index.sidebar.voted_info.#{name}", + count: investments(heading).count, + amount_spent: ballot.budget.formatted_amount(investments_price(heading))) + end + + def amount_available_info(heading) + I18n.t("budgets.ballots.show.amount_available.#{name}", + amount: formatted_amount_available(heading)) + end + + def amount_spent_info(heading) + I18n.t("budgets.ballots.show.amount_spent.#{name}", + amount: formatted_amount_spent(heading)) + end + + def amount_limit_info(heading) + I18n.t("budgets.ballots.show.amount_limit.#{name}", + amount: formatted_amount_limit(heading)) + end + + private + + def investments(heading) + ballot.investments.by_heading(heading.id) + end + + def investments_price(heading) + investments(heading).sum(:price).to_i + end end diff --git a/app/models/budget/voting_styles/knapsack.rb b/app/models/budget/voting_styles/knapsack.rb index 6c056115e..815a19102 100644 --- a/app/models/budget/voting_styles/knapsack.rb +++ b/app/models/budget/voting_styles/knapsack.rb @@ -4,14 +4,30 @@ class Budget::VotingStyles::Knapsack < Budget::VotingStyles::Base end def amount_available(heading) - ballot.budget.heading_price(heading) - amount_spent(heading) + amount_limit(heading) - amount_spent(heading) end def amount_spent(heading) - ballot.investments.by_heading(heading.id).sum(:price).to_i + investments_price(heading) + end + + def amount_limit(heading) + ballot.budget.heading_price(heading) end def formatted_amount_available(heading) - ballot.budget.formatted_amount(amount_available(heading)) + format(amount_available(heading)) + end + + def formatted_amount_spent(heading) + format(amount_spent(heading)) + end + + def formatted_amount_limit(heading) + format(amount_limit(heading)) + end + + def format(amount) + ballot.budget.formatted_amount(amount) end end diff --git a/app/views/budgets/ballot/_ballot.html.erb b/app/views/budgets/ballot/_ballot.html.erb index e288111a9..e8e79300f 100644 --- a/app/views/budgets/ballot/_ballot.html.erb +++ b/app/views/budgets/ballot/_ballot.html.erb @@ -26,14 +26,12 @@

<%= group.name %> - <%= heading.name %>

- <%= link_to sanitize(t("budgets.ballots.show.remaining", - amount: @ballot.formatted_amount_available(heading))), + <%= link_to sanitize(@ballot.amount_available_info(heading)), budget_group_path(@budget, group) %>
<% if @ballot.has_lines_in_group?(group) %>

- <%= sanitize(t("budgets.ballots.show.amount_spent", - amount: @ballot.formatted_amount_spent(heading))) %> + <%= sanitize(@ballot.amount_spent_info(heading)) %>

<% else %>

diff --git a/app/views/budgets/ballot/_progress_bar.html.erb b/app/views/budgets/ballot/_progress_bar.html.erb index 4c7035b41..06206b7da 100644 --- a/app/views/budgets/ballot/_progress_bar.html.erb +++ b/app/views/budgets/ballot/_progress_bar.html.erb @@ -1,5 +1,5 @@ - <%= @budget.formatted_heading_price(@heading) %> + <%= sanitize(@ballot.amount_limit_info(@heading)) %>

- <%= sanitize(t("budgets.investments.index.sidebar.change_vote_info", + <%= sanitize(@ballot.change_vote_info( link: link_to(t("budgets.investments.index.sidebar.change_vote_link"), budget_ballot_path(@budget)))) %>

@@ -39,11 +39,7 @@ <% if @ballot.investments.by_heading(@heading.id).count > 0 %>

- - <%= sanitize(t("budgets.investments.index.sidebar.voted", - count: @ballot.investments.by_heading(@heading.id).count, - amount_spent: @ballot.formatted_amount_spent(@heading))) %> - + <%= sanitize(@ballot.voted_info(@heading)) %>

<% elsif @assigned_heading.present? %>

diff --git a/config/i18n-tasks.yml b/config/i18n-tasks.yml index 29706ed60..f03a38d09 100644 --- a/config/i18n-tasks.yml +++ b/config/i18n-tasks.yml @@ -127,6 +127,7 @@ ignore_unused: - "budgets.phase.*" - "budgets.investments.index.orders.*" - "budgets.index.section_header.*" + - "budgets.investments.index.sidebar.voted_info.*" - "activerecord.*" - "activemodel.*" - "attributes.*" diff --git a/config/locales/en/budgets.yml b/config/locales/en/budgets.yml index 6dad9ade6..8047f44ad 100644 --- a/config/locales/en/budgets.yml +++ b/config/locales/en/budgets.yml @@ -3,8 +3,12 @@ en: ballots: show: title: Your ballot - amount_spent: "Amount spent %{amount}" - remaining: "You still have %{amount} to invest." + amount_available: + knapsack: "You still have %{amount} to invest." + amount_spent: + knapsack: "Amount spent %{amount}" + amount_limit: + knapsack: "%{amount}" no_balloted_group_yet: "You have not voted on this group yet, go vote!" remove: Remove vote voted: @@ -84,10 +88,12 @@ en: other: " containing the term '%{search_term}'" sidebar: my_ballot: My ballot - voted: - one: "You voted one proposal with a cost of %{amount_spent}" - other: "You voted %{count} proposals with a cost of %{amount_spent}" - change_vote_info: "You can %{link} at any time until the close of this phase. No need to spend all the money available." + voted_info: + knapsack: + one: "You voted one proposal with a cost of %{amount_spent}" + other: "You voted %{count} proposals with a cost of %{amount_spent}" + change_vote_info: + knapsack: "You can %{link} at any time until the close of this phase. No need to spend all the money available." change_vote_link: "change your vote" different_heading_assigned: "You have active votes in another heading: %{heading_link}" change_ballot: "If your change your mind you can remove your votes in %{check_ballot} and start again." diff --git a/config/locales/es/budgets.yml b/config/locales/es/budgets.yml index 5b0c3c918..9ccc52159 100644 --- a/config/locales/es/budgets.yml +++ b/config/locales/es/budgets.yml @@ -3,8 +3,12 @@ es: ballots: show: title: Mis votos - amount_spent: "Coste total %{amount}" - remaining: "Te quedan %{amount} para invertir" + amount_available: + knapsack: "Te quedan %{amount} para invertir" + amount_spent: + knapsack: "Coste total %{amount}" + amount_limit: + knapsack: "%{amount}" no_balloted_group_yet: "Todavía no has votado proyectos de este grupo, ¡vota!" remove: Quitar voto voted: @@ -84,10 +88,12 @@ es: other: " que contienen '%{search_term}'" sidebar: my_ballot: Mis votos - voted: - one: "Has votado un proyecto por un valor de %{amount_spent}" - other: "Has votado %{count} proyectos por un valor de %{amount_spent}" - change_vote_info: "Puedes %{link} en cualquier momento hasta el cierre de esta fase. No hace falta que gastes todo el dinero disponible." + voted_info: + knapsack: + one: "Has votado un proyecto por un valor de %{amount_spent}" + other: "Has votado %{count} proyectos por un valor de %{amount_spent}" + change_vote_info: + knapsack: "Puedes %{link} en cualquier momento hasta el cierre de esta fase. No hace falta que gastes todo el dinero disponible." change_vote_link: "cambiar tus votos" different_heading_assigned: "Ya apoyaste proyectos de otra sección del presupuesto: %{heading_link}" change_ballot: "Si cambias de opinión puedes borrar tus votos en %{check_ballot} y volver a empezar." From 160964fcdc551543c6196c53357cfd53ceae595e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 13 Jul 2020 14:18:05 +0200 Subject: [PATCH 11/15] Make method to check a line can be added generic In the Knapsack voting style, we can't add an investment if its cost is greater than the money we've got left, but in other voting styles money might not be the issue. So we're introducing the term "resources" and adapting the code accordingly. --- app/models/budget/ballot.rb | 5 +++-- app/models/budget/ballot/line.rb | 9 ++++++--- app/models/budget/investment.rb | 3 ++- app/models/budget/voting_styles/knapsack.rb | 10 +++++++++- 4 files changed, 20 insertions(+), 7 deletions(-) diff --git a/app/models/budget/ballot.rb b/app/models/budget/ballot.rb index 074bf6a8f..399982d8e 100644 --- a/app/models/budget/ballot.rb +++ b/app/models/budget/ballot.rb @@ -64,8 +64,9 @@ class Budget @voting_style ||= voting_style_class.new(self) end delegate :amount_available, :amount_available_info, :amount_spent, :amount_spent_info, - :amount_limit_info, :change_vote_info, :enough_money?, :formatted_amount_available, - :formatted_amount_limit, :formatted_amount_spent, :voted_info, + :amount_limit_info, :change_vote_info, :enough_resources?, :formatted_amount_available, + :formatted_amount_limit, :formatted_amount_spent, :not_enough_resources_error, + :reason_for_not_being_ballotable, :voted_info, to: :voting_style private diff --git a/app/models/budget/ballot/line.rb b/app/models/budget/ballot/line.rb index 9d9805781..58078de98 100644 --- a/app/models/budget/ballot/line.rb +++ b/app/models/budget/ballot/line.rb @@ -10,7 +10,7 @@ class Budget validates :ballot_id, :investment_id, :heading_id, :group_id, :budget_id, presence: true validate :check_selected - validate :check_sufficient_funds + validate :check_enough_resources validate :check_valid_heading scope :by_investment, ->(investment_id) { where(investment_id: investment_id) } @@ -18,9 +18,12 @@ class Budget before_validation :set_denormalized_ids after_save :store_user_heading - def check_sufficient_funds + def check_enough_resources ballot.lock! - errors.add(:money, "insufficient funds") unless ballot.enough_money?(investment) + + unless ballot.enough_resources?(investment) + errors.add(:resources, ballot.not_enough_resources_error) + end end def check_valid_heading diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index 63f03441a..4adf23556 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -268,8 +268,9 @@ class Budget return :not_selected unless selected? return :no_ballots_allowed unless budget.balloting? return :different_heading_assigned unless ballot.valid_heading?(heading) - return :not_enough_money unless ballot.enough_money?(self) return :casted_offline if ballot.casted_offline? + + ballot.reason_for_not_being_ballotable(self) end def permission_problem(user) diff --git a/app/models/budget/voting_styles/knapsack.rb b/app/models/budget/voting_styles/knapsack.rb index 815a19102..4f2c72a19 100644 --- a/app/models/budget/voting_styles/knapsack.rb +++ b/app/models/budget/voting_styles/knapsack.rb @@ -1,8 +1,16 @@ class Budget::VotingStyles::Knapsack < Budget::VotingStyles::Base - def enough_money?(investment) + def enough_resources?(investment) investment.price.to_i <= amount_available(investment.heading) end + def reason_for_not_being_ballotable(investment) + :not_enough_money unless enough_resources?(investment) + end + + def not_enough_resources_error + "insufficient funds" + end + def amount_available(heading) amount_limit(heading) - amount_spent(heading) end From ceee25fdc9546ff5bbc62339efc575f52ec87c8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 28 Jul 2020 20:25:48 +0200 Subject: [PATCH 12/15] Don't re-render the ballot twice We were rendering an individual ballot, and then rendering all ballots (including the already rendered one). So we can skip the first part, as pointed out by microweb10 in the comments of pull request 3036. --- app/views/budgets/ballot/lines/create.js.erb | 4 ---- app/views/budgets/ballot/lines/destroy.js.erb | 4 ---- 2 files changed, 8 deletions(-) diff --git a/app/views/budgets/ballot/lines/create.js.erb b/app/views/budgets/ballot/lines/create.js.erb index 22617ef02..e6569e69a 100644 --- a/app/views/budgets/ballot/lines/create.js.erb +++ b/app/views/budgets/ballot/lines/create.js.erb @@ -1,9 +1,5 @@ $("#progress_bar").html("<%= j render("/budgets/ballot/progress_bar", ballot: @ballot) %>"); $("#sidebar").html("<%= j render("/budgets/investments/sidebar") %>"); -$("#<%= dom_id(@investment) %>_ballot").html("<%= j render("/budgets/investments/ballot", - investment: @investment, - investment_ids: @investment_ids, - ballot: @ballot) %>"); <%= render "refresh_ballots", investment_ids: @investment_ids, diff --git a/app/views/budgets/ballot/lines/destroy.js.erb b/app/views/budgets/ballot/lines/destroy.js.erb index 078fae9a9..106cac60d 100644 --- a/app/views/budgets/ballot/lines/destroy.js.erb +++ b/app/views/budgets/ballot/lines/destroy.js.erb @@ -2,10 +2,6 @@ $("#progress_bar").html("<%= j render("budgets/ballot/progress_bar", ballot: @ba $("#sidebar").html("<%= j render("budgets/investments/sidebar") %>"); $("#ballot").html("<%= j render("budgets/ballot/ballot") %>") -$("#<%= dom_id(@investment) %>_ballot").html("<%= j render("/budgets/investments/ballot", - investment: @investment, - investment_ids: @investment_ids, - ballot: @ballot) %>"); <%= render "refresh_ballots", investment_ids: @investment_ids, ballot: @ballot %> From 8edcbcfd3b6b80cacd1768a2f0e3aea37ae3875f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 30 Jul 2020 13:58:07 +0200 Subject: [PATCH 13/15] Fix aria-valuenow attibute in ballot progress bar We were setting it to 0, and so screen reader users might be confused by it. The easiest way to reuse the code and using it for both this attribute and the width of the progress bar is to move this method to the voting style, just like the other methods used in this view. Note the progressbar ARIA role might not be right, since this isn't a task which is "progressing", but an indicator of the amount spent and amount available, which is exactly what the HTML5 tag was designed for. We might use a tag in the future. For now, I'm leaving it as it is because I'm not certain about how well is supported in accessibility tools, and because it's definitely not supported in Internet Explorer 11, which we haven't officially dropped support for. --- app/helpers/ballots_helper.rb | 5 ----- app/models/budget/ballot.rb | 2 +- app/models/budget/voting_styles/base.rb | 4 ++++ app/views/budgets/ballot/_progress_bar.html.erb | 10 +++------- 4 files changed, 8 insertions(+), 13 deletions(-) delete mode 100644 app/helpers/ballots_helper.rb diff --git a/app/helpers/ballots_helper.rb b/app/helpers/ballots_helper.rb deleted file mode 100644 index c6fde9c57..000000000 --- a/app/helpers/ballots_helper.rb +++ /dev/null @@ -1,5 +0,0 @@ -module BallotsHelper - def progress_bar_width(amount_available, amount_spent) - (amount_spent / amount_available.to_f * 100).to_s + "%" - end -end diff --git a/app/models/budget/ballot.rb b/app/models/budget/ballot.rb index 399982d8e..34939adcd 100644 --- a/app/models/budget/ballot.rb +++ b/app/models/budget/ballot.rb @@ -66,7 +66,7 @@ class Budget delegate :amount_available, :amount_available_info, :amount_spent, :amount_spent_info, :amount_limit_info, :change_vote_info, :enough_resources?, :formatted_amount_available, :formatted_amount_limit, :formatted_amount_spent, :not_enough_resources_error, - :reason_for_not_being_ballotable, :voted_info, + :percentage_spent, :reason_for_not_being_ballotable, :voted_info, to: :voting_style private diff --git a/app/models/budget/voting_styles/base.rb b/app/models/budget/voting_styles/base.rb index e407641db..87bca2d59 100644 --- a/app/models/budget/voting_styles/base.rb +++ b/app/models/budget/voting_styles/base.rb @@ -34,6 +34,10 @@ class Budget::VotingStyles::Base amount: formatted_amount_limit(heading)) end + def percentage_spent(heading) + 100.0 * amount_spent(heading) / amount_limit(heading) + end + private def investments(heading) diff --git a/app/views/budgets/ballot/_progress_bar.html.erb b/app/views/budgets/ballot/_progress_bar.html.erb index 06206b7da..a068016ba 100644 --- a/app/views/budgets/ballot/_progress_bar.html.erb +++ b/app/views/budgets/ballot/_progress_bar.html.erb @@ -4,20 +4,16 @@

+ aria-valuenow="<%= @ballot.percentage_spent(@heading) %>" aria-valuemin="0" aria-valuemax="100">
+ style="width: <%= @ballot.percentage_spent(@heading) %>%">
+ style="width: <%= @ballot.percentage_spent(@heading) %>%">

<%= t("budgets.progress_bar.assigned") %><%= @ballot.formatted_amount_spent(@heading) %> From 2216cb91d12ccdfcc4afad1aadeb8aad706dec5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 29 Jul 2020 14:12:28 +0200 Subject: [PATCH 14/15] Use local variables in progress bar partial We were even passing the `ballot` local variable in some places, which was ignored because we were using instace variables. --- app/views/budgets/ballot/_progress_bar.html.erb | 12 ++++++------ app/views/budgets/ballot/lines/create.js.erb | 2 +- app/views/budgets/ballot/lines/destroy.js.erb | 2 +- app/views/budgets/investments/_header.html.erb | 2 +- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/app/views/budgets/ballot/_progress_bar.html.erb b/app/views/budgets/ballot/_progress_bar.html.erb index a068016ba..f556ee10f 100644 --- a/app/views/budgets/ballot/_progress_bar.html.erb +++ b/app/views/budgets/ballot/_progress_bar.html.erb @@ -1,24 +1,24 @@ - <%= sanitize(@ballot.amount_limit_info(@heading)) %> + <%= sanitize(ballot.amount_limit_info(heading)) %>

+ aria-valuenow="<%= ballot.percentage_spent(heading) %>" aria-valuemin="0" aria-valuemax="100">
+ style="width: <%= ballot.percentage_spent(heading) %>%">
+ style="width: <%= ballot.percentage_spent(heading) %>%">

- <%= t("budgets.progress_bar.assigned") %><%= @ballot.formatted_amount_spent(@heading) %> + <%= t("budgets.progress_bar.assigned") %><%= ballot.formatted_amount_spent(heading) %> <%= t("budgets.progress_bar.available") %> - <%= @ballot.formatted_amount_available(@heading) %> + <%= ballot.formatted_amount_available(heading) %>

diff --git a/app/views/budgets/ballot/lines/create.js.erb b/app/views/budgets/ballot/lines/create.js.erb index e6569e69a..48e56054f 100644 --- a/app/views/budgets/ballot/lines/create.js.erb +++ b/app/views/budgets/ballot/lines/create.js.erb @@ -1,4 +1,4 @@ -$("#progress_bar").html("<%= j render("/budgets/ballot/progress_bar", ballot: @ballot) %>"); +$("#progress_bar").html("<%= j render("/budgets/ballot/progress_bar", ballot: @ballot, heading: @heading) %>"); $("#sidebar").html("<%= j render("/budgets/investments/sidebar") %>"); <%= render "refresh_ballots", diff --git a/app/views/budgets/ballot/lines/destroy.js.erb b/app/views/budgets/ballot/lines/destroy.js.erb index 106cac60d..c93517414 100644 --- a/app/views/budgets/ballot/lines/destroy.js.erb +++ b/app/views/budgets/ballot/lines/destroy.js.erb @@ -1,4 +1,4 @@ -$("#progress_bar").html("<%= j render("budgets/ballot/progress_bar", ballot: @ballot) %>"); +$("#progress_bar").html("<%= j render("budgets/ballot/progress_bar", ballot: @ballot, heading: @heading) %>"); $("#sidebar").html("<%= j render("budgets/investments/sidebar") %>"); $("#ballot").html("<%= j render("budgets/ballot/ballot") %>") diff --git a/app/views/budgets/investments/_header.html.erb b/app/views/budgets/investments/_header.html.erb index a5c63f160..0b18c3e1f 100644 --- a/app/views/budgets/investments/_header.html.erb +++ b/app/views/budgets/investments/_header.html.erb @@ -27,7 +27,7 @@ <%= t("budgets.investments.index.by_heading", heading: @heading.name) %>
- <%= render "budgets/ballot/progress_bar" %> + <%= render "budgets/ballot/progress_bar", ballot: @ballot, heading: @heading %>
<% else %> From 82ef5149c5ea31e41edf9be0cc0579862748378c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 29 Jul 2020 18:33:45 +0200 Subject: [PATCH 15/15] Remove redundant progress bar We were displaying two progress bars for the same thing, and hiding one of them. Displaying just one of them and readjusting the styles accordingly is a bit more intuitive IMHO. We're also getting the text inside the progress bar out of it; its purpose inside an element with the `progressbar` role is to provide the same information as the progress bar (which we aren't exactly doing, although it could be argued that we do), and in order to be accessible we should provide the same text in the `aria-valuetext` field, which we aren't doing. This also simplifies our CSS, which was working because we defined a padding which covered the height of the hidden extra progress bar and would have needed quite a few changes if we kept just one progress bar with text inside it. We can also remove a few CSS rules which we added to override foundation's rules for the `progress-meter-text` class. --- app/assets/stylesheets/participation.scss | 22 ++++++------------- .../budgets/ballot/_progress_bar.html.erb | 19 +++++----------- 2 files changed, 13 insertions(+), 28 deletions(-) diff --git a/app/assets/stylesheets/participation.scss b/app/assets/stylesheets/participation.scss index d1b1325ba..8485b5468 100644 --- a/app/assets/stylesheets/participation.scss +++ b/app/assets/stylesheets/participation.scss @@ -1285,6 +1285,7 @@ .progress { background: #212033; clear: both; + margin-bottom: 0; } .progress-meter { @@ -1293,29 +1294,20 @@ transition: width 2s; } - .spent-amount-progress, - .spent-amount-meter { - background: none !important; - } - .spent-amount-text { - color: #fff; - font-size: $base-font-size; - font-weight: normal; - position: absolute; - right: 0; + margin-bottom: 0; + position: relative; text-align: right; - top: 16px; - width: 100%; + white-space: nowrap; &::before { color: #a5a1ff; content: "\57"; font-family: "icons"; font-size: $small-font-size; + line-height: 0; position: absolute; - right: -6px; - top: -17px; + right: -0.5em; } } @@ -1328,7 +1320,6 @@ .amount-available { display: block; - text-align: right; span { font-size: rem-calc(24); @@ -1486,6 +1477,7 @@ height: auto; left: 0; padding: $line-height; + padding-bottom: $line-height / 2; position: fixed; top: 0; width: 100%; diff --git a/app/views/budgets/ballot/_progress_bar.html.erb b/app/views/budgets/ballot/_progress_bar.html.erb index f556ee10f..b3c563240 100644 --- a/app/views/budgets/ballot/_progress_bar.html.erb +++ b/app/views/budgets/ballot/_progress_bar.html.erb @@ -3,23 +3,16 @@
-
- -

- <%= t("budgets.progress_bar.assigned") %><%= ballot.formatted_amount_spent(heading) %> - - <%= t("budgets.progress_bar.available") %> - <%= ballot.formatted_amount_available(heading) %> - -

+

+ <%= t("budgets.progress_bar.assigned") %><%= ballot.formatted_amount_spent(heading) %> + + <%= t("budgets.progress_bar.available") %> + <%= ballot.formatted_amount_available(heading) %> -

+