Best way to save attribute inside save block?
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).
activation_sent_at
update_column
assign_attributes
guest?
for optimization, you can send the mail via ActiveJob by configuring and using
deliver_later; that will make it asynchronous. That will make sure your controller doesn't spend time composing, dealing with mail provider and sending email.– kiddorails
Jul 1 at 10:43
deliver_later
1 Answer
1
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.)
@account.guest?
AccountMailer.activation(@account).deliver_later
AccountMailer
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.
account
activation_sent_at
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:
activation_sent_at
The controller should be doing two transactions, not one. Which is why I said: Don't change it.
The
after_*before_* callbacks is a better place for things like this one @account.activation_sent_at = Time.zone.now– Зелёный
Jul 1 at 11:12
after_*before_*
@account.activation_sent_at = Time.zone.now
@Зелёный Is it though, really? Just because it's a part of the rails framework, that does not make is a "better place". Writing
after_save :set_activation_sent_at unless :guest? may make move the logic out of the controller, but it still causes the exact same problem I described above -- only now, it's even more convoluted.– Tom Lord
Jul 1 at 11:16
after_save :set_activation_sent_at unless :guest?
Keep things clean and simple, thin controller, fat model.
– Зелёный
Jul 1 at 11:17
Your suggestion doesn't simplify things, in my opinion. Also, models should ideally be skinny too. If you feel there's a better way to write this, I'd be interested to see it in full.
– Tom Lord
Jul 1 at 12:41
I would think about adding a
send_activation_mail method to User that does both - calling the AccountMailer and updating the activation_sent_at column.– spickermann
Jul 1 at 18:17
send_activation_mail
User
AccountMailer
activation_sent_at
By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.
The workaround is using
assign_attributes, but I would not recommend it. Your code is fine, and an additional update query to update the column shouldn't make much difference. Also, check likeguest?can also be applicable on some model hooks which won't be invoked with assign_attributes. Overall cost of using it will be making it slightly unreadable– kiddorails
Jul 1 at 10:38