From 20b2a0b235d01a63cb0cf47b6329b557242a3981 Mon Sep 17 00:00:00 2001 From: Jack Weeden Date: Wed, 26 Jun 2013 15:32:34 +0100 Subject: [PATCH] Ability to edit comments --- app/assets/javascripts/notes.js | 147 +++++++++++++++++- app/assets/stylesheets/sections/notes.scss | 29 ++++ app/helpers/notes_helper.rb | 7 + app/views/notes/_note.html.haml | 47 ++++-- spec/factories.rb | 7 + spec/features/notes_on_merge_requests_spec.rb | 66 ++++++++ spec/fixtures/dk.png | Bin 0 -> 1143 bytes 7 files changed, 285 insertions(+), 18 deletions(-) create mode 100644 spec/fixtures/dk.png diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index f5005ec2c94..62961b529fd 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -46,6 +46,26 @@ var NoteList = { ".js-note-delete", NoteList.removeNote); + // show the edit note form + $(document).on("click", + ".js-note-edit", + NoteList.showEditNoteForm); + + // cancel note editing + $(document).on("click", + ".note-edit-cancel", + NoteList.cancelNoteEdit); + + // delete note attachment + $(document).on("click", + ".js-note-attachment-delete", + NoteList.deleteNoteAttachment); + + // update the note after editing + $(document).on("ajax:complete", + "form.edit_note", + NoteList.updateNote); + // reset main target form after submit $(document).on("ajax:complete", ".js-main-target-form", @@ -53,12 +73,12 @@ var NoteList = { $(document).on("click", - ".js-choose-note-attachment-button", - NoteList.chooseNoteAttachment); + ".js-choose-note-attachment-button", + NoteList.chooseNoteAttachment); $(document).on("click", - ".js-show-outdated-discussion", - function(e) { $(this).next('.outdated-discussion').show(); e.preventDefault() }); + ".js-show-outdated-discussion", + function(e) { $(this).next('.outdated-discussion').show(); e.preventDefault() }); }, @@ -97,8 +117,8 @@ var NoteList = { /** * Called when clicking the "Choose File" button. - * - * Opesn the file selection dialog. + * + * Opens the file selection dialog. */ chooseNoteAttachment: function() { var form = $(this).closest("form"); @@ -133,7 +153,7 @@ var NoteList = { /** * Called in response to "cancel" on a diff note form. - * + * * Shows the reply button again. * Removes the form and if necessary it's temporary row. */ @@ -176,6 +196,59 @@ var NoteList = { NoteList.updateVotes(); }, + /** + * Called in response to clicking the edit note link + * + * Replaces the note text with the note edit form + * Adds a hidden div with the original content of the note to fill the edit note form with + * if the user cancels + */ + showEditNoteForm: function(e) { + e.preventDefault(); + var note = $(this).closest(".note"); + note.find(".note-text").hide(); + + // Show the attachment delete link + note.find(".js-note-attachment-delete").show(); + + var form = note.find(".note-edit-form"); + form.show(); + + + var textarea = form.find("textarea"); + var p = $("

").text(textarea.val()); + var hidden_div = $('
').append(p); + form.append(hidden_div); + hidden_div.hide(); + textarea.focus(); + }, + + /** + * Called in response to clicking the cancel button when editing a note + * + * Resets and hides the note editing form + */ + cancelNoteEdit: function(e) { + e.preventDefault(); + var note = $(this).closest(".note"); + NoteList.resetNoteEditing(note); + }, + + + /** + * Called in response to clicking the delete attachment link + * + * Removes the attachment wrapper view, including image tag if it exists + * Resets the note editing form + */ + deleteNoteAttachment: function() { + var note = $(this).closest(".note"); + note.find(".note-attachment").remove(); + NoteList.resetNoteEditing(note); + NoteList.rewriteTimestamp(note.find(".note-last-update")); + }, + + /** * Called when clicking on the "reply" button for a diff line. * @@ -426,5 +499,65 @@ var NoteList = { votes.find(".upvotes").text(votes.find(".upvotes").text().replace(/\d+/, upvotes)); votes.find(".downvotes").text(votes.find(".downvotes").text().replace(/\d+/, downvotes)); } + }, + + /** + * Called in response to the edit note form being submitted + * + * Updates the current note field. + * Hides the edit note form + */ + updateNote: function(e, xhr, settings) { + response = JSON.parse(xhr.responseText); + if (response.success) { + var note_li = $("#note_" + response.id); + var note_text = note_li.find(".note-text"); + note_text.html(response.note).show(); + + var note_form = note_li.find(".note-edit-form"); + note_form.hide(); + note_form.find(".btn-save").enableButton(); + + // Update the "Edited at xxx label" on the note to show it's just been updated + NoteList.rewriteTimestamp(note_li.find(".note-last-update")); + } + }, + + /** + * Called in response to the 'cancel note' link clicked, or after deleting a note attachment + * + * Hides the edit note form and shows the note + * Resets the edit note form textarea with the original content of the note + */ + resetNoteEditing: function(note) { + note.find(".note-text").show(); + + // Hide the attachment delete link + note.find(".js-note-attachment-delete").hide(); + + // Put the original content of the note back into the edit form textarea + var form = note.find(".note-edit-form"); + var original_content = form.find(".note-original-content"); + form.find("textarea").val(original_content.text()); + original_content.remove(); + + note.find(".note-edit-form").hide(); + }, + + /** + * Utility function to generate new timestamp text for a note + * + */ + rewriteTimestamp: function(element) { + // Strip all newlines from the existing timestamp + var ts = element.text().replace(/\n/g, ' ').trim(); + + // If the timestamp already has '(Edited xxx ago)' text, remove it + ts = ts.replace(new RegExp("\\(Edited [A-Za-z0-9 ]+\\)$", "gi"), ""); + + // Append "(Edited just now)" + ts = (ts + " (Edited just now)"); + + element.html(ts); } }; diff --git a/app/assets/stylesheets/sections/notes.scss b/app/assets/stylesheets/sections/notes.scss index d4bb4872ac7..bae1ac3aa9a 100644 --- a/app/assets/stylesheets/sections/notes.scss +++ b/app/assets/stylesheets/sections/notes.scss @@ -325,3 +325,32 @@ ul.notes { float: left; } } + +.note-edit-form { + display: none; + + .note_text { + border: 1px solid #DDD; + box-shadow: none; + font-size: 14px; + height: 80px; + width: 98.6%; + } + + .form-actions { + padding-left: 20px; + + .btn-save { + float: left; + } + + .note-form-option { + float: left; + padding: 2px 0 0 25px; + } + } +} + +.js-note-attachment-delete { + display: none; +} diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb index fbd0f01e5d4..a3ec4cca59d 100644 --- a/app/helpers/notes_helper.rb +++ b/app/helpers/notes_helper.rb @@ -28,4 +28,11 @@ module NotesHelper def loading_new_notes? params[:loading_new].present? end + + def note_timestamp(note) + # Shows the created at time and the updated at time if different + ts = "#{time_ago_in_words(note.created_at)} ago" + ts << content_tag(:small, " (Edited #{time_ago_in_words(note.updated_at)} ago)") if note.updated_at != note.created_at + ts.html_safe + end end diff --git a/app/views/notes/_note.html.haml b/app/views/notes/_note.html.haml index 88c450d61f9..6b08aa5afe8 100644 --- a/app/views/notes/_note.html.haml +++ b/app/views/notes/_note.html.haml @@ -6,13 +6,14 @@ Link here   - if(note.author_id == current_user.id) || can?(current_user, :admin_note, @project) - = link_to project_note_path(@project, note), title: "Remove comment", method: :delete, confirm: 'Are you sure you want to remove comment?', remote: true, class: "danger js-note-delete" do + = link_to "javascript:;", title: "Edit comment", class: "js-note-edit" do + %i.icon-edit + = link_to project_note_path(@project, note), title: "Remove comment", method: :delete, confirm: 'Are you sure you want to remove this comment?', remote: true, class: "danger js-note-delete" do %i.icon-trash.cred = image_tag gravatar_icon(note.author.email), class: "avatar s32", alt: '' = link_to_member(@project, note.author, avatar: false) %span.note-last-update - = time_ago_in_words(note.updated_at) - ago + = note_timestamp(note) - if note.upvote? %span.vote.upvote.label.label-success @@ -25,13 +26,37 @@ .note-body - = preserve do - = markdown(note.note) + .note-text + = preserve do + = markdown(note.note) + + .note-edit-form + = form_for note, url: project_note_path(@project, note), method: :put, remote: true do |f| + = f.text_area :note, class: 'note_text js-note-text js-gfm-input turn-on' + + .form-actions + = f.submit 'Save changes', class: "btn btn-primary btn-save" + + .note-form-option + %a.choose-btn.btn.btn-small.js-choose-note-attachment-button + %i.icon-paper-clip + %span Choose File ... +   + %span.file_name.js-attachment-filename File name... + = f.file_field :attachment, class: "js-note-attachment-input hide" + + = link_to 'Cancel', "javascript:;", class: "btn btn-cancel note-edit-cancel" + + - if note.attachment.url - - if note.attachment.image? - = image_tag note.attachment.url, class: 'note-image-attach' - .attachment.pull-right - = link_to note.attachment.secure_url, target: "_blank" do - %i.icon-paper-clip - = note.attachment_identifier + .note-attachment + - if note.attachment.image? + = image_tag note.attachment.url, class: 'note-image-attach' + .attachment.pull-right + = link_to note.attachment.secure_url, target: "_blank" do + %i.icon-paper-clip + = note.attachment_identifier + = link_to delete_attachment_project_note_path(@project, note), + title: "Delete this attachment", method: :delete, remote: true, confirm: 'Are you sure you want to remove the attachment?', class: "danger js-note-attachment-delete" do + %i.icon-trash.cred .clear diff --git a/spec/factories.rb b/spec/factories.rb index b596f80fa9e..bd2ec6abf62 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -1,3 +1,5 @@ +include ActionDispatch::TestProcess + FactoryGirl.define do sequence :sentence, aliases: [:title, :content] do Faker::Lorem.sentence @@ -120,6 +122,7 @@ FactoryGirl.define do factory :note_on_issue, traits: [:on_issue], aliases: [:votable_note] factory :note_on_merge_request, traits: [:on_merge_request] factory :note_on_merge_request_diff, traits: [:on_merge_request, :on_diff] + factory :note_on_merge_request_with_attachment, traits: [:on_merge_request, :with_attachment] trait :on_commit do project factory: :project_with_code @@ -141,6 +144,10 @@ FactoryGirl.define do noteable_id 1 noteable_type "Issue" end + + trait :with_attachment do + attachment { fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "image/png") } + end end factory :event do diff --git a/spec/features/notes_on_merge_requests_spec.rb b/spec/features/notes_on_merge_requests_spec.rb index 24f5437efff..d7bc66dd9c8 100644 --- a/spec/features/notes_on_merge_requests_spec.rb +++ b/spec/features/notes_on_merge_requests_spec.rb @@ -3,6 +3,7 @@ require 'spec_helper' describe "On a merge request", js: true do let!(:project) { create(:project_with_code) } let!(:merge_request) { create(:merge_request, project: project) } + let!(:note) { create(:note_on_merge_request_with_attachment, project: project) } before do login_as :user @@ -72,6 +73,71 @@ describe "On a merge request", js: true do should_not have_css(".note") end end + + describe "when editing a note", js: true do + it "should contain the hidden edit form" do + within("#note_#{note.id}") { should have_css(".note-edit-form", visible: false) } + end + + describe "editing the note" do + before do + find('.note').hover + find(".js-note-edit").click + end + + it "should show the note edit form and hide the note body" do + within("#note_#{note.id}") do + find(".note-edit-form", visible: true).should be_visible + find(".note-text", visible: false).should_not be_visible + end + end + + it "should reset the edit note form textarea with the original content of the note if cancelled" do + find('.note').hover + find(".js-note-edit").click + + within(".note-edit-form") do + fill_in "note[note]", with: "Some new content" + find(".btn-cancel").click + find(".js-note-text", visible: false).text.should == note.note + end + end + + it "appends the edited at time to the note" do + find('.note').hover + find(".js-note-edit").click + + within(".note-edit-form") do + fill_in "note[note]", with: "Some new content" + find(".btn-save").click + end + + within("#note_#{note.id}") do + should have_css(".note-last-update small") + find(".note-last-update small").text.should match(/Edited just now/) + end + end + end + + describe "deleting an attachment" do + before do + find('.note').hover + find(".js-note-edit").click + end + + it "shows the delete link" do + within(".note-attachment") do + should have_css(".js-note-attachment-delete") + end + end + + it "removes the attachment div and resets the edit form" do + find(".js-note-attachment-delete").click + should_not have_css(".note-attachment") + find(".note-edit-form", visible: false).should_not be_visible + end + end + end end describe "On a merge request diff", js: true, focus: true do diff --git a/spec/fixtures/dk.png b/spec/fixtures/dk.png new file mode 100644 index 0000000000000000000000000000000000000000..87ce25e877ab8a9602b77af019f191e6749003f9 GIT binary patch literal 1143 zcmeAS@N?(olHy`uVBq!ia0y~yU_1lFPAtqokv{(qvOr2Sz$e7@|NsBruWel`zyM?b zNgx|2^Zf5zFCfKS666=m;PCbMjb0B11{OX~7srr_xHq>iW*#;WaZ4;`e!Y;pBj*Bd z3wwtAE~jtuiXH<0Z!mesZ1oU(ZllW5T63p8h9z#@?>}dw#H0JyhD2Dg1RreS6jt+@ zVQ_cejOa5bzHH=L)?Yez;f&7xQtXj>JcoRIk3$u_-R=L(XuVTZgWvr{Um7b#H=Peq z`K`6Y@tSdPqCgrK$c*Ublk#0H+tZ$1dpp5g&%w)S>RPA0GZ$7(P)pPDIRrK5ukK8{ zuzid{s|y)_-8y|Q?b?HH|2kI2Upe%r;JwKSwq@u3^&GvzIl~~)j74}^_u2DPKFsq` zoo-~5I`y@rcf%JiA5MQC@z-arFny8tiF8j;Yw3l0U}NOWnb~T~K8b#}V@=!n?@WN5 z^qoIGj%o=|o0mP`ZqlA#e{-=<*37RjIgMVgz0e(NZILLz)nqV7?2^+4W^0c1jQrp3 zdnpJ!+hD-HS;yi}agG1P_hvhG=X_C}EV)e1jt%H8uyJR^7RrD6`Q!Q#9d75DXN-Ql zNr(CGcTmgzntc6vqG@lP8-3yn6U|sbVUzePFwiLYW!k9)x4YBZBI|iujcSj+h3Q)5 z#ChhY)|c{|#!lDUAI;2OqkI4oT&cnqlUJHt+qbC9aJD~hs7CkBXSZ}g#vN)|sd;nl zO!;r`B(^4Ma4==8Tc9M)Vbyt(HR#E|aL&9<0#g_dZu(bqFT`j^<-9}9yci)kBR}1S)aUruEs(N`|lDRj)ak_B@H|Isyf27_Xc^BmCa3c)Gc!;@?L*rUANhI^QWzP5dFsd3mToz9GkhhKY7)Zt=}$OJ9BClRI8MO{o6Ym zc^ec290J1^6fi#4&`UV@Z*_zumyl(*wq9j@Gdu6eiP}eGoOkZszt-O3?#b6P!q)Bl z5VJ5m8R~&;D?i?RvG~Le{erxcucbb3j{&;A1r%_WYR0UybP0l+XkK6K)1D literal 0 HcmV?d00001