Establish script to remove deleted and inactive accounts. #15210

ashercodeorg · 2017-05-19T13:18:20Z

The aim of the script is to remove all PII, PPII, and information owned by thirty day deleted users and seven year inactive users. Spec (probably somewhat out-of-date already).

VOCABULARY: Throughout, we use the term "purge" to refer to this removal of PII, PPII, and information owned. Throughout, we use the term "clean" to refer to the removal of the information while leaving the skeleton holding that information intact.

Given this involves data retention, adding explicit lists to this PR of the data we explicitly want to destroy and the data we explicitly want to keep.

DATA TO DESTROY

DATA TO KEEP

ashercodeorg · 2017-05-19T13:22:05Z

Looking for a high-level review, confirming that it is sensical to perform this process semi-manually rather than create triggers and dependent destroys within our models. Assuming so, please let me know whether you would prefer a bunch of incremental PRs or a long PR (expecting some back and forth).

ashercodeorg · 2017-05-19T15:16:07Z

bin/cron/delete_accounts

+
+  user.reload.update!(
+    name: '',
+    username: '',


For better or for worse, there are unique indexes on username, email, etc. How do we want to manipulate these fields while maintaining the DB constraints? Do we want to relax the DB constraints?

ashercodeorg · 2017-05-19T16:40:47Z

bin/cron/delete_accounts

+def delete_open_ended_answers(user_id)
+  UserLevels.where(user_id: user_id).find_each do |user_level|
+    if OPEN_ENDED_LEVEL_TYPES.include? user_level.level.type
+      user_level.level_source.destroy


How do we want do delete the level_source? Does it make sense to wipe its data (also md5?), keeping the row otherwise intact? Do we want to purge the whole row?

Note that, in both cases, there are implications for other users that happen to have the same level_source_id.

why are we destroying the level source at all and not just the user level?

Because we are fine with maintaining a record that the student did work, we want to eliminate the work that was done. This is motivated by student privacy law, I believe.

shouldn't we then update the user_level to no longer point to a level_source and delete the level_source only if there are no longer any user_levels pointing to it?

It isn't at all clear to me what makes the most sense, from a product or from a engineering perpsective (thus my starting this thread). If we could delete the level source only if there were no user_levels pointing to it, that'd be awesome. Unfortunately, we are lacking the index on user_levels.level_source_id to allow us to carry this out efficiently (practically).

Note that we do have an index on activities.level_source_id. That said, there would be some complexity in trying to make use of it, as we'd have to consult both activities and activities_old (the backup table from before our overrunning the ID counter).

Unless I'm misunderstanding something, deleting the level source indiscriminately seems like it isn't even an option, given how significant the impact would be on other users.

How bad would it be to simply remove the pointer from deleted user to level source and potentially have orphaned level sources floating around?

My belief is that this is not considered acceptable by product. Otherwise, I agree, this would be the easy answer.

For now, I'm going to suggest we not focus too much on this, as product still needs to finalize their requirements.

Hamms · 2017-05-19T18:19:41Z

What's the thinking behind doing this manually as opposed to relying on dependent destroys?

ashercodeorg · 2017-05-19T18:32:56Z

My concern with dependent destroys is that this pipeline only happens on "hard"-delete, not "soft"-delete. So we'd need to invoke this pipeline in a manner distinct from user.destroy, where we want the soft-delete dependent destroys but not the hard-delete dependent destroys.

Also, it seems less error prone to keep data deletion to a script. Though I could be very mistaken on that.

Hamms

I think I like the overall idea of doing this manually and carefully, but it does seem pretty error-prone; I'm not sure whether I'm more worried about things that should be deleted getting skipped and being orphaned or about the act of deleting something causing other things that we don't want to delete to break.

I'm in favor of this all being in a single large PR, in the hopes that that will make it easier to notice something that got missed.

Hamms · 2017-05-19T19:12:07Z

bin/cron/delete_accounts

+def delete_open_ended_answers(user_id)
+  UserLevels.where(user_id: user_id).find_each do |user_level|
+    if OPEN_ENDED_LEVEL_TYPES.include? user_level.level.type
+      user_level.level_source.destroy


why are we destroying the level source at all and not just the user level?

Hamms · 2017-05-19T19:12:59Z

bin/cron/delete_accounts

+  FreeResponse
+  Gamelab
+  Weblab
+).freeze


Should Artist and Playlab be included in this?

Most definitely. Amongst many other things...the exact list is still getting sorted out.

Hamms · 2017-05-19T19:15:13Z

bin/cron/delete_accounts

+    next unless Follower.where(student_user: student_user).count == 1
+
+    # TODO(asher): Determine whether this should be delete_user_and_dependencies(student_user).
+    student_user.destroy!


should this be a soft delete?

Per the TODO, I need to get a definite answer. Note that, after thirty days, the two are equivalent (as a soft-deleted user then has delete_user_and_dependencies run on it).

ashercodeorg · 2017-06-20T19:29:34Z

Ready for review... PTAL.

ashercodeorg · 2017-06-21T17:19:02Z

Actually, just kidding. There are a few more changes that I want to refactor from the script into models.

ashercodeorg · 2017-06-23T17:06:20Z

Those refactor changes have been made, PTAL.

Hamms · 2017-06-23T17:58:56Z

lib/cdo/delete_accounts_helper.rb

+  # Cleans all LevelSource-backed progress associated with the user.
+  # WARNING: Since the relationship between user_levels and activities and level_sources is
+  #   many-to-one, we cannot be certain that this is not messing with the integrity of the data for
+  #   other users.


I'm still quite concerned about this.

Noted. As am I. But I'm pretty sure @jeremydstone is not... (at least, I'm doing what I'm told).

Per offline discussion with @jeremydstone, this has been changed to severe the link between the user's level-backed progress and the user (see commit d39f371) rather than remove the actual student work (image). PTAL.

FYI @poorvasingal @Hamms @jeremydstone.

Hamms · 2017-06-23T18:04:21Z

lib/cdo/delete_accounts_helper.rb

+      where(dashboard_user_id: user_id).
+      map {|contact_rollup| contact_rollup[:pardot_id]}
+    failed_ids = Pardot.delete_prospects(pardot_ids)
+    raise "Pardot.delete_prospects failed for Pardot IDs #{failed_ids.join(', ')}." unless failed_ids


should this be unless failed_ids.empty??

Yes. Done (using if failed_ids.any?).

Hamms · 2017-06-23T18:06:54Z

lib/cdo/delete_accounts_helper.rb

+    DeleteAccountsHelper.remove_from_solr(user.id)
+
+    user.purged_at = Time.zone.now
+    user.save(validate: false)


does this imply that purged users are expected to fail validation?

No, purged users are expected to pass validation, I've gone to some trouble to make sure of that. That said, I want the purged_at update to get persisted, whether or not the user is valid.

jeremydstone · 2017-07-05T16:22:46Z

lib/cdo/delete_accounts_helper.rb

+  # NOTE: This method assumes the sections and followers associated with user_id have already been
+  # destroyed.
+  # @param [Integer] user_id for which to delete orphaned students.
+  def self.purge_orphaned_students(user_id)


This is maybe the scariest part to me. An area where we could be doing the right thing under the letter of our TOS/Privacy policy and yet have kids come in the in morning to discover all their stuff nuked.

Feel welcome to take the issue up in the spec or with @poorvasingal directly.

Teachers will have a super clear warning that their student accounts (that don't have a personal login) will get deleted if they delete their own account. I also don't think it's a common scenario for teachers to delete their accounts so I am not super worried.

SELECT user_type, COUNT(0) FROM users WHERE deleted_at IS NOT NULL GROUP BY 1; +-----------+----------+ | user_type | COUNT(0) | +-----------+----------+ | student | 71937 | | teacher | 6710 | +-----------+----------+

jeremydstone · 2017-07-05T16:25:06Z

The code looks good. Would like to have an interactive conversation with @ashercodeorg around testing, risks & mitigation prior to merge.

ashercodeorg · 2017-08-02T19:19:45Z

Per offline conversation, merging so that this can be tested in isolation on adhoc and test environments.

ashercodeorg added the data label May 19, 2017

ashercodeorg requested a review from Hamms May 19, 2017 13:18

ashercodeorg force-pushed the deleteScript branch from b3ffd60 to 83c3223 Compare May 19, 2017 13:19

This was referenced May 19, 2017

Add current sign in at deleted at indexes to users #15212

Closed

Adds indexes on current_sign_in_at and deleted_at to users. #15214

Merged

ashercodeorg commented May 19, 2017

View reviewed changes

ashercodeorg force-pushed the deleteScript branch 2 times, most recently from 720eb9b to 7b510ae Compare May 19, 2017 16:37

ashercodeorg commented May 19, 2017

View reviewed changes

Hamms reviewed May 19, 2017

View reviewed changes

This was referenced May 22, 2017

Create SolrHelper module. #15284

Merged

Add FREE_RESPONSE_ATTRS and NON_FREE_RESPONSE_ATTRS to SurveyResult. #15311

Merged

ashercodeorg force-pushed the deleteScript branch from b7a5670 to a56d0bc Compare May 25, 2017 19:51

ashercodeorg mentioned this pull request May 26, 2017

Add Pardot#delete_pardot_prospects. #15409

Merged

ashercodeorg force-pushed the deleteScript branch 2 times, most recently from c57cfce to 4f3951a Compare June 6, 2017 19:00

This was referenced Jun 9, 2017

Define LevelSource#clear_data to clear level_source data. #15731

Merged

Add PeerReview#clear_data method. #15733

Merged

ashercodeorg force-pushed the deleteScript branch from 4f3951a to 25dd94c Compare June 12, 2017 15:30

ashercodeorg mentioned this pull request Jun 12, 2017

Add OverflowActivity model for overflow_activites DB table. #15769

Merged

ashercodeorg force-pushed the deleteScript branch from 25dd94c to 5ff14cb Compare June 14, 2017 20:53

ashercodeorg mentioned this pull request Jun 14, 2017

Add FirebaseHelper#delete. #15861

Merged

ashercodeorg force-pushed the deleteScript branch from 5ff14cb to 54e483c Compare June 15, 2017 22:14

This was referenced Jun 16, 2017

Add SurveyResult#clear_open_ended_responses. #15896

Merged

Add purged_at column to users. #15936

Merged

ashercodeorg force-pushed the deleteScript branch from 54e483c to 323e9f8 Compare June 20, 2017 19:27

ashercodeorg requested review from ewjordan and jeremydstone June 20, 2017 19:29

This was referenced Jun 21, 2017

Define User#clear_user_and_mark_purged. #16001

Merged

Define UserGeo#clear_user_geo. #16004

Merged

ashercodeorg force-pushed the deleteScript branch 2 times, most recently from ef5af3f to 7ff8f19 Compare June 23, 2017 17:05

Hamms reviewed Jun 23, 2017

View reviewed changes

ashercodeorg added 2 commits July 5, 2017 11:11

Establish script to remove deleted and inactive accounts.

be748f4

PR feedback: check for emptiness, not nullness, of failed_ids.

3db10c4

jeremydstone reviewed Jul 5, 2017

View reviewed changes

Remove dead constant.

07e6844

ashercodeorg mentioned this pull request Jul 6, 2017

Make /c/ links obfuscate level_source_id. #16242

Closed

Severe the link to, don't delete, level-backed progress.

d39f371

ashercodeorg force-pushed the deleteScript branch from f9988f7 to d39f371 Compare July 6, 2017 16:17

ashercodeorg merged commit 7a50109 into staging Aug 2, 2017

ashercodeorg deleted the deleteScript branch August 2, 2017 19:19

islemaster mentioned this pull request May 2, 2018

Fully compliant user purge operation, part 1 #22166

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Establish script to remove deleted and inactive accounts. #15210

Establish script to remove deleted and inactive accounts. #15210

ashercodeorg commented May 19, 2017 •

edited

ashercodeorg commented May 19, 2017

ashercodeorg May 19, 2017

ashercodeorg May 19, 2017

Hamms May 19, 2017

ashercodeorg May 19, 2017

Hamms May 19, 2017

ashercodeorg May 19, 2017

Hamms May 19, 2017

ashercodeorg May 19, 2017

Hamms commented May 19, 2017

ashercodeorg commented May 19, 2017

Hamms left a comment

Hamms May 19, 2017

Hamms May 19, 2017

ashercodeorg May 19, 2017

Hamms May 19, 2017

ashercodeorg May 19, 2017

ashercodeorg commented Jun 20, 2017

ashercodeorg commented Jun 21, 2017

ashercodeorg commented Jun 23, 2017

Hamms Jun 23, 2017

ashercodeorg Jun 23, 2017

ashercodeorg Jul 6, 2017

Hamms Jun 23, 2017

ashercodeorg Jun 23, 2017

Hamms Jun 23, 2017

ashercodeorg Jun 23, 2017

jeremydstone Jul 5, 2017

ashercodeorg Jul 5, 2017

poorvasingal Jul 5, 2017

ashercodeorg Jul 5, 2017

jeremydstone commented Jul 5, 2017

ashercodeorg commented Aug 2, 2017

Establish script to remove deleted and inactive accounts. #15210

Establish script to remove deleted and inactive accounts. #15210

Conversation

ashercodeorg commented May 19, 2017 • edited

ashercodeorg commented May 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hamms commented May 19, 2017

ashercodeorg commented May 19, 2017

Hamms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashercodeorg commented Jun 20, 2017

ashercodeorg commented Jun 21, 2017

ashercodeorg commented Jun 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremydstone commented Jul 5, 2017

ashercodeorg commented Aug 2, 2017

ashercodeorg commented May 19, 2017 •

edited