From 9300bbea53096146c80f9ae35d235ec3beebf89b Mon Sep 17 00:00:00 2001
From: Eugene Kenny <elkenny@gmail.com>
Date: Thu, 30 Jul 2020 01:30:34 +0100
Subject: [PATCH] Preserve original callback execution order

This cop registers an offense when a callback appears after another
callback that will be executed later in the record's lifecycle.

The auto-correct logic currently always moves the offending callback to
before the first callback in the class. This does eventually produce the
correct result, but it's not very efficient, and can reorder callbacks
of the same type, which changes the order in which they're executed.

By moving the offending callback above the preceding one, the sort
becomes stable and the original callback execution order is preserved.
---
 CHANGELOG.md                                  |  9 ++++++++-
 .../rails/active_record_callbacks_order.rb    |  6 ++----
 .../active_record_callbacks_order_spec.rb     | 19 +++++++++++++++++++
 3 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 269d2547b6..8d2adec450 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,6 +7,14 @@
 * [#52](https://github.com/rubocop-hq/rubocop-rails/issues/52): Add new `Rails/AfterCommitOverride` cop. ([@fatkodima][])
 * [#274](https://github.com/rubocop-hq/rubocop-rails/pull/274): Add new `Rails/WhereNot` cop. ([@fatkodima][])
 
+### Bug fixes
+
+* [#313](https://github.com/rubocop-hq/rubocop-rails/pull/313): Fix `Rails/ActiveRecordCallbacksOrder` to preserve the original callback execution order. ([@eugeneius][])
+
+### Changes
+
+* [#312](https://github.com/rubocop-hq/rubocop-rails/pull/312): Mark `Rails/MailerName` as unsafe for auto-correct. ([@eugeneius][])
+
 ## 2.7.1 (2020-07-26)
 
 ### Bug fixes
@@ -18,7 +26,6 @@
 ### Changes
 
 * [#301](https://github.com/rubocop-hq/rubocop-rails/issues/301): Set disalbed by default for `Rails/PluckId`. ([@koic][])
-* [#312](https://github.com/rubocop-hq/rubocop-rails/pull/312): Mark `Rails/MailerName` as unsafe for auto-correct. ([@eugeneius][])
 
 ## 2.7.0 (2020-07-21)
 
diff --git a/lib/rubocop/cop/rails/active_record_callbacks_order.rb b/lib/rubocop/cop/rails/active_record_callbacks_order.rb
index c105b60cf5..d04e64d050 100644
--- a/lib/rubocop/cop/rails/active_record_callbacks_order.rb
+++ b/lib/rubocop/cop/rails/active_record_callbacks_order.rb
@@ -44,9 +44,7 @@ class ActiveRecordCallbacksOrder < Cop
           after_touch
         ].freeze
 
-        CALLBACKS_ORDER_MAP = Hash[
-          CALLBACKS_IN_ORDER.map.with_index { |name, index| [name, index] }
-        ].freeze
+        CALLBACKS_ORDER_MAP = CALLBACKS_IN_ORDER.each_with_index.to_h.freeze
 
         def on_class(class_node)
           previous_index = -1
@@ -68,7 +66,7 @@ def on_class(class_node)
 
         # Autocorrect by swapping between two nodes autocorrecting them
         def autocorrect(node)
-          previous = left_siblings_of(node).find do |sibling|
+          previous = left_siblings_of(node).reverse_each.find do |sibling|
             callback?(sibling)
           end
 
diff --git a/spec/rubocop/cop/rails/active_record_callbacks_order_spec.rb b/spec/rubocop/cop/rails/active_record_callbacks_order_spec.rb
index bd7e1f2931..3821f94a5b 100644
--- a/spec/rubocop/cop/rails/active_record_callbacks_order_spec.rb
+++ b/spec/rubocop/cop/rails/active_record_callbacks_order_spec.rb
@@ -83,6 +83,25 @@ class User < ApplicationRecord
     RUBY
   end
 
+  it 'preserves the original order of callbacks of the same type' do
+    expect_offense(<<~RUBY)
+      class User < ApplicationRecord
+        after_commit :after_commit_callback
+        after_save :after_save_callback1
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `after_save` is supposed to appear before `after_commit`.
+        after_save :after_save_callback2
+      end
+    RUBY
+
+    expect_correction(<<~RUBY)
+      class User < ApplicationRecord
+        after_save :after_save_callback1
+        after_save :after_save_callback2
+        after_commit :after_commit_callback
+      end
+    RUBY
+  end
+
   it 'does not register an offense when declared callbacks are correctly ordered' do
     expect_no_offenses(<<~RUBY)
       class User < ApplicationRecord