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.

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!

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

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.activate! if params[:event] == "activate"
-    @user.save
+    @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
 

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
+  # remove personal data - leave the account but purge most personal data
-  def destroy
+  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
 
   ##

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 %>
-                <li>
-                  <%= link_to t(".activate_user"), set_status_user_path(:status => "active", :display_name => @user.display_name), :method => :post, :data => { :confirm => t(".confirm") } %>
                 </li>
               <% end %>
 
-              <% if ["active", "suspended"].include? @user.status %>
+              <% if @user.may_confirm? %>
                 <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(".confirm_user"), set_status_user_path(:event => "confirm", :display_name => @user.display_name), :method => :post, :data => { :confirm => t(".confirm") } %>
+                </li>
+              <% end %>
+
+              <% if @user.may_hide? %>
+                <li>
+                  <%= 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>

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!
 

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

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

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

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

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))

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"