From c982cbb88aa18791e03bbd68af9ab64c4a6d09bc Mon Sep 17 00:00:00 2001 From: Henne Vogelsang Date: Wed, 23 Jul 2014 16:24:05 +0200 Subject: [PATCH] Refactored proposal. Enabled cancancan --- Gemfile | 4 +- Gemfile.lock | 4 +- app/controllers/application_controller.rb | 4 - app/controllers/proposal_controller.rb | 259 ++++++++------------ app/models/ability.rb | 35 +-- app/models/admin_ability.rb | 17 -- app/models/event.rb | 8 +- app/models/user.rb | 8 + app/views/proposal/_proposal_form.html.haml | 30 +-- app/views/proposal/index.html.haml | 12 +- config/routes.rb | 6 +- spec/features/proposal_spec.rb | 9 +- spec/models/ability_spec.rb | 39 +++ 13 files changed, 204 insertions(+), 231 deletions(-) delete mode 100644 app/models/admin_ability.rb create mode 100644 spec/models/ability_spec.rb diff --git a/Gemfile b/Gemfile index ed5ba3a5..77fa530c 100644 --- a/Gemfile +++ b/Gemfile @@ -20,8 +20,8 @@ gem 'omniauth-facebook' gem 'omniauth-openid' gem 'omniauth-google-oauth2' -# Use cancan as authorization framework -gem 'cancan' +# Use cancancan as authorization framework +gem 'cancancan' # Use transitions as state machine gem 'transitions', :require => %w( transitions active_record/transitions ) diff --git a/Gemfile.lock b/Gemfile.lock index 181a1c20..4a1db7dc 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -64,7 +64,7 @@ GEM byebug (3.1.2) columnize (~> 0.8) debugger-linecache (~> 1.2) - cancan (1.6.10) + cancancan (1.8.4) capybara (2.2.1) mime-types (>= 1.16) nokogiri (>= 1.3.3) @@ -399,7 +399,7 @@ DEPENDENCIES axlsx_rails bootstrap-sass byebug - cancan + cancancan capybara chart-js-rails cocoon diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index fade3e37..2a27b803 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -64,10 +64,6 @@ class ApplicationController < ActionController::Base redirect_to root_path unless has_role?(current_user, 'admin') end - def current_ability - @current_ability ||= AdminAbility.new(current_user) - end - rescue_from CanCan::AccessDenied do |exception| Rails.logger.debug("Access denied!") redirect_to root_path, alert: exception.message diff --git a/app/controllers/proposal_controller.rb b/app/controllers/proposal_controller.rb index 32c9cd87..dd2bf657 100644 --- a/app/controllers/proposal_controller.rb +++ b/app/controllers/proposal_controller.rb @@ -1,203 +1,162 @@ class ProposalController < ApplicationController before_filter :verify_user, except: [:show] - before_filter :setup - before_filter :verify_access, only: [:edit, :update, :destroy, :confirm, :restart] - - def setup - @user = current_user if current_user - # FIXME: @conference also comes from verify_user, but we need setup also in show - # which can be accessed anonymusly - @conference = Conference.find_by(short_title: params[:conference_id]) - @url = conference_proposal_index_path(@conference.short_title) - @event_types = @conference.event_types - end - - def verify_access - if params.has_key? :proposal_id - params[:id] = params[:proposal_id] - end - - begin - if !organizer_or_admin? - @event = @user.events.find(params[:id]) - else - @event = Event.find(params[:id]) - end - rescue => e - Rails.logger.debug("Proposal failure in verify_access: #{e.message}") - redirect_to(conference_proposal_index_path(conference_id: @conference.short_title), - alert: 'Invalid or uneditable proposal.') - end - end + before_action :set_conference, only: [:show] + before_action :set_event, only: [:show, :edit, :update, :destroy, :confirm, :restart] def index - @events = @user.proposals @conference + @events = current_user.proposals(@conference) end - def destroy - proposal = @user.events.find_by_id(params[:id]) - if proposal - proposal.withdraw - proposal.save - redirect_to(conference_proposal_index_path(conference_id: @conference.short_title), - alert: 'Proposal withdrawn.') - else - redirect_to(conference_proposal_index_path(conference_id: @conference.short_title), - alert: 'Error! Could not find proposal!') - end + def show + authorize! :show, @event + # FIXME: We should show more than the first speaker + @speaker = @event.speakers.first || @event.submitter end def new + authorize! :new, Event + @url = conference_proposal_index_path(@conference.short_title) @event = Event.new end def edit + authorize! :edit, @event @url = conference_proposal_path(@conference.short_title, params[:id]) - @event_types = @conference.event_types @attachments = @event.event_attachments - - if @event.nil? - redirect_to(conference_proposal_index_path(conference_id: @conference.short_title), - alert: 'Invalid or uneditable proposal.') - end - end - - def update - session[:return_to] ||= request.referer - submitter = params[:user] - - params[:event].delete :users_attributes - params[:event].delete :user - - if submitter[:name].blank? - redirect_to edit_conference_proposal_path(@conference.short_title, @event), - alert: 'Your name cannot be blank' - return - end - - if submitter[:biography].blank? - redirect_to edit_conference_proposal_path(@conference.short_title, @event), - alert: 'Your biography cannot be blank' - return - end - - if submitter[:name] != @user.name || submitter[:biography] != @user.biography - @user.update_attributes(submitter) - end - - event = Event.find_by_id(params[:id]) - - begin - event.update_attributes!(params[:event]) - redirect_to(conference_proposal_index_path(conference_id: @conference.short_title), - notice: "'#{event.title}' was successfully updated.") - rescue => e - redirect_to edit_conference_proposal_path(@conference.short_title, @event), alert: e.message - end end def create - user = current_user - session[:return_to] ||= request.referer + authorize! :create, Event + @url = conference_proposal_index_path(@conference.short_title) - event_params = params[:event] - submitter = params[:user] params[:event].delete :user - - @event = Event.new(event_params) + @event = Event.new(params[:event]) @event.conference = @conference - if submitter[:name].blank? - flash[:error] = 'Your public name cannot be blank.' - render action: 'new' - return - end - - if submitter[:biography].blank? - flash[:error] = 'Your biography cannot be blank.' - render action: 'new' - return - end - # First, update the submitter's info, if they've changed anything - if submitter[:name] != user.name || submitter[:biography] != user.biography - user.update_attributes(submitter) + current_user.assign_attributes(params[:user]) + if current_user.changed? + current_user.save end - @event.event_users.new(user: user, + @event.event_users.new(user: current_user, event_role: 'submitter') - @event.event_users.new(user: user, + @event.event_users.new(user: current_user, event_role: 'speaker') - begin - @event.save! - rescue => e - @url = conference_proposal_index_path(@conference.short_title) - @event_types = @conference.event_types - @user = current_user - - flash[:error] = "Could not submit proposal: #{e.message}" + if !@event.save + flash[:error] = "Could not submit proposal: #{@event.errors.full_messages.join(', ')}" render action: 'new' return end - registration = user.registrations.where(conference_id: @conference.id).first + registration = current_user.registrations.where(conference_id: @conference.id).first ahoy.track 'Event submission', title: 'New submission' if registration.nil? redirect_to(register_conference_path(@conference.short_title), - notice: 'Event was successfully submitted.\ - You probably want to register for the conference now!') + alert: 'Event was successfully submitted. + You should register for the conference now.') else redirect_to(conference_proposal_index_path(conference_id: @conference.short_title), notice: 'Event was successfully submitted.') end end - def show - @event = Event.find(params[:id]) - @speaker = @event.speakers.first || @event.submitter + def update + authorize! :update, @event + @url = conference_proposal_path(@conference.short_title, params[:id]) + + # First, update the submitter's info, if they've changed anything + current_user.assign_attributes(params[:user]) + if current_user.changed? + current_user.save + end + + # FIXME: Hmmmmm + params[:event].delete :users_attributes + params[:event].delete :user + + if !@event.update(params[:event]) + flash[:error] = "Could not update proposal: #{@event.errors.full_messages.join(', ')}" + render action: 'new' + return + end + + redirect_to(conference_proposal_index_path(conference_id: @conference.short_title), + notice: "Proposal was successfully updated.") + end + + def destroy + authorize! :destroy, @event + @url = conference_proposal_path(@conference.short_title, params[:id]) + + begin + @event.withdraw + rescue Transitions::InvalidTransition + redirect_to(:back, error: "Event can't be withdrawn") + return + end + + @event.save(validate: false) + redirect_to(conference_proposal_index_path(conference_id: @conference.short_title), + notice: "Proposal was successfully withdrawn.") end def confirm - if @event.transition_possible? :confirm - begin - @event.confirm! - rescue Transitions::InvalidTransition => e - redirect_to(conference_proposal_index_path(conference_id: @conference.short_title), - alert: "Event was NOT confirmed: #{e.message}") - return - end + authorize! :update, @event + @url = conference_proposal_path(@conference.short_title, params[:id]) - if !@conference.user_registered?(current_user) - redirect_to(register_conference_path(@conference.short_title), - notice: 'Event was confirmed. Please register to attend the conference.') - return - end - redirect_to(conference_proposal_index_path(conference_id: @conference.short_title), - notice: 'Event was confirmed.') - else - redirect_to(conference_proposal_index_path(conference_id: @conference.short_title), - alert: 'Event was NOT confirmed!') + begin + @event.confirm! + rescue Transitions::InvalidTransition + redirect_to(:back, error: "Event can't be confirmed") + return end + + if !@event.save + flash[:error] = "Could not confirm proposal: #{@event.errors.full_messages.join(', ')}" + render action: 'new' + return + end + + if !@conference.user_registered?(current_user) + redirect_to(register_conference_path(@conference.short_title), + alert: 'The proposal was confirmed. Please register to attend the conference.') + return + end + redirect_to(conference_proposal_index_path(conference_id: @conference.short_title), + notice: 'The proposal was confirmed.') end def restart - if @event.transition_possible? :restart - begin - @event.restart - @event.save - rescue Transitions::InvalidTransition => e - redirect_to(conference_proposal_index_path(conference_id: @conference.short_title), - alert: "Event was NOT restarted: #{e.message}") - return - end - # Success + authorize! :update, @event + @url = conference_proposal_path(@conference.short_title, params[:id]) + + begin + @event.restart + rescue Transitions::InvalidTransition redirect_to(conference_proposal_index_path(conference_id: @conference.short_title), - notice: 'Event was restarted. Review pending!') - else - # Error - redirect_to(conference_proposal_index_path(conference_id: @conference.short_title), - alert: 'Event was NOT restarted!') + error: "The proposal can't be re-submitted.") + return end + + if !@event.save + flash[:error] = "Could not re-submit proposal: #{@event.errors.full_messages.join(', ')}" + render action: 'new' + return + end + + redirect_to(conference_proposal_index_path(conference_id: @conference.short_title), + notice: "The proposal was re-submitted. The #{@conference.short_title} organizers will review it again.") + end + + private + + def set_conference + @conference = Conference.find_by(short_title: params[:conference_id]) + end + + def set_event + @event = Event.find(params[:id]) end end diff --git a/app/models/ability.rb b/app/models/ability.rb index 1ca0df1d..8c09734e 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -1,28 +1,17 @@ class Ability include CanCan::Ability - def initialize(user) # rubocop:disable Lint/UnusedMethodArgument - # Define abilities for the passed in user here. For example: - # - # user ||= User.new # guest user (not logged in) - # if user.admin? - # can :manage, :all - # else - # can :read, :all - # end - # - # The first argument to `can` is the action you are giving the user permission to do. - # If you pass :manage it will apply to every action. Other common actions here are - # :read, :create, :update and :destroy. - # - # The second argument is the resource the user can perform the action on. If you pass - # :all it will apply to every resource. Otherwise pass a Ruby class of the resource. - # - # The third argument is an optional hash of conditions to further filter the objects. - # For example, here the user can only update published articles. - # - # can :update, Article, :published => true - # - # See the wiki for details: https://github.com/ryanb/cancan/wiki/Defining-Abilities + def initialize(user) + # guest user (not logged in) + user ||= User.new + if user.admin? || user.organizer? + # An admin can manage everything + can :manage, :all + else + can [:update, :destroy], Event do |event| + event.users.include?(user) + end + can [:create, :read], Event + end end end diff --git a/app/models/admin_ability.rb b/app/models/admin_ability.rb deleted file mode 100644 index 8aff497a..00000000 --- a/app/models/admin_ability.rb +++ /dev/null @@ -1,17 +0,0 @@ -class AdminAbility - include CanCan::Ability - - def initialize(user) - @user = user || User.new # for guest - @user.get_roles.each { |role| send(role.name.downcase) } - end - - def organizer - can :manage, Event - end - - def admin - organizer - can :manage, :all - end -end diff --git a/app/models/event.rb b/app/models/event.rb index fcf32058..5020bfb9 100644 --- a/app/models/event.rb +++ b/app/models/event.rb @@ -31,10 +31,11 @@ class Event < ActiveRecord::Base validate :abstract_limit validate :before_end_of_conference - validate :biography_exists + validate :name_and_biography_exists validates :title, presence: true validates :abstract, presence: true validates :event_type, presence: true + validates :conference, presence: true validates :media_type, inclusion: { in: Conference.media_types.values }, allow_blank: true scope :confirmed, -> { where(state: 'confirmed') } @@ -218,8 +219,9 @@ class Event < ActiveRecord::Base errors.add(:abstract, "cannot have more than #{max_words} words") if len > max_words end - def biography_exists - errors.add(:user_biography, 'must be filled out') if submitter.biography_word_count == 0 + def name_and_biography_exists + errors.add(:biography, "cant' be blank") if submitter.biography.blank? + errors.add(:username, " can't be blank") if submitter.name.blank? end # TODO: create a module to be mixed into model to perform same operation diff --git a/app/models/user.rb b/app/models/user.rb index e8ab574a..c86c773e 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -52,6 +52,14 @@ class User < ActiveRecord::Base !!roles.find_by_name(role.to_s.downcase.camelize) end + def admin? + role?('Admin') + end + + def organizer? + role?('Organizer') + end + def get_roles roles end diff --git a/app/views/proposal/_proposal_form.html.haml b/app/views/proposal/_proposal_form.html.haml index 593c0669..86681512 100644 --- a/app/views/proposal/_proposal_form.html.haml +++ b/app/views/proposal/_proposal_form.html.haml @@ -14,7 +14,7 @@ %br %br - else - = f.input :event_type_id,:as => :select, :collection => @event_types.map {|x| ["#{x.title} - #{show_time(x.length)}", x.id]}, :include_blank => false, :label => "Session Type" + = f.input :event_type_id,:as => :select, :collection => @conference.event_types.map {|x| ["#{x.title} - #{show_time(x.length)}", x.id]}, :include_blank => false, :label => "Session Type" = f.input :difficulty_level, :as => :select, :collection => @conference.difficulty_levels, :include_blank => "(Please select)" if @conference.use_difficulty_levels = f.input :require_registration = f.input :abstract, :input_html => {:rows => 5, :class => "span11"}, @@ -41,22 +41,18 @@ = f.input :media_id, :label => "Media ID", :as => :string %section#information - = f.inputs :name => "Your Information" do - = semantic_fields_for @user do |u| - = u.input :name, :as => :string, :required => true - = u.input :affiliation, :as => :string, :hint => "This could be a company, a user group, or nothing at all." - = u.input :biography, :required => true, :input_html => {:rows => 5, :class => 'span11', "onkeyup" => "word_count(this, 'biography-count', 150)"} - You have used - %span#biography-count #{@user.biography_word_count} - words. Biographies are limited to 150 words. - %br - %br - %section#speakers - = f.inputs :name => "Additional Speakers" do - Will there be any additional speakers? If so, please enter their full names, email address, and a short - biography for each. - = f.input :proposal_additional_speakers, :input_html => {:rows => 5, :class => "span11"}, :label => false - = f.action :submit, :as => :button, :label => "Submit Session", :button_html => {:class => "btn btn-primary"} + - if current_user.name.blank? || current_user.biography.blank? + = f.inputs :name => "Your Information" do + = semantic_fields_for current_user do |u| + - if current_user.name.blank? + = u.input :name, :as => :string, :required => true + - if current_user.biography.blank? + = u.input :biography, :required => true, :input_html => {:rows => 5, :class => 'span11', "onkeyup" => "word_count(this, 'biography-count', 150)"} + You have used + %span#biography-count #{current_user.biography_word_count} + words. Biographies are limited to 150 words. + %p.text-right + = f.action :submit, :as => :button, :button_html => {:class => "btn btn-success"} :javascript var maxcount = 0; diff --git a/app/views/proposal/index.html.haml b/app/views/proposal/index.html.haml index c93456d3..96c8a4fc 100644 --- a/app/views/proposal/index.html.haml +++ b/app/views/proposal/index.html.haml @@ -3,8 +3,8 @@ %h1 = "My Proposals for #{@conference.title}" - if @conference.cfp_open? || organizer_or_admin? - = link_to "New Proposal", new_conference_proposal_path(@conference.short_title), :class => "btn btn-primary pull-right" -- if @user.proposal_count(@conference) > 0 + = link_to "New Proposal", new_conference_proposal_path(@conference.short_title), :class => "btn btn-success pull-right" +- if current_user.proposal_count(@conference) > 0 .row .col-md-12 %table.table.table-bordered.table-striped @@ -32,15 +32,15 @@ .pull-right - if event.transition_possible? :confirm = link_to 'Confirm', - conference_proposal_confirm_path(@conference.short_title, event), + confirm_conference_proposal_path(@conference.short_title, event), method: :patch, class: 'btn btn-mini btn-success', id: "confirm_proposal_#{event.id}" = link_to 'Edit', edit_conference_proposal_path(@conference.short_title, event.id), - class: 'btn btn-mini btn-primary', id: "edit_proposal_#{event.id}" + class: 'btn btn-mini btn-primary', id: "edit_proposal_#{event.id}" - if event.transition_possible? :withdraw = link_to 'Withdraw', conference_proposal_path(@conference.short_title, event.id), method: :delete, - confirm: 'Are you sure you want to withdraw this proposal?', class: 'btn btn-mini btn-danger', + confirm: 'Are you sure you want to withdraw this proposal?', class: 'btn btn-mini btn-warning', id: "delete_proposal_#{event.id}" - if event.state == 'withdrawn' || event.state == 'rejected' = link_to 'Re-Submit', - conference_proposal_restart_path(@conference.short_title, event), + restart_conference_proposal_path(@conference.short_title, event.id), method: :patch, class: 'btn btn-mini btn-success', id: "review_event_#{event.id}" diff --git a/config/routes.rb b/config/routes.rb index c62b3282..20c43a75 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -80,8 +80,10 @@ Osem::Application.routes.draw do resources :conference, only: [:show] do resources :proposal do resources :event_attachment, controller: "event_attachments" - patch '/confirm' => 'proposal#confirm' - patch '/restart' => 'proposal#restart' + member do + patch '/confirm' => 'proposal#confirm' + patch '/restart' => 'proposal#restart' + end end resource :schedule, only: [] do get "/" => "schedule#index" diff --git a/spec/features/proposal_spec.rb b/spec/features/proposal_spec.rb index 591015f7..fd25f51f 100644 --- a/spec/features/proposal_spec.rb +++ b/spec/features/proposal_spec.rb @@ -11,7 +11,7 @@ feature Event do feature: true, js: true do admin = create(:admin, email: 'admin@example.com') - participant = create(:participant, email: 'participant@example.com') + participant = create(:participant, email: 'participant@example.com', biography: "") expected_count = Event.count + 1 conference = create(:conference) @@ -37,9 +37,8 @@ feature Event do fill_in 'event_media_id', with: '123456' fill_in 'user_biography', with: 'Lorem ipsum biography' - fill_in 'user_name', with: 'Example User' - click_button 'Submit Session' + click_button 'Create Event' expect(current_path).to eq(register_conference_path(conference.short_title)) expect(Event.count).to eq(expected_count) @@ -77,7 +76,7 @@ feature Event do expect(page.has_content?('Unconfirmed')).to be true click_link "confirm_proposal_#{event.id}" expect(flash). - to eq('Event was confirmed. Please register to attend the conference.') + to eq('The proposal was confirmed. Please register to attend the conference.') # Register for conference find('#register').click @@ -87,7 +86,7 @@ feature Event do visit conference_proposal_index_path(conference.short_title) expect(page.has_content?('Confirmed')).to be true click_link "delete_proposal_#{event.id}" - expect(flash).to eq('Proposal withdrawn.') + expect(flash).to eq('Proposal was successfully withdrawn.') end end diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb new file mode 100644 index 00000000..e8a801f2 --- /dev/null +++ b/spec/models/ability_spec.rb @@ -0,0 +1,39 @@ +require 'spec_helper' +require "cancan/matchers" + +describe "User" do + describe "abilities" do + subject(:ability){ Ability.new(user) } + let(:user){ nil } + + context "when is an admin" do + let!(:user) { create(:admin) } + + it{ should be_able_to(:manage, Event.new) } + end + + context "when is an participant" do + let(:user) { build(:participant) } + + it{ should_not be_able_to(:manage, Event.new) } + it{ should be_able_to(:create, Event.new) } + it{ should be_able_to(:read, Event.new) } + end + + context "when is an event owner" do + let(:user) { create(:participant) } + let(:user2) { create(:participant) } + let(:myevent) { create(:event, users: [user]) } + let(:someevent) { create(:event, users: [user2]) } + + # Users are able to update and destroy their own events + it{ should be_able_to(:update, myevent) } + it{ should be_able_to(:destroy, myevent) } + + # Users are not able to update and destroy other users events + it{ should_not be_able_to(:update, someevent) } + it{ should_not be_able_to(:destroy, someevent) } + end + + end +end