diff options
Diffstat (limited to 'spec')
23 files changed, 1050 insertions, 5 deletions
diff --git a/spec/db/schema_spec.rb b/spec/db/schema_spec.rb index e8584846b56..7c505ee0d43 100644 --- a/spec/db/schema_spec.rb +++ b/spec/db/schema_spec.rb @@ -54,7 +54,8 @@ describe 'Database schema' do user_agent_details: %w[subject_id], users: %w[color_scheme_id created_by_id theme_id], users_star_projects: %w[user_id], - web_hooks: %w[service_id] + web_hooks: %w[service_id], + suggestions: %w[commit_id] }.with_indifferent_access.freeze context 'for table' do diff --git a/spec/factories/suggestions.rb b/spec/factories/suggestions.rb new file mode 100644 index 00000000000..307523cc061 --- /dev/null +++ b/spec/factories/suggestions.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :suggestion do + relative_order 0 + association :note, factory: :diff_note_on_merge_request + from_content " vars = {\n" + to_content " vars = [\n" + + trait :unappliable do + from_content "foo" + to_content "foo" + end + + trait :applied do + applied true + commit_id { RepoHelpers.sample_commit.id } + end + end +end diff --git a/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb b/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb new file mode 100644 index 00000000000..c19e299097e --- /dev/null +++ b/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb @@ -0,0 +1,85 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe 'User comments on a diff', :js do + include MergeRequestDiffHelpers + include RepoHelpers + + let(:project) { create(:project, :repository) } + let(:merge_request) do + create(:merge_request_with_diffs, source_project: project, target_project: project, source_branch: 'merge-test') + end + let(:user) { create(:user) } + + before do + project.add_maintainer(user) + sign_in(user) + + visit(diffs_project_merge_request_path(project, merge_request)) + end + + context 'single suggestion note' do + it 'suggestion is presented' do + click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']")) + + page.within('.js-discussion-note-form') do + fill_in('note_note', with: "```suggestion\n# change to a comment\n```") + click_button('Comment') + end + + wait_for_requests + + page.within('.diff-discussions') do + expect(page).to have_button('Apply suggestion') + expect(page).to have_content('Suggested change') + expect(page).to have_content(' url = https://github.com/gitlabhq/gitlab-shell.git') + expect(page).to have_content('# change to a comment') + end + end + + it 'suggestion is appliable' do + click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']")) + + page.within('.js-discussion-note-form') do + fill_in('note_note', with: "```suggestion\n# change to a comment\n```") + click_button('Comment') + end + + wait_for_requests + + page.within('.diff-discussions') do + expect(page).not_to have_content('Applied') + + click_button('Apply suggestion') + wait_for_requests + + expect(page).to have_content('Applied') + end + end + end + + context 'multiple suggestions in a single note' do + it 'suggestions are presented' do + click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']")) + + page.within('.js-discussion-note-form') do + fill_in('note_note', with: "```suggestion\n# change to a comment\n```\n```suggestion\n# or that\n```") + click_button('Comment') + end + + wait_for_requests + + page.within('.diff-discussions') do + suggestion_1 = page.all(:css, '.md-suggestion-diff')[0] + suggestion_2 = page.all(:css, '.md-suggestion-diff')[1] + + expect(suggestion_1).to have_content(' url = https://github.com/gitlabhq/gitlab-shell.git') + expect(suggestion_1).to have_content('# change to a comment') + + expect(suggestion_2).to have_content(' url = https://github.com/gitlabhq/gitlab-shell.git') + expect(suggestion_2).to have_content('# or that') + end + end + end +end diff --git a/spec/fixtures/api/schemas/entities/diff_line.json b/spec/fixtures/api/schemas/entities/diff_line.json index 66e8b443e1b..9657004cd2d 100644 --- a/spec/fixtures/api/schemas/entities/diff_line.json +++ b/spec/fixtures/api/schemas/entities/diff_line.json @@ -8,7 +8,8 @@ "new_line": { "type": ["integer", "null"] }, "text": { "type": ["string"] }, "rich_text": { "type": ["string"] }, - "meta_data": { "type": ["object", "null"] } + "meta_data": { "type": ["object", "null"] }, + "can_receive_suggestion": { "type": "boolean" } }, "additionalProperties": false } diff --git a/spec/fixtures/api/schemas/entities/merge_request_widget.json b/spec/fixtures/api/schemas/entities/merge_request_widget.json index 35971d564d5..193ab6821a5 100644 --- a/spec/fixtures/api/schemas/entities/merge_request_widget.json +++ b/spec/fixtures/api/schemas/entities/merge_request_widget.json @@ -119,7 +119,8 @@ "can_push_to_source_branch": { "type": "boolean" }, "rebase_path": { "type": ["string", "null"] }, "squash": { "type": "boolean" }, - "test_reports_path": { "type": ["string", "null"] } + "test_reports_path": { "type": ["string", "null"] }, + "can_receive_suggestion": { "type": "boolean" } }, "additionalProperties": false } diff --git a/spec/javascripts/diffs/components/diff_content_spec.js b/spec/javascripts/diffs/components/diff_content_spec.js index c25f6167163..d688560a260 100644 --- a/spec/javascripts/diffs/components/diff_content_spec.js +++ b/spec/javascripts/diffs/components/diff_content_spec.js @@ -17,6 +17,7 @@ describe('DiffContent', () => { current_user: { can_create_note: false, }, + preview_note_path: 'path/to/preview', }; vm = mountComponentWithStore(Component, { diff --git a/spec/javascripts/diffs/mock_data/diff_discussions.js b/spec/javascripts/diffs/mock_data/diff_discussions.js index 44313caba29..c1e9f791925 100644 --- a/spec/javascripts/diffs/mock_data/diff_discussions.js +++ b/spec/javascripts/diffs/mock_data/diff_discussions.js @@ -487,8 +487,19 @@ export default { ], }, diff_discussion: true, - truncated_diff_lines: - '<tr class="line_holder new" id="">\n<td class="diff-line-num new old_line" data-linenumber="1">\n \n</td>\n<td class="diff-line-num new new_line" data-linenumber="1">\n1\n</td>\n<td class="line_content new noteable_line"><span id="LC1" class="line" lang="plaintext"> - Bad dates</span>\n</td>\n</tr>\n<tr class="line_holder new" id="">\n<td class="diff-line-num new old_line" data-linenumber="1">\n \n</td>\n<td class="diff-line-num new new_line" data-linenumber="2">\n2\n</td>\n<td class="line_content new noteable_line"><span id="LC2" class="line" lang="plaintext"></span>\n</td>\n</tr>\n', + truncated_diff_lines: [ + { + text: 'line', + rich_text: + '<tr class="line_holder new" id="">\n<td class="diff-line-num new old_line" data-linenumber="1">\n \n</td>\n<td class="diff-line-num new new_line" data-linenumber="1">\n1\n</td>\n<td class="line_content new noteable_line"><span id="LC1" class="line" lang="plaintext"> - Bad dates</span>\n</td>\n</tr>\n<tr class="line_holder new" id="">\n<td class="diff-line-num new old_line" data-linenumber="1">\n \n</td>\n<td class="diff-line-num new new_line" data-linenumber="2">\n2\n</td>\n<td class="line_content new noteable_line"><span id="LC2" class="line" lang="plaintext"></span>\n</td>\n</tr>\n', + can_receive_suggestion: true, + line_code: '6f209374f7e565f771b95720abf46024c41d1885_1_1', + type: 'new', + old_line: null, + new_line: 1, + meta_data: null, + }, + ], }; export const imageDiffDiscussions = [ diff --git a/spec/javascripts/vue_shared/components/markdown/header_spec.js b/spec/javascripts/vue_shared/components/markdown/header_spec.js index 59613faa49f..e733a95288e 100644 --- a/spec/javascripts/vue_shared/components/markdown/header_spec.js +++ b/spec/javascripts/vue_shared/components/markdown/header_spec.js @@ -28,6 +28,7 @@ describe('Markdown field header component', () => { 'Add a numbered list', 'Add a task list', 'Add a table', + 'Insert suggestion', 'Go full screen', ]; const elements = vm.$el.querySelectorAll('.toolbar-btn'); @@ -93,4 +94,18 @@ describe('Markdown field header component', () => { '| header | header |\n| ------ | ------ |\n| cell | cell |\n| cell | cell |', ); }); + + it('renders suggestion template', () => { + vm.lineContent = 'Some content'; + + expect(vm.mdSuggestion).toEqual('```suggestion\n{text}\n```'); + }); + + it('does not render suggestion button if `canSuggest` is set to false', () => { + vm.canSuggest = false; + + Vue.nextTick(() => { + expect(vm.$el.querySelector('.qa-suggestion-btn')).toBe(null); + }); + }); }); diff --git a/spec/javascripts/vue_shared/components/markdown/suggestion_diff_header_spec.js b/spec/javascripts/vue_shared/components/markdown/suggestion_diff_header_spec.js new file mode 100644 index 00000000000..8187b3204b1 --- /dev/null +++ b/spec/javascripts/vue_shared/components/markdown/suggestion_diff_header_spec.js @@ -0,0 +1,69 @@ +import Vue from 'vue'; +import SuggestionDiffHeaderComponent from '~/vue_shared/components/markdown/suggestion_diff_header.vue'; + +const MOCK_DATA = { + canApply: true, + isApplied: false, + helpPagePath: 'path_to_docs', +}; + +describe('Suggestion Diff component', () => { + let vm; + + function createComponent(propsData) { + const Component = Vue.extend(SuggestionDiffHeaderComponent); + + return new Component({ + propsData, + }).$mount(); + } + + beforeEach(done => { + vm = createComponent(MOCK_DATA); + Vue.nextTick(done); + }); + + describe('init', () => { + it('renders a suggestion header', () => { + const header = vm.$el.querySelector('.qa-suggestion-diff-header'); + + expect(header).not.toBeNull(); + expect(header.innerHTML.includes('Suggested change')).toBe(true); + }); + + it('renders an apply button', () => { + const applyBtn = vm.$el.querySelector('.qa-apply-btn'); + + expect(applyBtn).not.toBeNull(); + expect(applyBtn.innerHTML.includes('Apply suggestion')).toBe(true); + }); + + it('does not render an apply button if `canApply` is set to false', () => { + const props = Object.assign(MOCK_DATA, { canApply: false }); + + vm = createComponent(props); + + expect(vm.$el.querySelector('.qa-apply-btn')).toBeNull(); + }); + }); + + describe('applySuggestion', () => { + it('emits when the apply button is clicked', () => { + const props = Object.assign(MOCK_DATA, { canApply: true }); + + vm = createComponent(props); + spyOn(vm, '$emit'); + vm.applySuggestion(); + + expect(vm.$emit).toHaveBeenCalled(); + }); + + it('does not emit when the canApply is set to false', () => { + spyOn(vm, '$emit'); + vm.canApply = false; + vm.applySuggestion(); + + expect(vm.$emit).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/spec/javascripts/vue_shared/components/markdown/suggestion_diff_spec.js b/spec/javascripts/vue_shared/components/markdown/suggestion_diff_spec.js new file mode 100644 index 00000000000..d4ed8f2f7a4 --- /dev/null +++ b/spec/javascripts/vue_shared/components/markdown/suggestion_diff_spec.js @@ -0,0 +1,79 @@ +import Vue from 'vue'; +import SuggestionDiffComponent from '~/vue_shared/components/markdown/suggestion_diff.vue'; + +const MOCK_DATA = { + canApply: true, + newLines: [ + { content: 'Line 1\n', lineNumber: 1 }, + { content: 'Line 2\n', lineNumber: 2 }, + { content: 'Line 3\n', lineNumber: 3 }, + ], + fromLine: 1, + fromContent: 'Old content', + suggestion: { + id: 1, + }, + helpPagePath: 'path_to_docs', +}; + +describe('Suggestion Diff component', () => { + let vm; + + beforeEach(done => { + const Component = Vue.extend(SuggestionDiffComponent); + + vm = new Component({ + propsData: MOCK_DATA, + }).$mount(); + + Vue.nextTick(done); + }); + + describe('init', () => { + it('renders a suggestion header', () => { + expect(vm.$el.querySelector('.qa-suggestion-diff-header')).not.toBeNull(); + }); + + it('renders a diff table', () => { + expect(vm.$el.querySelector('table.md-suggestion-diff')).not.toBeNull(); + }); + + it('renders the oldLineNumber', () => { + const fromLine = vm.$el.querySelector('.qa-old-diff-line-number').innerHTML; + + expect(parseInt(fromLine, 10)).toBe(vm.fromLine); + }); + + it('renders the oldLineContent', () => { + const fromContent = vm.$el.querySelector('.line_content.old').innerHTML; + + expect(fromContent.includes(vm.fromContent)).toBe(true); + }); + + it('renders the contents of newLines', () => { + const newLines = vm.$el.querySelectorAll('.line_holder.new'); + + newLines.forEach((line, i) => { + expect(newLines[i].innerHTML.includes(vm.newLines[i].content)).toBe(true); + }); + }); + + it('renders a line number for each line', () => { + const newLineNumbers = vm.$el.querySelectorAll('.qa-new-diff-line-number'); + + newLineNumbers.forEach((line, i) => { + expect(newLineNumbers[i].innerHTML.includes(vm.newLines[i].lineNumber)).toBe(true); + }); + }); + }); + + describe('applySuggestion', () => { + it('emits apply event when applySuggestion is called', () => { + const callback = () => {}; + spyOn(vm, '$emit'); + vm.applySuggestion(callback); + + expect(vm.$emit).toHaveBeenCalledWith('apply', { suggestionId: vm.suggestion.id, callback }); + }); + }); +}); diff --git a/spec/javascripts/vue_shared/components/markdown/suggestions_spec.js b/spec/javascripts/vue_shared/components/markdown/suggestions_spec.js new file mode 100644 index 00000000000..ab1b747c360 --- /dev/null +++ b/spec/javascripts/vue_shared/components/markdown/suggestions_spec.js @@ -0,0 +1,125 @@ +import Vue from 'vue'; +import SuggestionsComponent from '~/vue_shared/components/markdown/suggestions.vue'; + +const MOCK_DATA = { + fromLine: 1, + fromContent: 'Old content', + suggestions: [], + noteHtml: ` + <div class="suggestion"> + <div class="line">Suggestion 1</div> + </div> + + <div class="suggestion"> + <div class="line">Suggestion 2</div> + </div> + `, + isApplied: false, + helpPagePath: 'path_to_docs', +}; + +const generateLine = content => { + const line = document.createElement('div'); + line.className = 'line'; + line.innerHTML = content; + + return line; +}; + +const generateMockLines = () => { + const line1 = generateLine('Line 1'); + const line2 = generateLine('Line 2'); + const line3 = generateLine('Line 3'); + const container = document.createElement('div'); + + container.appendChild(line1); + container.appendChild(line2); + container.appendChild(line3); + + return container; +}; + +describe('Suggestion component', () => { + let vm; + let extractedLines; + let diffTable; + + beforeEach(done => { + const Component = Vue.extend(SuggestionsComponent); + + vm = new Component({ + propsData: MOCK_DATA, + }).$mount(); + + extractedLines = vm.extractNewLines(generateMockLines()); + diffTable = vm.generateDiff(extractedLines).$mount().$el; + + spyOn(vm, 'renderSuggestions'); + vm.renderSuggestions(); + Vue.nextTick(done); + }); + + describe('mounted', () => { + it('renders a flash container', () => { + expect(vm.$el.querySelector('.flash-container')).not.toBeNull(); + }); + + it('renders a container for suggestions', () => { + expect(vm.$refs.container).not.toBeNull(); + }); + + it('renders suggestions', () => { + expect(vm.renderSuggestions).toHaveBeenCalled(); + expect(vm.$el.innerHTML.includes('Suggestion 1')).toBe(true); + expect(vm.$el.innerHTML.includes('Suggestion 2')).toBe(true); + }); + }); + + describe('extractNewLines', () => { + it('extracts suggested lines', () => { + const expectedReturn = [ + { content: 'Line 1\n', lineNumber: 1 }, + { content: 'Line 2\n', lineNumber: 2 }, + { content: 'Line 3\n', lineNumber: 3 }, + ]; + + expect(vm.extractNewLines(generateMockLines())).toEqual(expectedReturn); + }); + + it('increments line number for each extracted line', () => { + expect(extractedLines[0].lineNumber).toEqual(1); + expect(extractedLines[1].lineNumber).toEqual(2); + expect(extractedLines[2].lineNumber).toEqual(3); + }); + + it('returns empty array if no lines are found', () => { + const el = document.createElement('div'); + + expect(vm.extractNewLines(el)).toEqual([]); + }); + }); + + describe('generateDiff', () => { + it('generates a diff table', () => { + expect(diffTable.querySelector('.md-suggestion-diff')).not.toBeNull(); + }); + + it('generates a diff table that contains contents of `oldLineContent`', () => { + expect(diffTable.innerHTML.includes(vm.fromContent)).toBe(true); + }); + + it('generates a diff table that contains contents the suggested lines', () => { + extractedLines.forEach((line, i) => { + expect(diffTable.innerHTML.includes(extractedLines[i].content)).toBe(true); + }); + }); + + it('generates a diff table with the correct line number for each suggested line', () => { + const lines = diffTable.getElementsByClassName('qa-new-diff-line-number'); + + expect([...lines][0].innerHTML).toBe('1'); + expect([...lines][1].innerHTML).toBe('2'); + expect([...lines][2].innerHTML).toBe('3'); + }); + }); +}); diff --git a/spec/lib/banzai/filter/suggestion_filter_spec.rb b/spec/lib/banzai/filter/suggestion_filter_spec.rb new file mode 100644 index 00000000000..55a141bf315 --- /dev/null +++ b/spec/lib/banzai/filter/suggestion_filter_spec.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Banzai::Filter::SuggestionFilter do + include FilterSpecHelper + + let(:input) { "<pre class='code highlight js-syntax-highlight suggestion'><code>foo\n</code></pre>" } + let(:default_context) do + { suggestions_filter_enabled: true } + end + + it 'includes `js-render-suggestion` class' do + doc = filter(input, default_context) + result = doc.css('code').first + + expect(result[:class]).to include('js-render-suggestion') + end + + it 'includes no `js-render-suggestion` when feature disabled' do + stub_feature_flags(diff_suggestions: false) + + doc = filter(input, default_context) + result = doc.css('code').first + + expect(result[:class]).to be_nil + end + + it 'includes no `js-render-suggestion` when filter is disabled' do + doc = filter(input) + result = doc.css('code').first + + expect(result[:class]).to be_nil + end +end diff --git a/spec/lib/banzai/suggestions_parser_spec.rb b/spec/lib/banzai/suggestions_parser_spec.rb new file mode 100644 index 00000000000..79658d710ce --- /dev/null +++ b/spec/lib/banzai/suggestions_parser_spec.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Banzai::SuggestionsParser do + describe '.parse' do + it 'returns a list of suggestion contents' do + markdown = <<-MARKDOWN.strip_heredoc + ```suggestion + foo + bar + ``` + + ``` + nothing + ``` + + ```suggestion + xpto + baz + ``` + + ```thing + this is not a suggestion, it's a thing + ``` + MARKDOWN + + expect(described_class.parse(markdown)).to eq([" foo\n bar", + " xpto\n baz"]) + end + end +end diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index bae5b21c26f..72abd8973cb 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -37,6 +37,7 @@ notes: - events - system_note_metadata - note_diff_file +- suggestions label_links: - target - label diff --git a/spec/lib/gitlab/usage_data_spec.rb b/spec/lib/gitlab/usage_data_spec.rb index deb19fe1a4b..2a09f581f68 100644 --- a/spec/lib/gitlab/usage_data_spec.rb +++ b/spec/lib/gitlab/usage_data_spec.rb @@ -117,6 +117,7 @@ describe Gitlab::UsageData do releases remote_mirrors snippets + suggestions todos uploads web_hooks diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb index 8624f0daa4d..40ce8ab736a 100644 --- a/spec/models/diff_note_spec.rb +++ b/spec/models/diff_note_spec.rb @@ -318,6 +318,24 @@ describe DiffNote do end end + describe '#supports_suggestion?' do + context 'when noteable does not support suggestions' do + it 'returns false' do + allow(subject.noteable).to receive(:supports_suggestion?) { false } + + expect(subject.supports_suggestion?).to be(false) + end + end + + context 'when line is not suggestible' do + it 'returns false' do + allow_any_instance_of(Gitlab::Diff::Line).to receive(:suggestible?) { false } + + expect(subject.supports_suggestion?).to be(false) + end + end + end + describe "image diff notes" do let(:path) { "files/images/any_image.png" } diff --git a/spec/models/suggestion_spec.rb b/spec/models/suggestion_spec.rb new file mode 100644 index 00000000000..cafc725dddb --- /dev/null +++ b/spec/models/suggestion_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Suggestion do + let(:suggestion) { create(:suggestion) } + + describe 'associations' do + it { is_expected.to belong_to(:note) } + end + + describe 'validations' do + it { is_expected.to validate_presence_of(:note) } + + context 'when suggestion is applied' do + before do + allow(subject).to receive(:applied?).and_return(true) + end + + it { is_expected.to validate_presence_of(:commit_id) } + end + end + + describe '#appliable?' do + context 'when note does not support suggestions' do + it 'returns false' do + expect_next_instance_of(DiffNote) do |note| + allow(note).to receive(:supports_suggestion?) { false } + end + + expect(suggestion).not_to be_appliable + end + end + + context 'when patch is already applied' do + let(:suggestion) { create(:suggestion, :applied) } + + it 'returns false' do + expect(suggestion).not_to be_appliable + end + end + + context 'when merge request is not opened' do + let(:merge_request) { create(:merge_request, :merged) } + let(:note) do + create(:diff_note_on_merge_request, project: merge_request.project, + noteable: merge_request) + end + + let(:suggestion) { create(:suggestion, note: note) } + + it 'returns false' do + expect(suggestion).not_to be_appliable + end + end + end +end diff --git a/spec/requests/api/suggestions_spec.rb b/spec/requests/api/suggestions_spec.rb new file mode 100644 index 00000000000..8e9f737fbd5 --- /dev/null +++ b/spec/requests/api/suggestions_spec.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe API::Suggestions do + let(:project) { create(:project, :repository) } + let(:user) { create(:user) } + + let(:merge_request) do + create(:merge_request, source_project: project, + target_project: project) + end + + let(:position) do + Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb", + new_path: "files/ruby/popen.rb", + old_line: nil, + new_line: 9, + diff_refs: merge_request.diff_refs) + end + + let(:diff_note) do + create(:diff_note_on_merge_request, noteable: merge_request, + position: position, + project: project) + end + + describe "PUT /suggestions/:id/apply" do + let(:url) { "/suggestions/#{suggestion.id}/apply" } + + context 'when successfully applies patch' do + let(:suggestion) do + create(:suggestion, note: diff_note, + from_content: " raise RuntimeError, \"System commands must be given as an array of strings\"\n", + to_content: " raise RuntimeError, 'Explosion'\n # explosion?") + end + + it 'returns 200 with json content' do + project.add_maintainer(user) + + put api(url, user), id: suggestion.id + + expect(response).to have_gitlab_http_status(200) + expect(json_response) + .to include('id', 'from_original_line', 'to_original_line', + 'from_line', 'to_line', 'appliable', 'applied', + 'from_content', 'to_content') + end + end + + context 'when not able to apply patch' do + let(:suggestion) do + create(:suggestion, :unappliable, note: diff_note) + end + + it 'returns 400 with json content' do + project.add_maintainer(user) + + put api(url, user), id: suggestion.id + + expect(response).to have_gitlab_http_status(400) + expect(json_response).to eq({ 'message' => 'Suggestion is not appliable' }) + end + end + + context 'when unauthorized' do + let(:suggestion) do + create(:suggestion, note: diff_note, + from_content: " raise RuntimeError, \"System commands must be given as an array of strings\"\n", + to_content: " raise RuntimeError, 'Explosion'\n # explosion?") + end + + it 'returns 403 with json content' do + project.add_reporter(user) + + put api(url, user), id: suggestion.id + + expect(response).to have_gitlab_http_status(403) + expect(json_response).to eq({ 'message' => '403 Forbidden' }) + end + end + end +end diff --git a/spec/serializers/suggestion_entity_spec.rb b/spec/serializers/suggestion_entity_spec.rb new file mode 100644 index 00000000000..047571f161c --- /dev/null +++ b/spec/serializers/suggestion_entity_spec.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe SuggestionEntity do + include RepoHelpers + + let(:user) { create(:user) } + let(:request) { double('request', current_user: user) } + let(:suggestion) { create(:suggestion) } + let(:entity) { described_class.new(suggestion, request: request) } + + subject { entity.as_json } + + it 'exposes correct attributes' do + expect(subject).to include(:id, :from_original_line, :to_original_line, :from_line, + :to_line, :appliable, :applied, :from_content, :to_content) + end + + it 'exposes current user abilities' do + expect(subject[:current_user]).to include(:can_apply) + end +end diff --git a/spec/services/notes/update_service_spec.rb b/spec/services/notes/update_service_spec.rb index 533dcdcd6cd..fd9bff46a06 100644 --- a/spec/services/notes/update_service_spec.rb +++ b/spec/services/notes/update_service_spec.rb @@ -20,6 +20,29 @@ describe Notes::UpdateService do @note.reload end + context 'suggestions' do + it 'refreshes note suggestions' do + markdown = <<-MARKDOWN.strip_heredoc + ```suggestion + foo + ``` + + ```suggestion + bar + ``` + MARKDOWN + + suggestion = create(:suggestion) + note = suggestion.note + + expect { described_class.new(project, user, note: markdown).execute(note) } + .to change { note.suggestions.count }.from(1).to(2) + + expect(note.suggestions.order(:relative_order).map(&:to_content)) + .to eq([" foo\n", " bar\n"]) + end + end + context 'todos' do let!(:todo) { create(:todo, :assigned, user: user, project: project, target: issue, author: user2) } diff --git a/spec/services/preview_markdown_service_spec.rb b/spec/services/preview_markdown_service_spec.rb index b69977c812a..458cb8f1f31 100644 --- a/spec/services/preview_markdown_service_spec.rb +++ b/spec/services/preview_markdown_service_spec.rb @@ -19,6 +19,31 @@ describe PreviewMarkdownService do end end + describe 'suggestions' do + let(:params) { { text: "```suggestion\nfoo\n```", preview_suggestions: preview_suggestions } } + let(:service) { described_class.new(project, user, params) } + + context 'when preview markdown param is present' do + let(:preview_suggestions) { true } + + it 'returns users referenced in text' do + result = service.execute + + expect(result[:suggestions]).to eq(['foo']) + end + end + + context 'when preview markdown param is not present' do + let(:preview_suggestions) { false } + + it 'returns users referenced in text' do + result = service.execute + + expect(result[:suggestions]).to eq([]) + end + end + end + context 'new note with quick actions' do let(:issue) { create(:issue, project: project) } let(:params) do diff --git a/spec/services/suggestions/apply_service_spec.rb b/spec/services/suggestions/apply_service_spec.rb new file mode 100644 index 00000000000..3a483717756 --- /dev/null +++ b/spec/services/suggestions/apply_service_spec.rb @@ -0,0 +1,229 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Suggestions::ApplyService do + include ProjectForksHelper + + let(:project) { create(:project, :repository) } + let(:user) { create(:user, :commit_email) } + + let(:position) do + Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb", + new_path: "files/ruby/popen.rb", + old_line: nil, + new_line: 9, + diff_refs: merge_request.diff_refs) + end + + let(:suggestion) do + create(:suggestion, note: diff_note, + from_content: " raise RuntimeError, \"System commands must be given as an array of strings\"\n", + to_content: " raise RuntimeError, 'Explosion'\n # explosion?\n") + end + + subject { described_class.new(user) } + + context 'patch is appliable' do + let(:expected_content) do + <<-CONTENT.strip_heredoc + require 'fileutils' + require 'open3' + + module Popen + extend self + + def popen(cmd, path=nil) + unless cmd.is_a?(Array) + raise RuntimeError, 'Explosion' + # explosion? + end + + path ||= Dir.pwd + + vars = { + "PWD" => path + } + + options = { + chdir: path + } + + unless File.directory?(path) + FileUtils.mkdir_p(path) + end + + @cmd_output = "" + @cmd_status = 0 + + Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| + @cmd_output << stdout.read + @cmd_output << stderr.read + @cmd_status = wait_thr.value.exitstatus + end + + return @cmd_output, @cmd_status + end + end + CONTENT + end + + context 'non-fork project' do + let(:merge_request) do + create(:merge_request, source_project: project, + target_project: project) + end + + let!(:diff_note) do + create(:diff_note_on_merge_request, noteable: merge_request, + position: position, + project: project) + end + + before do + project.add_maintainer(user) + end + + it 'updates the file with the new contents' do + subject.execute(suggestion) + + blob = project.repository.blob_at_branch(merge_request.source_branch, + position.new_path) + + expect(blob.data).to eq(expected_content) + end + + it 'returns success status' do + result = subject.execute(suggestion) + + expect(result[:status]).to eq(:success) + end + + it 'updates suggestion applied and commit_id columns' do + expect { subject.execute(suggestion) } + .to change(suggestion, :applied) + .from(false).to(true) + .and change(suggestion, :commit_id) + .from(nil) + end + + it 'created commit has users email and name' do + subject.execute(suggestion) + + commit = project.repository.commit + + expect(user.commit_email).not_to eq(user.email) + expect(commit.author_email).to eq(user.commit_email) + expect(commit.committer_email).to eq(user.commit_email) + expect(commit.author_name).to eq(user.name) + end + end + + context 'fork-project' do + let(:project) { create(:project, :public, :repository) } + + let(:forked_project) do + fork_project_with_submodules(project, user) + end + + let(:merge_request) do + create(:merge_request, + source_branch: 'conflict-resolvable-fork', source_project: forked_project, + target_branch: 'conflict-start', target_project: project) + end + + let!(:diff_note) do + create(:diff_note_on_merge_request, noteable: merge_request, position: position, project: project) + end + + before do + project.add_maintainer(user) + end + + it 'updates file in the source project' do + expect(Files::UpdateService).to receive(:new) + .with(merge_request.source_project, user, anything) + .and_call_original + + subject.execute(suggestion) + end + end + end + + context 'no permission' do + let(:merge_request) do + create(:merge_request, source_project: project, + target_project: project) + end + + let(:diff_note) do + create(:diff_note_on_merge_request, noteable: merge_request, + position: position, + project: project) + end + + context 'user cannot write in project repo' do + before do + project.add_reporter(user) + end + + it 'returns error' do + result = subject.execute(suggestion) + + expect(result).to eq(message: "You are not allowed to push into this branch", + status: :error) + end + end + end + + context 'patch is not appliable' do + let(:merge_request) do + create(:merge_request, source_project: project, + target_project: project) + end + + let(:diff_note) do + create(:diff_note_on_merge_request, noteable: merge_request, + position: position, + project: project) + end + + before do + project.add_maintainer(user) + end + + context 'suggestion was already applied' do + it 'returns success status' do + result = subject.execute(suggestion) + + expect(result[:status]).to eq(:success) + end + end + + context 'note is outdated' do + before do + allow(diff_note).to receive(:active?) { false } + end + + it 'returns error message' do + result = subject.execute(suggestion) + + expect(result).to eq(message: 'Suggestion is not appliable', + status: :error) + end + end + + context 'suggestion was already applied' do + before do + suggestion.update!(applied: true, commit_id: 'sha') + end + + it 'returns error message' do + result = subject.execute(suggestion) + + expect(result).to eq(message: 'Suggestion is not appliable', + status: :error) + end + end + end +end diff --git a/spec/services/suggestions/create_service_spec.rb b/spec/services/suggestions/create_service_spec.rb new file mode 100644 index 00000000000..f1142c88a69 --- /dev/null +++ b/spec/services/suggestions/create_service_spec.rb @@ -0,0 +1,110 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Suggestions::CreateService do + let(:project_with_repo) { create(:project, :repository) } + let(:merge_request) do + create(:merge_request, source_project: project_with_repo, + target_project: project_with_repo) + end + + let(:position) do + Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb", + new_path: "files/ruby/popen.rb", + old_line: nil, + new_line: 14, + diff_refs: merge_request.diff_refs) + end + + let(:markdown) do + <<-MARKDOWN.strip_heredoc + ```suggestion + foo + bar + ``` + + ``` + nothing + ``` + + ```suggestion + xpto + baz + ``` + + ```thing + this is not a suggestion, it's a thing + ``` + MARKDOWN + end + + subject { described_class.new(note) } + + describe '#execute' do + context 'should not try to parse suggestions' do + context 'when not a diff note for merge requests' do + let(:note) do + create(:diff_note_on_commit, project: project_with_repo, + note: markdown) + end + + it 'does not try to parse suggestions' do + expect(Banzai::SuggestionsParser).not_to receive(:parse) + + subject.execute + end + end + + context 'when diff note is not for text' do + let(:note) do + create(:diff_note_on_merge_request, project: project_with_repo, + noteable: merge_request, + position: position, + note: markdown) + end + + it 'does not try to parse suggestions' do + allow(note).to receive(:on_text?) { false } + + expect(Banzai::SuggestionsParser).not_to receive(:parse) + + subject.execute + end + end + end + + context 'should create suggestions' do + let(:note) do + create(:diff_note_on_merge_request, project: project_with_repo, + noteable: merge_request, + position: position, + note: markdown) + end + + context 'single line suggestions' do + it 'persists suggestion records' do + expect { subject.execute } + .to change { note.suggestions.count } + .from(0) + .to(2) + end + + it 'persists original from_content lines and suggested lines' do + subject.execute + + suggestions = note.suggestions.order(:relative_order) + + suggestion_1 = suggestions.first + suggestion_2 = suggestions.last + + expect(suggestion_1).to have_attributes(from_content: " vars = {\n", + to_content: " foo\n bar\n") + + expect(suggestion_2).to have_attributes(from_content: " vars = {\n", + to_content: " xpto\n baz\n") + end + end + end + end +end |