From 88e861e8951846ea41811315989a17c0114baae5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9gis=20Hanol?= <regis@hanol.fr>
Date: Fri, 4 Jan 2019 15:17:54 +0100
Subject: [PATCH] FIX: prevent error when badge has already been awarded
---
app/services/badge_granter.rb | 137 ++++++++++++++--------------
spec/services/badge_granter_spec.rb | 15 ++-
2 files changed, 78 insertions(+), 74 deletions(-)
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)