Merge remote-tracking branch 'origin/main' into sethherr/hartford

Author: sethherr

app/jobs/backfills/user_alert_alertable_job.rb

@@ -0,0 +1,48 @@
+# frozen_string_literal: true
+
+module Backfills
+  # theft_alert_id and user_phone_id were replaced by the alertable polymorphic pair.
+  # UserAlert still reads the old columns as a fallback, so this can run after the deploy
+  class UserAlertAlertableJob < ApplicationJob
+    include Sidekiq::IterableJob
+
+    sidekiq_options queue: "low_priority", retry: false
+
+    # batch_size has to be passed - the enumerator hands in_batches an explicit `of: nil` without it
+    def build_enumerator(cursor:)
+      active_record_relations_enumerator(user_alerts, cursor:, batch_size: 1_000)
+    end
+
+    # Backfilled rows drop out of the relation, but the cursor moves forward by id, so a resumed
+    # run doesn't skip anything
+    def each_iteration(batch)
+      batch.where.not(theft_alert_id: nil)
+        .update_all("alertable_type = 'TheftAlert', alertable_id = theft_alert_id")
+      batch.where.not(user_phone_id: nil)
+        .update_all("alertable_type = 'UserPhone', alertable_id = user_phone_id")
+    end
+
+    # The duplicates predate the uniqueness validation and are inert while alertable_id is blank.
+    # Backfilling arms the validation, which would leave the survivor unable to save
+    def on_complete
+      uniq_kind_alerts.where.not(id: lowest_of_each_group).delete_all
+    end
+
+    private
+
+    def uniq_kind_alerts
+      UserAlert.where(kind: UserAlert::UNIQ_KINDS).where.not(alertable_id: nil)
+    end
+
+    # find_or_build_by returns the lowest id, so the rest are the rows nothing reads
+    def lowest_of_each_group
+      uniq_kind_alerts.group(:user_id, :kind, :alertable_type, :alertable_id).select("MIN(id)")
+    end
+
+    def user_alerts
+      base = UserAlert.where(alertable_id: nil)
+
+      base.where.not(theft_alert_id: nil).or(base.where.not(user_phone_id: nil))
+    end
+  end
+end

app/models/user_alert.rb

@@ -4,12 +4,14 @@
 # Database name: primary
 #
 #  id              :bigint           not null, primary key
+#  alertable_type  :string
 #  dismissed_at    :datetime
 #  kind            :integer
 #  message         :text
 #  resolved_at     :datetime
 #  created_at      :datetime         not null
 #  updated_at      :datetime         not null
+#  alertable_id    :bigint
 #  bike_id         :bigint
 #  organization_id :bigint
 #  theft_alert_id  :bigint
@@ -18,6 +20,7 @@
 #
 # Indexes
 #
+#  index_user_alerts_on_alertable       (alertable_type,alertable_id)
 #  index_user_alerts_on_bike_id         (bike_id)
 #  index_user_alerts_on_theft_alert_id  (theft_alert_id)
 #  index_user_alerts_on_user_id         (user_id)
@@ -31,17 +34,24 @@ class UserAlert < ApplicationRecord
     unassigned_bike_org: 3
   }.freeze
 
+  UNIQ_KINDS = %w[phone_waiting_confirmation].freeze
+
+  # Deprecated columns, still read until Backfills::UserAlertAlertableJob has run
+  LEGACY_ALERTABLE_COLUMNS = {"TheftAlert" => :theft_alert_id, "UserPhone" => :user_phone_id}.freeze
+
   enum :kind, KIND_ENUM
 
   belongs_to :user
   belongs_to :bike
+  belongs_to :organization
+  belongs_to :alertable, polymorphic: true
   belongs_to :user_phone
   belongs_to :theft_alert
-  belongs_to :organization
 
   has_one :notification, as: :notifiable
 
-  validates :user_phone_id, uniqueness: {scope: %i[kind user_id]}, allow_blank: true
+  validates :alertable_id, uniqueness: {scope: %i[alertable_type kind user_id]},
+    allow_blank: true, if: :uniq_kind?
 
   before_validation :set_calculated_attributes
 
@@ -55,6 +65,16 @@ class UserAlert < ApplicationRecord
   scope :account, -> { where(kind: account_kinds) }
   scope :dismissable, -> { where(kind: dismissable_kinds) }
   scope :with_notification, -> { joins(:notification).where.not(notifications: {id: nil}) }
+  # Also matches rows Backfills::UserAlertAlertableJob hasn't reached yet
+  scope :for_alertable, ->(alertable) {
+    next all if alertable.blank?
+
+    type = alertable.class.polymorphic_name
+    matched = where(alertable_type: type, alertable_id: alertable.id)
+    legacy_column = LEGACY_ALERTABLE_COLUMNS[type]
+
+    legacy_column ? matched.or(where(legacy_column => alertable.id)) : matched
+  }
   scope :create_notification, -> {
     where(kind: notification_kinds, updated_at: notify_period)
       .left_joins(:notification).where(notifications: {id: nil})
@@ -103,13 +123,13 @@ def self.placement(kind)
   end
 
   def self.find_or_build_by(attrs)
-    where(attrs).first || new(attrs)
+    where(attrs.except(:alertable)).for_alertable(attrs[:alertable]).first || new(attrs)
   end
 
   def self.update_theft_alert_without_photo(user:, theft_alert:)
     # scope to just active, to alert if the theft alert once again has no image
     user_alert = UserAlert.active.find_or_build_by(kind: "theft_alert_without_photo",
-      user_id: user.id, theft_alert_id: theft_alert.id)
+      user_id: user.id, alertable: theft_alert)
     if theft_alert.missing_photo?
       user_alert.bike_id = theft_alert.bike&.id
       user_alert.save
@@ -141,7 +161,7 @@ def self.update_unassigned_bike_org(user:, organization:, bike:)
 
   def self.update_phone_waiting_confirmation(user:, user_phone:)
     user_alert = UserAlert.find_or_build_by(kind: "phone_waiting_confirmation",
-      user_id: user.id, user_phone_id: user_phone.id)
+      user_id: user.id, alertable: user_phone)
     if user_phone.confirmed?
       # Don't create if phone is already confirmed
       user_alert.id.blank? || user_alert.resolve!
@@ -150,6 +170,11 @@ def self.update_phone_waiting_confirmation(user:, user_phone:)
     end
   end
 
+  # Falls back to the legacy columns for rows Backfills::UserAlertAlertableJob hasn't reached
+  def alertable
+    super || theft_alert || user_phone
+  end
+
   def kind_humanized
     self.class.kind_humanized(kind)
   end
@@ -222,6 +247,12 @@ def email_subject
     end
   end
 
+  private
+
+  def uniq_kind?
+    UNIQ_KINDS.include?(kind)
+  end
+
   def set_calculated_attributes
     self.message = nil if message.blank?
   end

app/views/admin/user_alerts/_table.html.haml

@@ -45,13 +45,10 @@
                 %small bike:
                 = link_to user_alert.bike_id, admin_bike_path(user_alert.bike_id)
                 = link_to "alerts", url_for(sortable_search_params.merge(search_bike_id: user_alert.bike_id)), class: "small gray-link"
-            - if user_alert.user_phone.present?
+            - if (alertable = user_alert.alertable)
               %span.d-block
-                %small user_phone: #{user_alert.user_phone_id}
-            - if user_alert.theft_alert.present?
-              %span.d-block
-                %small theft_alert:
-                = link_to user_alert.theft_alert_id, admin_theft_alert_path(user_alert.theft_alert_id)
+                %small #{alertable.class.polymorphic_name.underscore}:
+                = link_to alertable.id, admin_path_for_object(alertable)
             - if user_alert.organization.present?
               %span.d-block
                 %small organization:

db/migrate/20260804100000_add_alertable_to_user_alerts.rb

@@ -0,0 +1,8 @@
+class AddAlertableToUserAlerts < ActiveRecord::Migration[8.1]
+  disable_ddl_transaction!
+
+  def change
+    add_reference :user_alerts, :alertable, polymorphic: true,
+      index: {algorithm: :concurrently}
+  end
+end

db/primary_replica_structure.sql

@@ -4198,7 +4198,9 @@ CREATE TABLE public.user_alerts (
     resolved_at timestamp without time zone,
     dismissed_at timestamp without time zone,
     created_at timestamp without time zone NOT NULL,
-    updated_at timestamp without time zone NOT NULL
+    updated_at timestamp without time zone NOT NULL,
+    alertable_type character varying,
+    alertable_id bigint
 );
 
 
@@ -7504,6 +7506,13 @@ CREATE INDEX index_theft_alerts_on_theft_alert_plan_id ON public.theft_alerts US
 CREATE INDEX index_theft_alerts_on_user_id ON public.theft_alerts USING btree (user_id);
 
 
+--
+-- Name: index_user_alerts_on_alertable; Type: INDEX; Schema: public; Owner: -
+--
+
+CREATE INDEX index_user_alerts_on_alertable ON public.user_alerts USING btree (alertable_type, alertable_id);
+
+
 --
 -- Name: index_user_alerts_on_bike_id; Type: INDEX; Schema: public; Owner: -
 --
@@ -7717,6 +7726,7 @@ ALTER TABLE ONLY public.bug_reports
 SET search_path TO "$user", public;
 
 INSERT INTO "schema_migrations" (version) VALUES
+('20260804100000'),
 ('20260801100000'),
 ('20260731100009'),
 ('20260731100008'),

db/structure.sql

@@ -4198,7 +4198,9 @@ CREATE TABLE public.user_alerts (
     resolved_at timestamp without time zone,
     dismissed_at timestamp without time zone,
     created_at timestamp without time zone NOT NULL,
-    updated_at timestamp without time zone NOT NULL
+    updated_at timestamp without time zone NOT NULL,
+    alertable_type character varying,
+    alertable_id bigint
 );
 
 
@@ -7504,6 +7506,13 @@ CREATE INDEX index_theft_alerts_on_theft_alert_plan_id ON public.theft_alerts US
 CREATE INDEX index_theft_alerts_on_user_id ON public.theft_alerts USING btree (user_id);
 
 
+--
+-- Name: index_user_alerts_on_alertable; Type: INDEX; Schema: public; Owner: -
+--
+
+CREATE INDEX index_user_alerts_on_alertable ON public.user_alerts USING btree (alertable_type, alertable_id);
+
+
 --
 -- Name: index_user_alerts_on_bike_id; Type: INDEX; Schema: public; Owner: -
 --
@@ -7717,6 +7726,7 @@ ALTER TABLE ONLY public.bug_reports
 SET search_path TO "$user", public;
 
 INSERT INTO "schema_migrations" (version) VALUES
+('20260804100000'),
 ('20260801100000'),
 ('20260731100009'),
 ('20260731100008'),

spec/jobs/backfills/user_alert_alertable_job_spec.rb

@@ -0,0 +1,58 @@
+require "rails_helper"
+
+RSpec.describe Backfills::UserAlertAlertableJob, type: :job do
+  describe "perform" do
+    let(:user) { FactoryBot.create(:user_confirmed) }
+    let(:user_phone) { FactoryBot.create(:user_phone, user:) }
+    let(:theft_alert) { FactoryBot.create(:theft_alert) }
+    let!(:legacy_phone) do
+      FactoryBot.create(:user_alert, user:, kind: "phone_waiting_confirmation", user_phone:)
+    end
+    let!(:legacy_theft_alert) do
+      FactoryBot.create(:user_alert, user:, kind: "theft_alert_without_photo", theft_alert:)
+    end
+    let!(:bike_alert) { FactoryBot.create(:user_alert_stolen_bike_without_location) }
+
+    it "copies the legacy columns into alertable" do
+      expect(legacy_phone.alertable_id).to be_blank
+      expect(legacy_theft_alert.alertable_id).to be_blank
+      updated_at = legacy_phone.updated_at
+
+      Sidekiq::Testing.inline! { described_class.perform_async }
+
+      expect(legacy_phone.reload.alertable).to eq user_phone
+      expect(legacy_phone.alertable_type).to eq "UserPhone"
+      # A bumped updated_at would put every backfilled row back in create_notification
+      expect(legacy_phone.updated_at).to be_within(0.001).of(updated_at)
+      expect(legacy_theft_alert.reload.alertable).to eq theft_alert
+      expect(legacy_theft_alert.alertable_type).to eq "TheftAlert"
+      # bike_id isn't part of alertable
+      expect(bike_alert.reload.alertable_id).to be_blank
+    end
+
+    # Duplicates predate the uniqueness validation, which the backfill arms
+    context "duplicates" do
+      let!(:duplicate_phone) do
+        FactoryBot.create(:user_alert, user:, kind: "phone_waiting_confirmation", user_phone:)
+      end
+      let!(:duplicate_theft_alert) do
+        FactoryBot.create(:user_alert, user:, kind: "theft_alert_without_photo", theft_alert:)
+      end
+      let!(:other_user_phone_alert) do
+        FactoryBot.create(:user_alert, kind: "phone_waiting_confirmation", user_phone:)
+      end
+
+      it "deletes the duplicate uniq_kind alerts" do
+        Sidekiq::Testing.inline! { described_class.perform_async }
+
+        expect(UserAlert.where(id: duplicate_phone.id).count).to eq 0
+        expect(legacy_phone.reload.alertable).to eq user_phone
+        expect(legacy_phone).to be_valid
+        # Only the lowest id of a group is kept, and only for uniq_kinds
+        expect(other_user_phone_alert.reload.user_id).to_not eq user.id
+        expect(other_user_phone_alert.alertable).to eq user_phone
+        expect(duplicate_theft_alert.reload.alertable).to eq theft_alert
+      end
+    end
+  end
+end