Add self-service account deletion #2246
Conversation
monorkin
force-pushed
the
account-deletion
branch
from
30da181 to
8c44f82
Compare
kevinmcconnell
left a comment
kevinmcconnell
left a comment
There was a problem hiding this comment.
Excited to see this! This will be handy.
I realise it's still WIP, but I left a couple questions/comments because I happened to notice it in passing :) Feel free to ignore if you're already on top of those bits.
We still want the step tracking so that the job can be interrupted. But, since the scope is idempotent, we don't need to track how far it got.
kevinmcconnell
left a comment
kevinmcconnell
left a comment
There was a problem hiding this comment.
Looks great! I added a few more rather minor questions :)
| include SmtpDeliveryErrorHandling | ||
|
|
||
| queue_as :backend | ||
| discard_on ActiveJob::DeserializationError |
There was a problem hiding this comment.
Minor: the other jobs have a blank line separating the queue_as/discard_on. Probably want to be consistent here.
| end | ||
|
|
||
| def cancel(initiated_by: Current.user) | ||
| with_lock do |
There was a problem hiding this comment.
Just curious, why is this a lock rather than a transaction?
|
|
||
| def ensure_can_access_account | ||
| if Current.user.blank? || !Current.user.active? | ||
| if Current.account.cancelled? || Current.user.blank? || !Current.user.active? |
There was a problem hiding this comment.
Also a minor thing, but something about having this clause here seems a little complicated to me. I think it's because it's covering two different types of situation in the same method. We're checking:
- Is the account active?
- Does the current identity have a user in the account?
Those are two different situations, but their details are mixed. And this method is only called by the before_action when account.present? as well, so the details are also a bit split, into two places.
Maybe worth extracting a method for each of those, so that the logic in the action is more obvious? Something like account_active? and user_active? maybe? And that could also cover the conditional that's attached to the before_action so we could simplify that.
It's not a big deal either way, but it just seems like it's starting to get intertwined a bit.
| @@ -1,5 +1,6 @@ | |||
| class ExportAccountDataJob < ApplicationJob | |||
| queue_as :backend | |||
| discard_on ActiveJob::DeserializationError | |||
There was a problem hiding this comment.
As above re blank line
| @@ -0,0 +1,4 @@ | |||
| class Account::Cancellation < ApplicationRecord | |||
| belongs_to :account | |||
| belongs_to :initiated_by, class_name: "User", optional: true | |||
There was a problem hiding this comment.
I don't think this association is actually optional, is it?
| @@ -0,0 +1,12 @@ | |||
| class Account::Incineration | |||
There was a problem hiding this comment.
I wonder if this object is really worth it. It's effectively just wrapping up a 2-line method, but then there's a 1-line method to call it. Is there an advantage to pulling it out into a separate object, as oppose to doing it directly in Account? Perhaps instead it could move into the Cancellable concern, since incinerating is really part of the wider concept of cancelling.
Also, as discussed elsewhere, it does feel like we may be missing a clear extension point for subscriptions. That might be part of what suggests this logic happening outside of Account. But if we had that, it'd be more natural to trigger it inside the class.
| <% if @account.try(:subscription) %> | ||
| <li>We won't charge you anymore</li> | ||
| <% end %> |
There was a problem hiding this comment.
Do we need this conditional? You can't cancel accounts outside of SaaS anyway, right?
(The modal shows up in the center of the screen, my screen just has more vertical space so it looks off when I crop the screenshot)