From: Matt Amos Date: Wed, 30 Sep 2009 15:44:29 +0000 (+0000) Subject: Tried to DRY the user_blocks controller. Moved the configuration of the blocking... X-Git-Tag: live~6604^2~31 X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/95c3d640a44857b0b92ec3be6dad606e40057c5c Tried to DRY the user_blocks controller. Moved the configuration of the blocking periods into application.yml. --- diff --git a/app/controllers/user_blocks_controller.rb b/app/controllers/user_blocks_controller.rb index d4bf1f8df..7d3830c25 100644 --- a/app/controllers/user_blocks_controller.rb +++ b/app/controllers/user_blocks_controller.rb @@ -3,19 +3,22 @@ class UserBlocksController < ApplicationController before_filter :authorize_web before_filter :set_locale - before_filter :require_user, :only => [:new, :create, :edit, :delete] - before_filter :require_moderator, :only => [:new, :create, :edit, :delete] + before_filter :require_user, :only => [:new, :create, :edit, :update, :revoke] + before_filter :require_moderator, :only => [:create, :update, :revoke] + before_filter :lookup_this_user, :only => [:new, :create, :blocks_on, :blocks_by] + before_filter :lookup_user_block, :only => [:show, :edit, :update, :revoke] + before_filter :require_valid_params, :only => [:create, :update] + before_filter :check_database_readable + before_filter :check_database_writable, :only => [:create, :update, :revoke] def index @user_blocks_pages, @user_blocks = paginate(:user_blocks, - :include => [:user, :creator, :revoker], - :order => "user_blocks.ends_at DESC", - :per_page => 20) + :include => [:user, :creator, :revoker], + :order => "user_blocks.ends_at DESC", + :per_page => 20) end def show - @user_block = UserBlock.find(params[:id]) - if @user and @user.id == @user_block.user_id @user_block.needs_view = false @user_block.save! @@ -24,68 +27,47 @@ class UserBlocksController < ApplicationController def new @user_block = UserBlock.new - @display_name = params[:display_name] - @this_user = User.find_by_display_name(@display_name, :conditions => {:visible => true}) end - # GET /user_blocks/1/edit def edit - @user_block = UserBlock.find(params[:id]) params[:user_block_period] = ((@user_block.ends_at - Time.now.getutc) / 1.hour).ceil.to_s end def create - @display_name = params[:display_name] - @this_user = User.find_by_display_name(@display_name, :conditions => {:visible => true}) - block_period = [UserBlock::PERIODS.max, params[:user_block_period].to_i].min + unless @valid_params + redirect_to :action => "new" + return + end @user_block = UserBlock.new(:user_id => @this_user.id, :creator_id => @user.id, :reason => params[:user_block][:reason], - :ends_at => Time.now.getutc() + block_period.hours, + :ends_at => Time.now.getutc() + @block_period.hours, :needs_view => params[:user_block][:needs_view]) - if (@this_user and @user.moderator? and - params[:tried_contacting] == "yes" and - params[:tried_waiting] == "yes" and - block_period >= 0) - if @user_block.save - flash[:notice] = t('user_block.create.flash', :name => @display_name) - redirect_to @user_block - else - render :action => "new" - end + if @user_block.save + flash[:notice] = t('user_block.create.flash', :name => @this_user.display_name) + redirect_to @user_block else - if !@user.moderator? - flash[:notice] = t('user_block.create.not_a_moderator') - elsif params[:tried_contacting] != "yes" - flash[:notice] = t('user_block.create.try_contacting') - elsif params[:tried_waiting] != "yes" - flash[:notice] = t('user_block.create.try_waiting') - else - flash[:notice] = t('user_block.create.bad_parameters') - end - @display_name = @this_user.nil? ? '' : @this_user.display_name - render :action => "new" end end - def update - @user_block = UserBlock.find(params[:id]) - block_period = [72, params[:user_block_period].to_i].min - + def update + unless @valid_params + redirect_to :action => "edit" + return + end + if @user_block.creator_id != @user.id flash[:notice] = t('user_block.update.only_creator_can_edit') - redirect_to(@user_block) - - elsif !@user_block.active? - flash[:notice] = t('user_block.update.block_expired') - redirect_to(@user_block) + redirect_to :action => "edit" + return + end - elsif @user_block.update_attributes({ :ends_at => Time.now.getutc() + block_period.hours, - :reason => params[:user_block][:reason], - :needs_view => params[:user_block][:needs_view] }) + if @user_block.update_attributes({ :ends_at => Time.now.getutc() + @block_period.hours, + :reason => params[:user_block][:reason], + :needs_view => params[:user_block][:needs_view] }) flash[:notice] = t('user_block.update.success') redirect_to(@user_block) else @@ -96,19 +78,10 @@ class UserBlocksController < ApplicationController ## # revokes the block, setting the end_time to now def revoke - @user_block = UserBlock.find(params[:id]) - - if !@user.moderator? - flash[:notice] = t('user_block.create.not_a_moderator') - redirect_to @user_block - - elsif params[:confirm] - if @user_block.revoke! + if params[:confirm] + if @user_block.revoke! @user flash[:notice] = t'user_block.revoke.flash' redirect_to(@user_block) - else - flash[:notice] = t'user_block.revoke.error' - render :action => "edit" end end end @@ -116,8 +89,6 @@ class UserBlocksController < ApplicationController ## # shows a list of all the blocks on the given user def blocks_on - @this_user = User.find_by_display_name(params[:display_name]) - @user_blocks_pages, @user_blocks = paginate(:user_blocks, :include => [:user, :creator, :revoker], :conditions => {:user_id => @this_user.id}, @@ -128,18 +99,55 @@ class UserBlocksController < ApplicationController ## # shows a list of all the blocks by the given user. def blocks_by - @this_user = User.find_by_display_name(params[:display_name]) - @user_blocks_pages, @user_blocks = paginate(:user_blocks, - :include => [:user, :creator, :revoker], - :conditions => {:creator_id => @this_user.id}, - :order => "user_blocks.ends_at DESC", - :per_page => 20) + :include => [:user, :creator, :revoker], + :conditions => {:creator_id => @this_user.id}, + :order => "user_blocks.ends_at DESC", + :per_page => 20) end private + ## + # require that the user is a moderator, or fill out a helpful error message + # and return them to the login screen where they might be able to login as + # a moderator. def require_moderator - redirect_to "/403.html" unless @user.moderator? + unless @user.moderator? + flash[:notice] = t('user_block.filter.not_a_moderator') + redirect_to :action => 'index' + end + end + + ## + # ensure that there is a "this_user" instance variable + def lookup_this_user + @this_user = User.find_by_display_name(params[:display_name]) + end + + ## + # ensure that there is a "user_block" instance variable + def lookup_user_block + @user_block = UserBlock.find(params[:id]) + end + + ## + # check that the input parameters are valid, setting an instance + # variable if not. note that this doesn't do any redirection, as it's + # called before two different actions, each of which should redirect + # to a different place. + def require_valid_params + @block_period = params[:user_block_period].to_i + @valid_params = false + + if !UserBlock::PERIODS.include?(@block_period) + flash[:notice] = t('user_block.filter.block_period') + + elsif @user_block and !@user_block.active? + flash[:notice] = t('user_block.filter.block_expired') + + else + @valid_params = true + end end end diff --git a/app/models/user_block.rb b/app/models/user_block.rb index cd3e613be..23e1bcab6 100644 --- a/app/models/user_block.rb +++ b/app/models/user_block.rb @@ -5,7 +5,7 @@ class UserBlock < ActiveRecord::Base belongs_to :creator, :class_name => "User", :foreign_key => :creator_id belongs_to :revoker, :class_name => "User", :foreign_key => :revoker_id - PERIODS = [0, 1, 3, 6, 12, 24, 48, 96] + PERIODS = APP_CONFIG['user_block_periods'] ## # returns true if the block is currently active (i.e: the user can't @@ -18,10 +18,9 @@ class UserBlock < ActiveRecord::Base # revokes the block, allowing the user to use the API again. the argument # is the user object who is revoking the ban. def revoke!(revoker) - attrs = { :ends_at => Time.now.getutc(), - :revoker_id => @user.id, - :needs_view => false } - revoker.moderator? and update_attributes(attrs) + update_attributes({ :ends_at => Time.now.getutc(), + :revoker_id => revoker.id, + :needs_view => false }) end private @@ -30,7 +29,7 @@ class UserBlock < ActiveRecord::Base # block. this should be caught and dealt with in the controller, # but i've also included it here just in case. def moderator_permissions - errors.add_to_base("Must be a moderator to create or update a block.") if creator_id_changed? and !creator.moderator? - errors.add_to_base("Must be a moderator to revoke a block.") unless revoker_id.nil? or revoker.moderator? + errors.add_to_base(I18n.t('user_block.model.non_moderator_update')) if creator_id_changed? and !creator.moderator? + errors.add_to_base(I18n.t('user_block.model.non_moderator_revoke')) unless revoker_id.nil? or revoker.moderator? end end diff --git a/app/views/user_blocks/edit.html.erb b/app/views/user_blocks/edit.html.erb index 3ca0b9031..c3111367d 100644 --- a/app/views/user_blocks/edit.html.erb +++ b/app/views/user_blocks/edit.html.erb @@ -9,7 +9,6 @@

<%= label_tag 'user_block_period', t('user_block.edit.period') %>
- <%= hidden_field_tag 'what is the period', params[:user_block_period] %> <%= select_tag('user_block_period', options_for_select(UserBlock::PERIODS.collect { |h| [t('user_block.period', :count => h), h.to_s] }, params[:user_block_period])) %>

diff --git a/app/views/user_blocks/new.html.erb b/app/views/user_blocks/new.html.erb index 99f9252d1..3d0d2d0bf 100644 --- a/app/views/user_blocks/new.html.erb +++ b/app/views/user_blocks/new.html.erb @@ -1,18 +1,10 @@ -

<%= t('user_block.new.title', :name => @display_name) %>

+

<%= t('user_block.new.title', :name => @this_user.display_name) %>

<% form_for(@user_block) do |f| %> <%= f.error_messages %>

- <%= check_box_tag 'tried_contacting', 'yes', (params[:tried_contacting] == "yes") %> - <%= label_tag 'tried_contacting', t('user_block.new.tried_contacting') %> -

-

- <%= check_box_tag 'tried_waiting', 'yes', (params[:tried_waiting] == "yes") %> - <%= label_tag 'tried_waiting', t('user_block.new.tried_waiting') %> -

-

- <%= f.label :reason, t('user_block.new.reason', :name => @display_name) %>
+ <%= f.label :reason, t('user_block.new.reason', :name => @this_user.display_name) %>
<%= f.text_area :reason %>

@@ -24,7 +16,7 @@ <%= f.label :needs_view, t('user_block.new.needs_view') %>

- <%= hidden_field_tag 'display_name', @display_name %> + <%= hidden_field_tag 'display_name', @this_user.display_name %> <%= f.submit t('user_block.new.submit') %>

<% end %> diff --git a/config/application.yml b/config/application.yml index 6241fb621..6a67a5123 100644 --- a/config/application.yml +++ b/config/application.yml @@ -13,6 +13,8 @@ standard_settings: &standard_settings geonames_zoom: 12 # Timeout for API calls in seconds api_timeout: 300 + # Periods (in hours) which are allowed for user blocks + user_block_periods: [0, 1, 3, 6, 12, 24, 48, 96] development: <<: *standard_settings diff --git a/config/locales/en.yml b/config/locales/en.yml index bfa728c1e..363f620f4 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1009,8 +1009,11 @@ en: confirm: "Confirm" fail: "Couldn't revoke role `{{role}}' from user `{{name}}'. Please check that the user and role are both valid." user_block: + model: + non_moderator_update: "Must be a moderator to create or update a block." + non_moderator_revoke: "Must be a moderator to revoke a block." new: - reason: "The reason why {{name}} is being blocked. Please be as calm and as reasonable as possible, giving as much detail as you can about the situation. Bear in mind that not all users understand the community jargon, so please try to use laymans terms." + reason: "The reason why {{name}} is being blocked. Please be as calm and as reasonable as possible, giving as much detail as you can about the situation, remembering that the message will be publicly visible. Bear in mind that not all users understand the community jargon, so please try to use laymans terms." period: "How long, starting now, the user will be blocked from the API for." submit: "Create block" tried_contacting: "I have contacted the user and asked them to stop." @@ -1025,15 +1028,16 @@ en: back: "Back" title: "Editing block on {{name}}" needs_view: "Does the user need to log in before this block will be cleared?" + filter: + not_a_moderator: "You need to be a moderator to perform that action." + block_expired: "The block has already expired and cannot be edited." + block_period: "The blocking period must be one of the values selectable in the drop-down list." create: - not_a_moderator: "User block could not be created: you are not a moderator." try_contacting: "Please try contacting the user before blocking them and giving them a reasonable time to respond." try_waiting: "Please try giving the user a reasonable time to respond before blocking them." - bad_parameters: "Could not create a new block due to bad parameters. Maybe the blocking period is not valid?" flash: "Created a block on user {{name}}." update: only_creator_can_edit: "Only the moderator who created this block can edit it." - block_expired: "The block has already expired and cannot be edited." success: "Block updated." index: heading: "Listing User Blocks"