FIX issue 1003; couldn't solve the abilities and pass the specs witho…

…ut solving the issue :-)

- Unchanged: a student cannot create a second team in one season, and can create a team in next season
- Fixed: a student in a team in current season doesn't get the '+ new team' button
- Fixed: a student in a team in current season has no access to the new team form
- Driving by: use the new rails 5 redirect-er to redirect back to the referer or fall back to root. (So that the forbidden access message appears on the page where the user tried something forbidden.)
- Update projects_spec for new redirect
- Update teams_controller_spec that failed because a student can't create a second team in the same season

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/teams_controller.rb

@@ -4,7 +4,7 @@ class TeamsController < ApplicationController
   before_action :set_users, only: [:new, :edit]
   before_action :set_display_roles, only: :index
 
-  load_and_authorize_resource except: [:index, :show]
+  authorize_resource except: [:index, :show]
 
   def index
     direction  = params[:direction] == 'asc' ? 'ASC' : 'DESC'

app/models/ability.rb

@@ -37,12 +37,14 @@ def initialize(user)
 
     # current_student
     if user.student?
-      can :create, Team if user.teams.none?
+      cannot :create, Team do |team|
+        on_team_for_season?(user, team.season)
+      end
       can :create, Conference
     end
 
     # supervisor
-    # Use old code
+    # Use old code, see below
 
     # project submitter
     can [:update, :destroy], Project, submitter_id: user.id
@@ -67,11 +69,7 @@ def initialize(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 :update_conference_preferences, Team do |team|
       team.accepted? && team.students.include?(user)
@@ -85,9 +83,6 @@ 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)

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/models/ability/team_member_spec.rb

@@ -36,13 +36,6 @@
     describe "Student" do
       let(:new_team) {build(:team, :in_current_season)}
 
-      # TODO FIX BUG [reveiled by PR 997]
-      # the test for creating a second team is passing in team_spec because the validation kicks in on :submit
-      # The ability _should_ prevent access to the new form in the first place.
-      # There are some things wonky with the cancancan implementation.
-      # To be fixed in a separate issue
-      # These tests show the intended behaviour after the fix.
-
       context "with a student role from an earlier season" do
         before {create :student_role, team: old_team, user: user}
 
@@ -55,7 +48,6 @@
         before { create :student_role, team: current_team, user: user }
 
         it 'cannot create a second team in current_season' do
-          pending 'fails; it is tested in team_spec.rb:33, and that passes. See bug description above ^'
           expect(subject).not_to be_able_to(:create, new_team)
         end
 
@@ -69,37 +61,32 @@
     end
 
     describe "Supervisor" do
-      let(:other_user) { create(:user) }
-      let(:other_team_user) { create(:user) }
+      let(:team_member) { create(:user) }
+      let(:user_in_other_team) { create(:user) }
 
       before do
         allow(user).to receive(:supervisor?).and_return(true)
       end
 
       context "when viewing user information" do
 
-        # FIXME Implementation
-        # It turns out that the `can? :read users_info` and `can? :read_email` are both view helpers
-        # (And they overlap)
-        # With the correct implementation of Cancancan, they are redundant.
-        # See issue #1001
-        # These new specs should pass with the correct implementation
-
         it_behaves_like "same public features as confirmed user"
 
+        # FIXME see issue #1001
+        # The following specs should pass after fixing
         before do
-          other_user.update!(hide_email: true)
-          other_team_user.update!(hide_email: true)
+          team_member.update!(hide_email: true)
+          user_in_other_team.update!(hide_email: true)
         end
 
         it "can read email address of team members in current season, even when hidden" do
-          pending 'fails; fix in separate issue'
-          allow(subject).to receive(:supervises?).with(other_user, user).and_return true
-          expect(subject).to be_able_to(:read, other_user.email)
+          pending 'fails; To be solved with issue #1001'
+          allow(subject).to receive(:supervises?).with(team_member, user).and_return true
+          expect(subject).to be_able_to(:read, team_member.email)
         end
 
         it 'cannot read email of other users' do
-          expect(subject).not_to be_able_to(:read, other_team_user.email)
+          expect(subject).not_to be_able_to(:read, user_in_other_team.email)
         end
       end
     end

spec/requests/projects_spec.rb

@@ -9,38 +9,38 @@
     let(:season)    { nil }
     let!(:project)  { create :project, season: season, submitter: submitter }
 
-    shared_examples_for 'returns to the root page' do
-      it 'returns to the root page' do
+    shared_examples_for 'returns to the previous page' do
+      it 'returns to the previous page' do
         get use_as_template_project_path(project), headers: { 'HTTP_REFERER' => '/previouspage' }
-        expect(response).to redirect_to '/' # load_and_authorize_resource ignores the referrer
+        expect(response).to redirect_to '/previouspage' # load_and_authorize_resource finds the referrer
         expect(flash[:alert]).to match %r{not authorized}
       end
     end
 
     context 'when the user is not logged in' do
-      it_behaves_like 'returns to the root page'
+      it_behaves_like 'returns to the previous page'
     end
 
     context 'when the user is logged in' do
       let(:user) { create :user }
       before { sign_in user }
 
       context 'when the user does not own the project' do
-        it_behaves_like 'returns to the root page'
+        it_behaves_like 'returns to the previous page'
       end
 
       context 'when the user has submitted the project before' do
         let(:submitter) { user }
 
         context 'when the project is from the current season' do
           let(:season) { Season.current }
-          it_behaves_like 'returns to the root page'
+          it_behaves_like 'returns to the previous page'
         end
 
         context 'when the project is from a past season' do
           let(:season) { Season.find_or_create_by!(name: '2013') }
 
-          it 'returns to the root page' do
+          it 'returns to the previous page' do
             get use_as_template_project_path(project)
             expect(response).to be_success
             expect(response.body).to match project.name