From 018760812a011b09376744b071312951fdcab1be Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Sun, 5 Oct 2025 14:24:45 +0200 Subject: [PATCH] Extract family functionality to a concern --- .../family/invitations_controller.rb | 19 +-- .../users/registrations_controller.rb | 16 +-- app/models/concerns/user_family.rb | 135 ++++++++++++++++++ app/models/user.rb | 131 +---------------- docs/database_index_audit.md | 29 ++++ 5 files changed, 174 insertions(+), 156 deletions(-) create mode 100644 app/models/concerns/user_family.rb create mode 100644 docs/database_index_audit.md diff --git a/app/controllers/family/invitations_controller.rb b/app/controllers/family/invitations_controller.rb index 7e4108ca..9bf53cd8 100644 --- a/app/controllers/family/invitations_controller.rb +++ b/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 diff --git a/app/controllers/users/registrations_controller.rb b/app/controllers/users/registrations_controller.rb index b6bb8f54..8193d199 100644 --- a/app/controllers/users/registrations_controller.rb +++ b/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 - family_path - else - super(resource) - end + return family_path if @invitation&.family + + super(resource) end def after_inactive_sign_up_path_for(resource) - if @invitation&.family - family_path - else - super(resource) - end + return family_path if @invitation&.family + + super(resource) end private diff --git a/app/models/concerns/user_family.rb b/app/models/concerns/user_family.rb new file mode 100644 index 00000000..de1f56ef --- /dev/null +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index b8076f32..c5d5d337 100644 --- a/app/models/user.rb +++ b/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 diff --git a/docs/database_index_audit.md b/docs/database_index_audit.md new file mode 100644 index 00000000..cd2e4c8e --- /dev/null +++ b/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.