Move "everyone" role and "instance actor" account magic number IDs to constants (#29260)

app/models/account.rb

@@ -64,6 +64,7 @@ class Account < ApplicationRecord
   )
 
   BACKGROUND_REFRESH_INTERVAL = 1.week.freeze
+  INSTANCE_ACTOR_ID = -99
 
   USERNAME_RE   = /[a-z0-9_]+([a-z0-9_.-]+[a-z0-9_]+)?/i
   MENTION_RE    = %r{(?<![=/[:word:]])@((#{USERNAME_RE})(?:@[[:word:].-]+[[:word:]]+)?)}i
@@ -118,7 +119,7 @@ class Account < ApplicationRecord
   scope :sensitized, -> { where.not(sensitized_at: nil) }
   scope :without_suspended, -> { where(suspended_at: nil) }
   scope :without_silenced, -> { where(silenced_at: nil) }
-  scope :without_instance_actor, -> { where.not(id: -99) }
+  scope :without_instance_actor, -> { where.not(id: INSTANCE_ACTOR_ID) }
   scope :recent, -> { reorder(id: :desc) }
   scope :bots, -> { where(actor_type: %w(Application Service)) }
   scope :groups, -> { where(actor_type: 'Group') }
@@ -176,7 +177,7 @@ class Account < ApplicationRecord
   end
 
   def instance_actor?
-    id == -99
+    id == INSTANCE_ACTOR_ID
   end
 
   alias bot bot?

app/models/concerns/account/finder_concern.rb

@@ -13,11 +13,11 @@ module Account::FinderConcern
     end
 
     def representative
-      actor = Account.find(-99).tap(&:ensure_keys!)
+      actor = Account.find(Account::INSTANCE_ACTOR_ID).tap(&:ensure_keys!)
       actor.update!(username: 'mastodon.internal') if actor.username.include?(':')
       actor
     rescue ActiveRecord::RecordNotFound
-      Account.create!(id: -99, actor_type: 'Application', locked: true, username: 'mastodon.internal')
+      Account.create!(id: Account::INSTANCE_ACTOR_ID, actor_type: 'Application', locked: true, username: 'mastodon.internal')
     end
 
     def find_local(username)

app/models/user_role.rb

@@ -38,6 +38,8 @@ class UserRole < ApplicationRecord
     delete_user_data: (1 << 19),
   }.freeze
 
+  EVERYONE_ROLE_ID = -99
+
   module Flags
     NONE = 0
     ALL  = FLAGS.values.reduce(&:|)
@@ -94,7 +96,7 @@ class UserRole < ApplicationRecord
 
   before_validation :set_position
 
-  scope :assignable, -> { where.not(id: -99).order(position: :asc) }
+  scope :assignable, -> { where.not(id: EVERYONE_ROLE_ID).order(position: :asc) }
 
   has_many :users, inverse_of: :role, foreign_key: 'role_id', dependent: :nullify
 
@@ -103,9 +105,9 @@ class UserRole < ApplicationRecord
   end
 
   def self.everyone
-    UserRole.find(-99)
+    UserRole.find(EVERYONE_ROLE_ID)
   rescue ActiveRecord::RecordNotFound
-    UserRole.create!(id: -99, permissions: Flags::DEFAULT)
+    UserRole.create!(id: EVERYONE_ROLE_ID, permissions: Flags::DEFAULT)
   end
 
   def self.that_can(*any_of_privileges)
@@ -113,7 +115,7 @@ class UserRole < ApplicationRecord
   end
 
   def everyone?
-    id == -99
+    id == EVERYONE_ROLE_ID
   end
 
   def nobody?

db/migrate/20190715164535_add_instance_actor.rb

@@ -5,6 +5,8 @@ class AddInstanceActor < ActiveRecord::Migration[5.2]
     # Dummy class, to make migration possible across version changes
     validates :username, uniqueness: { scope: :domain, case_sensitive: false }
 
+    INSTANCE_ACTOR_ID = -99
+
     before_create :generate_keys
 
     def generate_keys
@@ -15,10 +17,10 @@ class AddInstanceActor < ActiveRecord::Migration[5.2]
   end
 
   def up
-    Account.create!(id: -99, actor_type: 'Application', locked: true, username: Rails.configuration.x.local_domain)
+    Account.create!(id: Account::INSTANCE_ACTOR_ID, actor_type: 'Application', locked: true, username: Rails.configuration.x.local_domain)
   end
 
   def down
-    Account.find_by(id: -99, actor_type: 'Application').destroy!
+    Account.find_by(id: Account::INSTANCE_ACTOR_ID, actor_type: 'Application').destroy!
   end
 end

db/post_migrate/20220704024901_migrate_settings_to_user_roles.rb

@@ -3,7 +3,9 @@
 class MigrateSettingsToUserRoles < ActiveRecord::Migration[6.1]
   disable_ddl_transaction!
 
-  class UserRole < ApplicationRecord; end
+  class UserRole < ApplicationRecord
+    EVERYONE_ROLE_ID = -99
+  end
 
   def up
     process_role_everyone
@@ -17,7 +19,7 @@ class MigrateSettingsToUserRoles < ActiveRecord::Migration[6.1]
   private
 
   def process_role_everyone
-    everyone_role = UserRole.find_by(id: -99)
+    everyone_role = UserRole.find_by(id: UserRole::EVERYONE_ROLE_ID)
     return unless everyone_role
 
     everyone_role.permissions &= ~::UserRole::FLAGS[:invite_users] unless min_invite_role == 'user'

db/seeds/02_instance_actor.rb

@@ -1,3 +1,3 @@
 # frozen_string_literal: true
 
-Account.create_with(actor_type: 'Application', locked: true, username: 'mastodon.internal').find_or_create_by(id: -99)
+Account.create_with(actor_type: 'Application', locked: true, username: 'mastodon.internal').find_or_create_by(id: Account::INSTANCE_ACTOR_ID)

lib/tasks/tests.rake

@@ -50,7 +50,7 @@ namespace :tests do
         exit(1)
       end
 
-      if Account.find(-99).private_key.blank?
+      if Account.find(Account::INSTANCE_ACTOR_ID).private_key.blank?
         puts 'Instance actor does not have a private key'
         exit(1)
       end

spec/models/account_spec.rb

@@ -746,13 +746,13 @@ RSpec.describe Account do
       end
 
       it 'is valid if we are creating an instance actor account with a period' do
-        account = Fabricate.build(:account, id: -99, actor_type: 'Application', locked: true, username: 'example.com')
+        account = Fabricate.build(:account, id: described_class::INSTANCE_ACTOR_ID, actor_type: 'Application', locked: true, username: 'example.com')
         expect(account.valid?).to be true
       end
 
       it 'is valid if we are creating a possibly-conflicting instance actor account' do
         _account = Fabricate(:account, username: 'examplecom')
-        instance_account = Fabricate.build(:account, id: -99, actor_type: 'Application', locked: true, username: 'example.com')
+        instance_account = Fabricate.build(:account, id: described_class::INSTANCE_ACTOR_ID, actor_type: 'Application', locked: true, username: 'example.com')
         expect(instance_account.valid?).to be true
       end
 

spec/models/user_role_spec.rb

@@ -164,12 +164,12 @@ RSpec.describe UserRole do
   end
 
   describe '#everyone?' do
-    it 'returns true when id is -99' do
+    it 'returns true when id matches the everyone id' do
-      subject.id = -99
+      subject.id = described_class::EVERYONE_ROLE_ID
       expect(subject.everyone?).to be true
     end
 
-    it 'returns false when id is not -99' do
+    it 'returns false when id does not match the everyone id' do
       subject.id = 123
       expect(subject.everyone?).to be false
     end