From: Andy Allan <git@gravitystorm.co.uk>
Date: Wed, 5 Jan 2022 18:44:46 +0000 (+0000)
Subject: Use a state machine for user status
X-Git-Tag: live~1257^2~2
X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/1a11c4dc191d93b18fcf5aa917448c8cd6d2556b

Use a state machine for user status

The user status is a bit complex, since there are various states and
not all transitions between them make sense.

Using AASM means that we can name and restrict the transitions, which
hopefully makes them easier to reason about.
---

diff --git a/app/controllers/confirmations_controller.rb b/app/controllers/confirmations_controller.rb
index bcb4c1617..e54fa4a5d 100644
--- a/app/controllers/confirmations_controller.rb
+++ b/app/controllers/confirmations_controller.rb
@@ -25,7 +25,7 @@ class ConfirmationsController < ApplicationController
         render_unknown_user token.user.display_name
       else
         user = token.user
-        user.status = "active"
+        user.activate
         user.email_valid = true
         flash[:notice] = gravatar_status_message(user) if gravatar_enable(user)
         user.save!
diff --git a/app/controllers/passwords_controller.rb b/app/controllers/passwords_controller.rb
index 331575964..502b1357f 100644
--- a/app/controllers/passwords_controller.rb
+++ b/app/controllers/passwords_controller.rb
@@ -46,7 +46,7 @@ class PasswordsController < ApplicationController
         if params[:user]
           current_user.pass_crypt = params[:user][:pass_crypt]
           current_user.pass_crypt_confirmation = params[:user][:pass_crypt_confirmation]
-          current_user.status = "active" if current_user.status == "pending"
+          current_user.activate if current_user.may_activate?
           current_user.email_valid = true
 
           if current_user.save
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index f7a82c08c..ea04e14f6 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -164,8 +164,6 @@ class UsersController < ApplicationController
 
       Rails.logger.info "create: #{session[:referer]}"
 
-      current_user.status = "pending"
-
       if current_user.auth_provider.present? && current_user.pass_crypt.empty?
         # We are creating an account with external authentication and
         # no password was specified so create a random one
@@ -202,15 +200,17 @@ class UsersController < ApplicationController
   ##
   # sets a user's status
   def set_status
-    @user.status = params[:status]
-    @user.save
+    @user.activate! if params[:event] == "activate"
+    @user.confirm! if params[:event] == "confirm"
+    @user.hide! if params[:event] == "hide"
+    @user.unhide! if params[:event] == "unhide"
     redirect_to user_path(:display_name => params[:display_name])
   end
 
   ##
   # destroy a user, marking them as deleted and removing personal data
   def destroy
-    @user.destroy
+    @user.soft_destroy!
     redirect_to user_path(:display_name => params[:display_name])
   end
 
diff --git a/app/models/user.rb b/app/models/user.rb
index 8c75b4ef4..123ef3230 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -45,6 +45,7 @@
 
 class User < ApplicationRecord
   require "digest"
+  include AASM
 
   has_many :traces, -> { where(:visible => true) }
   has_many :diary_entries, -> { order(:created_at => :desc) }
@@ -158,6 +159,51 @@ class User < ApplicationRecord
     user
   end
 
+  aasm :column => :status, :no_direct_assignment => true do
+    state :pending, :initial => true
+    state :active
+    state :confirmed
+    state :suspended
+    state :deleted
+
+    # A normal account is active
+    event :activate do
+      transitions :from => :pending, :to => :active
+    end
+
+    # Used in test suite, not something that we would normally need to do.
+    event :deactivate do
+      transitions :from => :active, :to => :pending
+    end
+
+    # To confirm an account is used to override the spam scoring
+    event :confirm do
+      transitions :from => [:pending, :active, :suspended], :to => :confirmed
+    end
+
+    event :suspend do
+      transitions :from => [:pending, :active], :to => :suspended
+    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
+
+    # Mark the account as deleted and remove personal data
+    event :soft_destroy do
+      before do
+        remove_personal_data
+      end
+
+      transitions :from => [:pending, :active, :confirmed, :suspended], :to => :deleted
+    end
+  end
+
   def description
     RichText.new(self[:description_format], self[:description])
   end
@@ -241,8 +287,8 @@ class User < ApplicationRecord
   end
 
   ##
-  # destroy a user - leave the account but purge most personal data
-  def destroy
+  # remove personal data - leave the account but purge most personal data
+  def remove_personal_data
     avatar.purge_later
 
     self.display_name = "user_#{id}"
@@ -253,7 +299,6 @@ class User < ApplicationRecord
     self.new_email = nil
     self.auth_provider = nil
     self.auth_uid = nil
-    self.status = "deleted"
 
     save
   end
@@ -279,7 +324,7 @@ class User < ApplicationRecord
   ##
   # perform a spam check on a user
   def spam_check
-    update(:status => "suspended") if status == "active" && spam_score > Settings.spam_threshold
+    suspend! if may_suspend? && spam_score > Settings.spam_threshold
   end
 
   ##
diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb
index 95878402b..b3502f29e 100644
--- a/app/views/users/show.html.erb
+++ b/app/views/users/show.html.erb
@@ -139,30 +139,32 @@
         <nav class='secondary-actions'>
           <ul class='clearfix'>
             <% if can? :set_status, User %>
-              <% if ["active", "confirmed"].include? @user.status %>
+              <% if @user.may_activate? %>
                 <li>
-                  <%= link_to t(".deactivate_user"), set_status_user_path(:status => "pending", :display_name => @user.display_name), :method => :post, :data => { :confirm => t(".confirm") } %>
+                  <%= link_to t(".activate_user"), set_status_user_path(:event => "activate", :display_name => @user.display_name), :method => :post, :data => { :confirm => t(".confirm") } %>
                 </li>
-              <% elsif ["pending"].include? @user.status %>
+              <% end %>
+
+              <% if @user.may_confirm? %>
                 <li>
-                  <%= link_to t(".activate_user"), set_status_user_path(:status => "active", :display_name => @user.display_name), :method => :post, :data => { :confirm => t(".confirm") } %>
+                  <%= link_to t(".confirm_user"), set_status_user_path(:event => "confirm", :display_name => @user.display_name), :method => :post, :data => { :confirm => t(".confirm") } %>
                 </li>
               <% end %>
 
-              <% if ["active", "suspended"].include? @user.status %>
+              <% if @user.may_hide? %>
                 <li>
-                  <%= link_to t(".confirm_user"), set_status_user_path(:status => "confirmed", :display_name => @user.display_name), :method => :post, :data => { :confirm => t(".confirm") } %>
+                  <%= link_to t(".hide_user"), set_status_user_path(:event => "hide", :display_name => @user.display_name), :method => :post, :data => { :confirm => t(".confirm") } %>
+                </li>
+              <% end %>
+
+              <% if @user.may_unhide? %>
+                <li>
+                  <%= link_to t(".unhide_user"), set_status_user_path(:event => "unhide", :display_name => @user.display_name), :method => :post, :data => { :confirm => t(".confirm") } %>
                 </li>
               <% end %>
-              <li>
-                <% if ["pending", "active", "confirmed", "suspended"].include? @user.status %>
-                  <%= link_to t(".hide_user"), set_status_user_path(:status => "deleted", :display_name => @user.display_name), :method => :post, :data => { :confirm => t(".confirm") } %>
-                <% else %>
-                  <%= link_to t(".unhide_user"), set_status_user_path(:status => "active", :display_name => @user.display_name), :method => :post, :data => { :confirm => t(".confirm") } %>
-                <% end %>
-              </li>
             <% end %>
-            <% if can? :destroy, User %>
+
+            <% if can?(:destroy, User) && @user.may_soft_destroy? %>
               <li>
                 <%= link_to t(".delete_user"), user_path(:display_name => @user.display_name), :method => :delete, :data => { :confirm => t(".confirm") } %>
               </li>
diff --git a/test/controllers/browse_controller_test.rb b/test/controllers/browse_controller_test.rb
index d9f4401bf..9a77e8034 100644
--- a/test/controllers/browse_controller_test.rb
+++ b/test/controllers/browse_controller_test.rb
@@ -137,7 +137,7 @@ class BrowseControllerTest < ActionDispatch::IntegrationTest
   end
 
   def test_read_note_hidden_user_comment
-    hidden_user = create(:user, :status => "deleted")
+    hidden_user = create(:user, :deleted)
     note_with_hidden_user_comment = create(:note_with_comments, :comments_count => 2) do |note|
       create(:note_comment, :note => note, :author => hidden_user)
     end
@@ -161,7 +161,7 @@ class BrowseControllerTest < ActionDispatch::IntegrationTest
     assert_select "div.note-comments ul li", :count => 2
     assert_select "div.details", /Resolved by #{user.display_name}/
 
-    user.destroy
+    user.soft_destroy!
 
     reset!
 
diff --git a/test/controllers/confirmations_controller_test.rb b/test/controllers/confirmations_controller_test.rb
index 9c6e91afe..1ab4d253e 100644
--- a/test/controllers/confirmations_controller_test.rb
+++ b/test/controllers/confirmations_controller_test.rb
@@ -185,7 +185,7 @@ class UsersControllerTest < ActionDispatch::IntegrationTest
     post user_save_path, :params => { :read_ct => 1, :read_tou => 1 }
     confirm_string = User.find_by(:email => user.email).tokens.create.token
 
-    User.find_by(:display_name => user.display_name).update(:status => "deleted")
+    User.find_by(:display_name => user.display_name).hide!
 
     # Get the confirmation page
     get user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string }
@@ -242,7 +242,7 @@ class UsersControllerTest < ActionDispatch::IntegrationTest
     post user_new_path, :params => { :user => user.attributes }
     post user_save_path, :params => { :read_ct => 1, :read_tou => 1 }
 
-    User.find_by(:display_name => user.display_name).update(:status => "deleted")
+    User.find_by(:display_name => user.display_name).hide!
 
     assert_no_difference "ActionMailer::Base.deliveries.size" do
       perform_enqueued_jobs do
diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb
index 0a9a82f1c..97efec82b 100644
--- a/test/controllers/users_controller_test.rb
+++ b/test/controllers/users_controller_test.rb
@@ -547,21 +547,21 @@ class UsersControllerTest < ActionDispatch::IntegrationTest
     user = create(:user)
 
     # Try without logging in
-    post set_status_user_path(user), :params => { :status => "suspended" }
+    post set_status_user_path(user), :params => { :event => "confirm" }
     assert_response :forbidden
 
     # Now try as a normal user
     session_for(user)
-    post set_status_user_path(user), :params => { :status => "suspended" }
+    post set_status_user_path(user), :params => { :event => "confirm" }
     assert_response :redirect
     assert_redirected_to :controller => :errors, :action => :forbidden
 
     # Finally try as an administrator
     session_for(create(:administrator_user))
-    post set_status_user_path(user), :params => { :status => "suspended" }
+    post set_status_user_path(user), :params => { :event => "confirm" }
     assert_response :redirect
     assert_redirected_to :action => :show, :display_name => user.display_name
-    assert_equal "suspended", User.find(user.id).status
+    assert_equal "confirmed", User.find(user.id).status
   end
 
   def test_destroy
diff --git a/test/factories/user.rb b/test/factories/user.rb
index 48a124adb..07f1ef679 100644
--- a/test/factories/user.rb
+++ b/test/factories/user.rb
@@ -6,7 +6,10 @@ FactoryBot.define do
 
     # These attributes are not the defaults, but in most tests we want
     # a 'normal' user who can log in without being redirected etc.
-    status { "active" }
+    after(:build) do |user, _evaluator|
+      user.activate
+    end
+
     terms_seen { true }
     terms_agreed { Time.now.getutc }
     data_public { true }
@@ -17,23 +20,31 @@ FactoryBot.define do
     end
 
     trait :pending do
-      status { "pending" }
+      after(:build) do |user, _evaluator|
+        user.deactivate
+      end
     end
 
     trait :active do
-      status { "active" }
+      # status { "active" }
     end
 
     trait :confirmed do
-      status { "confirmed" }
+      after(:build) do |user, _evaluator|
+        user.confirm
+      end
     end
 
     trait :suspended do
-      status { "suspended" }
+      after(:build) do |user, _evaluator|
+        user.suspend
+      end
     end
 
     trait :deleted do
-      status { "deleted" }
+      after(:build) do |user, _evaluator|
+        user.soft_destroy
+      end
     end
 
     factory :moderator_user do
diff --git a/test/models/user_test.rb b/test/models/user_test.rb
index 8a6e41013..f35cdd23f 100644
--- a/test/models/user_test.rb
+++ b/test/models/user_test.rb
@@ -246,9 +246,9 @@ class UserTest < ActiveSupport::TestCase
     assert create(:moderator_user).has_role?("moderator")
   end
 
-  def test_destroy
+  def test_soft_destroy
     user = create(:user, :with_home_location, :description => "foo")
-    user.destroy
+    user.soft_destroy
     assert_equal "user_#{user.id}", user.display_name
     assert user.description.blank?
     assert_nil user.home_lat
diff --git a/test/system/diary_entry_test.rb b/test/system/diary_entry_test.rb
index 58992dafc..31e6d7b07 100644
--- a/test/system/diary_entry_test.rb
+++ b/test/system/diary_entry_test.rb
@@ -35,7 +35,7 @@ class DiaryEntrySystemTest < ApplicationSystemTestCase
   end
 
   test "deleted diary entries should not be shown to admins when the user is also deleted" do
-    @deleted_user = create(:user, :status => :deleted)
+    @deleted_user = create(:user, :deleted)
     @deleted_entry = create(:diary_entry, :visible => false, :user => @deleted_user)
 
     sign_in_as(create(:administrator_user))
diff --git a/test/system/user_suspension_test.rb b/test/system/user_suspension_test.rb
index 075303ff0..a72512357 100644
--- a/test/system/user_suspension_test.rb
+++ b/test/system/user_suspension_test.rb
@@ -7,7 +7,7 @@ class UserSuspensionTest < ApplicationSystemTestCase
     visit edit_account_path
     assert_content "My Settings"
 
-    user.update(:status => "suspended")
+    user.suspend!
 
     visit edit_account_path
     assert_content "This decision will be reviewed by an administrator shortly"