Edit and Delete Member notes

app/controllers/admin/member_notes_controller.rb

@@ -1,4 +1,5 @@
 class Admin::MemberNotesController < Admin::ApplicationController
+  before_action :authorize_note, only: %i[update destroy]
   def create
     @note = MemberNote.new(member_note_params)
     authorize @note
@@ -8,12 +9,42 @@ def create
       MemberActivityRecorder.record(actor: current_user, key: 'member_note.created',
                                     trackable: @note, recipient: @note.member)
     else
-      flash[:error] = @note.errors.full_messages
+      flash[:error] = @note.errors.full_messages.to_sentence
     end
     redirect_back fallback_location: root_path
   end
 
   def member_note_params
     params.expect(member_note: %i[note member_id])
   end
+
+  # member_id is create-only: updating must not move a note to another member,
+  # because authorization was checked against the note's original member.
+  def update_note_params
+    params.expect(member_note: %i[note])
+  end
+
+  def update
+    if @note.update(update_note_params)
+      flash[:notice] = 'Note successfully updated.'
+      redirect_to admin_member_path(@note.member)
+    else
+      flash[:error] = @note.errors.full_messages.to_sentence
+      redirect_back fallback_location: root_path
+    end
+  end
+
+  def destroy
+    if @note.destroy
+      flash[:notice] = 'Note successfully deleted.'
+    else
+      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
 end

app/policies/member_note_policy.rb

@@ -2,4 +2,24 @@ class MemberNotePolicy < ApplicationPolicy
   def create?
     user && (user.has_role?(:admin) || user.roles.where(resource_type: 'Chapter').any?)
   end
+
+  def update?
+    author_or_chapter_organiser?
+  end
+
+  def destroy?
+    author_or_chapter_organiser?
+  end
+
+  private
+
+  def author_or_chapter_organiser?
+    return false unless user
+
+    user.has_role?(:admin) || user == record.author || organiser_of_member_chapter?
+  end
+
+  def organiser_of_member_chapter?
+    record.member&.chapters&.any? { |chapter| user.has_role?(:organiser, chapter) }
+  end
 end

app/views/admin/member_notes/_member_note.html.haml

@@ -1,10 +1,16 @@
 .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
+    - if policy(action).update?
+      = 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
-    .date
+    .d-flex.align-items-center.justify-content-between
       = l(action.created_at, format: :website_format)
+      - if policy(action).destroy?
+        = 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 }

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 }

app/views/admin/members/_note.html.haml

@@ -1,12 +1,15 @@
-#note-modal.modal.fade{ 'aria-labelledby': 'modal-title', 'aria-hidden': 'true', role: 'dialog' }
+- modal_id = note.persisted? ? "edit-note-modal-#{note.id}" : 'note-modal'
+.modal.fade{ id: modal_id, 'aria-labelledby': "#{modal_id}-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{ id: "#{modal_id}-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|
-          = f.input :note, label: false, input_html: { rows: 3 }, placeholder: 'e.g. very enthusiastic student.'
+        = simple_form_for [:admin, note], html: { class: 'form-inline' } do |f|
+          - input_id = note.persisted? ? "member_note_note_#{note.id}" : 'member_note_note'
+          = f.input :note, label: false, input_html: { rows: 3, id: input_id }, placeholder: 'e.g. very enthusiastic student.'
           = f.hidden_field :member_id, value: @member.id
           .text-right
             = f.button :button, 'Save note', class: 'btn btn-primary mb-0'

config/routes.rb

@@ -104,7 +104,7 @@
       resources :bans, only: %i[index new create]
     end
 
-    resources :member_notes, only: [:create]
+    resources :member_notes, only: %i[create update destroy]
 
     resources :chapters, only: %i[index new create show edit update] do
       get :members

spec/controllers/admin/member_notes_controller_spec.rb

@@ -2,9 +2,20 @@
 
 RSpec.describe Admin::MemberNotesController do
   let(:member) { Fabricate(:member) }
-  let(:admin) { Fabricate(:chapter_organiser) }
+  let(:chapter) { Fabricate(:chapter) }
+  let(:organiser) { Fabricate(:member).tap { |m| m.add_role(:organiser, chapter) } }
+  let(:other_chapter_organiser) { Fabricate(:chapter_organiser) }
+  let(:admin) { Fabricate(:member).tap { |m| m.add_role(:admin) } }
+  # Notes are created by admins/organisers, and the admin area bounces anyone
+  # without an admin or organiser role, so author tests need an organising
+  # author whose chapter is not the member's.
+  let(:author) { Fabricate(:chapter_organiser) }
   let!(:member_note) { Fabricate(:member_note) }
 
+  before do
+    Fabricate(:students, chapter:, members: [member])
+  end
+
   describe 'POST #create' do
     it "Doesn't allow anonymous users to create notes" do
       expect do
@@ -21,7 +32,7 @@
     end
 
     it 'Allows chapter organisers to create notes' do
-      login admin
+      login organiser
       request.env['HTTP_REFERER'] = '/admin/member/3'
 
       expect do
@@ -36,13 +47,122 @@
     end
 
     it 'records member_note.created' do
-      member = Fabricate(:member)
-      login admin
+      login organiser
       request.env['HTTP_REFERER'] = '/admin/member/3'
 
       post :create, params: { member_note: { member_id: member.id, note: 'context' } }
 
       expect(PublicActivity::Activity.exists?(key: 'member_note.created', recipient: member)).to be(true)
     end
   end
+
+  describe 'PATCH #update' do
+    let!(:member_note) { Fabricate(:member_note, member:, author:, 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 "Doesn't allow organisers from other chapters to edit notes" do
+      login other_chapter_organiser
+
+      patch :update, params: { id: member_note.id, member_note: { note: 'Updated by other organiser' } }
+      expect(member_note.reload.note).to eq('Original note')
+    end
+
+    it 'Allows admins 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 'Allows organisers of the member\'s chapter to edit notes' do
+      login organiser
+
+      patch :update, params: { id: member_note.id, member_note: { note: 'Updated by organiser' } }
+      expect(member_note.reload.note).to eq('Updated by organiser')
+    end
+
+    it 'Allows the note author to edit notes' do
+      login author
+
+      patch :update, params: { id: member_note.id, member_note: { note: 'Updated by author' } }
+      expect(member_note.reload.note).to eq('Updated by author')
+    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
+
+    it "Doesn't allow member_id to be changed on update" do
+      other_member = Fabricate(:member)
+      login organiser
+
+      patch :update, params: { id: member_note.id, member_note: { note: 'Updated by organiser', member_id: other_member.id } }
+      expect(member_note.reload.member).to eq(member)
+      expect(member_note.reload.note).to eq('Updated by organiser')
+    end
+  end
+
+  describe 'DELETE #destroy' do
+    let!(:member_note) { Fabricate(:member_note, member:, author:, 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 "Doesn't allow organisers from other chapters to delete notes" do
+      login other_chapter_organiser
+
+      expect do
+        delete :destroy, params: { id: member_note.id }
+      end.not_to(change { MemberNote.all.count })
+    end
+
+    it 'Allows admins to delete notes' do
+      login admin
+
+      expect do
+        delete :destroy, params: { id: member_note.id }
+      end.to change(MemberNote, :count).by(-1)
+    end
+
+    it 'Allows organisers of the member\'s chapter to delete notes' do
+      login organiser
+
+      expect do
+        delete :destroy, params: { id: member_note.id }
+      end.to change(MemberNote, :count).by(-1)
+    end
+
+    it 'Allows the note author to delete notes' do
+      login author
+
+      expect do
+        delete :destroy, params: { id: member_note.id }
+      end.to change(MemberNote, :count).by(-1)
+    end
+  end
 end

spec/features/admin/members_spec.rb

@@ -95,4 +95,53 @@
       end
     end
   end
+
+  describe 'editing and deleting member notes' do
+    let(:global_admin) { Fabricate(:member).tap { |m| m.add_role(:admin) } }
+    let(:other_chapter_organiser) { Fabricate(:chapter_organiser) }
+    let!(:member_note) do
+      Fabricate(:member_note, member:, author: Fabricate(:member), note: 'Original note text')
+    end
+
+    before do
+      login(global_admin)
+      visit admin_member_path(member)
+    end
+
+    it 'can edit an existing note via the modal', :js do
+      within '.note' do
+        find("a[data-bs-target='#edit-note-modal-#{member_note.id}']").click
+      end
+      fill_in "member_note_note_#{member_note.id}", with: 'Revised note text'
+      click_on 'Save note'
+
+      expect(page).to have_text 'Note successfully updated.'
+      within '.note' do
+        expect(page).to have_text 'Revised note text'
+      end
+    end
+
+    it 'can delete an existing note', :js do
+      expect do
+        accept_confirm do
+          within '.note' do
+            find('a.btn-outline-danger').click
+          end
+        end
+      end.to change { member.member_notes.count }.by(-1)
+
+      expect(page).to have_text 'Note successfully deleted.'
+      expect(page).not_to have_text 'Original note text'
+    end
+
+    it 'hides edit and delete controls from organisers who may not modify the note' do
+      login(other_chapter_organiser)
+      visit admin_member_path(member)
+
+      within '.note' do
+        expect(page).not_to have_css("a[data-bs-target='#edit-note-modal-#{member_note.id}']")
+        expect(page).not_to have_css('a.btn-outline-danger')
+      end
+    end
+  end
 end