Merge pull request #997 from rails-girls-summer-of-code/clean_cans

Rearrange, not change the abilities in Ability

app/controllers/application_controller.rb

@@ -33,7 +33,7 @@ def after_sign_in_path_for(user)
   end
 
   rescue_from CanCan::AccessDenied do |exception|
-    redirect_to root_url, alert: exception.message
+    redirect_back(fallback_location: root_path, alert: exception.message)
   end
 
   def cors_preflight

app/controllers/mailings_controller.rb

@@ -1,11 +1,10 @@
 # frozen_string_literal: true
 class MailingsController < ApplicationController
 
-  load_and_authorize_resource except: :index
+  load_and_authorize_resource
 
   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

app/models/ability.rb

@@ -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
@@ -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, :destroy], User, id: user.id
+    can :resend_confirmation_instruction, User, id: user.id
+    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 :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
+    can :create, Comment
+
+    # Members in a team
+    can [:update, :destroy], Team do |team|
+      on_team?(user, team)
+    end
+
+    # current_student
+    if user.student?
+      cannot :create, Team do |team|
+        on_team_for_season?(user, team.season)
+      end
+      can :create, ApplicationDraft if user.application_drafts.in_current_season.none?
+      can :create, Conference
+    end
+
+    # supervisor
+    # Use old code, see below
+
+    # 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 :crud, Team do |team|
-      user.admin? || signed_in?(user) && team.new_record? || on_team?(user, team)
-    end
+    can :read, :users_info if user.admin? || user.supervisor?
+    #
 
     can :update_conference_preferences, Team do |team|
       team.accepted? && team.students.include?(user)
@@ -38,10 +85,7 @@ def initialize(user)
       team.students.include?(user)
     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
@@ -62,35 +106,7 @@ 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?

app/views/teams/index.html.slim

@@ -1,7 +1,8 @@
 nav.actions
   ul.list-inline
     li = link_to 'My Team', current_student.current_team, class: 'btn btn-default btn-sm' if current_student&.current_team
-    li = link_to icon('plus', 'New Team'), new_team_path, class: 'btn btn-primary btn-sm' if can? :create, Team.new(season: Season.current)
+    li = link_to icon('plus', 'New Team'), new_team_path, class: 'btn btn-primary btn-sm' if can? :create, Team
+    = 'Sign up to create your own team!' unless current_user
   div.pull-right.dropdown
     button.btn.btn-default.dropdown-toggle data-toggle="dropdown"
       | Past Teams

spec/controllers/teams_controller_spec.rb

@@ -12,11 +12,11 @@
 
   let(:valid_attributes) { build(:team).attributes.merge(roles_attributes: [{ name: 'coach', github_handle: 'tobias' }]) }
 
-  before do
-    user.roles.create(name: 'student', team: team)
-  end
-
   describe "GET index" do
+    before do
+      user.roles.create(name: 'student', team: team)
+    end
+
     context 'before acceptance letters are sent' do
       let(:last_season)      { Season.create name: Date.today.year - 1 }
       let!(:invisble_team)   { create :team, :in_current_season, kind: nil, invisible: true }
@@ -103,6 +103,10 @@
   end
 
   describe "GET edit" do
+    before do
+      user.roles.create(name: 'student', team: team)
+    end
+
     context "their own team" do
       let(:team) { create(:team) }
 
@@ -165,7 +169,10 @@
   end
 
   describe "PATCH update" do
-    before { sign_in user }
+    before do
+      sign_in user
+      user.roles.create(name: 'student', team: team)
+    end
 
     context "their own team" do
       let(:team) { create(:team) }
@@ -267,7 +274,10 @@
   end
 
   describe "DELETE destroy" do
-    before { sign_in user }
+    before do
+      sign_in user
+      user.roles.create(name: 'student', team: team)
+    end
 
     context "their own team" do
       let(:params) { { id: team.to_param } }

spec/factories/projects.rb

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

spec/factories/season.rb

@@ -2,4 +2,8 @@
   factory :season do
     sequence(:name, '2000')
   end
+
+  trait :past do
+    name '2010'
+  end
 end

spec/factories/users.rb

@@ -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 }
     name     { FFaker::Name.name }
     email    { FFaker::Internet.email }
     location { FFaker::Address.city }
@@ -84,5 +84,9 @@
         create(:reviewer_role, user: user)
       end
     end
+
+    trait :unconfirmed do
+      confirmed_at nil
+    end
   end
 end

spec/features/users/guest_user_spec.rb

@@ -0,0 +1,71 @@
+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, :in_current_season, name: 'Cheesy forever', project_name: project.name, 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
+        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

spec/models/ability/admin_spec.rb

@@ -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 { expect(subject).not_to be_able_to(:create, User.new) } # happens only via GitHub
+    # 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

spec/models/ability/confirmed_user_spec.rb

@@ -0,0 +1,31 @@
+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) { build_stubbed(:user) }
+  subject(:ability) { Ability.new(user) }
+  let(:other_user) { build_stubbed(:user) }
+
+  describe "Confirmed user" do
+
+    it_behaves_like 'has access to public features'
+
+    # same as unconfirmed:
+    it "can modify own account" do
+      expect(subject).to be_able_to([:update, :destroy], user)
+      expect(subject).to be_able_to(:resend_confirmation_instruction, User, id: user.id)
+    end
+    it { expect(subject).not_to be_able_to([:update, :destroy], other_user) }
+
+    # the perks of confirming
+    it { expect(subject).to be_able_to([:join, :create], Team) }
+    it { expect(subject).to be_able_to(:create, Comment) } # TODO needs work for polymorphism
+    it { expect(subject).to be_able_to(:create, Project) }
+    it { expect(subject).to be_able_to(:index, Mailing) }
+    it { expect(subject).to be_able_to(:read, Mailing, recipient: user )}
+  end
+end