From 56ebc3dc076336dc113b2947f4446a96776c33f7 Mon Sep 17 00:00:00 2001 From: miha Date: Sat, 10 Apr 2021 12:33:32 +0200 Subject: [PATCH] [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 | 7 ++- app/models/step.rb | 46 +++++++++++-------- .../import_protocol_service.rb | 3 +- ...0410100006_fix_last_changed_by_on_steps.rb | 33 +++++++++++++ spec/requests/api/v1/steps_controller_spec.rb | 11 +++++ 5 files changed, 79 insertions(+), 21 deletions(-) create mode 100644 db/migrate/20210410100006_fix_last_changed_by_on_steps.rb diff --git a/app/controllers/api/v1/steps_controller.rb b/app/controllers/api/v1/steps_controller.rb index 10f3c5897..1bbebd5b2 100644 --- a/app/controllers/api/v1/steps_controller.rb +++ b/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) diff --git a/app/models/step.rb b/app/models/step.rb index ed9f6f2e2..ee2589583 100644 --- a/app/models/step.rb +++ b/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 + + 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 - self.checklists.each do |checklist| - checklist.created_by ||= @current_user - checklist.last_modified_by = @current_user if checklist.changed? - checklist.checklist_items.each do |checklist_item| - checklist_item.created_by ||= @current_user - checklist_item.last_modified_by = @current_user if checklist_item.changed? + 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 ||= last_modified_by_id + if checklist_item.changed? + checklist_item.last_modified_by = last_modified_by_id + checklist_item.save! end end end diff --git a/app/services/protocol_importers/import_protocol_service.rb b/app/services/protocol_importers/import_protocol_service.rb index 2074b57a1..ead924f01 100644 --- a/app/services/protocol_importers/import_protocol_service.rb +++ b/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) diff --git a/db/migrate/20210410100006_fix_last_changed_by_on_steps.rb b/db/migrate/20210410100006_fix_last_changed_by_on_steps.rb new file mode 100644 index 000000000..1e5e2302d --- /dev/null +++ b/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 diff --git a/spec/requests/api/v1/steps_controller_spec.rb b/spec/requests/api/v1/steps_controller_spec.rb index 069da1851..71b85b29a 100644 --- a/spec/requests/api/v1/steps_controller_spec.rb +++ b/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