Tried to DRY the user_blocks controller. Moved the configuration of the blocking...
authorMatt Amos <zerebubuth@gmail.com>
Wed, 30 Sep 2009 15:44:29 +0000 (15:44 +0000)
committerMatt Amos <zerebubuth@gmail.com>
Wed, 30 Sep 2009 15:44:29 +0000 (15:44 +0000)
app/controllers/user_blocks_controller.rb
app/models/user_block.rb
app/views/user_blocks/edit.html.erb
app/views/user_blocks/new.html.erb
config/application.yml
config/locales/en.yml

index d4bf1f8..7d3830c 100644 (file)
@@ -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
index cd3e613..23e1bca 100644 (file)
@@ -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
index 3ca0b90..c311136 100644 (file)
@@ -9,7 +9,6 @@
   </p>
   <p>
     <%= label_tag 'user_block_period', t('user_block.edit.period') %><br />
-    <%= 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])) %>
   </p>
   <p>
index 99f9252..3d0d2d0 100644 (file)
@@ -1,18 +1,10 @@
-<h1><%= t('user_block.new.title', :name => @display_name) %></h1>
+<h1><%= t('user_block.new.title', :name => @this_user.display_name) %></h1>
 
 <% form_for(@user_block) do |f| %>
   <%= f.error_messages %>
 
   <p>
-    <%= check_box_tag 'tried_contacting', 'yes', (params[:tried_contacting] == "yes") %>
-    <%= label_tag 'tried_contacting', t('user_block.new.tried_contacting') %>
-  </p>
-  <p>
-    <%= check_box_tag 'tried_waiting', 'yes', (params[:tried_waiting] == "yes") %>
-    <%= label_tag 'tried_waiting', t('user_block.new.tried_waiting') %>
-  </p>
-  <p>
-    <%= f.label :reason, t('user_block.new.reason', :name => @display_name) %><br />
+    <%= f.label :reason, t('user_block.new.reason', :name => @this_user.display_name) %><br />
     <%= f.text_area :reason %>
   </p>
   <p>
@@ -24,7 +16,7 @@
     <%= f.label :needs_view, t('user_block.new.needs_view') %>
   </p>
   <p>
-    <%= hidden_field_tag 'display_name', @display_name %>
+    <%= hidden_field_tag 'display_name', @this_user.display_name %>
     <%= f.submit t('user_block.new.submit') %>
   </p>
 <% end %>
index 6241fb6..6a67a51 100644 (file)
@@ -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
index bfa728c..363f620 100644 (file)
@@ -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"