From b364fc475e38182288ca291ec44b615b63257f09 Mon Sep 17 00:00:00 2001 From: Bertocq Date: Wed, 11 Apr 2018 21:05:04 +0200 Subject: [PATCH 1/6] Move assigned_valuators from helper to model There's no good reason to call assigned_valuators(investment) when the logic can be at the model. Also removed the valuator_groups texts to be added in an independent method --- app/helpers/valuation_helper.rb | 5 ----- app/models/budget/investment.rb | 8 ++++++++ app/views/admin/budget_investments/_investments.html.erb | 2 +- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/app/helpers/valuation_helper.rb b/app/helpers/valuation_helper.rb index 311f8e6ec..1c11cb90b 100644 --- a/app/helpers/valuation_helper.rb +++ b/app/helpers/valuation_helper.rb @@ -13,11 +13,6 @@ module ValuationHelper ValuatorGroup.order("name ASC").collect { |g| [ g.name, "group_#{g.id}"] } end - def assigned_valuators(investment) - [investment.valuator_groups.collect(&:name) + - investment.valuators.collect(&:description_or_name)].flatten.join(', ') - end - def assigned_valuators_info(valuators) case valuators.size when 0 diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index 594610b6c..c913a562f 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -320,6 +320,14 @@ class Budget investments end + def assigned_valuators + self.valuators.collect(&:description_or_name).compact.join(', ').presence + end + + def assigned_valuation_groups + self.valuator_groups.collect(&:name).compact.join(', ').presence + end + def self.to_csv(investments, options = {}) attrs = [I18n.t("admin.budget_investments.index.table_id"), I18n.t("admin.budget_investments.index.table_title"), diff --git a/app/views/admin/budget_investments/_investments.html.erb b/app/views/admin/budget_investments/_investments.html.erb index 9d52f78d6..aaca20fe8 100644 --- a/app/views/admin/budget_investments/_investments.html.erb +++ b/app/views/admin/budget_investments/_investments.html.erb @@ -63,7 +63,7 @@ <% if investment.valuators.size == 0 && investment.valuator_groups.size == 0 %> <%= t("admin.budget_investments.index.no_valuators_assigned") %> <% else %> - <%= assigned_valuators(investment) %> + <%= investment.assigned_valuators %> <% end %> From 1dc8d29f2d1a64badc7c1df30fadb769cfbbcda4 Mon Sep 17 00:00:00 2001 From: Bertocq Date: Wed, 11 Apr 2018 21:12:46 +0200 Subject: [PATCH 2/6] Add Valuator Groups list to investment csv & table We've added the list of valuator groups assigned to each investment at both admin investment list and investment csv exported file --- app/models/budget/investment.rb | 7 +++++-- .../budget_investments/_investments.html.erb | 12 +++++++----- config/locales/en/admin.yml | 2 ++ config/locales/es/admin.yml | 2 ++ spec/features/admin/budget_investments_spec.rb | 16 ++++++++++++---- 5 files changed, 28 insertions(+), 11 deletions(-) diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index c913a562f..6c96578c0 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -334,6 +334,7 @@ class Budget I18n.t("admin.budget_investments.index.table_supports"), I18n.t("admin.budget_investments.index.table_admin"), I18n.t("admin.budget_investments.index.table_valuator"), + I18n.t("admin.budget_investments.index.table_valuation_group"), I18n.t("admin.budget_investments.index.table_geozone"), I18n.t("admin.budget_investments.index.table_feasibility"), I18n.t("admin.budget_investments.index.table_valuation_finished"), @@ -349,6 +350,8 @@ class Budget else I18n.t("admin.budget_investments.index.no_admin_assigned") end + assigned_valuators = investment.assigned_valuators || '-' + assigned_valuation_groups = investment.assigned_valuation_groups || '-' vals = if investment.valuators.empty? I18n.t("admin.budget_investments.index.no_valuators_assigned") else @@ -361,8 +364,8 @@ class Budget valuation_finished = investment.valuation_finished? ? I18n.t('shared.yes') : I18n.t('shared.no') - csv << [id, title, total_votes, admin, vals, heading_name, price, - valuation_finished] + csv << [id, title, total_votes, admin, assigned_valuators, assigned_valuation_groups, + heading_name, price, valuation_finished] end end csv_string diff --git a/app/views/admin/budget_investments/_investments.html.erb b/app/views/admin/budget_investments/_investments.html.erb index aaca20fe8..ed6d784ef 100644 --- a/app/views/admin/budget_investments/_investments.html.erb +++ b/app/views/admin/budget_investments/_investments.html.erb @@ -24,6 +24,7 @@ <%= t("admin.budget_investments.index.table_supports") %> <%= t("admin.budget_investments.index.table_admin") %> <%= t("admin.budget_investments.index.table_valuator") %> + <%= t("admin.budget_investments.index.table_valuation_group") %> <%= t("admin.budget_investments.index.table_geozone") %> <%= t("admin.budget_investments.index.table_feasibility") %> <%= t("admin.budget_investments.index.table_valuation_finished") %> @@ -60,11 +61,12 @@ <% end %> - <% if investment.valuators.size == 0 && investment.valuator_groups.size == 0 %> - <%= t("admin.budget_investments.index.no_valuators_assigned") %> - <% else %> - <%= investment.assigned_valuators %> - <% end %> + <% no_valuators_assigned = t("admin.budget_investments.index.no_valuators_assigned") %> + <%= investment.assigned_valuators || no_valuators_assigned %> + + + <% no_valuation_groups = t("admin.budget_investments.index.no_valuation_groups") %> + <%= investment.assigned_valuation_groups || no_valuation_groups %> <%= investment.heading.name %> diff --git a/config/locales/en/admin.yml b/config/locales/en/admin.yml index 07837319c..671049163 100644 --- a/config/locales/en/admin.yml +++ b/config/locales/en/admin.yml @@ -171,6 +171,7 @@ en: assigned_admin: Assigned administrator no_admin_assigned: No admin assigned no_valuators_assigned: No valuators assigned + no_valuation_groups: No valuation groups assigned feasibility: feasible: "Feasible (%{price})" unfeasible: "Unfeasible" @@ -182,6 +183,7 @@ en: table_supports: "Supports" table_admin: "Administrator" table_valuator: "Valuator" + table_valuation_group: "Valuation Group" table_geozone: "Scope of operation" table_feasibility: "Feasibility" table_valuation_finished: "Val. Fin." diff --git a/config/locales/es/admin.yml b/config/locales/es/admin.yml index 64e7dafd0..6718f6891 100644 --- a/config/locales/es/admin.yml +++ b/config/locales/es/admin.yml @@ -171,6 +171,7 @@ es: assigned_admin: Administrador asignado no_admin_assigned: Sin admin asignado no_valuators_assigned: Sin evaluador + no_valuation_groups: Sin grupos evaluadores feasibility: feasible: "Viable (%{price})" unfeasible: "Inviable" @@ -182,6 +183,7 @@ es: table_supports: "Apoyos" table_admin: "Administrador" table_valuator: "Evaluador" + table_valuation_group: "Grupos evaluadores" table_geozone: "Ámbito de actuación" table_feasibility: "Viabilidad" table_valuation_finished: "Ev. Fin." diff --git a/spec/features/admin/budget_investments_spec.rb b/spec/features/admin/budget_investments_spec.rb index 9a44249b4..8bf49f1a7 100644 --- a/spec/features/admin/budget_investments_spec.rb +++ b/spec/features/admin/budget_investments_spec.rb @@ -64,22 +64,27 @@ feature 'Admin budget investments' do admin = create(:administrator, user: create(:user, username: 'Gema')) budget_investment1.valuators << valuator1 - budget_investment2.valuator_ids = [valuator1.id, valuator2.id] - budget_investment3.update(administrator_id: admin.id) + budget_investment2.valuators << valuator1 + budget_investment2.valuators << valuator2 visit admin_budget_budget_investments_path(budget_id: budget.id) within("#budget_investment_#{budget_investment1.id}") do expect(page).to have_content("No admin assigned") expect(page).to have_content("Valuator Olga") + expect(page).to have_content("No valuation groups assigned") end within("#budget_investment_#{budget_investment2.id}") do expect(page).to have_content("No admin assigned") expect(page).to have_content("Valuator Olga") expect(page).to have_content("Valuator Miriam") + expect(page).to have_content("No valuation groups assigned") end + budget_investment3.update(administrator_id: admin.id) + visit admin_budget_budget_investments_path(budget_id: budget.id) + within("#budget_investment_#{budget_investment3.id}") do expect(page).to have_content("Gema") expect(page).to have_content("No valuators assigned") @@ -109,7 +114,7 @@ feature 'Admin budget investments' do end within("#budget_investment_#{budget_investment3.id}") do - expect(page).to have_content("No valuators assigned") + expect(page).to have_content("No valuation groups assigned") end end @@ -953,7 +958,9 @@ feature 'Admin budget investments' do price: 100) valuator = create(:valuator, user: create(:user, username: 'Rachel', email: 'rachel@val.org')) - investment.valuators.push(valuator) + group = create(:valuator_group, name: "Test name") + + investment.valuator_groups << group admin = create(:administrator, user: create(:user, username: 'Gema')) investment.update(administrator_id: admin.id) @@ -978,6 +985,7 @@ feature 'Admin budget investments' do expect(page).to have_content investment.administrator.name expect(page).to have_content valuators + expect(page).to have_content group.name expect(page).to have_content price expect(page).to have_content I18n.t('shared.no') end From 83f4f4f65c561353976a9839c2be38b37c49bfaa Mon Sep 17 00:00:00 2001 From: Bertocq Date: Wed, 11 Apr 2018 17:19:03 +0200 Subject: [PATCH 3/6] Remove unnecesary parameter at Investment to_csv If there's only one usage of `to_csv` and the parameter has always the same value... there's no good reason to bother using an additional argument. --- app/controllers/admin/budget_investments_controller.rb | 2 +- app/models/budget/investment.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/admin/budget_investments_controller.rb b/app/controllers/admin/budget_investments_controller.rb index bc4c8d49b..c85cbd98f 100644 --- a/app/controllers/admin/budget_investments_controller.rb +++ b/app/controllers/admin/budget_investments_controller.rb @@ -20,7 +20,7 @@ class Admin::BudgetInvestmentsController < Admin::BaseController format.html format.js format.csv do - send_data Budget::Investment.to_csv(@investments, headers: true), + send_data Budget::Investment.to_csv(@investments), filename: 'budget_investments.csv' end end diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index 6c96578c0..24d038d14 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -328,7 +328,7 @@ class Budget self.valuator_groups.collect(&:name).compact.join(', ').presence end - def self.to_csv(investments, options = {}) + def self.to_csv(investments) attrs = [I18n.t("admin.budget_investments.index.table_id"), I18n.t("admin.budget_investments.index.table_title"), I18n.t("admin.budget_investments.index.table_supports"), @@ -339,7 +339,7 @@ class Budget I18n.t("admin.budget_investments.index.table_feasibility"), I18n.t("admin.budget_investments.index.table_valuation_finished"), I18n.t("admin.budget_investments.index.table_selection")] - csv_string = CSV.generate(options) do |csv| + csv_string = CSV.generate(headers: true) do |csv| csv << attrs investments.each do |investment| id = investment.id.to_s From aacac713624f4059e37d594a3fb16b54906ad457 Mon Sep 17 00:00:00 2001 From: Bertocq Date: Wed, 11 Apr 2018 17:32:13 +0200 Subject: [PATCH 4/6] Extract Budget Investment to_csv to its own class The csv generation doesn't seem like a Model concern, at least not taking into account the amount of lines of the method (36+). Just a simple ruby class that encapsulates the logic makes it easier to read and maintain as we increase the columns exported.. also customize in case other forks need different values. --- .../admin/budget_investments_controller.rb | 2 +- app/models/budget/investment.rb | 45 -------------- app/models/budget/investment/exporter.rb | 59 +++++++++++++++++++ 3 files changed, 60 insertions(+), 46 deletions(-) create mode 100644 app/models/budget/investment/exporter.rb diff --git a/app/controllers/admin/budget_investments_controller.rb b/app/controllers/admin/budget_investments_controller.rb index c85cbd98f..7eee73a5d 100644 --- a/app/controllers/admin/budget_investments_controller.rb +++ b/app/controllers/admin/budget_investments_controller.rb @@ -20,7 +20,7 @@ class Admin::BudgetInvestmentsController < Admin::BaseController format.html format.js format.csv do - send_data Budget::Investment.to_csv(@investments), + send_data Budget::Investment::Exporter.new(@investments).to_csv, filename: 'budget_investments.csv' end end diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index 24d038d14..4f2775826 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -1,5 +1,3 @@ -require 'csv' - class Budget class Investment < ActiveRecord::Base SORTING_OPTIONS = %w(id title supports).freeze @@ -328,49 +326,6 @@ class Budget self.valuator_groups.collect(&:name).compact.join(', ').presence end - def self.to_csv(investments) - attrs = [I18n.t("admin.budget_investments.index.table_id"), - I18n.t("admin.budget_investments.index.table_title"), - I18n.t("admin.budget_investments.index.table_supports"), - I18n.t("admin.budget_investments.index.table_admin"), - I18n.t("admin.budget_investments.index.table_valuator"), - I18n.t("admin.budget_investments.index.table_valuation_group"), - I18n.t("admin.budget_investments.index.table_geozone"), - I18n.t("admin.budget_investments.index.table_feasibility"), - I18n.t("admin.budget_investments.index.table_valuation_finished"), - I18n.t("admin.budget_investments.index.table_selection")] - csv_string = CSV.generate(headers: true) do |csv| - csv << attrs - investments.each do |investment| - id = investment.id.to_s - title = investment.title - total_votes = investment.total_votes.to_s - admin = if investment.administrator.present? - investment.administrator.name - else - I18n.t("admin.budget_investments.index.no_admin_assigned") - end - assigned_valuators = investment.assigned_valuators || '-' - assigned_valuation_groups = investment.assigned_valuation_groups || '-' - vals = if investment.valuators.empty? - I18n.t("admin.budget_investments.index.no_valuators_assigned") - else - investment.valuators.collect(&:description_or_name).join(', ') - end - heading_name = investment.heading.name - price_string = "admin.budget_investments.index.feasibility"\ - ".#{investment.feasibility}" - price = I18n.t(price_string, price: investment.formatted_price) - valuation_finished = investment.valuation_finished? ? - I18n.t('shared.yes') : - I18n.t('shared.no') - csv << [id, title, total_votes, admin, assigned_valuators, assigned_valuation_groups, - heading_name, price, valuation_finished] - end - end - csv_string - end - private def set_denormalized_ids diff --git a/app/models/budget/investment/exporter.rb b/app/models/budget/investment/exporter.rb new file mode 100644 index 000000000..03900fd7e --- /dev/null +++ b/app/models/budget/investment/exporter.rb @@ -0,0 +1,59 @@ +class Budget::Investment::Exporter + require 'csv' + + def initialize(investments) + @investments = investments + end + + def to_csv + CSV.generate(headers: true) do |csv| + csv << headers + @investments.each { |investment| csv << csv_values(investment) } + end + end + + private + + def headers + [ + I18n.t("admin.budget_investments.index.table_id"), + I18n.t("admin.budget_investments.index.table_title"), + I18n.t("admin.budget_investments.index.table_supports"), + I18n.t("admin.budget_investments.index.table_admin"), + I18n.t("admin.budget_investments.index.table_valuator"), + I18n.t("admin.budget_investments.index.table_valuation_group"), + I18n.t("admin.budget_investments.index.table_geozone"), + I18n.t("admin.budget_investments.index.table_feasibility"), + I18n.t("admin.budget_investments.index.table_valuation_finished"), + I18n.t("admin.budget_investments.index.table_selection") + ] + end + + def csv_values(investment) + [ + investment.id.to_s, + investment.title, + investment.total_votes.to_s, + admin(investment), + investment.assigned_valuators || '-', + investment.assigned_valuation_groups || '-', + investment.heading.name, + price(investment), + investment.valuation_finished? ? I18n.t('shared.yes') : I18n.t('shared.no'), + investment.selected? ? I18n.t('shared.yes') : I18n.t('shared.no') + ] + end + + def admin(investment) + if investment.administrator.present? + investment.administrator.name + else + I18n.t("admin.budget_investments.index.no_admin_assigned") + end + end + + def price(investment) + price_string = "admin.budget_investments.index.feasibility.#{investment.feasibility}" + I18n.t(price_string, price: investment.formatted_price) + end +end From 71003875aed195662d7be70622933926aa723ba7 Mon Sep 17 00:00:00 2001 From: Bertocq Date: Wed, 11 Apr 2018 18:14:05 +0200 Subject: [PATCH 5/6] Refactor Investment CSV export download scenario The created investment didn't had all data to correctly assert each column values are correctly exported. The expectations checking if each particular text appears are invalid in this test. The objective is to check that the downloaded file contents are exactly as they should be... not particular parts checked in an independent way as for example "Yes" could appear in two different columns and just looking if the text appears is not a valid assertion. --- .../features/admin/budget_investments_spec.rb | 50 +++++++++---------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/spec/features/admin/budget_investments_spec.rb b/spec/features/admin/budget_investments_spec.rb index 8bf49f1a7..f710c8fcb 100644 --- a/spec/features/admin/budget_investments_spec.rb +++ b/spec/features/admin/budget_investments_spec.rb @@ -954,16 +954,25 @@ feature 'Admin budget investments' do context "Selecting csv" do scenario "Downloading CSV file" do - investment = create(:budget_investment, :feasible, budget: budget, - price: 100) - valuator = create(:valuator, user: create(:user, username: 'Rachel', - email: 'rachel@val.org')) - group = create(:valuator_group, name: "Test name") - - investment.valuator_groups << group - - admin = create(:administrator, user: create(:user, username: 'Gema')) - investment.update(administrator_id: admin.id) + admin = create(:administrator, user: create(:user, username: 'Admin')) + valuator = create(:valuator, user: create(:user, username: 'Valuator')) + valuator_group = create(:valuator_group, name: "Valuator Group") + budget_group = create(:budget_group, name: "Budget Group", budget: budget) + first_budget_heading = create(:budget_heading, group: budget_group, name: "Budget Heading") + second_budget_heading = create(:budget_heading, group: budget_group, name: "Other Heading") + first_investment = create(:budget_investment, :feasible, :selected, title: "Le Investment", + budget: budget, group: budget_group, + heading: first_budget_heading, + cached_votes_up: 88, price: 99, + valuators: [], + valuator_groups: [valuator_group], + administrator: admin) + second_investment = create(:budget_investment, :unfeasible, title: "Alt Investment", + budget: budget, group: budget_group, + heading: second_budget_heading, + cached_votes_up: 66, price: 88, + valuators: [valuator], + valuator_groups: []) visit admin_budget_budget_investments_path(budget) @@ -973,21 +982,12 @@ feature 'Admin budget investments' do expect(header).to match(/^attachment/) expect(header).to match(/filename="budget_investments.csv"$/) - valuators = investment.valuators.collect(&:description_or_name).join(', ') - feasibility_string = "admin.budget_investments.index"\ - ".feasibility.#{investment.feasibility}" - price = I18n.t(feasibility_string, price: investment.formatted_price) - - expect(page).to have_content investment.title - expect(page).to have_content investment.total_votes.to_s - expect(page).to have_content investment.id.to_s - expect(page).to have_content investment.heading.name - - expect(page).to have_content investment.administrator.name - expect(page).to have_content valuators - expect(page).to have_content group.name - expect(page).to have_content price - expect(page).to have_content I18n.t('shared.no') + csv_contents = "ID,Title,Supports,Administrator,Valuator,Valuation Group,Scope of operation,"\ + "Feasibility,Val. Fin.,Selected\n#{first_investment.id},Le Investment,88,"\ + "Admin,-,Valuator Group,Budget Heading,Feasible (€99),Yes,Yes\n"\ + "#{second_investment.id},Alt Investment,66,No admin assigned,Valuator,-,"\ + "Other Heading,Unfeasible,No,No\n" + expect(page.body).to eq(csv_contents) end scenario "Downloading CSV file with applied filter" do From 33b6fa3a02c3851f1753812e4de4fd502ea39a3e Mon Sep 17 00:00:00 2001 From: Bertocq Date: Wed, 11 Apr 2018 18:18:09 +0200 Subject: [PATCH 6/6] Refactor Investment csv download with filters test There's no need to check again headers in this scenario, previous one already does it. Correctly naming variables, as well as using explicit expectations is a good idea. Last but not least, expectations where reversed but by luck or lack of attention where passing. --- spec/features/admin/budget_investments_spec.rb | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/spec/features/admin/budget_investments_spec.rb b/spec/features/admin/budget_investments_spec.rb index f710c8fcb..3b75e9b92 100644 --- a/spec/features/admin/budget_investments_spec.rb +++ b/spec/features/admin/budget_investments_spec.rb @@ -991,22 +991,18 @@ feature 'Admin budget investments' do end scenario "Downloading CSV file with applied filter" do - investment1 = create(:budget_investment, :unfeasible, budget: budget, - title: 'compatible') - investment2 = create(:budget_investment, :finished, budget: budget, - title: 'finished') + unfeasible_investment = create(:budget_investment, :unfeasible, budget: budget, + title: 'Unfeasible one') + finished_investment = create(:budget_investment, :finished, budget: budget, + title: 'Finished Investment') visit admin_budget_budget_investments_path(budget) within('#filter-subnav') { click_link 'Valuation finished' } click_link "Download current selection" - header = page.response_headers['Content-Disposition'] - expect(header).to match(/^attachment/) - expect(header).to match(/filename="budget_investments.csv"$/) - - expect(page).to have_content investment2.title - expect(page).not_to have_content investment1.title + expect(page).to have_content('Finished Investment') + expect(page).not_to have_content('Unfeasible one') end end