diff --git a/CHANGELOG b/CHANGELOG index 5dc56d18c3a01ca30de4c2e039b617b7166ca694..160543d8bae93e1d074825a2758abe56ccd85f33 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -2,6 +2,7 @@ Please view this file on the master branch, on stable branches it's out of date. v 8.9.0 (unreleased) - Allow forking projects with restricted visibility level + - Improve note validation to prevent errors when creating invalid note via API - Redesign navigation for project pages - Fix groups API to list only user's accessible projects - Redesign account and email confirmation emails diff --git a/app/models/note.rb b/app/models/note.rb index 55b98557244aaab7fdc5534f01c60bdb09b57ed6..5f669c02e8ba155c5fcd9176edec0350089ed582 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -29,10 +29,17 @@ class Note < ActiveRecord::Base # Attachments are deprecated and are handled by Markdown uploader validates :attachment, file_size: { maximum: :max_attachment_size } - validates :noteable_id, presence: true, if: ->(n) { n.noteable_type.present? && n.noteable_type != 'Commit' } - validates :commit_id, presence: true, if: ->(n) { n.noteable_type == 'Commit' } + validates :noteable_type, presence: true + validates :noteable_id, presence: true, unless: :for_commit? + validates :commit_id, presence: true, if: :for_commit? validates :author, presence: true + validate unless: :for_commit? do |note| + unless note.noteable.try(:project) == note.project + errors.add(:invalid_project, 'Note and noteable project mismatch') + end + end + mount_uploader :attachment, AttachmentUploader # Scopes diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb index 01586994813cca5dcca3fe8f628c058d256425dd..2bb312bb252ca4096d0b9c22180bca22fcea42e5 100644 --- a/app/services/notes/create_service.rb +++ b/app/services/notes/create_service.rb @@ -5,8 +5,6 @@ module Notes note.author = current_user note.system = false - return unless valid_project?(note) - if note.save # Finish the harder work in the background NewNoteWorker.perform_in(2.seconds, note.id, params) @@ -15,14 +13,5 @@ module Notes note end - - private - - def valid_project?(note) - return false unless project - return true if note.for_commit? - - note.noteable.try(:project) == project - end end end diff --git a/features/steps/dashboard/todos.rb b/features/steps/dashboard/todos.rb index d3b6c7f6a15c6b25901a5b7153ce1e7021f131a8..bd8a270202ee750368e12ad4fef0e73908982257 100644 --- a/features/steps/dashboard/todos.rb +++ b/features/steps/dashboard/todos.rb @@ -20,7 +20,7 @@ class Spinach::Features::DashboardTodos < Spinach::FeatureSteps step 'I have todos' do create(:todo, user: current_user, project: project, author: mary_jane, target: issue, action: Todo::MENTIONED) create(:todo, user: current_user, project: project, author: john_doe, target: issue, action: Todo::ASSIGNED) - note = create(:note, author: john_doe, noteable: issue, note: "#{current_user.to_reference} Wdyt?") + note = create(:note, author: john_doe, noteable: issue, note: "#{current_user.to_reference} Wdyt?", project: project) create(:todo, user: current_user, project: project, author: john_doe, target: issue, action: Todo::MENTIONED, note: note) create(:todo, user: current_user, project: project, author: john_doe, target: merge_request, action: Todo::ASSIGNED) end diff --git a/features/steps/project/issues/issues.rb b/features/steps/project/issues/issues.rb index f2c68f007ef0d3d79e52500f97b509c3c3747d0f..5cd431e05d58025f8a7a92ac7bc106b212921a66 100644 --- a/features/steps/project/issues/issues.rb +++ b/features/steps/project/issues/issues.rb @@ -348,7 +348,7 @@ class Spinach::Features::ProjectIssues < Spinach::FeatureSteps step 'another user adds a comment with text "Yay!" to issue "Release 0.4"' do issue = Issue.find_by!(title: 'Release 0.4') - create(:note_on_issue, noteable: issue, note: 'Yay!') + create(:note_on_issue, noteable: issue, project: project, note: 'Yay!') end step 'I should see a new comment with text "Yay!"' do diff --git a/features/steps/project/merge_requests.rb b/features/steps/project/merge_requests.rb index cfa586f859f6832d03fbf87e6f7f042efb42e5e5..b2fa8750234fd44858ab2407e6eb20c092811539 100644 --- a/features/steps/project/merge_requests.rb +++ b/features/steps/project/merge_requests.rb @@ -273,7 +273,7 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps step 'user "John Doe" leaves a comment like "Line is wrong" on diff' do mr = MergeRequest.find_by(title: "Bug NS-05") create(:note_on_merge_request_diff, project: project, - noteable_id: mr.id, + noteable: mr, author: user_exists("John Doe"), line_code: sample_commit.line_code, note: 'Line is wrong') diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb index 26719f2652c277f2ab4b7c0fff0b0a957a62d2bc..c32e205ee69bb2e7b0d95361da43a7c5ee23bbf4 100644 --- a/spec/factories/notes.rb +++ b/spec/factories/notes.rb @@ -7,6 +7,7 @@ FactoryGirl.define do project note "Note" author + on_issue factory :note_on_commit, traits: [:on_commit] factory :note_on_commit_diff, traits: [:on_commit, :on_diff], class: LegacyDiffNote @@ -19,29 +20,26 @@ FactoryGirl.define do factory :upvote_note, traits: [:award, :upvote] trait :on_commit do - project + noteable nil + noteable_id nil + noteable_type 'Commit' commit_id RepoHelpers.sample_commit.id - noteable_type "Commit" end trait :on_diff do line_code "0_184_184" end - trait :on_merge_request do - project - noteable_id 1 - noteable_type "MergeRequest" + trait :on_issue do + noteable { create(:issue, project: project) } end - trait :on_issue do - noteable_id 1 - noteable_type "Issue" + trait :on_merge_request do + noteable { create(:merge_request, source_project: project) } end trait :on_project_snippet do - noteable_id 1 - noteable_type "Snippet" + noteable { create(:snippet, project: project) } end trait :system do diff --git a/spec/features/issues/note_polling_spec.rb b/spec/features/issues/note_polling_spec.rb index e4efdbe2421d0dfc5838dcba1bac30cb705c4e2b..f5cfe2d666ef93e3b1bc3f8a3887cfae7d621256 100644 --- a/spec/features/issues/note_polling_spec.rb +++ b/spec/features/issues/note_polling_spec.rb @@ -9,8 +9,11 @@ feature 'Issue notes polling' do end scenario 'Another user adds a comment to an issue', js: true do - note = create(:note_on_issue, noteable: issue, note: 'Looks good!') + note = create(:note, noteable: issue, project: project, + note: 'Looks good!') + page.execute_script('notes.refresh();') + expect(page).to have_selector("#note_#{note.id}", text: 'Looks good!') end end diff --git a/spec/features/issues_spec.rb b/spec/features/issues_spec.rb index 749ee01890c9e234dd5a7d1ff5aec4ee3535a4b8..9271964166a7dd8bc2a4b403f3a16ee3f8ef4c0d 100644 --- a/spec/features/issues_spec.rb +++ b/spec/features/issues_spec.rb @@ -125,7 +125,7 @@ describe 'Issues', feature: true do describe 'Issue info' do it 'excludes award_emoji from comment count' do issue = create(:issue, author: @user, assignee: @user, project: project, title: 'foobar') - create(:upvote_note, noteable: issue) + create(:upvote_note, noteable: issue, project: project) visit namespace_project_issues_path(project.namespace, project, assignee_id: @user.id) diff --git a/spec/features/notes_on_merge_requests_spec.rb b/spec/features/notes_on_merge_requests_spec.rb index 9e9fec01943368eff2ed82ca8473b50bf01110d5..2835cf444949202088e2d14eda353014f6749b47 100644 --- a/spec/features/notes_on_merge_requests_spec.rb +++ b/spec/features/notes_on_merge_requests_spec.rb @@ -19,10 +19,14 @@ describe 'Comments', feature: true do end describe 'On a merge request', js: true, feature: true do - let!(:merge_request) { create(:merge_request) } - let!(:project) { merge_request.source_project } + let!(:project) { create(:project) } + let!(:merge_request) do + create(:merge_request, source_project: project, target_project: project) + end + let!(:note) do - create(:note_on_merge_request, :with_attachment, project: project) + create(:note_on_merge_request, :with_attachment, noteable: merge_request, + project: project) end before do diff --git a/spec/features/participants_autocomplete_spec.rb b/spec/features/participants_autocomplete_spec.rb index 1adab7e9c6c3ef66f70eed0ecf83d0f081b8359b..c7c00a3266a32cfe5bd3e84301e7861b75d1e64f 100644 --- a/spec/features/participants_autocomplete_spec.rb +++ b/spec/features/participants_autocomplete_spec.rb @@ -32,7 +32,8 @@ feature 'Member autocomplete', feature: true do context 'adding a new note on a Issue', js: true do before do issue = create(:issue, author: author, project: project) - create(:note, note: 'Ultralight Beam', noteable: issue, author: participant) + create(:note, note: 'Ultralight Beam', noteable: issue, + project: project, author: participant) visit_issue(project, issue) end @@ -47,7 +48,8 @@ feature 'Member autocomplete', feature: true do context 'adding a new note on a Merge Request ', js: true do before do merge = create(:merge_request, source_project: project, target_project: project, author: author) - create(:note, note: 'Ultralight Beam', noteable: merge, author: participant) + create(:note, note: 'Ultralight Beam', noteable: merge, + project: project, author: participant) visit_merge_request(project, merge) end diff --git a/spec/features/task_lists_spec.rb b/spec/features/task_lists_spec.rb index b7368cca29d0cdb3dbfcca4f7910718866e10cc7..6ed279ef9be6f3413b683b6c793e72cf7ca6e2ad 100644 --- a/spec/features/task_lists_spec.rb +++ b/spec/features/task_lists_spec.rb @@ -75,7 +75,10 @@ feature 'Task Lists', feature: true do describe 'for Notes' do let!(:issue) { create(:issue, author: user, project: project) } - let!(:note) { create(:note, note: markdown, noteable: issue, author: user) } + let!(:note) do + create(:note, note: markdown, noteable: issue, + project: project, author: user) + end it 'renders for note body' do visit_issue(project, issue) diff --git a/spec/lib/gitlab/note_data_builder_spec.rb b/spec/lib/gitlab/note_data_builder_spec.rb index f093d0a0d8b3a7dd5b481841cf7e92d68abdc783..e848d88182fb07b030684e283ce80211d7563cd4 100644 --- a/spec/lib/gitlab/note_data_builder_spec.rb +++ b/spec/lib/gitlab/note_data_builder_spec.rb @@ -9,7 +9,8 @@ describe 'Gitlab::NoteDataBuilder', lib: true do before(:each) do expect(data).to have_key(:object_attributes) expect(data[:object_attributes]).to have_key(:url) - expect(data[:object_attributes][:url]).to eq(Gitlab::UrlBuilder.build(note)) + expect(data[:object_attributes][:url]) + .to eq(Gitlab::UrlBuilder.build(note)) expect(data[:object_kind]).to eq('note') expect(data[:user]).to eq(user.hook_attrs) end @@ -37,13 +38,21 @@ describe 'Gitlab::NoteDataBuilder', lib: true do end describe 'When asking for a note on issue' do - let(:issue) { create(:issue, created_at: fixed_time, updated_at: fixed_time) } - let(:note) { create(:note_on_issue, noteable_id: issue.id, project: project) } + let(:issue) do + create(:issue, created_at: fixed_time, updated_at: fixed_time, + project: project) + end + + let(:note) do + create(:note_on_issue, noteable: issue, project: project) + end it 'returns the note and issue-specific data' do expect(data).to have_key(:issue) - expect(data[:issue].except('updated_at')).to eq(issue.hook_attrs.except('updated_at')) - expect(data[:issue]['updated_at']).to be > issue.hook_attrs['updated_at'] + expect(data[:issue].except('updated_at')) + .to eq(issue.reload.hook_attrs.except('updated_at')) + expect(data[:issue]['updated_at']) + .to be > issue.hook_attrs['updated_at'] end include_examples 'project hook data' @@ -51,13 +60,23 @@ describe 'Gitlab::NoteDataBuilder', lib: true do end describe 'When asking for a note on merge request' do - let(:merge_request) { create(:merge_request, created_at: fixed_time, updated_at: fixed_time) } - let(:note) { create(:note_on_merge_request, noteable_id: merge_request.id, project: project) } + let(:merge_request) do + create(:merge_request, created_at: fixed_time, + updated_at: fixed_time, + source_project: project) + end + + let(:note) do + create(:note_on_merge_request, noteable: merge_request, + project: project) + end it 'returns the note and merge request data' do expect(data).to have_key(:merge_request) - expect(data[:merge_request].except('updated_at')).to eq(merge_request.hook_attrs.except('updated_at')) - expect(data[:merge_request]['updated_at']).to be > merge_request.hook_attrs['updated_at'] + expect(data[:merge_request].except('updated_at')) + .to eq(merge_request.reload.hook_attrs.except('updated_at')) + expect(data[:merge_request]['updated_at']) + .to be > merge_request.hook_attrs['updated_at'] end include_examples 'project hook data' @@ -65,13 +84,22 @@ describe 'Gitlab::NoteDataBuilder', lib: true do end describe 'When asking for a note on merge request diff' do - let(:merge_request) { create(:merge_request, created_at: fixed_time, updated_at: fixed_time) } - let(:note) { create(:note_on_merge_request_diff, noteable_id: merge_request.id, project: project) } + let(:merge_request) do + create(:merge_request, created_at: fixed_time, updated_at: fixed_time, + source_project: project) + end + + let(:note) do + create(:note_on_merge_request_diff, noteable: merge_request, + project: project) + end it 'returns the note and merge request diff data' do expect(data).to have_key(:merge_request) - expect(data[:merge_request].except('updated_at')).to eq(merge_request.hook_attrs.except('updated_at')) - expect(data[:merge_request]['updated_at']).to be > merge_request.hook_attrs['updated_at'] + expect(data[:merge_request].except('updated_at')) + .to eq(merge_request.reload.hook_attrs.except('updated_at')) + expect(data[:merge_request]['updated_at']) + .to be > merge_request.hook_attrs['updated_at'] end include_examples 'project hook data' @@ -79,13 +107,22 @@ describe 'Gitlab::NoteDataBuilder', lib: true do end describe 'When asking for a note on project snippet' do - let!(:snippet) { create(:project_snippet, created_at: fixed_time, updated_at: fixed_time) } - let!(:note) { create(:note_on_project_snippet, noteable_id: snippet.id, project: project) } + let!(:snippet) do + create(:project_snippet, created_at: fixed_time, updated_at: fixed_time, + project: project) + end + + let!(:note) do + create(:note_on_project_snippet, noteable: snippet, + project: project) + end it 'returns the note and project snippet data' do expect(data).to have_key(:snippet) - expect(data[:snippet].except('updated_at')).to eq(snippet.hook_attrs.except('updated_at')) - expect(data[:snippet]['updated_at']).to be > snippet.hook_attrs['updated_at'] + expect(data[:snippet].except('updated_at')) + .to eq(snippet.reload.hook_attrs.except('updated_at')) + expect(data[:snippet]['updated_at']) + .to be > snippet.hook_attrs['updated_at'] end include_examples 'project hook data' diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index 583827cdf42fb93faae9a00f37ad35f495ed31bc..70bbe63326930ac52f2c842219d14bc7c64e6c6d 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -189,7 +189,6 @@ describe Issue, "Issuable" do let(:data) { issue.to_hook_data(user) } let(:project) { issue.project } - it "returns correct hook data" do expect(data[:object_kind]).to eq("issue") expect(data[:user]).to eq(user.hook_attrs) @@ -229,11 +228,11 @@ describe Issue, "Issuable" do end describe "votes" do + let(:project) { issue.project } + before do - author = create :user - project = create :empty_project - issue.notes.awards.create!(note: "thumbsup", author: author, project: project) - issue.notes.awards.create!(note: "thumbsdown", author: author, project: project) + issue.notes.awards.create!(note: "thumbsup", author: user, project: project) + issue.notes.awards.create!(note: "thumbsdown", author: user, project: project) end it "returns correct values" do diff --git a/spec/models/legacy_diff_note_spec.rb b/spec/models/legacy_diff_note_spec.rb index 7c29bef54e48b8993c02d7f32e04eaccfeceddc2..b2d068538864d05f6808c51724fc243950b96ba2 100644 --- a/spec/models/legacy_diff_note_spec.rb +++ b/spec/models/legacy_diff_note_spec.rb @@ -63,7 +63,9 @@ describe LegacyDiffNote, models: true do code = Gitlab::Diff::LineCode.generate(diff.new_path, line.new_pos, line.old_pos) # We're persisting in order to trigger the set_diff callback - note = create(:note_on_merge_request_diff, noteable: merge, line_code: code) + note = create(:note_on_merge_request_diff, noteable: merge, + line_code: code, + project: merge.source_project) # Make sure we don't get a false positive from a guard clause expect(note).to receive(:find_noteable_diff).and_call_original diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index e269ff26a047f40f9d5f60c648edaf8a3cb773b5..4b67c2facf32a7e7c0476f4d32b9d1d767b98187 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -119,7 +119,8 @@ describe MergeRequest, models: true do before do allow(merge_request).to receive(:commits) { [merge_request.source_project.repository.commit] } - create(:note, commit_id: merge_request.commits.first.id, noteable_type: 'Commit', project: merge_request.project) + create(:note_on_commit, commit_id: merge_request.commits.first.id, + project: merge_request.project) create(:note, noteable: merge_request, project: merge_request.project) end @@ -129,7 +130,9 @@ describe MergeRequest, models: true do end it "should include notes for commits from target project as well" do - create(:note, commit_id: merge_request.commits.first.id, noteable_type: 'Commit', project: merge_request.target_project) + create(:note_on_commit, commit_id: merge_request.commits.first.id, + project: merge_request.target_project) + expect(merge_request.commits).not_to be_empty expect(merge_request.mr_and_commit_notes.count).to eq(3) end diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 5d916f0e6a65b2263cb506f583e033438d2f7436..8aad1b73add203d4826fec1204b83b561db223c3 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -12,6 +12,34 @@ describe Note, models: true do describe 'validation' do it { is_expected.to validate_presence_of(:note) } it { is_expected.to validate_presence_of(:project) } + + context 'when note is on commit' do + before { allow(subject).to receive(:for_commit?).and_return(true) } + + it { is_expected.to validate_presence_of(:commit_id) } + it { is_expected.not_to validate_presence_of(:noteable_id) } + end + + context 'when note is not on commit' do + before { allow(subject).to receive(:for_commit?).and_return(false) } + + it { is_expected.not_to validate_presence_of(:commit_id) } + it { is_expected.to validate_presence_of(:noteable_id) } + end + + context 'when noteable and note project differ' do + subject do + build(:note, noteable: build_stubbed(:issue), + project: build_stubbed(:project)) + end + + it { is_expected.to be_invalid } + end + + context 'when noteable and note project are the same' do + subject { create(:note) } + it { is_expected.to be_valid } + end end describe "Commit notes" do @@ -89,8 +117,8 @@ describe Note, models: true do end describe "#all_references" do - let!(:note1) { create(:note) } - let!(:note2) { create(:note) } + let!(:note1) { create(:note_on_issue) } + let!(:note2) { create(:note_on_issue) } it "reads the rendered note body from the cache" do expect(Banzai::Renderer).to receive(:render).with(note1.note, pipeline: :note, cache_key: [note1, "note"], project: note1.project) @@ -102,7 +130,7 @@ describe Note, models: true do end describe '.search' do - let(:note) { create(:note, note: 'WoW') } + let(:note) { create(:note_on_issue, note: 'WoW') } it 'returns notes with matching content' do expect(described_class.search(note.note)).to eq([note]) @@ -175,12 +203,18 @@ describe Note, models: true do let(:merge_request) { create :merge_request } it "converts aliases to actual name" do - note = create(:note, note: ":+1:", noteable: merge_request) + note = create(:note, note: ":+1:", + noteable: merge_request, + project: merge_request.project) + expect(note.reload.note).to eq("thumbsup") end it "is not an award emoji when comment is on a diff" do - note = create(:note_on_merge_request_diff, note: ":blowfish:", noteable: merge_request, line_code: "11d5d2e667e9da4f7f610f81d86c974b146b13bd_0_2") + note = create(:note_on_merge_request_diff, note: ":blowfish:", + noteable: merge_request, + project: merge_request.project, + line_code: "11d5d2e667e9da4f7f610f81d86c974b146b13bd_0_2") note = note.reload expect(note.note).to eq(":blowfish:") diff --git a/spec/models/project_services/hipchat_service_spec.rb b/spec/models/project_services/hipchat_service_spec.rb index f887ed4bafc14493a63520e20ddd49815da761cb..5f618322aabe07df6128cb6acc05c79d0f93e4f8 100644 --- a/spec/models/project_services/hipchat_service_spec.rb +++ b/spec/models/project_services/hipchat_service_spec.rb @@ -176,86 +176,117 @@ describe HipchatService, models: true do context "Note events" do let(:user) { create(:user) } let(:project) { create(:project, creator_id: user.id) } - let(:issue) { create(:issue, project: project) } - let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } - let(:snippet) { create(:project_snippet, project: project) } - let(:commit_note) { create(:note_on_commit, author: user, project: project, commit_id: project.repository.commit.id, note: 'a comment on a commit') } - let(:merge_request_note) { create(:note_on_merge_request, noteable_id: merge_request.id, note: "merge request note") } - let(:issue_note) { create(:note_on_issue, noteable_id: issue.id, note: "issue note")} - let(:snippet_note) { create(:note_on_project_snippet, noteable_id: snippet.id, note: "snippet note") } - - it "should call Hipchat API for commit comment events" do - data = Gitlab::NoteDataBuilder.build(commit_note, user) - hipchat.execute(data) - expect(WebMock).to have_requested(:post, api_url).once + context 'when commit comment event triggered' do + let(:commit_note) do + create(:note_on_commit, author: user, project: project, + commit_id: project.repository.commit.id, + note: 'a comment on a commit') + end + + it "should call Hipchat API for commit comment events" do + data = Gitlab::NoteDataBuilder.build(commit_note, user) + hipchat.execute(data) - message = hipchat.send(:create_message, data) + expect(WebMock).to have_requested(:post, api_url).once - obj_attr = data[:object_attributes] - commit_id = Commit.truncate_sha(data[:commit][:id]) - title = hipchat.send(:format_title, data[:commit][:message]) + message = hipchat.send(:create_message, data) - expect(message).to eq("#{user.name} commented on " \ - "commit #{commit_id} in " \ - "#{project_name}: " \ - "#{title}" \ - "
a comment on a commit
") + obj_attr = data[:object_attributes] + commit_id = Commit.truncate_sha(data[:commit][:id]) + title = hipchat.send(:format_title, data[:commit][:message]) + + expect(message).to eq("#{user.name} commented on " \ + "commit #{commit_id} in " \ + "#{project_name}: " \ + "#{title}" \ + "
a comment on a commit
") + end end - it "should call Hipchat API for merge request comment events" do - data = Gitlab::NoteDataBuilder.build(merge_request_note, user) - hipchat.execute(data) + context 'when merge request comment event triggered' do + let(:merge_request) do + create(:merge_request, source_project: project, + target_project: project) + end - expect(WebMock).to have_requested(:post, api_url).once + let(:merge_request_note) do + create(:note_on_merge_request, noteable: merge_request, + project: project, + note: "merge request note") + end - message = hipchat.send(:create_message, data) + it "should call Hipchat API for merge request comment events" do + data = Gitlab::NoteDataBuilder.build(merge_request_note, user) + hipchat.execute(data) - obj_attr = data[:object_attributes] - merge_id = data[:merge_request]['iid'] - title = data[:merge_request]['title'] + expect(WebMock).to have_requested(:post, api_url).once - expect(message).to eq("#{user.name} commented on " \ - "merge request !#{merge_id} in " \ - "#{project_name}: " \ - "#{title}" \ - "
merge request note
") + message = hipchat.send(:create_message, data) + + obj_attr = data[:object_attributes] + merge_id = data[:merge_request]['iid'] + title = data[:merge_request]['title'] + + expect(message).to eq("#{user.name} commented on " \ + "merge request !#{merge_id} in " \ + "#{project_name}: " \ + "#{title}" \ + "
merge request note
") + end end - it "should call Hipchat API for issue comment events" do - data = Gitlab::NoteDataBuilder.build(issue_note, user) - hipchat.execute(data) + context 'when issue comment event triggered' do + let(:issue) { create(:issue, project: project) } + let(:issue_note) do + create(:note_on_issue, noteable: issue, project: project, + note: "issue note") + end - message = hipchat.send(:create_message, data) + it "should call Hipchat API for issue comment events" do + data = Gitlab::NoteDataBuilder.build(issue_note, user) + hipchat.execute(data) - obj_attr = data[:object_attributes] - issue_id = data[:issue]['iid'] - title = data[:issue]['title'] + message = hipchat.send(:create_message, data) - expect(message).to eq("#{user.name} commented on " \ - "issue ##{issue_id} in " \ - "#{project_name}: " \ - "#{title}" \ - "
issue note
") + obj_attr = data[:object_attributes] + issue_id = data[:issue]['iid'] + title = data[:issue]['title'] + + expect(message).to eq("#{user.name} commented on " \ + "issue ##{issue_id} in " \ + "#{project_name}: " \ + "#{title}" \ + "
issue note
") + end end - it "should call Hipchat API for snippet comment events" do - data = Gitlab::NoteDataBuilder.build(snippet_note, user) - hipchat.execute(data) + context 'when snippet comment event triggered' do + let(:snippet) { create(:project_snippet, project: project) } + let(:snippet_note) do + create(:note_on_project_snippet, noteable: snippet, + project: project, + note: "snippet note") + end - expect(WebMock).to have_requested(:post, api_url).once + it "should call Hipchat API for snippet comment events" do + data = Gitlab::NoteDataBuilder.build(snippet_note, user) + hipchat.execute(data) - message = hipchat.send(:create_message, data) + expect(WebMock).to have_requested(:post, api_url).once - obj_attr = data[:object_attributes] - snippet_id = data[:snippet]['id'] - title = data[:snippet]['title'] + message = hipchat.send(:create_message, data) - expect(message).to eq("#{user.name} commented on " \ - "snippet ##{snippet_id} in " \ - "#{project_name}: " \ - "#{title}" \ - "
snippet note
") + obj_attr = data[:object_attributes] + snippet_id = data[:snippet]['id'] + title = data[:snippet]['title'] + + expect(message).to eq("#{user.name} commented on " \ + "snippet ##{snippet_id} in " \ + "#{project_name}: " \ + "#{title}" \ + "
snippet note
") + end end end diff --git a/spec/models/project_services/slack_service_spec.rb b/spec/models/project_services/slack_service_spec.rb index a97b7560137fdaec9860916a2926690bf2b71341..155f3e74e0d24989c51bdb357f494927477d481a 100644 --- a/spec/models/project_services/slack_service_spec.rb +++ b/spec/models/project_services/slack_service_spec.rb @@ -142,13 +142,6 @@ describe SlackService, models: true do let(:slack) { SlackService.new } let(:user) { create(:user) } let(:project) { create(:project, creator_id: user.id) } - let(:issue) { create(:issue, project: project) } - let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } - let(:snippet) { create(:project_snippet, project: project) } - let(:commit_note) { create(:note_on_commit, author: user, project: project, commit_id: project.repository.commit.id, note: 'a comment on a commit') } - let(:merge_request_note) { create(:note_on_merge_request, noteable_id: merge_request.id, note: "merge request note") } - let(:issue_note) { create(:note_on_issue, noteable_id: issue.id, note: "issue note")} - let(:snippet_note) { create(:note_on_project_snippet, noteable_id: snippet.id, note: "snippet note") } let(:webhook_url) { 'https://hooks.slack.com/services/SVRWFV0VVAR97N/B02R25XN3/ZBqu7xMupaEEICInN685' } before do @@ -162,32 +155,61 @@ describe SlackService, models: true do WebMock.stub_request(:post, webhook_url) end - it "should call Slack API for commit comment events" do - data = Gitlab::NoteDataBuilder.build(commit_note, user) - slack.execute(data) + context 'when commit comment event executed' do + let(:commit_note) do + create(:note_on_commit, author: user, + project: project, + commit_id: project.repository.commit.id, + note: 'a comment on a commit') + end - expect(WebMock).to have_requested(:post, webhook_url).once + it "should call Slack API for commit comment events" do + data = Gitlab::NoteDataBuilder.build(commit_note, user) + slack.execute(data) + + expect(WebMock).to have_requested(:post, webhook_url).once + end end - it "should call Slack API for merge request comment events" do - data = Gitlab::NoteDataBuilder.build(merge_request_note, user) - slack.execute(data) + context 'when merge request comment event executed' do + let(:merge_request_note) do + create(:note_on_merge_request, project: project, + note: "merge request note") + end - expect(WebMock).to have_requested(:post, webhook_url).once + it "should call Slack API for merge request comment events" do + data = Gitlab::NoteDataBuilder.build(merge_request_note, user) + slack.execute(data) + + expect(WebMock).to have_requested(:post, webhook_url).once + end end - it "should call Slack API for issue comment events" do - data = Gitlab::NoteDataBuilder.build(issue_note, user) - slack.execute(data) + context 'when issue comment event executed' do + let(:issue_note) do + create(:note_on_issue, project: project, note: "issue note") + end - expect(WebMock).to have_requested(:post, webhook_url).once + it "should call Slack API for issue comment events" do + data = Gitlab::NoteDataBuilder.build(issue_note, user) + slack.execute(data) + + expect(WebMock).to have_requested(:post, webhook_url).once + end end - it "should call Slack API for snippet comment events" do - data = Gitlab::NoteDataBuilder.build(snippet_note, user) - slack.execute(data) + context 'when snippet comment event executed' do + let(:snippet_note) do + create(:note_on_project_snippet, project: project, + note: "snippet note") + end - expect(WebMock).to have_requested(:post, webhook_url).once + it "should call Slack API for snippet comment events" do + data = Gitlab::NoteDataBuilder.build(snippet_note, user) + slack.execute(data) + + expect(WebMock).to have_requested(:post, webhook_url).once + end end end end diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb index ed1ed5aeb954a18a92030e4c9f5e404488d4f0c4..beb29a686920987541bb3b3f24c0b0d49934ba71 100644 --- a/spec/requests/api/notes_spec.rb +++ b/spec/requests/api/notes_spec.rb @@ -258,8 +258,8 @@ describe API::API, api: true do body: 'Hi!' end - it 'responds with 500' do - expect(response.status).to eq 500 + it 'responds with resource not found error' do + expect(response.status).to eq 404 end it 'does not create new note' do diff --git a/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb b/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb index 8224784981448d216667cb23b4ab6f57ebb8758a..0861d74aeded4729fdc9a803aa38a16ac6e114fb 100644 --- a/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb +++ b/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb @@ -1,8 +1,8 @@ require 'spec_helper' describe MergeRequests::MergeWhenBuildSucceedsService do - let(:user) { create(:user) } - let(:merge_request) { create(:merge_request) } + let(:user) { create(:user) } + let(:project) { create(:project) } let(:mr_merge_if_green_enabled) do create(:merge_request, merge_when_build_succeeds: true, merge_user: user, @@ -10,11 +10,15 @@ describe MergeRequests::MergeWhenBuildSucceedsService do source_project: project, target_project: project, state: "opened") end - let(:project) { create(:project) } let(:ci_commit) { create(:ci_commit_with_one_job, ref: mr_merge_if_green_enabled.source_branch, project: project) } let(:service) { MergeRequests::MergeWhenBuildSucceedsService.new(project, user, commit_message: 'Awesome message') } describe "#execute" do + let(:merge_request) do + create(:merge_request, target_project: project, source_project: project, + source_branch: "feature", target_branch: 'master') + end + context 'first time enabling' do before do allow(merge_request).to receive(:ci_commit).and_return(ci_commit) diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index ee976eb2926caddee28e52ce2521f892d609a059..29e0a63d8ce1d700bc39d592fc150e9861063b3a 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -208,8 +208,10 @@ describe SystemNoteService, services: true do end describe '.merge_when_build_succeeds' do - let(:ci_commit) { build :ci_commit_without_jobs } - let(:noteable) { create :merge_request } + let(:ci_commit) { build(:ci_commit_without_jobs )} + let(:noteable) do + create(:merge_request, source_project: project, target_project: project) + end subject { described_class.merge_when_build_succeeds(noteable, project, author, noteable.last_commit) } @@ -221,8 +223,10 @@ describe SystemNoteService, services: true do end describe '.cancel_merge_when_build_succeeds' do - let(:ci_commit) { build :ci_commit_without_jobs } - let(:noteable) { create :merge_request } + let(:ci_commit) { build(:ci_commit_without_jobs) } + let(:noteable) do + create(:merge_request, source_project: project, target_project: project) + end subject { described_class.cancel_merge_when_build_succeeds(noteable, project, author) }