Skip to content

Edit and Delete Member notes #2208

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
28 changes: 28 additions & 0 deletions app/controllers/admin/member_notes_controller.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
class Admin::MemberNotesController < Admin::ApplicationController
before_action :authorize_note, only: %i[update destroy]

def create
@note = MemberNote.new(member_note_params)
authorize @note
Expand All @@ -8,6 +10,32 @@ def create
redirect_back fallback_location: root_path
end

def edit; end

def update
if @note.update(member_note_params)
flash[:notice] = 'Note successfully updated.'
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
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

def member_note_params
params.require(:member_note).permit(:note, :member_id)
end
Expand Down
8 changes: 8 additions & 0 deletions app/policies/member_note_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,12 @@ 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

def update?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jazzygasper just one quick question. Is the intention here to allow admins and organisers to update the note of another admin/organiser? In that case this won't work for organisers, only admins. See line 3 above in the create? method. We need to check for the admin and organiser roles to allow for that.

If the intention was to allow admins to update anyone's notes but organisers to only update their own notes, this is fine.

Just wanted to clarify before I approved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matyikriszta Yeah my original idea was for admins to update anyone’s notes, and organisers only their own. But I'm happy to change it so any organiser can update any note, or restrict it to organisers updating notes from their own chapter. What do you think is best?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jazzygasper I think we should go with the second option, e.g. organisers updating notes from their own chapter. I think this might be useful in the case if an organiser leaves but a note they left needs to be updated. Other organisers from the same chapter should be able to do this without having to ping an admin.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jazzygasper are you okay to make the suggested changes?

user && (user.has_role?(:admin) || user == record.author)
end
end
12 changes: 8 additions & 4 deletions app/views/admin/member_notes/_member_note.html.haml
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
.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
.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
= render partial: 'note', locals: { note: action, member: @member }
2 changes: 1 addition & 1 deletion app/views/admin/members/_actions.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
7 changes: 4 additions & 3 deletions app/views/admin/members/_note.html.haml
Original file line number Diff line number Diff line change
@@ -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
Expand Down
2 changes: 1 addition & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@
resources :bans, only: %i[index new create]
end

resources :member_notes, only: [:create]
resources :member_notes, only: [:create, :destroy, :edit, :update]

resources :chapters, only: %i[index new create show edit update] do
get :members
Expand Down
56 changes: 56 additions & 0 deletions spec/controllers/admin/member_notes_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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