From beb88b848dae898b52e8c2c369294e774ba4f9c0 Mon Sep 17 00:00:00 2001
From: Stella Rouzi <differentreality@gmail.com>
Date: Wed, 13 Aug 2014 22:46:41 +0300
Subject: [PATCH] changes from comments, fix commercials failing test

---
 app/assets/stylesheets/osem.css.scss          |  4 +++
 app/controllers/admin/campaigns_controller.rb |  2 +-
 .../admin/commercials_controller.rb           |  4 ++-
 .../admin/conference_controller.rb            | 22 ++++++---------
 app/controllers/admin/contacts_controller.rb  |  8 +-----
 app/controllers/admin/questions_controller.rb |  9 +++---
 app/controllers/admin/users_controller.rb     |  7 +++--
 .../admin/volunteers_controller.rb            | 13 ++++-----
 app/helpers/application_helper.rb             |  8 ++++++
 app/views/admin/conference/_roles.html.haml   | 10 +++----
 app/views/admin/questions/index.html.haml     | 10 ++++---
 app/views/admin/users/show.html.haml          |  2 +-
 config/routes.rb                              |  7 +++--
 ...0140730104658_migrate_roles_for_can_can.rb | 28 -------------------
 spec/features/ability_spec.rb                 |  1 -
 15 files changed, 58 insertions(+), 77 deletions(-)
 delete mode 100644 db/migrate/20140730104658_migrate_roles_for_can_can.rb

diff --git a/app/assets/stylesheets/osem.css.scss b/app/assets/stylesheets/osem.css.scss
index 4c4353c0..46e5d747 100644
--- a/app/assets/stylesheets/osem.css.scss
+++ b/app/assets/stylesheets/osem.css.scss
@@ -168,3 +168,7 @@ body {
   -webkit-flex: 2;
   flex: 2;
 }
+
+.table20 {
+  width: 20%;
+}
diff --git a/app/controllers/admin/campaigns_controller.rb b/app/controllers/admin/campaigns_controller.rb
index fc8f91b0..0c933f23 100644
--- a/app/controllers/admin/campaigns_controller.rb
+++ b/app/controllers/admin/campaigns_controller.rb
@@ -4,7 +4,7 @@ module Admin
     load_and_authorize_resource :campaign, through: :conference
 
     def index
-      authorize! :show, Campaign.new(conference_id: @conference.id)
+      authorize! :index, Campaign.new(conference_id: @conference.id)
       @campaigns = @conference.campaigns
     end
 
diff --git a/app/controllers/admin/commercials_controller.rb b/app/controllers/admin/commercials_controller.rb
index c2a927a9..632f396e 100644
--- a/app/controllers/admin/commercials_controller.rb
+++ b/app/controllers/admin/commercials_controller.rb
@@ -1,7 +1,7 @@
 module Admin
   class CommercialsController < ApplicationController
     load_and_authorize_resource :conference, find_by: :short_title
-    load_and_authorize_resource through: :conference
+    load_and_authorize_resource through: :conference, except: [:new, :create]
 
     def index
       @commercials = @conference.commercials
@@ -9,12 +9,14 @@ module Admin
 
     def new
       @commercial = @conference.commercials.build
+      authorize! :create, @commercial
     end
 
     def edit; end
 
     def create
       @commercial = @conference.commercials.build(commercial_params)
+      authorize! :create, @commercial
 
       if @commercial.save
         redirect_to admin_conference_commercials_path,
diff --git a/app/controllers/admin/conference_controller.rb b/app/controllers/admin/conference_controller.rb
index 345d0074..ca712700 100644
--- a/app/controllers/admin/conference_controller.rb
+++ b/app/controllers/admin/conference_controller.rb
@@ -202,17 +202,14 @@ module Admin
       @user = User.new
       @roles = Role::ACTIONABLES + Role::LABELS
 
-      params[:user] ? (@selected = params[:user][:roles]) : (@selected = 'Organizer')
-      @selection = @selected.parameterize.underscore
+      params[:user] ? (@selection = params[:user][:roles].parameterize.underscore) : (@selection = 'organizer')
       @role = Role.where(name: @selection, resource: @conference)
       @role_users = get_users(@selection)
     end
 
     def add_user
       user = User.find_by(email: params[:user][:email])
-      @selected = params[:role]
-      @selection = @selected.parameterize.underscore
-
+      @selection = params[:role].parameterize.underscore
       @role_users = get_users(@selection)
 
       user.add_role @selection.to_sym, @conference
@@ -220,23 +217,22 @@ module Admin
     end
 
     def remove_user
-      @selected = params[:role]
-      @selection = @selected.parameterize.underscore
-      role = Role.where(name: @selection, resource: @conference).first
-
+      @selection = params[:role]
       @role_users = get_users(@selection)
 
-      @user.revoke role.name.to_sym, @conference
+      @user.revoke @selection.to_sym, @conference
       render 'roles', formats: [:js]
     end
 
     protected
 
-    def get_users(role)
+    def get_users(role_name)
       @role_users = {}
-      get_role = Role.where(name: role, resource: @conference)
-      get_role.blank? ? @role_users[role] = get_role : @role_users[role] = get_role.first.users
+      role = Role.where(name: role_name, resource: @conference)
+      role.blank? ? @role_users[role_name] = role : @role_users[role_name] = role.first.users
 
+      # Initialize @role variable, so that view can show the role description
+      @role = Role.where(name: role_name, resource: @conference)
       @role_users
     end
   end
diff --git a/app/controllers/admin/contacts_controller.rb b/app/controllers/admin/contacts_controller.rb
index cf9c8599..2507f4dc 100644
--- a/app/controllers/admin/contacts_controller.rb
+++ b/app/controllers/admin/contacts_controller.rb
@@ -12,18 +12,12 @@ module Admin
     # PATCH/PUT /:conference/contact
     def update
       if @contact.update(contact_params)
-        redirect_to admin_conference_contact_path, notice: 'Contact details were successfully updated.'
+        redirect_to edit_admin_conference_contact_path, notice: 'Contact details were successfully updated.'
       else
         render :edit
       end
     end
 
-    # DELETE /:conference/contact
-    def destroy
-      @contact.destroy
-      redirect_to admin_conference_contacts_url, notice: 'Contact details were successfully destroyed.'
-    end
-
     private
     # Only allow a trusted parameter "white list" through.
     def contact_params
diff --git a/app/controllers/admin/questions_controller.rb b/app/controllers/admin/questions_controller.rb
index 7d17c9cf..f64af6c2 100644
--- a/app/controllers/admin/questions_controller.rb
+++ b/app/controllers/admin/questions_controller.rb
@@ -4,7 +4,7 @@ module Admin
     load_and_authorize_resource through: :conference, except: [:new, :create]
 
     def index
-      authorize! :update, Question.new(conference_id: @conference.id)
+      authorize! :index, Question.new(conference_id: @conference.id)
       @questions = Question.where(global: true).all | Question.where(conference_id: @conference.id)
       @questions_conference = @conference.questions
       @new_question = @conference.questions.new
@@ -32,7 +32,7 @@ module Admin
 
     # GET questions/1/edit
     def edit
-      if @question.global == true && !(current_user.has_role? :organizer, @conference)
+      if @question.global
         redirect_to(admin_conference_questions_path(conference_id: @conference.short_title), alert: "Sorry, you cannot edit global questions. Create a new one.")
       end
     end
@@ -48,6 +48,7 @@ module Admin
 
     # Update questions used for the conference
     def update_conference
+      authorize! :update, Question.new(conference_id: @conference.id)
       if @conference.update_attributes(params[:conference])
         redirect_to(admin_conference_questions_path(conference_id: @conference.short_title), notice: "Questions for #{@conference.short_title} successfully updated.")
       else
@@ -59,7 +60,7 @@ module Admin
     def destroy
       if can? :destroy, @question
         # Do not delete global questions
-        if @question.global == false
+        if !@question.global
 
           # Delete question and its answers
           begin
@@ -67,7 +68,7 @@ module Admin
 
               @question.destroy
               @question.answers.each do |a|
-                a.delete
+                a.destroy
               end
               flash[:notice] = "Deleted question: #{@question.title} and its answers: #{@question.answers.map {|a| a.title}.join ','}"
             end
diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index 1ce567cb..cf41882e 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -20,8 +20,11 @@ module Admin
     end
 
     def update
-      @user.update_attributes!(params[:user])
-      redirect_to admin_users_path, notice: "Updated #{@user.email}"
+      if @user.update_attributes(params[:user])
+        redirect_to admin_users_path, notice: "Updated #{@user.email}"
+      else
+        redirect_to admin_users_path, alert: "Could not update #{@user.name}. #{@user.errors.full_messages.join '. '}."
+      end
     end
 
     def edit; end
diff --git a/app/controllers/admin/volunteers_controller.rb b/app/controllers/admin/volunteers_controller.rb
index d7a69765..00b06c90 100644
--- a/app/controllers/admin/volunteers_controller.rb
+++ b/app/controllers/admin/volunteers_controller.rb
@@ -3,7 +3,7 @@ module Admin
     load_and_authorize_resource :conference, find_by: :short_title
 
     def index
-      if (current_user.has_role? :organizer, @conference) || (current_user.has_role? :volunteer_coordinator, @conference)
+      if can_manage_volunteers(@conference)
         render :index
       else
         authorize! :index, :volunteer
@@ -11,7 +11,7 @@ module Admin
     end
 
     def show
-      if (current_user.has_role? :organizer, @conference) || (current_user.has_role? :volunteer_coordinator, @conference)
+      if can_manage_volunteers(@conference)
         if @conference.use_vpositions
           @volunteers = @conference.registrations.joins(:vchoices).uniq
         else
@@ -23,12 +23,11 @@ module Admin
     end
 
     def update
-      if (current_user.has_role? :organizer, @conference) || (current_user.has_role? :volunteer_coordinator, @conference)
-        begin
-          @conference.update_attributes!(params[:conference])
+      if can_manage_volunteers(@conference)
+        if @conference.update_attributes(params[:conference])
           redirect_to(admin_conference_volunteers_info_path(conference_id: params[:conference_id]), notice: "Volunteering options were successfully updated.")
-        rescue => e
-          redirect_to(admin_conference_volunteers_info_path(conference_id: params[:conference_id]), alert: "Volunteering options update failed: #{e.message}")
+        else
+          redirect_to(admin_conference_volunteers_info_path(conference_id: params[:conference_id]), alert: "Volunteering options update failed: #{@conference.errors.full_messages.join '. '}")
         end
       else
         authorize! :index, :volunteer
diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index 35e4acb2..caa1e560 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -203,4 +203,12 @@ module ApplicationHelper
   def show_roles(roles)
     roles.map { |x| x[0].titleize + ' ' + x[1] }.join ', '
   end
+
+  def can_manage_volunteers(conference)
+    if (current_user.has_role? :organizer, conference) || (current_user.has_role? :volunteer_coordinator, conference)
+      true
+    else
+      false
+    end
+  end
 end
diff --git a/app/views/admin/conference/_roles.html.haml b/app/views/admin/conference/_roles.html.haml
index 9d0f99d6..54235af3 100644
--- a/app/views/admin/conference/_roles.html.haml
+++ b/app/views/admin/conference/_roles.html.haml
@@ -6,13 +6,13 @@
   %hr
   .row
     .col-md-6
-      = semantic_form_for(:user, url: add_user_admin_conference_path(@conference.short_title, role: @selected), remote: true) do |f|
+      = semantic_form_for(:user, url: add_user_admin_conference_path(@conference.short_title, role: @selection), remote: true) do |f|
         %h4
-          = f.input :email, label: "Add role '#{@selected}' to user: ", placeholder: "User's email"
-        = f.action :submit, as: :button, label: "Add User", button_html: {value: 'Add', class: 'btn btn-primary'}
+          = f.input :email, label: "Add role '#{@selection.humanize.titleize}' to user: ", placeholder: "User's email", input_html: { required: 'required' }
+        = f.action :submit, as: :button, label: 'Add User', button_html: {value: 'Add', class: 'btn btn-primary'}
   .row
     .col-md-12
-      %h3 Users with role #{@selected}
+      %h3 Users with role #{@selection.humanize.titleize}
       %table.table.table-striped.table-bordered.table-hover
         %thead
           %th ID
@@ -22,7 +22,7 @@
           - @role_users[@selection].each do |user|
             %tr
               %td
-                = link_to remove_user_admin_conference_path(@conference.short_title, user_id: user.id, role: @selected), method: :delete, remote: true, title: 'Remove user' do
+                = link_to remove_user_admin_conference_path(@conference.short_title, user_id: user.id, role: @selection), method: :delete, remote: true, title: 'Remove user' do
                   %i{class: 'fa fa-times'}
                 = user.id
                 %td= user.name
diff --git a/app/views/admin/questions/index.html.haml b/app/views/admin/questions/index.html.haml
index e34e997a..7810ddb7 100644
--- a/app/views/admin/questions/index.html.haml
+++ b/app/views/admin/questions/index.html.haml
@@ -1,16 +1,18 @@
 .row
   .col-md-8
     .pull-right
-      %b= link_to 'Create New Question','#', 'data-toggle' => 'modal',
-                  'data-target' => '#new-question', class: 'btn btn-success'
+      - if can? :create, Question.new(conference_id: @conference.id)
+        %b= link_to 'Create New Question','#', 'data-toggle' => 'modal',
+                    'data-target' => '#new-question', class: 'btn btn-success'
       %br
       %br
     - if @questions.count > 0
       = semantic_form_for(@conference, url: admin_conference_questions_update_conference_path(@conference.short_title)) do |f|
         .questions{id: 'myquestions'}
           = render partial: 'questions'
-        = f.submit "Update Questions for #{@conference.short_title}", class: 'btn btn-primary',
-                    confirm: 'Are you sure you want to make these changes?'
+        - if can? :update, @conference
+          = f.submit "Update Questions for #{@conference.short_title}", class: 'btn btn-primary',
+                      confirm: 'Are you sure you want to make these changes?'
 
     .modal.fade{id: 'new-question', 'role' => 'dialog', 'aria-hidden' => 'true'}
       .modal-dialog
diff --git a/app/views/admin/users/show.html.haml b/app/views/admin/users/show.html.haml
index ed4632d2..161d8708 100644
--- a/app/views/admin/users/show.html.haml
+++ b/app/views/admin/users/show.html.haml
@@ -4,7 +4,7 @@
 %table.table
   - @show_attributes.each do |attr|
     %tr
-      %td{style: 'width:20%'}
+      %td{class: 'table20'}
         %b
           = attr.capitalize.gsub('_', ' ')
       - if attr == 'roles'
diff --git a/config/routes.rb b/config/routes.rb
index ad2b4c84..89fc6e9e 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -98,10 +98,11 @@ Osem::Application.routes.draw do
     resource :schedule, only: [] do
       get "/" => "schedule#index"
     end
+
+    get "/register" => "conference_registration#register"
+    patch "/register" => "conference_registration#update"
+    delete "/register" => "conference_registration#unregister"
     member do
-      get "/register" => "conference_registration#register"
-      patch "/register" => "conference_registration#update"
-      delete "/register" => "conference_registration#unregister"
       get "gallery_photos"
       patch "subscription" => "conference#subscribe"
       delete "subscription" => "conference#unsubscribe"
diff --git a/db/migrate/20140730104658_migrate_roles_for_can_can.rb b/db/migrate/20140730104658_migrate_roles_for_can_can.rb
deleted file mode 100644
index 4c833280..00000000
--- a/db/migrate/20140730104658_migrate_roles_for_can_can.rb
+++ /dev/null
@@ -1,28 +0,0 @@
-class MigrateRolesForCanCan < ActiveRecord::Migration
-
-  def up
-    # Store the number of existing roles
-    old_roles = Role.count
-
-    Role.all.each do |role|
-      role.users.each do |user|
-        Conference.all.each do |conference|
-          if role.name == 'Admin' || role.name == 'Organizer'
-            user.add_role :organizer, conference
-          else
-            user.add_role role.name.parameterize.underscore.to_sym, conference
-          end
-        end
-      end
-    end
-
-    # Delete old roles
-    Role.first(old_roles).each do |role|
-      role.destroy
-    end
-  end
-
-  def down
-    raise ActiveRecord::IrreversibleMigration.new('Cannot reverse migration. Deleted events cannot be re-created')
-  end
-end
diff --git a/spec/features/ability_spec.rb b/spec/features/ability_spec.rb
index ba32a52b..a651ece0 100644
--- a/spec/features/ability_spec.rb
+++ b/spec/features/ability_spec.rb
@@ -205,7 +205,6 @@ feature 'Has correct abilities' do
   scenario 'when user is volunteer coordinator' do
     sign_in user
     visit admin_conference_path(conference4.short_title)
-    save_and_open_page
 
     expect(page.has_content?('Basics')).to be true
     expect(page.has_content?('Dashboard')).to be true