Merge pull request #8 from bmic-development/kg-dashboard_visit_groups…

…_controller_specs

KG - Dashboard Visit Groups Controller Specs / Day Validation Bug

app/controllers/dashboard/visit_groups_controller.rb

@@ -20,28 +20,26 @@
 
 class Dashboard::VisitGroupsController < Dashboard::BaseController
   respond_to :json, :html
-  before_action :find_visit_group, only: [:update, :destroy]
+
+  before_action :find_visit_group,          only: [:update, :destroy]
+  before_action :find_service_request
+  before_action :find_sub_service_request
+  before_action :find_protocol,             only: [:new, :navigate]
+  before_action :authorize_admin_visit_group
 
   def new
-    @service_request = ServiceRequest.find(params[:service_request_id])
-    @sub_service_request = SubServiceRequest.find(params[:sub_service_request_id])
     @current_page = params[:current_page] # the current page of the study schedule
-    @protocol = Protocol.find(params[:protocol_id])
-    @visit_group = VisitGroup.new
     @schedule_tab = params[:schedule_tab]
-    @arm = params[:arm_id].present? ? Arm.find(params[:arm_id]) : @protocol.arms.first
+    @visit_group  = VisitGroup.new
+    @arm          = params[:arm_id].present? ? Arm.find(params[:arm_id]) : @protocol.arms.first
   end
 
   def create
-    @service_request = ServiceRequest.find(params[:service_request_id])
-    @sub_service_request = SubServiceRequest.find(params[:sub_service_request_id])
-    @arm =  Arm.find(params[:visit_group][:arm_id])
-    @visit_group = VisitGroup.new(params[:visit_group])
+    @arm          = Arm.find(params[:visit_group][:arm_id])
+    @visit_group  = VisitGroup.new(params[:visit_group])
+
     if @visit_group.valid?
       if @arm.add_visit(@visit_group.position, @visit_group.day, @visit_group.window_before, @visit_group.window_after, @visit_group.name, 'true')
-        @service_request.relevant_service_providers_and_super_users.each do |identity|
-          create_visit_change_toast(identity, @sub_service_request) unless identity == @user
-        end
         flash[:success] = t(:dashboard)[:visit_groups][:created]
       else
         @errors = @arm.errors
@@ -53,24 +51,21 @@ def create
 
   def navigate
     # Used in study schedule management for navigating to a visit group, given an index of them by arm.
-    @protocol = Protocol.find(params[:protocol_id])
-    @service_request = ServiceRequest.find(params[:service_request_id])
-    @sub_service_request = SubServiceRequest.find(params[:sub_service_request_id])
     @intended_action = params[:intended_action]
+
     if params[:visit_group_id]
-      @visit_group = VisitGroup.find(params[:visit_group_id])
-      @arm = @visit_group.arm
+      @visit_group  = VisitGroup.find(params[:visit_group_id])
+      @arm          = @visit_group.arm
     else
-      @arm = params[:arm_id].present? ? Arm.find(params[:arm_id]) : @protocol.arms.first
-      @visit_group = @arm.visit_groups.first
+      @arm          = params[:arm_id].present? ? Arm.find(params[:arm_id]) : @protocol.arms.first
+      @visit_group  = @arm.visit_groups.first
     end
   end
 
   def update
-    @service_request = ServiceRequest.find(params[:service_request_id])
-    @sub_service_request = SubServiceRequest.find(params[:sub_service_request_id])
-    @arm = @visit_group.arm
+    @arm                            = @visit_group.arm
     params[:visit_group][:position] = params[:visit_group][:position].to_i - 1
+
     if @visit_group.update_attributes(params[:visit_group])
       flash[:success] = t(:dashboard)[:visit_groups][:updated]
     else
@@ -79,14 +74,10 @@ def update
   end
 
   def destroy
-    @service_request = ServiceRequest.find(params[:service_request_id])
-    @sub_service_request = SubServiceRequest.find(params[:sub_service_request_id])
     @arm = @visit_group.arm
+
     if @arm.remove_visit(@visit_group.position)
       @arm.decrement!(:minimum_visit_count)
-      @service_request.relevant_service_providers_and_super_users.each do |identity|
-        create_visit_change_toast(identity, @sub_service_request) unless identity == @user
-      end
       flash.now[:alert] = t(:dashboard)[:visit_groups][:destroyed]
     else
       @errors = @arm.errors
@@ -99,13 +90,28 @@ def find_visit_group
     @visit_group = VisitGroup.find(params[:id])
   end
 
-  def create_visit_change_toast identity, sub_service_request
-    ToastMessage.create(
-      to: identity.id,
-      from: current_identity.id,
-      sending_class: 'SubServiceRequest',
-      sending_class_id: sub_service_request.id,
-      message: 'The visit count on this service request has been changed'
-    )
+  def find_service_request
+    @service_request = ServiceRequest.find(params[:service_request_id])
+  end
+
+  def find_sub_service_request
+    @sub_service_request = SubServiceRequest.find(params[:sub_service_request_id])
+  end
+
+  def find_protocol
+    @protocol = Protocol.find(params[:protocol_id])
+  end
+
+  def authorize_admin_visit_group
+    unless (@user.authorized_admin_organizations & @sub_service_request.org_tree).any?
+      @protocol            = nil
+      @service_request     = nil
+      @sub_service_request = nil
+      @visit_group         = nil
+
+      # This is an intruder
+      flash[:alert] = t(:authorization_error)[:dashboard][:visit_groups]
+      redirect_to dashboard_root_path
+    end
   end
 end

app/controllers/visit_groups_controller.rb

@@ -31,7 +31,24 @@ def update
     if @visit_group.update_attributes(visit_group_params)
       render nothing: true
     else
-      render json: @visit_group.errors, status: :unprocessable_entity
+      # If we update the visit group day, then @visit_group.day is already updated, therefore
+      # any errors for day are not deleted.
+      # If we update a different attribute, then day will be nil and the errors for day
+      # will be deleted.
+      if @visit_group.day.nil?
+        @visit_group.errors.delete(:day)
+      end
+
+      # If there are legitimate errors, render them
+      # Else, it means day validation caused the update_attributes to fail even though we didn't
+      # change it, so we ignore validation and update the attribute correctly.
+      if @visit_group.errors.any?
+        render json: @visit_group.errors, status: :unprocessable_entity
+      else
+        @visit_group.attributes = visit_group_params
+        @visit_group.save(validate: false)
+        render nothing: true
+      end
     end
   end
 

app/models/visit_group.rb

@@ -48,12 +48,9 @@ class VisitGroup < ActiveRecord::Base
   validates :window_before,
             :window_after,
             presence: true, numericality: { only_integer: true }
-  validates :day, presence: true, numericality: { only_integer: true }, if: :day_or_no_attr_changed?
+  validates :day, presence: true, numericality: { only_integer: true }
 
-  # TODO: fix. Currently, this validation fails for all VisitGroups with
-  # position == 0. This fails because the position attribute is changed
-  # to 1 from 0 by, I think, acts_as_list before validations are performed.
-  validate :day_must_be_in_order, if: :no_attr_changed?
+  validate :day_must_be_in_order
 
   def set_arm_edited_flag_on_subjects
     self.arm.set_arm_edited_flag_on_subjects
@@ -106,12 +103,4 @@ def day_must_be_in_order
       errors.add(:day, 'must be in order')
     end
   end
-
-  def day_or_no_attr_changed?
-    [[], ["day"]].include? changed
-  end
-
-  def no_attr_changed?
-    changed.empty?
-  end
 end

config/locales/en.yml

@@ -92,6 +92,8 @@ en:
   authorization_error:
     header: "Authorization Error"
     link: "Return to your Dashboard"
+    dashboard:
+      visit_groups: 'You are not allowed to access that Visit Group.'
 
   ####################
   # AUTHORIZED USERS #

spec/controllers/dashboard/visit_groups/delete_destroy_spec.rb

@@ -0,0 +1,109 @@
+# Copyright © 2011-2016 MUSC Foundation for Research Development~
+# All rights reserved.~
+
+# Redistribution and use in source and binary forms, with or without modification, are permitted provided that the following conditions are met:~
+
+# 1. Redistributions of source code must retain the above copyright notice, this list of conditions and the following disclaimer.~
+
+# 2. Redistributions in binary form must reproduce the above copyright notice, this list of conditions and the following~
+# disclaimer in the documentation and/or other materials provided with the distribution.~
+
+# 3. Neither the name of the copyright holder nor the names of its contributors may be used to endorse or promote products~
+# derived from this software without specific prior written permission.~
+
+# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING,~
+# BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT~
+# SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL~
+# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS~
+# INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR~
+# TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.~
+
+require 'rails_helper'
+
+RSpec.describe Dashboard::VisitGroupsController do
+  describe '#destroy' do
+    let!(:logged_in_user) { create(:identity) }
+
+    context 'users with authorization' do
+      before :each do
+        log_in_dashboard_identity(obj: logged_in_user)
+
+        org       = create(:organization)
+                    create(:super_user, organization: org, identity: logged_in_user)
+        @protocol = create(:protocol_federally_funded, primary_pi: logged_in_user)
+        @sr       = create(:service_request_without_validations, protocol: @protocol)
+        @ssr      = create(:sub_service_request, service_request: @sr, organization: org)
+        @arm      = create(:arm, protocol: @protocol)
+        @vg       = create(:visit_group, arm: @arm)
+
+        xhr :delete, :destroy, {
+          id: @vg.id,
+          service_request_id: @sr.id,
+          sub_service_request_id: @ssr.id
+        }
+      end
+
+      it 'should assign @visit_group' do
+        expect(assigns(:visit_group)).to eq(@vg)
+      end
+
+      it 'should assign @service_request' do
+        expect(assigns(:service_request)).to eq(@sr)
+      end
+
+      it 'should assign @sub_service_request' do
+        expect(assigns(:sub_service_request)).to eq(@ssr)
+      end
+
+      it 'should assign @arm' do
+        expect(assigns(:arm)).to eq(@arm)
+      end
+
+      it 'should destroy the visit group' do
+        expect(VisitGroup.count).to eq(0)
+      end
+
+      it 'should decrement the arm visit count' do
+        expect(@arm.reload.visit_count).to eq(0)
+      end
+
+      it 'should render template' do
+        expect(controller).to render_template(:destroy)
+      end
+
+      it 'should respond ok' do
+        expect(controller).to respond_with(:ok)          
+      end
+    end
+
+    context 'users without authorization' do
+      before :each do
+        log_in_dashboard_identity(obj: logged_in_user)
+
+        org       = create(:organization)
+        @protocol = create(:protocol_federally_funded, primary_pi: logged_in_user)
+        @sr       = create(:service_request_without_validations, protocol: @protocol)
+        @ssr      = create(:sub_service_request, service_request: @sr, organization: org)
+        @arm      = create(:arm, protocol: @protocol)
+        @vg       = create(:visit_group, arm: @arm)
+
+        xhr :delete, :destroy, {
+          id: @vg.id,
+          service_request_id: @sr.id,
+          sub_service_request_id: @ssr.id
+        }
+      end
+
+      it 'should not assign variables' do
+        expect(assigns(:protocol)).to eq(nil)
+        expect(assigns(:service_request)).to eq(nil)
+        expect(assigns(:sub_service_request)).to eq(nil)
+        expect(assigns(:visit_group)).to eq(nil)
+      end
+
+      it { is_expected.to redirect_to(dashboard_root_path) }
+
+      it { is_expected.to respond_with(302) }
+    end
+  end
+end