From 8dbad5c3d2ab927e6351ce19111c98a3c5244b7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 8 Nov 2019 20:36:41 +0100 Subject: [PATCH 1/7] Simplify tests for stats and results permissions Now these tests look like the other ability tests. --- spec/models/abilities/everyone_spec.rb | 86 ++++---------------------- 1 file changed, 12 insertions(+), 74 deletions(-) diff --git a/spec/models/abilities/everyone_spec.rb b/spec/models/abilities/everyone_spec.rb index 6e2ef637e..899639a60 100644 --- a/spec/models/abilities/everyone_spec.rb +++ b/spec/models/abilities/everyone_spec.rb @@ -31,81 +31,19 @@ describe Abilities::Everyone do it { should_not be_able_to(:create, LocalCensusRecords::Import) } it { should_not be_able_to(:show, LocalCensusRecords::Import) } - context "when accessing poll results" do - let(:results_enabled) { true } - let(:poll) { create(:poll, :expired, results_enabled: results_enabled) } + it { should be_able_to(:results, create(:poll, :expired, results_enabled: true)) } + it { should_not be_able_to(:results, create(:poll, :expired, results_enabled: false)) } + it { should_not be_able_to(:results, create(:poll, :current, results_enabled: true)) } - it { should be_able_to(:results, poll) } + it { should be_able_to(:stats, create(:poll, :expired, stats_enabled: true)) } + it { should_not be_able_to(:stats, create(:poll, :expired, stats_enabled: false)) } + it { should_not be_able_to(:stats, create(:poll, :current, stats_enabled: true)) } - context "and results disabled" do - let(:results_enabled) { false } + it { should be_able_to(:read_results, create(:budget, :finished, results_enabled: true)) } + it { should_not be_able_to(:read_results, create(:budget, :finished, results_enabled: false)) } + it { should_not be_able_to(:read_results, create(:budget, :reviewing_ballots, results_enabled: true)) } - it { should_not be_able_to(:results, poll) } - end - - context "and not expired" do - let(:poll) { create(:poll, :current, results_enabled: true) } - - it { should_not be_able_to(:results, poll) } - end - end - - context "when accessing poll stats" do - let(:stats_enabled) { true } - let(:poll) { create(:poll, :expired, stats_enabled: stats_enabled) } - - it { should be_able_to(:stats, poll) } - - context "and stats disabled" do - let(:stats_enabled) { false } - - it { should_not be_able_to(:stats, poll) } - end - - context "and not expired" do - let(:poll) { create(:poll, :current, stats_enabled: true) } - - it { should_not be_able_to(:stats, poll) } - end - end - - context "when accessing budget results" do - context "budget is not finished" do - let(:budget) { create(:budget, :reviewing_ballots, results_enabled: true) } - - it { should_not be_able_to(:read_results, budget) } - end - - context "budget is finished" do - let(:budget) { create(:budget, :finished) } - - it { should be_able_to(:read_results, budget) } - end - - context "results disabled" do - let(:budget) { create(:budget, :finished, results_enabled: false) } - - it { should_not be_able_to(:read_results, budget) } - end - end - - context "when accessing budget stats" do - context "supports phase is not finished" do - let(:budget) { create(:budget, :selecting, stats_enabled: true) } - - it { should_not be_able_to(:read_stats, budget) } - end - - context "supports phase is finished" do - let(:budget) { create(:budget, :valuating, stats_enabled: true) } - - it { should be_able_to(:read_stats, budget) } - end - - context "stats disabled" do - let(:budget) { create(:budget, :valuating, stats_enabled: false) } - - it { should_not be_able_to(:read_stats, budget) } - end - end + it { should be_able_to(:read_stats, create(:budget, :valuating, stats_enabled: true)) } + it { should_not be_able_to(:read_stats, create(:budget, :valuating, stats_enabled: false)) } + it { should_not be_able_to(:read_stats, create(:budget, :selecting, stats_enabled: true)) } end From 2029d7baa50c02ac32c35185b431f98e8b7ff511 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 8 Nov 2019 20:49:28 +0100 Subject: [PATCH 2/7] Limit admin access to poll stats and results There's no reason to allow administrators to check stats and results for a poll when it isn't finished or when results and stats are not enabled. Now admins have the same permissions as everyone else. --- app/models/abilities/administrator.rb | 2 +- spec/models/abilities/administrator_spec.rb | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/app/models/abilities/administrator.rb b/app/models/abilities/administrator.rb index b997e56a4..25dcb60c7 100644 --- a/app/models/abilities/administrator.rb +++ b/app/models/abilities/administrator.rb @@ -75,7 +75,7 @@ module Abilities can [:index, :create, :edit, :update, :destroy], Geozone - can [:read, :create, :update, :destroy, :add_question, :search_booths, :search_officers, :booth_assignments, :results, :stats], Poll + can [:read, :create, :update, :destroy, :add_question, :search_booths, :search_officers, :booth_assignments], Poll can [:read, :create, :update, :destroy, :available], Poll::Booth can [:search, :create, :index, :destroy], ::Poll::Officer can [:create, :destroy, :manage], ::Poll::BoothAssignment diff --git a/spec/models/abilities/administrator_spec.rb b/spec/models/abilities/administrator_spec.rb index c904dcb91..bbf8f91fe 100644 --- a/spec/models/abilities/administrator_spec.rb +++ b/spec/models/abilities/administrator_spec.rb @@ -6,7 +6,6 @@ describe Abilities::Administrator do let(:user) { administrator.user } let(:administrator) { create(:administrator) } - let(:poll) { create(:poll, :current, stats_enabled: false, results_enabled: false) } let(:other_user) { create(:user) } let(:hidden_user) { create(:user, :hidden) } @@ -89,9 +88,6 @@ describe Abilities::Administrator do it { should_not be_able_to(:destroy, budget_investment_document) } it { should be_able_to(:manage, Dashboard::Action) } - it { should be_able_to(:stats, poll) } - it { should be_able_to(:results, poll) } - it { should be_able_to(:read, Poll::Question) } it { should be_able_to(:create, Poll::Question) } it { should be_able_to(:update, Poll::Question) } From 864f750d92832bf3fb78637ecc361b498ba84f87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 8 Nov 2019 21:00:09 +0100 Subject: [PATCH 3/7] Remove duplication in poll permissions We were checking for `expired?` and `results_enabled?` in views and helpers, when we've already defined a rule for accessing stats and results for a poll. This way we also fix a bug when stats were enabled but the poll wasn't finished. In this scenario, the link pointed to the stats page, but when clicking it we'd get a "you don't have permission" message. Now the link doesn't point to the stats page anymore. --- app/helpers/polls_helper.rb | 8 ++------ app/views/polls/_poll_subnav.html.erb | 6 +++--- spec/features/polls/polls_spec.rb | 8 ++++---- 3 files changed, 9 insertions(+), 13 deletions(-) diff --git a/app/helpers/polls_helper.rb b/app/helpers/polls_helper.rb index f4f57ec5c..a3de04ccf 100644 --- a/app/helpers/polls_helper.rb +++ b/app/helpers/polls_helper.rb @@ -49,19 +49,15 @@ module PollsHelper end def link_to_poll(text, poll) - if poll.results_enabled? + if can?(:results, poll) link_to text, results_poll_path(id: poll.slug || poll.id) - elsif poll.stats_enabled? + elsif can?(:stats, poll) link_to text, stats_poll_path(id: poll.slug || poll.id) else link_to text, poll_path(id: poll.slug || poll.id) end end - def show_stats_or_results? - @poll.expired? && (@poll.results_enabled? || @poll.stats_enabled?) - end - def results_menu? controller_name == "polls" && action_name == "results" end diff --git a/app/views/polls/_poll_subnav.html.erb b/app/views/polls/_poll_subnav.html.erb index e50d64e0a..1fdc2d71d 100644 --- a/app/views/polls/_poll_subnav.html.erb +++ b/app/views/polls/_poll_subnav.html.erb @@ -1,8 +1,8 @@ -<% if show_stats_or_results? %> +<% if can?(:stats, @poll) || can?(:results, @poll) %>
    - <% if @poll.results_enabled? %> + <% if can?(:results, @poll) %> <% if results_menu? %>

  • <%= t("polls.show.results_menu") %>

    @@ -12,7 +12,7 @@ <% end %> <% end %> - <% if @poll.stats_enabled? %> + <% if can?(:stats, @poll) %> <% if stats_menu? %>

  • <%= t("polls.show.stats_menu") %>

    diff --git a/spec/features/polls/polls_spec.rb b/spec/features/polls/polls_spec.rb index 968479094..6479285f4 100644 --- a/spec/features/polls/polls_spec.rb +++ b/spec/features/polls/polls_spec.rb @@ -110,17 +110,17 @@ describe "Polls" do end scenario "Poll title link to stats if enabled" do - poll = create(:poll, name: "Poll with stats", stats_enabled: true) + poll = create(:poll, :expired, name: "Poll with stats", stats_enabled: true) - visit polls_path + visit polls_path(filter: "expired") expect(page).to have_link("Poll with stats", href: stats_poll_path(poll.slug)) end scenario "Poll title link to results if enabled" do - poll = create(:poll, name: "Poll with results", stats_enabled: true, results_enabled: true) + poll = create(:poll, :expired, name: "Poll with results", stats_enabled: true, results_enabled: true) - visit polls_path + visit polls_path(filter: "expired") expect(page).to have_link("Poll with results", href: results_poll_path(poll.slug)) end From 9e27027f56f406b4e6ac3668832e314c668c4c1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 8 Nov 2019 21:11:25 +0100 Subject: [PATCH 4/7] Remove duplicate scope The scopes `created_by_admin` and `public_polls` were very similar. I'm using `created_by_admin` because `Poll.public_polls` feels redundant, and the reason for that name is we should not name the scope `public` because `public` is a ruby access modifier. --- app/controllers/polls_controller.rb | 2 +- app/models/poll.rb | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/app/controllers/polls_controller.rb b/app/controllers/polls_controller.rb index 78b11d321..2e4e12045 100644 --- a/app/controllers/polls_controller.rb +++ b/app/controllers/polls_controller.rb @@ -13,7 +13,7 @@ class PollsController < ApplicationController def index @polls = Kaminari.paginate_array( - @polls.public_polls.not_budget.send(@current_filter).includes(:geozones).sort_for_list + @polls.created_by_admin.not_budget.send(@current_filter).includes(:geozones).sort_for_list ).page(params[:page]) end diff --git a/app/models/poll.rb b/app/models/poll.rb index 8b07b8aeb..bd577670b 100644 --- a/app/models/poll.rb +++ b/app/models/poll.rb @@ -40,7 +40,6 @@ class Poll < ApplicationRecord accepts_nested_attributes_for :questions, reject_if: :all_blank, allow_destroy: true scope :for, ->(element) { where(related: element) } - scope :public_polls, -> { where(related: nil) } scope :current, -> { where("starts_at <= ? and ? <= ends_at", Date.current.beginning_of_day, Date.current.beginning_of_day) } scope :expired, -> { where("ends_at < ?", Date.current.beginning_of_day) } scope :recounting, -> { where(ends_at: (Date.current.beginning_of_day - RECOUNT_DURATION)..Date.current.beginning_of_day) } From f8e6e98d3ab53f635822ddebaaf6b47847a4d0f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 8 Nov 2019 22:11:25 +0100 Subject: [PATCH 5/7] Define stats and result permissions with scopes When defining abilities, scopes cover more cases because they can be used to check permissions for a record and to filter a collection. Ruby blocks can only be used to check permissions for a record. Note the `Budget::Phase.kind_or_later` name sounds funny, probably because we use the word "phase" for both an an attribute in the budgets table and an object associated with the budget, and so naming methods for a budget phase is a bit tricky. --- app/models/abilities/everyone.rb | 12 ++++------ app/models/budget.rb | 1 + app/models/budget/phase.rb | 6 ++++- app/models/concerns/reportable.rb | 4 ++++ spec/models/budget_spec.rb | 36 ++++++++++++++++++++++-------- spec/models/concerns/reportable.rb | 30 +++++++++++++++++++++++++ 6 files changed, 71 insertions(+), 18 deletions(-) diff --git a/app/models/abilities/everyone.rb b/app/models/abilities/everyone.rb index 6cdc8b6c9..e2a59c140 100644 --- a/app/models/abilities/everyone.rb +++ b/app/models/abilities/everyone.rb @@ -7,20 +7,16 @@ module Abilities can [:read, :map, :summary, :share], Proposal can :read, Comment can :read, Poll - can :results, Poll do |poll| - poll.expired? && poll.results_enabled? - end - can :stats, Poll do |poll| - poll.expired? && poll.stats_enabled? - end + can :results, Poll, id: Poll.expired.results_enabled.ids + can :stats, Poll, id: Poll.expired.stats_enabled.ids can :read, Poll::Question can :read, User can [:read, :welcome], Budget can [:read], Budget can [:read], Budget::Group can [:read, :print, :json_data], Budget::Investment - can(:read_results, Budget) { |budget| budget.results_enabled? && budget.finished? } - can(:read_stats, Budget) { |budget| budget.stats_enabled? && budget.valuating_or_later? } + can :read_results, Budget, id: Budget.finished.results_enabled.ids + can :read_stats, Budget, id: Budget.valuating_or_later.stats_enabled.ids can :read_executions, Budget, phase: "finished" can :new, DirectMessage can [:read, :debate, :draft_publication, :allegations, :result_publication, diff --git a/app/models/budget.rb b/app/models/budget.rb index 57be318ca..26489419c 100644 --- a/app/models/budget.rb +++ b/app/models/budget.rb @@ -47,6 +47,7 @@ class Budget < ApplicationRecord scope :reviewing, -> { where(phase: "reviewing") } scope :selecting, -> { where(phase: "selecting") } scope :valuating, -> { where(phase: "valuating") } + scope :valuating_or_later, -> { where(phase: Budget::Phase.kind_or_later("valuating")) } scope :publishing_prices, -> { where(phase: "publishing_prices") } scope :balloting, -> { where(phase: "balloting") } scope :reviewing_ballots, -> { where(phase: "reviewing_ballots") } diff --git a/app/models/budget/phase.rb b/app/models/budget/phase.rb index de90075a7..de3b8c0f1 100644 --- a/app/models/budget/phase.rb +++ b/app/models/budget/phase.rb @@ -32,6 +32,10 @@ class Budget define_singleton_method(phase) { find_by(kind: phase) } end + def self.kind_or_later(phase) + PHASE_KINDS[PHASE_KINDS.index(phase)..-1] + end + def next_enabled_phase next_phase&.enabled? ? next_phase : next_phase&.next_enabled_phase end @@ -92,7 +96,7 @@ class Budget end def in_phase_or_later?(phase) - PHASE_KINDS.index(kind) >= PHASE_KINDS.index(phase) + self.class.kind_or_later(phase).include?(kind) end end end diff --git a/app/models/concerns/reportable.rb b/app/models/concerns/reportable.rb index f584faa86..5a57551f8 100644 --- a/app/models/concerns/reportable.rb +++ b/app/models/concerns/reportable.rb @@ -4,6 +4,10 @@ module Reportable included do has_one :report, as: :process, inverse_of: :process, dependent: :destroy accepts_nested_attributes_for :report + + Report::KINDS.each do |kind| + scope "#{kind}_enabled", -> { joins(:report).where("reports.#{kind}": true) } + end end def report diff --git a/spec/models/budget_spec.rb b/spec/models/budget_spec.rb index 08fb0cea2..3b85e61d9 100644 --- a/spec/models/budget_spec.rb +++ b/spec/models/budget_spec.rb @@ -7,6 +7,33 @@ describe Budget do it_behaves_like "reportable" it_behaves_like "globalizable", :budget + describe "scopes" do + describe ".open" do + it "returns all budgets that are not in the finished phase" do + (Budget::Phase::PHASE_KINDS - ["finished"]).each do |phase| + budget = create(:budget, phase: phase) + expect(Budget.open).to include(budget) + end + end + end + + describe ".valuating_or_later" do + it "returns budgets valuating or later" do + valuating = create(:budget, :valuating) + finished = create(:budget, :finished) + + expect(Budget.valuating_or_later).to match_array([valuating, finished]) + end + + it "does not return budgets which haven't reached valuation" do + create(:budget, :drafting) + create(:budget, :selecting) + + expect(Budget.valuating_or_later).to be_empty + end + end + end + describe "name" do before do budget.update(name_en: "object name") @@ -172,15 +199,6 @@ describe Budget do end end - describe "#open" do - it "returns all budgets that are not in the finished phase" do - (Budget::Phase::PHASE_KINDS - ["finished"]).each do |phase| - budget = create(:budget, phase: phase) - expect(Budget.open).to include(budget) - end - end - end - describe "heading_price" do it "returns the heading price if the heading provided is part of the budget" do heading = create(:budget_heading, price: 100, budget: budget) diff --git a/spec/models/concerns/reportable.rb b/spec/models/concerns/reportable.rb index 107a739fb..e35e4e759 100644 --- a/spec/models/concerns/reportable.rb +++ b/spec/models/concerns/reportable.rb @@ -1,6 +1,36 @@ shared_examples "reportable" do let(:reportable) { create(model_name(described_class)) } + describe "scopes" do + describe ".results_enabled" do + it "includes records with results enabled" do + reportable.update!(results_enabled: true) + + expect(described_class.results_enabled).to eq [reportable] + end + + it "does not include records without results enabled" do + reportable.update!(results_enabled: false) + + expect(described_class.results_enabled).to be_empty + end + end + + describe ".stats_enabled" do + it "includes records with stats enabled" do + reportable.update!(stats_enabled: true) + + expect(described_class.stats_enabled).to eq [reportable] + end + + it "does not include records without stats enabled" do + reportable.update!(stats_enabled: false) + + expect(described_class.stats_enabled).to be_empty + end + end + end + describe "#results_enabled" do it "can write and read the attribute" do reportable.results_enabled = true From 01a01c834d6e456c3d9fdfb93702276780992ec1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 8 Nov 2019 22:12:52 +0100 Subject: [PATCH 6/7] Only generate stats if we can access them There's no point generating stats nobody can access. Note with this change we're automatically excluding polls created in the dashboard, since these polls don't have stats enabled. --- lib/tasks/stats.rake | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/tasks/stats.rake b/lib/tasks/stats.rake index 2d68613c5..66456a0b5 100644 --- a/lib/tasks/stats.rake +++ b/lib/tasks/stats.rake @@ -2,13 +2,14 @@ namespace :stats do desc "Generates stats which are not cached yet" task generate: :environment do ApplicationLogger.new.info "Updating budget and poll stats" + admin_ability = Ability.new(Administrator.first.user) - Budget.find_each do |budget| + Budget.accessible_by(admin_ability, :read_stats).find_each do |budget| Budget::Stats.new(budget).generate print "." end - Poll.find_each do |poll| + Poll.accessible_by(admin_ability, :stats).find_each do |poll| Poll::Stats.new(poll).generate print "." end From ed6a4a1b95e212d657fbf4799e49a32678bf63e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 9 Nov 2019 15:02:32 +0100 Subject: [PATCH 7/7] Don't generate stats for budget polls The link to show stats for these polls is nowhere to be seen in the application, and these stats are included in the budget stats, so it makes sense to restrict access to them. --- app/models/abilities/everyone.rb | 4 ++-- spec/models/abilities/everyone_spec.rb | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app/models/abilities/everyone.rb b/app/models/abilities/everyone.rb index e2a59c140..89a29f4b9 100644 --- a/app/models/abilities/everyone.rb +++ b/app/models/abilities/everyone.rb @@ -7,8 +7,8 @@ module Abilities can [:read, :map, :summary, :share], Proposal can :read, Comment can :read, Poll - can :results, Poll, id: Poll.expired.results_enabled.ids - can :stats, Poll, id: Poll.expired.stats_enabled.ids + can :results, Poll, id: Poll.expired.results_enabled.not_budget.ids + can :stats, Poll, id: Poll.expired.stats_enabled.not_budget.ids can :read, Poll::Question can :read, User can [:read, :welcome], Budget diff --git a/spec/models/abilities/everyone_spec.rb b/spec/models/abilities/everyone_spec.rb index 899639a60..cfdbbf932 100644 --- a/spec/models/abilities/everyone_spec.rb +++ b/spec/models/abilities/everyone_spec.rb @@ -34,10 +34,12 @@ describe Abilities::Everyone do it { should be_able_to(:results, create(:poll, :expired, results_enabled: true)) } it { should_not be_able_to(:results, create(:poll, :expired, results_enabled: false)) } it { should_not be_able_to(:results, create(:poll, :current, results_enabled: true)) } + it { should_not be_able_to(:results, create(:poll, :for_budget, :expired, results_enabled: true)) } it { should be_able_to(:stats, create(:poll, :expired, stats_enabled: true)) } it { should_not be_able_to(:stats, create(:poll, :expired, stats_enabled: false)) } it { should_not be_able_to(:stats, create(:poll, :current, stats_enabled: true)) } + it { should_not be_able_to(:stats, create(:poll, :for_budget, :expired, stats_enabled: true)) } it { should be_able_to(:read_results, create(:budget, :finished, results_enabled: true)) } it { should_not be_able_to(:read_results, create(:budget, :finished, results_enabled: false)) }