New migration to add better auditing to user_roles and better column names there...

diff --git a/app/controllers/user_blocks_controller.rb b/app/controllers/user_blocks_controller.rb
index 2dcac3e8a3e118732e4ad4d0309601543701300c..d4bf1f8dff6337f2d6171be910dc6387a6267ad9 100644 (file)
--- 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 7e56693df5c4c44ba362183e7717da4f99c7e2ee..9064b811d0a1f3a43270ae1ac2f73c75e1e328ff 100644 (file)
--- 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 (file)
index 0000000..3cd9373
--- /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 36d3df3b407015cfc8b9f8edf052083689b39708..5268446724c9dd587d5c0e46ecbf883191792188 100644 (file)
--- 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 66b2c81ff5d28eaa320ded5afe8da0ccea04b3d3..cd3e613be5b5836835895589b658a62582ce3ceb 100644 (file)
--- 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 789ebddfe6bc0d66646e7d04037b120466e49097..0e2b3a287bcd006f94efaf6b1ba3781c89b59f1d 100644 (file)
--- a/app/views/user_blocks/_block.html.erb
+++ b/app/views/user_blocks/_block.html.erb
@@ -4,21 +4,11 @@
   <% if show_user_name %>
   <td class="<%= c1 %>"><%= link_to h(block.user.display_name), :controller => 'user', :action => 'view', :display_name => block.user.display_name %></td>
   <% end %>
-  <% if show_moderator_name %>
-  <td class="<%= c1 %>"><%= link_to h(block.moderator.display_name), :controller => 'user', :action => 'view', :display_name => block.moderator.display_name %></td>
+  <% if show_creator_name %>
+  <td class="<%= c1 %>"><%= link_to h(block.creator.display_name), :controller => 'user', :action => 'view', :display_name => block.creator.display_name %></td>
   <% end %>
   <td class="<%= c1 %>"><%=h truncate(block.reason) %></td>
-  <td class="<%= c1 %>">
-    <% 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 %>
-  </td>
+  <td class="<%= c1 %>"><%=h block_status(block) %></td>
   <td class="<%= c1 %>">
     <% if block.revoker_id.nil? %>
       <%= t('user_block.partial.not_revoked') %>
@@ -27,7 +17,7 @@
     <% end %>
   </td>
   <td class="<%= c1 %>"><%= link_to t('user_block.partial.show'), block %></td>
-  <td class="<%= c1 %>"><% if @user and @user.id == block.moderator_id and block.active? %><%= link_to t('user_block.partial.edit'), edit_user_block_path(block) %><% end %></td>
+  <td class="<%= c1 %>"><% if @user and @user.id == block.creator_id and block.active? %><%= link_to t('user_block.partial.edit'), edit_user_block_path(block) %><% end %></td>
   <% if show_revoke_link %>
   <td class="<%= c1 %>"><% if block.active? %><%= link_to t('user_block.partial.revoke'), block, :confirm => t('user_block.partial.confirm'), :action => :revoke %><% end %></td>
   <% end %>

diff --git a/app/views/user_blocks/_blocks.html.erb b/app/views/user_blocks/_blocks.html.erb
index 5338535522e45d5a1d548d9bf0a086ed826c706f..fa279e9b0f8806fade14ef477d7a0ba7f7bdb802 100644 (file)
--- a/app/views/user_blocks/_blocks.html.erb
+++ b/app/views/user_blocks/_blocks.html.erb
@@ -3,8 +3,8 @@
     <% if show_user_name %>
     <th><%= t'user_block.partial.display_name' %></th>
     <% end %>
-    <% if show_moderator_name %>
-    <th><%= t'user_block.partial.moderator_name' %></th>
+    <% if show_creator_name %>
+    <th><%= t'user_block.partial.creator_name' %></th>
     <% end %>
     <th><%= t'user_block.partial.reason' %></th>
     <th><%= t'user_block.partial.status' %></th>
@@ -15,5 +15,5 @@
     <th></th>
     <% end %>
   </tr>
-  <%= 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? %>
 </table>

diff --git a/app/views/user_blocks/blocks_by.html.erb b/app/views/user_blocks/blocks_by.html.erb
index aaafb52be3350e4a2942298b14aa73b2dc7f0cfd..d49a74c0a934146f5837e0dec134f242e18c5390 100644 (file)
--- a/app/views/user_blocks/blocks_by.html.erb
+++ b/app/views/user_blocks/blocks_by.html.erb
@@ -1,3 +1,3 @@
 <h1><%= t('user_block.blocks_by.heading', :name => @this_user.display_name) %></h1>
 
-<%= 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 0b3187bdb23357014da0955baae2d4807628a04d..8d468433928001dd83db931a4452864687f88a3e 100644 (file)
--- a/app/views/user_blocks/blocks_on.html.erb
+++ b/app/views/user_blocks/blocks_on.html.erb
@@ -1,3 +1,3 @@
 <h1><%= t('user_block.blocks_on.heading', :name => @this_user.display_name) %></h1>
 
-<%= 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 9318a1dede8a0014580b12444c7924f15e74359b..c9ab16dadeca7ead6cbd4a54109683554fc72fb8 100644 (file)
--- a/app/views/user_blocks/index.html.erb
+++ b/app/views/user_blocks/index.html.erb
@@ -1,3 +1,3 @@
 <h1><%= t('user_block.index.heading') %></h1>
 
-<%= 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 b1a4dea7470dfe189148dbc2ef82030f8d7ec73b..321b145a3907ca39c66911b06f555f42f9a470c8 100644 (file)
--- a/app/views/user_blocks/revoke.html.erb
+++ b/app/views/user_blocks/revoke.html.erb
@@ -1,10 +1,10 @@
 <h1><%= t('user_block.revoke.heading', 
        :block_on => @user_block.user.display_name, 
-        :block_by => @user_block.moderator.display_name) %></h1>
+        :block_by => @user_block.creator.display_name) %></h1>
 
-<% if @user_block.end_at > Time.now %>
+<% if @user_block.ends_at > Time.now %>
 <p><b>
-  <%= 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)) %>
 </b></p>
 
 <% form_for :revoke, :url => { :action => "revoke" } do |f| %>
@@ -20,6 +20,6 @@
 
 <% else %>
 <p>
-  <%= 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)) %>
 </p>
 <% end %>

diff --git a/app/views/user_blocks/show.html.erb b/app/views/user_blocks/show.html.erb
index f8e8b523e3aaf6da60af85132379a8769e03b342..a1123e21e269a6c29a33b529af49ed6319c5d889 100644 (file)
--- a/app/views/user_blocks/show.html.erb
+++ b/app/views/user_blocks/show.html.erb
@@ -1,6 +1,6 @@
 <h1><%= t('user_block.show.heading', 
        :block_on => @user_block.user.display_name, 
-        :block_by => @user_block.moderator.display_name) %></h1>
+        :block_by => @user_block.creator.display_name) %></h1>
 
 <% if @user_block.revoker %>
 <p>
@@ -9,17 +9,7 @@
 </p>
 <% end %>
 
-<p>
-  <% 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 %>
-</p>
-
-<% if @user_block.needs_view %>
-<p><%= t'user_block.show.needs_view' %></p>
-<% end %>
+<p><b><%= t'user_block.show.status' %></b>: <%= block_status(@user_block) %></p>
 
 <p>
   <b><%= t'user_block.show.reason' %></b>
@@ -27,8 +17,8 @@
 </p>
 
 
-<% 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 5459cd1dae07e511dfb17f1dbaa3532df0960d7c..bfa728c1e26ed70bfe4adedcaf40b296989dd4d6 100644 (file)
--- 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 (file)
index 0000000..db0813e
--- /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 fd568daf75945be098cd5ed7c54d9a914b10ab79..a5f81425547c3b4fe2ace25a8e3b017222d9ab29 100644 (file)
--- 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