From 88e1646e87165b7ef83ab5df638167e90c3d03a0 Mon Sep 17 00:00:00 2001 From: AEtherC0r3 Date: Thu, 2 Feb 2017 01:28:22 +0200 Subject: [PATCH] Fix unreachable code (#1140) in ConferenceRegistrationController#new Remove the unreachable code from ConferenceRegistrationController#new Rework the error message logic in ConferenceRegistrationController#new Fix redirect to #edit when the user is registered Refactor ConferenceRegistrationController#new spec and add more tests --- app/controllers/application_controller.rb | 2 +- .../conference_registrations_controller.rb | 15 +- ...conference_registration_controller_spec.rb | 250 +++++++++++++++--- 3 files changed, 215 insertions(+), 52 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 481d1e53..f1a31894 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -46,7 +46,7 @@ class ApplicationController < ActionController::Base rescue_from CanCan::AccessDenied do |exception| Rails.logger.debug "Access denied on #{exception.action} #{exception.subject.inspect}" message = exception.message - message << ' Maybe you need to sign in?' unless current_user + message << ' Maybe you need to sign in?' unless @ignore_not_signed_in_user || current_user redirect_to root_path, alert: message end diff --git a/app/controllers/conference_registrations_controller.rb b/app/controllers/conference_registrations_controller.rb index 176f7937..ea02202e 100644 --- a/app/controllers/conference_registrations_controller.rb +++ b/app/controllers/conference_registrations_controller.rb @@ -6,22 +6,21 @@ class ConferenceRegistrationsController < ApplicationController def new @registration = Registration.new(conference_id: @conference.id) - authorize! :new, @registration, message: "Sorry, you can not register for #{@conference.title}. Registration limit exceeded or the registration is not open." # Redirect to registration edit when user is already registered if @conference.user_registered?(current_user) + # Authorization needs to happen in every action before the return statement + # We authorize the #edit action, since we redirect to it + authorize! :edit, current_user.registrations.find_by(conference_id: @conference.id) redirect_to edit_conference_conference_registration_path(@conference.short_title) return - # ichain does not allow us to create users during registration - elsif (ENV['OSEM_ICHAIN_ENABLED'] == 'true') && !current_user - redirect_to root_path, alert: 'You need to sign in or sign up before continuing.' - return end - # avoid openid sign_in to redirect to register/new when the sign_in user had already a registration - if current_user && @conference.user_registered?(current_user) - redirect_to edit_conference_conference_registration_path(@conference.short_title) + if !@conference.registration_open? || @conference.registration_limit_exceeded? + message = "Sorry, you can not register for #{@conference.title}. Registration limit exceeded or the registration is not open." + @ignore_not_signed_in_user = true end + authorize! :new, @registration, message: message # @user variable needs to be set so that _sign_up_form_embedded works properly @user = @registration.build_user diff --git a/spec/controllers/conference_registration_controller_spec.rb b/spec/controllers/conference_registration_controller_spec.rb index 377e884e..c919d90d 100644 --- a/spec/controllers/conference_registration_controller_spec.rb +++ b/spec/controllers/conference_registration_controller_spec.rb @@ -1,51 +1,167 @@ require 'spec_helper' describe ConferenceRegistrationsController, type: :controller do - let(:conference) { create(:conference) } + let(:conference) { create(:conference, title: 'My Conference', short_title: 'myconf') } let(:user) { create(:user) } + let(:not_registered_user) { create(:user) } + let(:registered_user) { create(:user) } + let!(:registration) { create(:registration, conference: conference, user: registered_user, created_at: 1.day.ago) } + + shared_examples 'access #new action' do |user, ichain, path, message| + before :each do + sign_in send(user) if user + stub_const('ENV', ENV.to_hash.merge('OSEM_ICHAIN_ENABLED' => ichain)) + get :new, conference_id: conference.short_title + end + + it 'redirects' do + expect(response).to redirect_to path + end + + it 'shows flash alert' do + expect(flash[:alert]).to eq message + end + end context 'user is signed in' do - before { sign_in(user) } + before :each do + sign_in user + end describe 'GET #new' do context 'registration period open' do - before do - @registration_period = create(:registration_period, conference: conference) + before :each do + create(:registration_period, conference: conference, start_date: 3.days.ago, end_date: 1.day.from_now) end - context 'user registered' do - before do - @registration = create(:registration, conference: conference, user: user) - get :new, conference_id: conference.short_title + context 'registration limit not exceeded' do + before :each do + conference.registration_limit = 0 + conference.save! end - it 'redirects to edit conference registration' do - expect(response).to redirect_to edit_conference_conference_registration_path(conference.short_title) + context 'OSEM_ICHAIN_ENABLED is true' do + before :each do + stub_const('ENV', ENV.to_hash.merge('OSEM_ICHAIN_ENABLED' => 'true')) + end + + context 'user registered' do + it_behaves_like 'access #new action', :registered_user, 'true', '/conferences/myconf/register/edit', nil + end + + context 'user not registered' do + before :each do + get :new, conference_id: conference.short_title + end + + it 'user variable exists' do + expect(assigns(:user)).not_to be_nil + end + + it 'renders the new template' do + expect(response).to render_template('new') + end + end + end + + context 'OSEM_ICHAIN_ENABLED is false' do + before :each do + stub_const('ENV', ENV.to_hash.merge('OSEM_ICHAIN_ENABLED' => 'false')) + end + + context 'user registered' do + it_behaves_like 'access #new action', :registered_user, 'false', '/conferences/myconf/register/edit', nil + end + + context 'user not registered' do + before :each do + get :new, conference_id: conference.short_title + end + + it 'user variable exists' do + expect(assigns(:user)).not_to be_nil + end + + it 'renders the new template' do + expect(response).to render_template('new') + end + end end end - context 'user not registered' do - before do - get :new, conference_id: conference.short_title + context 'registration limit exceeded' do + before :each do + conference.registration_limit = 1 + conference.save! end - it 'user variable exists' do - expect(assigns(:user)).not_to be_nil + context 'OSEM_ICHAIN_ENABLED true, user registered' do + it_behaves_like 'access #new action', :registered_user, 'true', '/conferences/myconf/register/edit', nil end - it 'renders the new template' do - expect(response).to render_template('new') + context 'OSEM_ICHAIN_ENABLED true, user not registered' do + it_behaves_like 'access #new action', :not_registered_user, 'true', '/', 'Sorry, you can not register for My Conference. Registration limit exceeded or the registration is not open.' + end + + context 'OSEM_ICHAIN_ENABLED false, user registered' do + it_behaves_like 'access #new action', :registered_user, 'false', '/conferences/myconf/register/edit', nil + end + + context 'OSEM_ICHAIN_ENABLED false, user not registered' do + it_behaves_like 'access #new action', :not_registered_user, 'false', '/', 'Sorry, you can not register for My Conference. Registration limit exceeded or the registration is not open.' end end end context 'registration period not open' do - before do - get :new, conference_id: conference.short_title + before :each do + create(:registration_period, conference: conference, start_date: 3.days.ago, end_date: 1.day.ago) end - it 'shows flash alert telling user they are unable to register' do - expect(flash[:alert]).to eq "Sorry, you can not register for #{conference.title}. Registration limit exceeded or the registration is not open." + context 'registration limit not exceeded' do + before :each do + conference.registration_limit = 0 + conference.save! + end + + context 'OSEM_ICHAIN_ENABLED true, user registered' do + it_behaves_like 'access #new action', :registered_user, 'true', '/conferences/myconf/register/edit', nil + end + + context 'OSEM_ICHAIN_ENABLED true, user not registered' do + it_behaves_like 'access #new action', :not_registered_user, 'true', '/', 'Sorry, you can not register for My Conference. Registration limit exceeded or the registration is not open.' + end + + context 'OSEM_ICHAIN_ENABLED false, user registered' do + it_behaves_like 'access #new action', :registered_user, 'false', '/conferences/myconf/register/edit', nil + end + + context 'OSEM_ICHAIN_ENABLED false, user not registered' do + it_behaves_like 'access #new action', :not_registered_user, 'false', '/', 'Sorry, you can not register for My Conference. Registration limit exceeded or the registration is not open.' + end + end + + context 'registration limit exceeded' do + before do + conference.registration_limit = 1 + conference.save! + end + + context 'OSEM_ICHAIN_ENABLED true, user registered' do + it_behaves_like 'access #new action', :registered_user, 'true', '/conferences/myconf/register/edit', nil + end + + context 'OSEM_ICHAIN_ENABLED true, user not registered' do + it_behaves_like 'access #new action', :not_registered_user, 'true', '/', 'Sorry, you can not register for My Conference. Registration limit exceeded or the registration is not open.' + end + + context 'OSEM_ICHAIN_ENABLED false, user registered' do + it_behaves_like 'access #new action', :registered_user, 'false', '/conferences/myconf/register/edit', nil + end + + context 'OSEM_ICHAIN_ENABLED false, user not registered' do + it_behaves_like 'access #new action', :not_registered_user, 'false', '/', 'Sorry, you can not register for My Conference. Registration limit exceeded or the registration is not open.' + end end end end @@ -186,7 +302,7 @@ describe ConferenceRegistrationsController, type: :controller do it 'deletes the registration' do expect do delete :destroy, conference_id: conference.short_title - end.to change{ Registration.count }.from(1).to(0) + end.to change{ Registration.count }.from(2).to(1) end end @@ -213,40 +329,88 @@ describe ConferenceRegistrationsController, type: :controller do context 'user is not signed in' do describe 'GET #new' do - before do - @registration_period = create(:registration_period, conference: conference) - end - - context 'OSEM_ICHAIN_ENABLED is true' do - before do - stub_const('ENV', ENV.to_hash.merge('OSEM_ICHAIN_ENABLED' => 'true')) - get :new, conference_id: conference.short_title + context 'registration period open' do + before :each do + create(:registration_period, conference: conference, start_date: 3.days.ago, end_date: 1.day.from_now) end - it 'redirects to root' do - expect(response).to redirect_to root_path + context 'registration limit not exceeded' do + before :each do + conference.registration_limit = 0 + conference.save! + end + + context 'OSEM_ICHAIN_ENABLED is true' do + it_behaves_like 'access #new action', nil, 'true', '/', 'You are not authorized to access this page. Maybe you need to sign in?' + end + + context 'OSEM_ICHAIN_ENABLED is false' do + before :each do + stub_const('ENV', ENV.to_hash.merge('OSEM_ICHAIN_ENABLED' => 'false')) + get :new, conference_id: conference.short_title + end + + it 'user variable exists' do + expect(assigns(:user)).not_to be_nil + end + + it 'renders the new template' do + expect(response).to render_template('new') + end + end end - it 'shows flash alert telling user they cannot register and they need to sign in' do - expect(flash[:alert]).to eq "Sorry, you can not register for #{conference.title}. Registration limit exceeded or the registration is not open. Maybe you need to sign in?" + context 'registration limit exceeded' do + before :each do + conference.registration_limit = 1 + conference.save! + end + + context 'OSEM_ICHAIN_ENABLED is true' do + it_behaves_like 'access #new action', nil, 'true', '/', 'Sorry, you can not register for My Conference. Registration limit exceeded or the registration is not open.' + end + + context 'OSEM_ICHAIN_ENABLED is false' do + it_behaves_like 'access #new action', nil, 'false', '/', 'Sorry, you can not register for My Conference. Registration limit exceeded or the registration is not open.' + end end end - context 'OSEM_ICHAIN_ENABLED is false' do - before do - stub_const('ENV', ENV.to_hash.merge('OSEM_ICHAIN_ENABLED' => 'false')) - get :new, conference_id: conference.short_title + context 'registration period not open' do + before :each do + create(:registration_period, conference: conference, start_date: 3.days.ago, end_date: 1.day.ago) end - it 'user variable exists' do - expect(assigns(:user)).not_to be_nil + context 'registration limit not exceeded' do + before :each do + conference.registration_limit = 0 + conference.save! + end + + context 'OSEM_ICHAIN_ENABLED is true' do + it_behaves_like 'access #new action', nil, 'true', '/', 'Sorry, you can not register for My Conference. Registration limit exceeded or the registration is not open.' + end + + context 'OSEM_ICHAIN_ENABLED is false' do + it_behaves_like 'access #new action', nil, 'false', '/', 'Sorry, you can not register for My Conference. Registration limit exceeded or the registration is not open.' + end end - it 'renders the new template' do - expect(response).to render_template('new') + context 'registration limit exceeded' do + before :each do + conference.registration_limit = 1 + conference.save! + end + + context 'OSEM_ICHAIN_ENABLED is true' do + it_behaves_like 'access #new action', nil, 'true', '/', 'Sorry, you can not register for My Conference. Registration limit exceeded or the registration is not open.' + end + + context 'OSEM_ICHAIN_ENABLED is false' do + it_behaves_like 'access #new action', nil, 'false', '/', 'Sorry, you can not register for My Conference. Registration limit exceeded or the registration is not open.' + end end end end end - end