gavin/maybe
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 28 Wiki Activity

add basic support for deleting account #683

Closed
jemiluv8 wants to merge 22 commits from account-deletion into main
pull from: account-deletion
merge into: gavin:main
Conversation 25 Commits 22 Files Changed 19 +194 -10
jemiluv8 commented 2024-04-27 03:42:42 +08:00 (Migrated from github.com)
Copy Link

fixes #675
/claim #675

DEMO 1 - admin is not allowed to delete account until there are no members

https://github.com/maybe-finance/maybe/assets/119384208/3cf8f17c-460e-49b1-a0f8-030ecc297d28

DEMO 2 - admin account deletion after all members are deleted

https://github.com/maybe-finance/maybe/assets/119384208/86202816-9012-472e-bd7e-941cd63244b5

This draft PR demonstrates a naive implementation of user account deletion that I'll build on over the next few hours.
This implementation deletes the user and attempts to delete other related data in the same request "cycle".

The problem identified while deleting the demo account is that the delete operation can really take a long time depending on the number of accounts associated with the user's family. I have thus decided to do the cleanup in a background job.

UI Requirements

  • User must confirm deletion via a dialog
  • After deletion, user is redirected to the sign up page with success notification
  • Deletions should be atomic. A delete action should never result in invalid states or partially deleted data.
  • i18n translations provided for en.yml

BACKEND Requirements

  • Appropriate tests written
  • Introduce background worker to do cleanup according to logic relating to Family, Accounts, admin and member User types
  • Write tests for endpoint and background worker
  • Final review and testing
fixes #675 /claim #675 ### DEMO 1 - admin is not allowed to delete account until there are no members https://github.com/maybe-finance/maybe/assets/119384208/3cf8f17c-460e-49b1-a0f8-030ecc297d28 ### DEMO 2 - admin account deletion after all members are deleted https://github.com/maybe-finance/maybe/assets/119384208/86202816-9012-472e-bd7e-941cd63244b5 This draft PR demonstrates a naive implementation of user account deletion that I'll build on over the next few hours. This implementation deletes the user and attempts to delete other related data in the same request "cycle". The problem identified while deleting the demo account is that the delete operation can really take a long time depending on the number of accounts associated with the user's family. I have thus decided to do the cleanup in a background job. **UI Requirements** - [X] User must confirm deletion via a dialog - [X] After deletion, user is redirected to the sign up page with success notification - [X] Deletions should be atomic. A delete action should never result in invalid states or partially deleted data. - [X] i18n translations provided for en.yml BACKEND Requirements - [X] Appropriate tests written - [X] Introduce background worker to do cleanup according to logic relating to Family, Accounts, admin and member User types - [X] Write tests for endpoint and background worker - [ ] Final review and testing
jemiluv8 (Migrated from github.com) reviewed 2024-04-27 20:35:03 +08:00
app/controllers/settings/profiles_controller.rb
@@ -17,10 +17,34 @@ class Settings::ProfilesController < ApplicationController
end
end
jemiluv8 (Migrated from github.com) commented 2024-04-27 20:35:03 +08:00
Copy Link

the primary controller that handles deletion of the user account
It simply deletes the user account (which is the most basic requirement)
and schedules a background job to do required cleanup

the primary controller that handles deletion of the user account It simply deletes the user account (which is the most basic requirement) and schedules a background job to do required cleanup
jemiluv8 (Migrated from github.com) reviewed 2024-04-27 20:36:17 +08:00
app/javascript/controllers/application.js
jemiluv8 (Migrated from github.com) commented 2024-04-27 20:36:17 +08:00
Copy Link

Hack really to allow us too add a reject button down there

Hack really to allow us too add a reject button down there
jemiluv8 (Migrated from github.com) reviewed 2024-04-27 20:36:30 +08:00
app/javascript/controllers/application.js
@@ -25,1 +29,4 @@
}
if (reject) {
const className = document.getElementById("turbo-confirm-reject").className.replace("hidden", "")
jemiluv8 (Migrated from github.com) commented 2024-04-27 20:36:30 +08:00
Copy Link

Allows us to customize the accept button

Allows us to customize the accept button
jemiluv8 (Migrated from github.com) reviewed 2024-04-27 20:38:08 +08:00
app/jobs/delete_user_job.rb
@@ -0,0 +1,7 @@
class DeleteUserJob < ApplicationJob
jemiluv8 (Migrated from github.com) commented 2024-04-27 20:38:07 +08:00
Copy Link

this deletes accounts and related data

this deletes accounts and related data
jemiluv8 (Migrated from github.com) reviewed 2024-04-27 20:38:56 +08:00
app/jobs/delete_user_job.rb
@@ -0,0 +1,7 @@
class DeleteUserJob < ApplicationJob
jemiluv8 (Migrated from github.com) commented 2024-04-27 20:38:56 +08:00
Copy Link

This is my addition thus far.
I figured since family_id is required on all user models,
any existing user with a non-existent family is "invalid"?

This is my addition thus far. I figured since family_id is required on all user models, any existing user with a non-existent family is "invalid"?
jemiluv8 (Migrated from github.com) reviewed 2024-04-27 20:40:43 +08:00
test/fixtures/users.yml
@@ -4,6 +4,15 @@ family_admin:
last_name: Dylan
email: bob@bobdylan.com
password_digest: <%= BCrypt::Password.create('password') %>
role: admin
jemiluv8 (Migrated from github.com) commented 2024-04-27 20:40:42 +08:00
Copy Link

I just wonder why whoever created this test fixture never bothered to specify the admin role?

I just wonder why whoever created this test fixture never bothered to specify the admin role?
jemiluv8 (Migrated from github.com) reviewed 2024-04-27 20:41:43 +08:00
test/jobs/delete_user_job_test.rb
@@ -0,0 +1,52 @@
require "test_helper"
jemiluv8 (Migrated from github.com) commented 2024-04-27 20:41:43 +08:00
Copy Link

We could have more edge cases to test but these are it for now.

We could have more edge cases to test but these are it for now.
zachgoll (Migrated from github.com) reviewed 2024-04-28 00:30:23 +08:00
zachgoll (Migrated from github.com) left a comment
Copy Link

I like the test cases here and overall strategy. Just left a few suggestions on how we could simplify some of the logic.

@jemiluv8 also, I've updated the issue requirements to make the deletion logic simpler as I realize from the implementation that the previous requirements may have been trying to achieve too much and added unnecessary complexity:

CleanShot 2024-04-27 at 12 42 04

I like the test cases here and overall strategy. Just left a few suggestions on how we could simplify some of the logic. @jemiluv8 also, I've updated the issue requirements to make the deletion logic simpler as I realize from the implementation that the previous requirements may have been trying to achieve too much and added unnecessary complexity: ![CleanShot 2024-04-27 at 12 42 04](https://github.com/maybe-finance/maybe/assets/16676157/5958b00b-ec69-4da5-ae14-b6446948f3d7)
app/controllers/settings/profiles_controller.rb
zachgoll (Migrated from github.com) commented 2024-04-28 00:18:08 +08:00
Copy Link

While I think you're right that we should be doing this in a background job, I think a potentially more stable strategy would be something like this:

def mark_user_for_deletion_and_logout
  Current.user.update!(marked_for_deletion: true)
  DeleteUserJob.perform_later(Current.user)
  logout
end

And then on login, we could check marked_for_deletion and fail early if it is marked true.

This would allow us to work directly with the User record all in one spot and avoid some of the error handling here.

While I think you're right that we should be doing this in a background job, I think a potentially more stable strategy would be something like this: ```rb def mark_user_for_deletion_and_logout Current.user.update!(marked_for_deletion: true) DeleteUserJob.perform_later(Current.user) logout end ``` And then on login, we could check `marked_for_deletion` and fail early if it is marked `true`. This would allow us to work directly with the `User` record all in one spot and avoid some of the error handling here.
app/javascript/controllers/application.js
@@ -20,9 +20,19 @@ Turbo.setConfirmMethod((message) => {
document.getElementById("turbo-confirm-body").innerHTML = body;
zachgoll (Migrated from github.com) commented 2024-04-28 00:05:25 +08:00
Copy Link

I think we could probably move this to the _confirm_modal partial and then toggle classes as needed here.

I think we could probably move this to the `_confirm_modal` partial and then toggle classes as needed here.
app/jobs/delete_user_job.rb
@@ -0,0 +1,7 @@
class DeleteUserJob < ApplicationJob
zachgoll (Migrated from github.com) commented 2024-04-28 00:29:34 +08:00
Copy Link

While I think my comment above (passing in the user instance) will simplify all of this logic a bit, we should move a lot of this method into the models themselves. As an example:

other_admins = User.where(family_id: user.family_id, role: "admin").where.not(id: user.id).count

This should likely be a scope or method on Family so that we can call:

other_admins_count = Current.family.admins.where.not(id: user.id).count
While I think my comment above (passing in the user instance) will simplify all of this logic a bit, we should move a lot of this method into the models themselves. As an example: ```rb other_admins = User.where(family_id: user.family_id, role: "admin").where.not(id: user.id).count ``` This should likely be a scope or method on `Family` so that we can call: ```rb other_admins_count = Current.family.admins.where.not(id: user.id).count ```
app/models/user.rb
zachgoll (Migrated from github.com) commented 2024-04-28 00:06:59 +08:00
Copy Link

These are not necessary since we've got an enum. You should be able to already call User.first.member? and User.first.admin?.

These are not necessary since we've got an enum. You should be able to already call `User.first.member?` and `User.first.admin?`.
zachgoll (Migrated from github.com) reviewed 2024-04-28 00:43:03 +08:00
app/controllers/settings/profiles_controller.rb
zachgoll (Migrated from github.com) commented 2024-04-28 00:43:03 +08:00
Copy Link

FYI - https://github.com/maybe-finance/maybe/issues/675#issuecomment-2081051730

FYI - https://github.com/maybe-finance/maybe/issues/675#issuecomment-2081051730
zachgoll (Migrated from github.com) reviewed 2024-04-28 00:43:11 +08:00
app/jobs/delete_user_job.rb
@@ -0,0 +1,7 @@
class DeleteUserJob < ApplicationJob
zachgoll (Migrated from github.com) commented 2024-04-28 00:43:11 +08:00
Copy Link

FYI - https://github.com/maybe-finance/maybe/issues/675#issuecomment-2081051730

FYI - https://github.com/maybe-finance/maybe/issues/675#issuecomment-2081051730
jemiluv8 (Migrated from github.com) reviewed 2024-04-28 01:02:16 +08:00
app/models/user.rb
jemiluv8 (Migrated from github.com) commented 2024-04-28 01:02:16 +08:00
Copy Link

Gotcha

Gotcha
jemiluv8 (Migrated from github.com) reviewed 2024-04-28 01:03:34 +08:00
app/javascript/controllers/application.js
@@ -20,9 +20,19 @@ Turbo.setConfirmMethod((message) => {
document.getElementById("turbo-confirm-body").innerHTML = body;
jemiluv8 (Migrated from github.com) commented 2024-04-28 01:03:34 +08:00
Copy Link

I just didn't want to modify the _confirm_modal partial. I'll do that now instead. So we just toggle visibility based on args.

I just didn't want to modify the _confirm_modal partial. I'll do that now instead. So we just toggle visibility based on args.
jemiluv8 (Migrated from github.com) reviewed 2024-04-28 01:04:59 +08:00
app/jobs/delete_user_job.rb
@@ -0,0 +1,7 @@
class DeleteUserJob < ApplicationJob
jemiluv8 (Migrated from github.com) commented 2024-04-28 01:04:59 +08:00
Copy Link

Gotcha.

Gotcha.
jemiluv8 commented 2024-04-28 01:06:19 +08:00 (Migrated from github.com)
Copy Link

@zachgoll , I'll work on the changes and revert.

@zachgoll , I'll work on the changes and revert.
jemiluv8 (Migrated from github.com) reviewed 2024-04-28 02:55:18 +08:00
app/controllers/sessions_controller.rb
@@ -9,3 +9,3 @@
def create
if user = User.authenticate_by(email: params[:email], password: params[:password])
if user = User.authenticate_by(email: params[:email], password: params[:password], marked_for_deletion: false)
login user
jemiluv8 (Migrated from github.com) commented 2024-04-28 02:55:18 +08:00
Copy Link

prevents users marked_for_deletion from logging in.

prevents users marked_for_deletion from logging in.
jemiluv8 (Migrated from github.com) reviewed 2024-04-28 02:59:38 +08:00
app/controllers/settings/profiles_controller.rb
@@ -18,2 +18,4 @@
end
def destroy
begin
jemiluv8 (Migrated from github.com) commented 2024-04-28 02:59:38 +08:00
Copy Link

This is much simpler I hope.
We just refuse to delete last admin with members - otherwise all goes through

This is much simpler I hope. We just refuse to delete last admin with members - otherwise all goes through
jemiluv8 (Migrated from github.com) reviewed 2024-04-28 03:00:04 +08:00
app/controllers/settings/profiles_controller.rb
@@ -23,3 +41,4 @@
params.require(:user).permit(:first_name, :last_name,
family_attributes: [ :name, :id ])
end
jemiluv8 (Migrated from github.com) commented 2024-04-28 03:00:04 +08:00
Copy Link

It read so well. I couldn't help using it. Thanks for the snippet.

It read so well. I couldn't help using it. Thanks for the snippet.
jemiluv8 commented 2024-04-28 08:52:00 +08:00 (Migrated from github.com)
Copy Link

@zachgoll , I've made changes based on review. Let me know how the click-testing goes.

@zachgoll , I've made changes based on review. Let me know how the click-testing goes.
zachgoll (Migrated from github.com) reviewed 2024-04-30 03:11:46 +08:00
app/jobs/delete_user_job.rb
@@ -0,0 +4,4 @@
def perform(user)
user.purge_data
end
end
zachgoll (Migrated from github.com) commented 2024-04-30 03:06:39 +08:00
Copy Link

The logic here looks better, but I think we should be handling all of this within user.rb and family.rb. Something along the lines of:

# delete_user_job.rb

def perform(user)
  user.purge_data
end

# user.rb

def purge_data_later
  DeleteUserJob.perform_later(self)
end

def purge_data
    if family.can_be_destroyed_by?(self)
      family.destroy! # with correct cascade behavior, this should destroy the user too
    else
      destroy!
    end
end

# family.rb

def can_be_destroyed_by?(user)
  users.count == 1 && user.admin?
end

# profiles_controller.rb
def mark_user_for_deletion_and_logout
    Current.user.update!(marked_for_deletion: true)
    Current.user.purge_data_later
    logout
end
The logic here looks better, but I think we should be handling all of this within `user.rb` and `family.rb`. Something along the lines of: ```rb # delete_user_job.rb def perform(user) user.purge_data end ``` ```rb # user.rb def purge_data_later DeleteUserJob.perform_later(self) end def purge_data if family.can_be_destroyed_by?(self) family.destroy! # with correct cascade behavior, this should destroy the user too else destroy! end end ``` ```rb # family.rb def can_be_destroyed_by?(user) users.count == 1 && user.admin? end ``` ```rb # profiles_controller.rb def mark_user_for_deletion_and_logout Current.user.update!(marked_for_deletion: true) Current.user.purge_data_later logout end ```
app/models/family.rb
@@ -8,0 +14,4 @@
end
def can_be_destroyed_by?(user)
users.count == 1 && user.admin?
zachgoll (Migrated from github.com) commented 2024-04-30 02:44:53 +08:00
Copy Link

This is bringing in context that is not necessary at the family level (marked_for_deletion).

It may be better to instead keep these as basic scopes and let the job logic filter out relevant admins.

scope :admins, -> { users.where(role: "admin") }
scope :members, -> { users.where(role: "member") }
This is bringing in context that is not necessary at the family level (`marked_for_deletion`). It may be better to instead keep these as basic scopes and let the job logic filter out relevant admins. ```rb scope :admins, -> { users.where(role: "admin") } scope :members, -> { users.where(role: "member") } ```
app/views/shared/_confirm_modal.html.erb
zachgoll (Migrated from github.com) commented 2024-04-30 02:47:10 +08:00
Copy Link

Is this ID being used for something? I think we can safely remove it.

Is this ID being used for something? I think we can safely remove it.
jemiluv8 (Migrated from github.com) reviewed 2024-04-30 03:34:18 +08:00
app/views/shared/_confirm_modal.html.erb
jemiluv8 (Migrated from github.com) commented 2024-04-30 03:34:18 +08:00
Copy Link

It is not being used. Will clean it up.

It is not being used. Will clean it up.
jemiluv8 (Migrated from github.com) reviewed 2024-04-30 22:17:41 +08:00
app/models/family.rb
@@ -8,0 +14,4 @@
end
def can_be_destroyed_by?(user)
users.count == 1 && user.admin?
jemiluv8 (Migrated from github.com) commented 2024-04-30 22:17:41 +08:00
Copy Link

updated the method to not user the marked_for_deletion query.
Had challenges using scope here though so maintained the method.

updated the method to not user the marked_for_deletion query. Had challenges using scope here though so maintained the method.
zachgoll commented 2024-04-30 23:32:08 +08:00 (Migrated from github.com)
Copy Link

@jemiluv8 thanks for the updates here. I think this PR is at a final state now, although in #698 there is a "deactivate" strategy implemented which I think offers a more concise and intuitive way to handle what we had come up here with the marked_for_deletion flag on the User model.

Since there's a lot of good work here that allowed us to see the requirements for this issue more clearly and come to a solid solution, we'll award a split bounty on this.

I'll close this PR out, but there will be a payment awarded after #698 is merged. Thanks for all the work on this!

@jemiluv8 thanks for the updates here. I think this PR is at a final state now, although in #698 there is a "deactivate" strategy implemented which I think offers a more concise and intuitive way to handle what we had come up here with the `marked_for_deletion` flag on the `User` model. Since there's a lot of good work here that allowed us to see the requirements for this issue more clearly and come to a solid solution, we'll award a split bounty on this. I'll close this PR out, but there will be a payment awarded after #698 is merged. Thanks for all the work on this!
👍 1
jemiluv8 commented 2024-04-30 23:34:17 +08:00 (Migrated from github.com)
Copy Link

That's great @zachgoll, I learned a lot and been intrigued by the deactivation strategy.
It looked solid.

That's great @zachgoll, I learned a lot and been intrigued by the deactivation strategy. It looked solid.
👍 1
zachgoll commented 2024-04-30 23:44:05 +08:00 (Migrated from github.com)
Copy Link

@jemiluv8 just an FYI, the bounty split didn't work properly when the PR was merged, but we're working to get that resolved right now so you get the appropriate payout.

@jemiluv8 just an FYI, the bounty split didn't work properly when the PR was merged, but we're working to get that resolved right now so you get the appropriate payout.
jemiluv8 commented 2024-04-30 23:44:40 +08:00 (Migrated from github.com)
Copy Link

Gotcha @zachgoll. Thanks.

Gotcha @zachgoll. Thanks.

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
zachgoll
jemiluv8
Labels
Clear labels
📆 Planned
Not ready to be worked on yet, but planned for future
 💻 Self Hosted only
Issues pertaining to self-hosted versions of Maybe
 ⚙️ Config
ℹ️ Needs info
Further information is requested
 📱 Mobile
Bugs exclusively happening on mobile devices
 1️⃣ High Priority
High priority issues will be worked on next
 🚨 Urgent
Issues that are urgent for app functionality
 3️⃣ Low priority
Contributions accepted, but Maybe team will not be working on this in the near term
 2️⃣ Medium Priority
Community contributions accepted, Maybe team only works on if there are no high priority items open
 ⚡ Performance
Performance issues to address
 🎨 UI / UX
Issues related to UI / UX interactions and design.
 💎 Bounty
💰 Rewarded
📊 Data issue
Issues where the app functionally works, but the data is missing or inaccurate.
 🙋 Bounty claim
$1.5K
Community
This is a great issue for community members to work on
 dependencies
Pull requests that update a dependency file
 Design
Hosted only
Hosted app only
 Required self hoster data migration
Self hosters must run data migrations to resolve this issue
 ruby
Pull requests that update Ruby code
No Label 🙋 Bounty claim
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
gavin
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: gavin/maybe#683
Delete Branch "account-deletion"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?