[SCI-5635] add a migration to fix broken steps, fix the api so it sets last_modified_by_id, minor step model refactor

migration fix

remove the conditoin for set_last_modified_by callback

set the last_modified_by_id in before_save callback (in case it is not set yet)

remove trailing spaces

Set last modified in protocl import service

app/controllers/api/v1/steps_controller.rb

@@ -32,13 +32,16 @@ module Api
 
         step = @protocol.steps.create!(step_params.merge!(completed: false,
                                                           user: current_user,
-                                                          position: @protocol.number_of_steps))
+                                                          position: @protocol.number_of_steps,
+                                                          last_modified_by_id: current_user.id))
 
         render jsonapi: step, serializer: StepSerializer, status: :created
       end
 
       def update
-        @step.assign_attributes(step_params)
+        @step.assign_attributes(
+          step_params.merge!(last_modified_by_id: current_user.id)
+        )
 
         if @step.changed? && @step.save!
           if @step.saved_change_to_attribute?(:completed)

app/models/step.rb

@@ -107,11 +107,6 @@ class Step < ApplicationRecord
     StepComment.from(comments, :comments).order(created_at: :asc)
   end
 
-  def save(current_user=nil)
-    @current_user = current_user
-    super()
-  end
-
   def space_taken
     st = 0
     assets.each do |asset|
@@ -202,21 +197,36 @@ class Step < ApplicationRecord
   end
 
   def set_last_modified_by
-    if @current_user&.is_a?(User)
-      self.tables.each do |t|
-        t.created_by ||= @current_user
-        t.last_modified_by = @current_user if t.changed?
+    self.last_modified_by_id ||= user_id
+
+    tables.each do |t|
+      t.created_by ||= last_modified_by_id
+      if t.changed?
+        t.last_modified_by = last_modified_by_id
+        t.save!
       end
-      self.assets.each do |a|
-        a.created_by ||= @current_user
-        a.last_modified_by = @current_user if a.changed?
     end
-      self.checklists.each do |checklist|
-        checklist.created_by ||= @current_user
-        checklist.last_modified_by = @current_user if checklist.changed?
+
+    assets.each do |a|
+      a.created_by ||= last_modified_by_id
+      if a.changed?
+        a.last_modified_by = last_modified_by_id
+        a.save!
+      end
+    end
+
+    checklists.each do |checklist|
+      checklist.created_by ||= last_modified_by_id
+      if checklist.changed?
+        checklist.last_modified_by = last_modified_by_id
+        checklist.save!
+      end
+
       checklist.checklist_items.each do |checklist_item|
-          checklist_item.created_by ||= @current_user
-          checklist_item.last_modified_by = @current_user if checklist_item.changed?
+        checklist_item.created_by ||= last_modified_by_id
+        if checklist_item.changed?
+          checklist_item.last_modified_by = last_modified_by_id
+          checklist_item.save!
         end
       end
     end

app/services/protocol_importers/import_protocol_service.rb

@@ -24,7 +24,8 @@ module ProtocolImporters
         s = Step.new(step_params
                        .symbolize_keys
                        .slice(:name, :position, :description, :tables_attributes)
-                       .merge(user: @user, completed: false))
+                       .merge(user: @user, completed: false)
+                       .merge(last_modified_by_id: @user.id))
 
         # 'Manually' create assets here. "Accept nasted attributes" won't work for assets
         s.assets << AttachmentsBuilder.generate(step_params.deep_symbolize_keys, user: @user, team: @team)

db/migrate/20210410100006_fix_last_changed_by_on_steps.rb

@@ -0,0 +1,33 @@
+# frozen_string_literal: true
+
+class Step < ApplicationRecord
+  # Making sure we don't get undesired callbacks in the future
+  # But still enjoy the handyness of using the model
+end
+
+class FixLastChangedByOnSteps < ActiveRecord::Migration[6.1]
+  # There was an issue with API steps endpoint not settings last_modified_by_id
+  # this migration fix the affected records
+  # At the moment it seems only the complete and uncomplete actions were problematic
+  #
+  # The enterprise build of SciNote also uses the Audit Trails
+  # In this instance we don't neet to worry about these changes
+  # because we're not recording the changes to last_modified_by_id
+
+  def up
+    Step.where(last_modified_by_id: nil).each do |step|
+      # Try to find corresponding activity, if step was changed activity should exist
+      # we cannot completely rely on that, so for the fallback we use the step user
+      activity = Activity
+                 .where(type_of: %i(complete_step uncomplete_step))
+                 .where("values -> 'message_items' -> 'step' ->> 'id' = ?", step.id.to_s)
+                 .order(created_at: :desc)
+                 .first
+      if activity && user_id = activity.values.dig('message_items', 'user', 'id')
+        step.update!(last_modified_by_id: user_id)
+      else
+        step.update!(last_modified_by_id: step.user_id)
+      end
+    end
+  end
+end

spec/requests/api/v1/steps_controller_spec.rb

@@ -133,6 +133,12 @@ RSpec.describe 'Api::V1::StepsController', type: :request do
           )
         )
       end
+
+      it 'sets the last_changed_by' do
+        action
+
+        expect(Step.find(json['data']['id']).last_modified_by_id).to be @user.id
+      end
     end
 
     context 'when has missing param' do
@@ -205,6 +211,11 @@ RSpec.describe 'Api::V1::StepsController', type: :request do
           )
         )
       end
+
+      it 'sets the last_changed_by' do
+        action
+        expect(Step.find(json['data']['id']).last_modified_by_id).to be @user.id
+      end
     end
 
     context 'when has missing param' do