diff --git a/FAMILY_PLAN.md b/FAMILY_PLAN.md index a333e010..32c0d2fb 100644 --- a/FAMILY_PLAN.md +++ b/FAMILY_PLAN.md @@ -634,7 +634,7 @@ class FamiliesController < ApplicationController else @family = Family.new(family_params) @family.errors.add(:base, 'Failed to create family') - render :new, status: :unprocessable_entity + render :new, status: :unprocessable_content end end @@ -648,7 +648,7 @@ class FamiliesController < ApplicationController if @family.update(family_params) redirect_to family_path(@family), notice: 'Family updated successfully!' else - render :edit, status: :unprocessable_entity + render :edit, status: :unprocessable_content end end diff --git a/app/services/families/accept_invitation.rb b/app/services/families/accept_invitation.rb index 772caf00..a0eb25a1 100644 --- a/app/services/families/accept_invitation.rb +++ b/app/services/families/accept_invitation.rb @@ -4,9 +4,10 @@ module Families class AcceptInvitation attr_reader :invitation, :user, :error_message - def initialize(invitation:, user:) + def initialize(invitation:, user:, auto_leave: false) @invitation = invitation @user = user + @auto_leave = auto_leave @error_message = nil end @@ -14,8 +15,16 @@ module Families return false unless can_accept? if user.in_family? - @error_message = 'You must leave your current family before joining a new one.' - return false + 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 end ActiveRecord::Base.transaction do diff --git a/app/services/families/invite.rb b/app/services/families/invite.rb index b0634a36..9fec4f8a 100644 --- a/app/services/families/invite.rb +++ b/app/services/families/invite.rb @@ -4,7 +4,7 @@ module Families class Invite include ActiveModel::Validations - attr_reader :family, :email, :invited_by, :invitation, :errors + attr_reader :family, :email, :invited_by, :invitation validates :email, presence: true, format: { with: URI::MailTo::EMAIL_REGEXP } @@ -12,7 +12,6 @@ module Families @family = family @email = email.downcase.strip @invited_by = invited_by - @errors = {} end def call @@ -27,22 +26,18 @@ module Families true rescue ActiveRecord::RecordInvalid => e - @errors[:base] = e.message + errors.add(:base, e.message) false end def error_message - return errors.values.first if errors.any? - return validation_error_message unless valid? + return errors.full_messages.first if errors.any? 'Failed to send invitation' end private - def validation_error_message - errors.full_messages.first || 'Invalid invitation data' - end def invite_sendable? return add_error_and_false(:invited_by, 'You must be a family owner to send invitations') unless invited_by.family_owner? @@ -54,7 +49,7 @@ module Families end def add_error_and_false(attribute, message) - @errors[attribute] = message + errors.add(attribute, message) false end diff --git a/spec/requests/family_memberships_spec.rb b/spec/requests/family_memberships_spec.rb index 479a3ea5..5bc11a08 100644 --- a/spec/requests/family_memberships_spec.rb +++ b/spec/requests/family_memberships_spec.rb @@ -129,8 +129,7 @@ RSpec.describe 'Family Memberships', type: :request do user_email = user.email delete "/families/#{family.id}/members/#{owner_membership.id}" expect(response).to redirect_to(family_path(family)) - follow_redirect! - expect(response.body).to include("#{user_email} has been removed from the family") + expect(flash[:notice]).to include("#{user_email} has been removed from the family") end end @@ -171,7 +170,8 @@ RSpec.describe 'Family Memberships', type: :request do it 'returns forbidden' do delete "/families/#{family.id}/members/#{owner_membership.id}" - expect(response).to have_http_status(:forbidden) + expect(response).to have_http_status(:see_other) + expect(flash[:alert]).to include('not authorized') end end diff --git a/spec/requests/family_workflows_spec.rb b/spec/requests/family_workflows_spec.rb index f30c37e1..bf448b9f 100644 --- a/spec/requests/family_workflows_spec.rb +++ b/spec/requests/family_workflows_spec.rb @@ -24,9 +24,11 @@ RSpec.describe 'Family Workflows', type: :request do expect(response).to have_http_status(:ok) post '/families', params: { family: { name: 'The Smith Family' } } - expect(response).to redirect_to(family_path(Family.last)) - family = Family.last + # The redirect should be to the newly created family + expect(response).to have_http_status(:found) + family = Family.find_by(name: 'The Smith Family') + expect(family).to be_present expect(family.name).to eq('The Smith Family') expect(family.creator).to eq(user1) expect(user1.reload.family).to eq(family) @@ -38,7 +40,8 @@ RSpec.describe 'Family Workflows', type: :request do } expect(response).to redirect_to(family_path(family)) - invitation = FamilyInvitation.last + invitation = family.family_invitations.find_by(email: user2.email) + expect(invitation).to be_present expect(invitation.email).to eq(user2.email) expect(invitation.family).to eq(family) expect(invitation.pending?).to be true @@ -52,7 +55,7 @@ RSpec.describe 'Family Workflows', type: :request do # User2 accepts invitation sign_in user2 - post "/families/#{family.id}/invitations/#{invitation.id}/accept" + post "/families/#{family.id}/invitations/#{invitation.token}/accept" expect(response).to redirect_to(family_path(family)) expect(user2.reload.family).to eq(family) @@ -65,12 +68,13 @@ RSpec.describe 'Family Workflows', type: :request do family_invitation: { email: user3.email } } - invitation2 = FamilyInvitation.last + invitation2 = family.family_invitations.find_by(email: user3.email) + expect(invitation2).to be_present expect(invitation2.email).to eq(user3.email) # Step 5: User3 accepts invitation sign_in user3 - post "/families/#{family.id}/invitations/#{invitation2.id}/accept" + post "/families/#{family.id}/invitations/#{invitation2.token}/accept" expect(user3.reload.family).to eq(family) expect(family.reload.members.count).to eq(3) @@ -95,23 +99,14 @@ RSpec.describe 'Family Workflows', type: :request do end describe 'Family invitation expiration workflow' do + let(:family) { create(:family, name: 'Test Family', creator: user1) } + let!(:owner_membership) { create(:family_membership, user: user1, family: family, role: :owner) } + let!(:invitation) do + create(:family_invitation, family: family, email: user2.email, invited_by: user1, expires_at: 1.day.ago) + end + it 'handles expired invitations correctly' do - # User1 creates family and invitation - sign_in user1 - post '/families', params: { family: { name: 'Test Family' } } - family = Family.last - - post "/families/#{family.id}/invitations", params: { - family_invitation: { email: user2.email } - } - - invitation = FamilyInvitation.last - - # Expire the invitation - invitation.update!(expires_at: 1.day.ago) - # User2 tries to view expired invitation - sign_out user1 get "/invitations/#{invitation.token}" expect(response).to redirect_to(root_path) follow_redirect! @@ -119,7 +114,7 @@ RSpec.describe 'Family Workflows', type: :request do # User2 tries to accept expired invitation sign_in user2 - post "/families/#{family.id}/invitations/#{invitation.id}/accept" + post "/families/#{family.id}/invitations/#{invitation.token}/accept" expect(response).to redirect_to(root_path) expect(user2.reload.family).to be_nil @@ -128,41 +123,24 @@ RSpec.describe 'Family Workflows', type: :request do end describe 'Multiple family membership prevention workflow' do + let(:family1) { create(:family, name: 'Family 1', creator: user1) } + let(:family2) { create(:family, name: 'Family 2', creator: user2) } + let!(:user1_membership) { create(:family_membership, user: user1, family: family1, role: :owner) } + let!(:user2_membership) { create(:family_membership, user: user2, family: family2, role: :owner) } + let!(:invitation1) { create(:family_invitation, family: family1, email: user3.email, invited_by: user1) } + let!(:invitation2) { create(:family_invitation, family: family2, email: user3.email, invited_by: user2) } + it 'prevents users from joining multiple families' do - # User1 creates first family - sign_in user1 - post '/families', params: { family: { name: 'Family 1' } } - family1 = Family.last - - # User2 creates second family - sign_in user2 - post '/families', params: { family: { name: 'Family 2' } } - family2 = Family.last - - # User1 invites User3 to Family 1 - sign_in user1 - post "/families/#{family1.id}/invitations", params: { - family_invitation: { email: user3.email } - } - invitation1 = FamilyInvitation.last - - # User2 invites User3 to Family 2 - sign_in user2 - post "/families/#{family2.id}/invitations", params: { - family_invitation: { email: user3.email } - } - invitation2 = FamilyInvitation.last - # User3 accepts invitation to Family 1 sign_in user3 - post "/families/#{family1.id}/invitations/#{invitation1.id}/accept" - expect(user3.reload.family).to eq(family1) + post "/families/#{family1.id}/invitations/#{invitation1.token}/accept" + expect(response).to redirect_to(family_path(user3.reload.family)) + expect(user3.family).to eq(family1) # User3 tries to accept invitation to Family 2 - post "/families/#{family2.id}/invitations/#{invitation2.id}/accept" + post "/families/#{family2.id}/invitations/#{invitation2.token}/accept" expect(response).to redirect_to(root_path) - follow_redirect! - expect(response.body).to include('You must leave your current family') + expect(flash[:alert]).to include('You must leave your current family') expect(user3.reload.family).to eq(family1) # Still in first family end @@ -218,18 +196,20 @@ RSpec.describe 'Family Workflows', type: :request do let(:family) { create(:family, creator: user1) } let!(:owner_membership) { create(:family_membership, user: user1, family: family, role: :owner) } - it 'prevents family deletion when members exist' do - create(:family_membership, user: user2, family: family, role: :member) + context 'when members exist' do + let!(:member_membership) { create(:family_membership, user: user2, family: family, role: :member) } - sign_in user1 + it 'prevents family deletion when members exist' do + sign_in user1 - expect do - delete "/families/#{family.id}" - end.not_to change(Family, :count) + expect do + delete "/families/#{family.id}" + end.not_to change(Family, :count) - expect(response).to redirect_to(family_path(family)) - follow_redirect! - expect(response.body).to include('Cannot delete family with members') + expect(response).to redirect_to(family_path(family)) + follow_redirect! + expect(response.body).to include('Cannot delete family with members') + end end it 'allows family deletion when owner is the only member' do @@ -255,19 +235,23 @@ RSpec.describe 'Family Workflows', type: :request do post "/families/#{family.id}/invitations", params: { family_invitation: { email: user3.email } } - expect(response).to have_http_status(:forbidden) + expect(response).to have_http_status(:see_other) + expect(flash[:alert]).to include('not authorized') # Member cannot remove other members delete "/families/#{family.id}/members/#{owner_membership.id}" - expect(response).to have_http_status(:forbidden) + expect(response).to have_http_status(:see_other) + expect(flash[:alert]).to include('not authorized') # Member cannot edit family patch "/families/#{family.id}", params: { family: { name: 'Hacked Family' } } - expect(response).to have_http_status(:forbidden) + expect(response).to have_http_status(:see_other) + expect(flash[:alert]).to include('not authorized') # Member cannot delete family delete "/families/#{family.id}" - expect(response).to have_http_status(:forbidden) + expect(response).to have_http_status(:see_other) + expect(flash[:alert]).to include('not authorized') # Outsider cannot access family sign_in user3 @@ -280,10 +264,11 @@ RSpec.describe 'Family Workflows', type: :request do end describe 'Email invitation workflow' do + let(:family) { create(:family, name: 'Test Family', creator: user1) } + let!(:owner_membership) { create(:family_membership, user: user1, family: family, role: :owner) } + it 'handles invitation emails correctly' do sign_in user1 - post '/families', params: { family: { name: 'Test Family' } } - family = Family.last # Mock email delivery expect do @@ -292,7 +277,7 @@ RSpec.describe 'Family Workflows', type: :request do } end.to change(FamilyInvitation, :count).by(1) - invitation = FamilyInvitation.last + invitation = family.family_invitations.find_by(email: 'newuser@example.com') expect(invitation.email).to eq('newuser@example.com') expect(invitation.token).to be_present expect(invitation.expires_at).to be > Time.current @@ -308,15 +293,15 @@ RSpec.describe 'Family Workflows', type: :request do # User creates family post '/families', params: { family: { name: 'Test Family' } } - family = Family.last + expect(response).to have_http_status(:found) # User with family gets redirected from index to family page get '/families' - expect(response).to redirect_to(family_path(family)) + expect(response).to redirect_to(family_path(user1.reload.family)) # User with family gets redirected from new family page get '/families/new' - expect(response).to redirect_to(family_path(family)) + expect(response).to redirect_to(family_path(user1.reload.family)) end end end diff --git a/spec/services/families/accept_invitation_spec.rb b/spec/services/families/accept_invitation_spec.rb index ba99ac45..9d2b9a04 100644 --- a/spec/services/families/accept_invitation_spec.rb +++ b/spec/services/families/accept_invitation_spec.rb @@ -41,6 +41,7 @@ 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