Commit 9f3e8eb5 authored by Yorick Peterse's avatar Yorick Peterse

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.
parent c3b1ac85
......@@ -53,7 +53,7 @@ export default {
return this.state && this.state.length > 0;
},
isOpen() {
return this.state === 'opened' || this.state === 'reopened';
return this.state === 'opened';
},
isClosed() {
return this.state === 'closed';
......
......@@ -67,7 +67,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;
......
......@@ -85,7 +85,6 @@ class IssuableFinder
end
counts[:all] = counts.values.sum
counts[:opened] += counts[:reopened]
counts.with_indifferent_access
end
......
class Burndown
Issue = Struct.new(:closed_at, :weight, :state)
class Issue
attr_reader :closed_at, :weight, :state
def initialize(closed_at, weight, state)
@closed_at = closed_at
@weight = weight
@state = state
end
def reopened?
@state == 'opened' && @closed_at.present?
end
end
attr_reader :start_date, :due_date, :end_date, :issues_count, :issues_weight, :accurate, :legacy_data
alias_method :accurate?, :accurate
......@@ -12,8 +24,8 @@ class Burndown
@end_date = @milestone.due_date
@end_date = Date.today if @end_date.present? && @end_date > Date.today
@accurate = milestone_closed_issues.all?(&:closed_at)
@legacy_data = milestone_closed_issues.any? && milestone_closed_issues.none?(&:closed_at)
@accurate = milestone_issues.all?(&:closed_at)
@legacy_data = milestone_issues.any? && milestone_issues.none?(&:closed_at)
@issues_count, @issues_weight = milestone.issues.reorder(nil).pluck('COUNT(*), COALESCE(SUM(weight), 0)').first
end
......@@ -59,21 +71,21 @@ class Burndown
current_date = date.to_date
closed =
milestone_closed_issues.select do |issue|
milestone_issues.select do |issue|
(issue.closed_at&.to_date || start_date) == current_date
end
reopened = closed.select { |issue| issue.state == 'reopened' }
reopened = closed.select(&:reopened?)
[closed, reopened]
end
def milestone_closed_issues
@milestone_closed_issues ||=
def milestone_issues
@milestone_issues ||=
@milestone.issues
.where("state IN ('reopened', 'closed')")
.order("closed_at ASC")
.where("state = 'closed' OR (state = 'opened' AND closed_at IS NOT NULL)")
.reorder("closed_at ASC")
.pluck("closed_at, weight, state")
.map {|attrs| ::Burndown::Issue.new(*attrs) }
.map {|attrs| Issue.new(*attrs) }
end
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 :order_milestone_due_desc, -> { outer_join_milestone.reorder('milestones.due_date IS NULL ASC, milestones.due_date DESC, milestones.id DESC') }
scope :order_milestone_due_asc, -> { outer_join_milestone.reorder('milestones.due_date IS NULL ASC, milestones.due_date ASC, milestones.id ASC') }
......@@ -241,7 +240,7 @@ module Issuable
end
def open?
opened? || reopened?
opened?
end
def user_notes_count
......
......@@ -73,15 +73,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|
......
......@@ -45,23 +45,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|
......@@ -75,7 +75,6 @@ class MergeRequest < ActiveRecord::Base
end
state :opened
state :reopened
state :closed
state :merged
state :locked
......@@ -372,7 +371,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
......@@ -2,8 +2,8 @@ module MergeRequests
class BaseService < ::IssuableBaseService
prepend EE::MergeRequests::BaseService
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)
......@@ -46,7 +46,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)
......
......@@ -82,7 +82,7 @@ module MergeRequests
# Note: Closed merge requests also need approvals reset.
def reset_approvals_for_merge_requests
merge_requests = merge_requests_for(@branch_name, mr_states: [:opened, :reopened, :closed])
merge_requests = merge_requests_for(@branch_name, mr_states: [:opened, :closed])
merge_requests.each do |merge_request|
target_project = merge_request.target_project
......
......@@ -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)
......
---
title: Merge issuable "reopened" state into "opened"
merge_request:
author:
# 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
......@@ -17,12 +17,8 @@ FactoryGirl.define do
closed_at Time.now
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
......@@ -80,7 +76,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_approver, traits: [:with_approver]
factory :merge_request_with_diff_notes do
......
......@@ -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,
......
......@@ -63,14 +63,14 @@ describe Burndown do
expect(burndown).to be_accurate
end
context "when all closed and reopened issues does not have closed_at" do
context "when all closed issues does not have closed_at" do
before do
milestone.issues.update_all(closed_at: nil)
end
it "considers closed_at as milestone start date" do
expect(subject).to eq([
["2017-03-01", 15, 30],
["2017-03-01", 27, 54],
["2017-03-02", 27, 54],
["2017-03-03", 27, 54],
["2017-03-04", 27, 54],
......@@ -85,7 +85,7 @@ describe Burndown do
end
end
context "when one or more closed or reopened issues does not have closed_at" do
context "when one or more closed issues does not have closed_at" do
before do
milestone.issues.closed.first.update(closed_at: nil)
end
......
......@@ -1228,7 +1228,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
......
......@@ -1116,7 +1116,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
......
......@@ -23,7 +23,7 @@ describe Boards::Issues::ListService do
let!(:opened_issue1) { create(:labeled_issue, project: project, milestone: m1, title: 'Issue 1', labels: [bug]) }
let!(:opened_issue2) { create(:labeled_issue, project: project, milestone: m2, title: 'Issue 2', labels: [p2]) }
let!(:reopened_issue1) { create(:issue, :reopened, project: project, title: 'Issue 3' ) }
let!(:reopened_issue1) { create(:issue, :opened, project: project, title: 'Issue 3' ) }
let!(:list1_issue1) { create(:labeled_issue, project: project, milestone: m1, labels: [p2, development]) }
let!(:list1_issue2) { create(:labeled_issue, project: project, milestone: m2, 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
......
......@@ -35,7 +35,7 @@ describe Issues::ExportCsvService do
issue.update!(milestone: milestone,
assignees: [user],
description: 'Issue with details',
state: :reopened,
state: :opened,
due_date: DateTime.new(2014, 3, 2),
created_at: DateTime.new(2015, 4, 3, 2, 1, 0),
updated_at: DateTime.new(2016, 5, 4, 3, 2, 1),
......
......@@ -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
......
......@@ -86,7 +86,7 @@ describe MergeRequests::RefreshService do
let(:refresh_service) { service.new(@project, @user) }
before do
@merge_request.update(state: :reopened)
@merge_request.update(state: :opened)
allow(refresh_service).to receive(:execute_hooks)
refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
......
......@@ -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)
......
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