Merge issuable "reopened" state into "opened"
Having two states that essentially mean the same thing is very much like having a boolean "true" and boolean "mostly-true": it's rather silly. This commit merges the "reopened" state into the "opened" state while taking care of system notes still showing messages along the lines of "Alice reopened this issue". A big benefit from having only two states (opened and closed) is that indexing and querying becomes simpler and more performant. For example, to get all the opened queries we no longer have to query both states: SELECT * FROM issues WHERE project_id = 2 AND state IN ('opened', 'reopened'); Instead we can query a single state directly, which can be much faster: SELECT * FROM issues WHERE project_id = 2 AND state = 'opened'; Further, only having two states makes indexing easier as we will only ever filter (and thus scan an index) using a single value. Partial indexes could help but aren't supported on MySQL, complicating the development process and not being helpful for MySQL.
This commit is contained in:
parent
8626cdc3b3
commit
6ef87a2083
|
@ -65,7 +65,7 @@ export default class MergeRequestStore {
|
|||
this.mergeCheckPath = data.merge_check_path;
|
||||
this.mergeActionsContentPath = data.commit_change_content_path;
|
||||
this.isRemovingSourceBranch = this.isRemovingSourceBranch || false;
|
||||
this.isOpen = data.state === 'opened' || data.state === 'reopened' || false;
|
||||
this.isOpen = data.state === 'opened';
|
||||
this.hasMergeableDiscussionsState = data.mergeable_discussions_state === false;
|
||||
this.canRemoveSourceBranch = currentUser.can_remove_source_branch || false;
|
||||
this.canMerge = !!data.merge_path;
|
||||
|
|
|
@ -81,7 +81,6 @@ class IssuableFinder
|
|||
end
|
||||
|
||||
counts[:all] = counts.values.sum
|
||||
counts[:opened] += counts[:reopened]
|
||||
|
||||
counts
|
||||
end
|
||||
|
|
|
@ -71,9 +71,8 @@ module Issuable
|
|||
scope :of_projects, ->(ids) { where(project_id: ids) }
|
||||
scope :of_milestones, ->(ids) { where(milestone_id: ids) }
|
||||
scope :with_milestone, ->(title) { left_joins_milestones.where(milestones: { title: title }) }
|
||||
scope :opened, -> { with_state(:opened, :reopened) }
|
||||
scope :opened, -> { with_state(:opened) }
|
||||
scope :only_opened, -> { with_state(:opened) }
|
||||
scope :only_reopened, -> { with_state(:reopened) }
|
||||
scope :closed, -> { with_state(:closed) }
|
||||
|
||||
scope :left_joins_milestones, -> { joins("LEFT OUTER JOIN milestones ON #{table_name}.milestone_id = milestones.id") }
|
||||
|
@ -234,7 +233,7 @@ module Issuable
|
|||
end
|
||||
|
||||
def open?
|
||||
opened? || reopened?
|
||||
opened?
|
||||
end
|
||||
|
||||
def user_notes_count
|
||||
|
|
|
@ -62,15 +62,14 @@ class Issue < ActiveRecord::Base
|
|||
|
||||
state_machine :state, initial: :opened do
|
||||
event :close do
|
||||
transition [:reopened, :opened] => :closed
|
||||
transition [:opened] => :closed
|
||||
end
|
||||
|
||||
event :reopen do
|
||||
transition closed: :reopened
|
||||
transition closed: :opened
|
||||
end
|
||||
|
||||
state :opened
|
||||
state :reopened
|
||||
state :closed
|
||||
|
||||
before_transition any => :closed do |issue|
|
||||
|
|
|
@ -42,23 +42,23 @@ class MergeRequest < ActiveRecord::Base
|
|||
|
||||
state_machine :state, initial: :opened do
|
||||
event :close do
|
||||
transition [:reopened, :opened] => :closed
|
||||
transition [:opened] => :closed
|
||||
end
|
||||
|
||||
event :mark_as_merged do
|
||||
transition [:reopened, :opened, :locked] => :merged
|
||||
transition [:opened, :locked] => :merged
|
||||
end
|
||||
|
||||
event :reopen do
|
||||
transition closed: :reopened
|
||||
transition closed: :opened
|
||||
end
|
||||
|
||||
event :lock_mr do
|
||||
transition [:reopened, :opened] => :locked
|
||||
transition [:opened] => :locked
|
||||
end
|
||||
|
||||
event :unlock_mr do
|
||||
transition locked: :reopened
|
||||
transition locked: :opened
|
||||
end
|
||||
|
||||
after_transition any => :locked do |merge_request, transition|
|
||||
|
@ -72,7 +72,6 @@ class MergeRequest < ActiveRecord::Base
|
|||
end
|
||||
|
||||
state :opened
|
||||
state :reopened
|
||||
state :closed
|
||||
state :merged
|
||||
state :locked
|
||||
|
@ -368,7 +367,7 @@ class MergeRequest < ActiveRecord::Base
|
|||
errors.add :branch_conflict, "You can not use same project/branch for source and target"
|
||||
end
|
||||
|
||||
if opened? || reopened?
|
||||
if opened?
|
||||
similar_mrs = self.target_project.merge_requests.where(source_branch: source_branch, target_branch: target_branch, source_project_id: source_project.try(:id)).opened
|
||||
similar_mrs = similar_mrs.where('id not in (?)', self.id) if self.id
|
||||
if similar_mrs.any?
|
||||
|
|
|
@ -114,7 +114,7 @@ class DroneCiService < CiService
|
|||
end
|
||||
|
||||
def merge_request_valid?(data)
|
||||
%w(opened reopened).include?(data[:object_attributes][:state]) &&
|
||||
data[:object_attributes][:state] == 'opened' &&
|
||||
data[:object_attributes][:merge_status] == 'unchecked'
|
||||
end
|
||||
end
|
||||
|
|
|
@ -5,7 +5,7 @@ module Issues
|
|||
|
||||
if issue.reopen
|
||||
event_service.reopen_issue(issue, current_user)
|
||||
create_note(issue)
|
||||
create_note(issue, 'reopened')
|
||||
notification_service.reopen_issue(issue, current_user)
|
||||
execute_hooks(issue, 'reopen')
|
||||
invalidate_cache_counts(issue, users: issue.assignees)
|
||||
|
@ -16,8 +16,8 @@ module Issues
|
|||
|
||||
private
|
||||
|
||||
def create_note(issue)
|
||||
SystemNoteService.change_status(issue, issue.project, current_user, issue.state, nil)
|
||||
def create_note(issue, state = issue.state)
|
||||
SystemNoteService.change_status(issue, issue.project, current_user, state, nil)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -1,7 +1,7 @@
|
|||
module MergeRequests
|
||||
class BaseService < ::IssuableBaseService
|
||||
def create_note(merge_request)
|
||||
SystemNoteService.change_status(merge_request, merge_request.target_project, current_user, merge_request.state, nil)
|
||||
def create_note(merge_request, state = merge_request.state)
|
||||
SystemNoteService.change_status(merge_request, merge_request.target_project, current_user, state, nil)
|
||||
end
|
||||
|
||||
def create_title_change_note(issuable, old_title)
|
||||
|
@ -44,7 +44,7 @@ module MergeRequests
|
|||
end
|
||||
|
||||
# Returns all origin and fork merge requests from `@project` satisfying passed arguments.
|
||||
def merge_requests_for(source_branch, mr_states: [:opened, :reopened])
|
||||
def merge_requests_for(source_branch, mr_states: [:opened])
|
||||
MergeRequest
|
||||
.with_state(mr_states)
|
||||
.where(source_branch: source_branch, source_project_id: @project.id)
|
||||
|
|
|
@ -5,7 +5,7 @@ module MergeRequests
|
|||
|
||||
if merge_request.reopen
|
||||
event_service.reopen_mr(merge_request, current_user)
|
||||
create_note(merge_request)
|
||||
create_note(merge_request, 'reopened')
|
||||
notification_service.reopen_mr(merge_request, current_user)
|
||||
execute_hooks(merge_request, 'reopen')
|
||||
merge_request.reload_diff(current_user)
|
||||
|
|
|
@ -0,0 +1,4 @@
|
|||
---
|
||||
title: Merge issuable "reopened" state into "opened"
|
||||
merge_request:
|
||||
author:
|
|
@ -0,0 +1,32 @@
|
|||
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
|
||||
# for more information on how to write migrations for GitLab.
|
||||
|
||||
class MergeIssuableReopenedIntoOpenedState < ActiveRecord::Migration
|
||||
include Gitlab::Database::MigrationHelpers
|
||||
|
||||
DOWNTIME = false
|
||||
|
||||
disable_ddl_transaction!
|
||||
|
||||
class Issue < ActiveRecord::Base
|
||||
self.table_name = 'issues'
|
||||
|
||||
include EachBatch
|
||||
end
|
||||
|
||||
class MergeRequest < ActiveRecord::Base
|
||||
self.table_name = 'merge_requests'
|
||||
|
||||
include EachBatch
|
||||
end
|
||||
|
||||
def up
|
||||
[Issue, MergeRequest].each do |model|
|
||||
say "Changing #{model.table_name}.state from 'reopened' to 'opened'"
|
||||
|
||||
model.where(state: 'reopened').each_batch do |batch|
|
||||
batch.update_all(state: 'opened')
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
|
@ -16,12 +16,8 @@ FactoryGirl.define do
|
|||
state :closed
|
||||
end
|
||||
|
||||
trait :reopened do
|
||||
state :reopened
|
||||
end
|
||||
|
||||
factory :closed_issue, traits: [:closed]
|
||||
factory :reopened_issue, traits: [:reopened]
|
||||
factory :reopened_issue, traits: [:opened]
|
||||
|
||||
factory :labeled_issue do
|
||||
transient do
|
||||
|
|
|
@ -44,10 +44,6 @@ FactoryGirl.define do
|
|||
state :opened
|
||||
end
|
||||
|
||||
trait :reopened do
|
||||
state :reopened
|
||||
end
|
||||
|
||||
trait :locked do
|
||||
state :locked
|
||||
end
|
||||
|
@ -74,7 +70,7 @@ FactoryGirl.define do
|
|||
|
||||
factory :merged_merge_request, traits: [:merged]
|
||||
factory :closed_merge_request, traits: [:closed]
|
||||
factory :reopened_merge_request, traits: [:reopened]
|
||||
factory :reopened_merge_request, traits: [:opened]
|
||||
factory :merge_request_with_diffs, traits: [:with_diffs]
|
||||
factory :merge_request_with_diff_notes do
|
||||
after(:create) do |mr|
|
||||
|
|
|
@ -107,14 +107,6 @@ describe Banzai::Filter::IssuableStateFilter do
|
|||
expect(doc.css('a').last.text).to eq(issue.to_reference)
|
||||
end
|
||||
|
||||
it 'ignores reopened issue references' do
|
||||
issue = create_issue(:reopened)
|
||||
link = create_link(issue.to_reference, issue: issue.id, reference_type: 'issue')
|
||||
doc = filter(link, context)
|
||||
|
||||
expect(doc.css('a').last.text).to eq(issue.to_reference)
|
||||
end
|
||||
|
||||
it 'appends state to closed issue references' do
|
||||
link = create_link(closed_issue.to_reference, issue: closed_issue.id, reference_type: 'issue')
|
||||
doc = filter(link, context)
|
||||
|
@ -139,7 +131,7 @@ describe Banzai::Filter::IssuableStateFilter do
|
|||
end
|
||||
|
||||
it 'ignores reopened merge request references' do
|
||||
merge_request = create_merge_request(:reopened)
|
||||
merge_request = create_merge_request(:opened)
|
||||
|
||||
link = create_link(
|
||||
merge_request.to_reference,
|
||||
|
|
|
@ -1259,7 +1259,7 @@ describe API::Issues do
|
|||
put api("/projects/#{project.id}/issues/#{closed_issue.iid}", user), state_event: 'reopen'
|
||||
|
||||
expect(response).to have_http_status(200)
|
||||
expect(json_response['state']).to eq 'reopened'
|
||||
expect(json_response['state']).to eq 'opened'
|
||||
end
|
||||
|
||||
context 'when an admin or owner makes the request' do
|
||||
|
|
|
@ -1114,7 +1114,7 @@ describe API::V3::Issues do
|
|||
put v3_api("/projects/#{project.id}/issues/#{closed_issue.id}", user), state_event: 'reopen'
|
||||
|
||||
expect(response).to have_http_status(200)
|
||||
expect(json_response['state']).to eq 'reopened'
|
||||
expect(json_response['state']).to eq 'opened'
|
||||
end
|
||||
|
||||
context 'when an admin or owner makes the request' do
|
||||
|
|
|
@ -20,7 +20,7 @@ describe Boards::Issues::ListService do
|
|||
|
||||
let!(:opened_issue1) { create(:labeled_issue, project: project, labels: [bug]) }
|
||||
let!(:opened_issue2) { create(:labeled_issue, project: project, labels: [p2]) }
|
||||
let!(:reopened_issue1) { create(:issue, :reopened, project: project) }
|
||||
let!(:reopened_issue1) { create(:issue, :opened, project: project) }
|
||||
|
||||
let!(:list1_issue1) { create(:labeled_issue, project: project, labels: [p2, development]) }
|
||||
let!(:list1_issue2) { create(:labeled_issue, project: project, labels: [development]) }
|
||||
|
|
|
@ -73,7 +73,7 @@ describe Boards::Issues::MoveService do
|
|||
issue.reload
|
||||
|
||||
expect(issue.labels).to contain_exactly(bug, testing)
|
||||
expect(issue).to be_reopened
|
||||
expect(issue).to be_opened
|
||||
end
|
||||
end
|
||||
|
||||
|
|
|
@ -43,7 +43,7 @@ describe DeleteMergedBranchesService do
|
|||
context 'open merge requests' do
|
||||
it 'does not delete branches from open merge requests' do
|
||||
fork_link = create(:forked_project_link, forked_from_project: project)
|
||||
create(:merge_request, :reopened, source_project: project, target_project: project, source_branch: 'branch-merged', target_branch: 'master')
|
||||
create(:merge_request, :opened, source_project: project, target_project: project, source_branch: 'branch-merged', target_branch: 'master')
|
||||
create(:merge_request, :opened, source_project: fork_link.forked_to_project, target_project: project, target_branch: 'improve/awesome', source_branch: 'master')
|
||||
|
||||
service.execute
|
||||
|
|
|
@ -78,7 +78,7 @@ describe MergeRequests::GetUrlsService do
|
|||
end
|
||||
|
||||
context 'pushing to existing branch and merge request is reopened' do
|
||||
let!(:merge_request) { create(:merge_request, :reopened, source_project: project, source_branch: source_branch) }
|
||||
let!(:merge_request) { create(:merge_request, :opened, source_project: project, source_branch: source_branch) }
|
||||
let(:changes) { existing_branch_changes }
|
||||
it_behaves_like 'show_merge_request_url'
|
||||
end
|
||||
|
|
|
@ -28,7 +28,7 @@ describe MergeRequests::ReopenService do
|
|||
end
|
||||
|
||||
it { expect(merge_request).to be_valid }
|
||||
it { expect(merge_request).to be_reopened }
|
||||
it { expect(merge_request).to be_opened }
|
||||
|
||||
it 'executes hooks with reopen action' do
|
||||
expect(service).to have_received(:execute_hooks)
|
||||
|
|
Loading…
Reference in New Issue