Commit b2fb3a9c authored by Felipe Artur's avatar Felipe Artur

Address review comments

parent 16a3fea3
......@@ -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
......@@ -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
......
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment