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

Soft delete users #3726

Merged
merged 7 commits into from
Dec 15, 2022
Merged

Conversation

vkWeb
Copy link
Member

@vkWeb vkWeb commented Oct 10, 2022

Summary

Brings in user soft delete. User account data is kept intact. Recovery of user account is now possible. 🎉

Manual verification steps performed

  • user cannot sign in after deleting account.
  • user cannot register after deleting account.
  • after calling user.recover() user can login after email verification.
  • after calling user.recover() user can register after email verification. (behaves same as invitation workflow)

Reviewer guidance

Does manual verification mentioned above works on your end?

References

Closes #1990.


Contributor's Checklist

PR process:

  • If this is an important user-facing change, PR or related issue the CHANGELOG label been added to this PR. Note: items with this label will be added to the CHANGELOG at a later time
  • If this includes an internal dependency change, a link to the diff is provided
  • The docs label has been added if this introduces a change that needs to be updated in the user docs?
  • If any Python requirements have changed, the updated requirements.txt files also included in this PR
  • Opportunities for using Google Analytics here are noted
  • Migrations are safe for a large db

Studio-specifc:

  • All user-facing strings are translated properly
  • The notranslate class been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)
  • All UI components are LTR and RTL compliant
  • Views are organized into pages, components, and layouts directories as described in the docs
  • Users' storage used is recalculated properly on any changes to main tree files
  • If there new ways this uses user data that needs to be factored into our Privacy Policy, it has been noted.

Testing:

  • Code is clean and well-commented
  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Any new interactions have been added to the QA Sheet
  • Critical and brittle code paths are covered by unit tests

Reviewer's Checklist

This section is for reviewers to fill out.

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@vkWeb vkWeb requested a review from bjester October 10, 2022 14:15
@vkWeb vkWeb marked this pull request as draft October 10, 2022 19:36
Copy link
Member

@bjester bjester left a comment

Choose a reason for hiding this comment

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

This is looking great! One comment about the UserHistory in combination with deleted_at, and see some tests are failing

@vkWeb vkWeb requested review from bjester and rtibbles October 31, 2022 08:43
@vkWeb vkWeb marked this pull request as ready for review October 31, 2022 08:43
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

I think one more test would be good to give confidence here.

files = File.objects.filter(contentnode__in=contentnode_ids)
files_on_storage = files.values_list("file_on_disk", flat=True)

for disk_file_path in files_on_storage:
Copy link
Member

Choose a reason for hiding this comment

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

Reinstating the file deletion behaviour here does make sense in the context of the PR - but would be good to have some tests on this function just to guarantee that we are definitely not deleting files that are being used in other channels - regardless of how simple the code may seem here.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reinstation of file deletion plays a critical role in garbage collection so I was determined to make sure it doesn't bring in bad things.

Thankfully, there were tests already written for the case when two contentnodes share the same file and there are more file deletion test cases under CleanUpContentNodesTestCase that cover other behaviours also:

class CleanUpContentNodesTestCase(StudioTestCase):

def test_doesnt_delete_shared_files(self):
"""
Make sure that a file shared between two file objects doesn't
get deleted when one of the file objects gets deleted
"""

I have made sure that changing the file deletion behaviour fails the above tests. Since the current code passes the above tests, I am confident with my changes.

@rtibbles sir, if you think there's a case that's remaining to be tested...? then I'll definitely love to write test for that case.

Copy link
Member Author

Choose a reason for hiding this comment

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

@rtibbles umm, sir, thoughts?

@vkWeb vkWeb requested a review from rtibbles November 6, 2022 12:02
@bjester
Copy link
Member

bjester commented Dec 6, 2022

Just a note that I've done some code review and testing of this, and so far everything is good. I still need to test the delayed portion

@vkWeb
Copy link
Member Author

vkWeb commented Dec 6, 2022

Yes @bjester sir please take your time.

@bjester
Copy link
Member

bjester commented Dec 15, 2022

I believe you mentioned this to me at one point, @vkWeb, but I don't think I understood-- seems that the copy in the user's account is a bit misleading:

You must delete these channels manually or invite others to edit them before you can delete your account.

If you've added an editor to the referenced channels, you still can't delete your account. It's wholly unrelated to your feature work here though

Copy link
Member

@bjester bjester left a comment

Choose a reason for hiding this comment

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

We might want to remove the user from any channel's editors after the 90 day period, but I'm not sure if there are consequences to that. We could implement that another time.

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.

Change hard delete process for account deletion to soft delete
3 participants