FIX: prevent error when badge has already been awarded

FIX: prevent error when badge has already been awarded

diff --git a/app/services/badge_granter.rb b/app/services/badge_granter.rb
index 6999be2..675a58e 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?)
-
-      I18n.with_locale(notification_locale) do
-        notification = Notification.create!(
-                          user_id: row.user_id,
-                          notification_type: Notification.types[:granted_badge],
-                          data: {

[... diff too long, it was truncated ...]

GitHub
sha: 88e861e8

Note, in future I would split this into 2 commits,

  1. To fix the actual issue
  2. To “tidy up”

It is very hard to figure out what the actual fix is. Totally support all the unless removal its just a bit hard to review this way.

1 Like