From b704e338eff61e6fc18e0e4207d14583eea96702 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Mon, 16 Jan 2023 11:55:00 +0100 Subject: [PATCH] DEV: extract anniversary badge query (#19716) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit So it can easily be overwritten in a plugin for example. ### Added more tests to provide better coverage We previously only had `u.silenced_till IS NULL` but I made it consistent with pretty much every other places where we check for "active" users. These two new lines do change the query a tiny bit though. **Before** - You could not get the badge if you were currently silenced (no matter what period is being checked) - You could get the badge if you were suspended 😬 **After** - You can't get the badge if you were silenced during the past year - You can't get the badge if you were suspended during the past year ### Improved the performance of the query by using `NOT EXISTS` instead of `LEFT JOIN / COUNT() = 0` There is no difference in behaviour between ```sql LEFT JOIN user_badges AS ub ON ub.user_id = u.id AND ... [...] HAVING COUNT(ub.*) = 0 ``` and ```sql NOT EXISTS (SELECT 1 FROM user_badges AS ub WHERE ub.user_id = u.id AND ...) ``` The only difference is performance-wise. The `NOT EXISTS` is 10-30% faster on very large databases (aka. posts and users in X millions). I checked on 3 of the largest datasets I could find. --- .../scheduled/grant_anniversary_badges.rb | 27 ++------------ lib/badge_queries.rb | 28 +++++++++++++++ spec/jobs/grant_anniversary_badges_spec.rb | 36 +++++++++++++++++++ 3 files changed, 67 insertions(+), 24 deletions(-) diff --git a/app/jobs/scheduled/grant_anniversary_badges.rb b/app/jobs/scheduled/grant_anniversary_badges.rb index c244a2e92ee..06e35cb3684 100644 --- a/app/jobs/scheduled/grant_anniversary_badges.rb +++ b/app/jobs/scheduled/grant_anniversary_badges.rb @@ -6,34 +6,13 @@ module Jobs def execute(args) return unless SiteSetting.enable_badges? - badge = Badge.find_by(id: Badge::Anniversary, enabled: true) - return unless badge + return unless badge = Badge.find_by(id: Badge::Anniversary, enabled: true) start_date = args[:start_date] || 1.year.ago end_date = start_date + 1.year - fmt_end_date = end_date.iso8601(6) - fmt_start_date = start_date.iso8601(6) - - user_ids = DB.query_single <<~SQL - SELECT u.id AS user_id - FROM users AS u - INNER JOIN posts AS p ON p.user_id = u.id - INNER JOIN topics AS t ON p.topic_id = t.id - LEFT OUTER JOIN user_badges AS ub ON ub.user_id = u.id AND - ub.badge_id = #{Badge::Anniversary} AND - ub.granted_at BETWEEN '#{fmt_start_date}' AND '#{fmt_end_date}' - WHERE u.active AND - u.silenced_till IS NULL AND - NOT p.hidden AND - p.deleted_at IS NULL AND - t.visible AND - t.archetype <> 'private_message' AND - p.created_at BETWEEN '#{fmt_start_date}' AND '#{fmt_end_date}' AND - u.created_at <= '#{fmt_start_date}' - GROUP BY u.id - HAVING COUNT(p.id) > 0 AND COUNT(ub.id) = 0 - SQL + sql = BadgeQueries.anniversaries(start_date, end_date) + user_ids = DB.query_single(sql) User .where(id: user_ids) diff --git a/lib/badge_queries.rb b/lib/badge_queries.rb index 4eda8098739..c73c579d5ca 100644 --- a/lib/badge_queries.rb +++ b/lib/badge_queries.rb @@ -271,4 +271,32 @@ module BadgeQueries WHERE "rank" = 1 SQL end + + def self.anniversaries(start_date, end_date) + start_date = start_date.iso8601(6) + end_date = end_date.iso8601(6) + + <<~SQL + SELECT u.id + FROM users AS u + JOIN posts AS p ON p.user_id = u.id + JOIN topics AS t ON p.topic_id = t.id + WHERE u.id > 0 + AND u.active + AND NOT u.staged + AND (u.silenced_till IS NULL OR u.silenced_till < '#{start_date}') + AND (u.suspended_till IS NULL OR u.suspended_till < '#{start_date}') + AND u.created_at <= '#{start_date}' + AND NOT p.hidden + AND p.deleted_at IS NULL + AND p.created_at BETWEEN '#{start_date}' AND '#{end_date}' + AND t.visible + AND t.archetype <> 'private_message' + AND t.deleted_at IS NULL + AND NOT EXISTS (SELECT 1 FROM user_badges AS ub WHERE ub.user_id = u.id AND ub.badge_id = #{Badge::Anniversary} AND ub.granted_at BETWEEN '#{start_date}' AND '#{end_date}') + AND NOT EXISTS (SELECT 1 FROM anonymous_users AS au WHERE au.user_id = u.id) + GROUP BY u.id + HAVING COUNT(p.id) > 0 + SQL + end end diff --git a/spec/jobs/grant_anniversary_badges_spec.rb b/spec/jobs/grant_anniversary_badges_spec.rb index 5cee3130870..57b54708c9b 100644 --- a/spec/jobs/grant_anniversary_badges_spec.rb +++ b/spec/jobs/grant_anniversary_badges_spec.rb @@ -12,6 +12,15 @@ RSpec.describe Jobs::GrantAnniversaryBadges do expect(badge).to be_blank end + it "doesn't award to a bot" do + user = Fabricate(:bot, created_at: 400.days.ago) + Fabricate(:post, user: user, created_at: 1.week.ago) + granter.execute({}) + + badge = user.user_badges.where(badge_id: Badge::Anniversary) + expect(badge).to be_blank + end + it "doesn't award to an inactive user" do user = Fabricate(:user, created_at: 400.days.ago, active: false) Fabricate(:post, user: user, created_at: 1.week.ago) @@ -30,6 +39,33 @@ RSpec.describe Jobs::GrantAnniversaryBadges do expect(badge).to be_blank end + it "doesn't award to a suspended user" do + user = Fabricate(:user, created_at: 400.days.ago, suspended_till: 1.year.from_now) + Fabricate(:post, user: user, created_at: 1.week.ago) + granter.execute({}) + + badge = user.user_badges.where(badge_id: Badge::Anniversary) + expect(badge).to be_blank + end + + it "doesn't award to a staged user" do + user = Fabricate(:user, created_at: 400.days.ago, staged: true) + Fabricate(:post, user: user, created_at: 1.week.ago) + granter.execute({}) + + badge = user.user_badges.where(badge_id: Badge::Anniversary) + expect(badge).to be_blank + end + + it "doesn't award to an anonymous user" do + user = Fabricate(:anonymous, created_at: 400.days.ago) + Fabricate(:post, user: user, created_at: 1.week.ago) + granter.execute({}) + + badge = user.user_badges.where(badge_id: Badge::Anniversary) + expect(badge).to be_blank + end + it "doesn't award when a post is deleted" do user = Fabricate(:user, created_at: 400.days.ago) Fabricate(:post, user: user, created_at: 1.week.ago, deleted_at: 1.day.ago)