Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -105,15 +105,15 @@ def redirect_path
end

def authenticate_admin!
redirect_to root_path, notice: "You can't be here" unless logged_in? && current_user.has_role?(:admin)
redirect_to root_path, notice: "You can't be here" unless logged_in? && current_user.is_admin?
end

def authenticate_admin_or_organiser!
redirect_to root_path, notice: "You can't be here" unless manager?
end

def manager?
logged_in? && (current_user.is_admin? || current_user.has_role?(:organiser, :any))
logged_in? && current_user.manager?
end

helper_method :manager?
Expand Down
10 changes: 10 additions & 0 deletions app/models/member.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,20 @@ class Member < ApplicationRecord

scope :with_skill, ->(skill_name) { tagged_with(skill_name) }

scope :admin, -> { with_role(:admin) }
Copy link
Contributor

Choose a reason for hiding this comment

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

@gnclmorais my Rails is so rusty, could you explain why we needed to define the :admin, and :organiser scopes here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

One reason for having scopes named like this is that it enables the creation of a very legible manager scope: scope :manager, -> { admin.or(organiser).distinct } reads quite clearly.

The manager? method seems to have a fast-path is_admin? first, before querying the more-expensive has_role? call.

Looks nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main reason was pointed out by Olle: by defining the scopes, we create building blocks that can be reused anywhere and, most importantly for this PR, allows us to express in a clear way that a manager is either an admin or an organiser.

The manager? method came from moving the code that was on app/controllers/application_controller.rb to a more suitable place (i.e. it should be the Member model to own the s_admin? || has_role?(:organiser, :any) code).

Copy link
Contributor

@matyikriszta matyikriszta Dec 8, 2025

Choose a reason for hiding this comment

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

@gnclmorais I think I asked my question wrong. I wanted to know where the :admin and organiser scopes were being called but I went back and re-read the :manager scope definition and answered my own question, I didn't read it closely enough and couldn't see where they were called. Thanks for the update.

scope :organiser, -> { with_role(:organiser, :any) }
scope :manager, -> { admin.or(organiser).distinct }

scope :sort_alphabetically, -> { order(:name, :surname) }

acts_as_taggable_on :skills

attr_accessor :attendance, :newsletter

def manager?
is_admin? || has_role?(:organiser, :any)
end

def banned?
bans.active.present? || bans.permanent.present?
end
Expand Down
14 changes: 9 additions & 5 deletions app/views/admin/events/_form.html.haml
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
:ruby
all_sponsors = Sponsor.all
all_managers = Member.manager.sort_alphabetically

= simple_form_for [:admin, @event] do |f|
.row
.col-12
Expand Down Expand Up @@ -34,15 +38,15 @@
%p
%strong Please add sponsors only to either Standard level OR Gold/Silver/Bronze levels.
.col-12
= f.association :sponsors, label: 'Standard sponsors', input_html: { data: { placeholder: 'Select standard sponsors' }}, collection: Sponsor.all
= f.association :sponsors, label: 'Standard sponsors', input_html: { data: { placeholder: 'Select standard sponsors' }}, collection: all_sponsors
.col-12.col-md-6.col-lg-4
= f.association :bronze_sponsors, input_html: { data: { placeholder: 'Select bronze sponsors' }}, collection: Sponsor.all
= f.association :bronze_sponsors, input_html: { data: { placeholder: 'Select bronze sponsors' }}, collection: all_sponsors
.col-12.col-md-6.col-lg-4
= f.association :silver_sponsors, input_html: { data: { placeholder: 'Select silver sponsors' }}, collection: Sponsor.all
= f.association :silver_sponsors, input_html: { data: { placeholder: 'Select silver sponsors' }}, collection: all_sponsors
.col-12.col-md-6.col-lg-4
= f.association :gold_sponsors, input_html: { data: { placeholder: 'Select gold sponsors' }}, collection: Sponsor.all
= f.association :gold_sponsors, input_html: { data: { placeholder: 'Select gold sponsors' }}, collection: all_sponsors
.col-12
= f.input :organisers, collection: Member.all, value_method: :id, label_method: :full_name, selected: @event.organisers.map(&:id), input_html: { multiple: true }
= f.input :organisers, collection: all_managers, value_method: :id, label_method: :full_name, selected: @event.organisers.pluck(&:id), input_html: { multiple: true }
.col-12
= f.input :announce_only, as: :boolean, hint: 'Events where invitations are not handled via our application'
.col-12
Expand Down
44 changes: 44 additions & 0 deletions spec/models/member_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,50 @@
expect(Member.find_members_by_name('').size).to eq(0)
end
end
end

describe '.admin' do
it 'returns members with admin role' do
admin_member = Fabricate(:member)
admin_member.add_role :admin
non_admin = Fabricate(:member)

expect(described_class.admin).to include(admin_member)
expect(described_class.admin).not_to include(non_admin)
end
end

describe '.organiser' do
it 'returns members with organiser role' do
chapter = Fabricate(:chapter)
organiser = Fabricate(:member)
organiser.add_role :organiser, chapter
non_organiser = Fabricate(:member)

expect(described_class.organiser).to include(organiser)
expect(described_class.organiser).not_to include(non_organiser)
end
end

describe '.manager' do
it 'returns both admins and organisers' do
admin_member = Fabricate(:member)
admin_member.add_role :admin

chapter = Fabricate(:chapter)
organiser = Fabricate(:member)
organiser.add_role :organiser, chapter
# Add another role to test for duplicates
organiser.add_role :admin

non_manager = Fabricate(:member)

managers = described_class.manager

expect(managers).to include(admin_member, organiser)
expect(managers).not_to include(non_manager)

expect(managers.size).to eq(managers.distinct.size)
end
end
end