From c00917e48fcfb45c686a164fe706a9c17e6797da Mon Sep 17 00:00:00 2001 From: Jack Weeden Date: Tue, 25 Jun 2013 09:49:09 +0100 Subject: [PATCH 1/8] Don't show 'Filter by labels' text if there are no labels Don't show project search form if there are no projects --- app/views/dashboard/projects.html.haml | 28 ++++++++++++++------------ 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/app/views/dashboard/projects.html.haml b/app/views/dashboard/projects.html.haml index 9b16db340b2..25239105e3b 100644 --- a/app/views/dashboard/projects.html.haml +++ b/app/views/dashboard/projects.html.haml @@ -20,22 +20,24 @@ = nav_tab :scope, 'joined' do = link_to "Joined", projects_dashboard_path(scope: 'joined') - %p.light Filter by label: - %ul.bordered-list - - @labels.each do |label| - %li{ class: (label.name == params[:label]) ? 'active' : 'light' } - = link_to projects_dashboard_path(scope: params[:scope], label: label.name) do - %i.icon-tag - = label.name + - if @labels.any? + %p.light Filter by label: + %ul.bordered-list + - @labels.each do |label| + %li{ class: (label.name == params[:label]) ? 'active' : 'light' } + = link_to projects_dashboard_path(scope: params[:scope], label: label.name) do + %i.icon-tag + = label.name .span9 - = form_tag projects_dashboard_path, method: 'get' do - %fieldset.dashboard-search-filter - = hidden_field_tag "scope", params[:scope] - = search_field_tag "search", params[:search], { id: 'dashboard_projects_search', placeholder: 'Search', class: 'left input-xxlarge'} - = button_tag type: 'submit', class: 'btn' do - %i.icon-search + - if @projects.any? + = form_tag projects_dashboard_path, method: 'get' do + %fieldset.dashboard-search-filter + = hidden_field_tag "scope", params[:scope] + = search_field_tag "search", params[:search], { id: 'dashboard_projects_search', placeholder: 'Search', class: 'left input-xxlarge'} + = button_tag type: 'submit', class: 'btn' do + %i.icon-search %ul.bordered-list - @projects.each do |project| -- GitLab From 6a84d926724d60c208d101525a83ab832ccfcdec Mon Sep 17 00:00:00 2001 From: Jack Weeden Date: Tue, 25 Jun 2013 23:46:07 +0100 Subject: [PATCH 2/8] Added update and delete_attachment actions to note controller --- app/controllers/notes_controller.rb | 26 ++++++++++++++++++++++++++ config/routes.rb | 5 ++++- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/app/controllers/notes_controller.rb b/app/controllers/notes_controller.rb index 15ca963f281..dbec660b761 100644 --- a/app/controllers/notes_controller.rb +++ b/app/controllers/notes_controller.rb @@ -38,6 +38,32 @@ class NotesController < ProjectResourceController end end + def update + @note = @project.notes.find(params[:id]) + return access_denied! unless can?(current_user, :admin_note, @note) + + @note.update_attributes(params[:note]) + + respond_to do |format| + format.js do + render js: { success: @note.valid?, id: @note.id, note: view_context.markdown(@note.note) }.to_json + end + format.html do + redirect_to :back + end + end + end + + def delete_attachment + @note = @project.notes.find(params[:id]) + @note.remove_attachment! + @note.update_attribute(:attachment, nil) + + respond_to do |format| + format.js { render nothing: true } + end + end + def preview render text: view_context.markdown(params[:note]) end diff --git a/config/routes.rb b/config/routes.rb index 39c79635c40..f545ad82f77 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -319,7 +319,10 @@ Gitlab::Application.routes.draw do end end - resources :notes, only: [:index, :create, :destroy] do + resources :notes, only: [:index, :create, :destroy, :update] do + member do + delete :delete_attachment + end collection do post :preview end -- GitLab From b62c9e5991dae0e4a8ae8e947b438cdd4e753c79 Mon Sep 17 00:00:00 2001 From: Jack Weeden Date: Wed, 26 Jun 2013 10:27:49 +0000 Subject: [PATCH 3/8] Prevent 'toplevel constant SnippetsController referenced by Project::SnippetsController' warning --- spec/routing/project_routing_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/routing/project_routing_spec.rb b/spec/routing/project_routing_spec.rb index a716f13a4cd..0b6528bfc81 100644 --- a/spec/routing/project_routing_spec.rb +++ b/spec/routing/project_routing_spec.rb @@ -262,7 +262,7 @@ end # project_snippet GET /:project_id/snippets/:id(.:format) snippets#show # PUT /:project_id/snippets/:id(.:format) snippets#update # DELETE /:project_id/snippets/:id(.:format) snippets#destroy -describe Projects::SnippetsController, "routing" do +describe SnippetsController, "routing" do it "to #raw" do get("/gitlabhq/snippets/1/raw").should route_to('projects/snippets#raw', project_id: 'gitlabhq', id: '1') end -- GitLab From 20b2a0b235d01a63cb0cf47b6329b557242a3981 Mon Sep 17 00:00:00 2001 From: Jack Weeden Date: Wed, 26 Jun 2013 15:32:34 +0100 Subject: [PATCH 4/8] 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_8UX!05!n%)r3V=l?;Lfq_9Zz$e7@|NsBruWel` z!0`Y7|Fr@P->+?DU|@Lucdi!$0|RqOkY6x^!`I_CdOa8zSol0$978JN-rT;JdDuY2 zEwP;W^+N8BoC~}y>>2XAoW98`dIsY)kEyLjVeoP&7Jlbmbi7l|D25ykM3U^ z5@E#>e6WR6Sj}gK!QFW?qR*W8vXN_9f9c$XGdlN6u}A9h9P;r!4ps1WxBoMv^-fU@ ze)kuBX{;38bUr}kx7HHJYsSHe0%=?zGoqVM%6GMFPkVOl?F4f@2QR0oYn}GaTv#-4>}YY)Et>sT3o<U4PU%`IQ@OZU!S?c^hMq$(mg?~r5Eafjgd2FW~(jx zB>LTsHErj=GXZwecmDV|swF^eUiN&uNqc_%&BZ=hGrzv%Gr|a#HW@fKZJ^%@>RAGzBD^0HLTU2H^+n+a7 zqkHGGTe?1ASFF^$xpt=fw|5d-6E!%PGS)3n66dh$JjojLv-X?)5j0ZRUtGO3q zw4-v~q2}H%?#lvBuPu4rMTIP&@ogUl21=LUcd}dCY7sMBa?raxCZU4tx9Pn|4 z=ub*&V=TEGH6h`xeDzruW!caBS^V|=>0-~cbIETk8h!$eNn&YYSB*6DLb%EA8aosGN=3IYy+;R^~FA8Y6(9Q?OBLXu0!vRhlPvc8#}_vA$F zBQnlA_wHY7Z*lkJ>ltC|c7BLi7@iFEz_yhiZ@yT3VuyY~-pSWepSQ;(B|U2a1)Qat uG3zWjhsIU$Dr#VmR^f1^ZaVJ-xtvwLXLkJEsms8?z~JfX=d#Wzp$PyJZU!&_ literal 0 HcmV?d00001 -- GitLab From 1216a62e3ba2e5c9d959fca748136661755d72e8 Mon Sep 17 00:00:00 2001 From: Jack Weeden Date: Wed, 26 Jun 2013 16:01:59 +0100 Subject: [PATCH 5/8] Authorize user on note update --- app/controllers/notes_controller.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/controllers/notes_controller.rb b/app/controllers/notes_controller.rb index dbec660b761..6acdd2afae3 100644 --- a/app/controllers/notes_controller.rb +++ b/app/controllers/notes_controller.rb @@ -2,6 +2,7 @@ class NotesController < ProjectResourceController # Authorize before_filter :authorize_read_note! before_filter :authorize_write_note!, only: [:create] + before_filter :authorize_modify_note!, only: [:update] respond_to :js -- GitLab From 22c00e64626791bf138a7691205d3e6e89cd315c Mon Sep 17 00:00:00 2001 From: Jack Weeden Date: Wed, 26 Jun 2013 16:25:24 +0100 Subject: [PATCH 6/8] Removed before_filter as authentication is done within update method --- app/controllers/notes_controller.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/controllers/notes_controller.rb b/app/controllers/notes_controller.rb index 6acdd2afae3..dbec660b761 100644 --- a/app/controllers/notes_controller.rb +++ b/app/controllers/notes_controller.rb @@ -2,7 +2,6 @@ class NotesController < ProjectResourceController # Authorize before_filter :authorize_read_note! before_filter :authorize_write_note!, only: [:create] - before_filter :authorize_modify_note!, only: [:update] respond_to :js -- GitLab From af81ff4e098241fae4f61ddd3542c44a457fba2c Mon Sep 17 00:00:00 2001 From: Jack Weeden Date: Sun, 30 Jun 2013 17:44:59 +0100 Subject: [PATCH 7/8] Changed 'javascript:;' links to '#' --- app/views/notes/_note.html.haml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/notes/_note.html.haml b/app/views/notes/_note.html.haml index 6b08aa5afe8..68137960b74 100644 --- a/app/views/notes/_note.html.haml +++ b/app/views/notes/_note.html.haml @@ -6,7 +6,7 @@ Link here   - if(note.author_id == current_user.id) || can?(current_user, :admin_note, @project) - = link_to "javascript:;", title: "Edit comment", class: "js-note-edit" do + = link_to "#", 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 @@ -45,7 +45,7 @@ %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" + = link_to 'Cancel', "#", class: "btn btn-cancel note-edit-cancel" - if note.attachment.url -- GitLab From 6fa06893e9f165a05e88330a835e30b8c084e9bb Mon Sep 17 00:00:00 2001 From: Jack Weeden Date: Tue, 25 Jun 2013 23:46:07 +0100 Subject: [PATCH 8/8] Add editable notes feature --- app/views/notes/_note.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/notes/_note.html.haml b/app/views/notes/_note.html.haml index 68137960b74..6089b9505b2 100644 --- a/app/views/notes/_note.html.haml +++ b/app/views/notes/_note.html.haml @@ -59,4 +59,4 @@ = 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 + .clear \ No newline at end of file -- GitLab