From 6fed108703c88d5c8e44658cc7fec82dde53454c Mon Sep 17 00:00:00 2001
From: Matt Jankowski <matt@jankowski.online>
Date: Wed, 17 Apr 2024 04:16:51 -0400
Subject: [PATCH] Use Rails `upsert` to generate update_count! query in
 Counters concern (#28738)

Co-authored-by: Claire <claire.github-309c@sitedethib.com>
---
 app/models/concerns/account/counters.rb       | 55 ++++++++-----------
 spec/models/concerns/account/counters_spec.rb | 13 +++++
 2 files changed, 37 insertions(+), 31 deletions(-)

diff --git a/app/models/concerns/account/counters.rb b/app/models/concerns/account/counters.rb
index 448839812..536d5ca7b 100644
--- a/app/models/concerns/account/counters.rb
+++ b/app/models/concerns/account/counters.rb
@@ -35,40 +35,11 @@ module Account::Counters
     raise ArgumentError, "Invalid key #{key}" unless ALLOWED_COUNTER_KEYS.include?(key)
     raise ArgumentError, 'Do not call update_count! on dirty objects' if association(:account_stat).loaded? && account_stat&.changed? && account_stat.changed_attribute_names_to_save == %w(id)
 
-    value = value.to_i
-    default_value = value.positive? ? value : 0
-
-    # We do an upsert using manually written SQL, as Rails' upsert method does
-    # not seem to support writing expressions in the UPDATE clause, but only
-    # re-insert the provided values instead.
-    # Even ARel seem to be missing proper handling of upserts.
-    sql = if value.positive? && key == :statuses_count
-            <<-SQL.squish
-              INSERT INTO account_stats(account_id, #{key}, created_at, updated_at, last_status_at)
-                VALUES (:account_id, :default_value, now(), now(), now())
-              ON CONFLICT (account_id) DO UPDATE
-              SET #{key} = account_stats.#{key} + :value,
-                  last_status_at = now(),
-                  updated_at = now()
-              RETURNING id;
-            SQL
-          else
-            <<-SQL.squish
-              INSERT INTO account_stats(account_id, #{key}, created_at, updated_at)
-                VALUES (:account_id, :default_value, now(), now())
-              ON CONFLICT (account_id) DO UPDATE
-              SET #{key} = account_stats.#{key} + :value,
-                  updated_at = now()
-              RETURNING id;
-            SQL
-          end
-
-    sql = AccountStat.sanitize_sql([sql, account_id: id, default_value: default_value, value: value])
-    account_stat_id = AccountStat.connection.exec_query(sql)[0]['id']
+    result = updated_account_stat(key, value.to_i)
 
     # Reload account_stat if it was loaded, taking into account newly-created unsaved records
     if association(:account_stat).loaded?
-      account_stat.id = account_stat_id if account_stat.new_record?
+      account_stat.id = result.first['id'] if account_stat.new_record?
       account_stat.reload
     end
   end
@@ -79,6 +50,28 @@ module Account::Counters
 
   private
 
+  def updated_account_stat(key, value)
+    AccountStat.upsert(
+      initial_values(key, value),
+      on_duplicate: Arel.sql(
+        duplicate_values(key, value).join(', ')
+      ),
+      unique_by: :account_id
+    )
+  end
+
+  def initial_values(key, value)
+    { :account_id => id, key => [value, 0].max }.tap do |values|
+      values.merge!(last_status_at: Time.current) if key == :statuses_count
+    end
+  end
+
+  def duplicate_values(key, value)
+    ["#{key} = (account_stats.#{key} + #{value})", 'updated_at = CURRENT_TIMESTAMP'].tap do |values|
+      values << 'last_status_at = CURRENT_TIMESTAMP' if key == :statuses_count && value.positive?
+    end
+  end
+
   def save_account_stat
     return unless association(:account_stat).loaded? && account_stat&.changed?
 
diff --git a/spec/models/concerns/account/counters_spec.rb b/spec/models/concerns/account/counters_spec.rb
index 3c063a2fa..ccac9e95d 100644
--- a/spec/models/concerns/account/counters_spec.rb
+++ b/spec/models/concerns/account/counters_spec.rb
@@ -44,5 +44,18 @@ describe Account::Counters do
 
       expect(account.statuses_count).to eq 5
     end
+
+    it 'preserves last_status_at when decrementing statuses_count' do
+      account_stat = Fabricate(
+        :account_stat,
+        account: account,
+        last_status_at: 3.days.ago,
+        statuses_count: 10
+      )
+
+      expect { account.decrement_count!(:statuses_count) }
+        .to change(account_stat.reload, :statuses_count).by(-1)
+        .and not_change(account_stat.reload, :last_status_at)
+    end
   end
 end