Fix broken ToS checkbox handling

Resolves #1227
- add integration specs for ToS checkbox
- fix banned user view handling
- remove check_tos method and only update user if checkout succeeds
- tweak JS confirmation message on manage reservation page

Author: orenyk

app/assets/javascripts/manage_reservation.js

@@ -37,7 +37,10 @@ function validate_checkin(){
 
   function confirm_checkinout(flag){
     if (flag){
-      if( confirm("Oops! We've noticed one of the following issues:\n\nYou checked off procedures for an item you're not checking in/out.\n\n       or\n\nYou didn't check off all procedures for an item that you are checking in/out.\n\nAre you sure you want to continue?")){
+      if( confirm("Oops! We've noticed one of the following issues:\n\n"+
+        "You didn't check off all procedures for an item your're checking "+
+        "in/out,\n\n\tor\n\nYou didn't select an item for a reservation you "+
+        "checked off procedures for.\n\nAre you sure you want to continue?")){
         (this).submit();
         return false;
       } else {

app/controllers/application_controller.rb

@@ -312,21 +312,6 @@ def markdown_help
     end
   end
 
-  # Checks if params[:terms_of_service_accepted] is necessary; if filled-out,
-  # saves the state of the user; if not filled out and necessary, returns false.
-  # Otherwise, returns true.
-  def check_tos(user)
-    return true if user.terms_of_service_accepted
-
-    user.terms_of_service_accepted = params[:terms_of_service_accepted].present?
-    if user.terms_of_service_accepted
-      user.save
-    else
-      (flash[:error] = 'You must confirm that the user accepts the Terms of '\
-      'Service.') && false
-    end
-  end
-
   private
 
   # modify redirect after signing in

app/controllers/reservations_controller.rb

@@ -14,6 +14,7 @@ def set_user
     @user = User.find(params[:user_id])
     return unless @user.role == 'banned'
     flash[:error] = 'This user is banned and cannot check out equipment.'
+    params[:banned] = true
   end
 
   def set_reservation
@@ -260,7 +261,13 @@ def checkout # rubocop:disable all
 
     ## Basic-logic checks, only need to be done once
 
-    redirect_to(:back) && return unless check_tos(@user)
+    # check terms of service
+    unless @user.terms_of_service_accepted ||
+           params[:terms_of_service_accepted].present?
+      flash[:error] = 'You must confirm that the user accepts the Terms of '\
+        'Service.'
+      redirect_to(:back) && return
+    end
 
     if checked_out_reservations.empty?
       flash[:error] = 'No reservation selected.'
@@ -297,6 +304,11 @@ def checkout # rubocop:disable all
       end
     end
 
+    # update user with terms of service acceptance now that checkout worked
+    unless @user.terms_of_service_accepted
+      @user.update_attributes(terms_of_service_accepted: true)
+    end
+
     # prep for receipt page and exit
     @check_in_set = []
     @check_out_set = checked_out_reservations
@@ -355,15 +367,19 @@ def upcoming
   end
 
   def manage # initializer
-    redirect_to(root_path) && return unless flash[:error].nil?
+    if params[:banned] && current_user.view_mode != 'superuser'
+      redirect_to(root_path) && return
+    end
     @check_out_set = @user.due_for_checkout
     @check_in_set = @user.due_for_checkin
 
     render :manage, layout: 'application'
   end
 
   def current
-    redirect_to(root_path) && return unless flash[:error].nil?
+    if params[:banned] && current_user.view_mode != 'superuser'
+      redirect_to(root_path) && return
+    end
     @user_overdue_reservations_set =
       [Reservation.overdue.for_reserver(@user)].delete_if(&:empty?)
     @user_checked_out_today_reservations_set =

spec/controllers/reservations_controller_spec.rb

@@ -1054,16 +1054,33 @@
       before { put :checkout, user_id: @banned.id }
     end
 
-    context 'when tos returns false' do
+    context 'when tos not accepted and not checked off' do
       before do
         request.env['HTTP_REFERER'] = 'where_i_came_from'
         sign_in @admin
-        allow(@controller).to receive(:check_tos).and_return(false)
+        @user.update_attributes(terms_of_service_accepted: false)
         put :checkout, user_id: @user.id, reservations: {}
       end
       it { expect(response).to redirect_to 'where_i_came_from' }
     end
 
+    context 'when tos accepted' do
+      before do
+        sign_in @admin
+        @user.update_attributes(terms_of_service_accepted: false)
+        @item =
+          FactoryGirl.create(:equipment_item,
+                             equipment_model: @reservation.equipment_model)
+        reservations_params =
+          { @reservation.id.to_s => { notes: '',
+                                      equipment_item_id: @item.id } }
+        put :checkout, user_id: @user.id, reservations: reservations_params,
+                       terms_of_service_accepted: true
+      end
+
+      it { expect(response).to be_success }
+    end
+
     context 'when not all procedures are filled out' do
       before do
         sign_in @admin

spec/features/reservations_spec.rb

@@ -180,6 +180,7 @@
     before(:each) do
       @res = FactoryGirl.create :valid_reservation, reserver: @user,
                                                     equipment_model: @eq_model
+      visit manage_reservations_for_user_path(@user)
     end
 
     shared_examples 'can handle reservation transactions' do
@@ -209,6 +210,14 @@
       end
     end
 
+    context 'as guest' do
+      it 'should redirect to the catalog page' do
+        visit manage_reservations_for_user_path(@user)
+        expect(page).to have_content 'Sign In'
+        expect(page.current_url).to eq(new_user_session_url)
+      end
+    end
+
     context 'as patron' do
       before { sign_in_as_user(@user) }
       after { sign_out }
@@ -240,6 +249,64 @@
 
       it_behaves_like 'can handle reservation transactions'
     end
+
+    context 'ToS checkbox' do
+      before(:each) do
+        @user.update_attributes(terms_of_service_accepted: false)
+      end
+
+      shared_examples 'can utilize the ToS checkbox' do
+        before(:each) { visit manage_reservations_for_user_path(@user) }
+
+        it 'fails when the box isn\'t checked off' do
+          # skip the checkbox
+          select "#{@eq_model.equipment_items.first.name}",
+                 from: 'Equipment Item'
+          click_button 'Check-Out Equipment'
+
+          expect(page).to have_content 'You must confirm that the user '\
+            'accepts the Terms of Service.'
+          expect(page.current_url).to \
+            eq(manage_reservations_for_user_url(@user))
+        end
+
+        it 'succeeds when the box is checked off' do
+          check 'terms_of_service_accepted'
+          select "#{@eq_model.equipment_items.first.name}",
+                 from: 'Equipment Item'
+          click_button 'Check-Out Equipment'
+
+          expect(page).to have_content 'Check-Out Receipt'
+          expect(page).to have_content current_user.name
+          @res.reload
+          expect(@res.equipment_item_id).to \
+            eq(@eq_model.equipment_items.first.id)
+          expect(@res.checkout_handler).to eq(current_user)
+          expect(@res.checked_out).not_to be_nil
+        end
+      end
+
+      context 'as checkout person' do
+        before { sign_in_as_user(@checkout_person) }
+        after { sign_out }
+
+        it_behaves_like 'can utilize the ToS checkbox'
+      end
+
+      context 'as admin' do
+        before { sign_in_as_user(@admin) }
+        after { sign_out }
+
+        it_behaves_like 'can utilize the ToS checkbox'
+      end
+
+      context 'as superuser' do
+        before { sign_in_as_user(@superuser) }
+        after { sign_out }
+
+        it_behaves_like 'can utilize the ToS checkbox'
+      end
+    end
   end
 
   context 'renewing reservations' do