]> git.openstreetmap.org Git - rails.git/commitdiff
Use a state machine for user status
authorAndy Allan <git@gravitystorm.co.uk>
Wed, 5 Jan 2022 18:44:46 +0000 (18:44 +0000)
committerAndy Allan <git@gravitystorm.co.uk>
Wed, 12 Jan 2022 18:16:14 +0000 (18:16 +0000)
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.

12 files changed:
app/controllers/confirmations_controller.rb
app/controllers/passwords_controller.rb
app/controllers/users_controller.rb
app/models/user.rb
app/views/users/show.html.erb
test/controllers/browse_controller_test.rb
test/controllers/confirmations_controller_test.rb
test/controllers/users_controller_test.rb
test/factories/user.rb
test/models/user_test.rb
test/system/diary_entry_test.rb
test/system/user_suspension_test.rb

index bcb4c1617d25ca3a6ac8df8d6ac8b2b2836b8653..e54fa4a5db8de2bded6ae8d3de9d39255a748581 100644 (file)
@@ -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!
index 331575964aba6abaec47a8ade9467ae4504b2186..502b1357f29f9fb085833ecc78af61da0f964246 100644 (file)
@@ -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
index f7a82c08c6c9df94c00f0b8c6b8be42e19d0c335..ea04e14f6a9d4757d693e547f8e8cbadd8c4d2b4 100644 (file)
@@ -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
 
index 8c75b4ef45e7ad4fd608a4fd59c2ecb7a332b538..123ef3230e39fd025dc3e06bd1c1a2e219486d8c 100644 (file)
@@ -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
 
   ##
index 95878402bde87eb4e415a6098ff523aec17f4178..b3502f29ec738d933bc2fb982950e38ea797f6f5 100644 (file)
         <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>
index d9f4401bfbf83e61ddd5d6a54c944e715754a816..9a77e803417c2ac1b7b73f3d182c7ab3baf36e5c 100644 (file)
@@ -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!
 
index 9c6e91afe65a1733d8fc75ff2e71d77336bbf3a4..1ab4d253e5461d6a33bcdfb3f7014e71893eb5ed 100644 (file)
@@ -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
index 0a9a82f1cb4ce04781e2514580d23a033a7e2d91..97efec82b4d8599ebb0abc6005dafe4fb8c29582 100644 (file)
@@ -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
index 48a124adb7c0fe9872cf1513ebb37eca461ea8ef..07f1ef679b8a6470bee38c3c986f185e7157a007 100644 (file)
@@ -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
index 8a6e41013b15eb93f6f02175396d7b3c46c3b912..f35cdd23fb4c7eabb44f066aaf3d064b2e299b43 100644 (file)
@@ -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
index 58992dafc66a1ed267d416fbe708f39777d7ff6c..31e6d7b075e3ba3b9ebb07d6efc3177c0eedeca9 100644 (file)
@@ -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))
index 075303ff0f926ade5f20fc53b30f132d640205c6..a7251235770971ff33b49f2bcb3cc912f604b3f5 100644 (file)
@@ -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"