Send Devise emails triggered from the 'Email' model asynchronously
What does this MR do?
This change sends Devise
emails triggered from the Email
model asynchronously.
This change has been made because of the error observed @ #217831 (closed)
Background
Currently, User
is the primary model that is backed by Devise
modules like lockable
, confirmable
etc.
Devise
triggers emails on certain events (eg: Confirm your email
email when your email is updated), and by default these emails are triggered along with the request.
To trigger these emails in async fashion, we currently use this method, which pushes the ActionMailer
job via ActiveJob
to Sidekiq
, thus triggering the emails outside of the web request.
Problem
We store secondary emails in the Email
model and this model also contains the Devise
module confirmable
included in it.
Which means that an email will be triggered when a new secondary email is added for a user.
However, this model does not contain the overridden
method send_devise_notification
, to send these emails in asynchronous fashion.
Problem
The lack of the overridden method makes Devise
trigger these emails synchronously and this can be problematic because:
- The web-request takes longer to complete - user has to wait a longer period for the request to be successful.
- Any problem that arises during the sending of these email causes a
500
error to the user. This has been observed here, hence the reason to make this change. - Had the email been async, Sidekiq would have put the failed email in
retry
queue and re-tried sending later. However, now since the email is part of the request-response cycle, we lose the ability to retry a failed email.
Changes:
- The current
send_devise_notification
has been extracted to a separate module so that it can be reused across models. - This new module has been added in
Email
model, making theDevise
emails triggered from this model asynchronous. - Added specs for testing
Devise
emails are being enqueued in themailer
queue from both theUser
andEmail
model. - Changed some specs that relied on the behaviour of confirmation emails being delivered syncronously.
Screenshots
Does this MR meet the acceptance criteria?
Conformity
-
Changelog entry -
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers -
Informed Infrastructure department of a default or new setting change, if applicable per definition of done
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
-
Label as security and @ mention @gitlab-com/gl-security/appsec
-
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods -
Security reports checked/validated by a reviewer from the AppSec team