Unify some account reset classes #10740
Conversation
- Move logic to AccountResetRequest model
- Move logic into AccountResetRequest#cancel!
- Move logic into AccountResetRequest#cancel!
| def notify_user_via_email_of_account_reset_cancellation | ||
| user.confirmed_email_addresses.each do |email_address| | ||
| UserMailer.with(user: user, email_address: email_address).account_reset_cancel. | ||
| deliver_now_or_later | ||
| end | ||
| end | ||
|
|
||
| def notify_user_via_phone_of_account_reset_cancellation | ||
| MfaContext.new(user).phone_configurations.each do |phone_configuration| | ||
| phone = phone_configuration.phone | ||
| Telephony.send_account_reset_cancellation_notice( | ||
| to: phone, | ||
| country_code: Phonelib.parse(phone).country, | ||
| ) | ||
| end | ||
| end |
There was a problem hiding this comment.
I don't love model classes that send emails and notify and stuff, but the Profile class already does that, so it's not new to our codebase
There was a problem hiding this comment.
Maybe there's a case there to at least keep the (a) service class for sending the notifications? I don't feel too strongly either way. Or to @monfresh's point, keeping one service class for canceling a pending request, but collapsing everything into it.
There was a problem hiding this comment.
Keeping one service class is a good point
changelog: Internal, Source code, Unify related classes for account reset
| ).order(requested_at: :asc).first | ||
| end | ||
|
|
||
| def cancel!(now: Time.zone.now) |
There was a problem hiding this comment.
One potential issue with this being a public method on the model is that someone could cancel any Account Reset Request. Whereas before, it was clear that it only applies to the ones that were pending.
There was a problem hiding this comment.
Maybe it could raise or short-circuit unless the request is pending?
|
|
||
| def call(now: Time.zone.now) | ||
| account_reset_request.update!(cancelled_at: now) | ||
| NotifyUserOfRequestCancellation.new(user).call |
There was a problem hiding this comment.
Since this was only used here, perhaps one way to reduce the amount of files you need to read to understand this is to pull in the notification into this class.
|
Looks like the build got caught up on an unnecessary |
| @@ -9,6 +11,24 @@ class AccountResetRequest < ApplicationRecord | |||
| primary_key: 'issuer' | |||
| # rubocop:enable Rails/InverseOf | |||
|
|
|||
| # @return [AccountResetRequest, nil] | |||
| def self.pending_request_for(user) | |||
There was a problem hiding this comment.
Curious how we decide whether it makes sense for this method to live here or in the User model. I guess the upside of the latter is that it's a little more concise to do user.pending_account_reset_request vs. AccountResetRequest.pending_request_for(user), but perhaps at the risk of the User model becoming more bloated than it already is.
| ).order(requested_at: :asc).first | ||
| end | ||
|
|
||
| def cancel!(now: Time.zone.now) |
There was a problem hiding this comment.
Maybe it could raise or short-circuit unless the request is pending?
| def notify_user_via_email_of_account_reset_cancellation | ||
| user.confirmed_email_addresses.each do |email_address| | ||
| UserMailer.with(user: user, email_address: email_address).account_reset_cancel. | ||
| deliver_now_or_later | ||
| end | ||
| end | ||
|
|
||
| def notify_user_via_phone_of_account_reset_cancellation | ||
| MfaContext.new(user).phone_configurations.each do |phone_configuration| | ||
| phone = phone_configuration.phone | ||
| Telephony.send_account_reset_cancellation_notice( | ||
| to: phone, | ||
| country_code: Phonelib.parse(phone).country, | ||
| ) | ||
| end | ||
| end |
There was a problem hiding this comment.
Maybe there's a case there to at least keep the (a) service class for sending the notifications? I don't feel too strongly either way. Or to @monfresh's point, keeping one service class for canceling a pending request, but collapsing everything into it.
|
I think I'm going to need to come back to this with single account reset service, will close in the meantime |
Removes:
There are still more single-method classes we could remove, but this seemed like a good start. After #10732 lands, that might make it easier to remove:
And the others, maybe could reorganize those, but it would take a little more thinking