Address review comments.

6 files changed, 22 insertions, 19 deletions

diff --git a/app/models/concerns/atomic_internal_id.rb b/app/models/concerns/atomic_internal_id.rb
index 343edc237c9..6895c7d7e95 100644
--- a/app/models/concerns/atomic_internal_id.rb
+++ b/app/models/concerns/atomic_internal_id.rb
@@ -1,6 +1,6 @@
 # Include atomic internal id generation scheme for a model
 #
-# This allows to atomically generate internal ids that are
+# This allows us to atomically generate internal ids that are
 # unique within a given scope.
 #
 # For example, let's generate internal ids for Issue per Project:
@@ -25,18 +25,18 @@ module AtomicInternalId
   extend ActiveSupport::Concern
 
   module ClassMethods
-    def has_internal_id(on, scope:, usage: nil, init:) # rubocop:disable Naming/PredicateName
+    def has_internal_id(column, scope:, init:) # rubocop:disable Naming/PredicateName
       before_validation(on: :create) do
-        if self.public_send(on).blank? # rubocop:disable GitlabSecurity/PublicSend
+        if self.public_send(column).blank? # rubocop:disable GitlabSecurity/PublicSend
           scope_attrs = { scope => self.public_send(scope) } # rubocop:disable GitlabSecurity/PublicSend
-          usage = (usage || self.class.table_name).to_sym
+          usage = self.class.table_name.to_sym
 
           new_iid = InternalId.generate_next(self, scope_attrs, usage, init)
-          self.public_send("#{on}=", new_iid) # rubocop:disable GitlabSecurity/PublicSend
+          self.public_send("#{column}=", new_iid) # rubocop:disable GitlabSecurity/PublicSend
         end
       end
 
-      validates on, presence: true, numericality: true
+      validates column, presence: true, numericality: true
     end
   end
 
diff --git a/app/models/internal_id.rb b/app/models/internal_id.rb
index f3630ed1ac5..cbec735c2dd 100644
--- a/app/models/internal_id.rb
+++ b/app/models/internal_id.rb
@@ -66,6 +66,7 @@ class InternalId < ActiveRecord::Base
     # scope: Attributes that define the scope for id generation.
     # usage: Symbol to define the usage of the internal id, see InternalId.usages
     # init: Block that gets called to initialize InternalId record if not present
+    #       Make sure to not throw exceptions in the absence of records (if this is expected).
     attr_reader :subject, :scope, :init, :scope_attrs, :usage
 
     def initialize(subject, scope, usage, init)
@@ -74,9 +75,9 @@ class InternalId < ActiveRecord::Base
       @init = init
       @usage = usage
 
-      raise ArgumentError, 'scope is not well-defined, need at least one column for scope (given: 0)' if scope.empty?
+      raise ArgumentError, 'Scope is not well-defined, need at least one column for scope (given: 0)' if scope.empty?
 
-      unless InternalId.usages.keys.include?(usage.to_s)
+      unless InternalId.usages.has_key?(usage.to_s)
         raise ArgumentError, "Usage '#{usage}' is unknown. Supported values are #{InternalId.usages.keys} from InternalId.usages"
       end
     end
@@ -85,7 +86,7 @@ class InternalId < ActiveRecord::Base
     def generate
       subject.transaction do
         # Create a record in internal_ids if one does not yet exist
-        # and increment it's last value
+        # and increment its last value
         #
         # Note this will acquire a ROW SHARE lock on the InternalId record
         (lookup || create_record).increment_and_save!
diff --git a/config/initializers/ar_native_database_types.rb b/config/initializers/ar_native_database_types.rb
index d7b4b348957..3522b1db536 100644
--- a/config/initializers/ar_native_database_types.rb
+++ b/config/initializers/ar_native_database_types.rb
@@ -4,7 +4,7 @@ module ActiveRecord
   module ConnectionAdapters
     class AbstractMysqlAdapter
       NATIVE_DATABASE_TYPES.merge!(
-        bigserial:  { name: 'bigint(20) auto_increment PRIMARY KEY' }
+        bigserial: { name: 'bigint(20) auto_increment PRIMARY KEY' }
       )
     end
   end
diff --git a/db/migrate/20180305095250_create_internal_ids_table.rb b/db/migrate/20180305095250_create_internal_ids_table.rb
index e972432fb98..432086fe98b 100644
--- a/db/migrate/20180305095250_create_internal_ids_table.rb
+++ b/db/migrate/20180305095250_create_internal_ids_table.rb
@@ -3,7 +3,7 @@ class CreateInternalIdsTable < ActiveRecord::Migration
 
   DOWNTIME = false
 
-  def up
+  def change
     create_table :internal_ids, id: :bigserial do |t|
       t.references :project, null: false, foreign_key: { on_delete: :cascade }
       t.integer :usage, null: false
@@ -12,8 +12,4 @@ class CreateInternalIdsTable < ActiveRecord::Migration
       t.index [:usage, :project_id], unique: true
     end
   end
-
-  def down
-    drop_table :internal_ids
-  end
 end
diff --git a/spec/models/internal_id_spec.rb b/spec/models/internal_id_spec.rb
index ef6db2daa95..40d777c46cc 100644
--- a/spec/models/internal_id_spec.rb
+++ b/spec/models/internal_id_spec.rb
@@ -41,10 +41,11 @@ describe InternalId do
     end
 
     it 'generates a strictly monotone, gapless sequence' do
-      seq = (0..rand(1000)).map do
+      seq = (0..rand(100)).map do
         described_class.generate_next(issue, scope, usage, init)
       end
       normalized = seq.map { |i| i - seq.min }
+
       expect(normalized).to eq((0..seq.size - 1).to_a)
     end
 
@@ -58,6 +59,7 @@ describe InternalId do
 
       it 'calculates next internal ids on the fly' do
         val = rand(1..100)
+
         expect(init).to receive(:call).with(issue).and_return(val)
         expect(subject).to eq(val + 1)
       end
@@ -70,11 +72,13 @@ describe InternalId do
 
     it 'returns incremented iid' do
       value = id.last_value
+
       expect(subject).to eq(value + 1)
     end
 
     it 'saves the record' do
       subject
+
       expect(id.changed?).to be_falsey
     end
 
diff --git a/spec/support/shared_examples/models/atomic_internal_id_spec.rb b/spec/support/shared_examples/models/atomic_internal_id_spec.rb
index 671aa314bd6..144af4fc475 100644
--- a/spec/support/shared_examples/models/atomic_internal_id_spec.rb
+++ b/spec/support/shared_examples/models/atomic_internal_id_spec.rb
@@ -24,14 +24,16 @@ shared_examples_for 'AtomicInternalId' do
 
       it 'calls InternalId.generate_next and sets internal id attribute' do
         iid = rand(1..1000)
+
         expect(InternalId).to receive(:generate_next).with(instance, scope_attrs, usage, any_args).and_return(iid)
         subject
-        expect(instance.public_send(internal_id_attribute)).to eq(iid) # rubocop:disable GitlabSecurity/PublicSend
+        expect(instance.public_send(internal_id_attribute)).to eq(iid)
       end
 
       it 'does not overwrite an existing internal id' do
-        instance.public_send("#{internal_id_attribute}=", 4711) # rubocop:disable GitlabSecurity/PublicSend
-        expect { subject }.not_to change { instance.public_send(internal_id_attribute) } # rubocop:disable GitlabSecurity/PublicSend
+        instance.public_send("#{internal_id_attribute}=", 4711)
+
+        expect { subject }.not_to change { instance.public_send(internal_id_attribute) }
       end
     end
   end