From bad8c03abea6a4c3a6e78552678baaeb54567f4d Mon Sep 17 00:00:00 2001 From: Jazzy Gasper Date: Wed, 16 Apr 2025 22:36:25 -0700 Subject: [PATCH 1/5] Add delete functionality to member notes --- app/controllers/admin/member_notes_controller.rb | 11 +++++++++++ app/policies/member_note_policy.rb | 4 ++++ app/views/admin/member_notes/_member_note.html.haml | 4 +++- config/routes.rb | 2 +- 4 files changed, 19 insertions(+), 2 deletions(-) diff --git a/app/controllers/admin/member_notes_controller.rb b/app/controllers/admin/member_notes_controller.rb index 2f2e4f0b7..d929cd468 100644 --- a/app/controllers/admin/member_notes_controller.rb +++ b/app/controllers/admin/member_notes_controller.rb @@ -11,4 +11,15 @@ def create def member_note_params params.require(:member_note).permit(:note, :member_id) end + + def destroy + @note = MemberNote.find(params[:id]) + authorize @note + if @note.destroy + flash[:notice] = "Note successfully deleted." + else + flash[:error] = "Failed to delete note." + end + redirect_back fallback_location: root_path + end end diff --git a/app/policies/member_note_policy.rb b/app/policies/member_note_policy.rb index 7b8baa327..112e2c25b 100644 --- a/app/policies/member_note_policy.rb +++ b/app/policies/member_note_policy.rb @@ -2,4 +2,8 @@ class MemberNotePolicy < ApplicationPolicy def create? user && (user.has_role?(:admin) || user.roles.where(resource_type: 'Chapter').any?) end + + def destroy? + user && (user.has_role?(:admin) || user == record.author) + end end diff --git a/app/views/admin/member_notes/_member_note.html.haml b/app/views/admin/member_notes/_member_note.html.haml index 741db5844..c132478f5 100644 --- a/app/views/admin/member_notes/_member_note.html.haml +++ b/app/views/admin/member_notes/_member_note.html.haml @@ -6,5 +6,7 @@ .col-9.col-md-11 %strong Note added by #{link_to(action.author.full_name, admin_member_path(action.author))} %blockquote.blockquote.mb-0=action.note - .date + .d-flex.align-items-center.justify-content-between = l(action.created_at, format: :website_format) + = link_to admin_member_note_path(action), method: :delete, data: { confirm: "Are you sure you want to delete this note?" }, class: "btn btn-sm btn-outline-danger" do + %i.fas.fa-trash diff --git a/config/routes.rb b/config/routes.rb index f5b29ccb0..71e06c9ee 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -91,7 +91,7 @@ resources :bans, only: %i[index new create] end - resources :member_notes, only: [:create] + resources :member_notes, only: %i[create destroy] resources :chapters, only: %i[index new create show edit update] do get :members From cee823c78120b62c8a562a1562455f7379685c66 Mon Sep 17 00:00:00 2001 From: Jazzy Gasper Date: Wed, 23 Apr 2025 11:51:00 -0700 Subject: [PATCH 2/5] Add edit functionality to member notes --- .../admin/member_notes_controller.rb | 17 +++++++++++++++++ app/policies/member_note_policy.rb | 4 ++++ .../admin/member_notes/_member_note.html.haml | 8 +++++--- app/views/admin/members/_actions.html.haml | 2 +- app/views/admin/members/_note.html.haml | 7 ++++--- config/routes.rb | 2 +- 6 files changed, 32 insertions(+), 8 deletions(-) diff --git a/app/controllers/admin/member_notes_controller.rb b/app/controllers/admin/member_notes_controller.rb index d929cd468..d2fecbd5c 100644 --- a/app/controllers/admin/member_notes_controller.rb +++ b/app/controllers/admin/member_notes_controller.rb @@ -12,6 +12,23 @@ def member_note_params params.require(:member_note).permit(:note, :member_id) end + def edit + @note = MemberNote.find(params[:id]) + authorize @note + end + + def update + @note = MemberNote.find(params[:id]) + + if @note.update(member_note_params) + flash[:notice] = "Note updated successfully." + redirect_to admin_member_path(@note.member) + else + flash[:error] = @note.errors.full_messages unless @note.save + redirect_back fallback_location: root_path + end + end + def destroy @note = MemberNote.find(params[:id]) authorize @note diff --git a/app/policies/member_note_policy.rb b/app/policies/member_note_policy.rb index 112e2c25b..e72bdf34d 100644 --- a/app/policies/member_note_policy.rb +++ b/app/policies/member_note_policy.rb @@ -6,4 +6,8 @@ def create? def destroy? user && (user.has_role?(:admin) || user == record.author) end + + def edit? + user && (user.has_role?(:admin) || user == record.author) + end end diff --git a/app/views/admin/member_notes/_member_note.html.haml b/app/views/admin/member_notes/_member_note.html.haml index c132478f5..6546e0cf7 100644 --- a/app/views/admin/member_notes/_member_note.html.haml +++ b/app/views/admin/member_notes/_member_note.html.haml @@ -1,8 +1,9 @@ .row.d-flex.align-items-start.note .col-2.col-md-1 - %span.fa-stack.text-primary - %i.fas.fa-circle.fa-stack-2x - %i.fas.fa-pencil-alt.fa-stack-1x.fa-inverse + = link_to '#', class: "btn btn-sm p-0 me-2", turbo: false, 'data-bs-toggle': 'modal', 'data-bs-target': "#edit-note-modal-#{action.id}" do + %span.fa-stack.text-primary + %i.fas.fa-circle.fa-stack-2x + %i.fas.fa-pencil-alt.fa-stack-1x.fa-inverse .col-9.col-md-11 %strong Note added by #{link_to(action.author.full_name, admin_member_path(action.author))} %blockquote.blockquote.mb-0=action.note @@ -10,3 +11,4 @@ = l(action.created_at, format: :website_format) = link_to admin_member_note_path(action), method: :delete, data: { confirm: "Are you sure you want to delete this note?" }, class: "btn btn-sm btn-outline-danger" do %i.fas.fa-trash + = render partial: 'note', locals: { note: action, member: @member } \ No newline at end of file diff --git a/app/views/admin/members/_actions.html.haml b/app/views/admin/members/_actions.html.haml index 924a58094..94e3e7af6 100644 --- a/app/views/admin/members/_actions.html.haml +++ b/app/views/admin/members/_actions.html.haml @@ -17,4 +17,4 @@ = link_to new_admin_member_ban_path(@member), class: 'btn btn-primary d-block py-4 rounded-0' do %i.fas.fa-ban.warning.me-2 Suspend - = render 'note' + = render partial: 'note', locals: { note: MemberNote.new, member: @member } diff --git a/app/views/admin/members/_note.html.haml b/app/views/admin/members/_note.html.haml index 57e16ec9a..0e10f87ce 100644 --- a/app/views/admin/members/_note.html.haml +++ b/app/views/admin/members/_note.html.haml @@ -1,11 +1,12 @@ -#note-modal.modal.fade{ 'aria-labelledby': 'modal-title', 'aria-hidden': 'true', role: 'dialog' } +.modal.fade{ id: "#{note.persisted? ? "edit-note-modal-#{note.id}" : 'note-modal'}", 'aria-labelledby': 'modal-title', 'aria-hidden': 'true', role: 'dialog' } .modal-dialog .modal-content .modal-header - %h5.modal-title#modal-title Add a note for #{@member.full_name} + %h5.modal-title#modal-title + = note.persisted? ? "Update note for #{member.full_name}" : "Add a note for #{member.full_name}" %button.btn-close{ type: 'button', 'data-bs-dismiss': 'modal', 'aria-label': 'Close' } .modal-body - = simple_form_for [:admin, MemberNote.new], html: { class: 'form-inline' } do |f| + = simple_form_for [:admin, note], html: { class: 'form-inline' } do |f| = f.input :note, label: false, input_html: { rows: 3 }, placeholder: 'e.g. very enthusiastic student.' = f.hidden_field :member_id, value: @member.id .text-right diff --git a/config/routes.rb b/config/routes.rb index 71e06c9ee..05193f76f 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -91,7 +91,7 @@ resources :bans, only: %i[index new create] end - resources :member_notes, only: %i[create destroy] + resources :member_notes, only: [:create, :destroy, :edit, :update] resources :chapters, only: %i[index new create show edit update] do get :members From 7d3ddd8613ccbe779883b21aff49c66bc9341b57 Mon Sep 17 00:00:00 2001 From: Jazzy Gasper Date: Wed, 23 Apr 2025 12:20:01 -0700 Subject: [PATCH 3/5] Add tests for member_note_controller --- .../admin/member_notes_controller.rb | 6 +- app/policies/member_note_policy.rb | 2 +- .../admin/member_notes_controller_spec.rb | 56 +++++++++++++++++++ 3 files changed, 59 insertions(+), 5 deletions(-) diff --git a/app/controllers/admin/member_notes_controller.rb b/app/controllers/admin/member_notes_controller.rb index d2fecbd5c..c4b7c9eea 100644 --- a/app/controllers/admin/member_notes_controller.rb +++ b/app/controllers/admin/member_notes_controller.rb @@ -12,13 +12,11 @@ def member_note_params params.require(:member_note).permit(:note, :member_id) end - def edit - @note = MemberNote.find(params[:id]) - authorize @note - end + def edit; end def update @note = MemberNote.find(params[:id]) + authorize @note if @note.update(member_note_params) flash[:notice] = "Note updated successfully." diff --git a/app/policies/member_note_policy.rb b/app/policies/member_note_policy.rb index e72bdf34d..bdf428602 100644 --- a/app/policies/member_note_policy.rb +++ b/app/policies/member_note_policy.rb @@ -7,7 +7,7 @@ def destroy? user && (user.has_role?(:admin) || user == record.author) end - def edit? + def update? user && (user.has_role?(:admin) || user == record.author) end end diff --git a/spec/controllers/admin/member_notes_controller_spec.rb b/spec/controllers/admin/member_notes_controller_spec.rb index 35f9b0a0d..764073b1f 100644 --- a/spec/controllers/admin/member_notes_controller_spec.rb +++ b/spec/controllers/admin/member_notes_controller_spec.rb @@ -35,4 +35,60 @@ end.not_to change { MemberNote.all.count } end end + + describe 'PATCH #update' do + let!(:member_note) { Fabricate(:member_note, member: member, author: admin, note: 'Original note') } + + it "Doesn't allow anonymous users to edit notes" do + patch :update, params: { id: member_note.id, member_note: { note: 'Updated anonymously' } } + expect(member_note.reload.note).to eq('Original note') + end + + it "Doesn't allow regular users to edit notes" do + login member + + patch :update, params: { id: member_note.id, member_note: { note: 'Updated by member' } } + expect(member_note.reload.note).to eq('Original note') + end + + it 'Allows admin to edit notes' do + login admin + + patch :update, params: { id: member_note.id, member_note: { note: 'Updated by admin' } } + expect(member_note.reload.note).to eq('Updated by admin') + end + + it "Doesn't allow notes to be updated to be blank" do + login admin + + patch :update, params: { id: member_note.id, member_note: { note: '' } } + expect(member_note.reload.note).to eq('Original note') + end + end + + describe 'DELETE #destroy' do + let!(:member_note) { Fabricate(:member_note, member: member, author: admin, note: 'Note') } + + it "Doesn't allow anonymous users to delete notes" do + expect do + delete :destroy, params: { id: member_note.id } + end.not_to change { MemberNote.all.count } + end + + it "Doesn't allow regular users to delete notes" do + login member + + expect do + delete :destroy, params: { id: member_note.id } + end.not_to change { MemberNote.all.count } + end + + it 'Allows note owner to delete notes' do + login admin + + expect do + delete :destroy, params: { id: member_note.id } + end.to change { MemberNote.count }.by(-1) + end + end end From 7647f905189ef654d0ed91b2af3221de9e7a3092 Mon Sep 17 00:00:00 2001 From: Jazzy Gasper Date: Thu, 24 Apr 2025 14:15:51 -0700 Subject: [PATCH 4/5] rubocop fixes --- .../admin/member_notes_controller.rb | 28 ++++++++++--------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/app/controllers/admin/member_notes_controller.rb b/app/controllers/admin/member_notes_controller.rb index c4b7c9eea..06818ad4f 100644 --- a/app/controllers/admin/member_notes_controller.rb +++ b/app/controllers/admin/member_notes_controller.rb @@ -1,4 +1,6 @@ class Admin::MemberNotesController < Admin::ApplicationController + before_action :authorize_note, only: [:update, :destroy] + def create @note = MemberNote.new(member_note_params) authorize @note @@ -8,18 +10,11 @@ def create redirect_back fallback_location: root_path end - def member_note_params - params.require(:member_note).permit(:note, :member_id) - end - def edit; end - + def update - @note = MemberNote.find(params[:id]) - authorize @note - if @note.update(member_note_params) - flash[:notice] = "Note updated successfully." + flash[:notice] = 'Note successfully updated.' redirect_to admin_member_path(@note.member) else flash[:error] = @note.errors.full_messages unless @note.save @@ -28,13 +23,20 @@ def update end def destroy - @note = MemberNote.find(params[:id]) - authorize @note if @note.destroy - flash[:notice] = "Note successfully deleted." + flash[:notice] = 'Note successfully deleted.' else - flash[:error] = "Failed to delete note." + flash[:error] = 'Failed to delete note.' end redirect_back fallback_location: root_path end + + def authorize_note + @note = MemberNote.find(params[:id]) + authorize @note + end + + def member_note_params + params.require(:member_note).permit(:note, :member_id) + end end From 6449fc9336703431b99edfbc918405c69f753109 Mon Sep 17 00:00:00 2001 From: Jazzy Gasper Date: Thu, 24 Apr 2025 14:19:25 -0700 Subject: [PATCH 5/5] rubocop fix --- app/controllers/admin/member_notes_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/admin/member_notes_controller.rb b/app/controllers/admin/member_notes_controller.rb index 06818ad4f..45aa8662d 100644 --- a/app/controllers/admin/member_notes_controller.rb +++ b/app/controllers/admin/member_notes_controller.rb @@ -1,5 +1,5 @@ class Admin::MemberNotesController < Admin::ApplicationController - before_action :authorize_note, only: [:update, :destroy] + before_action :authorize_note, only: %i[update destroy] def create @note = MemberNote.new(member_note_params)