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.

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

Allow users to delete their own accounts #3398

Merged
merged 1 commit into from Feb 16, 2022

Conversation

gravitystorm
Copy link
Collaborator

Account deletion is something that administrators currently do manually, when emailed with a request, but there's no reason to prevent users from being able to delete their own accounts.

I'm opening this as a draft PR in order to get early feedback on the UI. Obviously tests need to be written, and to help with development, it doesn't actually delete the account yet 😄

The idea is to have a button on the account editing page:

Screenshot from 2021-12-15 16-59-00

Which then leads to a description of what exactly will happen when you delete your account:

Screenshot from 2021-12-15 16-54-42

Please let me know what you think. If you want to see exactly what happens when you delete an account currently, have a look at the reasonably straightforward User#destroy method.

@gravitystorm gravitystorm added the ui User Interface label Dec 15, 2021
@mmd-osm
Copy link
Contributor

mmd-osm commented Dec 15, 2021

I think we've talked about this before in #1853 (comment) Did you by chance implement some grace period of 14 days, for people who regret their decision after a few days?

Then, I believe EWG has put the idea of "Self deletion facility" on their shortlist for future project ideas they want to fund. Did they talk to you already about this idea? See https://wiki.osmfoundation.org/wiki/Working_Group_Minutes/EWG_2021-11-29

@gravitystorm
Copy link
Collaborator Author

I think we've talked about this before in #1853 (comment) Did you by chance implement some grace period of 14 days, for people who regret their decision after a few days?

I'd forgotten about that idea of a grace period! It's something that I could add though.

Then, I believe EWG has put the idea of "Self deletion facility" on their shortlist for future project ideas they want to fund. Did they talk to you already about this idea?

No, or at least not recently. I'm of course happy if someone else wants to work on this, or if I finish this work I'm sure there's plenty more things on various lists. It's at the top of my personal priority list since I've spoken with @tomhughes who said it's taking up some of his time (in his role as a site administrator).

@tomhughes
Copy link
Member

A few comments on the descriptive text:

  • While it's true that diary entries and comments are retained they are hidden from other users once an account is deleted
  • Not sure exactly what the situation with notes, note comments and changeset comments offhand

Where an account has no edits (possibly no other content?) we should either remove the email address, or offer the option of doing so, and when we aren't able to do that we should explain why it is being retained.

This PR allows users to delete their own accounts. The logic implemented matches
that currently used by the admins when they manually close accounts, although
there is room to be more complex in future e.g. completely removing accounts
with no content.

The error handling has been slightly adapted for namespaced controllers, by
anchoring the controller name with a leading forward slash.
@gravitystorm gravitystorm changed the title WIP: Allow users to delete their own accounts Allow users to delete their own accounts Feb 9, 2022
@gravitystorm gravitystorm marked this pull request as ready for review February 9, 2022 16:28
@gravitystorm
Copy link
Collaborator Author

I've finished this off, so it's ready for review:

  • I've updated to work with the user status state machine (Use a state machine for user status #3419)
  • I've updated the language for some of the caveats, to mark which ones are retained but hidden from view
  • I've added a caveat about changeset comments
  • I've not implemented any cooling off period, since on reflection that will be a lot of additional complexity, and I want to just mimic what the admins do at the moment (i.e. no cooling off period there either)
  • I've not implemented any logic for dealing with "empty" (or "no-edit") accounts, that could be added as a followup

@@ -349,7 +349,7 @@ def deny_access(_exception)
elsif current_user
set_locale
respond_to do |format|
format.html { redirect_to :controller => "errors", :action => "forbidden" }
format.html { redirect_to :controller => "/errors", :action => "forbidden" }
Copy link
Member

Choose a reason for hiding this comment

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

How is that even valid? That's not a controller name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The controller names are a bit more complex than it first appears! So if you are in a namespaced controller (e.g. Account::DeletionsController) then redirect_to :controller => "errors" means redirecting to Account::ErrorsController. I guess this makes some sense in some places but it's not what we want here. Using the forward slash is the equivalent of using the :: prefix for ruby class names (e.g. ::File), so it makes sure the redirection is to ErrorsController regardless of any namespacing in the requesting controller.

This is all irrelevant if we used named paths everywhere, but that's still a work in progress and we don't yet have named paths for the errors controller.

@tordans
Copy link
Contributor

tordans commented Feb 10, 2022

This looks great IMO. Two thoughts came to mind …

  • could I create a new account using the same email-address? And if yes, why is the email-address retained? I think it right to inform that the address is retained; however it opens the question as to why (or maybe more like "why forever").
  • the list of help info would be a good place to link the privacy docs IMO. For example in a last passage

    Learn more in our privacy policy.

However, it was interesting to read that there was not a more information about the whole deletion process there.

@tomhughes
Copy link
Member

No you can't create a new account with the same email address and yes we do need an option to remove it (or just do it) when there are no edits - when there are edits policy is that we need to keep the address.

That is what @gravitystorm was referring to when he mentioned "no edit" accounts not being done yet.

@simonpoole
Copy link
Contributor

However, it was interesting to read that there was not a more information about the whole deletion process there.

There is a, quite long, section on account removal. However it isn't clear at this point in time if this PR implements the policy described there or not.

@tomhughes
Copy link
Member

It is designed too yes, except that it doesn't implement the option of also removing the email address if not edits have been made,

The one caveat is that it depends how you define "not actively contributed" exactly as there is a clause there which I wasn't aware of which suggests we will remove things like diary posts and changeset comments we which have never done or had any intention of doing.

Diary posts and comments do get hidden but not actually removed - that is true for all deleted accounts. Other things like changeset comments and notes might get hidden or might get shown without user details.

Nothing in this PR changes the effect of a deletion, it just allows a user to do it themselves instead of me doing it.

@gravitystorm
Copy link
Collaborator Author

Nothing in this PR changes the effect of a deletion, it just allows a user to do it themselves instead of me doing it.

Yeah, I think that's a key point. I hadn't read the privacy policy since I'm not implementing any new behaviour in this PR - just allowing self-service of what Tom currently does manually.

I can see that there are some differences between policy and practice. But I think that should be resolved in a followup PRs or policy rewording, rather than being a blocker here.

@tomhughes
Copy link
Member

My main concern with merging this is that I'm not sure how useful it is without the zero edit stuff to allow email address removal.

It's probably OK to merge as is but there's a risk that it will replace polite emails to support asking for account closed with angry emails to support demanding email address removal when they see that self closing won't do that.

It will also limit which support requests I can refer back with a "use self closure" response.

@tomhughes tomhughes merged commit 1f8df78 into openstreetmap:master Feb 16, 2022
@gravitystorm gravitystorm deleted the account_deletion branch March 1, 2022 10:01
@harry-wood
Copy link
Contributor

Updated these wiki pages to describe the new "Delete account" button: OpenStreetMap account and FAQ.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ui User Interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants