diff options
author | Timothy Andrew <mail@timothyandrew.net> | 2017-02-16 10:05:10 +0300 |
---|---|---|
committer | Timothy Andrew <mail@timothyandrew.net> | 2017-02-24 14:20:20 +0300 |
commit | f2ed82fa8486875660b80dd061827ac8b86d00b6 (patch) | |
tree | 07b0221a225f32de7a3063a4bee2ff97463f9c54 /app/services/users | |
parent | 3bd2a98f64a12a2e23a6b9ce77427a9c2033a6bf (diff) |
Implement final review comments from @DouweM and @rymai
- Have `Uniquify` take a block instead of a Proc/function. This is more
idiomatic than passing around a function in Ruby.
- Block a user before moving their issues to the ghost user. This avoids a data
race where an issue is created after the issues are migrated to the ghost user,
and before the destroy takes place.
- No need to migrate issues (to the ghost user) in a transaction, because
we're using `update_all`
- Other minor changes
Diffstat (limited to 'app/services/users')
-rw-r--r-- | app/services/users/destroy_service.rb | 13 |
1 files changed, 9 insertions, 4 deletions
diff --git a/app/services/users/destroy_service.rb b/app/services/users/destroy_service.rb index bf1e49734cc..523279944ae 100644 --- a/app/services/users/destroy_service.rb +++ b/app/services/users/destroy_service.rb @@ -37,13 +37,18 @@ module Users end private - + def move_issues_to_ghost_user(user) + # Block the user before moving issues to prevent a data race. + # If the user creates an issue after `move_issues_to_ghost_user` + # runs and before the user is destroyed, the destroy will fail with + # an exception. We block the user so that issues can't be created + # after `move_issues_to_ghost_user` runs and before the destroy happens. + user.block + ghost_user = User.ghost - Issue.transaction do - user.issues.update_all(author_id: ghost_user.id) - end + user.issues.update_all(author_id: ghost_user.id) user.reload end |