CODEWITHSUNDEEP
ruby-on-railsrubyactiverecord
Tintin81
Tintin81

Reputation: 10207

Best way to save attribute inside save block?

In my controllers I often have functionality like this:

@account = Account.new(account_params)
if @account.save
  if @account.guest?
    ...
  else
    AccountMailer.activation(@account).deliver_later
    @account.update_column(:activation_sent_at, Time.zone.now)
    flash[:success] = "We've sent you an email."
    redirect_to root_path
  end
end

What's the best way to send an email and update the activation_sent_at attribute without having to save the record twice? Calling update_column doesn't feel right to me here because AFAIK it creates an extra SQL query (correct me if I'm wrong).

Upvotes: 0

Views: 328

Answers (1)

Tom Lord
Tom Lord

Reputation: 28305

Your code is fine. I wouldn't change it.

For example, you might be tempted to do something like this:

@account = Account.new(account_params)
@account.activation_sent_at = Time.zone.now unless @account.guest?
if @account.save
  if @account.guest?
    ...
  else
    AccountMailer.activation(@account).deliver_later
    flash[:success] = "We've sent you an email."
    redirect_to root_path
  end
end

Aside from the small issue that there's now repeated logic around @account.guest?, what happens if AccountMailer.activation(@account).deliver_later fails? (When I say "fails", I mean - for example - AccountMailer has been renamed, so the controller returns a 500 error.)

In that case, you'd end up with a bunch of account records which have an activation_sent_at but were never sent an email; and you'd have no easy way to distinguish them.

Therefore, this code warrants running two database calls anyway: One to create the record, and then another to confirm that an email was sent. If you refactor the code to only perform a single database call, then you'll become vulnerable to either:

  • Sending an email to a non-created user, or
  • Marking a user with activation_sent_at despite o email being sent.

The controller should be doing two transactions, not one. Which is why I said: Don't change it.

Upvotes: 2

Related Questions