From 5a54630b572d222b0abea05f3e19e1b1951f0aee Mon Sep 17 00:00:00 2001 From: Tom Hughes Date: Sat, 1 May 2010 16:13:47 +0100 Subject: [PATCH] Add support for suspended and confirmed users Replace the existing "active" and "visible" with an enumerated status that allows for extra cases. Currently we have "suspended" for users who hve triggered the spam detector and "confirmed" for users that have triggered the detector but have been confirmed as vald by an admin. --- app/controllers/application_controller.rb | 2 +- app/controllers/diary_entry_controller.rb | 14 +++++----- app/controllers/user_controller.rb | 32 ++++++++++++++--------- app/models/diary_entry.rb | 2 +- app/models/user.rb | 22 +++++++++++++--- app/models/user_sweeper.rb | 2 +- app/views/user/view.html.erb | 21 ++++++++------- config/locales/en.yml | 1 + config/routes.rb | 1 + db/migrate/051_add_status_to_user.rb | 29 ++++++++++++++++++++ 10 files changed, 89 insertions(+), 37 deletions(-) create mode 100644 db/migrate/051_add_status_to_user.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 0c4117047..eebc9eb28 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -8,7 +8,7 @@ class ApplicationController < ActionController::Base def authorize_web if session[:user] - @user = User.find(session[:user], :conditions => {:visible => true}) + @user = User.find(session[:user], :conditions => {:status => ["active", "confirmed"]}) elsif session[:token] @user = User.authenticate(:token => session[:token]) session[:user] = @user.id diff --git a/app/controllers/diary_entry_controller.rb b/app/controllers/diary_entry_controller.rb index 6c7c9658b..52ce742bf 100644 --- a/app/controllers/diary_entry_controller.rb +++ b/app/controllers/diary_entry_controller.rb @@ -71,7 +71,7 @@ class DiaryEntryController < ApplicationController def list if params[:display_name] - @this_user = User.find_by_display_name(params[:display_name], :conditions => { :visible => true }) + @this_user = User.find_by_display_name(params[:display_name], :conditions => { :status => ["active", "confirmed"] }) if @this_user @title = t 'diary_entry.list.user_title', :user => @this_user.display_name @@ -92,7 +92,7 @@ class DiaryEntryController < ApplicationController @title = t 'diary_entry.list.in_language_title', :language => Language.find(params[:language]).english_name @entry_pages, @entries = paginate(:diary_entries, :include => :user, :conditions => { - :users => { :visible => true }, + :users => { :status => ["active", "confirmed"] }, :visible => true, :language_code => params[:language] }, @@ -102,7 +102,7 @@ class DiaryEntryController < ApplicationController @title = t 'diary_entry.list.title' @entry_pages, @entries = paginate(:diary_entries, :include => :user, :conditions => { - :users => { :visible => true }, + :users => { :status => ["active", "confirmed"] }, :visible => true }, :order => 'created_at DESC', @@ -114,7 +114,7 @@ class DiaryEntryController < ApplicationController request.format = :rss if params[:display_name] - user = User.find_by_display_name(params[:display_name], :conditions => { :visible => true }) + user = User.find_by_display_name(params[:display_name], :conditions => { :status => ["active", "confirmed"] }) if user @entries = DiaryEntry.find(:all, @@ -133,7 +133,7 @@ class DiaryEntryController < ApplicationController elsif params[:language] @entries = DiaryEntry.find(:all, :include => :user, :conditions => { - :users => { :visible => true }, + :users => { :status => ["active", "confirmed"] }, :visible => true, :language_code => params[:language] }, @@ -145,7 +145,7 @@ class DiaryEntryController < ApplicationController else @entries = DiaryEntry.find(:all, :include => :user, :conditions => { - :users => { :visible => true }, + :users => { :status => ["active", "confirmed"] }, :visible => true }, :order => 'created_at DESC', @@ -157,7 +157,7 @@ class DiaryEntryController < ApplicationController end def view - user = User.find_by_display_name(params[:display_name], :conditions => { :visible => true }) + user = User.find_by_display_name(params[:display_name], :conditions => { :status => ["active", "confirmed"] }) if user @entry = DiaryEntry.find(:first, :conditions => { diff --git a/app/controllers/user_controller.rb b/app/controllers/user_controller.rb index 9551ac6d8..839c94e3a 100644 --- a/app/controllers/user_controller.rb +++ b/app/controllers/user_controller.rb @@ -11,8 +11,8 @@ class UserController < ApplicationController before_filter :require_allow_read_prefs, :only => [:api_details] before_filter :require_allow_read_gpx, :only => [:api_gpx_files] before_filter :require_cookies, :only => [:login, :confirm] - before_filter :require_administrator, :only => [:activate, :deactivate, :hide, :unhide, :delete] - before_filter :lookup_this_user, :only => [:activate, :deactivate, :hide, :unhide, :delete] + before_filter :require_administrator, :only => [:activate, :deactivate, :confirm, :hide, :unhide, :delete] + before_filter :lookup_this_user, :only => [:activate, :deactivate, :confirm, :hide, :unhide, :delete] filter_parameter_logging :password, :pass_crypt, :pass_crypt_confirmation @@ -26,7 +26,7 @@ class UserController < ApplicationController else @user = User.new(params[:user]) - @user.visible = true + @user.status = "pending" @user.data_public = true @user.description = "" if @user.description.nil? @user.creation_ip = request.remote_ip @@ -102,7 +102,7 @@ class UserController < ApplicationController @title = t 'user.lost_password.title' if params[:user] and params[:user][:email] - user = User.find_by_email(params[:user][:email], :conditions => {:visible => true}) + user = User.find_by_email(params[:user][:email], :conditions => {:status => ["pending", "active", "confirmed"]}) if user token = user.tokens.create @@ -127,7 +127,7 @@ class UserController < ApplicationController if params[:user] @user.pass_crypt = params[:user][:pass_crypt] @user.pass_crypt_confirmation = params[:user][:pass_crypt_confirmation] - @user.active = true + @user.status = "active" @user.email_valid = true if @user.save @@ -207,7 +207,7 @@ class UserController < ApplicationController token = UserToken.find_by_token(params[:confirm_string]) if token and !token.user.active? @user = token.user - @user.active = true + @user.status = "active" @user.email_valid = true @user.save! referer = token.referer @@ -232,7 +232,6 @@ class UserController < ApplicationController @user = token.user @user.email = @user.new_email @user.new_email = nil - @user.active = true @user.email_valid = true if @user.save flash[:notice] = t 'user.confirm_email.success' @@ -272,7 +271,7 @@ class UserController < ApplicationController def make_friend if params[:display_name] name = params[:display_name] - new_friend = User.find_by_display_name(name, :conditions => {:visible => true}) + new_friend = User.find_by_display_name(name, :conditions => {:status => ["active", "confirmed"]}) friend = Friend.new friend.user_id = @user.id friend.friend_user_id = new_friend.id @@ -298,7 +297,7 @@ class UserController < ApplicationController def remove_friend if params[:display_name] name = params[:display_name] - friend = User.find_by_display_name(name, :conditions => {:visible => true}) + friend = User.find_by_display_name(name, :conditions => {:status => ["active", "confirmed"]}) if @user.is_friends_with?(friend) Friend.delete_all "user_id = #{@user.id} AND friend_user_id = #{friend.id}" flash[:notice] = t 'user.remove_friend.success', :name => friend.display_name @@ -317,28 +316,35 @@ class UserController < ApplicationController ## # activate a user, allowing them to log in def activate - @this_user.update_attributes(:active => true) + @this_user.update_attributes(:status => "active") redirect_to :controller => 'user', :action => 'view', :display_name => params[:display_name] end ## # deactivate a user, preventing them from logging in def deactivate - @this_user.update_attributes(:active => false) + @this_user.update_attributes(:status => "pending") + redirect_to :controller => 'user', :action => 'view', :display_name => params[:display_name] + end + + ## + # confirm a user, overriding any suspension triggered by spam scoring + def confirm + @this_user.update_attributes(:status => "confirmed") redirect_to :controller => 'user', :action => 'view', :display_name => params[:display_name] end ## # hide a user, marking them as logically deleted def hide - @this_user.update_attributes(:visible => false) + @this_user.update_attributes(:status => "deleted") redirect_to :controller => 'user', :action => 'view', :display_name => params[:display_name] end ## # unhide a user, clearing the logically deleted flag def unhide - @this_user.update_attributes(:visible => true) + @this_user.update_attributes(:status => "active") redirect_to :controller => 'user', :action => 'view', :display_name => params[:display_name] end diff --git a/app/models/diary_entry.rb b/app/models/diary_entry.rb index 0524b75cf..9146eb800 100644 --- a/app/models/diary_entry.rb +++ b/app/models/diary_entry.rb @@ -8,7 +8,7 @@ class DiaryEntry < ActiveRecord::Base has_many :visible_comments, :class_name => "DiaryComment", :include => :user, :conditions => { - :users => { :visible => true }, + :users => { :status => ["active", "confirmed" ] }, :visible => true }, :order => "diary_comments.id" diff --git a/app/models/user.rb b/app/models/user.rb index c6a9ad518..23e95bc88 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -7,7 +7,7 @@ class User < ActiveRecord::Base has_many :messages, :foreign_key => :to_user_id, :conditions => { :to_user_visible => true }, :order => 'sent_on DESC' has_many :new_messages, :class_name => "Message", :foreign_key => :to_user_id, :conditions => { :to_user_visible => true, :message_read => false }, :order => 'sent_on DESC' has_many :sent_messages, :class_name => "Message", :foreign_key => :from_user_id, :conditions => { :from_user_visible => true }, :order => 'sent_on DESC' - has_many :friends, :include => :befriendee, :conditions => ["users.visible = ?", true] + has_many :friends, :include => :befriendee, :conditions => "users.status IN ('active', 'confirmed')" has_many :tokens, :class_name => "UserToken" has_many :preferences, :class_name => "UserPreference" has_many :changesets @@ -107,7 +107,8 @@ class User < ActiveRecord::Base bounds = gc.bounds(radius) sql_for_distance = gc.sql_for_distance("home_lat", "home_lon") nearby = User.find(:all, - :conditions => ["id != ? AND visible = ? AND data_public = ? AND #{sql_for_distance} <= ?", id, true, true, radius], :order => sql_for_distance, :limit => num) + :conditions => ["id != ? AND status IN (\'active\', \'confirmed\') AND data_public = ? AND #{sql_for_distance} <= ?", id, true, radius], + :order => sql_for_distance, :limit => num) else nearby = [] end @@ -129,6 +130,18 @@ class User < ActiveRecord::Base return false end + ## + # returns true if a user is visible + def visible? + ["pending","active","confirmed"].include? self.status + end + + ## + # returns true if a user is active + def active? + ["active","confirmed"].include? self.status + end + ## # returns true if the user has the moderator role, false otherwise def moderator? @@ -154,8 +167,9 @@ class User < ActiveRecord::Base active_blocks.detect { |b| b.needs_view? } end + ## + # delete a user - leave the account but purge most personal data def delete - self.active = false self.display_name = "user_#{self.id}" self.description = "" self.home_lat = nil @@ -163,7 +177,7 @@ class User < ActiveRecord::Base self.image = nil self.email_valid = false self.new_email = nil - self.visible = false + self.status = "deleted" self.save end diff --git a/app/models/user_sweeper.rb b/app/models/user_sweeper.rb index 7f172317d..d2fd983f7 100644 --- a/app/models/user_sweeper.rb +++ b/app/models/user_sweeper.rb @@ -14,7 +14,7 @@ private def expire_cache_for(old_record, new_record) if old_record and (new_record.nil? or - old_record.visible != new_record.visible or + old_record.visible? != new_record.visible? or old_record.display_name != new_record.display_name) old_record.diary_entries.each do |entry| expire_action(:controller => 'diary_entry', :action => 'view', :display_name => old_record.display_name, :id => entry.id) diff --git a/app/views/user/view.html.erb b/app/views/user/view.html.erb index b333b5c67..07c31a1e2 100644 --- a/app/views/user/view.html.erb +++ b/app/views/user/view.html.erb @@ -59,19 +59,20 @@ <% end %> <% if @user and @user.administrator? %>
- <% if @this_user.active? %> - <%= link_to t('user.view.deactivate_user'), {:controller => 'user', :action => 'deactivate', :display_name => @this_user.display_name}, {:confirm => t('user.view.confirm')} %> - <% else %> - <%= link_to t('user.view.activate_user'), {:controller => 'user', :action => 'activate', :display_name => @this_user.display_name}, {:confirm => t('user.view.confirm')} %> + <% if ["active", "confirmed"].include? @this_user.status %> + <%= link_to t('user.view.deactivate_user'), {:controller => 'user', :action => 'deactivate', :display_name => @this_user.display_name}, {:confirm => t('user.view.confirm')} %> | + <% elsif ["pending"].include? @this_user.status %> + <%= link_to t('user.view.activate_user'), {:controller => 'user', :action => 'activate', :display_name => @this_user.display_name}, {:confirm => t('user.view.confirm')} %> | <% end %> - | - <% if @this_user.visible? %> - <%= link_to t('user.view.hide_user'), {:controller => 'user', :action => 'hide', :display_name => @this_user.display_name}, {:confirm => t('user.view.confirm')} %> - | - <%= link_to t('user.view.delete_user'), {:controller => 'user', :action => 'delete', :display_name => @this_user.display_name}, {:confirm => t('user.view.confirm')} %> + <% if ["active", "suspended"].include? @this_user.status %> + <%= link_to t('user.view.confirm_user'), {:controller => 'user', :action => 'confirm', :display_name => @this_user.display_name}, {:confirm => t('user.view.confirm')} %> | + <% end %> + <% if ["pending", "active", "confirmed", "suspended"].include? @this_user.status %> + <%= link_to t('user.view.hide_user'), {:controller => 'user', :action => 'hide', :display_name => @this_user.display_name}, {:confirm => t('user.view.confirm')} %> | <% else %> - <%= link_to t('user.view.unhide_user'), {:controller => 'user', :action => 'unhide', :display_name => @this_user.display_name}, {:confirm => t('user.view.confirm')} %> + <%= link_to t('user.view.unhide_user'), {:controller => 'user', :action => 'unhide', :display_name => @this_user.display_name}, {:confirm => t('user.view.confirm')} %> | <% end %> + <%= link_to t('user.view.delete_user'), {:controller => 'user', :action => 'delete', :display_name => @this_user.display_name}, {:confirm => t('user.view.confirm')} %> <% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index ba76a9b4a..ae3b0d566 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1573,6 +1573,7 @@ en: create_block: "block this user" activate_user: "activate this user" deactivate_user: "deactivate this user" + confirm_user: "confirm this user" hide_user: "hide this user" unhide_user: "unhide this user" delete_user: "delete this user" diff --git a/config/routes.rb b/config/routes.rb index 9a86f9200..9a7231041 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -160,6 +160,7 @@ ActionController::Routing::Routes.draw do |map| map.connect '/user/:display_name/account', :controller => 'user', :action => 'account' map.connect '/user/:display_name/activate', :controller => 'user', :action => 'activate' map.connect '/user/:display_name/deactivate', :controller => 'user', :action => 'deactivate' + map.connect '/user/:display_name/confirm', :controller => 'user', :action => 'confirm' map.connect '/user/:display_name/hide', :controller => 'user', :action => 'hide' map.connect '/user/:display_name/unhide', :controller => 'user', :action => 'unhide' map.connect '/user/:display_name/delete', :controller => 'user', :action => 'delete' diff --git a/db/migrate/051_add_status_to_user.rb b/db/migrate/051_add_status_to_user.rb new file mode 100644 index 000000000..cc8a2f238 --- /dev/null +++ b/db/migrate/051_add_status_to_user.rb @@ -0,0 +1,29 @@ +require 'lib/migrate' + +class AddStatusToUser < ActiveRecord::Migration + def self.up + create_enumeration :user_status_enum, ["pending","active","confirmed","suspended","deleted"] + + add_column :users, :status, :user_status_enum, :null => false, :default => "pending" + + User.update_all("status = 'deleted'", { :visible => false }) + User.update_all("status = 'pending'", { :visible => true, :active => 0 }) + User.update_all("status = 'active'", { :visible => true, :active => 1 }) + + remove_column :users, :active + remove_column :users, :visible + end + + def self.down + add_column :users, :visible, :boolean, :default => true, :null => false + add_column :users, :active, :integer, :default => 0, :null => false + + User.update_all("visible = true, active = 1", { :status => "active" }) + User.update_all("visible = true, active = 0", { :status => "pending" }) + User.update_all("visible = false, active = 1", { :status => "deleted" }) + + remove_column :users, :status + + drop_enumeration :user_status_enum + end +end -- 2.43.2