Merge pull request #1957 from sparc-request/ac-current-user-delayed-j…

…ob-fix

Fixing issue with delayed job not knowing current user on model save

app/controllers/service_requests_controller.rb

@@ -155,7 +155,7 @@ def confirmation
   def save_and_exit
     respond_to do |format|
       format.html {
-        @service_request.update_status('draft')
+        @service_request.update_status('draft', current_user)
         @service_request.ensure_ssr_ids
         redirect_to dashboard_root_path
       }

app/lib/notifier_logic.rb

@@ -51,14 +51,14 @@ def update_ssrs_and_send_emails
     # @to_notify holds the SSRs that require an "initial submission" email
     @send_request_amendment_and_not_initial = @ssrs_updated_from_un_updatable_status.present? || @destroyed_ssrs_needing_notification.present? || @created_ssrs_needing_notification.present?
     @service_request.previous_submitted_at = @service_request.submitted_at
-    @to_notify = @service_request.update_status('submitted')
+    @to_notify = @service_request.update_status('submitted', @current_user)
     @service_request.update_arm_minimum_counts
     send_request_amendment_email_evaluation
     send_initial_submission_email
   end
 
   def update_status_and_send_get_a_cost_estimate_email
-    to_notify = @service_request.update_status('get_a_cost_estimate')
+    to_notify = @service_request.update_status('get_a_cost_estimate', @current_user)
     sub_service_requests = @service_request.sub_service_requests.where(id: to_notify)
     if !sub_service_requests.empty? # if nothing is set to notify then we shouldn't send out e-mails
       send_user_notifications(request_amendment: false, admin_delete_ssr: false, deleted_ssr: nil)

app/models/service_request.rb

@@ -422,13 +422,13 @@ def relevant_service_providers_and_super_users
 
   # Returns the SSR ids that need an initial submission email, updates the SR status,
   # and updates the SSR status to new status if appropriate
-  def update_status(new_status)
+  def update_status(new_status, current_user)
     # Do not change the Service Request if it has been submitted
     update_attribute(:status, new_status) unless self.previously_submitted?
     update_attribute(:submitted_at, Time.now) if new_status == 'submitted'
 
     self.sub_service_requests.map do |ssr|
-      ssr.update_status_and_notify(new_status)
+      ssr.update_status_and_notify(new_status, current_user)
     end.compact
   end
 

app/models/sub_service_request.rb

@@ -71,6 +71,8 @@ class SubServiceRequest < ApplicationRecord
   scope :in_work_fulfillment, -> { where(in_work_fulfillment: true) }
   scope :imported_to_fulfillment, -> { where(imported_to_fulfillment: true) }
 
+  attribute :current_user_id, :big_integer
+
   def consult_arranged_date=(date)
     write_attribute(:consult_arranged_date, date.present? ? Time.strptime(date, "%m/%d/%Y") : nil)
   end
@@ -275,7 +277,8 @@ def ready_for_fulfillment?
 
   # Returns the SSR id that need an initial submission email and updates
   # the SSR status to new status if appropriate
-  def update_status_and_notify(new_status)
+  def update_status_and_notify(new_status, current_user)
+    self.current_user_id = current_user.id
     if self.status != new_status && self.can_be_edited? && Status.updatable?(self.status)
       if new_status == 'submitted'
         ### For 'submitted' status ONLY:
@@ -352,9 +355,13 @@ def arms_editable?
 
   def update_past_status
     if saved_change_to_status? && !@prev_status.blank?
-      past_status = self.past_statuses.create(status: @prev_status, new_status: status, date: Time.now)
-      user_id = AuditRecovery.where(auditable_id: past_status.id, auditable_type: 'PastStatus').first.user_id
-      past_status.update_attribute(:changed_by_id, user_id)
+      past_status = self.past_statuses.create(status: @prev_status, new_status: status, date: Time.now, changed_by_id: self.current_user_id)
+
+      # fall back to old method of assigning using audit trail, TODO: this is more of safety measure as it is hard to tell everywhere a SSR is saved/updated
+      if past_status.changed_by_id.blank?
+        user_id = AuditRecovery.where(auditable_id: past_status.id, auditable_type: 'PastStatus').first.user_id
+        past_status.update_attribute(:changed_by_id, user_id)
+      end
     end
   end
 

spec/api/v1/sub_service_requests/get_sub_service_request_spec.rb

@@ -66,7 +66,7 @@
         parsed_body         = JSON.parse(response.body)
         expected_attributes = @sub_service_request.attributes.
                                 keys.
-                                reject! { |key| ['id', 'created_at', 'updated_at', 'deleted_at', 'submitted_at', 'protocol_id'].include?(key) }.
+                                reject! { |key| ['id', 'created_at', 'updated_at', 'deleted_at', 'submitted_at', 'protocol_id', 'current_user_id'].include?(key) }.
                                 push('callback_url', 'sparc_id', 'grand_total').
                                 sort
 
@@ -82,7 +82,7 @@
         parsed_body         = JSON.parse(response.body)
         expected_attributes = @sub_service_request.attributes.
                                 keys.
-                                reject! { |key| ['id', 'created_at', 'updated_at', 'deleted_at', 'submitted_at', 'protocol_id'].include?(key) }.
+                                reject! { |key| ['id', 'created_at', 'updated_at', 'deleted_at', 'submitted_at', 'protocol_id', 'current_user_id'].include?(key) }.
                                 push('callback_url', 'sparc_id', 'line_items', 'service_request', 'grand_total').
                                 sort
 

spec/api/v1/sub_service_requests/get_sub_service_requests_spec.rb

@@ -77,7 +77,7 @@
         parsed_body         = JSON.parse(response.body)
         expected_attributes = build(:sub_service_request).attributes.
                                 keys.
-                                reject { |key| ['id', 'created_at', 'updated_at', 'deleted_at', 'submitted_at', 'protocol_id'].include?(key) }.
+                                reject { |key| ['id', 'created_at', 'updated_at', 'deleted_at', 'submitted_at', 'protocol_id', 'current_user_id'].include?(key) }.
                                 push('callback_url', 'sparc_id', 'grand_total').
                                 sort
 
@@ -93,7 +93,7 @@
         parsed_body         = JSON.parse(response.body)
         expected_attributes = build(:sub_service_request).attributes.
                                 keys.
-                                reject { |key| ['id', 'created_at', 'updated_at', 'deleted_at', 'submitted_at', 'protocol_id'].include?(key) }.
+                                reject { |key| ['id', 'created_at', 'updated_at', 'deleted_at', 'submitted_at', 'protocol_id', 'current_user_id'].include?(key) }.
                                 push('callback_url', 'sparc_id', 'line_items', 'service_request', 'grand_total').
                                 sort
 

spec/api/v1/sub_service_requests/get_sub_service_requests_with_ids_spec.rb

@@ -78,7 +78,7 @@
         parsed_body         = JSON.parse(response.body)
         expected_attributes = build(:sub_service_request).attributes.
                                 keys.
-                                reject { |key| ['id', 'created_at', 'updated_at', 'deleted_at', 'submitted_at', 'protocol_id'].include?(key) }.
+                                reject { |key| ['id', 'created_at', 'updated_at', 'deleted_at', 'submitted_at', 'protocol_id', 'current_user_id'].include?(key) }.
                                 push('callback_url', 'sparc_id', 'grand_total').
                                 sort
 
@@ -94,7 +94,7 @@
         parsed_body         = JSON.parse(response.body)
         expected_attributes = build(:sub_service_request).attributes.
                                 keys.
-                                reject { |key| ['id', 'created_at', 'updated_at', 'deleted_at', 'submitted_at', 'protocol_id'].include?(key) }.
+                                reject { |key| ['id', 'created_at', 'updated_at', 'deleted_at', 'submitted_at', 'protocol_id', 'current_user_id'].include?(key) }.
                                 push('callback_url', 'sparc_id', 'line_items', 'service_request', 'grand_total').
                                 sort
 

spec/controllers/service_requests/get_save_and_exit_spec.rb

@@ -25,6 +25,10 @@
   let!(:before_filters) { find_before_filters }
   let!(:logged_in_user) { create(:identity) }
 
+  before(:each) do
+    allow(controller).to receive(:current_user).and_return(logged_in_user)
+  end
+
   describe '#obtain_research_pricing' do
     it 'should call before_filter #initialize_service_request' do
       expect(before_filters.include?(:initialize_service_request)).to eq(true)

spec/models/service_request/update_status_spec.rb

@@ -22,21 +22,22 @@
 RSpec.describe ServiceRequest, type: :model do
   let!(:org)      { create(:organization_with_process_ssrs) }
   let!(:protocol) { create(:study_federally_funded, primary_pi: build_stubbed(:identity)) }
+  let!(:identity) { build_stubbed(:identity) }
 
   describe "#update_status" do
     it 'should try to update SubServiceRequests and notify' do
       sr  = create(:service_request_without_validations, protocol: protocol)
       ssr = create(:sub_service_request_without_validations, service_request: sr, organization: org)
       sr.reload
-      expect_any_instance_of(SubServiceRequest).to receive(:update_status_and_notify).with('get_a_cost_estimate').and_return(ssr.id)
-      expect(sr.update_status('get_a_cost_estimate')).to eq([ssr.id])
+      expect_any_instance_of(SubServiceRequest).to receive(:update_status_and_notify).with('get_a_cost_estimate', identity).and_return(ssr.id)
+      expect(sr.update_status('get_a_cost_estimate', identity)).to eq([ssr.id])
     end
 
     context 'ServiceRequest has been submitted prior' do
       let!(:sr) { create(:service_request_without_validations, :submitted, protocol: protocol) }
 
       it 'should not update the ServiceRequest' do
-        sr.update_status('draft')
+        sr.update_status('draft', identity)
         sr.reload
         expect(sr.status).to eq('submitted')
       end
@@ -47,7 +48,7 @@
 
       context 'and new_status == submitted' do
         before :each do
-          sr.update_status('submitted')
+          sr.update_status('submitted', identity)
           sr.reload
         end
 
@@ -58,7 +59,7 @@
       end
 
       it 'should update the status but not submitted_at' do
-        sr.update_status('get_a_cost_estimate')
+        sr.update_status('get_a_cost_estimate', identity)
         sr.reload
         expect(sr.status).to eq('get_a_cost_estimate')
         expect(sr.submitted_at).to be_nil

spec/models/sub_service_request/update_status_and_notify_spec.rb

@@ -23,6 +23,7 @@
   let!(:org)      { create(:organization_with_process_ssrs) }
   let!(:protocol) { create(:study_federally_funded, primary_pi: build_stubbed(:identity)) }
   let!(:sr)       { create(:service_request_without_validations, protocol: protocol) }
+  let!(:identity) { build_stubbed(:identity) }
 
   describe "#update_status_and_notify" do
     context 'new status is different than current status' do
@@ -31,7 +32,7 @@
           context 'and new_status == submitted' do
             it 'should update status and nursing_nutrition, lab, imaging, and committee approvals' do
               ssr = create(:sub_service_request_without_validations, service_request: sr, organization: org)
-              ssr.update_status_and_notify('submitted')
+              ssr.update_status_and_notify('submitted', identity)
               ssr.reload
 
               expect(ssr.status).to eq('submitted')
@@ -47,7 +48,7 @@
 
                 context 'and past_status is nil indicating a newly created SubServiceRequest' do
                   it 'should update the SubServiceRequest and notify' do
-                    expect(ssr.update_status_and_notify('submitted')).to eq(ssr.id)
+                    expect(ssr.update_status_and_notify('submitted', identity)).to eq(ssr.id)
                     expect(ssr.reload.status).to eq('submitted')
                   end
                 end
@@ -56,7 +57,7 @@
                   let!(:past_status) { create(:past_status, sub_service_request: ssr, status: 'draft') }
 
                   it 'should update the SubServiceRequest and notify' do
-                    expect(ssr.update_status_and_notify('submitted')).to eq(ssr.id)
+                    expect(ssr.update_status_and_notify('submitted', identity)).to eq(ssr.id)
                     expect(ssr.reload.status).to eq('submitted')
                   end
                 end
@@ -65,7 +66,7 @@
                   let!(:past_status) { create(:past_status, sub_service_request: ssr, status: 'complete') }
 
                   it 'should update the SubServiceRequest but not notify' do
-                    expect(ssr.update_status_and_notify('submitted')).to eq(nil)
+                    expect(ssr.update_status_and_notify('submitted', identity)).to eq(nil)
                     expect(ssr.reload.status).to eq('submitted')
                   end
                 end
@@ -75,7 +76,7 @@
                 let!(:ssr) { create(:sub_service_request_without_validations, service_request: sr, organization: org, status: 'awaiting_pi_approval') }
 
                 it 'should update the SubServiceRequest and notify' do
-                  expect(ssr.update_status_and_notify('submitted')).to eq(ssr.id)
+                  expect(ssr.update_status_and_notify('submitted', identity)).to eq(ssr.id)
                   expect(ssr.reload.status).to eq('submitted')
                 end
               end
@@ -85,7 +86,7 @@
               let!(:ssr) { create(:sub_service_request_without_validations, service_request: sr, organization: org, status: 'draft', submitted_at: DateTime.now) }
 
               it 'should not notify' do
-                expect(ssr.update_status_and_notify('submitted')).to eq(nil)
+                expect(ssr.update_status_and_notify('submitted', identity)).to eq(nil)
               end
             end
           end
@@ -94,7 +95,7 @@
             let!(:ssr) { create(:sub_service_request_without_validations, service_request: sr, organization: org, nursing_nutrition_approved: nil, lab_approved: nil, imaging_approved: nil, committee_approved: nil) }
 
             it 'should only update status and notify' do
-              expect(ssr.update_status_and_notify('get_a_cost_estimate')).to eq(ssr.id)
+              expect(ssr.update_status_and_notify('get_a_cost_estimate', identity)).to eq(ssr.id)
               ssr.reload
               expect(ssr.status).to eq('get_a_cost_estimate')
               expect(ssr.nursing_nutrition_approved).to eq(nil)
@@ -109,7 +110,7 @@
           let!(:ssr) { create(:sub_service_request_without_validations, service_request: sr, organization: org, status: 'pending') }
 
           it 'should not update the SubServiceRequest nor notify' do
-            expect(ssr.update_status_and_notify('draft')).to eq(nil)
+            expect(ssr.update_status_and_notify('draft', identity)).to eq(nil)
             expect(ssr.reload.status).to eq('pending')
           end
         end
@@ -123,7 +124,7 @@
         }
 
         it 'should not update the SubServiceRequest nor notify' do
-          expect(ssr.update_status_and_notify('submitted')).to eq(nil)
+          expect(ssr.update_status_and_notify('submitted', identity)).to eq(nil)
           expect(ssr.reload.status).to eq('draft')
         end
       end
@@ -133,7 +134,7 @@
       let!(:ssr) { create(:sub_service_request_without_validations, service_request: sr, organization: org, status: 'draft') }
 
       it 'should not update the SSR nor notify' do
-        expect(ssr.update_status_and_notify('draft')).to eq(nil)
+        expect(ssr.update_status_and_notify('draft', identity)).to eq(nil)
         expect(ssr.reload.status).to eq('draft')
       end
     end