DEV: Remove badge_granted_title column from user_profiles (#20476)

That column is obsolete since we added the `granted_title_badge_id` column in 2019 (56d3e29a698e704f647cf2e500d0d94d7d9e21bf). Having both columns can lead to inconsistencies (mostly due to old data from before 2019).

For example, `BadgeGranter.revoke_ungranted_titles!` doesn't work correctly if `badge_granted_title` is `false` while `granted_title_badge_id` points to the badge that is used as title.

app/models/badge.rb

@@ -190,7 +190,7 @@ class Badge < ActiveRecord::Base
     SQL
     DB.exec(<<~SQL, badge_id: self.id)
       UPDATE user_profiles AS up
-      SET badge_granted_title = false, granted_title_badge_id = NULL
+      SET granted_title_badge_id = NULL
       WHERE up.granted_title_badge_id = :badge_id
     SQL
   end

app/models/user.rb

@@ -2130,10 +2130,7 @@ class User < ActiveRecord::Base
           badges.find do |badge|
             badge.allow_title? && (badge.display_name == title || badge.name == title)
           end
-      user_profile.update(
+      user_profile.update!(granted_title_badge_id: badge_matching_title&.id)
-        badge_granted_title: badge_matching_title.present?,
-        granted_title_badge_id: badge_matching_title&.id,
-      )
     end
   end
 

app/models/user_profile.rb

@@ -1,6 +1,9 @@
 # frozen_string_literal: true
 
 class UserProfile < ActiveRecord::Base
+  # TODO Remove `badge_granted_title` after 2023-09-01
+  self.ignored_columns = ["badge_granted_title"]
+
   BAKED_VERSION = 1
 
   belongs_to :user, inverse_of: :user_profile
@@ -227,7 +230,6 @@ end
 #  bio_cooked                   :text
 #  dismissed_banner_key         :integer
 #  bio_cooked_version           :integer
-#  badge_granted_title          :boolean          default(FALSE)
 #  views                        :integer          default(0), not null
 #  profile_background_upload_id :integer
 #  card_background_upload_id    :integer

app/services/badge_granter.rb

@@ -500,7 +500,6 @@ class BadgeGranter
       WHERE u.title IS NOT NULL
         AND u.title <> ''
         AND up.user_id = u.id
-        AND up.badge_granted_title
         AND up.granted_title_badge_id IS NOT NULL
         AND NOT EXISTS(
           SELECT 1
@@ -514,12 +513,11 @@ class BadgeGranter
 
     DB.exec <<~SQL
       UPDATE user_profiles up
-      SET badge_granted_title    = FALSE,
+      SET granted_title_badge_id = NULL
-          granted_title_badge_id = NULL
       FROM users u
       WHERE up.user_id = u.id
         AND (u.title IS NULL OR u.title = '')
-        AND (up.badge_granted_title OR up.granted_title_badge_id IS NOT NULL)
+        AND up.granted_title_badge_id IS NOT NULL
     SQL
   end
 

app/services/user_merger.rb

@@ -286,7 +286,7 @@ class UserMerger
         bio_cooked_version   = COALESCE(t.bio_cooked_version, s.bio_cooked_version),
         profile_background_upload_id = COALESCE(t.profile_background_upload_id, s.profile_background_upload_id),
         dismissed_banner_key = COALESCE(t.dismissed_banner_key, s.dismissed_banner_key),
-        badge_granted_title  = t.badge_granted_title OR s.badge_granted_title,
+        granted_title_badge_id = COALESCE(t.granted_title_badge_id, s.granted_title_badge_id),
         card_background_upload_id = COALESCE(t.card_background_upload_id, s.card_background_upload_id),
         views                = t.views + s.views
       FROM user_profiles AS s

db/post_migrate/20230228105851_drop_badge_granted_title_column.rb

@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+class DropBadgeGrantedTitleColumn < ActiveRecord::Migration[7.0]
+  DROPPED_COLUMNS ||= { user_profiles: %i[badge_granted_title] }
+
+  def up
+    DROPPED_COLUMNS.each { |table, columns| Migration::ColumnDropper.execute_drop(table, columns) }
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

spec/models/user_spec.rb

@@ -2664,24 +2664,22 @@ RSpec.describe User do
   describe "#title=" do
     fab!(:badge) { Fabricate(:badge, name: "Badge", allow_title: false) }
 
-    it "sets badge_granted_title correctly" do
+    it "sets granted_title_badge_id correctly" do
       BadgeGranter.grant(badge, user)
 
       user.update!(title: badge.name)
-      expect(user.user_profile.reload.badge_granted_title).to eq(false)
+      expect(user.user_profile.reload.granted_title_badge_id).to be_nil
 
       user.update!(title: "Custom")
-      expect(user.user_profile.reload.badge_granted_title).to eq(false)
+      expect(user.user_profile.reload.granted_title_badge_id).to be_nil
 
       badge.update!(allow_title: true)
       user.badges.reload
       user.update!(title: badge.name)
-      expect(user.user_profile.reload.badge_granted_title).to eq(true)
       expect(user.user_profile.reload.granted_title_badge_id).to eq(badge.id)
 
       user.update!(title: nil)
-      expect(user.user_profile.reload.badge_granted_title).to eq(false)
+      expect(user.user_profile.granted_title_badge_id).to be_nil
-      expect(user.user_profile.granted_title_badge_id).to eq(nil)
     end
 
     context "when a custom badge name has been set and it matches the title" do
@@ -2691,12 +2689,11 @@ RSpec.describe User do
         TranslationOverride.upsert!(I18n.locale, Badge.i18n_key(badge.name), customized_badge_name)
       end
 
-      it "sets badge_granted_title correctly" do
+      it "sets granted_title_badge_id correctly" do
         BadgeGranter.grant(badge, user)
 
         badge.update!(allow_title: true)
         user.update!(title: customized_badge_name)
-        expect(user.user_profile.reload.badge_granted_title).to eq(true)
         expect(user.user_profile.reload.granted_title_badge_id).to eq(badge.id)
       end
 

spec/requests/users_controller_spec.rb

@@ -2996,7 +2996,6 @@ RSpec.describe UsersController do
           }
 
       expect(user1.reload.title).to eq(badge.display_name)
-      expect(user1.user_profile.badge_granted_title).to eq(true)
       expect(user1.user_profile.granted_title_badge_id).to eq(badge.id)
 
       badge.update allow_title: false
@@ -3009,7 +3008,6 @@ RSpec.describe UsersController do
       user1.reload
       user1.user_profile.reload
       expect(user1.title).to eq("")
-      expect(user1.user_profile.badge_granted_title).to eq(false)
       expect(user1.user_profile.granted_title_badge_id).to eq(nil)
     end
 

spec/services/badge_granter_spec.rb

@@ -22,14 +22,12 @@ RSpec.describe BadgeGranter do
       BadgeGranter.revoke_ungranted_titles!
       user.reload
       expect(user.title).to eq(badge.name)
-      expect(user.user_profile.badge_granted_title).to eq(true)
       expect(user.user_profile.granted_title_badge_id).to eq(badge.id)
 
       badge.update_column(:allow_title, false)
       BadgeGranter.revoke_ungranted_titles!
       user.reload
       expect(user.title).to be_blank
-      expect(user.user_profile.badge_granted_title).to eq(false)
       expect(user.user_profile.granted_title_badge_id).to be_nil
     end
 
@@ -40,14 +38,12 @@ RSpec.describe BadgeGranter do
       BadgeGranter.revoke_ungranted_titles!
       user.reload
       expect(user.title).to eq(badge.name)
-      expect(user.user_profile.badge_granted_title).to eq(true)
       expect(user.user_profile.granted_title_badge_id).to eq(badge.id)
 
       badge.update_column(:enabled, false)
       BadgeGranter.revoke_ungranted_titles!
       user.reload
       expect(user.title).to be_blank
-      expect(user.user_profile.badge_granted_title).to eq(false)
       expect(user.user_profile.granted_title_badge_id).to be_nil
     end
 
@@ -58,14 +54,12 @@ RSpec.describe BadgeGranter do
       BadgeGranter.revoke_ungranted_titles!
       user.reload
       expect(user.title).to eq(badge.name)
-      expect(user.user_profile.badge_granted_title).to eq(true)
       expect(user.user_profile.granted_title_badge_id).to eq(badge.id)
 
       BadgeGranter.revoke(user.user_badges.first)
       BadgeGranter.revoke_ungranted_titles!
       user.reload
       expect(user.title).to be_blank
-      expect(user.user_profile.badge_granted_title).to eq(false)
       expect(user.user_profile.granted_title_badge_id).to be_nil
     end
 
@@ -91,13 +85,11 @@ RSpec.describe BadgeGranter do
 
       user.reload
       expect(user.title).to eq(badge_name)
-      expect(user.user_profile.badge_granted_title).to eq(true)
       expect(user.user_profile.granted_title_badge_id).to eq(badge.id)
 
       BadgeGranter.revoke_ungranted_titles!
       user.reload
       expect(user.title).to eq(badge_name)
-      expect(user.user_profile.badge_granted_title).to eq(true)
       expect(user.user_profile.granted_title_badge_id).to eq(badge.id)
     end
   end

spec/services/user_updater_spec.rb

@@ -456,12 +456,12 @@ RSpec.describe UserUpdater do
       context "when badge can be used as a title" do
         before { badge.update(allow_title: true) }
 
-        it "can use as title, sets badge_granted_title" do
+        it "can use as title, sets granted_title_badge_id" do
           BadgeGranter.grant(badge, user)
           updater = UserUpdater.new(user, user)
           updater.update(title: badge.name)
           user.reload
-          expect(user.user_profile.badge_granted_title).to eq(true)
+          expect(user.user_profile.granted_title_badge_id).to eq(badge.id)
         end
 
         it "badge has not been granted, does not change title" do
@@ -470,12 +470,12 @@ RSpec.describe UserUpdater do
           updater.update(title: badge.name)
           user.reload
           expect(user.title).not_to eq(badge.name)
-          expect(user.user_profile.badge_granted_title).to eq(false)
+          expect(user.user_profile.granted_title_badge_id).to be_nil
         end
 
-        it "changing to a title that is not from a badge, unsets badge_granted_title" do
+        it "changing to a title that is not from a badge, unsets granted_title_badge_id" do
           user.update(title: badge.name)
-          user.user_profile.update(badge_granted_title: true)
+          user.user_profile.update(granted_title_badge_id: badge.id)
 
           Guardian.any_instance.stubs(:can_grant_title?).with(user, "Dancer").returns(true)
 
@@ -483,7 +483,7 @@ RSpec.describe UserUpdater do
           updater.update(title: "Dancer")
           user.reload
           expect(user.title).to eq("Dancer")
-          expect(user.user_profile.badge_granted_title).to eq(false)
+          expect(user.user_profile.granted_title_badge_id).to be_nil
         end
       end
 
@@ -493,7 +493,7 @@ RSpec.describe UserUpdater do
         updater.update(title: badge.name)
         user.reload
         expect(user.title).not_to eq(badge.name)
-        expect(user.user_profile.badge_granted_title).to eq(false)
+        expect(user.user_profile.granted_title_badge_id).to be_nil
       end
     end