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 14 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
100 changes: 61 additions & 39 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,21 +8,70 @@ 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 :resend_confirmation_instruction, User if user.admin?
can :read_email, User, hide_email: false

return unless user.confirmed?

# confirmed user
can [:update, :destroy], User, id: user.id
can :resend_confirmation_instruction, User, id: user.id
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.student?
can :create, Team if user.teams.none?
can :create, Conference
end

# supervisor
# Use old code

# 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 ############

# FIXME Leave this in until issue #1001 is fixed
# 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?)
end
can :read, :users_info if user.admin? || user.supervisor?

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

# 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)
Expand All @@ -38,10 +85,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 +109,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
4 changes: 4 additions & 0 deletions spec/factories/season.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,8 @@
factory :season do
sequence(:name, '2000')
end

trait :past do
name '2010'
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
end
end
end
72 changes: 72 additions & 0 deletions spec/features/users/guest_user_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
require 'rails_helper'

RSpec.describe 'Guest User', type: :feature do

let!(:activity) { create(:status_update, :published, team: team1) }
let!(:other_user) { create(:user) }
let!(:project) { create(:project, :in_current_season, :accepted, submitter: other_user) }
let!(:team1) { create(:team, name: 'Cheesy forever', project_id: project.id) }
let!(:out_of_season) { Season.current.starts_at - 1.week }
let!(:summer_season) { Season.current.starts_at + 1.week }

context "when visiting public pages" do

context 'All Year' do
before { Timecop.travel(out_of_season) }
after { Timecop.return }

it 'can view Activities' do
visit root_path
expect(page).to have_css('h1', text: 'Activities')
find('.title', match: :smart).click
expect(page).to have_content(activity.title)
expect(page).to have_content('You must be logged in to add a comment.')
end

it 'can view Community and User' do
visit community_path
expect(page).to have_css('h1', text: 'Community')
find_link(other_user.name, match: :smart).click
expect(page).to have_content("About me")
expect(page).to have_link("All participants")
expect(page).not_to have_link("Edit") # check
end

it 'can view projects' do
visit projects_path
expect(page).to have_css('h1', text: 'Projects') # can be empty table
end

it 'has a nav menu with public links' do
visit root_path
expect(page).to have_link("Activities")
find_link("Summer of Code").click
expect(page).to have_link("Teams")
expect(page).to have_link("Community")
expect(page).to have_link("Help")
end

it 'has access to sign in link' do
visit root_path
expect(page).to have_link('Sign in')
end
end

context 'in season' do
before do
Timecop.travel(summer_season)
end
after { Timecop.return }

it "can view the current season's accepted and selected projects" do
# project not visible on page. why?
Copy link
Member Author

Choose a reason for hiding this comment

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

Related to #1002?

Copy link
Member

Choose a reason for hiding this comment

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

Good catch! Could be...

Copy link
Member Author

Choose a reason for hiding this comment

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

AHA! I had to use a in_current_season scope 🌞

visit projects_path
expect(page).to have_css('h1', text: 'Projects')
# find_link(project.name, match: :smart).click
# expect(page).to have_content project.description
# expect(page).not_to have_link("Edit")
end
end
end
# continuing story in: sign_in_unconfirmed_user || sign_in_confirmed_user || sign_in_fail
end
25 changes: 25 additions & 0 deletions spec/models/ability/admin_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
require 'rails_helper'
require 'cancan/matchers'

# Run this file with
# $ rspec spec/models/ability_spec.rb -fd
# to see the output of specs running inside the shared examples [mdv]
RSpec.describe Ability, type: :model do

let(:admin) { create(:user) }
subject(:ability) { Ability.new(admin) }

let(:other_user) { build_stubbed(:user, hide_email: true) }

describe "Admin" do
before { allow(admin).to receive(:admin?).and_return true }

it_behaves_like "can not create new user"
# it "has access to almost everything else"
# Only test the most exclusive, the most sensitive and the 'cannots':
it { expect(subject).to be_able_to(:crud, Team) }
it { expect(subject).to be_able_to([:read, :update, :destroy], User) }
it { expect(subject).to be_able_to(:read_email, other_user) }
it { expect(subject).to be_able_to(:read, :users_info, other_user) }
end
end
22 changes: 22 additions & 0 deletions spec/models/ability/confirmed_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
require 'rails_helper'
require 'cancan/matchers'

# Run this file with
# $ rspec spec/models/ability_spec.rb -fd
# to see the output of specs running inside the shared examples [mdv]
RSpec.describe Ability, type: :model do

let(:user) { create(:user) }
subject(:ability) { Ability.new(user) }

describe "Confirmed user" do

it_behaves_like "same as logged in user"
it_behaves_like "can create a Project"
it_behaves_like "can see mailings list too"
it_behaves_like "can read mailings sent to them"
it_behaves_like "can comment now" # not implemented; false positives; needs work
it { expect(subject).to be_able_to([:join, :create], Team) }

end
end
22 changes: 22 additions & 0 deletions spec/models/ability/guest_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
require 'rails_helper'
require 'cancan/matchers'

# Run this file with
# $ rspec spec/models/ability_spec.rb -fd
# to see the output of specs running inside the shared examples [mdv]
RSpec.describe Ability, type: :model do

let(:user){ nil }
subject(:ability) { Ability.new(user) }

let(:other_user) { build_stubbed(:user) }

describe "Guest User" do
it_behaves_like "can view public pages"
it_behaves_like "can not modify things on public pages"
it_behaves_like "can not create new user"
it_behaves_like "can not comment"
it_behaves_like "has no access to other user's accounts"
it_behaves_like "can not read role restricted or owner restricted pages"
end
end
26 changes: 26 additions & 0 deletions spec/models/ability/project_maintainer_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
require 'rails_helper'
require 'cancan/matchers'

# Run this file with
# $ rspec spec/models/ability_spec.rb -fd
# to see the output of specs running inside the shared examples [mdv]

RSpec.describe Ability, type: :model do

describe "Project Submitter" do

let(:user) { build_stubbed(:user) }
subject(:ability) { Ability.new(user) }

let(:old_project) { build_stubbed(:project, submitter: user) }
let(:other_project) { build_stubbed(:project, submitter: other_user) }
let(:same_season_project) { build :project, :in_current_season, submitter: user }
let(:other_user) { build_stubbed(:user) }

it_behaves_like "same public features as confirmed user"
it { expect(subject).to be_able_to([:update, :destroy], Project.new(submitter: user)) }
it { expect(subject).to be_able_to(:use_as_template, old_project) }
it { expect(subject).not_to be_able_to(:use_as_template, other_project) }
it { expect(subject).not_to be_able_to :use_as_template, same_season_project }
end
end
Loading