From 22cd2596871711b44c28660f48c2aeef0fc439bb Mon Sep 17 00:00:00 2001 From: Sam Date: Mon, 11 Aug 2014 09:21:06 +1000 Subject: [PATCH] FIX: remove faulty "ensure consistency" badge job --- app/jobs/scheduled/ensure_db_consistency.rb | 2 -- app/models/user.rb | 2 +- app/models/user_badge.rb | 30 ++-------------- app/services/badge_granter.rb | 16 +++++++-- ...40809224243_add_user_badge_unique_index.rb | 34 +++++++++++++++++++ spec/services/badge_granter_spec.rb | 6 +++- 6 files changed, 55 insertions(+), 35 deletions(-) create mode 100644 db/migrate/20140809224243_add_user_badge_unique_index.rb diff --git a/app/jobs/scheduled/ensure_db_consistency.rb b/app/jobs/scheduled/ensure_db_consistency.rb index c4e940f81..d04b30f3c 100644 --- a/app/jobs/scheduled/ensure_db_consistency.rb +++ b/app/jobs/scheduled/ensure_db_consistency.rb @@ -9,8 +9,6 @@ module Jobs Group.refresh_automatic_groups! Notification.ensure_consistency! UserAction.ensure_consistency! - UserBadge.ensure_consistency! - # ensure consistent UserStat.update_view_counts(13.hours.ago) end end diff --git a/app/models/user.rb b/app/models/user.rb index 379e27dca..d977401b4 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -773,8 +773,8 @@ end # uploaded_avatar_id :integer # email_always :boolean default(FALSE), not null # mailing_list_mode :boolean default(FALSE), not null -# primary_group_id :integer # locale :string(10) +# primary_group_id :integer # registration_ip_address :inet # last_redirected_to_top_at :datetime # disable_jump_reply :boolean default(FALSE), not null diff --git a/app/models/user_badge.rb b/app/models/user_badge.rb index 82f5555e9..5c6894d4d 100644 --- a/app/models/user_badge.rb +++ b/app/models/user_badge.rb @@ -17,34 +17,6 @@ class UserBadge < ActiveRecord::Base after_destroy do Badge.decrement_counter 'grant_count', self.badge_id end - - - # Make sure we don't have duplicate badges. - def self.ensure_consistency! - dup_ids = exec_sql("SELECT u1.id - FROM user_badges u1, user_badges u2, badges - WHERE u1.badge_id = badges.id - AND u1.user_id = u2.user_id - AND u1.badge_id = u2.badge_id - AND (NOT badges.multiple_grant) - AND u1.granted_at > u2.granted_at - LIMIT 1000").to_a - - dup_ids << exec_sql("SELECT u1.id - FROM user_badges u1, user_badges u2, badges - WHERE u1.badge_id = badges.id - AND u1.user_id = u2.user_id - AND u1.badge_id = u2.badge_id - AND badges.multiple_grant - AND u1.post_id = u2.post_id - AND u1.granted_at > u2.granted_at - LIMIT 1000").to_a - - dup_ids.flatten! - dup_ids.map! {|row| row['id'].to_i } - dup_ids.uniq! - UserBadge.where(id: dup_ids).destroy_all - end end # == Schema Information @@ -58,9 +30,11 @@ end # granted_by_id :integer not null # post_id :integer # notification_id :integer +# seq :integer default(0), not null # # Indexes # # index_user_badges_on_badge_id_and_user_id (badge_id,user_id) # index_user_badges_on_badge_id_and_user_id_and_post_id (badge_id,user_id,post_id) UNIQUE +# index_user_badges_on_badge_id_and_user_id_and_seq (badge_id,user_id,seq) UNIQUE # diff --git a/app/services/badge_granter.rb b/app/services/badge_granter.rb index 10e97df61..19aede8e6 100644 --- a/app/services/badge_granter.rb +++ b/app/services/badge_granter.rb @@ -23,17 +23,27 @@ class BadgeGranter if user_badge.nil? || (@badge.multiple_grant? && @post_id.nil?) UserBadge.transaction do - user_badge = UserBadge.create!(badge: @badge, user: @user, + seq = 0 + if @badge.multiple_grant? + seq = UserBadge.where(badge: @badge, user: @user).maximum(:seq) + seq = (seq || -1) + 1 + end + + user_badge = UserBadge.create!(badge: @badge, + user: @user, granted_by: @granted_by, granted_at: Time.now, - post_id: @post_id) + post_id: @post_id, + seq: seq) if @granted_by != Discourse.system_user StaffActionLogger.new(@granted_by).log_badge_grant(user_badge) end if SiteSetting.enable_badges? - notification = @user.notifications.create(notification_type: Notification.types[:granted_badge], data: { badge_id: @badge.id, badge_name: @badge.name }.to_json) + notification = @user.notifications.create( + notification_type: Notification.types[:granted_badge], + data: { badge_id: @badge.id, badge_name: @badge.name }.to_json) user_badge.update_attributes notification_id: notification.id end end diff --git a/db/migrate/20140809224243_add_user_badge_unique_index.rb b/db/migrate/20140809224243_add_user_badge_unique_index.rb new file mode 100644 index 000000000..aefb27af4 --- /dev/null +++ b/db/migrate/20140809224243_add_user_badge_unique_index.rb @@ -0,0 +1,34 @@ +class AddUserBadgeUniqueIndex < ActiveRecord::Migration + def up + # used to keep badges distinct + add_column :user_badges, :seq, :integer, default: 0, null: false + + # invent artificial seq for badges + execute " + UPDATE user_badges ub1 SET seq = X.seq + FROM ( + SELECT ub.id, rank() OVER (PARTITION BY user_id ORDER BY granted_at) seq + FROM user_badges ub + JOIN badges b ON b.id = ub.badge_id + WHERE b.multiple_grant + ) X + WHERE ub1.id = X.id + " + + # delete all single award dupes + execute " + DELETE FROM user_badges ub1 + WHERE ub1.id NOT IN ( + SELECT MIN(ub.id) + FROM user_badges ub + GROUP BY ub.user_id, ub.badge_id, ub.seq + ) + " + + add_index :user_badges, [:badge_id, :user_id, :seq], unique: true, where: 'post_id IS NULL' + end + + def down + remove_column :user_badges, :seq, :integer + end +end diff --git a/spec/services/badge_granter_spec.rb b/spec/services/badge_granter_spec.rb index 426c877c3..c2d4cb5ca 100644 --- a/spec/services/badge_granter_spec.rb +++ b/spec/services/badge_granter_spec.rb @@ -50,9 +50,13 @@ describe BadgeGranter do describe 'grant' do - it 'grants a badge' do + it 'grants multiple badges' do + badge = Fabricate(:badge, multiple_grant: true) + user_badge = BadgeGranter.grant(badge, user) user_badge = BadgeGranter.grant(badge, user) user_badge.should be_present + + UserBadge.where(user_id: user.id).count.should == 2 end it 'sets granted_at' do