[openstreetmap/openstreetmap-website] Use a state machine for user status (PR #3419)

[Tobias]

@tordans commented on this pull request.

This makes the code a lot better 👍 .

> +    # Used in test suite, not something that we would normally need to do.
+    event :deactivate do
+      transitions :from => :active, :to => :pending
+    end

You could make it more explicit that this is just for specs by guarding it or wrapping it in an `if Rails.env.test?` block (not sure about the exact syntax, just both is just to illustrate).

```suggestion
    # Used in test suite, not something that we would normally need to do.
    event :deactivate do
      transitions :from => :active, :to => :pending, : guard => Proc.new { |_| Rails.env.test? }
    end
```

> +    # Mark the account as deleted but keep all data intact
+    event :hide do
+      transitions :from => [:pending, :active, :confirmed, :suspended], :to => :deleted
+    end
+
+    event :unhide do
+      transitions :from => [:deleted], :to => :active
+    end

This signals to me that hiding is not really a state but a flag on the user. A bit like act_as_paranoid ([and similar](https://github.com/rubysherpas/paranoia)) did back in the day. 

Using a flag would clarify the `:unhide` event. Right now, a `pending` user that gets deleted would become `active` without ever running through the activation steps.

Another way to solve this would be to transition to `pending` within `unhide` and ask the user to re-take the activation steps.

---

However, I wonder what the use case for hiding an account would be. I had assumed that `remove_personal_data` was applied to all deleted users. Which IMO is desirable since we don't want personal info lying around on accounts that are not used and not visible, right? Could we maybe remove the `hide` usecase altogether?

> @@ -158,6 +159,51 @@ def self.authenticate(options)
     user
   end
 
+  aasm :column => :status, :no_direct_assignment => true do

Since the user model file is already very long, [this aasm section](https://github.com/openstreetmap/openstreetmap-website/blob/1a11c4dc191d93b18fcf5aa917448c8cd6d2556b/app/models/user.rb#L162-L205) would be a good candidate to extract into a concern IMO. However, skimming over https://codeclimate.com/blog/7-ways-to-decompose-fat-activerecord-models/ again suggests that a Concern is also smelly, so maybe there is a better way to separate the large file(?).

-- 
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/3419#pullrequestreview-851267221
You are receiving this because you are subscribed to this thread.

Message ID: <openstreetmap/openstreetmap-website/pull/3419/review/851267221 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20220112/4edd9360/attachment.htm>