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 :)
| 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?
There was a problem hiding this comment.
Because of the subscription pausing. I didn't want to end up in a wonky state with Stripe and QB so I wrapped everything in a lock to ensure things happen in sequence.
app/models/account/incineration.rb
Outdated
| @@ -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.
There was a problem hiding this comment.
Agreed.
I followed the pattern I saw in our other apps thinking that the familiarity would outweigh the burden of an object with just two lines of code in it. And I though it would be a nice extension point for the mixing approach.
But it doesn't seem like that ended up being the case.
There was a problem hiding this comment.
Changed in 210e9ed
While working on this I also came up with a completely different approach for doing the OSS <> SAAS integration using callbacks.
They look like the perfect use-case for this.
The OSS part just defines the callback points, and the SAAS gem adds whatever callbacks it wants.
To me this seems more readable than the placeholder methods used in other parts of the app, and nicer than having a dummy subscription object (given that the fake subscription object is implemented through a active_subscription method that would get complicated, and the need for some kind of FakeSubscription in the OSS version)
Curious as to what you think of this approach.
There was a problem hiding this comment.
Yes, I really like this way! I think for the immediate case we could've gotten awary with simply adding a method to Account that we override in the concern. But using callbacks makes for a nicer separation, and also means people could hook in other custom behavior to those events for their own integrations.
Could even consider defining helpers to make these look more like AR callbacks, along the lines of:
def self.after_cancel(...)
set_callback :cancel, :after, ...
end...so that the billing integration can then just say
after_cancel -> { subscription&.pause }But up to you if you think that's worth it. In any case, I think the callback approach is a good call 👍
@andyra the list is now left-aligned The copy in the UI and email is OK? |
(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)