diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index efa370fa7..06953056b 100644 --- a/app/controllers/application_controller.rb +++ b/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 diff --git a/app/controllers/teams_controller.rb b/app/controllers/teams_controller.rb index 6273deb75..d9f6453d1 100644 --- a/app/controllers/teams_controller.rb +++ b/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' diff --git a/app/models/ability.rb b/app/models/ability.rb index bc27c0040..837da8bd6 100644 --- a/app/models/ability.rb +++ b/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) diff --git a/app/views/teams/index.html.slim b/app/views/teams/index.html.slim index 743430f36..454dcfa7b 100644 --- a/app/views/teams/index.html.slim +++ b/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 diff --git a/spec/controllers/teams_controller_spec.rb b/spec/controllers/teams_controller_spec.rb index 162972c4f..c0c9c4b17 100644 --- a/spec/controllers/teams_controller_spec.rb +++ b/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 } } diff --git a/spec/models/ability/team_member_spec.rb b/spec/models/ability/team_member_spec.rb index 515be563b..da0c788a6 100644 --- a/spec/models/ability/team_member_spec.rb +++ b/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,8 +61,8 @@ 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) @@ -78,28 +70,23 @@ 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 diff --git a/spec/requests/projects_spec.rb b/spec/requests/projects_spec.rb index f54f6743c..bdbffc921 100644 --- a/spec/requests/projects_spec.rb +++ b/spec/requests/projects_spec.rb @@ -9,16 +9,16 @@ 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 @@ -26,7 +26,7 @@ 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 @@ -34,13 +34,13 @@ 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