From 9e5ac7285cc6b9b641d863457086b2e5292699e1 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Wed, 9 Dec 2015 11:30:21 -0200 Subject: Ensure notes are replaced on a merge request diff when they're updated --- app/assets/javascripts/notes.js.coffee | 2 +- features/project/merge_requests.feature | 10 ++++++++++ features/steps/project/merge_requests.rb | 25 +++++++++++++++++++++++++ 3 files changed, 36 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/notes.js.coffee b/app/assets/javascripts/notes.js.coffee index 533d00bfb0c..5fe0318de6f 100644 --- a/app/assets/javascripts/notes.js.coffee +++ b/app/assets/javascripts/notes.js.coffee @@ -286,7 +286,7 @@ class @Notes $html.find('.js-task-list-container').taskList('enable') # Find the note's `li` element by ID and replace it with the updated HTML - $note_li = $("#note_#{note.id}") + $note_li = $('.note-row-' + note.id) $note_li.replaceWith($html) ### diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature index 6cd081c868e..5a7f7f7c642 100644 --- a/features/project/merge_requests.feature +++ b/features/project/merge_requests.feature @@ -83,6 +83,16 @@ Feature: Project Merge Requests And I switch to the merge request's comments tab Then I should see a discussion has started on diff + @javascript + Scenario: I edit a comment on a merge request diff + Given project "Shop" have "Bug NS-05" open merge request with diffs inside + And I visit merge request page "Bug NS-05" + And I click on the Changes tab + And I leave a comment like "Line is wrong" on diff + And I change the comment "Line is wrong" to "Typo, please fix" on diff + Then I should not see a diff comment saying "Line is wrong" + And I should see a diff comment saying "Typo, please fix" + @javascript Scenario: I comment on a line of a commit in merge request Given project "Shop" have "Bug NS-05" open merge request with diffs inside diff --git a/features/steps/project/merge_requests.rb b/features/steps/project/merge_requests.rb index 822cf0ffe1c..74539d0b019 100644 --- a/features/steps/project/merge_requests.rb +++ b/features/steps/project/merge_requests.rb @@ -186,6 +186,31 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps leave_comment "Line is wrong" end + step 'I change the comment "Line is wrong" to "Typo, please fix" on diff' do + page.within('.diff-file:nth-of-type(5) .note') do + find('.js-note-edit').click + + page.within('.current-note-edit-form', visible: true) do + fill_in 'note_note', with: 'Typo, please fix' + click_button 'Save Comment' + end + + expect(page).not_to have_button 'Save Comment', disabled: true, visible: true + end + end + + step 'I should not see a diff comment saying "Line is wrong"' do + page.within('.diff-file:nth-of-type(5) .note') do + expect(page).not_to have_visible_content 'Line is wrong' + end + end + + step 'I should see a diff comment saying "Typo, please fix"' do + page.within('.diff-file:nth-of-type(5) .note') do + expect(page).to have_visible_content 'Typo, please fix' + end + end + step 'I should see a discussion has started on diff' do page.within(".notes .discussion") do page.should have_content "#{current_user.name} started a discussion" -- cgit v1.2.1 From 05f06955bad652fa68901d620c32b30969237c69 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Wed, 9 Dec 2015 11:59:32 -0200 Subject: Ensure existing notes are highlighted properly on a merge request diff --- app/assets/javascripts/merge_request_tabs.js.coffee | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/merge_request_tabs.js.coffee b/app/assets/javascripts/merge_request_tabs.js.coffee index b0eeb1db536..1f0b6d9ced0 100644 --- a/app/assets/javascripts/merge_request_tabs.js.coffee +++ b/app/assets/javascripts/merge_request_tabs.js.coffee @@ -144,7 +144,9 @@ class @MergeRequestTabs @_get url: "#{source}.json" + @_location.search success: (data) => - document.getElementById('diffs').innerHTML = data.html + html = $(data.html) + html.syntaxHighlight() + $('#diffs').html(html) @diffsLoaded = true @scrollToElement("#diffs") -- cgit v1.2.1 From 429932a8b40540faa4d5cbe39119bfeccddd7bab Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Wed, 9 Dec 2015 12:16:24 -0200 Subject: Ensure new notes are highlighted properly on a merge request diff --- app/assets/javascripts/notes.js.coffee | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/notes.js.coffee b/app/assets/javascripts/notes.js.coffee index 5fe0318de6f..b1df56b24fe 100644 --- a/app/assets/javascripts/notes.js.coffee +++ b/app/assets/javascripts/notes.js.coffee @@ -148,6 +148,8 @@ class @Notes @note_ids.push(note.id) form = $("form[rel='" + note.discussion_id + "']") row = form.closest("tr") + note_html = $(note.html) + note_html.syntaxHighlight() # is this the first note of discussion? if row.is(".js-temp-notes-holder") @@ -158,14 +160,16 @@ class @Notes row.next().find(".note").remove() # Add note to 'Changes' page discussions - $(".notes[rel='" + note.discussion_id + "']").append note.html + $(".notes[rel='" + note.discussion_id + "']").append note_html # Init discussion on 'Discussion' page if it is merge request page if $('body').attr('data-page').indexOf('projects:merge_request') == 0 - $('ul.main-notes-list').append(note.discussion_with_diff_html) + discussion_html = $(note.discussion_with_diff_html) + discussion_html.syntaxHighlight() + $('ul.main-notes-list').append(discussion_html) else # append new note to all matching discussions - $(".notes[rel='" + note.discussion_id + "']").append note.html + $(".notes[rel='" + note.discussion_id + "']").append note_html # cleanup after successfully creating a diff/discussion note @removeDiscussionNoteForm(form) -- cgit v1.2.1 From 364041e7ddf56a16a90f35ec85e35c05e5758621 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Wed, 9 Dec 2015 14:48:00 -0200 Subject: Update CHANGELOG --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index 59fe30746c6..08a40d8d4d0 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -26,6 +26,7 @@ v 8.3.0 (unreleased) - Add languages page to graphs - Block LDAP user when they are no longer found in the LDAP server - Improve wording on project visibility levels (Zeger-Jan van de Weg) + - Fix editing notes on a merge request diff v 8.2.3 - Fix application settings cache not expiring after changes (Stan Hu) -- cgit v1.2.1 From 0c54c9a8a3cb025d7a7aec7aaf8ab17f61bb6ed6 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Thu, 10 Dec 2015 13:42:10 -0200 Subject: USe innerHtml to avoid slow performance on a MR with very large diff --- app/assets/javascripts/merge_request_tabs.js.coffee | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/merge_request_tabs.js.coffee b/app/assets/javascripts/merge_request_tabs.js.coffee index 1f0b6d9ced0..e4fa03c0ac5 100644 --- a/app/assets/javascripts/merge_request_tabs.js.coffee +++ b/app/assets/javascripts/merge_request_tabs.js.coffee @@ -144,9 +144,8 @@ class @MergeRequestTabs @_get url: "#{source}.json" + @_location.search success: (data) => - html = $(data.html) - html.syntaxHighlight() - $('#diffs').html(html) + document.getElementById('diffs').innerHTML = data.html + $('div#diffs .js-syntax-highlight').syntaxHighlight() @diffsLoaded = true @scrollToElement("#diffs") -- cgit v1.2.1 From 67913670dcc6ca228654f2c6e14d0992636c5bb7 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Thu, 10 Dec 2015 13:50:31 -0200 Subject: Make selectors more specific when setting the tab content Reason: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/2023 --- app/assets/javascripts/merge_request_tabs.js.coffee | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/merge_request_tabs.js.coffee b/app/assets/javascripts/merge_request_tabs.js.coffee index e4fa03c0ac5..79ef746d57f 100644 --- a/app/assets/javascripts/merge_request_tabs.js.coffee +++ b/app/assets/javascripts/merge_request_tabs.js.coffee @@ -133,7 +133,7 @@ class @MergeRequestTabs @_get url: "#{source}.json" success: (data) => - document.getElementById('commits').innerHTML = data.html + document.querySelector("div#commits").innerHTML = data.html $('.js-timeago').timeago() @commitsLoaded = true @scrollToElement("#commits") @@ -144,7 +144,7 @@ class @MergeRequestTabs @_get url: "#{source}.json" + @_location.search success: (data) => - document.getElementById('diffs').innerHTML = data.html + document.querySelector("div#diffs").innerHTML = data.html $('div#diffs .js-syntax-highlight').syntaxHighlight() @diffsLoaded = true @scrollToElement("#diffs") @@ -155,7 +155,7 @@ class @MergeRequestTabs @_get url: "#{source}.json" success: (data) => - document.getElementById('builds').innerHTML = data.html + document.querySelector("div#builds").innerHTML = data.html $('.js-timeago').timeago() @buildsLoaded = true @scrollToElement("#builds") -- cgit v1.2.1 From 9d6e5f0a28cda3dcbf2dcbcb8a869c0873ea3afc Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Thu, 10 Dec 2015 15:42:00 -0200 Subject: Make selectors more specific when scroll to element --- app/assets/javascripts/merge_request_tabs.js.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/merge_request_tabs.js.coffee b/app/assets/javascripts/merge_request_tabs.js.coffee index 79ef746d57f..9e2dc1250c9 100644 --- a/app/assets/javascripts/merge_request_tabs.js.coffee +++ b/app/assets/javascripts/merge_request_tabs.js.coffee @@ -77,7 +77,7 @@ class @MergeRequestTabs scrollToElement: (container) -> if window.location.hash - $el = $("#{container} #{window.location.hash}") + $el = $("div#{container} #{window.location.hash}") $('body').scrollTo($el.offset().top) if $el.length # Activate a tab based on the current action -- cgit v1.2.1