Skip to content

Fix Newsletter Mailer#464

Draft
audreeedale wants to merge 5 commits into
rubyaustralia:mainfrom
audreeedale:fix_newsletter-mailer
Draft

Fix Newsletter Mailer#464
audreeedale wants to merge 5 commits into
rubyaustralia:mainfrom
audreeedale:fix_newsletter-mailer

Conversation

@audreeedale
Copy link
Copy Markdown

@audreeedale audreeedale commented May 18, 2026

What

Why

Links

#398

Copy link
Copy Markdown

@spacepotato spacepotato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work on this! Let me know if you'd like to pair on any of the spec changes.

Before opening this up for a more detailed review I'd also recommend checking out: https://github.com/rubyaustralia/ruby_au/blob/main/CONTRIBUTING.md#submitting-your-contribution to make sure that you've ticked all of the contributing requirements.

Comment thread app/models/user.rb

def primary_email
emails.find_by(primary: true)&.email || email
emails.find_by(primary: true)&.email || email.presence || read_attribute_before_type_cast("email").presence
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@audreeedale can you add some context to this change?

require "rails_helper"

RSpec.describe CampaignsMailer, type: :mailer do
describe '#campaign_email' do
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@audreeedale I think we can make the tests a fair bit slimmer if we hoist some of the common values up to let declarations.

Looking at the logic for campaign_email it looks like the following things are true:

  • The membership only exists to identify the user
  • We only need the user to access the attached emails

So with that in mind we could make the first spec something like this:

describe '#campaign_email' do
  let!(:user) { create(:user) }
  let!(:membership) { create(:membership, user:) }
  let!(:campaign) { create(:campaign, subject: 'Test campaign') }

  let(:primary_email) { create(:email, user:, email: 'primary@test.com', primary: true) }
  let(:default_email) { create(:email, user:, email: 'default@test.com', primary: false) }

  it "sends to the user's primary email" do
    primary_email && default_email

    mail = described_class.campaign_email(campaign, membership, nil).deliver_now

    expect(mail.to).to eq(primary_email.email)
  end
end

I've deliberately used let (lazy loaded) instead of let! (eager loaded) because it makes the second spec where we don't want the primary email to exist easier to implement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

2 participants