diff --git a/app/controllers/workshops_controller.rb b/app/controllers/workshops_controller.rb index a11ac7cce..1cb23df2e 100644 --- a/app/controllers/workshops_controller.rb +++ b/app/controllers/workshops_controller.rb @@ -44,10 +44,13 @@ def find_attending_invitation(workshop, user) end def find_or_create_invitation(workshop, user, role) - invitation = WorkshopInvitation.create_or_find_by(workshop:, - member: user, - role:) - invitation.persisted? ? invitation : WorkshopInvitation.find_by(workshop:, member: user, role:) + # Identity is workshop + member, matching InvitationManager; the member's + # role choice wins, so an existing invitation with the other role is updated. + invitation = WorkshopInvitation.find_or_create_by!(workshop:, member: user) { |record| record.role = role } + invitation.update!(role:) unless invitation.role.eql?(role) + invitation + rescue ActiveRecord::RecordNotUnique + WorkshopInvitation.find_by(workshop:, member: user) end def user_attending_or_waitlisted?(workshop, user) diff --git a/app/mailers/event_invitation_mailer.rb b/app/mailers/event_invitation_mailer.rb index aaf22740f..b1c4b5177 100644 --- a/app/mailers/event_invitation_mailer.rb +++ b/app/mailers/event_invitation_mailer.rb @@ -20,7 +20,9 @@ def invite_coach(event, member, invitation) @member = member @invitation = invitation @host_address = AddressPresenter.new(@event.venue.address) if @event.venue.present? - @everyone_is_invited = !event.audience + # Coach emails are labelled as such only when the event is actually for + # coaches; blank or missing audience means a general invitation. + @everyone_is_invited = !event.audience.eql?('Coaches') mail_to_member(member, @everyone_is_invited ? "Invitation: #{@event.name}" : "Coach Invitation: #{@event.name}", &:html) diff --git a/app/services/invitation_manager.rb b/app/services/invitation_manager.rb index b4b39d58d..6a0f42c91 100644 --- a/app/services/invitation_manager.rb +++ b/app/services/invitation_manager.rb @@ -164,14 +164,16 @@ def chapter_coaches(chapter) end def create_invitation(workshop, member, role) - WorkshopInvitation.find_or_create_by!(workshop:, member:, role:) + # Identity is workshop + member; role applies only on create so a member + # subscribed as both student and coach gets one invite, not one per role. + WorkshopInvitation.find_or_create_by!(workshop:, member:) { |invitation| invitation.role = role } rescue StandardError => e log_invitation_failure(workshop, member, role, e) nil end def create_event_invitation(event, member, role) - Invitation.find_or_create_by!(event:, member:, role:) + Invitation.find_or_create_by!(event:, member:) { |invitation| invitation.role = role } rescue StandardError => e log_event_meeting_invitation_failure("event_id=#{event.id}", member, e) nil diff --git a/spec/controllers/workshops_controller_spec.rb b/spec/controllers/workshops_controller_spec.rb index d6421424e..79f7694fd 100644 --- a/spec/controllers/workshops_controller_spec.rb +++ b/spec/controllers/workshops_controller_spec.rb @@ -25,6 +25,21 @@ end end + context 'when the member has an invitation with a different role' do + let!(:invitation) do + Fabricate(:workshop_invitation, workshop:, member:, role: 'Student', attending: nil) + end + + it 'updates the existing invitation to the requested role instead of creating a second one' do + expect do + post :rsvp, params: { id: workshop.id, role: 'Coach' } + end.not_to change(WorkshopInvitation, :count) + + expect(invitation.reload.role).to eq('Coach') + expect(response).to redirect_to(invitation_path(invitation)) + end + end + context 'when the member does not have an invitation for the workshop and role' do it 'creates a new invitation and redirects' do expect do diff --git a/spec/mailers/event_invitation_mailer_spec.rb b/spec/mailers/event_invitation_mailer_spec.rb index bf34c61c7..d8c407d03 100644 --- a/spec/mailers/event_invitation_mailer_spec.rb +++ b/spec/mailers/event_invitation_mailer_spec.rb @@ -52,6 +52,15 @@ expect(email.body.encoded).to match('hello@codebar.io') end + it 'sends a generic invitation if the event audience is blank' do + blank_audience_event = Fabricate(:event, name: 'Test event', audience: '') + blank_invitation = Fabricate(:invitation, event: blank_audience_event, member:) + + described_class.invite_coach(blank_audience_event, member, blank_invitation).deliver_now + + expect(email.subject).to eq("Invitation: #{blank_audience_event.name}") + end + it 'sends a coach invitation of the event is for coaches' do email_subject = "Coach Invitation: #{event.name}" described_class.invite_coach(coach_event, member, invitation).deliver_now diff --git a/spec/services/invitation_manager_spec.rb b/spec/services/invitation_manager_spec.rb index d352421aa..dbea8e83b 100644 --- a/spec/services/invitation_manager_spec.rb +++ b/spec/services/invitation_manager_spec.rb @@ -30,67 +30,54 @@ it 'can email only students' do event = Fabricate(:event, chapters: [chapter], audience: 'Students') - students.each do |student| - allow(Invitation).to receive(:find_or_create_by!).with( - event:, member: student, role: 'Student' - ).and_call_original - end - - manager.send_event_emails(event, chapter) - students.each do |student| - expect(Invitation).to have_received(:find_or_create_by!).with(event:, member: student, role: 'Student') - end + expect do + manager.send_event_emails(event, chapter) + end.to change(Invitation, :count).by(students.count) - coaches.each do |student| - expect(Invitation).not_to have_received(:find_or_create_by!).with(event:, member: student, role: 'Coach') - end + expect(event.invitations.students.map(&:member)).to match_array(students) + expect(event.invitations.coaches).to be_empty end it 'can email only coaches' do event = Fabricate(:event, chapters: [chapter], audience: 'Coaches') - coaches.each do |student| - allow(Invitation).to receive(:find_or_create_by!).with( - event:, member: student, role: 'Coach' - ).and_call_original - end - - manager.send_event_emails(event, chapter) - - students.each do |student| - expect(Invitation).not_to have_received(:find_or_create_by!).with(event:, member: student, role: 'Student') - end + expect do + manager.send_event_emails(event, chapter) + end.to change(Invitation, :count).by(coaches.count) - coaches.each do |student| - expect(Invitation).to have_received(:find_or_create_by!).with(event:, member: student, role: 'Coach') - end + expect(event.invitations.coaches.map(&:member)).to match_array(coaches) + expect(event.invitations.students).to be_empty end it 'can email both students and coaches' do event = Fabricate(:event, chapters: [chapter]) - students.each do |student| - allow(Invitation).to receive(:find_or_create_by!).with( - event:, member: student, role: 'Student' - ).and_call_original - end + expect do + manager.send_event_emails(event, chapter) + end.to change(Invitation, :count).by(students.count + coaches.count) - coaches.each do |student| - allow(Invitation).to receive(:find_or_create_by!).with( - event:, member: student, role: 'Coach' - ).and_call_original - end + expect(event.invitations.students.map(&:member)).to match_array(students) + expect(event.invitations.coaches.map(&:member)).to match_array(coaches) + end - manager.send_event_emails(event, chapter) + it 'sends one invitation to a member subscribed as both student and coach' do + dual_member = Fabricate(:member) + Fabricate(:students, chapter:, members: [dual_member]) + Fabricate(:coaches, chapter:, members: [dual_member]) + event = Fabricate(:event, chapters: [chapter]) - students.each do |student| - expect(Invitation).to have_received(:find_or_create_by!).with(event:, member: student, role: 'Student') - end + expect do + manager.send_event_emails(event, chapter) + end.to change { Invitation.where(event:, member: dual_member).count }.by(1) - coaches.each do |student| - expect(Invitation).to have_received(:find_or_create_by!).with(event:, member: student, role: 'Coach') - end + invitation = Invitation.find_by(event:, member: dual_member) + # Coaches are invited first for events, so the coach pass wins for dual members. + expect(invitation.role).to eq('Coach') + + deliveries_to_dual_member = ActionMailer::Base.deliveries.select { |e| e.to.include?(dual_member.email) } + expect(deliveries_to_dual_member.count).to eq(1) + expect(deliveries_to_dual_member.first.subject).to eq("Invitation: #{event.name}") end it 'emails only students that accepted toc' do @@ -99,21 +86,11 @@ first_student, *other_students = students first_student.update(accepted_toc_at: nil) - other_students.each do |other_student| - allow(Invitation).to( - receive(:find_or_create_by!) - .with(event:, member: other_student, role: 'Student') - .and_call_original - ) - end - - manager.send_event_emails(event, chapter) - - expect(Invitation).not_to have_received(:find_or_create_by!).with(event:, member: first_student, role: 'Student') + expect do + manager.send_event_emails(event, chapter) + end.to change(Invitation, :count).by(other_students.count) - other_students.each do |other_student| - expect(Invitation).to have_received(:find_or_create_by!).with(event:, member: other_student, role: 'Student') - end + expect(event.invitations.students.map(&:member)).to match_array(other_students) end it 'emails only coaches that accepted toc' do @@ -122,21 +99,11 @@ first_coach, *other_coaches = coaches first_coach.update(accepted_toc_at: nil) - other_coaches.each do |other_coach| - allow(Invitation).to( - receive(:find_or_create_by!) - .with(event:, member: other_coach, role: 'Coach') - .and_call_original - ) - end - - manager.send_event_emails(event, chapter) - - expect(Invitation).not_to have_received(:find_or_create_by!).with(event:, member: first_coach, role: 'Coach') + expect do + manager.send_event_emails(event, chapter) + end.to change(Invitation, :count).by(other_coaches.count) - other_coaches.each do |other_coach| - expect(Invitation).to have_received(:find_or_create_by!).with(event:, member: other_coach, role: 'Coach') - end + expect(event.invitations.coaches.map(&:member)).to match_array(other_coaches) end end @@ -279,6 +246,16 @@ expect(invitation.role).to eq('Student') end + it 'returns existing invitation with previously_new_record? as false when called with a different role' do + invitation1 = manager.send(:create_invitation, workshop, member, 'Student') + + invitation2 = manager.send(:create_invitation, workshop, member, 'Coach') + + expect(invitation2.previously_new_record?).to be false + expect(invitation2.id).to eq(invitation1.id) + expect(invitation2.role).to eq('Student') + end + it 'returns existing invitation with previously_new_record? as false on duplicate call' do # First call creates the invitation invitation1 = manager.send(:create_invitation, workshop, member, 'Student') @@ -462,17 +439,16 @@ coaches_group.members << member_in_both_groups end - it 'creates one invitation per role when audience is everyone' do + it 'sends one invitation and one email when audience is everyone' do expect do manager.send_workshop_emails(workshop, 'everyone') - end.to change(WorkshopInvitation, :count).by(2) - - student_invitation = WorkshopInvitation.find_by(workshop:, member: member_in_both_groups, role: 'Student') - coach_invitation = WorkshopInvitation.find_by(workshop:, member: member_in_both_groups, role: 'Coach') + end.to change(WorkshopInvitation, :count).by(1) + .and change { ActionMailer::Base.deliveries.count }.by(1) - expect(student_invitation).to be_present - expect(coach_invitation).to be_present - expect(student_invitation.id).not_to eq(coach_invitation.id) + invitation = WorkshopInvitation.find_by(workshop:, member: member_in_both_groups) + # Students are invited first for workshops, so the student pass wins for dual members. + expect(invitation.role).to eq('Student') + expect(ActionMailer::Base.deliveries.last.subject).to start_with('Workshop Invitation') end end end diff --git a/spec/support/shared_examples/behaves_like_sending_workshop_emails.rb b/spec/support/shared_examples/behaves_like_sending_workshop_emails.rb index ef1907f99..38c810634 100644 --- a/spec/support/shared_examples/behaves_like_sending_workshop_emails.rb +++ b/spec/support/shared_examples/behaves_like_sending_workshop_emails.rb @@ -3,7 +3,7 @@ Fabricate(:students, chapter:, members: students) students.each do |student| - allow(WorkshopInvitation).to receive(:find_or_create_by!).with(workshop:, member: student, role: 'Student').and_call_original + allow(WorkshopInvitation).to receive(:find_or_create_by!).with(workshop:, member: student).and_call_original end expect do @@ -12,7 +12,7 @@ .and change { WorkshopInvitation.where(workshop:, role: 'Student').count }.by(students.count) students.each do |student| - expect(WorkshopInvitation).to have_received(:find_or_create_by!).with(workshop:, member: student, role: 'Student') + expect(WorkshopInvitation).to have_received(:find_or_create_by!).with(workshop:, member: student) end # Verify emails were sent to the right recipients @@ -25,7 +25,7 @@ Fabricate(:coaches, chapter:, members: coaches) coaches.each do |coach| - allow(WorkshopInvitation).to receive(:find_or_create_by!).with(workshop:, member: coach, role: 'Coach').and_call_original + allow(WorkshopInvitation).to receive(:find_or_create_by!).with(workshop:, member: coach).and_call_original end expect do @@ -34,7 +34,7 @@ .and change { WorkshopInvitation.where(workshop:, role: 'Coach').count }.by(coaches.count) coaches.each do |coach| - expect(WorkshopInvitation).to have_received(:find_or_create_by!).with(workshop:, member: coach, role: 'Coach') + expect(WorkshopInvitation).to have_received(:find_or_create_by!).with(workshop:, member: coach) end # Verify emails were sent to the right recipients @@ -48,15 +48,15 @@ Fabricate(:coaches, chapter:, members: coaches + [banned_coach]) coaches.each do |coach| - allow(WorkshopInvitation).to receive(:find_or_create_by!).with(workshop:, member: coach, role: 'Coach').and_call_original + allow(WorkshopInvitation).to receive(:find_or_create_by!).with(workshop:, member: coach).and_call_original end manager.send(send_email, workshop, 'coaches') coaches.each do |coach| - expect(WorkshopInvitation).to have_received(:find_or_create_by!).with(workshop:, member: coach, role: 'Coach') + expect(WorkshopInvitation).to have_received(:find_or_create_by!).with(workshop:, member: coach) end - expect(WorkshopInvitation).not_to have_received(:find_or_create_by!).with(workshop:, member: banned_coach, role: 'Coach') + expect(WorkshopInvitation).not_to have_received(:find_or_create_by!).with(workshop:, member: banned_coach) end it 'sends emails when a WorkshopInvitation is created' do