Extract family functionality to a concern

app/controllers/family/invitations_controller.rb

@@ -3,10 +3,9 @@
 class Family::InvitationsController < ApplicationController
   before_action :authenticate_user!, except: %i[show accept]
   before_action :ensure_family_feature_enabled!, except: %i[show accept]
-  before_action :set_invitation_by_token, only: %i[show]
-  before_action :set_invitation_by_id, only: %i[accept]
   before_action :set_family, except: %i[show accept]
   before_action :set_invitation_by_id_and_family, only: %i[destroy]
+  before_action :set_invitation_by_id, only: %i[accept]
 
   def index
     authorize @family, :show?
@@ -15,7 +14,8 @@ class Family::InvitationsController < ApplicationController
   end
 
   def show
-    # Public endpoint for invitation acceptance
+    @invitation = FamilyInvitation.find_by!(token: params[:token])
+
     if @invitation.expired?
       redirect_to root_path, alert: 'This invitation has expired.' and return
     end
@@ -23,9 +23,6 @@ class Family::InvitationsController < ApplicationController
     unless @invitation.pending?
       redirect_to root_path, alert: 'This invitation is no longer valid.' and return
     end
-
-    # Show the invitation landing page regardless of authentication status
-    # The view will handle showing appropriate actions based on whether user is logged in
   end
 
   def create
@@ -45,9 +42,6 @@ class Family::InvitationsController < ApplicationController
   end
 
   def accept
-    authenticate_user!
-
-    # Additional validations before attempting to accept
     unless @invitation.pending?
       redirect_to root_path, alert: 'This invitation has already been processed' and return
     end
@@ -98,14 +92,7 @@ class Family::InvitationsController < ApplicationController
     redirect_to new_family_path, alert: 'You are not in a family' and return unless @family
   end
 
-  def set_invitation_by_token
-    # For public unauthenticated route: /invitations/:token
-    @invitation = FamilyInvitation.find_by!(token: params[:token])
-  end
-
   def set_invitation_by_id
-    # For authenticated nested routes without family validation: /families/:family_id/invitations/:id/accept
-    # The :id param contains the token value
     @invitation = FamilyInvitation.find_by!(token: params[:id])
   end
 

app/controllers/users/registrations_controller.rb

@@ -25,19 +25,15 @@ class Users::RegistrationsController < Devise::RegistrationsController
   protected
 
   def after_sign_up_path_for(resource)
-    if @invitation&.family
+    return family_path if @invitation&.family
-      family_path
+
-    else
+    super(resource)
-      super(resource)
-    end
   end
 
   def after_inactive_sign_up_path_for(resource)
-    if @invitation&.family
+    return family_path if @invitation&.family
-      family_path
+
-    else
+    super(resource)
-      super(resource)
-    end
   end
 
   private

app/models/concerns/user_family.rb

@@ -0,0 +1,135 @@
+# frozen_string_literal: true
+
+module UserFamily
+  extend ActiveSupport::Concern
+
+  included do
+    # Family associations
+    has_one :family_membership, dependent: :destroy
+    has_one :family, through: :family_membership
+    has_one :created_family, class_name: 'Family', foreign_key: 'creator_id', inverse_of: :creator, dependent: :destroy
+    has_many :sent_family_invitations, class_name: 'FamilyInvitation', foreign_key: 'invited_by_id',
+             inverse_of: :invited_by, dependent: :destroy
+
+    before_destroy :check_family_ownership
+  end
+
+  def in_family?
+    family_membership.present?
+  end
+
+  def family_owner?
+    family_membership&.owner? == true
+  end
+
+  def can_delete_account?
+    return true unless family_owner?
+    return true unless family
+
+    family.members.count <= 1
+  end
+
+  def family_sharing_enabled?
+    # User must be in a family and have explicitly enabled location sharing
+    return false unless in_family?
+
+    sharing_settings = settings.dig('family', 'location_sharing')
+    return false if sharing_settings.blank?
+
+    # If it's a boolean (legacy support), return it
+    return sharing_settings if [true, false].include?(sharing_settings)
+
+    # If it's time-limited sharing, check if it's still active
+    if sharing_settings.is_a?(Hash)
+      return false unless sharing_settings['enabled'] == true
+
+      # Check if sharing has an expiration
+      expires_at = sharing_settings['expires_at']
+      return expires_at.blank? || Time.parse(expires_at) > Time.current
+    end
+
+    false
+  end
+
+  def update_family_location_sharing!(enabled, duration: nil)
+    return false unless in_family?
+
+    current_settings = settings || {}
+    current_settings['family'] ||= {}
+
+    if enabled
+      sharing_config = { 'enabled' => true }
+
+      # Add expiration if duration is specified
+      if duration.present?
+        expiration_time = case duration
+        when '1h'
+          1.hour.from_now
+        when '6h'
+          6.hours.from_now
+        when '12h'
+          12.hours.from_now
+        when '24h'
+          24.hours.from_now
+        when 'permanent'
+          nil # No expiration
+        else
+          duration.to_i.hours.from_now if duration.to_i > 0
+        end
+
+        sharing_config['expires_at'] = expiration_time.iso8601 if expiration_time
+        sharing_config['duration'] = duration
+      end
+
+      current_settings['family']['location_sharing'] = sharing_config
+    else
+      current_settings['family']['location_sharing'] = { 'enabled' => false }
+    end
+
+    update!(settings: current_settings)
+  end
+
+  def family_sharing_expires_at
+    sharing_settings = settings.dig('family', 'location_sharing')
+    return nil unless sharing_settings.is_a?(Hash)
+
+    expires_at = sharing_settings['expires_at']
+    Time.parse(expires_at) if expires_at.present?
+  rescue ArgumentError
+    nil
+  end
+
+  def family_sharing_duration
+    settings.dig('family', 'location_sharing', 'duration') || 'permanent'
+  end
+
+  def latest_location_for_family
+    return nil unless family_sharing_enabled?
+
+    # Use select to only fetch needed columns and limit to 1 for efficiency
+    latest_point = points.select(:latitude, :longitude, :timestamp)
+                         .order(timestamp: :desc)
+                         .limit(1)
+                         .first
+
+    return nil unless latest_point
+
+    {
+      user_id: id,
+      email: email,
+      latitude: latest_point.latitude,
+      longitude: latest_point.longitude,
+      timestamp: latest_point.timestamp,
+      updated_at: Time.at(latest_point.timestamp)
+    }
+  end
+
+  private
+
+  def check_family_ownership
+    return if can_delete_account?
+
+    errors.add(:base, 'Cannot delete account while being a family owner with other members')
+    raise ActiveRecord::DeleteRestrictionError, 'Cannot delete user with family members'
+  end
+end

app/models/user.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 class User < ApplicationRecord # rubocop:disable Metrics/ClassLength
+  include UserFamily
   devise :database_authenticatable, :registerable,
          :recoverable, :rememberable, :validatable, :trackable
 
@@ -15,21 +16,12 @@ class User < ApplicationRecord # rubocop:disable Metrics/ClassLength
   has_many :trips,  dependent: :destroy
   has_many :tracks, dependent: :destroy
 
-  # Family associations
-  has_one :family_membership, dependent: :destroy
-  has_one :family, through: :family_membership
-  has_one :created_family, class_name: 'Family', foreign_key: 'creator_id', inverse_of: :creator, dependent: :destroy
-  has_many :sent_family_invitations, class_name: 'FamilyInvitation', foreign_key: 'invited_by_id',
-inverse_of: :invited_by, dependent: :destroy
-
   after_create :create_api_key
   after_commit :activate, on: :create, if: -> { DawarichSettings.self_hosted? }
   after_commit :start_trial, on: :create, if: -> { !DawarichSettings.self_hosted? }
 
   before_save :sanitize_input
 
-  before_destroy :check_family_ownership
-
   validates :email, presence: true
   validates :reset_password_token, uniqueness: true, allow_nil: true
 
@@ -171,13 +163,6 @@ inverse_of: :invited_by, dependent: :destroy
     settings.try(:[], 'maps')&.try(:[], 'url')&.strip!
   end
 
-  def check_family_ownership
-    return if can_delete_account?
-
-    errors.add(:base, 'Cannot delete account while being a family owner with other members')
-    raise ActiveRecord::DeleteRestrictionError, 'Cannot delete user with family members'
-  end
-
   def start_trial
     update(status: :trial, active_until: 7.days.from_now)
     schedule_welcome_emails
@@ -197,118 +182,4 @@ inverse_of: :invited_by, dependent: :destroy
     Users::MailerSendingJob.set(wait: 9.days).perform_later(id, 'post_trial_reminder_early')
     Users::MailerSendingJob.set(wait: 14.days).perform_later(id, 'post_trial_reminder_late')
   end
-
-  public
-
-  # Family-related methods
-  def in_family?
-    family_membership.present?
-  end
-
-  def family_owner?
-    family_membership&.owner? == true
-  end
-
-  def can_delete_account?
-    return true unless family_owner?
-    return true unless family
-
-    family.members.count <= 1
-  end
-
-  def family_sharing_enabled?
-    # User must be in a family and have explicitly enabled location sharing
-    return false unless in_family?
-
-    sharing_settings = settings.dig('family', 'location_sharing')
-    return false if sharing_settings.blank?
-
-    # If it's a boolean (legacy support), return it
-    return sharing_settings if [true, false].include?(sharing_settings)
-
-    # If it's time-limited sharing, check if it's still active
-    if sharing_settings.is_a?(Hash)
-      return false unless sharing_settings['enabled'] == true
-
-      # Check if sharing has an expiration
-      expires_at = sharing_settings['expires_at']
-      return expires_at.blank? || Time.parse(expires_at) > Time.current
-    end
-
-    false
-  end
-
-  def update_family_location_sharing!(enabled, duration: nil)
-    return false unless in_family?
-
-    current_settings = settings || {}
-    current_settings['family'] ||= {}
-
-    if enabled
-      sharing_config = { 'enabled' => true }
-
-      # Add expiration if duration is specified
-      if duration.present?
-        expiration_time = case duration
-        when '1h'
-          1.hour.from_now
-        when '6h'
-          6.hours.from_now
-        when '12h'
-          12.hours.from_now
-        when '24h'
-          24.hours.from_now
-        when 'permanent'
-          nil # No expiration
-        else
-          # Custom duration in hours
-          duration.to_i.hours.from_now if duration.to_i > 0
-        end
-
-        sharing_config['expires_at'] = expiration_time.iso8601 if expiration_time
-        sharing_config['duration'] = duration
-      end
-
-      current_settings['family']['location_sharing'] = sharing_config
-    else
-      current_settings['family']['location_sharing'] = { 'enabled' => false }
-    end
-
-    update!(settings: current_settings)
-  end
-
-  def family_sharing_expires_at
-    sharing_settings = settings.dig('family', 'location_sharing')
-    return nil unless sharing_settings.is_a?(Hash)
-
-    expires_at = sharing_settings['expires_at']
-    Time.parse(expires_at) if expires_at.present?
-  rescue ArgumentError
-    nil
-  end
-
-  def family_sharing_duration
-    settings.dig('family', 'location_sharing', 'duration') || 'permanent'
-  end
-
-  def latest_location_for_family
-    return nil unless family_sharing_enabled?
-
-    # Use select to only fetch needed columns and limit to 1 for efficiency
-    latest_point = points.select(:latitude, :longitude, :timestamp)
-                         .order(timestamp: :desc)
-                         .limit(1)
-                         .first
-
-    return nil unless latest_point
-
-    {
-      user_id: id,
-      email: email,
-      latitude: latest_point.latitude,
-      longitude: latest_point.longitude,
-      timestamp: latest_point.timestamp,
-      updated_at: Time.at(latest_point.timestamp)
-    }
-  end
 end

docs/database_index_audit.md

@@ -0,0 +1,29 @@
+# Database Index Audit (2025-10-05)
+
+## Observed ActiveRecord Query Patterns
+- **Visits range filter** – `log/test.log:91056` shows repeated lookups with `WHERE "visits"."user_id" = ? AND (started_at >= ? AND ended_at <= ?)` ordered by `started_at`.
+- **Imports deduplication checks** – `log/test.log:11130` runs `SELECT 1 FROM "imports" WHERE "name" = ? AND "user_id" = ?` (and variants excluding an `id`).
+- **Family invitations association** – `app/models/user.rb:22` loads `sent_family_invitations`, which issues queries on `invited_by_id` even though only `family_id` currently has an index (`db/schema.rb:108-120`).
+
+## Missing or Weak Index Coverage
+1. **`family_invitations(invited_by_id)`**  
+   - Evidence: association in `app/models/user.rb:22` plus schema definition at `db/schema.rb:112` lacking an index.  
+   - Risk: every `user.sent_family_invitations` call scans by `invited_by_id`, which will degrade as invitation counts grow.  
+   - Suggested fix: add `add_index :family_invitations, :invited_by_id` (consider `validate: false` first, then `validate_foreign_key` to avoid locking).
+
+2. **`visits(user_id, started_at, ended_at)`**  
+   - Evidence: range queries in `log/test.log:91056` rely on `user_id` plus `started_at`/`ended_at`, yet the table only has single-column indexes on `user_id` and `started_at` (`db/schema.rb:338-339`).  
+   - Risk: planner must combine two indexes or fall back to seq scans for wide ranges.  
+   - Suggested fix: add a composite index such as `add_index :visits, [:user_id, :started_at, :ended_at]` (or at minimum `[:user_id, :started_at]`) to cover the filter and ordering.
+
+3. **`imports(user_id, name)`**  
+   - Evidence: deduplication queries in `log/test.log:11130` filter on both columns while only `user_id` is indexed (`db/schema.rb:146-148`).  
+   - Risk: duplicate checks for large import histories become progressively slower.  
+   - Suggested fix: add a unique composite index `add_index :imports, [:user_id, :name], unique: true` if business rules prevent duplicate filenames per user.
+
+## Potentially Unused Indexes
+- `active_storage_attachments.blob_id` (`db/schema.rb:34`) and `active_storage_variant_records(blob_id, variation_digest)` (`db/schema.rb:53`) do not appear in application code outside Active Storage internals. They are required for Active Storage itself, so no action recommended beyond periodic verification with `ANALYZE` stats.
+
+## Next Steps
+- Generate and run migrations for the suggested indexes in development, then `EXPLAIN ANALYZE` the affected queries to confirm improved plans.  
+- After deploying, monitor `pg_stat_statements` or query logs to ensure the new indexes are used and to detect any remaining hotspots.