From fd5fa2da79d67757a2ee514f0005883857d30953 Mon Sep 17 00:00:00 2001 From: taitus Date: Wed, 20 Sep 2023 15:06:45 +0200 Subject: [PATCH] Refactoring: Move 'vote' action to Votes Controllers As far as possible I think the code is clearer if we use CRUD actions rather than custom actions. This will make it easier to add the action to remove votes in the next commit. Note that we are adding this line as we need to validate it that a vote can be created on a debate by the current user: ```authorize! :create, Vote.new(voter: current_user, votable: @debate)``` We have done it this way and not with the following code as you might expect, as this way two votes are created instead of one. ```load_and_authorize_resource through: :debate, through_association: :votes_for``` This line tries to load the resource @debate and through the association "votes_for" it tries to create a new vote associated to that debate. Therefore a vote is created when trying to authorise the resource and then another one in the create action, when calling @debate.vote_by (which is called by @debate.register_vote). --- .../legislation/proposals/votes_component.rb | 4 +- .../in_favor_against_component.html.erb | 4 +- .../shared/in_favor_against_component.rb | 8 ++++ app/controllers/debates/votes_controller.rb | 15 +++++++ app/controllers/debates_controller.rb | 4 -- .../legislation/proposals/votes_controller.rb | 18 ++++++++ .../legislation/proposals_controller.rb | 4 -- app/models/abilities/common.rb | 5 ++- app/models/budget/investment.rb | 2 +- app/models/legislation/proposal.rb | 8 ---- .../{vote.js.erb => votes/show.js.erb} | 0 .../{vote.js.erb => votes/show.js.erb} | 0 config/routes/debate.rb | 3 +- config/routes/legislation.rb | 3 +- .../shared/in_favor_against_component_spec.rb | 16 +++++++ .../debates/votes_controller_spec.rb | 43 +++++++++++++++++++ spec/controllers/debates_controller_spec.rb | 22 ---------- .../proposals/votes_controller_spec.rb | 40 +++++++++++++++++ spec/models/abilities/administrator_spec.rb | 1 - spec/models/abilities/common_spec.rb | 29 +++++++++---- spec/models/abilities/moderator_spec.rb | 1 - spec/models/abilities/organization_spec.rb | 2 +- 22 files changed, 173 insertions(+), 59 deletions(-) create mode 100644 app/controllers/debates/votes_controller.rb create mode 100644 app/controllers/legislation/proposals/votes_controller.rb rename app/views/debates/{vote.js.erb => votes/show.js.erb} (100%) rename app/views/legislation/proposals/{vote.js.erb => votes/show.js.erb} (100%) create mode 100644 spec/controllers/debates/votes_controller_spec.rb create mode 100644 spec/controllers/legislation/proposals/votes_controller_spec.rb diff --git a/app/components/legislation/proposals/votes_component.rb b/app/components/legislation/proposals/votes_component.rb index 96495800f..10d16e74f 100644 --- a/app/components/legislation/proposals/votes_component.rb +++ b/app/components/legislation/proposals/votes_component.rb @@ -1,6 +1,6 @@ class Legislation::Proposals::VotesComponent < ApplicationComponent attr_reader :proposal - delegate :current_user, :link_to_verify_account, to: :helpers + delegate :current_user, :link_to_verify_account, :can?, to: :helpers def initialize(proposal) @proposal = proposal @@ -9,7 +9,7 @@ class Legislation::Proposals::VotesComponent < ApplicationComponent private def can_vote? - proposal.votable_by?(current_user) + can?(:create, proposal.votes_for.new(voter: current_user)) end def cannot_vote_text diff --git a/app/components/shared/in_favor_against_component.html.erb b/app/components/shared/in_favor_against_component.html.erb index be4040983..c4c403649 100644 --- a/app/components/shared/in_favor_against_component.html.erb +++ b/app/components/shared/in_favor_against_component.html.erb @@ -1,6 +1,6 @@
- <%= button_to polymorphic_path(votable, action: :vote, value: "yes"), + <%= button_to vote_in_favor_against_path("yes"), title: t("votes.agree"), "aria-label": agree_aria_label, "aria-pressed": pressed?("yes"), @@ -12,7 +12,7 @@
- <%= button_to polymorphic_path(votable, action: :vote, value: "no"), + <%= button_to vote_in_favor_against_path("no"), title: t("votes.disagree"), "aria-label": disagree_aria_label, "aria-pressed": pressed?("no"), diff --git a/app/components/shared/in_favor_against_component.rb b/app/components/shared/in_favor_against_component.rb index a8561e458..0c0736407 100644 --- a/app/components/shared/in_favor_against_component.rb +++ b/app/components/shared/in_favor_against_component.rb @@ -26,4 +26,12 @@ class Shared::InFavorAgainstComponent < ApplicationComponent false end end + + def vote_in_favor_against_path(value) + if votable.class.name == "Debate" + debate_votes_path(votable, value: value) + else + legislation_process_proposal_votes_path(votable.process, votable, value: value) + end + end end diff --git a/app/controllers/debates/votes_controller.rb b/app/controllers/debates/votes_controller.rb new file mode 100644 index 000000000..e0b92c472 --- /dev/null +++ b/app/controllers/debates/votes_controller.rb @@ -0,0 +1,15 @@ +module Debates + class VotesController < ApplicationController + before_action :authenticate_user! + load_and_authorize_resource :debate + + def create + authorize! :create, Vote.new(voter: current_user, votable: @debate) + @debate.register_vote(current_user, params[:value]) + + respond_to do |format| + format.js { render :show } + end + end + end +end diff --git a/app/controllers/debates_controller.rb b/app/controllers/debates_controller.rb index c08963a6c..33c7fd899 100644 --- a/app/controllers/debates_controller.rb +++ b/app/controllers/debates_controller.rb @@ -28,10 +28,6 @@ class DebatesController < ApplicationController redirect_to debate_path(@debate), status: :moved_permanently if request.path != debate_path(@debate) end - def vote - @debate.register_vote(current_user, params[:value]) - end - def unmark_featured @debate.update!(featured_at: nil) redirect_to debates_path diff --git a/app/controllers/legislation/proposals/votes_controller.rb b/app/controllers/legislation/proposals/votes_controller.rb new file mode 100644 index 000000000..03886c41c --- /dev/null +++ b/app/controllers/legislation/proposals/votes_controller.rb @@ -0,0 +1,18 @@ +module Legislation + module Proposals + class VotesController < ApplicationController + before_action :authenticate_user! + load_and_authorize_resource :process, class: "Legislation::Process" + load_and_authorize_resource :proposal, class: "Legislation::Proposal", through: :process + + def create + authorize! :create, Vote.new(voter: current_user, votable: @proposal) + @proposal.vote_by(voter: current_user, vote: params[:value]) + + respond_to do |format| + format.js { render :show } + end + end + end + end +end diff --git a/app/controllers/legislation/proposals_controller.rb b/app/controllers/legislation/proposals_controller.rb index 86b34bce0..d9adb14e3 100644 --- a/app/controllers/legislation/proposals_controller.rb +++ b/app/controllers/legislation/proposals_controller.rb @@ -37,10 +37,6 @@ class Legislation::ProposalsController < Legislation::BaseController end end - def vote - @proposal.register_vote(current_user, params[:value]) - end - private def proposal_params diff --git a/app/models/abilities/common.rb b/app/models/abilities/common.rb index fe96be60b..53378b4be 100644 --- a/app/models/abilities/common.rb +++ b/app/models/abilities/common.rb @@ -81,14 +81,15 @@ module Abilities can [:create, :destroy], DirectUpload unless user.organization? - can :vote, Debate + can :create, ActsAsVotable::Vote, voter_id: user.id, votable_type: "Debate" can :vote, Comment end if user.level_two_or_three_verified? can :vote, Proposal, &:published? - can :vote, Legislation::Proposal + can :create, ActsAsVotable::Vote, voter_id: user.id, votable_type: "Legislation::Proposal" + can :create, Legislation::Answer can :create, Budget::Investment, budget: { phase: "accepting" } diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index 086a4265b..1808d8033 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -286,7 +286,7 @@ class Budget def permission_problem(user) return :not_logged_in unless user return :organization if user.organization? - return :not_verified unless user.can?(:create, ActsAsVotable::Vote) + return :not_verified unless user.level_two_or_three_verified? nil end diff --git a/app/models/legislation/proposal.rb b/app/models/legislation/proposal.rb index ec653d4c3..497cea5db 100644 --- a/app/models/legislation/proposal.rb +++ b/app/models/legislation/proposal.rb @@ -108,14 +108,6 @@ class Legislation::Proposal < ApplicationRecord author_id == user.id && editable? end - def votable_by?(user) - user&.level_two_or_three_verified? - end - - def register_vote(user, vote_value) - vote_by(voter: user, vote: vote_value) if votable_by?(user) - end - def code "#{Setting["proposal_code_prefix"]}-#{created_at.strftime("%Y-%m")}-#{id}" end diff --git a/app/views/debates/vote.js.erb b/app/views/debates/votes/show.js.erb similarity index 100% rename from app/views/debates/vote.js.erb rename to app/views/debates/votes/show.js.erb diff --git a/app/views/legislation/proposals/vote.js.erb b/app/views/legislation/proposals/votes/show.js.erb similarity index 100% rename from app/views/legislation/proposals/vote.js.erb rename to app/views/legislation/proposals/votes/show.js.erb diff --git a/config/routes/debate.rb b/config/routes/debate.rb index 58f1f9215..45d6f06f6 100644 --- a/config/routes/debate.rb +++ b/config/routes/debate.rb @@ -1,6 +1,5 @@ resources :debates do member do - post :vote put :flag put :unflag put :mark_featured @@ -11,4 +10,6 @@ resources :debates do get :suggest put "recommendations/disable", only: :index, controller: "debates", action: :disable_recommendations end + + resources :votes, controller: "debates/votes", only: :create end diff --git a/config/routes/legislation.rb b/config/routes/legislation.rb index 4a18ef1cd..7e3890c42 100644 --- a/config/routes/legislation.rb +++ b/config/routes/legislation.rb @@ -16,13 +16,14 @@ namespace :legislation do resources :proposals, except: [:index] do member do - post :vote put :flag put :unflag end collection do get :suggest end + + resources :votes, controller: "proposals/votes", only: :create end resources :draft_versions, only: [:show] do diff --git a/spec/components/shared/in_favor_against_component_spec.rb b/spec/components/shared/in_favor_against_component_spec.rb index 32d88f574..ece805749 100644 --- a/spec/components/shared/in_favor_against_component_spec.rb +++ b/spec/components/shared/in_favor_against_component_spec.rb @@ -6,6 +6,22 @@ describe Shared::InFavorAgainstComponent do let(:user) { create(:user) } describe "Agree and disagree buttons" do + it "can create a vote when the user has not yet voted" do + sign_in user + + render_inline component + + page.find(".in-favor") do |in_favor_block| + expect(in_favor_block).to have_css "form[action*='votes'][method='post']" + expect(in_favor_block).not_to have_css "input[name='_method']", visible: :all + end + + page.find(".against") do |against_block| + expect(against_block).to have_css "form[action*='votes'][method='post']" + expect(against_block).not_to have_css "input[name='_method']", visible: :all + end + end + it "does not include result percentages" do create(:vote, votable: debate) sign_in(user) diff --git a/spec/controllers/debates/votes_controller_spec.rb b/spec/controllers/debates/votes_controller_spec.rb new file mode 100644 index 000000000..9e6090ee6 --- /dev/null +++ b/spec/controllers/debates/votes_controller_spec.rb @@ -0,0 +1,43 @@ +require "rails_helper" + +describe Debates::VotesController do + describe "POST create" do + it "does not authorize unauthenticated users" do + debate = create(:debate) + + post :create, xhr: true, params: { debate_id: debate.id, value: "yes" } + + expect(response).to be_unauthorized + end + + it "redirects unauthenticated users without JavaScript to the sign in page" do + debate = create(:debate) + + post :create, params: { debate_id: debate.id, value: "yes" } + + expect(response).to redirect_to new_user_session_path + end + + describe "Vote with too many anonymous votes" do + it "allows vote if user is allowed" do + Setting["max_ratio_anon_votes_on_debates"] = 100 + debate = create(:debate) + sign_in create(:user) + + expect do + post :create, xhr: true, params: { debate_id: debate.id, value: "yes" } + end.to change { debate.reload.votes_for.size }.by(1) + end + + it "does not allow voting if user is not allowed" do + Setting["max_ratio_anon_votes_on_debates"] = 0 + debate = create(:debate, cached_votes_total: 1000) + sign_in create(:user) + + expect do + post :create, xhr: true, params: { debate_id: debate.id, value: "yes" } + end.not_to change { debate.reload.votes_for.size } + end + end + end +end diff --git a/spec/controllers/debates_controller_spec.rb b/spec/controllers/debates_controller_spec.rb index 01af1db82..c3d96dcea 100644 --- a/spec/controllers/debates_controller_spec.rb +++ b/spec/controllers/debates_controller_spec.rb @@ -37,28 +37,6 @@ describe DebatesController do end end - describe "Vote with too many anonymous votes" do - it "allows vote if user is allowed" do - Setting["max_ratio_anon_votes_on_debates"] = 100 - debate = create(:debate) - sign_in create(:user) - - expect do - post :vote, xhr: true, params: { id: debate.id, value: "yes" } - end.to change { debate.reload.votes_for.size }.by(1) - end - - it "does not allow vote if user is not allowed" do - Setting["max_ratio_anon_votes_on_debates"] = 0 - debate = create(:debate, cached_votes_total: 1000) - sign_in create(:user) - - expect do - post :vote, xhr: true, params: { id: debate.id, value: "yes" } - end.not_to change { debate.reload.votes_for.size } - end - end - describe "PUT mark_featured" do it "ignores query parameters" do debate = create(:debate) diff --git a/spec/controllers/legislation/proposals/votes_controller_spec.rb b/spec/controllers/legislation/proposals/votes_controller_spec.rb new file mode 100644 index 000000000..fd8c780a6 --- /dev/null +++ b/spec/controllers/legislation/proposals/votes_controller_spec.rb @@ -0,0 +1,40 @@ +require "rails_helper" + +describe Legislation::Proposals::VotesController do + let(:legislation_process) { create(:legislation_process) } + let(:proposal) { create(:legislation_proposal, process: legislation_process) } + + describe "POST create" do + let(:vote_params) do + { process_id: legislation_process.id, legislation_proposal_id: proposal.id, value: "yes" } + end + + it "does not authorize unauthenticated users" do + post :create, xhr: true, params: vote_params + + expect(response).to be_unauthorized + end + + it "redirects unauthenticated users without JavaScript to the sign in page" do + post :create, params: vote_params + + expect(response).to redirect_to new_user_session_path + end + + it "allows vote if user is level_two_or_three_verified" do + sign_in create(:user, :level_two) + + expect do + post :create, xhr: true, params: vote_params + end.to change { proposal.reload.votes_for.size }.by(1) + end + + it "does not allow voting if user is not level_two_or_three_verified" do + sign_in create(:user) + + expect do + post :create, xhr: true, params: vote_params + end.not_to change { proposal.reload.votes_for.size } + end + end +end diff --git a/spec/models/abilities/administrator_spec.rb b/spec/models/abilities/administrator_spec.rb index 39a42dbd6..7fb48b225 100644 --- a/spec/models/abilities/administrator_spec.rb +++ b/spec/models/abilities/administrator_spec.rb @@ -48,7 +48,6 @@ describe Abilities::Administrator do it { should be_able_to(:index, Debate) } it { should be_able_to(:show, debate) } - it { should be_able_to(:vote, debate) } it { should be_able_to(:index, Proposal) } it { should be_able_to(:show, proposal) } diff --git a/spec/models/abilities/common_spec.rb b/spec/models/abilities/common_spec.rb index 6c3918ec8..9ba947776 100644 --- a/spec/models/abilities/common_spec.rb +++ b/spec/models/abilities/common_spec.rb @@ -7,6 +7,7 @@ describe Abilities::Common do let(:geozone) { create(:geozone) } let(:user) { create(:user, geozone: geozone) } + let(:another_user) { create(:user) } let(:debate) { create(:debate) } let(:comment) { create(:comment) } @@ -15,6 +16,7 @@ describe Abilities::Common do let(:own_comment) { create(:comment, author: user) } let(:own_proposal) { create(:proposal, author: user) } let(:own_legislation_proposal) { create(:legislation_proposal, author: user) } + let(:legislation_proposal) { create(:legislation_proposal) } let(:accepting_budget) { create(:budget, :accepting) } let(:reviewing_budget) { create(:budget, :reviewing) } @@ -73,7 +75,8 @@ describe Abilities::Common do it { should be_able_to(:index, Debate) } it { should be_able_to(:show, debate) } - it { should be_able_to(:vote, debate) } + it { should be_able_to(:create, user.votes.build(votable: debate)) } + it { should_not be_able_to(:create, another_user.votes.build(votable: debate)) } it { should be_able_to(:show, user) } it { should be_able_to(:edit, user) } @@ -137,20 +140,16 @@ describe Abilities::Common do end describe "follows" do - let(:other_user) { create(:user) } - it { should be_able_to(:create, build(:follow, :followed_proposal, user: user)) } - it { should_not be_able_to(:create, build(:follow, :followed_proposal, user: other_user)) } + it { should_not be_able_to(:create, build(:follow, :followed_proposal, user: another_user)) } it { should be_able_to(:destroy, create(:follow, :followed_proposal, user: user)) } - it { should_not be_able_to(:destroy, create(:follow, :followed_proposal, user: other_user)) } + it { should_not be_able_to(:destroy, create(:follow, :followed_proposal, user: another_user)) } end describe "other users" do - let(:other_user) { create(:user) } - - it { should be_able_to(:show, other_user) } - it { should_not be_able_to(:edit, other_user) } + it { should be_able_to(:show, another_user) } + it { should_not be_able_to(:edit, another_user) } end describe "editing debates" do @@ -182,6 +181,18 @@ describe Abilities::Common do it { should_not be_able_to(:edit, own_legislation_proposal) } it { should_not be_able_to(:update, own_legislation_proposal) } + describe "vote legislation proposal" do + context "when user is not level_two_or_three_verified" do + it { should_not be_able_to(:create, user.votes.build(votable: legislation_proposal)) } + end + + context "when user is level_two_or_three_verified" do + before { user.update(level_two_verified_at: Date.current) } + it { should be_able_to(:create, user.votes.build(votable: legislation_proposal)) } + it { should_not be_able_to(:create, another_user.votes.build(votable: legislation_proposal)) } + end + end + describe "proposals dashboard" do it { should be_able_to(:dashboard, own_proposal) } it { should_not be_able_to(:dashboard, proposal) } diff --git a/spec/models/abilities/moderator_spec.rb b/spec/models/abilities/moderator_spec.rb index b9df69067..9dd4f4c0c 100644 --- a/spec/models/abilities/moderator_spec.rb +++ b/spec/models/abilities/moderator_spec.rb @@ -25,7 +25,6 @@ describe Abilities::Moderator do it { should be_able_to(:index, Debate) } it { should be_able_to(:show, debate) } - it { should be_able_to(:vote, debate) } it { should be_able_to(:index, Proposal) } it { should be_able_to(:show, proposal) } diff --git a/spec/models/abilities/organization_spec.rb b/spec/models/abilities/organization_spec.rb index 874a5ce40..9bfd9b69f 100644 --- a/spec/models/abilities/organization_spec.rb +++ b/spec/models/abilities/organization_spec.rb @@ -14,7 +14,7 @@ describe "Abilities::Organization" do it { should be_able_to(:index, Debate) } it { should be_able_to(:show, debate) } - it { should_not be_able_to(:vote, debate) } + it { should_not be_able_to(:create, user.votes.build(votable: debate)) } it { should be_able_to(:index, Proposal) } it { should be_able_to(:show, proposal) }