Refactor RichTextLink and related migrations to improve link handling and ensure default values for link_type, position, and locale

Author: rsmithlal

app/jobs/better_together/metrics/rich_text_link_checker_queue_job.rb

@@ -40,7 +40,7 @@ def model_collection
         # metrics rich_text_links table if records exist, otherwise operate on
         # the full Link set.
         if BetterTogether::Metrics::RichTextLink.table_exists?
-          model_class.joins("INNER JOIN better_together_metrics_rich_text_links rt ON rt.link_id = #{model_class.table_name}.id")
+          model_class.joins(:rich_text_links)
                      .where('last_checked_at IS NULL OR last_checked_at < ?', last_checked_lt)
         else
           model_class.where('last_checked_at IS NULL OR last_checked_at < ?', last_checked_lt)

app/models/better_together/metrics/rich_text_link.rb

@@ -5,6 +5,8 @@ module Metrics
     # Tracks occurrences of links found inside ActionText rich content and
     # associates them with the original Link record and owning rich text.
     class RichTextLink < ApplicationRecord
+      include Positioned
+
       belongs_to :link, class_name: 'BetterTogether::Content::Link'
       belongs_to :rich_text, class_name: 'ActionText::RichText'
       belongs_to :rich_text_record, polymorphic: true

app/services/better_together/metrics/rich_text_link_identifier.rb

@@ -95,10 +95,15 @@ def persist_link_and_rich_text_link(rich_text, link, index, canonical_host, uri_
         bt_link.host ||= canonical_host
         bt_link.scheme ||= uri_obj.scheme
         bt_link.external = (canonical_host.present? && (rt_platform_host != canonical_host))
+        # Ensure link_type is set to a sensible default before persisting to avoid
+        # NOT NULL constraint violations (some callers create links directly).
+        bt_link.link_type ||= 'website'
         bt_link.save! if bt_link.changed?
 
         # Persist the RichTextLink depending on the schema available.
-        if BetterTogether::Metrics::RichTextLink.column_names.include?('link_id')
+        model = BetterTogether::Metrics::RichTextLink
+
+        if model.column_names.include?('link_id')
           attrs = {
             link_id: bt_link.id,
             rich_text_id: rich_text.id,
@@ -108,21 +113,42 @@ def persist_link_and_rich_text_link(rich_text, link, index, canonical_host, uri_
             locale: rich_text.locale
           }
 
-          BetterTogether::Metrics::RichTextLink.find_or_create_by!(attrs)
+          # Build optional metadata hash only for columns that exist on the table
+          optional_cols = %w[url link_type external valid_link host error_message]
+          optional_cols.each do |c|
+            next unless model.column_names.include?(c)
+
+            attrs[c.to_sym] = case c
+                              when 'url' then bt_link.url
+                              when 'link_type' then bt_link.link_type || 'website'
+                              when 'external' then bt_link.external || false
+                              when 'valid_link' then bt_link.valid_link || false
+                              when 'host' then bt_link.host
+                              when 'error_message' then bt_link.error_message
+                              end
+          end
+
+          begin
+            model.create!(attrs)
+          rescue ActiveRecord::RecordNotUnique
+            # another process inserted concurrently; ignore
+          end
         else
           # Fallback schema: metrics rich_text_links store URL and metadata inline.
-          model = BetterTogether::Metrics::RichTextLink
-          unless model.where(rich_text_id: rich_text.id, url: link).exists?
-            conn = model.connection
-            table = model.table_name
-            now = Time.current
-            cols = %w[rich_text_id url link_type external valid host error_message created_at updated_at]
-            vals = [rich_text.id, link, bt_link.link_type || 'website', bt_link.external || false,
-                    bt_link.valid_link || false, bt_link.host, bt_link.error_message, now, now]
-            sql = "INSERT INTO #{table} (#{cols.join(',')}) VALUES (#{vals.map do |v|
-              conn.quote(v)
-            end.join(',')})"
-            conn.execute(sql)
+          attrs = {
+            rich_text_id: rich_text.id,
+            url: link,
+            link_type: bt_link.link_type || 'website',
+            external: bt_link.external || false,
+            valid_link: bt_link.valid_link || false,
+            host: bt_link.host,
+            error_message: bt_link.error_message
+          }
+
+          begin
+            model.create!(attrs.merge(position: index, locale: rich_text.locale))
+          rescue ActiveRecord::RecordNotUnique
+            # ignore duplicate insertion races
           end
         end
       end
@@ -133,13 +159,17 @@ def parse_uri(link)
       end
 
       def create_invalid(rich_text, index, link, invalid_type)
+        # Create the content link with a default link_type to satisfy DB constraints
+        bt_link = BetterTogether::Content::Link.create!(url: link, valid_link: false, error_message: invalid_type,
+                                                        link_type: 'website')
+
         BetterTogether::Metrics::RichTextLink.create!(
           rich_text_id: rich_text.id,
           rich_text_record_id: rich_text.record_id,
           rich_text_record_type: rich_text.record_type,
           position: index,
           locale: rich_text.locale,
-          link: BetterTogether::Content::Link.create!(url: link, valid_link: false, error_message: invalid_type)
+          link: bt_link
         )
       end
 

db/migrate/20250902203000_add_rich_text_link_associations.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 # Migration to add association columns to better_together_metrics_rich_text_links
 class AddRichTextLinkAssociations < ActiveRecord::Migration[7.1]
   def change

db/migrate/20250902203002_add_position_and_locale_to_rich_text_links.rb

@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+# Migration to add position and locale columns (and a unique index) to the
+# better_together_metrics_rich_text_links table for ordering and locale-aware
+# rich-text link tracking.
+class AddPositionAndLocaleToRichTextLinks < ActiveRecord::Migration[7.1]
+  def change
+    add_column :better_together_metrics_rich_text_links, :position, :integer, null: false, default: 0
+    add_column :better_together_metrics_rich_text_links, :locale, :string, limit: 5, null: false,
+                                                                           default: I18n.default_locale
+
+    add_index :better_together_metrics_rich_text_links, %i[rich_text_id position locale],
+              name: 'idx_bt_rtl_on_rich_text_pos_loc', unique: true
+  end
+end

db/migrate/20250902203003_rename_valid_to_valid_link_on_metrics_rich_text_links.rb

@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+# Migration to rename a legacy `valid` boolean column to `valid_link` on the
+# better_together_metrics_rich_text_links table to avoid method name collisions
+# with ActiveRecord predicate methods.
+class RenameValidToValidLinkOnMetricsRichTextLinks < ActiveRecord::Migration[7.1]
+  def change
+    table = :better_together_metrics_rich_text_links
+    return unless column_exists?(table, :valid) && !column_exists?(table, :valid_link)
+
+    rename_column table, :valid, :valid_link
+  end
+end

db/migrate/20250902203004_ensure_link_type_default_on_content_links.rb

@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+# Migration to ensure `link_type` on better_together_content_links has a sensible
+# default (`'website'`) and is non-nullable to satisfy callers that expect a
+# present link_type value.
+class EnsureLinkTypeDefaultOnContentLinks < ActiveRecord::Migration[7.1]
+  def up
+    table = :better_together_content_links
+    if column_exists?(table, :link_type)
+      change_column_default table, :link_type, 'website'
+      # Set existing nulls to default before enforcing NOT NULL
+      execute <<-SQL.squish
+        UPDATE #{table} SET link_type = 'website' WHERE link_type IS NULL
+      SQL
+      change_column_null table, :link_type, false
+    else
+      add_column table, :link_type, :string, null: false, default: 'website'
+    end
+  end
+
+  def down
+    table = :better_together_content_links
+    return unless column_exists?(table, :link_type)
+
+    change_column_null table, :link_type, true
+    change_column_default table, :link_type, nil
+  end
+end

spec/dummy/db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.2].define(version: 2025_09_02_203001) do
+ActiveRecord::Schema[7.2].define(version: 2025_09_02_203004) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "pgcrypto"
   enable_extension "plpgsql"
@@ -358,7 +358,7 @@
     t.integer "lock_version", default: 0, null: false
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
-    t.string "link_type", null: false
+    t.string "link_type", default: "website", null: false
     t.string "url", null: false
     t.string "scheme"
     t.string "host"
@@ -914,13 +914,16 @@
     t.string "url", null: false
     t.string "link_type", null: false
     t.boolean "external", null: false
-    t.boolean "valid", default: false
+    t.boolean "valid_link", default: false
     t.string "host"
     t.text "error_message"
     t.uuid "link_id"
     t.string "rich_text_record_type"
     t.uuid "rich_text_record_id"
+    t.integer "position", default: 0, null: false
+    t.string "locale", limit: 5, default: "en", null: false
     t.index ["link_id"], name: "bt_metrics_rich_text_links_on_link_id"
+    t.index ["rich_text_id", "position", "locale"], name: "idx_bt_rtl_on_rich_text_pos_loc", unique: true
     t.index ["rich_text_id"], name: "index_better_together_metrics_rich_text_links_on_rich_text_id"
     t.index ["rich_text_record_type", "rich_text_record_id"], name: "bt_metrics_rich_text_links_on_rich_text_record"
   end