-
Notifications
You must be signed in to change notification settings - Fork 0
Calling deliver_later for forms instead of deliver_now #16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
added struct file for affiliate borrow request moved struct into model for borrow_request removed borrow_request.rb struct file Changing doemoff_patron_email_form to use struct similar to affiliate_borrower Changing stack pass for to use deliver_later Changed deliver_now to deliver_later for more forms
|
deliver_later needs to be serialized and several of the forms don't have db entries so needed to be reworked a bit. The request_mailer views for several of these use dot notation. I think this approach should work for those. (e.g. AffilliateBorrowRequestForm, SecurityIncidentReportForm) |
awilfox
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, a few questions on some of the details.
| expect do | ||
| @form.submit! | ||
| end.to have_enqueued_job(ActionMailer::MailDeliveryJob).with( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have expect { @form.submit! } in other places (included the AffiliateBorrowRequestFormSpec just above this file in the diff). I think we should be consistent with block syntax with these tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rubocop complained and said that the format above "expect { @form.submit! }" shouldn't be used for multi line blocks. I don't agree with rubocop though. I think it looks better having be consistent
| expect do | ||
| invoice.submit! | ||
| end.to have_enqueued_job(ActionMailer::MailDeliveryJob) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, this should probably be expect { invoice.submit! }.to …
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
…equest_form, already a default
anarchivist
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generally looks good to me - thank you for taking this on!
added struct file for affiliate borrow request
moved struct into model for borrow_request
removed borrow_request.rb struct file
Changing doemoff_patron_email_form to use struct similar to affiliate_borrower
Changing stack pass for to use deliver_later
Changed deliver_now to deliver_later for more forms