From 478decb27caea8d428027af01ceaedcd97e81c40 Mon Sep 17 00:00:00 2001 From: Chris Mytton Date: Mon, 13 Nov 2023 12:23:25 +0000 Subject: [PATCH 1/6] Prevent banned users from appearing on leaderboards --- app/models/statistics/leaderboard.rb | 4 ++++ spec/models/statistics/leaderboard_spec.rb | 15 ++++++++++----- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/app/models/statistics/leaderboard.rb b/app/models/statistics/leaderboard.rb index 974e69274a..d81af3f46f 100644 --- a/app/models/statistics/leaderboard.rb +++ b/app/models/statistics/leaderboard.rb @@ -4,6 +4,7 @@ class Leaderboard def all_time_requesters InfoRequest.is_public. joins(:user). + merge(User.not_banned). group(:user). order(count_info_requests_all: :desc). limit(10). @@ -15,6 +16,7 @@ def last_28_day_requesters InfoRequest.is_public. where('info_requests.created_at >= ?', 28.days.ago). joins(:user). + merge(User.not_banned). group(:user). order(count_info_requests_all: :desc). limit(10). @@ -24,6 +26,7 @@ def last_28_day_requesters def all_time_commenters commenters = Comment.visible. joins(:user). + merge(User.not_banned). group('comments.user_id'). order(count_all: :desc). limit(10). @@ -40,6 +43,7 @@ def last_28_day_commenters commenters = Comment.visible. where('comments.created_at >= ?', 28.days.ago). joins(:user). + merge(User.not_banned). group('comments.user_id'). order(count_all: :desc). limit(10). diff --git a/spec/models/statistics/leaderboard_spec.rb b/spec/models/statistics/leaderboard_spec.rb index 09421f011e..96ff1b0f65 100644 --- a/spec/models/statistics/leaderboard_spec.rb +++ b/spec/models/statistics/leaderboard_spec.rb @@ -2,19 +2,20 @@ 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) 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) } end expect(statistics.all_time_requesters). @@ -36,6 +37,8 @@ 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) } expect(statistics.last_28_day_requesters). to eql({ user_with_3_requests => 3, @@ -48,6 +51,7 @@ 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) } before do FactoryBot.create(:comment, user: many_comments) @@ -56,14 +60,13 @@ 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) } 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 +82,8 @@ 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) } expect(statistics.last_28_day_commenters). to eql({ user_with_3_comments => 3, From 9a97886bc632059500cb6f968db187ec219ba1b8 Mon Sep 17 00:00:00 2001 From: Chris Mytton Date: Tue, 14 Nov 2023 10:34:41 +0000 Subject: [PATCH 2/6] Have leaderboard comment methods return user instances directly Changed `.group('comments.user_id')` to `.group(:user)` to match the InfoRequest-based methods above. --- app/models/statistics/leaderboard.rb | 40 +++++++++++----------------- 1 file changed, 15 insertions(+), 25 deletions(-) diff --git a/app/models/statistics/leaderboard.rb b/app/models/statistics/leaderboard.rb index d81af3f46f..8e203474da 100644 --- a/app/models/statistics/leaderboard.rb +++ b/app/models/statistics/leaderboard.rb @@ -24,35 +24,25 @@ def last_28_day_requesters end def all_time_commenters - commenters = Comment.visible. - joins(:user). - merge(User.not_banned). - 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.not_banned). + 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). - merge(User.not_banned). - 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.not_banned). + group(:user). + order(count_all: :desc). + limit(10). + count end end end From d074cd6b4833d4d2c9d2726bed30122574e6a503 Mon Sep 17 00:00:00 2001 From: Chris Mytton Date: Tue, 14 Nov 2023 12:56:52 +0000 Subject: [PATCH 3/6] Add test for hidden and embargoed requests This tests that the InfoRequest.is_public bit in the leaderboards methods is working as expected. --- spec/models/statistics/leaderboard_spec.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/spec/models/statistics/leaderboard_spec.rb b/spec/models/statistics/leaderboard_spec.rb index 96ff1b0f65..7c0ce39e7c 100644 --- a/spec/models/statistics/leaderboard_spec.rb +++ b/spec/models/statistics/leaderboard_spec.rb @@ -16,6 +16,8 @@ 2.times { FactoryBot.create(:info_request, user: user2) } FactoryBot.create(:info_request, user: user3) 10.times { FactoryBot.create(:info_request, user: banned_user) } + 3.times { FactoryBot.create(:info_request, :hidden, user: user1) } + 3.times { FactoryBot.create(:embargoed_request, user: user1) } end expect(statistics.all_time_requesters). @@ -39,6 +41,10 @@ created_at: 2.months.ago) banned_user = FactoryBot.create(:user, :banned) 10.times { FactoryBot.create(:info_request, user: banned_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, From 3ac2c07979243a2e30d63f35dc93373dc5bc3b06 Mon Sep 17 00:00:00 2001 From: Chris Mytton Date: Tue, 14 Nov 2023 14:36:43 +0000 Subject: [PATCH 4/6] Prevent users with closed accounts from appearing on leaderboards --- app/models/statistics/leaderboard.rb | 8 ++++---- spec/models/statistics/leaderboard_spec.rb | 8 ++++++++ 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/app/models/statistics/leaderboard.rb b/app/models/statistics/leaderboard.rb index 8e203474da..7c8dffcf8b 100644 --- a/app/models/statistics/leaderboard.rb +++ b/app/models/statistics/leaderboard.rb @@ -4,7 +4,7 @@ class Leaderboard def all_time_requesters InfoRequest.is_public. joins(:user). - merge(User.not_banned). + merge(User.active). group(:user). order(count_info_requests_all: :desc). limit(10). @@ -16,7 +16,7 @@ def last_28_day_requesters InfoRequest.is_public. where('info_requests.created_at >= ?', 28.days.ago). joins(:user). - merge(User.not_banned). + merge(User.active). group(:user). order(count_info_requests_all: :desc). limit(10). @@ -26,7 +26,7 @@ def last_28_day_requesters def all_time_commenters Comment.visible. joins(:user). - merge(User.not_banned). + merge(User.active). group(:user). order(count_all: :desc). limit(10). @@ -38,7 +38,7 @@ def last_28_day_commenters Comment.visible. where('comments.created_at >= ?', 28.days.ago). joins(:user). - merge(User.not_banned). + merge(User.active). group(:user). order(count_all: :desc). limit(10). diff --git a/spec/models/statistics/leaderboard_spec.rb b/spec/models/statistics/leaderboard_spec.rb index 7c0ce39e7c..e4d7ed6319 100644 --- a/spec/models/statistics/leaderboard_spec.rb +++ b/spec/models/statistics/leaderboard_spec.rb @@ -10,12 +10,14 @@ 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 @@ -41,6 +43,8 @@ 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) @@ -58,6 +62,7 @@ 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) @@ -67,6 +72,7 @@ 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 @@ -90,6 +96,8 @@ 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, From eb2892da2a6f11adbb8321f9fbc966495aadb4b5 Mon Sep 17 00:00:00 2001 From: Chris Mytton Date: Tue, 14 Nov 2023 14:40:25 +0000 Subject: [PATCH 5/6] Update CHANGES.md --- doc/CHANGES.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/doc/CHANGES.md b/doc/CHANGES.md index af7cd4fba7..8a72c644c9 100644 --- a/doc/CHANGES.md +++ b/doc/CHANGES.md @@ -9,6 +9,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 From 4be3380685d7fd712fb32e65628cdcf80dfa339c Mon Sep 17 00:00:00 2001 From: Chris Mytton Date: Wed, 15 Nov 2023 11:59:31 +0000 Subject: [PATCH 6/6] Only show active users on request classification league table --- app/controllers/request_game_controller.rb | 2 +- app/models/request_classification.rb | 3 ++- spec/models/request_classification_spec.rb | 4 ++++ 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/app/controllers/request_game_controller.rb b/app/controllers/request_game_controller.rb index 0af3622dff..6e3a9906dd 100644 --- a/app/controllers/request_game_controller.rb +++ b/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 diff --git a/app/models/request_classification.rb b/app/models/request_classification.rb index 32b3b07173..bd195f3fa5 100644 --- a/app/models/request_classification.rb +++ b/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 diff --git a/spec/models/request_classification_spec.rb b/spec/models/request_classification_spec.rb index e754bba676..dc47199167 100644 --- a/spec/models/request_classification_spec.rb +++ b/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