Merge remote-tracking branch 'origin/7946-only-active-users-in-leader…

…boards' into develop

app/controllers/request_game_controller.rb

@@ -38,7 +38,7 @@ def play
     end
 
     @league_table_28_days = RequestClassification.league_table(10,
-      [ "created_at >= ?", Time.zone.now - 28.days ])
+      [ "request_classifications.created_at >= ?", Time.zone.now - 28.days ])
     @league_table_all_time = RequestClassification.league_table(10)
     @play_urls = true
   end

app/models/request_classification.rb

@@ -30,7 +30,8 @@ def self.league_table(size, conditions=nil)
       group('user_id').
         order(cnt: :desc).
           limit(size).
-            includes(:user)
+            joins(:user).
+              merge(User.active)
     query = query.where(*conditions) if conditions
     query
   end

app/models/statistics/leaderboard.rb

@@ -4,6 +4,7 @@ class Leaderboard
     def all_time_requesters
       InfoRequest.is_public.
                   joins(:user).
+                  merge(User.active).
                   group(:user).
                   order(count_info_requests_all: :desc).
                   limit(10).
@@ -15,40 +16,33 @@ def last_28_day_requesters
       InfoRequest.is_public.
                   where('info_requests.created_at >= ?', 28.days.ago).
                   joins(:user).
+                  merge(User.active).
                   group(:user).
                   order(count_info_requests_all: :desc).
                   limit(10).
                   count
     end
 
     def all_time_commenters
-      commenters = Comment.visible.
-                           joins(:user).
-                           group('comments.user_id').
-                           order(count_all: :desc).
-                           limit(10).
-                           count
-      # TODO: Have user objects automatically instantiated like the InfoRequest
-      # queries above
-      result = {}
-      commenters.each { |user_id, count| result[User.find(user_id)] = count }
-      result
+      Comment.visible.
+              joins(:user).
+              merge(User.active).
+              group(:user).
+              order(count_all: :desc).
+              limit(10).
+              count
     end
 
     def last_28_day_commenters
       # TODO: Refactor as it's basically the same as all_time_commenters
-      commenters = Comment.visible.
-                           where('comments.created_at >= ?', 28.days.ago).
-                           joins(:user).
-                           group('comments.user_id').
-                           order(count_all: :desc).
-                           limit(10).
-                           count
-      # TODO: Have user objects automatically instantiated like the InfoRequest
-      # queries above
-      result = {}
-      commenters.each { |user_id, count| result[User.find(user_id)] = count }
-      result
+      Comment.visible.
+              where('comments.created_at >= ?', 28.days.ago).
+              joins(:user).
+              merge(User.active).
+              group(:user).
+              order(count_all: :desc).
+              limit(10).
+              count
     end
   end
 end

doc/CHANGES.md

@@ -11,6 +11,8 @@
 * Add admin view of unmasked version of main body part attachments (Gareth Rees)
 * Add internal ID number to authority CSV download (Alex Parsons, Graeme
   Porteous)
+* Don't show users that have closed their account or been banned on leaderboards
+  (Chris Mytton)
 
 # 0.44.0.0
 

spec/models/request_classification_spec.rb

@@ -19,9 +19,13 @@
     before do
       @user_one = FactoryBot.create(:user)
       @user_two = FactoryBot.create(:user)
+      @banned_user = FactoryBot.create(:user, :banned)
+      @closed_account_user = FactoryBot.create(:user, :closed)
       FactoryBot.create(:request_classification, user: @user_one)
       FactoryBot.create(:request_classification, user: @user_one)
       FactoryBot.create(:request_classification, user: @user_two)
+      10.times { FactoryBot.create(:request_classification, user: @banned_user) }
+      FactoryBot.create(:request_classification, user: @closed_account_user)
     end
 
     it "returns a list of users' classifications with counts in descending order" do

spec/models/statistics/leaderboard_spec.rb

@@ -2,19 +2,24 @@
 
 RSpec.describe Statistics::Leaderboard do
   let(:statistics) { described_class.new }
+  before { User.destroy_all }
 
   describe '#all_time_requesters' do
     it 'gets most frequent requesters' do
-      User.destroy_all
-
       user1 = FactoryBot.create(:user)
       user2 = FactoryBot.create(:user)
       user3 = FactoryBot.create(:user)
+      banned_user = FactoryBot.create(:user, :banned)
+      closed_account_user = FactoryBot.create(:user, :closed)
 
       travel_to(6.months.ago) do
         5.times { FactoryBot.create(:info_request, user: user1) }
         2.times { FactoryBot.create(:info_request, user: user2) }
         FactoryBot.create(:info_request, user: user3)
+        10.times { FactoryBot.create(:info_request, user: banned_user) }
+        10.times { FactoryBot.create(:info_request, user: closed_account_user) }
+        3.times { FactoryBot.create(:info_request, :hidden, user: user1) }
+        3.times { FactoryBot.create(:embargoed_request, user: user1) }
       end
 
       expect(statistics.all_time_requesters).
@@ -36,6 +41,14 @@
       FactoryBot.create(:info_request,
                         user: user_with_an_old_request,
                         created_at: 2.months.ago)
+      banned_user = FactoryBot.create(:user, :banned)
+      10.times { FactoryBot.create(:info_request, user: banned_user) }
+      closed_account_user = FactoryBot.create(:user, :closed)
+      10.times { FactoryBot.create(:info_request, user: closed_account_user) }
+      user_with_embargoed_request = FactoryBot.create(:user)
+      FactoryBot.create(:embargoed_request, user: user_with_embargoed_request)
+      user_with_hidden_request = FactoryBot.create(:user)
+      FactoryBot.create(:info_request, :hidden, user: user_with_hidden_request)
 
       expect(statistics.last_28_day_requesters).
         to eql({ user_with_3_requests => 3,
@@ -48,6 +61,8 @@
     let(:many_comments) { FactoryBot.create(:user) }
     let(:some_comments) { FactoryBot.create(:user) }
     let!(:none_comments) { FactoryBot.create(:user) }
+    let(:banned_user) { FactoryBot.create(:user, :banned) }
+    let(:closed_account_user) { FactoryBot.create(:user, :closed) }
 
     before do
       FactoryBot.create(:comment, user: many_comments)
@@ -56,14 +71,14 @@
       FactoryBot.create(:comment, user: many_comments)
       FactoryBot.create(:comment, user: some_comments)
       FactoryBot.create(:comment, user: many_comments)
+      10.times { FactoryBot.create(:comment, user: banned_user) }
+      10.times { FactoryBot.create(:comment, user: closed_account_user) }
     end
 
     it 'gets most frequent commenters' do
-      # FIXME: This uses fixtures. Change it to use factories when we can.
       expect(statistics.all_time_commenters).
         to eql({ many_comments => 4,
-                 some_comments => 2,
-                 users(:silly_name_user) => 1 })
+                 some_comments => 2 })
     end
   end
 
@@ -79,6 +94,10 @@
       FactoryBot.create(:comment,
                         user: user_with_an_old_comment,
                         created_at: 2.months.ago)
+      banned_user = FactoryBot.create(:user, :banned)
+      10.times { FactoryBot.create(:comment, user: banned_user) }
+      closed_account_user = FactoryBot.create(:user, :closed)
+      10.times { FactoryBot.create(:comment, user: closed_account_user) }
 
       expect(statistics.last_28_day_commenters).
         to eql({ user_with_3_comments => 3,