From 923ea113c8b7bfd559421ffeb24effc95c6892a7 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Sat, 11 Oct 2025 14:17:48 +0200 Subject: [PATCH] Fix some minor stuff --- .github/workflows/build_and_push.yml | 12 - app/jobs/family/invitations/cleanup_job.rb | 4 +- app/mailers/family_mailer.rb | 10 + app/models/family.rb | 3 - app/models/family/invitation.rb | 2 +- app/models/family/membership.rb | 2 +- app/models/family_invitation.rb | 44 --- app/models/family_membership.rb | 21 -- app/policies/family/invitation_policy.rb | 6 +- app/policies/family/membership_policy.rb | 10 +- app/services/families/accept_invitation.rb | 22 +- app/services/families/create.rb | 31 +- app/services/families/invite.rb | 5 +- app/services/families/locations.rb | 5 +- app/services/families/memberships/destroy.rb | 300 ++++++++--------- .../families/update_location_sharing.rb | 11 +- app/views/devise/sessions/new.html.erb | 5 - app/views/families/show.html.erb | 2 +- .../family_mailer/member_joined.html.erb | 39 +++ .../family_mailer/member_joined.text.erb | 18 + docs/database_index_audit.md | 29 -- .../policies/family/invitation_policy_spec.rb | 264 +++++++++++++++ .../policies/family/membership_policy_spec.rb | 312 ++++++++++++++++++ 23 files changed, 852 insertions(+), 305 deletions(-) delete mode 100644 app/models/family_invitation.rb delete mode 100644 app/models/family_membership.rb create mode 100644 app/views/family_mailer/member_joined.html.erb create mode 100644 app/views/family_mailer/member_joined.text.erb delete mode 100644 docs/database_index_audit.md create mode 100644 spec/policies/family/invitation_policy_spec.rb create mode 100644 spec/policies/family/membership_policy_spec.rb diff --git a/.github/workflows/build_and_push.yml b/.github/workflows/build_and_push.yml index b88c72e8..3c04cdb6 100644 --- a/.github/workflows/build_and_push.yml +++ b/.github/workflows/build_and_push.yml @@ -74,18 +74,6 @@ jobs: # Set platforms based on version type and release type PLATFORMS="linux/amd64,linux/arm64,linux/arm/v8,linux/arm/v7" - # Check if this is a patch version (x.y.z where z > 0) - if [[ $VERSION =~ ^[0-9]+\.[0-9]+\.[1-9][0-9]*$ ]]; then - echo "Detected patch version ($VERSION) - building for AMD64 only" - PLATFORMS="linux/amd64" - elif [[ $VERSION =~ ^[0-9]+\.[0-9]+\.0$ ]]; then - echo "Detected minor version ($VERSION) - building for all platforms" - PLATFORMS="linux/amd64,linux/arm64,linux/arm/v8,linux/arm/v7" - else - echo "Version format not recognized or non-semver - using AMD64 only for safety" - PLATFORMS="linux/amd64" - fi - # Add :rc tag for pre-releases if [ "${{ github.event.release.prerelease }}" = "true" ]; then TAGS="${TAGS},freikin/dawarich:rc" diff --git a/app/jobs/family/invitations/cleanup_job.rb b/app/jobs/family/invitations/cleanup_job.rb index b5076472..9d084e92 100644 --- a/app/jobs/family/invitations/cleanup_job.rb +++ b/app/jobs/family/invitations/cleanup_job.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true -class FamilyInvitationsCleanupJob < ApplicationJob - queue_as :default +class Family::Invitations::CleanupJob < ApplicationJob + queue_as :family def perform Rails.logger.info 'Starting family invitations cleanup' diff --git a/app/mailers/family_mailer.rb b/app/mailers/family_mailer.rb index e86c68ea..b0c2673b 100644 --- a/app/mailers/family_mailer.rb +++ b/app/mailers/family_mailer.rb @@ -12,4 +12,14 @@ class FamilyMailer < ApplicationMailer subject: "🎉 You've been invited to join #{@family.name} on Dawarich!" ) end + + def member_joined(family, user) + @family = family + @user = user + + mail( + to: @family.owner.email, + subject: "👪 #{@user.name} has joined your family #{@family.name} on Dawarich!" + ) + end end diff --git a/app/models/family.rb b/app/models/family.rb index 421fce79..51123293 100644 --- a/app/models/family.rb +++ b/app/models/family.rb @@ -10,9 +10,6 @@ class Family < ApplicationRecord MAX_MEMBERS = 5 - scope :with_members, -> { includes(:members, :family_memberships) } - scope :with_pending_invitations, -> { includes(family_invitations: :invited_by) } - def can_add_members? (member_count + pending_invitations_count) < MAX_MEMBERS end diff --git a/app/models/family/invitation.rb b/app/models/family/invitation.rb index 68fb4c6d..a2739291 100644 --- a/app/models/family/invitation.rb +++ b/app/models/family/invitation.rb @@ -41,6 +41,6 @@ class Family::Invitation < ApplicationRecord end def clear_family_cache - family&.clear_member_cache! + family.clear_member_cache! end end diff --git a/app/models/family/membership.rb b/app/models/family/membership.rb index 63ef5e1f..da982204 100644 --- a/app/models/family/membership.rb +++ b/app/models/family/membership.rb @@ -18,6 +18,6 @@ class Family::Membership < ApplicationRecord private def clear_family_cache - family&.clear_member_cache! + family.clear_member_cache! end end diff --git a/app/models/family_invitation.rb b/app/models/family_invitation.rb deleted file mode 100644 index 2f197280..00000000 --- a/app/models/family_invitation.rb +++ /dev/null @@ -1,44 +0,0 @@ -# frozen_string_literal: true - -class FamilyInvitation < ApplicationRecord - EXPIRY_DAYS = 7 - - belongs_to :family - belongs_to :invited_by, class_name: 'User' - - validates :email, presence: true, format: { with: URI::MailTo::EMAIL_REGEXP } - validates :token, presence: true, uniqueness: true - validates :expires_at, :status, presence: true - - enum :status, { pending: 0, accepted: 1, expired: 2, cancelled: 3 } - - scope :active, -> { where(status: :pending).where('expires_at > ?', Time.current) } - - before_validation :generate_token, :set_expiry, on: :create - - after_create :clear_family_cache - after_update :clear_family_cache, if: :saved_change_to_status? - after_destroy :clear_family_cache - - def expired? - expires_at < Time.current - end - - def can_be_accepted? - pending? && !expired? - end - - private - - def generate_token - self.token = SecureRandom.urlsafe_base64(32) if token.blank? - end - - def set_expiry - self.expires_at = EXPIRY_DAYS.days.from_now if expires_at.blank? - end - - def clear_family_cache - family&.clear_member_cache! - end -end diff --git a/app/models/family_membership.rb b/app/models/family_membership.rb deleted file mode 100644 index 711e1ef1..00000000 --- a/app/models/family_membership.rb +++ /dev/null @@ -1,21 +0,0 @@ -# frozen_string_literal: true - -class FamilyMembership < ApplicationRecord - belongs_to :family - belongs_to :user - - validates :user_id, presence: true, uniqueness: true - validates :role, presence: true - - enum :role, { owner: 0, member: 1 } - - after_create :clear_family_cache - after_update :clear_family_cache - after_destroy :clear_family_cache - - private - - def clear_family_cache - family&.clear_member_cache! - end -end diff --git a/app/policies/family/invitation_policy.rb b/app/policies/family/invitation_policy.rb index b65d6ae8..537cd7ec 100644 --- a/app/policies/family/invitation_policy.rb +++ b/app/policies/family/invitation_policy.rb @@ -7,16 +7,20 @@ class Family::InvitationPolicy < ApplicationPolicy end def create? + return false unless user + user.family == record.family && user.family_owner? end def accept? # Users can accept invitations sent to their email + return false unless user + user.email == record.email end def destroy? # Only family owners can cancel invitations - user.family == record.family && user.family_owner? + create? end end diff --git a/app/policies/family/membership_policy.rb b/app/policies/family/membership_policy.rb index 4a80b125..d0e46954 100644 --- a/app/policies/family/membership_policy.rb +++ b/app/policies/family/membership_policy.rb @@ -2,22 +2,28 @@ class Family::MembershipPolicy < ApplicationPolicy def show? + return false unless user + user.family == record.family end def update? + return false unless user + # Users can update their own settings return true if user == record.user # Family owners can update any member's settings - user.family == record.family && user.family_owner? + show? && user.family_owner? end def destroy? + return false unless user + # Users can remove themselves (handled by family leave logic) return true if user == record.user # Family owners can remove other members - user.family == record.family && user.family_owner? + update? end end diff --git a/app/services/families/accept_invitation.rb b/app/services/families/accept_invitation.rb index 4869d9fe..a270cdd0 100644 --- a/app/services/families/accept_invitation.rb +++ b/app/services/families/accept_invitation.rb @@ -15,6 +15,7 @@ module Families if user.in_family? @error_message = 'You must leave your current family before joining a new one.' + return false end @@ -47,6 +48,7 @@ module Families return true if invitation.can_be_accepted? @error_message = 'This invitation is no longer valid or has expired.' + false end @@ -54,6 +56,7 @@ module Families return true if invitation.email == user.email @error_message = 'This invitation is not for your email address.' + false end @@ -61,6 +64,7 @@ module Families return true unless invitation.family.full? @error_message = 'This family has reached the maximum number of members.' + false end @@ -100,21 +104,21 @@ module Families content: "#{user.email} has joined your family" ) rescue StandardError => e - # Don't fail the entire operation if notification fails - Rails.logger.warn "Failed to send family join notification: #{e.message}" + ExceptionReporter.call(e, "Unexpected error in Families::AcceptInvitation: #{e.message}") end def handle_record_invalid_error(error) - @error_message = if error.record&.errors&.any? - error.record.errors.full_messages.first - else - "Failed to join family: #{error.message}" - end + @error_message = + if error.record&.errors&.any? + error.record.errors.full_messages.first + else + "Failed to join family: #{error.message}" + end end def handle_generic_error(error) - Rails.logger.error "Unexpected error in Families::AcceptInvitation: #{error.message}" - Rails.logger.error error.backtrace.join("\n") + ExceptionReporter.call(error, "Unexpected error in Families::AcceptInvitation: #{error.message}") + @error_message = 'An unexpected error occurred while joining the family. Please try again' end end diff --git a/app/services/families/create.rb b/app/services/families/create.rb index c958dc32..6cd56e21 100644 --- a/app/services/families/create.rb +++ b/app/services/families/create.rb @@ -32,12 +32,15 @@ module Families true rescue ActiveRecord::RecordInvalid => e handle_record_invalid_error(e) + false rescue ActiveRecord::RecordNotUnique => e handle_uniqueness_error(e) + false rescue StandardError => e handle_generic_error(e) + false end @@ -60,11 +63,13 @@ module Families def validate_feature_access return true if can_create_family? - @error_message = if DawarichSettings.self_hosted? - 'Family feature is not available on this instance' - else - 'Family feature requires an active subscription' - end + @error_message = + if DawarichSettings.self_hosted? + 'Family feature is not available on this instance' + else + 'Family feature requires an active subscription' + end + false end @@ -99,15 +104,16 @@ module Families ) rescue StandardError => e # Don't fail the entire operation if notification fails - Rails.logger.warn "Failed to send family creation notification: #{e.message}" + ExceptionReporter.call(e, "Unexpected error in Families::Create: #{e.message}") end def handle_record_invalid_error(error) - if family&.errors&.any? - @error_message = family.errors.full_messages.first - else - @error_message = "Failed to create family: #{error.message}" - end + @error_message = + if family&.errors&.any? + family.errors.full_messages.first + else + "Failed to create family: #{error.message}" + end end def handle_uniqueness_error(_error) @@ -115,8 +121,7 @@ module Families end def handle_generic_error(error) - Rails.logger.error "Unexpected error in Families::Create: #{error.message}" - Rails.logger.error error.backtrace.join("\n") + ExceptionReporter.call(error, "Unexpected error in Families::Create: #{error.message}") @error_message = 'An unexpected error occurred while creating the family. Please try again' end end diff --git a/app/services/families/invite.rb b/app/services/families/invite.rb index 322109cf..b288c921 100644 --- a/app/services/families/invite.rb +++ b/app/services/families/invite.rb @@ -100,7 +100,7 @@ module Families ) rescue StandardError => e # Don't fail the entire operation if notification fails - Rails.logger.warn "Failed to send invitation notification: #{e.message}" + ExceptionReporter.call(e, "Unexpected error in Families::Invite: #{e.message}") end def handle_record_invalid_error(error) @@ -120,8 +120,7 @@ module Families end def handle_generic_error(error) - Rails.logger.error "Unexpected error in Families::Invite: #{error.message}" - Rails.logger.error error.backtrace.join("\n") + ExceptionReporter.call(error, "Unexpected error in Families::Invite: #{error.message}") @custom_error_message = 'An unexpected error occurred while sending the invitation. Please try again' end end diff --git a/app/services/families/locations.rb b/app/services/families/locations.rb index 4fa6ab9d..babe7b3c 100644 --- a/app/services/families/locations.rb +++ b/app/services/families/locations.rb @@ -30,7 +30,8 @@ class Families::Locations end def build_family_locations(sharing_members) - latest_points = sharing_members.map { |member| member.points.last }.compact + latest_points = + sharing_members.map { _1.points.last }.compact latest_points.map do |point| next unless point @@ -42,7 +43,7 @@ class Families::Locations latitude: point.lat.to_f, longitude: point.lon.to_f, timestamp: point.timestamp.to_i, - updated_at: Time.at(point.timestamp.to_i) + updated_at: Time.zone.at(point.timestamp.to_i) } end.compact end diff --git a/app/services/families/memberships/destroy.rb b/app/services/families/memberships/destroy.rb index 5439f40e..1f6d181e 100644 --- a/app/services/families/memberships/destroy.rb +++ b/app/services/families/memberships/destroy.rb @@ -5,176 +5,176 @@ module Families class Destroy attr_reader :user, :member_to_remove, :error_message - def initialize(user:, member_to_remove: nil) - @user = user # The user performing the action (current_user) - @member_to_remove = member_to_remove || user # The user being removed (defaults to self) - @error_message = nil - end - - def call - return false unless validate_can_leave - - # Store family info before removing membership - @family_name = member_to_remove.family.name - @family_owner = member_to_remove.family.owner - - ActiveRecord::Base.transaction do - handle_ownership_transfer if member_to_remove.family_owner? - remove_membership - send_notifications + def initialize(user:, member_to_remove: nil) + @user = user # The user performing the action (current_user) + @member_to_remove = member_to_remove || user # The user being removed (defaults to self) + @error_message = nil end - true - rescue ActiveRecord::RecordInvalid => e - handle_record_invalid_error(e) - false - rescue StandardError => e - handle_generic_error(e) - false - end + def call + return false unless validate_can_leave - private + # Store family info before removing membership + @family_name = member_to_remove.family.name + @family_owner = member_to_remove.family.owner - def validate_can_leave - return false unless validate_in_family - return false unless validate_removal_allowed + ActiveRecord::Base.transaction do + handle_ownership_transfer if member_to_remove.family_owner? + remove_membership + send_notifications + end - true - end + true + rescue ActiveRecord::RecordInvalid => e + handle_record_invalid_error(e) - def validate_in_family - return true if member_to_remove.in_family? + false + rescue StandardError => e + handle_generic_error(e) - @error_message = 'User is not currently in a family.' - false - end - - def validate_removal_allowed - # If removing self (user == member_to_remove) - if removing_self? - return validate_owner_can_leave + false end - # If removing another member, user must be owner and member must be in same family - return false unless validate_remover_is_owner - return false unless validate_same_family - return false unless validate_not_removing_owner + private - true - end + def validate_can_leave + return false unless validate_in_family + return false unless validate_removal_allowed - def removing_self? - user == member_to_remove - end - - def validate_owner_can_leave - return true unless member_to_remove.family_owner? - - @error_message = 'Family owners cannot remove their own membership. To leave the family, delete it instead.' - false - end - - def validate_remover_is_owner - return true if user.family_owner? - - @error_message = 'Only family owners can remove other members.' - false - end - - def validate_same_family - return true if user.family == member_to_remove.family - - @error_message = 'Cannot remove members from a different family.' - false - end - - def validate_not_removing_owner - return true unless member_to_remove.family_owner? - - @error_message = 'Cannot remove the family owner. The owner must delete the family or leave on their own.' - false - end - - def family_has_other_members? - member_to_remove.family.members.count > 1 - end - - def handle_ownership_transfer - # If this is the last member (owner), delete the family - return unless member_to_remove.family.members.count == 1 - - member_to_remove.family.destroy! - - # If owner tries to leave with other members, it should be prevented in validation - end - - def remove_membership - member_to_remove.family_membership.destroy! - end - - def send_notifications - return unless defined?(Notification) - - if removing_self? - send_self_removal_notifications - else - send_member_removed_notifications + true end - end - def send_self_removal_notifications - # Notify the user who left - Notification.create!( - user: member_to_remove, - kind: :info, - title: 'Left Family', - content: "You've left the family \"#{@family_name}\"" - ) + def validate_in_family + return true if member_to_remove.in_family? - # Notify the family owner - return unless @family_owner&.persisted? + @error_message = 'User is not currently in a family.' + false + end - Notification.create!( - user: @family_owner, - kind: :info, - title: 'Family Member Left', - content: "#{member_to_remove.email} has left the family \"#{@family_name}\"" - ) - end + def validate_removal_allowed + # If removing self (user == member_to_remove) + return validate_owner_can_leave if removing_self? - def send_member_removed_notifications - # Notify the member who was removed - Notification.create!( - user: member_to_remove, - kind: :info, - title: 'Removed from Family', - content: "You have been removed from the family \"#{@family_name}\" by #{user.email}" - ) + # If removing another member, user must be owner and member must be in same family + return false unless validate_remover_is_owner + return false unless validate_same_family + return false unless validate_not_removing_owner - # Notify the owner who removed the member (if different from the member) - return unless user != member_to_remove + true + end - Notification.create!( - user: user, - kind: :info, - title: 'Member Removed', - content: "#{member_to_remove.email} has been removed from the family \"#{@family_name}\"" - ) - end + def removing_self? + user == member_to_remove + end - def handle_record_invalid_error(error) - @error_message = if error.record&.errors&.any? - error.record.errors.full_messages.first - else - "Failed to leave family: #{error.message}" - end - end + def validate_owner_can_leave + return true unless member_to_remove.family_owner? - def handle_generic_error(error) - Rails.logger.error "Unexpected error in Families::Memberships::Destroy: #{error.message}" - Rails.logger.error error.backtrace.join("\n") - @error_message = 'An unexpected error occurred while removing the membership. Please try again' - end + @error_message = 'Family owners cannot remove their own membership. To leave the family, delete it instead.' + false + end + + def validate_remover_is_owner + return true if user.family_owner? + + @error_message = 'Only family owners can remove other members.' + false + end + + def validate_same_family + return true if user.family == member_to_remove.family + + @error_message = 'Cannot remove members from a different family.' + false + end + + def validate_not_removing_owner + return true unless member_to_remove.family_owner? + + @error_message = 'Cannot remove the family owner. The owner must delete the family or leave on their own.' + false + end + + def family_has_other_members? + member_to_remove.family.members.count > 1 + end + + def handle_ownership_transfer + # If this is the last member (owner), delete the family + return unless member_to_remove.family.members.count == 1 + + member_to_remove.family.destroy! + + # If owner tries to leave with other members, it should be prevented in validation + end + + def remove_membership + member_to_remove.family_membership.destroy! + end + + def send_notifications + return unless defined?(Notification) + + if removing_self? + send_self_removal_notifications + else + send_member_removed_notifications + end + end + + def send_self_removal_notifications + # Notify the user who left + Notification.create!( + user: member_to_remove, + kind: :info, + title: 'Left Family', + content: "You've left the family \"#{@family_name}\"" + ) + + # Notify the family owner + return unless @family_owner&.persisted? + + Notification.create!( + user: @family_owner, + kind: :info, + title: 'Family Member Left', + content: "#{member_to_remove.email} has left the family \"#{@family_name}\"" + ) + end + + def send_member_removed_notifications + # Notify the member who was removed + Notification.create!( + user: member_to_remove, + kind: :info, + title: 'Removed from Family', + content: "You have been removed from the family \"#{@family_name}\" by #{user.email}" + ) + + # Notify the owner who removed the member (if different from the member) + return unless user != member_to_remove + + Notification.create!( + user: user, + kind: :info, + title: 'Member Removed', + content: "#{member_to_remove.email} has been removed from the family \"#{@family_name}\"" + ) + end + + def handle_record_invalid_error(error) + @error_message = + if error.record&.errors&.any? + error.record.errors.full_messages.first + else + "Failed to leave family: #{error.message}" + end + end + + def handle_generic_error(error) + ExceptionReporter.call(error, "Unexpected error in Families::Memberships::Destroy: #{error.message}") + @error_message = 'An unexpected error occurred while removing the membership. Please try again' + end end end end diff --git a/app/services/families/update_location_sharing.rb b/app/services/families/update_location_sharing.rb index 258f9d7e..a0cc9f12 100644 --- a/app/services/families/update_location_sharing.rb +++ b/app/services/families/update_location_sharing.rb @@ -11,13 +11,12 @@ class Families::UpdateLocationSharing end def call - if update_location_sharing - success_result - else - failure_result('Failed to update location sharing setting', :unprocessable_content) - end + return success_result if update_location_sharing + + failure_result('Failed to update location sharing setting', :unprocessable_content) rescue => error - Rails.logger.error("Failed to update family location sharing: #{error.message}") if defined?(Rails) + ExceptionReporter.call(error, "Error in Families::UpdateLocationSharing: #{error.message}") + failure_result('An error occurred while updating location sharing', :internal_server_error) end diff --git a/app/views/devise/sessions/new.html.erb b/app/views/devise/sessions/new.html.erb index a573f170..b471a5cf 100644 --- a/app/views/devise/sessions/new.html.erb +++ b/app/views/devise/sessions/new.html.erb @@ -16,11 +16,6 @@ <% else %>

Login now

and take control over your location data.

- <% if ENV['DEMO_ENV'] == 'true' %> -

- Demo account: demo@dawarich.app / password: password -

- <% end %> <% end %>
diff --git a/app/views/families/show.html.erb b/app/views/families/show.html.erb index a379c543..4ab423f8 100644 --- a/app/views/families/show.html.erb +++ b/app/views/families/show.html.erb @@ -209,7 +209,7 @@ <%= t('families.show.invite_member', default: 'Invite New Member') %> - <%= form_with model: [@family, FamilyInvitation.new], url: family_invitations_path(@family), local: true, class: "space-y-3" do |form| %> + <%= form_with model: [@family, Family::Invitation.new], url: family_invitations_path(@family), local: true, class: "space-y-3" do |form| %>
<%= form.label :email, t('families.show.email_label', default: 'Email Address'), class: "label label-text font-medium mb-1" %> <%= form.email_field :email, diff --git a/app/views/family_mailer/member_joined.html.erb b/app/views/family_mailer/member_joined.html.erb new file mode 100644 index 00000000..f2429b92 --- /dev/null +++ b/app/views/family_mailer/member_joined.html.erb @@ -0,0 +1,39 @@ +
+
+

🎉 Great news! Someone joined your family!

+ +

Hi <%= @family.owner.email %>!

+ +

+ We're excited to let you know that <%= @user.email %> has just joined your family + "<%= @family.name %>" on Dawarich! +

+ +
+

Now you can:

+
    +
  • See <%= @user.email %>'s current location (if they've enabled sharing)
  • +
  • Stay connected with your growing family
  • +
  • Share your location with <%= @user.email %>
  • +
  • Manage family members and settings from your family page
  • +
+
+ +
+

+ 💡 Tip: You can manage your family members and privacy settings at any time from your family dashboard. +

+
+ +

+ Your family now has <%= @family.member_count %> member<%= @family.member_count == 1 ? '' : 's' %>. +

+ +
+ +

+ Best regards,
+ Evgenii from Dawarich +

+
+
diff --git a/app/views/family_mailer/member_joined.text.erb b/app/views/family_mailer/member_joined.text.erb new file mode 100644 index 00000000..ba840d38 --- /dev/null +++ b/app/views/family_mailer/member_joined.text.erb @@ -0,0 +1,18 @@ +Great news! Someone joined your family! + +Hi <%= @family.owner.email %>! + +We're excited to let you know that <%= @user.email %> has just joined your family "<%= @family.name %>" on Dawarich! + +Now you can: +• See <%= @user.email %>'s current location (if they've enabled sharing) +• Stay connected with your growing family +• Share your location with <%= @user.email %> +• Manage family members and settings from your family page + +TIP: You can manage your family members and privacy settings at any time from your family dashboard. + +Your family now has <%= @family.member_count %> member<%= @family.member_count == 1 ? '' : 's' %>. + +Best regards, +Evgenii from Dawarich diff --git a/docs/database_index_audit.md b/docs/database_index_audit.md deleted file mode 100644 index cd2e4c8e..00000000 --- a/docs/database_index_audit.md +++ /dev/null @@ -1,29 +0,0 @@ -# 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. diff --git a/spec/policies/family/invitation_policy_spec.rb b/spec/policies/family/invitation_policy_spec.rb new file mode 100644 index 00000000..8cfd4dac --- /dev/null +++ b/spec/policies/family/invitation_policy_spec.rb @@ -0,0 +1,264 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Family::InvitationPolicy, type: :policy do + let(:family) { create(:family) } + let(:owner) { family.creator } + let(:member) { create(:user) } + let(:other_user) { create(:user) } + let(:invitation) { create(:family_invitation, family: family, invited_by: owner) } + + before do + # Set up family membership for owner + create(:family_membership, family: family, user: owner, role: :owner) + # Set up family membership for regular member + create(:family_membership, family: family, user: member, role: :member) + end + + describe '#show?' do + context 'with authenticated user' do + it 'allows any authenticated user to view invitation' do + policy = Family::InvitationPolicy.new(owner, invitation) + + expect(policy).to permit(:show) + end + + it 'allows other users to view invitation' do + policy = Family::InvitationPolicy.new(other_user, invitation) + + expect(policy).to permit(:show) + end + end + + context 'with unauthenticated user' do + it 'allows unauthenticated access (public endpoint)' do + policy = Family::InvitationPolicy.new(nil, invitation) + + expect(policy).to permit(:show) + end + end + end + + describe '#create?' do + context 'when user is family owner' do + before do + allow(owner).to receive(:family).and_return(family) + allow(owner).to receive(:family_owner?).and_return(true) + end + + it 'allows family owner to create invitations' do + policy = Family::InvitationPolicy.new(owner, invitation) + + expect(policy).to permit(:create) + end + end + + context 'when user is regular family member' do + before do + allow(member).to receive(:family).and_return(family) + allow(member).to receive(:family_owner?).and_return(false) + end + + it 'denies regular family member from creating invitations' do + policy = Family::InvitationPolicy.new(member, invitation) + + expect(policy).not_to permit(:create) + end + end + + context 'when user is not in the family' do + it 'denies user not in the family from creating invitations' do + policy = Family::InvitationPolicy.new(other_user, invitation) + + expect(policy).not_to permit(:create) + end + end + + context 'with unauthenticated user' do + it 'denies unauthenticated user from creating invitations' do + policy = Family::InvitationPolicy.new(nil, invitation) + + expect(policy).not_to permit(:create) + end + end + end + + describe '#accept?' do + context 'when user email matches invitation email' do + let(:invited_user) { create(:user, email: invitation.email) } + + it 'allows user to accept invitation sent to their email' do + policy = Family::InvitationPolicy.new(invited_user, invitation) + + expect(policy).to permit(:accept) + end + end + + context 'when user email does not match invitation email' do + it 'denies user with different email from accepting invitation' do + policy = Family::InvitationPolicy.new(other_user, invitation) + + expect(policy).not_to permit(:accept) + end + end + + context 'when family owner tries to accept invitation' do + it 'denies family owner from accepting invitation sent to different email' do + policy = Family::InvitationPolicy.new(owner, invitation) + + expect(policy).not_to permit(:accept) + end + end + + context 'with unauthenticated user' do + it 'denies unauthenticated user from accepting invitation' do + policy = Family::InvitationPolicy.new(nil, invitation) + + expect(policy).not_to permit(:accept) + end + end + end + + describe '#destroy?' do + context 'when user is family owner' do + before do + allow(owner).to receive(:family).and_return(family) + allow(owner).to receive(:family_owner?).and_return(true) + end + + it 'allows family owner to cancel invitations' do + policy = Family::InvitationPolicy.new(owner, invitation) + + expect(policy).to permit(:destroy) + end + end + + context 'when user is regular family member' do + before do + allow(member).to receive(:family).and_return(family) + allow(member).to receive(:family_owner?).and_return(false) + end + + it 'denies regular family member from cancelling invitations' do + policy = Family::InvitationPolicy.new(member, invitation) + + expect(policy).not_to permit(:destroy) + end + end + + context 'when user is not in the family' do + it 'denies user not in the family from cancelling invitations' do + policy = Family::InvitationPolicy.new(other_user, invitation) + + expect(policy).not_to permit(:destroy) + end + end + + context 'with unauthenticated user' do + it 'denies unauthenticated user from cancelling invitations' do + policy = Family::InvitationPolicy.new(nil, invitation) + + expect(policy).not_to permit(:destroy) + end + end + end + + describe 'edge cases' do + context 'when invitation belongs to different family' do + let(:other_family) { create(:family) } + let(:other_family_owner) { other_family.creator } + let(:other_invitation) { create(:family_invitation, family: other_family, invited_by: other_family_owner) } + + before do + create(:family_membership, family: other_family, user: other_family_owner, role: :owner) + allow(owner).to receive(:family).and_return(family) + allow(owner).to receive(:family_owner?).and_return(true) + end + + it 'denies owner from creating invitations for different family' do + policy = Family::InvitationPolicy.new(owner, other_invitation) + + expect(policy).not_to permit(:create) + end + + it 'denies owner from destroying invitations for different family' do + policy = Family::InvitationPolicy.new(owner, other_invitation) + + expect(policy).not_to permit(:destroy) + end + end + + context 'with expired invitation' do + let(:expired_invitation) { create(:family_invitation, :expired, family: family, invited_by: owner) } + let(:invited_user) { create(:user, email: expired_invitation.email) } + + it 'still allows user to attempt to accept expired invitation (business logic handles expiry)' do + policy = Family::InvitationPolicy.new(invited_user, expired_invitation) + + expect(policy).to permit(:accept) + end + + it 'allows owner to destroy expired invitation' do + allow(owner).to receive(:family).and_return(family) + allow(owner).to receive(:family_owner?).and_return(true) + policy = Family::InvitationPolicy.new(owner, expired_invitation) + + expect(policy).to permit(:destroy) + end + end + + context 'with accepted invitation' do + let(:accepted_invitation) { create(:family_invitation, :accepted, family: family, invited_by: owner) } + + it 'allows owner to destroy accepted invitation' do + allow(owner).to receive(:family).and_return(family) + allow(owner).to receive(:family_owner?).and_return(true) + policy = Family::InvitationPolicy.new(owner, accepted_invitation) + + expect(policy).to permit(:destroy) + end + end + + context 'with cancelled invitation' do + let(:cancelled_invitation) { create(:family_invitation, :cancelled, family: family, invited_by: owner) } + + it 'allows owner to destroy cancelled invitation' do + allow(owner).to receive(:family).and_return(family) + allow(owner).to receive(:family_owner?).and_return(true) + policy = Family::InvitationPolicy.new(owner, cancelled_invitation) + + expect(policy).to permit(:destroy) + end + end + end + + describe 'authorization consistency' do + it 'ensures owner can both create and destroy invitations' do + allow(owner).to receive(:family).and_return(family) + allow(owner).to receive(:family_owner?).and_return(true) + policy = Family::InvitationPolicy.new(owner, invitation) + + expect(policy).to permit(:create) + expect(policy).to permit(:destroy) + end + + it 'ensures regular members cannot create or destroy invitations' do + allow(member).to receive(:family).and_return(family) + allow(member).to receive(:family_owner?).and_return(false) + policy = Family::InvitationPolicy.new(member, invitation) + + expect(policy).not_to permit(:create) + expect(policy).not_to permit(:destroy) + end + + it 'ensures invited users can only accept their own invitations' do + invited_user = create(:user, email: invitation.email) + policy = Family::InvitationPolicy.new(invited_user, invitation) + + expect(policy).to permit(:accept) + expect(policy).not_to permit(:create) + expect(policy).not_to permit(:destroy) + end + end +end diff --git a/spec/policies/family/membership_policy_spec.rb b/spec/policies/family/membership_policy_spec.rb new file mode 100644 index 00000000..a6c83630 --- /dev/null +++ b/spec/policies/family/membership_policy_spec.rb @@ -0,0 +1,312 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Family::MembershipPolicy, type: :policy do + let(:family) { create(:family) } + let(:owner) { family.creator } + let(:member) { create(:user) } + let(:another_member) { create(:user) } + let(:other_user) { create(:user) } + + let(:owner_membership) { create(:family_membership, :owner, family: family, user: owner) } + let(:member_membership) { create(:family_membership, family: family, user: member) } + let(:another_member_membership) { create(:family_membership, family: family, user: another_member) } + + describe '#show?' do + context 'when user is in the same family' do + before do + allow(owner).to receive(:family).and_return(family) + end + + it 'allows family owner to view member details' do + policy = Family::MembershipPolicy.new(owner, member_membership) + + expect(policy).to permit(:show) + end + + it 'allows family owner to view their own membership' do + policy = Family::MembershipPolicy.new(owner, owner_membership) + + expect(policy).to permit(:show) + end + + it 'allows regular member to view other members' do + allow(member).to receive(:family).and_return(family) + policy = Family::MembershipPolicy.new(member, another_member_membership) + + expect(policy).to permit(:show) + end + + it 'allows member to view their own membership' do + allow(member).to receive(:family).and_return(family) + policy = Family::MembershipPolicy.new(member, member_membership) + + expect(policy).to permit(:show) + end + end + + context 'when user is not in the same family' do + it 'denies user from different family from viewing membership' do + policy = Family::MembershipPolicy.new(other_user, member_membership) + + expect(policy).not_to permit(:show) + end + end + + context 'with unauthenticated user' do + it 'denies unauthenticated user from viewing membership' do + policy = Family::MembershipPolicy.new(nil, member_membership) + + expect(policy).not_to permit(:show) + end + end + end + + describe '#update?' do + context 'when user is updating their own membership' do + it 'allows user to update their own membership settings' do + allow(member).to receive(:family).and_return(family) + policy = Family::MembershipPolicy.new(member, member_membership) + + expect(policy).to permit(:update) + end + + it 'allows owner to update their own membership' do + allow(owner).to receive(:family).and_return(family) + policy = Family::MembershipPolicy.new(owner, owner_membership) + + expect(policy).to permit(:update) + end + end + + context 'when user is family owner' do + before do + allow(owner).to receive(:family).and_return(family) + allow(owner).to receive(:family_owner?).and_return(true) + end + + it 'allows family owner to update other members settings' do + policy = Family::MembershipPolicy.new(owner, member_membership) + + expect(policy).to permit(:update) + end + + it 'allows family owner to update multiple members' do + policy1 = Family::MembershipPolicy.new(owner, member_membership) + policy2 = Family::MembershipPolicy.new(owner, another_member_membership) + + expect(policy1).to permit(:update) + expect(policy2).to permit(:update) + end + end + + context 'when user is regular family member' do + before do + allow(member).to receive(:family).and_return(family) + allow(member).to receive(:family_owner?).and_return(false) + end + + it 'denies regular member from updating other members settings' do + policy = Family::MembershipPolicy.new(member, another_member_membership) + + expect(policy).not_to permit(:update) + end + + it 'denies regular member from updating owner settings' do + policy = Family::MembershipPolicy.new(member, owner_membership) + + expect(policy).not_to permit(:update) + end + end + + context 'when user is not in the family' do + it 'denies user from updating membership of different family' do + policy = Family::MembershipPolicy.new(other_user, member_membership) + + expect(policy).not_to permit(:update) + end + end + + context 'with unauthenticated user' do + it 'denies unauthenticated user from updating membership' do + policy = Family::MembershipPolicy.new(nil, member_membership) + + expect(policy).not_to permit(:update) + end + end + end + + describe '#destroy?' do + context 'when user is removing themselves' do + it 'allows user to remove their own membership (leave family)' do + allow(member).to receive(:family).and_return(family) + policy = Family::MembershipPolicy.new(member, member_membership) + + expect(policy).to permit(:destroy) + end + + it 'allows owner to remove their own membership' do + allow(owner).to receive(:family).and_return(family) + policy = Family::MembershipPolicy.new(owner, owner_membership) + + expect(policy).to permit(:destroy) + end + end + + context 'when user is family owner' do + before do + allow(owner).to receive(:family).and_return(family) + allow(owner).to receive(:family_owner?).and_return(true) + end + + it 'allows family owner to remove other members' do + policy = Family::MembershipPolicy.new(owner, member_membership) + + expect(policy).to permit(:destroy) + end + + it 'allows family owner to remove multiple members' do + policy1 = Family::MembershipPolicy.new(owner, member_membership) + policy2 = Family::MembershipPolicy.new(owner, another_member_membership) + + expect(policy1).to permit(:destroy) + expect(policy2).to permit(:destroy) + end + end + + context 'when user is regular family member' do + before do + allow(member).to receive(:family).and_return(family) + allow(member).to receive(:family_owner?).and_return(false) + end + + it 'denies regular member from removing other members' do + policy = Family::MembershipPolicy.new(member, another_member_membership) + + expect(policy).not_to permit(:destroy) + end + + it 'denies regular member from removing owner' do + policy = Family::MembershipPolicy.new(member, owner_membership) + + expect(policy).not_to permit(:destroy) + end + end + + context 'when user is not in the family' do + it 'denies user from removing membership of different family' do + policy = Family::MembershipPolicy.new(other_user, member_membership) + + expect(policy).not_to permit(:destroy) + end + end + + context 'with unauthenticated user' do + it 'denies unauthenticated user from removing membership' do + policy = Family::MembershipPolicy.new(nil, member_membership) + + expect(policy).not_to permit(:destroy) + end + end + end + + describe 'edge cases' do + context 'when membership belongs to different family' do + let(:other_family) { create(:family) } + let(:other_family_owner) { other_family.creator } + let(:other_family_membership) do + create(:family_membership, :owner, family: other_family, user: other_family_owner) + end + + before do + allow(owner).to receive(:family).and_return(family) + allow(owner).to receive(:family_owner?).and_return(true) + end + + it 'denies owner from viewing membership of different family' do + policy = Family::MembershipPolicy.new(owner, other_family_membership) + + expect(policy).not_to permit(:show) + end + + it 'denies owner from updating membership of different family' do + policy = Family::MembershipPolicy.new(owner, other_family_membership) + + expect(policy).not_to permit(:update) + end + + it 'denies owner from destroying membership of different family' do + policy = Family::MembershipPolicy.new(owner, other_family_membership) + + expect(policy).not_to permit(:destroy) + end + end + + context 'when owner tries to modify another owners membership' do + let(:co_owner) { create(:user) } + let(:co_owner_membership) { create(:family_membership, :owner, family: family, user: co_owner) } + + before do + allow(owner).to receive(:family).and_return(family) + allow(owner).to receive(:family_owner?).and_return(true) + end + + it 'allows owner to view another owner' do + policy = Family::MembershipPolicy.new(owner, co_owner_membership) + + expect(policy).to permit(:show) + end + + it 'allows owner to update another owner (family owner has full control)' do + policy = Family::MembershipPolicy.new(owner, co_owner_membership) + + expect(policy).to permit(:update) + end + + it 'allows owner to remove another owner (family owner has full control)' do + policy = Family::MembershipPolicy.new(owner, co_owner_membership) + + expect(policy).to permit(:destroy) + end + end + end + + describe 'authorization consistency' do + it 'ensures owner can view, update, and destroy all memberships in their family' do + allow(owner).to receive(:family).and_return(family) + allow(owner).to receive(:family_owner?).and_return(true) + + policy = Family::MembershipPolicy.new(owner, member_membership) + + expect(policy).to permit(:show) + expect(policy).to permit(:update) + expect(policy).to permit(:destroy) + end + + it 'ensures regular members can only manage their own membership' do + allow(member).to receive(:family).and_return(family) + allow(member).to receive(:family_owner?).and_return(false) + + own_policy = Family::MembershipPolicy.new(member, member_membership) + other_policy = Family::MembershipPolicy.new(member, another_member_membership) + + # Can manage own membership + expect(own_policy).to permit(:show) + expect(own_policy).to permit(:update) + expect(own_policy).to permit(:destroy) + + # Can view but not manage others + expect(other_policy).to permit(:show) + expect(other_policy).not_to permit(:update) + expect(other_policy).not_to permit(:destroy) + end + + it 'ensures users can always leave the family (remove own membership)' do + allow(member).to receive(:family).and_return(family) + policy = Family::MembershipPolicy.new(member, member_membership) + + expect(policy).to permit(:destroy) + end + end +end