Skip to content

Ability to delete accounts#676

Merged
ConnorRigby merged 1 commit into
nerves-hub:mainfrom
brianberlin:421-soft-delete
Apr 14, 2021
Merged

Ability to delete accounts#676
ConnorRigby merged 1 commit into
nerves-hub:mainfrom
brianberlin:421-soft-delete

Conversation

@brianberlin
Copy link
Copy Markdown
Contributor

@brianberlin brianberlin commented Oct 21, 2020

connect #421

This pull request allows users to delete their accounts when they're done using NervesHub. When an account is deleted, most things can be "hard deleted". However, in order to handle orphaned devices that may become disruptive we elect to "soft delete". I've identified users, orgs, org_users, products, and devices to become soft deleted.

@brianberlin brianberlin marked this pull request as draft October 21, 2020 14:27
@brianberlin brianberlin changed the title Adds soft deleting for anything deletable Ability to delete accounts Oct 28, 2020
@brianberlin brianberlin marked this pull request as ready for review December 17, 2020 16:00
@fhunleth
Copy link
Copy Markdown
Contributor

@brianberlin Thanks for sending this PR and congrats on getting past draft! Could you rebase and squash out the intermediate work? I'm not sure what the conflict is. If you need assistance, let us know.

Copy link
Copy Markdown
Collaborator

@jjcarstens jjcarstens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The soft delete functionality seems okay to me. But I might be a little loss on the Account removal. It seems like if we're removing an account, we'll also soft delete the org that user/account was a part of even if that org is still active with other users?

Comment thread apps/nerves_hub_web_core/lib/nerves_hub_web_core/accounts/remove_account.ex Outdated
@fhunleth
Copy link
Copy Markdown
Contributor

@brianberlin Do you have time to fix the tests this week? @jjcarstens and I are looking at merging the PR and it seems really close. We just want to make sure the tests pass and then do a quick pass on trying it out in staging.

@brianberlin
Copy link
Copy Markdown
Contributor Author

@fhunleth Sorry I haven't had much time to spare lately. I'll get it fixed up soon.

@brianberlin
Copy link
Copy Markdown
Contributor Author

@fhunleth I think all the tests should pass now. I still need to address @jjcarstens concerns.

|> Multi.delete_all(:deployments, &query_by_org_id(Deployment, &1))
|> Multi.delete_all(:firmware_deltas, &query_firmware_deltas/1)
|> Multi.delete_all(:firmware_transfers, &query_by_org_id(FirmwareTransfer, &1))
|> Multi.merge(&delete_firmwares/1)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part of the multi could be an issue. It will make an s3 request for each firmware object that needs to be deleted. If it were to get halfway through and hit an error and roll back. That would cause the account to fail again and again. Thoughts on how to handle?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like oban is available. Maybe adding a job for each firmware in the Firmwares.delete_firmware/1 function?

@brianberlin
Copy link
Copy Markdown
Contributor Author

@fhunleth I think this maybe ready. 🤞

Comment thread apps/nerves_hub_web_core/lib/nerves_hub_web_core/deployments.ex Outdated
Comment thread apps/nerves_hub_web_core/lib/nerves_hub_web_core/firmwares.ex Outdated
Comment thread apps/nerves_hub_web_core/lib/nerves_hub_web_core/repo.ex Outdated
Comment thread apps/nerves_hub_web_core/test/nerves_hub_web_core/accounts/accounts_test.exs Outdated
Comment thread apps/nerves_hub_www/lib/nerves_hub_www_web/controllers/account_controller.ex Outdated
Copy link
Copy Markdown
Collaborator

@jjcarstens jjcarstens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 Thanks @brianberlin

This LGTM and really sorry for the delay. There has been quite a push on some other things that we've been behind on and are more immediately needed. Once those are in, we'll get this in as well to test around with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants