From 42f62e5c361eab649c9db62974732acbc4a08bca Mon Sep 17 00:00:00 2001 From: rgarcia Date: Wed, 17 May 2017 15:13:28 +0200 Subject: [PATCH 1/3] stores reclassified votes --- app/models/budget/ballot/line.rb | 2 + app/models/budget/investment.rb | 32 +++++++++++--- app/models/budget/reclassified_vote.rb | 12 +++++ ...123042_create_budget_reclassified_votes.rb | 11 +++++ db/schema.rb | 16 ++++++- spec/factories.rb | 6 +++ spec/models/budget/ballot/line_spec.rb | 39 +++++++++++++--- spec/models/budget/investment_spec.rb | 44 +++++++++++++++---- spec/models/budget/reclassified_vote_spec.rb | 40 +++++++++++++++++ 9 files changed, 179 insertions(+), 23 deletions(-) create mode 100644 app/models/budget/reclassified_vote.rb create mode 100644 db/migrate/20170517123042_create_budget_reclassified_votes.rb create mode 100644 spec/models/budget/reclassified_vote_spec.rb diff --git a/app/models/budget/ballot/line.rb b/app/models/budget/ballot/line.rb index 0b56716c6..175e9886d 100644 --- a/app/models/budget/ballot/line.rb +++ b/app/models/budget/ballot/line.rb @@ -13,6 +13,8 @@ class Budget validate :check_sufficient_funds validate :check_valid_heading + scope :by_investment, -> (investment_id) { where(investment_id: investment_id) } + before_validation :set_denormalized_ids def check_sufficient_funds diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index 4f7659aa2..1d4230e0c 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -244,22 +244,40 @@ class Budget end def check_for_reclassification - if reclassified? - log_reclassification - remove_reclassified_votes - end + if heading_changed? + log_heading_change + store_reclassified_votes("heading_changed") + remove_reclassified_votes + elsif marked_as_unfeasible? + store_reclassified_votes("unfeasible") + remove_reclassified_votes + end end - def reclassified? + def heading_changed? budget.balloting? && heading_id_changed? end - def log_reclassification + def log_heading_change update_column(:previous_heading_id, heading_id_was) end + def marked_as_unfeasible? + budget.balloting? && feasibility_changed? && unfeasible? + end + + def store_reclassified_votes(reason) + ballot_lines_for_investment.each do |line| + Budget::ReclassifiedVote.create!(user: line.ballot.user, investment: self, reason: reason) + end + end + def remove_reclassified_votes - Budget::Ballot::Line.where(investment: self).destroy_all + ballot_lines_for_investment.destroy_all + end + + def ballot_lines_for_investment + Budget::Ballot::Line.by_investment(self.id) end private diff --git a/app/models/budget/reclassified_vote.rb b/app/models/budget/reclassified_vote.rb new file mode 100644 index 000000000..0eb44d8cc --- /dev/null +++ b/app/models/budget/reclassified_vote.rb @@ -0,0 +1,12 @@ +class Budget + class ReclassifiedVote < ActiveRecord::Base + REASONS = %w(heading_changed unfeasible) + + belongs_to :user + belongs_to :investment + + validates :user, presence: true + validates :investment, presence: true + validates :reason, inclusion: {in: REASONS, allow_nil: false} + end +end \ No newline at end of file diff --git a/db/migrate/20170517123042_create_budget_reclassified_votes.rb b/db/migrate/20170517123042_create_budget_reclassified_votes.rb new file mode 100644 index 000000000..27cf531d3 --- /dev/null +++ b/db/migrate/20170517123042_create_budget_reclassified_votes.rb @@ -0,0 +1,11 @@ +class CreateBudgetReclassifiedVotes < ActiveRecord::Migration + def change + create_table :budget_reclassified_votes do |t| + t.integer :user_id + t.integer :investment_id + t.string :reason, default: nil + + t.timestamps null: false + end + end +end diff --git a/db/schema.rb b/db/schema.rb index b525da1d1..d1cd6b640 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20170513110025) do +ActiveRecord::Schema.define(version: 20170517123042) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -63,6 +63,12 @@ ActiveRecord::Schema.define(version: 20170513110025) do add_index "annotations", ["legislation_id"], name: "index_annotations_on_legislation_id", using: :btree add_index "annotations", ["user_id"], name: "index_annotations_on_user_id", using: :btree + create_table "ar_internal_metadata", primary_key: "key", force: :cascade do |t| + t.string "value" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + end + create_table "banners", force: :cascade do |t| t.string "title", limit: 80 t.string "description" @@ -154,6 +160,14 @@ ActiveRecord::Schema.define(version: 20170513110025) do add_index "budget_investments", ["heading_id"], name: "index_budget_investments_on_heading_id", using: :btree add_index "budget_investments", ["tsv"], name: "index_budget_investments_on_tsv", using: :gin + create_table "budget_reclassified_votes", force: :cascade do |t| + t.integer "user_id" + t.integer "investment_id" + t.string "reason" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + end + create_table "budget_valuator_assignments", force: :cascade do |t| t.integer "valuator_id" t.integer "investment_id" diff --git a/spec/factories.rb b/spec/factories.rb index f0a4fc5a2..60fc5a872 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -302,6 +302,12 @@ FactoryGirl.define do association :investment, factory: :budget_investment end + factory :budget_reclassified_vote, class: 'Budget::ReclassifiedVote' do + user + association :investment, factory: :budget_investment + reason "unfeasible" + end + factory :vote do association :votable, factory: :debate association :voter, factory: :user diff --git a/spec/models/budget/ballot/line_spec.rb b/spec/models/budget/ballot/line_spec.rb index 7ce4dcbf7..e1da8a485 100644 --- a/spec/models/budget/ballot/line_spec.rb +++ b/spec/models/budget/ballot/line_spec.rb @@ -2,13 +2,14 @@ require 'rails_helper' describe "Budget::Ballot::Line" do + let(:budget){ create(:budget) } + let(:group){ create(:budget_group, budget: budget) } + let(:heading){ create(:budget_heading, group: group, price: 10000000) } + let(:investment){ create(:budget_investment, :selected, price: 5000000, heading: heading) } + let(:ballot) { create(:budget_ballot, budget: budget) } + let(:ballot_line) { build(:budget_ballot_line, ballot: ballot, investment: investment) } + describe 'Validations' do - let(:budget){ create(:budget) } - let(:group){ create(:budget_group, budget: budget) } - let(:heading){ create(:budget_heading, group: group, price: 10000000) } - let(:investment){ create(:budget_investment, :selected, price: 5000000, heading: heading) } - let(:ballot) { create(:budget_ballot, budget: budget) } - let(:ballot_line) { build(:budget_ballot_line, ballot: ballot, investment: investment) } it "should be valid and automatically denormallyze budget, group and heading when validated" do expect(ballot_line).to be_valid @@ -42,4 +43,30 @@ describe "Budget::Ballot::Line" do end end + + describe "scopes" do + + describe "by_investment" do + + it "should return ballot lines for an investment" do + investment1 = create(:budget_investment, :selected, heading: heading) + investment2 = create(:budget_investment, :selected, heading: heading) + + ballot1 = create(:budget_ballot, budget: budget) + ballot2 = create(:budget_ballot, budget: budget) + ballot3 = create(:budget_ballot, budget: budget) + + ballot_line1 = create(:budget_ballot_line, ballot: ballot1, investment: investment1) + ballot_line2 = create(:budget_ballot_line, ballot: ballot2, investment: investment1) + ballot_line3 = create(:budget_ballot_line, ballot: ballot3, investment: investment2) + + ballot_lines_by_investment = Budget::Ballot::Line.by_investment(investment1.id) + + expect(ballot_lines_by_investment).to include ballot_line1 + expect(ballot_lines_by_investment).to include ballot_line2 + expect(ballot_lines_by_investment).to_not include ballot_line3 + end + + end + end end diff --git a/spec/models/budget/investment_spec.rb b/spec/models/budget/investment_spec.rb index 5c8406970..5b922cf95 100644 --- a/spec/models/budget/investment_spec.rb +++ b/spec/models/budget/investment_spec.rb @@ -681,27 +681,27 @@ describe Budget::Investment do end - describe "Reclassification" do + describe "Reclassification", :focus do let(:budget) { create(:budget, phase: "balloting") } let(:group) { create(:budget_group, budget: budget) } let(:heading1) { create(:budget_heading, group: group) } let(:heading2) { create(:budget_heading, group: group) } - describe "reclassified?" do + describe "heading_changed?" do it "returns true if budget is in balloting phase and heading has changed" do investment = create(:budget_investment, heading: heading1) investment.heading = heading2 - expect(investment.reclassified?).to eq(true) + expect(investment.heading_changed?).to eq(true) end it "returns false if heading has not changed" do investment = create(:budget_investment) investment.heading = investment.heading - expect(investment.reclassified?).to eq(false) + expect(investment.heading_changed?).to eq(false) end it "returns false if budget is not balloting phase" do @@ -711,13 +711,13 @@ describe Budget::Investment do investment.heading = heading2 - expect(investment.reclassified?).to eq(false) + expect(investment.heading_changed?).to eq(false) end end end - describe "log_reclassification" do + describe "log_heading_change" do it "stores the previous heading before being reclassified" do investment = create(:budget_investment, heading: heading1) @@ -735,6 +735,30 @@ describe Budget::Investment do end + describe "store_reclassified_votes" do + + it "stores the votes for a reclassified investment", :focus do + investment = create(:budget_investment, :selected, heading: heading1) + + 3.times do + ballot = create(:budget_ballot, budget: budget) + ballot.investments << investment + end + + expect(investment.ballot_lines_count).to eq(3) + + investment.heading = heading2 + investment.store_reclassified_votes("heading_changed") + + reclassified_vote = Budget::ReclassifiedVote.first + + expect(Budget::ReclassifiedVote.count).to eq(3) + expect(reclassified_vote.investment_id).to eq(investment.id) + expect(reclassified_vote.user_id).to eq(Budget::Ballot.first.user.id) + expect(reclassified_vote.reason).to eq("heading_changed") + end + end + describe "remove_reclassified_votes" do it "removes votes from invesment" do @@ -756,9 +780,9 @@ describe Budget::Investment do end - describe "check_for_reclassification" do + describe "check_for_reclassification", :focus do - it "removes votes if an investment has been reclassified" do + it "stores reclassfied votes and removes actual votes if an investment has been reclassified" do investment = create(:budget_investment, :selected, heading: heading1) 3.times do @@ -773,9 +797,10 @@ describe Budget::Investment do investment.reload expect(investment.ballot_lines_count).to eq(0) + expect(Budget::ReclassifiedVote.count).to eq(3) end - it "does not remove votes if the investment has not been reclassifed" do + it "does not store reclassified votes nor remove actual votes if the investment has not been reclassifed" do investment = create(:budget_investment, :selected, heading: heading1) 3.times do @@ -789,6 +814,7 @@ describe Budget::Investment do investment.reload expect(investment.ballot_lines_count).to eq(3) + expect(Budget::ReclassifiedVote.count).to eq(0) end end diff --git a/spec/models/budget/reclassified_vote_spec.rb b/spec/models/budget/reclassified_vote_spec.rb new file mode 100644 index 000000000..bf689039d --- /dev/null +++ b/spec/models/budget/reclassified_vote_spec.rb @@ -0,0 +1,40 @@ +require 'rails_helper' + +describe Budget::ReclassifiedVote do + + describe "Validations", :focus do + let(:reclassified_vote) { build(:budget_reclassified_vote) } + + it "should be valid" do + expect(reclassified_vote).to be_valid + end + + it "should not be valid without a user" do + reclassified_vote.user_id = nil + expect(reclassified_vote).to_not be_valid + end + + it "should not be valid without an investment" do + reclassified_vote.investment_id = nil + expect(reclassified_vote).to_not be_valid + end + + it "should not be valid without a valid reason" do + reclassified_vote.reason = nil + expect(reclassified_vote).to_not be_valid + + reclassified_vote.reason = "" + expect(reclassified_vote).to_not be_valid + + reclassified_vote.reason = "random" + expect(reclassified_vote).to_not be_valid + + reclassified_vote.reason = "heading_changed" + expect(reclassified_vote).to be_valid + + reclassified_vote.reason = "unfeasible" + expect(reclassified_vote).to be_valid + end + end + +end \ No newline at end of file From 47f97e342de4fbfaf0cb1bec1bdcac47b8858acb Mon Sep 17 00:00:00 2001 From: rgarcia Date: Wed, 17 May 2017 15:18:39 +0200 Subject: [PATCH 2/3] extracts reclassification to a module --- app/models/budget/investment.rb | 39 +------------------ app/models/budget/reclassification.rb | 47 +++++++++++++++++++++++ spec/features/budgets/investments_spec.rb | 2 +- 3 files changed, 49 insertions(+), 39 deletions(-) create mode 100644 app/models/budget/reclassification.rb diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index 1d4230e0c..ea9276d91 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -5,6 +5,7 @@ class Budget include Sanitizable include Taggable include Searchable + include Reclassification acts_as_votable acts_as_paranoid column: :hidden_at @@ -61,7 +62,6 @@ class Budget before_save :calculate_confidence_score before_validation :set_responsible_name before_validation :set_denormalized_ids - after_save :check_for_reclassification def self.filter_params(params) params.select{|x,_| %w{heading_id group_id administrator_id tag_name valuator_id}.include? x.to_s } @@ -243,43 +243,6 @@ class Budget investments end - def check_for_reclassification - if heading_changed? - log_heading_change - store_reclassified_votes("heading_changed") - remove_reclassified_votes - elsif marked_as_unfeasible? - store_reclassified_votes("unfeasible") - remove_reclassified_votes - end - end - - def heading_changed? - budget.balloting? && heading_id_changed? - end - - def log_heading_change - update_column(:previous_heading_id, heading_id_was) - end - - def marked_as_unfeasible? - budget.balloting? && feasibility_changed? && unfeasible? - end - - def store_reclassified_votes(reason) - ballot_lines_for_investment.each do |line| - Budget::ReclassifiedVote.create!(user: line.ballot.user, investment: self, reason: reason) - end - end - - def remove_reclassified_votes - ballot_lines_for_investment.destroy_all - end - - def ballot_lines_for_investment - Budget::Ballot::Line.by_investment(self.id) - end - private def set_denormalized_ids diff --git a/app/models/budget/reclassification.rb b/app/models/budget/reclassification.rb new file mode 100644 index 000000000..5265912ef --- /dev/null +++ b/app/models/budget/reclassification.rb @@ -0,0 +1,47 @@ +class Budget + module Reclassification + extend ActiveSupport::Concern + + included do + after_save :check_for_reclassification + end + + def check_for_reclassification + if heading_changed? + log_heading_change + store_reclassified_votes("heading_changed") + remove_reclassified_votes + elsif marked_as_unfeasible? + store_reclassified_votes("unfeasible") + remove_reclassified_votes + end + end + + def heading_changed? + budget.balloting? && heading_id_changed? + end + + def log_heading_change + update_column(:previous_heading_id, heading_id_was) + end + + def marked_as_unfeasible? + budget.balloting? && feasibility_changed? && unfeasible? + end + + def store_reclassified_votes(reason) + ballot_lines_for_investment.each do |line| + Budget::ReclassifiedVote.create!(user: line.ballot.user, investment: self, reason: reason) + end + end + + def remove_reclassified_votes + ballot_lines_for_investment.destroy_all + end + + def ballot_lines_for_investment + Budget::Ballot::Line.by_investment(self.id) + end + + end +end \ No newline at end of file diff --git a/spec/features/budgets/investments_spec.rb b/spec/features/budgets/investments_spec.rb index a6f988e56..ea7cf6c99 100644 --- a/spec/features/budgets/investments_spec.rb +++ b/spec/features/budgets/investments_spec.rb @@ -670,7 +670,7 @@ feature 'Budget Investments' do expect(page).to_not have_link("Vote") end - scenario "Reclassification" do + scenario "Reclassification", :focus do user = create(:user, :level_two) investment = create(:budget_investment, :selected, heading: heading) heading2 = create(:budget_heading, group: group) From a63ca9649e60c56a09b1150d281ca3d0383717e7 Mon Sep 17 00:00:00 2001 From: rgarcia Date: Wed, 17 May 2017 20:32:05 +0200 Subject: [PATCH 3/3] cleans up --- app/models/budget/reclassification.rb | 13 +++-- db/schema.rb | 6 --- spec/features/budgets/investments_spec.rb | 52 +++++++++++++++----- spec/models/budget/investment_spec.rb | 6 +-- spec/models/budget/reclassified_vote_spec.rb | 2 +- 5 files changed, 51 insertions(+), 28 deletions(-) diff --git a/app/models/budget/reclassification.rb b/app/models/budget/reclassification.rb index 5265912ef..f8d1db7fd 100644 --- a/app/models/budget/reclassification.rb +++ b/app/models/budget/reclassification.rb @@ -21,17 +21,20 @@ class Budget budget.balloting? && heading_id_changed? end - def log_heading_change - update_column(:previous_heading_id, heading_id_was) - end - def marked_as_unfeasible? budget.balloting? && feasibility_changed? && unfeasible? end + def log_heading_change + update_column(:previous_heading_id, heading_id_was) + end + def store_reclassified_votes(reason) ballot_lines_for_investment.each do |line| - Budget::ReclassifiedVote.create!(user: line.ballot.user, investment: self, reason: reason) + attrs = { user: line.ballot.user, + investment: self, + reason: reason } + Budget::ReclassifiedVote.create!(attrs) end end diff --git a/db/schema.rb b/db/schema.rb index d1cd6b640..d3953f638 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -63,12 +63,6 @@ ActiveRecord::Schema.define(version: 20170517123042) do add_index "annotations", ["legislation_id"], name: "index_annotations_on_legislation_id", using: :btree add_index "annotations", ["user_id"], name: "index_annotations_on_user_id", using: :btree - create_table "ar_internal_metadata", primary_key: "key", force: :cascade do |t| - t.string "value" - t.datetime "created_at", null: false - t.datetime "updated_at", null: false - end - create_table "banners", force: :cascade do |t| t.string "title", limit: 80 t.string "description" diff --git a/spec/features/budgets/investments_spec.rb b/spec/features/budgets/investments_spec.rb index ea7cf6c99..d8855712c 100644 --- a/spec/features/budgets/investments_spec.rb +++ b/spec/features/budgets/investments_spec.rb @@ -670,25 +670,51 @@ feature 'Budget Investments' do expect(page).to_not have_link("Vote") end - scenario "Reclassification", :focus do - user = create(:user, :level_two) - investment = create(:budget_investment, :selected, heading: heading) - heading2 = create(:budget_heading, group: group) + feature "Reclassification" do - ballot = create(:budget_ballot, user: user, budget: budget) - ballot.investments << investment + scenario "Due to heading change" do + user = create(:user, :level_two) + investment = create(:budget_investment, :selected, heading: heading) + heading2 = create(:budget_heading, group: group) - login_as(user) - visit budget_ballot_path(budget) + ballot = create(:budget_ballot, user: user, budget: budget) + ballot.investments << investment - expect(page).to have_content("You have voted one investment") + login_as(user) + visit budget_ballot_path(budget) - investment.heading = heading2 - investment.save + expect(page).to have_content("You have voted one investment") - visit budget_ballot_path(budget) + investment.heading = heading2 + investment.save + + visit budget_ballot_path(budget) + + expect(page).to have_content("You have voted 0 investment") + end + + scenario "Due to being unfeasible" do + user = create(:user, :level_two) + investment = create(:budget_investment, :selected, heading: heading) + heading2 = create(:budget_heading, group: group) + + ballot = create(:budget_ballot, user: user, budget: budget) + ballot.investments << investment + + login_as(user) + visit budget_ballot_path(budget) + + expect(page).to have_content("You have voted one investment") + + investment.feasibility = "unfeasible" + investment.unfeasibility_explanation = "too expensive" + investment.save + + visit budget_ballot_path(budget) + + expect(page).to have_content("You have voted 0 investment") + end - expect(page).to have_content("You have voted 0 investment") end end end diff --git a/spec/models/budget/investment_spec.rb b/spec/models/budget/investment_spec.rb index 5b922cf95..4ca255c9c 100644 --- a/spec/models/budget/investment_spec.rb +++ b/spec/models/budget/investment_spec.rb @@ -681,7 +681,7 @@ describe Budget::Investment do end - describe "Reclassification", :focus do + describe "Reclassification" do let(:budget) { create(:budget, phase: "balloting") } let(:group) { create(:budget_group, budget: budget) } @@ -737,7 +737,7 @@ describe Budget::Investment do describe "store_reclassified_votes" do - it "stores the votes for a reclassified investment", :focus do + it "stores the votes for a reclassified investment" do investment = create(:budget_investment, :selected, heading: heading1) 3.times do @@ -780,7 +780,7 @@ describe Budget::Investment do end - describe "check_for_reclassification", :focus do + describe "check_for_reclassification" do it "stores reclassfied votes and removes actual votes if an investment has been reclassified" do investment = create(:budget_investment, :selected, heading: heading1) diff --git a/spec/models/budget/reclassified_vote_spec.rb b/spec/models/budget/reclassified_vote_spec.rb index bf689039d..c4da8d7fb 100644 --- a/spec/models/budget/reclassified_vote_spec.rb +++ b/spec/models/budget/reclassified_vote_spec.rb @@ -2,7 +2,7 @@ require 'rails_helper' describe Budget::ReclassifiedVote do - describe "Validations", :focus do + describe "Validations" do let(:reclassified_vote) { build(:budget_reclassified_vote) } it "should be valid" do