Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add delete account option (attempt 2) #980

Merged
merged 7 commits into from
May 16, 2023
Merged

feat: add delete account option (attempt 2) #980

merged 7 commits into from
May 16, 2023

Conversation

yannickulrich
Copy link
Contributor

This is implementing an account deletion option, similar to #266. I've made the following changes

  • added a remove_user function to the database that deletes the user from users, sessions, and history
  • added an API endpoint: currently this is the DELETE handler for the /register endpoint but could be moved. If this gets called by a logged in user, the account gets delete
  • added the unregister command to the CLI
$ atuin unregister

I'm wondering how we should prevent accidental account deletion. We could ask the user provide their password, email, or username

$ atuin unregister -e user@example.com

Any suggestions?

PS: Thanks for looking into this and thanks for building this, it's a really cool tool 😊

@vercel
Copy link

vercel bot commented May 15, 2023 •
edited

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
atuin ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 16, 2023 5:57am

ellie
ellie requested changes May 15, 2023
View reviewed changes
Copy link
Member

@ellie ellie left a comment

Choose a reason for hiding this comment

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

Hey! Thank you for your PR! Just a couple of comments

I was actually halfway through implementing this when you opened your PR, so also have a database migration that will make this really fast on the server. I can include my changes once this is in :D

atuin-client/src/api_client.rs Outdated Show resolved Hide resolved
atuin-common/src/api.rs Outdated Show resolved Hide resolved
atuin-server/src/database.rs Outdated Show resolved Hide resolved
atuin-server/src/database.rs Outdated Show resolved Hide resolved
atuin-server/src/handlers/user.rs Outdated Show resolved Hide resolved
atuin-server/src/handlers/user.rs Outdated Show resolved Hide resolved
atuin/src/command/client/sync/unregister.rs Outdated Show resolved Hide resolved
docs/docs/commands/sync.md Outdated Show resolved Hide resolved
@yannickulrich
Copy link
Contributor Author

Oh wow, that's such a quick response 🤩 I assume you still want to merge this, right? If you've already done a lot, I'm happy to close this..

Assuming you do, I rename it everywhere. Are we keeping the endpoint as it is?

ellie
ellie reviewed May 15, 2023
View reviewed changes
atuin-server/src/router.rs Outdated Show resolved Hide resolved
@ellie
Copy link
Member

ellie commented May 15, 2023

Oh wow, that's such a quick response 🤩 I assume you still want to merge this, right? If you've already done a lot, I'm happy to close this..

Assuming you do, I rename it everywhere. Are we keeping the endpoint as it is?

Hahaha, I was looking at this anyway so all the context is there :D

Yup we can go with this! I'll probs just rebase what I have, as there are a couple of related changes I want to add. Thank you!

ellie
ellie reviewed May 15, 2023
View reviewed changes
Copy link
Member

@ellie ellie left a comment

Choose a reason for hiding this comment

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

Just a couple more things then I think we are good to go! 🚀

atuin-server/src/database.rs Outdated Show resolved Hide resolved
docs/docs/commands/sync.md Show resolved Hide resolved
@yannickulrich
Copy link
Contributor Author

I think I've changed everything you wanted, though I might've missed something somewhere (please let me know if there is)

@yannickulrich yannickulrich requested a review from ellie May 16, 2023 09:13
ellie
ellie approved these changes May 16, 2023
View reviewed changes
Copy link
Member

@ellie ellie left a comment

Choose a reason for hiding this comment

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

Great! Thank you for working on this 🙏

Seeing as this is your first time contributing, if you would like a holographic contributors-only Atuin sticker, then please fill out this form!

We do also have a Discord if you'd like to ask any questions, or just fancy hanging out!

Either way - thank you so much for the time and effort here ✨

@ellie ellie merged commit 7b9dea7 into atuinsh:main May 16, 2023
9 checks passed
@yannickulrich
Copy link
Contributor Author

Thanks so much, I really love the idea of the stickers 🤩 Have just filled the form. I still worry about accidental deletion though. Would it not make sense to put some sort of safety in?

ealap pushed a commit to ealap/atuin that referenced this pull request May 21, 2023
@yannickulrich @ealap
feat: add delete account option (attempt 2) (atuinsh#980)
9c5b1ff 
* Added DELETE register endpoint

* Added remove function to database

* Added unregister to client

* Updated docs

* Renamed functions

* Reformatting

* Used execute instead of fetch in delete_user
@razvanazamfirei razvanazamfirei mentioned this pull request May 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ellie ellie ellie approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@yannickulrich @ellie