diff --git a/app/services/badge_granter.rb b/app/services/badge_granter.rb index 6999be26e1a..675a58e8321 100644 --- a/app/services/badge_granter.rb +++ b/app/services/badge_granter.rb @@ -139,8 +139,7 @@ class BadgeGranter end def self.find_by_type(type) - id = "Badge::Trigger::#{type}".constantize - Badge.where(trigger: id) + Badge.where(trigger: "Badge::Trigger::#{type}".constantize) end def self.queue_key @@ -151,15 +150,18 @@ class BadgeGranter # :target_posts - whether the badge targets posts # :trigger - the Badge::Trigger id def self.contract_checks!(sql, opts = {}) - return unless sql.present? + return if sql.blank? + if Badge::Trigger.uses_post_ids?(opts[:trigger]) raise("Contract violation:\nQuery triggers on posts, but does not reference the ':post_ids' array") unless sql.match(/:post_ids/) raise "Contract violation:\nQuery triggers on posts, but references the ':user_ids' array" if sql.match(/:user_ids/) end + if Badge::Trigger.uses_user_ids?(opts[:trigger]) raise "Contract violation:\nQuery triggers on users, but does not reference the ':user_ids' array" unless sql.match(/:user_ids/) raise "Contract violation:\nQuery triggers on users, but references the ':post_ids' array" if sql.match(/:post_ids/) end + if opts[:trigger] && !Badge::Trigger.is_none?(opts[:trigger]) raise "Contract violation:\nQuery is triggered, but does not reference the ':backfill' parameter.\n(Hint: if :backfill is TRUE, you should ignore the :post_ids/:user_ids)" unless sql.match(/:backfill/) end @@ -168,6 +170,7 @@ class BadgeGranter if opts[:target_posts] raise "Contract violation:\nQuery targets posts, but does not return a 'post_id' column" unless sql.match(/post_id/) end + raise "Contract violation:\nQuery does not return a 'user_id' column" unless sql.match(/user_id/) raise "Contract violation:\nQuery does not return a 'granted_at' column" unless sql.match(/granted_at/) raise "Contract violation:\nQuery ends with a semicolon. Remove the semicolon; your sql will be used in a subquery." if sql.match(/;\s*\z/) @@ -186,22 +189,25 @@ class BadgeGranter count_sql = "SELECT COUNT(*) count FROM (#{sql}) q WHERE :backfill = :backfill" grant_count = DB.query_single(count_sql, params).first.to_i - grants_sql = - if opts[:target_posts] - "SELECT u.id, u.username, q.post_id, t.title, q.granted_at - FROM(#{sql}) q - JOIN users u on u.id = q.user_id + grants_sql = if opts[:target_posts] + <<~SQL + SELECT u.id, u.username, q.post_id, t.title, q.granted_at + FROM (#{sql}) q + JOIN users u on u.id = q.user_id LEFT JOIN badge_posts p on p.id = q.post_id LEFT JOIN topics t on t.id = p.topic_id - WHERE :backfill = :backfill - LIMIT 10" - else - "SELECT u.id, u.username, q.granted_at - FROM(#{sql}) q - JOIN users u on u.id = q.user_id - WHERE :backfill = :backfill - LIMIT 10" - end + WHERE :backfill = :backfill + LIMIT 10 + SQL + else + <<~SQL + SELECT u.id, u.username, q.granted_at + FROM (#{sql}) q + JOIN users u on u.id = q.user_id + WHERE :backfill = :backfill + LIMIT 10 + SQL + end query_plan = nil # HACK: active record sanitization too flexible, force it to go down the sanitization path that cares not for % stuff @@ -235,29 +241,30 @@ class BadgeGranter user_ids = opts[:user_ids] if opts # safeguard fall back to full backfill if more than 200 - if (post_ids && post_ids.length > MAX_ITEMS_FOR_DELTA) || - (user_ids && user_ids.length > MAX_ITEMS_FOR_DELTA) + if (post_ids && post_ids.size > MAX_ITEMS_FOR_DELTA) || + (user_ids && user_ids.size > MAX_ITEMS_FOR_DELTA) post_ids = nil user_ids = nil end - post_ids = nil unless post_ids.present? - user_ids = nil unless user_ids.present? + post_ids = nil if post_ids.blank? + user_ids = nil if user_ids.blank? full_backfill = !user_ids && !post_ids post_clause = badge.target_posts ? "AND (q.post_id = ub.post_id OR NOT :multiple_grant)" : "" post_id_field = badge.target_posts ? "q.post_id" : "NULL" - sql = "DELETE FROM user_badges - WHERE id in ( - SELECT ub.id - FROM user_badges ub - LEFT JOIN ( #{badge.query} ) q - ON q.user_id = ub.user_id - #{post_clause} - WHERE ub.badge_id = :id AND q.user_id IS NULL - )" + sql = <<~SQL + DELETE FROM user_badges + WHERE id IN ( + SELECT ub.id + FROM user_badges ub + LEFT JOIN (#{badge.query}) q ON q.user_id = ub.user_id + #{post_clause} + WHERE ub.badge_id = :id AND q.user_id IS NULL + ) + SQL DB.exec( sql, @@ -270,33 +277,34 @@ class BadgeGranter sql = <<~SQL WITH w as ( - INSERT INTO user_badges(badge_id, user_id, granted_at, granted_by_id, post_id) - SELECT :id, q.user_id, q.granted_at, -1, #{post_id_field} - FROM ( #{badge.query} ) q - LEFT JOIN user_badges ub ON - ub.badge_id = :id AND ub.user_id = q.user_id + INSERT INTO user_badges(badge_id, user_id, granted_at, granted_by_id, post_id) + SELECT :id, q.user_id, q.granted_at, -1, #{post_id_field} + FROM (#{badge.query}) q + LEFT JOIN user_badges ub ON ub.badge_id = :id AND ub.user_id = q.user_id #{post_clause} - /*where*/ - RETURNING id, user_id, granted_at + /*where*/ + ON CONFLICT DO NOTHING + RETURNING id, user_id, granted_at ) - select w.*, username, locale, (u.admin OR u.moderator) AS staff FROM w - JOIN users u on u.id = w.user_id + SELECT w.*, username, locale, (u.admin OR u.moderator) AS staff + FROM w + JOIN users u on u.id = w.user_id SQL builder = DB.build(sql) - builder.where("ub.badge_id IS NULL AND q.user_id <> -1") + builder.where("ub.badge_id IS NULL AND q.user_id > 0") if (post_ids || user_ids) && !badge.query.include?(":backfill") Rails.logger.warn "Your triggered badge query for #{badge.name} does not include the :backfill param, skipping!" return end - if (post_ids && !badge.query.include?(":post_ids")) + if post_ids && !badge.query.include?(":post_ids") Rails.logger.warn "Your triggered badge query for #{badge.name} does not include the :post_ids param, skipping!" return end - if (user_ids && !badge.query.include?(":user_ids")) + if user_ids && !badge.query.include?(":user_ids") Rails.logger.warn "Your triggered badge query for #{badge.name} does not include the :user_ids param, skipping!" return end @@ -309,32 +317,29 @@ class BadgeGranter user_ids: user_ids || [-2]).each do |row| # old bronze badges do not matter - next if badge.badge_type_id == (BadgeType::Bronze) && row.granted_at < (2.days.ago) + next if badge.badge_type_id == BadgeType::Bronze && row.granted_at < 2.days.ago # Try to use user locale in the badge notification if possible without too much resources - notification_locale = - if SiteSetting.allow_user_locale && row.locale.present? - row.locale - else - SiteSetting.default_locale - end + notification_locale = if SiteSetting.allow_user_locale && row.locale.present? + row.locale + else + SiteSetting.default_locale + end - # Make this variable in this scope - notification = nil + next if row.staff && badge.awarded_for_trust_level? - next if (row.staff && badge.awarded_for_trust_level?) - - I18n.with_locale(notification_locale) do - notification = Notification.create!( - user_id: row.user_id, - notification_type: Notification.types[:granted_badge], - data: { - badge_id: badge.id, - badge_name: badge.display_name, - badge_slug: badge.slug, - badge_title: badge.allow_title, - username: row.username - }.to_json) + notification = I18n.with_locale(notification_locale) do + Notification.create!( + user_id: row.user_id, + notification_type: Notification.types[:granted_badge], + data: { + badge_id: badge.id, + badge_name: badge.display_name, + badge_slug: badge.slug, + badge_title: badge.allow_title, + username: row.username + }.to_json + ) end DB.exec( @@ -345,9 +350,9 @@ class BadgeGranter end badge.reset_grant_count! - rescue => ex + rescue => e Rails.logger.error("Failed to backfill '#{badge.name}' badge: #{opts}") - raise ex + raise e end def self.revoke_ungranted_titles! diff --git a/spec/services/badge_granter_spec.rb b/spec/services/badge_granter_spec.rb index 451cb158a1a..e79288ce7e1 100644 --- a/spec/services/badge_granter_spec.rb +++ b/spec/services/badge_granter_spec.rb @@ -74,31 +74,30 @@ describe BadgeGranter do end it 'should grant missing badges' do + nice_topic = Badge.find(Badge::NiceTopic) good_topic = Badge.find(Badge::GoodTopic) post = Fabricate(:post, like_count: 30) + 2.times { - BadgeGranter.backfill(Badge.find(Badge::NiceTopic), post_ids: [post.id]) + BadgeGranter.backfill(nice_topic, post_ids: [post.id]) BadgeGranter.backfill(good_topic) } # TODO add welcome - expect(post.user.user_badges.pluck(:badge_id).sort).to eq([Badge::NiceTopic, Badge::GoodTopic]) - + expect(post.user.user_badges.pluck(:badge_id)).to contain_exactly(nice_topic.id, good_topic.id) expect(post.user.notifications.count).to eq(2) - notification = post.user.notifications.last - data = notification.data_hash + data = post.user.notifications.last.data_hash expect(data["badge_id"]).to eq(good_topic.id) expect(data["badge_slug"]).to eq(good_topic.slug) expect(data["username"]).to eq(post.user.username) - expect(Badge.find(Badge::NiceTopic).grant_count).to eq(1) - expect(Badge.find(Badge::GoodTopic).grant_count).to eq(1) + expect(nice_topic.grant_count).to eq(1) + expect(good_topic.grant_count).to eq(1) end it 'should grant badges in the user locale' do - SiteSetting.allow_user_locale = true nice_topic = Badge.find(Badge::NiceTopic)