kotovalexarian-likes-gitlab
/
gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

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
This commit is contained in:
Jarka Košanová 2019-01-14 11:46:39 +01:00
parent 66b20a66f4
commit 30ab6ee416
19 changed files with 187 additions and 73 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
app/models/concerns/issuable.rb
View File

@ -74,6 +74,7 @@ module Issuable
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 @@ module Issuable
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

7
app/models/merge_request.rb
View File

@ -189,6 +189,9 @@ class MergeRequest < ActiveRecord::Base
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 @@ class MergeRequest < ActiveRecord::Base
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

6
app/services/issuable_base_service.rb
View File

@ -387,4 +387,10 @@ class IssuableBaseService < BaseService
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

4
app/services/issues/build_service.rb
View File

@ -6,7 +6,9 @@ module Issues
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

1
app/services/merge_requests/build_service.rb
View File

@ -19,6 +19,7 @@ module MergeRequests
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

5
changelogs/unreleased/51971-milestones-visibility.yml Normal file
View File

@ -0,0 +1,5 @@
---
title: Check if desired milestone for an issue is available
merge_request:
author:
type: security

2
spec/controllers/dashboard/milestones_controller_spec.rb
View File

@ -15,7 +15,7 @@ describe Dashboard::MilestonesController do
)
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]) }

4
spec/features/issues_spec.rb
View File

@ -233,8 +233,8 @@ describe 'Issues' do
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)

4
spec/features/merge_requests/user_lists_merge_requests_spec.rb
View File

@ -13,7 +13,7 @@ describe 'Merge requests > User lists merge requests' do
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 @@ describe 'Merge requests > User lists merge requests' do
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,

97
spec/models/concerns/issuable_spec.rb
View File

@ -32,17 +32,56 @@ describe Issuable do
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 @@ describe Issuable do
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') }

6
spec/models/issue/metrics_spec.rb
View File

@ -9,7 +9,7 @@ describe Issue::Metrics do
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 @@ describe Issue::Metrics do
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

8
spec/models/milestone_spec.rb
View File

@ -182,7 +182,7 @@ describe Milestone do
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 Milestone do
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

2
spec/requests/api/issues_spec.rb
View File

@ -49,7 +49,7 @@ describe API::Issues do
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

4
spec/services/issuable/common_system_notes_service_spec.rb
View File

@ -3,7 +3,7 @@ require 'spec_helper'
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 @@ describe Issuable::CommonSystemNotesService do
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) }

76
spec/services/issues/build_service_spec.rb
View File

@ -8,29 +8,29 @@ describe Issues::BuildService do
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) }
subject { build_issue(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
expect(issue.title).to include('Hello world')
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 @@ describe Issues::BuildService do
end
describe '#execute' do
it 'has the merge request reference in the title' do
expect(issue.title).to include(merge_request.title)
let(:base_params) { { merge_request_to_resolve_discussions_of: merge_request.iid } }
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 'has the reference of the merge request in the description' do
expect(issue.description).to include(merge_request.to_reference)
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 @@ describe Issues::BuildService do
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 Issues::BuildService do
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 @@ describe Issues::BuildService do
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
milestone = create(:milestone, project: project)
issue = build_issue(milestone_id: milestone.id)
expect(issue.title).to eq('Issue #1')
expect(issue.description).to eq('Issue description')
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

6
spec/services/issues/update_service_spec.rb
View File

@ -356,7 +356,7 @@ describe Issues::UpdateService, :mailer do
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 @@ describe Issues::UpdateService, :mailer do
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 @@ describe Issues::UpdateService, :mailer do
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)

9
spec/services/merge_requests/build_service_spec.rb
View File

@ -229,6 +229,15 @@ describe MergeRequests::BuildService do
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

6
spec/services/merge_requests/update_service_spec.rb
View File

@ -328,7 +328,7 @@ describe MergeRequests::UpdateService, :mailer do
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 @@ describe MergeRequests::UpdateService, :mailer do
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 @@ describe MergeRequests::UpdateService, :mailer do
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)

2
spec/support/shared_examples/issuable_shared_examples.rb
View File

@ -31,7 +31,7 @@ shared_examples 'system notes for milestones' do
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