From e17f732706fcd0880f5296267e3e258a4b2a7a1c Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Sat, 27 Sep 2025 21:14:40 +0200 Subject: [PATCH] Minor changes --- app/controllers/families_controller.rb | 21 ++++++------ .../family_invitations_controller.rb | 6 ++-- app/mailers/family_mailer.rb | 2 +- app/models/user.rb | 2 +- app/services/families/accept_invitation.rb | 15 ++------- app/services/families/create.rb | 5 +++ app/services/families/invite.rb | 8 +++-- app/services/families/leave.rb | 13 ++++---- spec/models/user_family_spec.rb | 2 +- .../families/accept_invitation_spec.rb | 18 ++++++++--- spec/services/families/create_spec.rb | 32 +++++++++++++++++++ spec/services/families/invite_spec.rb | 11 +++++-- spec/services/families/leave_spec.rb | 9 ++++-- 13 files changed, 97 insertions(+), 47 deletions(-) diff --git a/app/controllers/families_controller.rb b/app/controllers/families_controller.rb index ce3c6cf4..d6310a26 100644 --- a/app/controllers/families_controller.rb +++ b/app/controllers/families_controller.rb @@ -65,19 +65,20 @@ class FamiliesController < ApplicationController end def leave - authorize @family, :leave? + authorize @family, :leave? - service = Families::Leave.new(user: current_user) + service = Families::Leave.new(user: current_user) - if service.call - redirect_to families_path, notice: 'You have left the family' - else - redirect_to family_path(@family), alert: service.error_message || 'Cannot leave family.' + if service.call + redirect_to families_path, notice: 'You have left the family' + else + redirect_to family_path(@family), alert: service.error_message || 'Cannot leave family.' + end + rescue Pundit::NotAuthorizedError + # Handle case where owner with members tries to leave + redirect_to family_path(@family), + alert: 'You cannot leave the family while you are the owner and there are other members. Remove all members first or transfer ownership.' end -rescue Pundit::NotAuthorizedError - # Handle case where owner with members tries to leave - redirect_to family_path(@family), alert: 'You cannot leave the family while you are the owner and there are other members. Remove all members first or transfer ownership.' -end private diff --git a/app/controllers/family_invitations_controller.rb b/app/controllers/family_invitations_controller.rb index b6d5a716..cbe559d2 100644 --- a/app/controllers/family_invitations_controller.rb +++ b/app/controllers/family_invitations_controller.rb @@ -49,8 +49,7 @@ class FamilyInvitationsController < ApplicationController ) if service.call - current_user.reload - redirect_to family_path(current_user.family), notice: 'Welcome to the family!' + redirect_to family_path(current_user.reload.family), notice: 'Welcome to the family!' else redirect_to root_path, alert: service.error_message || 'Unable to accept invitation' end @@ -67,7 +66,8 @@ class FamilyInvitationsController < ApplicationController def set_family @family = current_user.family - redirect_to families_path unless @family + + redirect_to families_path, alert: 'Family not found' and return unless @family end def set_invitation diff --git a/app/mailers/family_mailer.rb b/app/mailers/family_mailer.rb index 02b39953..0b6d4023 100644 --- a/app/mailers/family_mailer.rb +++ b/app/mailers/family_mailer.rb @@ -12,4 +12,4 @@ class FamilyMailer < ApplicationMailer subject: "You've been invited to join #{@family.name} on Dawarich" ) end -end \ No newline at end of file +end diff --git a/app/models/user.rb b/app/models/user.rb index e9d9b71b..a05a4026 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -18,7 +18,7 @@ class User < ApplicationRecord # rubocop:disable Metrics/ClassLength # Family associations has_one :family_membership, dependent: :destroy has_one :family, through: :family_membership - has_many :created_families, class_name: 'Family', foreign_key: 'creator_id', inverse_of: :creator, dependent: :destroy + 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 diff --git a/app/services/families/accept_invitation.rb b/app/services/families/accept_invitation.rb index a0eb25a1..772caf00 100644 --- a/app/services/families/accept_invitation.rb +++ b/app/services/families/accept_invitation.rb @@ -4,10 +4,9 @@ module Families class AcceptInvitation attr_reader :invitation, :user, :error_message - def initialize(invitation:, user:, auto_leave: false) + def initialize(invitation:, user:) @invitation = invitation @user = user - @auto_leave = auto_leave @error_message = nil end @@ -15,16 +14,8 @@ module Families return false unless can_accept? if user.in_family? - if @auto_leave - leave_service = Families::Leave.new(user: user) - unless leave_service.call - @error_message = leave_service.error_message || 'Failed to leave current family.' - return false - end - else - @error_message = 'You must leave your current family before joining a new one.' - return false - end + @error_message = 'You must leave your current family before joining a new one.' + return false end ActiveRecord::Base.transaction do diff --git a/app/services/families/create.rb b/app/services/families/create.rb index ad62c155..0bfc3052 100644 --- a/app/services/families/create.rb +++ b/app/services/families/create.rb @@ -16,6 +16,11 @@ module Families return false end + if user.created_family.present? + @errors[:user] = 'User has already created a family' + return false + end + unless can_create_family? @errors[:base] = 'Cannot create family' return false diff --git a/app/services/families/invite.rb b/app/services/families/invite.rb index 9fec4f8a..ad7c541b 100644 --- a/app/services/families/invite.rb +++ b/app/services/families/invite.rb @@ -38,9 +38,11 @@ module Families private - def invite_sendable? - return add_error_and_false(:invited_by, 'You must be a family owner to send invitations') unless invited_by.family_owner? + unless invited_by.family_owner? + return add_error_and_false(:invited_by, + 'You must be a family owner to send invitations') + end return add_error_and_false(:family, 'Family is full') if family.members.count >= Family::MAX_MEMBERS return add_error_and_false(:email, 'User is already in a family') if user_already_in_family? return add_error_and_false(:email, 'Invitation already sent to this email') if pending_invitation_exists? @@ -84,4 +86,4 @@ module Families ) end end -end \ No newline at end of file +end diff --git a/app/services/families/leave.rb b/app/services/families/leave.rb index a7493d3c..06b5108c 100644 --- a/app/services/families/leave.rb +++ b/app/services/families/leave.rb @@ -53,12 +53,13 @@ module Families end def handle_ownership_transfer - # If this is the last member (owner), delete the family - if user.family.members.count == 1 - user.family.destroy! - end - # If owner tries to leave with other members, it should be prevented in validation -end + # If this is the last member (owner), delete the family + return unless user.family.members.count == 1 + + user.family.destroy! + + # If owner tries to leave with other members, it should be prevented in validation + end def remove_membership user.family_membership.destroy! diff --git a/spec/models/user_family_spec.rb b/spec/models/user_family_spec.rb index bdcea5c8..9afacb1c 100644 --- a/spec/models/user_family_spec.rb +++ b/spec/models/user_family_spec.rb @@ -9,7 +9,7 @@ RSpec.describe User, 'family methods', type: :model do it { is_expected.to have_one(:family_membership).dependent(:destroy) } it { is_expected.to have_one(:family).through(:family_membership) } it { - is_expected.to have_many(:created_families).class_name('Family').with_foreign_key('creator_id').dependent(:destroy) + is_expected.to have_one(:created_family).class_name('Family').with_foreign_key('creator_id').dependent(:destroy) } it { is_expected.to have_many(:sent_family_invitations).class_name('FamilyInvitation').with_foreign_key('invited_by_id').dependent(:destroy) diff --git a/spec/services/families/accept_invitation_spec.rb b/spec/services/families/accept_invitation_spec.rb index 9d2b9a04..80492e6c 100644 --- a/spec/services/families/accept_invitation_spec.rb +++ b/spec/services/families/accept_invitation_spec.rb @@ -41,14 +41,22 @@ RSpec.describe Families::AcceptInvitation do context 'when user is already in another family' do let(:other_family) { create(:family) } let!(:existing_membership) { create(:family_membership, user: invitee, family: other_family) } - let(:service) { described_class.new(invitation: invitation, user: invitee, auto_leave: true) } - it 'leaves current family before joining new one' do - expect(Families::Leave).to receive(:new).with(user: invitee).and_call_original + it 'returns false' do + expect(service.call).to be false + end + + it 'does not create membership' do + expect { service.call }.not_to change(FamilyMembership, :count) + end + + it 'sets appropriate error message' do service.call + expect(service.error_message).to eq('You must leave your current family before joining a new one.') + end - # Should have new membership in the invited family - expect(invitee.reload.family).to eq(family) + it 'does not change user family' do + expect { service.call }.not_to(change { invitee.reload.family }) end end diff --git a/spec/services/families/create_spec.rb b/spec/services/families/create_spec.rb index f4e9b94a..e7c9e96e 100644 --- a/spec/services/families/create_spec.rb +++ b/spec/services/families/create_spec.rb @@ -40,6 +40,38 @@ RSpec.describe Families::Create do it 'does not create a membership' do expect { service.call }.not_to change(FamilyMembership, :count) end + + it 'sets appropriate error message' do + service.call + expect(service.errors[:user]).to eq('User is already in a family') + end + end + + context 'when user has already created a family before' do + before do + # User creates and then deletes their family membership, but family still exists + old_family = create(:family, creator: user) + membership = create(:family_membership, user: user, family: old_family, role: :owner) + membership.destroy! # User leaves the family but family still exists + user.reload # Ensure user association is refreshed + end + + it 'returns false' do + expect(service.call).to be false + end + + it 'does not create a family' do + expect { service.call }.not_to change(Family, :count) + end + + it 'does not create a membership' do + expect { service.call }.not_to change(FamilyMembership, :count) + end + + it 'sets appropriate error message' do + service.call + expect(service.errors[:user]).to eq('User has already created a family') + end end end end diff --git a/spec/services/families/invite_spec.rb b/spec/services/families/invite_spec.rb index efa3aaa3..3367efd3 100644 --- a/spec/services/families/invite_spec.rb +++ b/spec/services/families/invite_spec.rb @@ -13,7 +13,9 @@ RSpec.describe Families::Invite do context 'when invitation is valid' do it 'creates an invitation' do expect { service.call }.to change(FamilyInvitation, :count).by(1) - invitation = FamilyInvitation.last + + invitation = owner.sent_family_invitations.last + expect(invitation.family).to eq(family) expect(invitation.email).to eq(email) expect(invitation.invited_by).to eq(owner) @@ -27,7 +29,9 @@ RSpec.describe Families::Invite do it 'sends notification to inviter' do expect { service.call }.to change(Notification, :count).by(1) - notification = Notification.last + + notification = owner.notifications.last + expect(notification.user).to eq(owner) expect(notification.title).to eq('Invitation Sent') end @@ -116,7 +120,8 @@ RSpec.describe Families::Invite do it 'normalizes email to lowercase and strips whitespace' do service.call - invitation = FamilyInvitation.last + invitation = family.family_invitations.last + expect(invitation.email).to eq('upper@example.com') end end diff --git a/spec/services/families/leave_spec.rb b/spec/services/families/leave_spec.rb index 306f6532..fe016bfb 100644 --- a/spec/services/families/leave_spec.rb +++ b/spec/services/families/leave_spec.rb @@ -20,8 +20,9 @@ RSpec.describe Families::Leave do it 'sends notification' do expect { service.call }.to change(Notification, :count).by(1) - notification = Notification.last - expect(notification.user).to eq(member) + + notification = member.notifications.last + expect(notification.title).to eq('Left Family') end @@ -38,6 +39,10 @@ RSpec.describe Families::Leave do expect(user.reload.family_membership).to be_nil end + it 'deletes the family' do + expect { service.call }.to change(Family, :count).by(-1) + end + it 'returns true' do expect(service.call).to be true end