Address review comments

3 files changed, 14 insertions, 23 deletions

diff --git a/app/models/concerns/issuable_states.rb b/app/models/concerns/issuable_states.rb
index 2ebd5013a4c..b722c541580 100644
--- a/app/models/concerns/issuable_states.rb
+++ b/app/models/concerns/issuable_states.rb
@@ -17,11 +17,9 @@ module IssuableStates
     # Needed to prevent breaking some migration specs that
     # rollback database to a point where state_id does not exist.
     # We can use this guard clause for now since this file will
-    # be removed on a future the next release.
-    return unless self.respond_to?(:state_id)
+    # be removed in the next release.
+    return unless self.has_attribute?(:state_id)
 
-    states_hash = self.class.available_states
-
-    self.state_id = states_hash[state]
+    self.state_id = self.class.available_states[state]
   end
 end
diff --git a/lib/gitlab/background_migration/sync_issues_state_id.rb b/lib/gitlab/background_migration/sync_issues_state_id.rb
index 33b997c8533..33002f58be6 100644
--- a/lib/gitlab/background_migration/sync_issues_state_id.rb
+++ b/lib/gitlab/background_migration/sync_issues_state_id.rb
@@ -5,7 +5,7 @@ module Gitlab
   module BackgroundMigration
     class SyncIssuesStateId
       def perform(start_id, end_id)
-        Rails.logger.info("Issues - Populating state_id: #{start_id} - #{end_id}")
+        Gitlab::AppLogger.info("Issues - Populating state_id: #{start_id} - #{end_id}")
 
         ActiveRecord::Base.connection.execute <<~SQL
           UPDATE issues
diff --git a/spec/migrations/schedule_sync_issuables_state_id_spec.rb b/spec/migrations/schedule_sync_issuables_state_id_spec.rb
index 6b1ea804342..bf974d60b24 100644
--- a/spec/migrations/schedule_sync_issuables_state_id_spec.rb
+++ b/spec/migrations/schedule_sync_issuables_state_id_spec.rb
@@ -9,11 +9,8 @@ describe ScheduleSyncIssuablesStateId, :migration do
   let(:merge_requests) { table(:merge_requests) }
   let(:issues) { table(:issues) }
   let(:migration) { described_class.new }
-
-  before do
-    @group = namespaces.create!(name: 'gitlab', path: 'gitlab')
-    @project = projects.create!(namespace_id: @group.id)
-  end
+  let(:group) { namespaces.create!(name: 'gitlab', path: 'gitlab') }
+  let(:project) { projects.create!(namespace_id: group.id) }
 
   shared_examples 'scheduling migrations' do
     before do
@@ -40,14 +37,12 @@ describe ScheduleSyncIssuablesStateId, :migration do
         opened_issue = issues.create!(description: 'first', state: 'opened')
         closed_issue = issues.create!(description: 'second', state: 'closed')
         invalid_state_issue = issues.create!(description: 'fourth', state: 'not valid')
-        nil_state_issue = issues.create!(description: 'third', state: nil)
 
         migrate!
 
         expect(opened_issue.reload.state_id).to eq(Issue.available_states[:opened])
         expect(closed_issue.reload.state_id).to eq(Issue.available_states[:closed])
         expect(invalid_state_issue.reload.state_id).to be_nil
-        expect(nil_state_issue.reload.state_id).to be_nil
       end
 
       it_behaves_like 'scheduling migrations' do
@@ -61,11 +56,10 @@ describe ScheduleSyncIssuablesStateId, :migration do
 
     context 'merge requests' do
       it 'migrates state column to integer' do
-        opened_merge_request = merge_requests.create!(state: 'opened', target_project_id: @project.id, target_branch: 'feature1', source_branch: 'master')
-        closed_merge_request = merge_requests.create!(state: 'closed', target_project_id: @project.id, target_branch: 'feature2', source_branch: 'master')
-        merged_merge_request = merge_requests.create!(state: 'merged', target_project_id: @project.id, target_branch: 'feature3', source_branch: 'master')
-        locked_merge_request = merge_requests.create!(state: 'locked', target_project_id: @project.id, target_branch: 'feature4', source_branch: 'master')
-        invalid_state_merge_request = merge_requests.create!(state: 'not valid', target_project_id: @project.id, target_branch: 'feature5', source_branch: 'master')
+        opened_merge_request = merge_requests.create!(state: 'opened', target_project_id: project.id, target_branch: 'feature1', source_branch: 'master')
+        closed_merge_request = merge_requests.create!(state: 'closed', target_project_id: project.id, target_branch: 'feature2', source_branch: 'master')
+        merged_merge_request = merge_requests.create!(state: 'merged', target_project_id: project.id, target_branch: 'feature3', source_branch: 'master')
+        locked_merge_request = merge_requests.create!(state: 'locked', target_project_id: project.id, target_branch: 'feature4', source_branch: 'master')
 
         migrate!
 
@@ -73,15 +67,14 @@ describe ScheduleSyncIssuablesStateId, :migration do
         expect(closed_merge_request.reload.state_id).to eq(MergeRequest.available_states[:closed])
         expect(merged_merge_request.reload.state_id).to eq(MergeRequest.available_states[:merged])
         expect(locked_merge_request.reload.state_id).to eq(MergeRequest.available_states[:locked])
-        expect(invalid_state_merge_request.reload.state_id).to be_nil
       end
 
       it_behaves_like 'scheduling migrations' do
         let(:migration) { described_class::MERGE_REQUESTS_MIGRATION }
-        let!(:resource_1) { merge_requests.create!(state: 'opened', target_project_id: @project.id, target_branch: 'feature1', source_branch: 'master') }
-        let!(:resource_2) { merge_requests.create!(state: 'closed', target_project_id: @project.id, target_branch: 'feature2', source_branch: 'master') }
-        let!(:resource_3) { merge_requests.create!(state: 'merged', target_project_id: @project.id, target_branch: 'feature3', source_branch: 'master') }
-        let!(:resource_4) { merge_requests.create!(state: 'locked', target_project_id: @project.id, target_branch: 'feature4', source_branch: 'master') }
+        let!(:resource_1) { merge_requests.create!(state: 'opened', target_project_id: project.id, target_branch: 'feature1', source_branch: 'master') }
+        let!(:resource_2) { merge_requests.create!(state: 'closed', target_project_id: project.id, target_branch: 'feature2', source_branch: 'master') }
+        let!(:resource_3) { merge_requests.create!(state: 'merged', target_project_id: project.id, target_branch: 'feature3', source_branch: 'master') }
+        let!(:resource_4) { merge_requests.create!(state: 'locked', target_project_id: project.id, target_branch: 'feature4', source_branch: 'master') }
       end
     end
   end