From ca06b3c7b1a742307e1f6d6b8f809bab2dd0d484 Mon Sep 17 00:00:00 2001 From: Matt Amos Date: Tue, 29 Sep 2009 16:44:03 +0000 Subject: [PATCH 1/1] New migration to add better auditing to user_roles and better column names there and on user_blocks. Added a helper for displaying block status messages. --- app/controllers/user_blocks_controller.rb | 24 +++++++-------- app/controllers/user_roles_controller.rb | 2 +- app/helpers/user_blocks_helper.rb | 20 +++++++++++++ app/models/user.rb | 2 +- app/models/user_block.rb | 8 ++--- app/views/user_blocks/_block.html.erb | 18 +++--------- app/views/user_blocks/_blocks.html.erb | 6 ++-- app/views/user_blocks/blocks_by.html.erb | 2 +- app/views/user_blocks/blocks_on.html.erb | 2 +- app/views/user_blocks/index.html.erb | 2 +- app/views/user_blocks/revoke.html.erb | 8 ++--- app/views/user_blocks/show.html.erb | 18 +++--------- config/locales/en.yml | 10 ++++--- db/migrate/046_alter_user_roles_and_blocks.rb | 29 +++++++++++++++++++ test/fixtures/user_roles.yml | 2 ++ 15 files changed, 93 insertions(+), 60 deletions(-) create mode 100644 app/helpers/user_blocks_helper.rb create mode 100644 db/migrate/046_alter_user_roles_and_blocks.rb diff --git a/app/controllers/user_blocks_controller.rb b/app/controllers/user_blocks_controller.rb index 2dcac3e8a..d4bf1f8df 100644 --- a/app/controllers/user_blocks_controller.rb +++ b/app/controllers/user_blocks_controller.rb @@ -8,8 +8,8 @@ class UserBlocksController < ApplicationController def index @user_blocks_pages, @user_blocks = paginate(:user_blocks, - :include => [:user, :moderator, :revoker], - :order => "user_blocks.end_at DESC", + :include => [:user, :creator, :revoker], + :order => "user_blocks.ends_at DESC", :per_page => 20) end @@ -31,7 +31,7 @@ class UserBlocksController < ApplicationController # GET /user_blocks/1/edit def edit @user_block = UserBlock.find(params[:id]) - params[:user_block_period] = ((@user_block.end_at - Time.now.getutc) / 1.hour).ceil.to_s + params[:user_block_period] = ((@user_block.ends_at - Time.now.getutc) / 1.hour).ceil.to_s end def create @@ -40,9 +40,9 @@ class UserBlocksController < ApplicationController block_period = [UserBlock::PERIODS.max, params[:user_block_period].to_i].min @user_block = UserBlock.new(:user_id => @this_user.id, - :moderator_id => @user.id, + :creator_id => @user.id, :reason => params[:user_block][:reason], - :end_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 @@ -75,7 +75,7 @@ class UserBlocksController < ApplicationController @user_block = UserBlock.find(params[:id]) block_period = [72, params[:user_block_period].to_i].min - if @user_block.moderator_id != @user.id + if @user_block.creator_id != @user.id flash[:notice] = t('user_block.update.only_creator_can_edit') redirect_to(@user_block) @@ -83,7 +83,7 @@ class UserBlocksController < ApplicationController flash[:notice] = t('user_block.update.block_expired') redirect_to(@user_block) - elsif @user_block.update_attributes({ :end_at => Time.now.getutc() + block_period.hours, + 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] }) flash[:notice] = t('user_block.update.success') @@ -119,9 +119,9 @@ class UserBlocksController < ApplicationController @this_user = User.find_by_display_name(params[:display_name]) @user_blocks_pages, @user_blocks = paginate(:user_blocks, - :include => [:user, :moderator, :revoker], + :include => [:user, :creator, :revoker], :conditions => {:user_id => @this_user.id}, - :order => "user_blocks.end_at DESC", + :order => "user_blocks.ends_at DESC", :per_page => 20) end @@ -131,9 +131,9 @@ class UserBlocksController < ApplicationController @this_user = User.find_by_display_name(params[:display_name]) @user_blocks_pages, @user_blocks = paginate(:user_blocks, - :include => [:user, :moderator, :revoker], - :conditions => {:moderator_id => @this_user.id}, - :order => "user_blocks.end_at DESC", + :include => [:user, :creator, :revoker], + :conditions => {:creator_id => @this_user.id}, + :order => "user_blocks.ends_at DESC", :per_page => 20) end diff --git a/app/controllers/user_roles_controller.rb b/app/controllers/user_roles_controller.rb index 7e56693df..9064b811d 100644 --- a/app/controllers/user_roles_controller.rb +++ b/app/controllers/user_roles_controller.rb @@ -10,7 +10,7 @@ class UserRolesController < ApplicationController if params[:nonce] and params[:nonce] == session[:nonce] this_user = User.find_by_display_name(params[:display_name], :conditions => {:visible => true}) if this_user and UserRole::ALL_ROLES.include? params[:role] - this_user.roles.create(:role => params[:role]) + this_user.roles.create(:role => params[:role], :granter_id => @user.id) redirect_to :controller => 'user', :action => 'view', :display_name => params[:display_name] else flash[:notice] = t('user_role.grant.fail', :role => params[:role], :name => params[:display_name]) diff --git a/app/helpers/user_blocks_helper.rb b/app/helpers/user_blocks_helper.rb new file mode 100644 index 000000000..3cd9373c1 --- /dev/null +++ b/app/helpers/user_blocks_helper.rb @@ -0,0 +1,20 @@ +module UserBlocksHelper + ## + # returns a translated string representing the status of the + # user block (i.e: whether it's active, what the expiry time is) + def block_status(block) + if block.active? + if block.needs_view? + I18n.t('user_block.helper.until_login') + else + I18n.t('user_block.helper.time_future', :time => distance_of_time_in_words_to_now(block.ends_at)) + end + else + # the max of the last update time or the ends_at time is when this block finished + # either because the user viewed the block (updated_at) or it expired or was + # revoked (ends_at) + last_time = [block.ends_at, block.updated_at].max + I18n.t('user_block.helper.time_past', :time => distance_of_time_in_words_to_now(last_time)) + end + end +end diff --git a/app/models/user.rb b/app/models/user.rb index 36d3df3b4..526844672 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -14,7 +14,7 @@ class User < ActiveRecord::Base has_many :client_applications has_many :oauth_tokens, :class_name => "OauthToken", :order => "authorized_at desc", :include => [:client_application] - has_many :blocks, :class_name => "UserBlock", :conditions => ["user_blocks.end_at > now() or user_blocks.needs_view"] + has_many :blocks, :class_name => "UserBlock", :conditions => ["user_blocks.ends_at > now() or user_blocks.needs_view"] has_many :roles, :class_name => "UserRole" validates_presence_of :email, :display_name diff --git a/app/models/user_block.rb b/app/models/user_block.rb index 66b2c81ff..cd3e613be 100644 --- a/app/models/user_block.rb +++ b/app/models/user_block.rb @@ -2,7 +2,7 @@ class UserBlock < ActiveRecord::Base validate :moderator_permissions belongs_to :user, :class_name => "User", :foreign_key => :user_id - belongs_to :moderator, :class_name => "User", :foreign_key => :moderator_id + 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] @@ -11,14 +11,14 @@ class UserBlock < ActiveRecord::Base # returns true if the block is currently active (i.e: the user can't # use the API). def active? - needs_view or end_at > Time.now.getutc + needs_view or ends_at > Time.now.getutc end ## # 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 = { :end_at => Time.now.getutc(), + attrs = { :ends_at => Time.now.getutc(), :revoker_id => @user.id, :needs_view => false } revoker.moderator? and update_attributes(attrs) @@ -30,7 +30,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 moderator_id_changed? and !moderator.moderator? + 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? end end diff --git a/app/views/user_blocks/_block.html.erb b/app/views/user_blocks/_block.html.erb index 789ebddfe..0e2b3a287 100644 --- a/app/views/user_blocks/_block.html.erb +++ b/app/views/user_blocks/_block.html.erb @@ -4,21 +4,11 @@ <% if show_user_name %> <%= link_to h(block.user.display_name), :controller => 'user', :action => 'view', :display_name => block.user.display_name %> <% end %> - <% if show_moderator_name %> - <%= link_to h(block.moderator.display_name), :controller => 'user', :action => 'view', :display_name => block.moderator.display_name %> + <% if show_creator_name %> + <%= link_to h(block.creator.display_name), :controller => 'user', :action => 'view', :display_name => block.creator.display_name %> <% end %> <%=h truncate(block.reason) %> - - <% if block.active? %> - <% if block.needs_view? %> - <%= t'user_block.partial.until_login' %> - <% else %> - <%= t('user_block.partial.time_future', :time => distance_of_time_in_words_to_now(block.end_at)) %> - <% end %> - <% else %> - <%= t'user_block.partial.not_active' %> - <% end %> - + <%=h block_status(block) %> <% if block.revoker_id.nil? %> <%= t('user_block.partial.not_revoked') %> @@ -27,7 +17,7 @@ <% end %> <%= link_to t('user_block.partial.show'), block %> - <% if @user and @user.id == block.moderator_id and block.active? %><%= link_to t('user_block.partial.edit'), edit_user_block_path(block) %><% end %> + <% if @user and @user.id == block.creator_id and block.active? %><%= link_to t('user_block.partial.edit'), edit_user_block_path(block) %><% end %> <% if show_revoke_link %> <% if block.active? %><%= link_to t('user_block.partial.revoke'), block, :confirm => t('user_block.partial.confirm'), :action => :revoke %><% end %> <% end %> diff --git a/app/views/user_blocks/_blocks.html.erb b/app/views/user_blocks/_blocks.html.erb index 533853552..fa279e9b0 100644 --- a/app/views/user_blocks/_blocks.html.erb +++ b/app/views/user_blocks/_blocks.html.erb @@ -3,8 +3,8 @@ <% if show_user_name %> <%= t'user_block.partial.display_name' %> <% end %> - <% if show_moderator_name %> - <%= t'user_block.partial.moderator_name' %> + <% if show_creator_name %> + <%= t'user_block.partial.creator_name' %> <% end %> <%= t'user_block.partial.reason' %> <%= t'user_block.partial.status' %> @@ -15,5 +15,5 @@ <% end %> - <%= render :partial => 'block', :locals => {:show_revoke_link => show_revoke_link, :show_user_name => show_user_name, :show_moderator_name => show_moderator_name }, :collection => @user_blocks unless @user_blocks.nil? %> + <%= render :partial => 'block', :locals => {:show_revoke_link => show_revoke_link, :show_user_name => show_user_name, :show_creator_name => show_creator_name }, :collection => @user_blocks unless @user_blocks.nil? %> diff --git a/app/views/user_blocks/blocks_by.html.erb b/app/views/user_blocks/blocks_by.html.erb index aaafb52be..d49a74c0a 100644 --- a/app/views/user_blocks/blocks_by.html.erb +++ b/app/views/user_blocks/blocks_by.html.erb @@ -1,3 +1,3 @@

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

-<%= render :partial => 'blocks', :locals => { :show_revoke_link => (@user and @user.moderator?), :show_user_name => true, :show_moderator_name => false } %> +<%= render :partial => 'blocks', :locals => { :show_revoke_link => (@user and @user.moderator?), :show_user_name => true, :show_creator_name => false } %> diff --git a/app/views/user_blocks/blocks_on.html.erb b/app/views/user_blocks/blocks_on.html.erb index 0b3187bdb..8d4684339 100644 --- a/app/views/user_blocks/blocks_on.html.erb +++ b/app/views/user_blocks/blocks_on.html.erb @@ -1,3 +1,3 @@

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

-<%= render :partial => 'blocks', :locals => { :show_revoke_link => (@user and @user.moderator?), :show_user_name => false, :show_moderator_name => true } %> +<%= render :partial => 'blocks', :locals => { :show_revoke_link => (@user and @user.moderator?), :show_user_name => false, :show_creator_name => true } %> diff --git a/app/views/user_blocks/index.html.erb b/app/views/user_blocks/index.html.erb index 9318a1ded..c9ab16dad 100644 --- a/app/views/user_blocks/index.html.erb +++ b/app/views/user_blocks/index.html.erb @@ -1,3 +1,3 @@

<%= t('user_block.index.heading') %>

-<%= render :partial => 'blocks', :locals => { :show_revoke_link => (@user and @user.moderator?), :show_user_name => true, :show_moderator_name => true } %> +<%= render :partial => 'blocks', :locals => { :show_revoke_link => (@user and @user.moderator?), :show_user_name => true, :show_creator_name => true } %> diff --git a/app/views/user_blocks/revoke.html.erb b/app/views/user_blocks/revoke.html.erb index b1a4dea74..321b145a3 100644 --- a/app/views/user_blocks/revoke.html.erb +++ b/app/views/user_blocks/revoke.html.erb @@ -1,10 +1,10 @@

<%= t('user_block.revoke.heading', :block_on => @user_block.user.display_name, - :block_by => @user_block.moderator.display_name) %>

+ :block_by => @user_block.creator.display_name) %> -<% if @user_block.end_at > Time.now %> +<% if @user_block.ends_at > Time.now %>

- <%= t('user_block.revoke.time_future', :time => distance_of_time_in_words_to_now(@user_block.end_at)) %> + <%= t('user_block.revoke.time_future', :time => distance_of_time_in_words_to_now(@user_block.ends_at)) %>

<% form_for :revoke, :url => { :action => "revoke" } do |f| %> @@ -20,6 +20,6 @@ <% else %>

- <%= t('user_block.revoke.past', :time => distance_of_time_in_words_to_now(@user_block.end_at)) %> + <%= t('user_block.revoke.past', :time => distance_of_time_in_words_to_now(@user_block.ends_at)) %>

<% end %> diff --git a/app/views/user_blocks/show.html.erb b/app/views/user_blocks/show.html.erb index f8e8b523e..a1123e21e 100644 --- a/app/views/user_blocks/show.html.erb +++ b/app/views/user_blocks/show.html.erb @@ -1,6 +1,6 @@

<%= t('user_block.show.heading', :block_on => @user_block.user.display_name, - :block_by => @user_block.moderator.display_name) %>

+ :block_by => @user_block.creator.display_name) %> <% if @user_block.revoker %>

@@ -9,17 +9,7 @@

<% end %> -

- <% if @user_block.end_at > Time.now %> - <%= t('user_block.show.time_future', :time => distance_of_time_in_words_to_now(@user_block.end_at)) %> - <% else %> - <%= t('user_block.show.time_past', :time => distance_of_time_in_words_to_now(@user_block.end_at)) %> - <% end %> -

- -<% if @user_block.needs_view %> -

<%= t'user_block.show.needs_view' %>

-<% end %> +

<%= t'user_block.show.status' %>: <%= block_status(@user_block) %>

<%= t'user_block.show.reason' %> @@ -27,8 +17,8 @@

-<% if @user_block.end_at > Time.now.getutc %> -<% if @user and @user.id == @user_block.moderator_id %> +<% if @user_block.ends_at > Time.now.getutc %> +<% if @user and @user.id == @user_block.creator_id %> <%= link_to t('user_block.show.edit'), edit_user_block_path(@user_block) %> | <% end %> <% if @user and @user.moderator? %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 5459cd1da..bfa728c1e 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1053,14 +1053,15 @@ en: revoke: "Revoke!" confirm: "Are you sure?" display_name: "Blocked User" - moderator_name: "Moderator" + creator_name: "Creator" reason: "Reason for block" status: "Status" revoker_name: "Revoked by" not_revoked: "(not revoked)" - time_future: "Ends in {{time}}" - until_login: "Until the user logs in" - not_active: "(not active)" + helper: + time_future: "Ends in {{time}}." + until_login: "Active until the user logs in." + time_past: "Ended {{time}} ago." blocks_on: heading: "List blocks on {{name}}" blocks_by: @@ -1069,6 +1070,7 @@ en: heading: "Block on {{block_on}} by {{block_by}}" time_future: "Ends in {{time}}" time_past: "Ended {{time}} ago" + status: "Status" show: "Show" edit: "Edit" revoke: "Revoke!" diff --git a/db/migrate/046_alter_user_roles_and_blocks.rb b/db/migrate/046_alter_user_roles_and_blocks.rb new file mode 100644 index 000000000..db0813e33 --- /dev/null +++ b/db/migrate/046_alter_user_roles_and_blocks.rb @@ -0,0 +1,29 @@ +require 'lib/migrate' + +class AlterUserRolesAndBlocks < ActiveRecord::Migration + def self.up + # the initial granter IDs can be "self" - there are none of these + # in the current live DB, but there may be some in people's own local + # copies. + add_column :user_roles, :granter_id, :bigint + UserRole.update_all("granter_id = user_id") + change_column :user_roles, :granter_id, :bigint, :null => false + add_foreign_key :user_roles, [:granter_id], :users, [:id] + + # make sure that [user_id, role] is unique + add_index :user_roles, [:user_id, :role], :name => "user_roles_id_role_unique", :unique => true + + # change the user_blocks to have a creator_id rather than moderator_id + rename_column :user_blocks, :moderator_id, :creator_id + + # change the "end_at" column to the more grammatically correct "ends_at" + rename_column :user_blocks, :end_at, :ends_at + end + + def self.down + remove_column :user_roles, :granter_id + remove_index :user_roles, :name => "user_roles_id_role_unique" + rename_column :user_blocks, :creator_id, :moderator_id + rename_column :user_blocks, :ends_at, :end_at + end +end diff --git a/test/fixtures/user_roles.yml b/test/fixtures/user_roles.yml index fd568daf7..a5f814255 100644 --- a/test/fixtures/user_roles.yml +++ b/test/fixtures/user_roles.yml @@ -3,7 +3,9 @@ administrator: user_id: 6 role: administrator + granter_id: 6 moderator: user_id: 5 role: moderator + granter_id: 6 -- 2.43.2