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

[WIP] Add cancel account button to Admin user edit view #321

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

cofiem
Copy link
Contributor

@cofiem cofiem commented Feb 8, 2017

Closes #295

@cofiem cofiem self-assigned this Feb 8, 2017
@cofiem cofiem changed the title Add cancel account button to Admin user edit view [WIP] Add cancel account button to Admin user edit view Feb 8, 2017
@@ -10,7 +10,7 @@ development:
database: baw_local_dev
pool: 5
username: postgres
password: ''
password: 'anothersecret938@'
Copy link
Member

Choose a reason for hiding this comment

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

this should not have been changed - or at least checked in?

@@ -10,7 +10,7 @@ test:
database: baw_local_test
pool: 5
username: postgres
password: ''
password: 'anothersecret938@'
Copy link
Member

Choose a reason for hiding this comment

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

this should not have been changed - or at least not checked in?

db/structure.sql Outdated

SET statement_timeout = 0;
SET lock_timeout = 0;
SET client_encoding = 'UTF8';
SET standard_conforming_strings = on;
SET check_function_bodies = false;
SET client_min_messages = warning;
SET row_security = off;
Copy link
Member

Choose a reason for hiding this comment

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

this should be reverted - it interferes with the postgres version we are using

@@ -1,3 +0,0 @@
---
Copy link
Member

Choose a reason for hiding this comment

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

why was this deleted?

The stub was needed at one point

@atruskie
Copy link
Member

atruskie commented Feb 8, 2017

In addition to the review, I have a few questions (because this appears to be more than skin-deep changes):

User accounts are not archivable/paranoid:

  1. Are they permanently deleted?
  2. What happens to associated records?
  • I did not see a unit test that ensured all relevant records are deleted?
  • Not a question: this potential for exponential deletions scares me... we've got referential integrity enabled... what happens when the user is a modifier on another user's project/site/audio_event?
  1. What happens to on-disk audio recordings, and other non-db stuff if an account is deleted

I'm tempted to suggest that users should be paranoid...

Most comments addressed - user paranoid/archive not implemented yet
@coveralls
Copy link

coveralls commented Feb 8, 2017

Coverage Status

Coverage increased (+0.03%) to 34.777% when pulling c235524 on feature-delete-account-for-admin into 42da8bb on develop.

@cofiem
Copy link
Contributor Author

cofiem commented Feb 11, 2017

So, regarding deleting a user, two (and a half) main options:

  1. Delete user completely. This requires changing all the linked items in the db to:
    • have fields that link to users be nullable
    • be set to another user
    • also be deleted when a user is deleted
  2. Archive user. This means the user might still appear in some places (e.g. last modified by),
    but the user cannot log in, and the user details are deleted.
  3. Some combination of the two - some items are also deleted, some are kept.

Deleting is the most obvious/easiest to explain to users. It is also the most difficult to implement,
as users are associated with almost every other table in the db. Changing to nullable or setting to e.g.
admin or nobody or something looses information and might introduce other issues.

Archiving makes it easier on the developers, but is more complicated to explain to users.
Yes, we can delete preferences and other user-specific info (e.g. email?, user name?),
but the users will still be in the db. Can they re-activate their account? What will be shown in the
UI when a 'deleted' user is the owner? Last modifier?

A combination might make the most sense, but is also even more complex - need to modify some tables
to be nullable/set to other user as well as keep the user.

What shall we do?

@atruskie
Copy link
Member

Definitely archiving - and we'll keep it simple for now.

Use the language "deactivate my account" to imply we will keep the data. Add explaining terminology:

When your account is deactivated, you can no longer log in, or make any changes. No other user will be able to see your identifiable information.

  • can they reactivate their account? yes - but through a contact us form (admin intervention only)
  • we'll keep identifiable information for now, and only anonymize (delete email/username) on request (TODO: add note to terms and conditions)
  • we'll show user ids (e.g. in data exports and API responses) where relevant and in other places (e.g. the modified/created tags) we'll add some generic fallback text (maybe: "Anonymous" or "Deactivated user")

I think if we ensure the account is deactivated, we can fix up small UI bugs later on, so don't worry about all the edge cases so much.

(which is all much better than a hard deletion option because we'd have to consider every edge case).

@cofiem
Copy link
Contributor Author

cofiem commented Feb 14, 2017

Will likely close #292 (see https://github.com/QutBioacoustics/baw-server/pull/321/files#diff-92cb535b3e202ecb0d7670f6d43a23f3R7)

(i.e. after_destroy :set_deleter_id, on: :delete in place of before_validation :set_deleter_id, on: :delete)

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

Successfully merging this pull request may close these issues.

3 participants