Improve completed_on generation for steps/my_modules [SCI-4776]

app/controllers/steps_controller.rb

@@ -319,11 +319,6 @@ class StepsController < ApplicationController
       changed = @step.completed != completed
       @step.completed = completed
 
-      # Update completed_on
-      if changed
-        @step.completed_on = completed ? Time.current : nil
-      end
-
       if @step.save
         if @protocol.in_module?
           ready_to_complete = @protocol.my_module.check_completness_status

app/models/my_module.rb

@@ -7,7 +7,7 @@ class MyModule < ApplicationRecord
   enum state: Extends::TASKS_STATES
 
   before_create :create_blank_protocol
-  before_save -> { self.completed_on = completed? ? DateTime.now : nil }, if: :state_changed?
+  before_validation :set_completed_on, if: :state_changed?
 
   auto_strip_attributes :name, :description, nullify: false
   validates :name,
@@ -18,6 +18,7 @@ class MyModule < ApplicationRecord
   validates :experiment, presence: true
   validates :my_module_group, presence: true, if: proc { |mm| !mm.my_module_group_id.nil? }
   validate :coordinates_uniqueness_check, if: :active?
+  validates :completed_on, presence: true, if: proc { |mm| mm.completed? }
 
   belongs_to :created_by,
              foreign_key: 'created_by_id',
@@ -536,6 +537,12 @@ class MyModule < ApplicationRecord
 
   private
 
+  def set_completed_on
+    return if completed? && completed_on.present?
+
+    self.completed_on = completed? ? DateTime.now : nil
+  end
+
   def create_blank_protocol
     protocols << Protocol.new_blank_for_module(self)
   end

app/models/step.rb

@@ -14,9 +14,10 @@ class Step < ApplicationRecord
   validates :user, :protocol, presence: true
   validates :completed_on, presence: true, if: proc { |s| s.completed? }
 
+  before_validation :set_completed_on, if: :completed_changed?
+  before_save :set_last_modified_by
   before_destroy :cascade_before_destroy
   before_destroy :adjust_positions_on_destroy
-  before_save :set_last_modified_by
 
   belongs_to :user, inverse_of: :steps
   belongs_to :last_modified_by, foreign_key: 'last_modified_by_id', class_name: 'User', optional: true
@@ -135,6 +136,12 @@ class Step < ApplicationRecord
     tables.each(&:destroy)
   end
 
+  def set_completed_on
+    return if completed? && completed_on.present?
+
+    self.completed_on = completed? ? DateTime.now : nil
+  end
+
   def set_last_modified_by
     if @current_user&.is_a?(User)
       self.tables.each do |t|