GitLab steht wegen Wartungsarbeiten am Montag, den 10. Mai, zwischen 17:00 und 19:00 Uhr nicht zur Verfügung.

Commit 98df4053 authored by Jarka Košanová's avatar Jarka Košanová

Check issue milestone availability

Add project when creating milestone in specs

We validate milestone is from the same
project/parent group as issuable ->
we need to set project in specs correctly

Improve methods names and specs organization
parent 5c225467
......@@ -74,6 +74,7 @@ def award_emojis_loaded?
validates :author, presence: true
validates :title, presence: true, length: { maximum: 255 }
validate :milestone_is_valid
scope :authored, ->(user) { where(author_id: user) }
scope :recent, -> { reorder(id: :desc) }
......@@ -117,6 +118,16 @@ def allows_multiple_assignees?
def has_multiple_assignees?
assignees.count > 1
end
def milestone_available?
project_id == milestone&.project_id || project.ancestors_upto.compact.include?(milestone&.group)
end
private
def milestone_is_valid
errors.add(:milestone_id, message: "is invalid") if milestone_id.present? && !milestone_available?
end
end
class_methods do
......
......@@ -189,6 +189,9 @@ def check_state?(merge_status)
after_save :keep_around_commit
alias_attribute :project, :target_project
alias_attribute :project_id, :target_project_id
def self.reference_prefix
'!'
end
......@@ -837,10 +840,6 @@ def for_fork?
target_project != source_project
end
def project
target_project
end
# If the merge request closes any issues, save this information in the
# `MergeRequestsClosingIssues` model. This is a performance optimization.
# Calculating this information for a number of merge requests requires
......
......@@ -387,4 +387,10 @@ def update_project_counter_caches?(issuable)
def parent
project
end
# we need to check this because milestone from milestone_id param is displayed on "new" page
# where private project milestone could leak without this check
def ensure_milestone_available(issuable)
issuable.milestone_id = nil unless issuable.milestone_available?
end
end
......@@ -6,7 +6,9 @@ class BuildService < Issues::BaseService
def execute
filter_resolve_discussion_params
@issue = project.issues.new(issue_params)
@issue = project.issues.new(issue_params).tap do |issue|
ensure_milestone_available(issue)
end
end
def issue_params_with_info_from_discussions
......
......@@ -19,6 +19,7 @@ def execute
merge_request.target_project = find_target_project
merge_request.target_branch = find_target_branch
merge_request.can_be_created = projects_and_branches_valid?
ensure_milestone_available(merge_request)
# compare branches only if branches are valid, otherwise
# compare_branches may raise an error
......
---
title: Check if desired milestone for an issue is available
merge_request:
author:
type: security
......@@ -15,7 +15,7 @@
)
end
let(:issue) { create(:issue, project: project, milestone: project_milestone) }
let(:group_issue) { create(:issue, milestone: group_milestone) }
let(:group_issue) { create(:issue, milestone: group_milestone, project: create(:project, group: group)) }
let!(:label) { create(:label, project: project, title: 'Issue Label', issues: [issue]) }
let!(:group_label) { create(:group_label, group: group, title: 'Group Issue Label', issues: [group_issue]) }
......
......@@ -233,8 +233,8 @@
created_at: Time.now - (index * 60))
end
end
let(:newer_due_milestone) { create(:milestone, due_date: '2013-12-11') }
let(:later_due_milestone) { create(:milestone, due_date: '2013-12-12') }
let(:newer_due_milestone) { create(:milestone, project: project, due_date: '2013-12-11') }
let(:later_due_milestone) { create(:milestone, project: project, due_date: '2013-12-12') }
it 'sorts by newest' do
visit project_issues_path(project, sort: sort_value_created_date)
......
......@@ -13,7 +13,7 @@
source_project: project,
source_branch: 'fix',
assignee: user,
milestone: create(:milestone, due_date: '2013-12-11'),
milestone: create(:milestone, project: project, due_date: '2013-12-11'),
created_at: 1.minute.ago,
updated_at: 1.minute.ago)
create(:merge_request,
......@@ -21,7 +21,7 @@
source_project: project,
source_branch: 'markdown',
assignee: user,
milestone: create(:milestone, due_date: '2013-12-12'),
milestone: create(:milestone, project: project, due_date: '2013-12-12'),
created_at: 2.minutes.ago,
updated_at: 2.minutes.ago)
create(:merge_request,
......
......@@ -32,17 +32,56 @@
end
describe "Validation" do
subject { build(:issue) }
context 'general validations' do
subject { build(:issue) }
before do
allow(InternalId).to receive(:generate_next).and_return(nil)
before do
allow(InternalId).to receive(:generate_next).and_return(nil)
end
it { is_expected.to validate_presence_of(:project) }
it { is_expected.to validate_presence_of(:iid) }
it { is_expected.to validate_presence_of(:author) }
it { is_expected.to validate_presence_of(:title) }
it { is_expected.to validate_length_of(:title).is_at_most(255) }
end
it { is_expected.to validate_presence_of(:project) }
it { is_expected.to validate_presence_of(:iid) }
it { is_expected.to validate_presence_of(:author) }
it { is_expected.to validate_presence_of(:title) }
it { is_expected.to validate_length_of(:title).is_at_most(255) }
describe 'milestone' do
let(:project) { create(:project) }
let(:milestone_id) { create(:milestone, project: project).id }
let(:params) do
{
title: 'something',
project: project,
author: build(:user),
milestone_id: milestone_id
}
end
subject { issuable_class.new(params) }
context 'with correct params' do
it { is_expected.to be_valid }
end
context 'with empty string milestone' do
let(:milestone_id) { '' }
it { is_expected.to be_valid }
end
context 'with nil milestone id' do
let(:milestone_id) { nil }
it { is_expected.to be_valid }
end
context 'with a milestone id from another project' do
let(:milestone_id) { create(:milestone).id }
it { is_expected.to be_invalid }
end
end
end
describe "Scope" do
......@@ -66,6 +105,48 @@
end
end
describe '#milestone_available?' do
let(:group) { create(:group) }
let(:project) { create(:project, group: group) }
let(:issue) { create(:issue, project: project) }
def build_issuable(milestone_id)
issuable_class.new(project: project, milestone_id: milestone_id)
end
it 'returns true with a milestone from the issue project' do
milestone = create(:milestone, project: project)
expect(build_issuable(milestone.id).milestone_available?).to be_truthy
end
it 'returns true with a milestone from the issue project group' do
milestone = create(:milestone, group: group)
expect(build_issuable(milestone.id).milestone_available?).to be_truthy
end
it 'returns true with a milestone from the the parent of the issue project group', :nested_groups do
parent = create(:group)
group.update(parent: parent)
milestone = create(:milestone, group: parent)
expect(build_issuable(milestone.id).milestone_available?).to be_truthy
end
it 'returns false with a milestone from another project' do
milestone = create(:milestone)
expect(build_issuable(milestone.id).milestone_available?).to be_falsey
end
it 'returns false with a milestone from another group' do
milestone = create(:milestone, group: create(:group))
expect(build_issuable(milestone.id).milestone_available?).to be_falsey
end
end
describe ".search" do
let!(:searchable_issue) { create(:issue, title: "Searchable awesome issue") }
let!(:searchable_issue2) { create(:issue, title: 'Aw') }
......
......@@ -9,7 +9,7 @@
context "milestones" do
it "records the first time an issue is associated with a milestone" do
time = Time.now
Timecop.freeze(time) { subject.update(milestone: create(:milestone)) }
Timecop.freeze(time) { subject.update(milestone: create(:milestone, project: project)) }
metrics = subject.metrics
expect(metrics).to be_present
......@@ -18,9 +18,9 @@
it "does not record the second time an issue is associated with a milestone" do
time = Time.now
Timecop.freeze(time) { subject.update(milestone: create(:milestone)) }
Timecop.freeze(time) { subject.update(milestone: create(:milestone, project: project)) }
Timecop.freeze(time + 2.hours) { subject.update(milestone: nil) }
Timecop.freeze(time + 6.hours) { subject.update(milestone: create(:milestone)) }
Timecop.freeze(time + 6.hours) { subject.update(milestone: create(:milestone, project: project)) }
metrics = subject.metrics
expect(metrics).to be_present
......
......@@ -182,7 +182,7 @@
describe '#total_items_count' do
before do
create :closed_issue, milestone: milestone, project: project
create :merge_request, milestone: milestone
create :merge_request, milestone: milestone, source_project: project
end
it 'returns total count of issues and merge requests assigned to milestone' do
......@@ -192,10 +192,10 @@
describe '#can_be_closed?' do
before do
milestone = create :milestone
create :closed_issue, milestone: milestone
milestone = create :milestone, project: project
create :closed_issue, milestone: milestone, project: project
create :issue
create :issue, project: project
end
it 'returns true if milestone active and all nested issues closed' do
......
......@@ -49,7 +49,7 @@
create(:label, title: 'label', color: '#FFAABB', project: project)
end
let!(:label_link) { create(:label_link, label: label, target: issue) }
set(:milestone) { create(:milestone, title: '1.0.0', project: project) }
let(:milestone) { create(:milestone, title: '1.0.0', project: project) }
set(:empty_milestone) do
create(:milestone, title: '2.0.0', project: project)
end
......
......@@ -3,7 +3,7 @@
describe Issuable::CommonSystemNotesService do
let(:user) { create(:user) }
let(:project) { create(:project) }
let(:issuable) { create(:issue) }
let(:issuable) { create(:issue, project: project) }
context 'on issuable update' do
it_behaves_like 'system note creation', { title: 'New title' }, 'changed title'
......@@ -70,7 +70,7 @@
end
context 'on issuable create' do
let(:issuable) { build(:issue) }
let(:issuable) { build(:issue, project: project) }
subject { described_class.new(project, user).execute(issuable, old_labels: [], is_update: false) }
......
......@@ -8,29 +8,29 @@
project.add_developer(user)
end
def build_issue(issue_params = {})
described_class.new(project, user, issue_params).execute
end
context 'for a single discussion' do
describe '#execute' do
let(:merge_request) { create(:merge_request, title: "Hello world", source_project: project) }
let(:discussion) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, note: "Almost done").to_discussion }
let(:service) { described_class.new(project, user, merge_request_to_resolve_discussions_of: merge_request.iid, discussion_to_resolve: discussion.id) }
it 'references the noteable title in the issue title' do
issue = service.execute
subject { build_issue(merge_request_to_resolve_discussions_of: merge_request.iid, discussion_to_resolve: discussion.id) }
expect(issue.title).to include('Hello world')
it 'references the noteable title in the issue title' do
expect(subject.title).to include('Hello world')
end
it 'adds the note content to the description' do
issue = service.execute
expect(issue.description).to include('Almost done')
expect(subject.description).to include('Almost done')
end
end
end
context 'for discussions in a merge request' do
let(:merge_request) { create(:merge_request_with_diff_notes, source_project: project) }
let(:issue) { described_class.new(project, user, merge_request_to_resolve_discussions_of: merge_request.iid).execute }
describe '#items_for_discussions' do
it 'has an item for each discussion' do
......@@ -66,28 +66,30 @@
end
describe '#execute' do
it 'has the merge request reference in the title' do
expect(issue.title).to include(merge_request.title)
end
let(:base_params) { { merge_request_to_resolve_discussions_of: merge_request.iid } }
it 'has the reference of the merge request in the description' do
expect(issue.description).to include(merge_request.to_reference)
context 'without additional params' do
subject { build_issue(base_params) }
it 'has the merge request reference in the title' do
expect(subject.title).to include(merge_request.title)
end
it 'has the reference of the merge request in the description' do
expect(subject.description).to include(merge_request.to_reference)
end
end
it 'does not assign title when a title was given' do
issue = described_class.new(project, user,
merge_request_to_resolve_discussions_of: merge_request,
title: 'What an issue').execute
it 'uses provided title if title param given' do
issue = build_issue(base_params.merge(title: 'What an issue'))
expect(issue.title).to eq('What an issue')
end
it 'does not assign description when a description was given' do
issue = described_class.new(project, user,
merge_request_to_resolve_discussions_of: merge_request,
description: 'Fix at your earliest conveignance').execute
it 'uses provided description if description param given' do
issue = build_issue(base_params.merge(description: 'Fix at your earliest convenience'))
expect(issue.description).to eq('Fix at your earliest conveignance')
expect(issue.description).to eq('Fix at your earliest convenience')
end
describe 'with multiple discussions' do
......@@ -96,20 +98,20 @@
it 'mentions all the authors in the description' do
authors = merge_request.resolvable_discussions.map(&:author)
expect(issue.description).to include(*authors.map(&:to_reference))
expect(build_issue(base_params).description).to include(*authors.map(&:to_reference))
end
it 'has a link for each unresolved discussion in the description' do
notes = merge_request.resolvable_discussions.map(&:first_note)
links = notes.map { |note| Gitlab::UrlBuilder.build(note) }
expect(issue.description).to include(*links)
expect(build_issue(base_params).description).to include(*links)
end
it 'mentions additional notes' do
create_list(:diff_note_on_merge_request, 2, noteable: merge_request, project: merge_request.target_project, in_reply_to: diff_note)
expect(issue.description).to include('(+2 comments)')
expect(build_issue(base_params).description).to include('(+2 comments)')
end
end
end
......@@ -120,7 +122,7 @@
describe '#execute' do
it 'mentions the merge request in the description' do
issue = described_class.new(project, user, merge_request_to_resolve_discussions_of: merge_request.iid).execute
issue = build_issue(merge_request_to_resolve_discussions_of: merge_request.iid)
expect(issue.description).to include("Review the conversation in #{merge_request.to_reference}")
end
......@@ -128,20 +130,18 @@
end
describe '#execute' do
let(:milestone) { create(:milestone, project: project) }
it 'builds a new issues with given params' do
issue = described_class.new(
project,
user,
title: 'Issue #1',
description: 'Issue description',
milestone_id: milestone.id
).execute
expect(issue.title).to eq('Issue #1')
expect(issue.description).to eq('Issue description')
milestone = create(:milestone, project: project)
issue = build_issue(milestone_id: milestone.id)
expect(issue.milestone).to eq(milestone)
end
it 'sets milestone to nil if it is not available for the project' do
milestone = create(:milestone, project: create(:project))
issue = build_issue(milestone_id: milestone.id)
expect(issue.milestone).to be_nil
end
end
end
......@@ -356,7 +356,7 @@ def update_issue(opts)
it_behaves_like 'system notes for milestones'
it 'sends notifications for subscribers of changed milestone' do
issue.milestone = create(:milestone)
issue.milestone = create(:milestone, project: project)
issue.save
......@@ -380,7 +380,7 @@ def update_issue(opts)
end
it 'marks todos as done' do
update_issue(milestone: create(:milestone))
update_issue(milestone: create(:milestone, project: project))
expect(todo.reload.done?).to eq true
end
......@@ -389,7 +389,7 @@ def update_issue(opts)
it 'sends notifications for subscribers of changed milestone' do
perform_enqueued_jobs do
update_issue(milestone: create(:milestone))
update_issue(milestone: create(:milestone, project: project))
end
should_email(subscriber)
......
......@@ -229,6 +229,15 @@ def stub_compare
end
end
end
context 'when a milestone is from another project' do
let(:milestone) { create(:milestone, project: create(:project)) }
let(:milestone_id) { milestone.id }
it 'sets milestone to nil' do
expect(merge_request.milestone).to be_nil
end
end
end
end
......
......@@ -328,7 +328,7 @@ def update_merge_request(opts)
it_behaves_like 'system notes for milestones'
it 'sends notifications for subscribers of changed milestone' do
merge_request.milestone = create(:milestone)
merge_request.milestone = create(:milestone, project: project)
merge_request.save
......@@ -352,7 +352,7 @@ def update_merge_request(opts)
end
it 'marks pending todos as done' do
update_merge_request({ milestone: create(:milestone) })
update_merge_request({ milestone: create(:milestone, project: project) })
expect(pending_todo.reload).to be_done
end
......@@ -361,7 +361,7 @@ def update_merge_request(opts)
it 'sends notifications for subscribers of changed milestone' do
perform_enqueued_jobs do
update_merge_request(milestone: create(:milestone))
update_merge_request(milestone: create(:milestone, project: project))
end
should_email(subscriber)
......
......@@ -31,7 +31,7 @@ def update_issuable(opts)
context 'project milestones' do
it 'creates a system note' do
expect do
update_issuable(milestone: create(:milestone))
update_issuable(milestone: create(:milestone, project: project))
end.to change { Note.system.count }.by(1)
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