FIX: prevent error when badge has already been awarded

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: "Badge::Trigger::#{type}".constantize)
-    Badge.where(trigger: id)
   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 =
+    grants_sql = if opts[:target_posts]
-     if opts[:target_posts]
+      <<~SQL
-       "SELECT u.id, u.username, q.post_id, t.title, q.granted_at
+        SELECT u.id, u.username, q.post_id, t.title, q.granted_at
-     FROM(#{sql}) q
+          FROM (#{sql}) q
-     JOIN users u on u.id = q.user_id
+          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
+         WHERE :backfill = :backfill
-     LIMIT 10"
+         LIMIT 10
-     else
+      SQL
-       "SELECT u.id, u.username, q.granted_at
+    else
-     FROM(#{sql}) q
+      <<~SQL
-     JOIN users u on u.id = q.user_id
+        SELECT u.id, u.username, q.granted_at
-     WHERE :backfill = :backfill
+         FROM (#{sql}) q
-     LIMIT 10"
+         JOIN users u on u.id = q.user_id
-     end
+        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) ||
+    if (post_ids && post_ids.size > MAX_ITEMS_FOR_DELTA) ||
-       (user_ids && user_ids.length > 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?
+    post_ids = nil if post_ids.blank?
-    user_ids = nil unless user_ids.present?
+    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
+    sql = <<~SQL
-           WHERE id in (
+      DELETE FROM user_badges
-             SELECT ub.id
+       WHERE id IN (
-             FROM user_badges ub
+         SELECT ub.id
-             LEFT JOIN ( #{badge.query} ) q
+           FROM user_badges ub
-             ON q.user_id = ub.user_id
+      LEFT JOIN (#{badge.query}) q ON q.user_id = ub.user_id
-              #{post_clause}
+         #{post_clause}
-             WHERE ub.badge_id = :id AND q.user_id IS NULL
+         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)
+        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}
+        SELECT :id, q.user_id, q.granted_at, -1, #{post_id_field}
-      FROM ( #{badge.query} ) q
+          FROM (#{badge.query}) q
-      LEFT JOIN user_badges ub ON
+     LEFT JOIN user_badges ub ON ub.badge_id = :id AND ub.user_id = q.user_id
-        ub.badge_id = :id AND ub.user_id = q.user_id
         #{post_clause}
-      /*where*/
+        /*where*/
-      RETURNING id, user_id, granted_at
+        ON CONFLICT DO NOTHING
+        RETURNING id, user_id, granted_at
       )
-      select w.*, username, locale, (u.admin OR u.moderator) AS staff FROM w
+      SELECT w.*, username, locale, (u.admin OR u.moderator) AS staff
-      JOIN users u on u.id = w.user_id
+        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 =
+      notification_locale = if SiteSetting.allow_user_locale && row.locale.present?
-        if SiteSetting.allow_user_locale && row.locale.present?
+        row.locale
-          row.locale
+      else
-        else
+        SiteSetting.default_locale
-          SiteSetting.default_locale
+      end
-        end
 
-      # Make this variable in this scope
+      next if row.staff && badge.awarded_for_trust_level?
-      notification = nil
 
-      next if (row.staff && badge.awarded_for_trust_level?)
+      notification = I18n.with_locale(notification_locale) do
-
+        Notification.create!(
-      I18n.with_locale(notification_locale) do
+          user_id: row.user_id,
-        notification = Notification.create!(
+          notification_type: Notification.types[:granted_badge],
-                          user_id: row.user_id,
+          data: {
-                          notification_type: Notification.types[:granted_badge],
+            badge_id: badge.id,
-                          data: {
+            badge_name: badge.display_name,
-                            badge_id: badge.id,
+            badge_slug: badge.slug,
-                            badge_name: badge.display_name,
+            badge_title: badge.allow_title,
-                            badge_slug: badge.slug,
+            username: row.username
-                            badge_title: badge.allow_title,
+          }.to_json
-                            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!

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 = post.user.notifications.last.data_hash
-      data = notification.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(nice_topic.grant_count).to eq(1)
-      expect(Badge.find(Badge::GoodTopic).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)