Merge remote-tracking branch 'upstream/pull/3419'

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,19 @@ 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.unconfirm! if params[:event] == "unconfirm"
+    @user.hide! if params[:event] == "hide"
+    @user.unhide! if params[:event] == "unhide"
+    @user.unsuspend! if params[:event] == "unsuspend"
     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,64 @@ 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.
+    if Rails.env.test?
+      event :deactivate do
+        transitions :from => :active, :to => :pending
+      end
+    end
+
+    # To confirm an account is used to override the spam scoring
+    event :confirm do
+      transitions :from => [:pending, :active, :suspended], :to => :confirmed
+    end
+
+    # To unconfirm an account is to make it subject to future spam scoring again
+    event :unconfirm do
+      transitions :from => :confirmed, :to => :active
+    end
+
+    # Accounts can be automatically suspended by spam_check
+    event :suspend do
+      transitions :from => [:pending, :active], :to => :suspended
+    end
+
+    # Unsuspending an account moves it back to active without overriding the spam scoring
+    event :unsuspend do
+      transitions :from => :suspended, :to => :active
+    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 +300,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 +312,6 @@ class User < ApplicationRecord
     self.new_email = nil
     self.auth_provider = nil
     self.auth_uid = nil
-    self.status = "deleted"
 
     save
   end
@@ -279,7 +337,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,44 @@
         <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_unsuspend? %>
                 <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(".unsuspend_user"), set_status_user_path(:event => "unsuspend", :display_name => @user.display_name), :method => :post, :data => { :confirm => t(".confirm") } %>
+                </li>
+              <% end %>
+
+              <% if @user.may_confirm? %>
+                <li>
+                  <%= 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_unconfirm? %>
+                <li>
+                  <%= link_to t(".unconfirm_user"), set_status_user_path(:event => "unconfirm", :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>

config/locales/en.yml

@@ -2591,6 +2591,8 @@ en:
       activate_user: "Activate this User"
       deactivate_user: "Deactivate this User"
       confirm_user: "Confirm this User"
+      unconfirm_user: "Unconfirm this User"
+      unsuspend_user: "Unsuspend this User"
       hide_user: "Hide this User"
       unhide_user: "Unhide this User"
       delete_user: "Delete this User"

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_status_change_test.rb

@@ -0,0 +1,32 @@
+require "application_system_test_case"
+
+class UserStatusChangeTest < ApplicationSystemTestCase
+  def setup
+    admin = create(:administrator_user)
+    sign_in_as(admin)
+  end
+
+  test "Admin can unsuspend a user" do
+    user = create(:user, :suspended)
+    visit user_path(user)
+    accept_confirm do
+      click_on "Unsuspend"
+    end
+
+    assert_no_content "Unsuspend"
+    user.reload
+    assert_equal "active", user.status
+  end
+
+  test "Admin can confirm a user" do
+    user = create(:user, :suspended)
+    visit user_path(user)
+    accept_confirm do
+      click_on "Confirm"
+    end
+
+    assert_no_content "Unsuspend"
+    user.reload
+    assert_equal "confirmed", user.status
+  end
+end

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"