Skip to content
This repository has been archived by the owner on Jun 27, 2023. It is now read-only.

Rearrange, not change the abilities in Ability #997

Merged
merged 20 commits into from
May 23, 2018
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
d00277c
WIP Rearrange, not change the abilities in Ability, cf recommended be…
emcoding May 15, 2018
27ead4d
Clean file, without todo's and check comments
emcoding May 15, 2018
e1b56f1
First examples of the New And Complete ability specs. Now with update…
emcoding May 15, 2018
6b4664e
Halfway
emcoding May 16, 2018
bfaeb99
WIP All things processed and annotated
emcoding May 17, 2018
f6da595
Cleanup file ; see previous commit for annotations
emcoding May 17, 2018
b172a3f
hot fix for failing spec MailingsController (because of wrong authori…
emcoding May 17, 2018
ec3c037
Finetuning
emcoding May 18, 2018
f84460e
Finetuning
emcoding May 18, 2018
dad17d8
Write tests for desired behaviour of student ability whether or not t…
emcoding May 18, 2018
ff1e8ba
Write tests for desired behaviour of whether a supervisor can or cann…
emcoding May 18, 2018
975877f
Restore old code for can read_email and can users_info. Add tests for…
emcoding May 18, 2018
1e5db7d
WIP Split the ability specs in separate files per role - thanks to @k…
emcoding May 18, 2018
cabcae9
WIP First try feature test guest user access
emcoding May 18, 2018
6e7d601
FIX issue 1003; couldn't solve the abilities and pass the specs witho…
emcoding May 20, 2018
9a0f39b
Remove one level of nesting in the ability specs, removed the repitio…
emcoding May 21, 2018
e765a2b
:cop:
emcoding May 21, 2018
9d685da
Revert changes to authorize method in controllers that are not needed…
emcoding May 21, 2018
419bdf7
Declutter
emcoding May 21, 2018
cb126b9
Add scope to solve failing specs.
emcoding May 21, 2018
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
3 changes: 1 addition & 2 deletions app/controllers/mailings_controller.rb
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
# frozen_string_literal: true
class MailingsController < ApplicationController

load_and_authorize_resource except: :index
load_and_authorize_resource
Copy link
Member

Choose a reason for hiding this comment

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

👍


def index
@mailings = Mailing.order('id DESC').page(params[:page])
authorize! :read, :mailing
end

# These actions are here to enable the cancancan 'not authorised' notice
Expand Down
103 changes: 60 additions & 43 deletions app/models/ability.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
# frozen_string_literal: true
# See the wiki for details:
# https://github.com/ryanb/cancan/wiki/Defining-Abilities

class Ability
include CanCan::Ability
Expand All @@ -10,22 +8,66 @@ def initialize(user)

alias_action :create, :read, :update, :destroy, to: :crud

can :crud, User, id: user.id
can :crud, User if user.admin?
# guest user
can :read, [Activity, User, Team, Project, Conference]

return unless signed_in?(user)

# unconfirmed, logged in user
can :update, User, id: user.id
can :resend_confirmation_instruction, User, id: user.id
can :read_email, User, hide_email: false # view helper # delete? only used once

return unless user.confirmed? && signed_in?(user)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the signed_in?(user) is redundant - you've already returned for signed out users above.


# confirmed user
can [:update, :destroy], User, id: user.id
can :resend_confirmation_instruction, User, id: user.id
can :resend_confirmation_instruction, User if user.admin?
can :create, Project
can [:join, :create], Team
can :index, Mailing
can :read, Mailing do |mailing|
mailing.recipient? user
end

# Members in a team
can [:update, :destroy], Team do |team|
on_team?(user, team)
end

# current_student
if user.current_student? # TODO is this a valid check?
Copy link
Member

Choose a reason for hiding this comment

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

From what I remember this checks whether the user is a student in an accepted team for the current season.

So this may already be scoped too narrow here... At least

user.teams.none?

and creating teams in general does not make much sense then 🤔

can :create, Team if user.teams.none?
can :create, Conference
end

# visibility of email address in user profile
can :read_email, User, id: user.id if !user.hide_email?
can :read_email, User if user.admin?
can :read_email, User do |other_user|
user.confirmed? && (supervises?(other_user, user) || !other_user.hide_email?)
# supervisor
if user.supervisor?
can :read, :users_info
can :read_email, User do |other_user|
supervises?(other_user, user)
end
end

can :crud, Team do |team|
user.admin? || signed_in?(user) && team.new_record? || on_team?(user, team)
# project submitter
can [:update, :destroy], Project, submitter_id: user.id
can :use_as_template, Project do |project|
user == project.submitter && !project.season&.current?
end

# admin
if user.admin?
can :manage, :all
# MEMO add "cannot's" only; and only after this line
cannot :create, User # this only happens through GitHub
end

################# REMAININGS FROM OLD FILE, # = rewritten above ############

# can :crud, Team do |team|
# user.admin? || signed_in?(user) && team.new_record? || on_team?(user, team)
# end

can :update_conference_preferences, Team do |team|
team.accepted? && team.students.include?(user)
end
Expand All @@ -38,10 +80,10 @@ def initialize(user)
team.students.include?(user)
end

cannot :create, Team do |team|
on_team_for_season?(user, team.season) || !user.confirmed?
end

# cannot :create, Team do |team|
# on_team_for_season?(user, team.season) || !user.confirmed?
# end
# todo helpdesk team join
can :join, Team do |team|
team.helpdesk_team? and signed_in?(user) and user.confirmed? and not on_team?(user, team)
end
Expand All @@ -62,35 +104,10 @@ def initialize(user)
user.admin? || (preference.team.students.include? user)
end

can :crud, Conference if user.admin? || user.current_student?

# todo add mailing controller and view for users in their namespace, where applicable
can :read, Mailing do |mailing|
mailing.recipient? user
end

can :crud, :comments if user.admin?
can :read, :users_info if user.admin? || user.supervisor?

# projects
can :crud, Project do |project|
user.admin? ||
(user.confirmed? && user == project.submitter)
end
can :use_as_template, Project do |project|
user == project.submitter && !project.season&.current?
end

can :create, Project if user.confirmed?
cannot :create, Project if !user.confirmed?

# activities
can :read, :feed_entry
can :read, :mailing if signed_in?(user)

# applications
can :create, :application_draft if user.student? && user.application_drafts.in_current_season.none?
end

end # initializer

def signed_in?(user)
user.persisted?
Expand Down
4 changes: 4 additions & 0 deletions spec/factories/projects.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,5 +30,9 @@
trait :in_current_season do
season { Season.current }
end

trait :in_last_season do
season { Season.find_or_create_by name: (Date.today.year - 1) }
end
end
end
6 changes: 5 additions & 1 deletion spec/factories/users.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
FactoryBot.define do
factory :user, aliases: [:member] do
github_handle { FFaker::InternetSE.user_name_variant_short }
github_handle { FFaker::InternetSE.unique.user_name_variant_short }
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes flickering specs because of failing uniqueness validation on gh handle.

name { FFaker::Name.name }
email { FFaker::Internet.email }
location { FFaker::Address.city }
Expand Down Expand Up @@ -84,5 +84,9 @@
create(:reviewer_role, user: user)
end
end

trait :unconfirmed do
confirmed_at { nil }
Copy link
Member

Choose a reason for hiding this comment

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

No need for a block here.

end
end
end
Loading