Add cascade delete constraints, allow users to delete their accounts#316
Conversation
38beb90 to
19b18f5
Compare
e759e33 to
e2c5f16
Compare
e2c5f16 to
ff3eda6
Compare
ff3eda6 to
ff825ab
Compare
|
What remains to be done here? Do we just need a final decision on what should be deleted, or are there code pieces left? |
|
Hm, can't really remember the technical points, maybe the deletion of zips when individual mod versions are deleted had still to be done. |
ff825ab to
6b42944
Compare
Would it be easier to proceed if we deferred this decision to a config setting? |
|
Should we add cascade deletion to |
|
|
||
|
|
||
| def upgrade() -> None: | ||
| op.drop_constraint('downloadevent_version_id_fkey', 'downloadevent', type_='foreignkey') |
There was a problem hiding this comment.
Would it make sense to try to delete old records that weren't deleted in the past and now have invalid or missing values?
templates/profile.html
Outdated
| <div class="col-md-6"> | ||
| <h2 title="change-password">Delete User Account</h2> | ||
| </div> |
KerbalStuff/blueprints/api.py
Outdated
| if form_username != username: | ||
| return {'error': True, 'reason': 'Wrong username'} |
There was a problem hiding this comment.
Hmm, it doesn't feel right to assume form inputs in an /api/ route. An "API" should be usable by whatever random JSON clients people want to write (e.g. Netkan or Nertea's publishing scripts), not just the front-end.
Instead of checking whether there's a match after the user clicks the Delete confirmation button, could we try this?
- The Delete button in the popup starts out disabled (but Cancel is always enabled)
- An event handler in the username box checks whether its value is correct, and then enables or disables the Delete button based on that
That way the user could only click the button after they type the name. If some admin wants to delete users with an API script independently, the confirmation flow would not be relevant to them anyway.
This may also make it possible to change the popup form's action to /api/user/{{ profile.username }}/delete and delete most of the new script code.
There was a problem hiding this comment.
Hmm, it doesn't feel right to assume form inputs in an /api/ route. An "API" should be usable by whatever random JSON clients people want to write (e.g. Netkan or Nertea's publishing scripts), not just the front-end.
This is pretty normal and standard, we do this in a lot of API routes. That's why they are POSTs. You need some way to transmit data to the server.
I'm not aware of any HTTP client that is incapable of including form data in a POST request. Netkan only reads from the API and doesn't write anything, thus it also doesn't use POSTs. But I'm very confident that WebClient could include form data if it needed to.
Here it is primarily meant as a "confirmation" feature so you don't delete your account accidentally. Whether that's useful for APIs is debatable I guess, I'm okay with removing it. I'm pretty sure that just came from adopting the password change mechanism.
There was a problem hiding this comment.
Oh right, the api.md documentation does say:
Submit all POSTS with the request body encoded as
multipart/form-data. Your HTTP library
of choice probably handles that for you. All responses are JSON.
... so using a form input is fine. I guess my objection is just to this one specific form input, then.
There was a problem hiding this comment.
Added an implementation of this suggestion in #417, with script code enabling/disabling the button.
KerbalStuff/blueprints/api.py
Outdated
| if current_user.username == username: | ||
| deletable = True | ||
| if not deletable: | ||
| return {'error': True, 'reason': 'Unauthorized'} |
There was a problem hiding this comment.
Should we use HTTP status codes for this function's returns? The change password route does this, specifically with 403.
6b42944 to
309896a
Compare
|
Rebased and added a fix for Looks like the discussion of what to delete has wound down, and this PR's approach will be fine. |

Motivation
Every now and then users want to delete their accounts (we should ask for the reason the next time, out of interest).
Every website with the option to create accounts should (and also must) have another option to delete these accounts again.
How and what
There's still the discussion on what we actually want to delete if a user deletes their account.
For know, to get us started, I decided to stay consistent with what we already do and delete everything linked to that user, both in the database and on disk. This means mods of these users are deleted too.
Please continue this discussion in #215 or on the Discord and leave the comments on this PR to actual code review.
I'll update this PR depending on how we decide on that.
Changes
_restore_game_info()threw an exception. Now it returnsNone.This needs a database migration.
Closes #215