Merge remote-tracking branch 'upstream/pull/5104'

app/abilities/ability.rb

@@ -60,7 +60,10 @@ def initialize(user)
           can [:index, :show, :resolve, :ignore, :reopen], Issue
           can :create, IssueComment
           can [:new, :create, :edit, :update, :destroy], Redaction
-          can [:new, :edit, :create, :update, :revoke, :revoke_all], UserBlock
+          can [:new, :create, :revoke, :revoke_all], UserBlock
+          can :update, UserBlock, :creator => user
+          can :update, UserBlock, :revoker => user
+          can :update, UserBlock, :active? => true
         end
 
         if user.administrator?

app/controllers/user_blocks_controller.rb

@@ -70,8 +70,7 @@ def create
 
   def update
     if @valid_params
-      if current_user != @user_block.creator &&
-         current_user != @user_block.revoker
+      if cannot?(:update, @user_block)
         flash[:error] = t(@user_block.revoker ? ".only_creator_or_revoker_can_edit" : ".only_creator_can_edit")
         redirect_to :action => "edit"
       else
@@ -81,7 +80,10 @@ def update
         @user_block.ends_at = Time.now.utc + @block_period.hours
         @user_block.deactivates_at = (@user_block.ends_at unless @user_block.needs_view)
         @user_block.revoker = current_user if user_block_was_active && !@user_block.active?
-        if !user_block_was_active && @user_block.active?
+        if user_block_was_active && @user_block.active? && current_user != @user_block.creator
+          flash.now[:error] = t(".only_creator_can_edit_without_revoking")
+          render :action => "edit"
+        elsif !user_block_was_active && @user_block.active?
           flash.now[:error] = t(".inactive_block_cannot_be_reactivated")
           render :action => "edit"
         else

app/views/user_blocks/_block.html.erb

@@ -15,8 +15,7 @@
     <% end %>
   </td>
   <td><%= link_to t(".show"), block %></td>
-  <td><% if current_user && (current_user.id == block.creator_id ||
-                             current_user.id == block.revoker_id) %><%= link_to t(".edit"), edit_user_block_path(block) %><% end %></td>
+  <td><% if can?(:edit, block) %><%= link_to t(".edit"), edit_user_block_path(block) %><% end %></td>
   <% if can?(:revoke, UserBlock) %>
   <td><% if block.active? %><%= link_to t(".revoke"), revoke_user_block_path(block) %><% end %></td>
   <% end %>

app/views/user_blocks/edit.html.erb

@@ -9,7 +9,7 @@
 <%= bootstrap_form_for(@user_block) do |f| %>
   <%= f.richtext_field :reason, :cols => 80, :rows => 20, :format => @user_block.reason_format %>
 
-  <% if @user_block.active? %>
+  <% if @user_block.active? && @user_block.creator == current_user %>
     <%= f.form_group do %>
       <%= label_tag "user_block_period", t(".period"), :class => "form-label" %>
       <%= select_tag "user_block_period",
@@ -21,14 +21,24 @@
     <%= f.form_group :needs_view do %>
       <%= f.check_box :needs_view %>
     <% end %>
+
+    <%= f.primary %>
   <% else %>
     <div class="alert alert-info">
-      <%= t "user_blocks.update.inactive_block_cannot_be_reactivated" %>
+      <% if @user_block.active? %>
+        <%= t "user_blocks.update.only_creator_can_edit_without_revoking" %>
+      <% else %>
+        <%= t "user_blocks.update.inactive_block_cannot_be_reactivated" %>
+      <% end %>
     </div>
 
     <%= hidden_field_tag "user_block_period", 0 %>
-    <%= f.hidden_field :needs_view %>
-  <% end %>
+    <%= hidden_field_tag "user_block[needs_view]", false %>
 
-  <%= f.primary %>
+    <% if @user_block.active? %>
+      <%= f.submit t(".revoke"), :class => "btn btn-danger" %>
+    <% else %>
+      <%= f.primary %>
+    <% end %>
+  <% end %>
 <% end %>

app/views/user_blocks/show.html.erb

@@ -30,8 +30,7 @@
                        current_user.id == @user_block.revoker_id) ||
       can?(:revoke, UserBlock) && @user_block.active? %>
   <div>
-    <% if current_user && (current_user.id == @user_block.creator_id ||
-                           current_user.id == @user_block.revoker_id) %>
+    <% if can?(:edit, @user_block) %>
       <%= link_to t(".edit"), edit_user_block_path(@user_block), :class => "btn btn-outline-primary" %>
     <% end %>
     <% if can?(:revoke, UserBlock) && @user_block.active? %>

config/locales/en.yml

@@ -2943,12 +2943,14 @@ en:
       title: "Editing block on %{name}"
       heading_html: "Editing block on %{name}"
       period: "How long, starting now, the user will be blocked from the API for."
+      revoke: "Revoke block"
     filter:
       block_period: "The blocking period must be one of the values selectable in the drop-down list."
     create:
       flash: "Created a block on user %{name}."
     update:
       only_creator_can_edit: "Only the moderator who created this block can edit it."
+      only_creator_can_edit_without_revoking: "Only the moderator who created this block can edit it without revoking."
       only_creator_or_revoker_can_edit: "Only the moderators who created or revoked this block can edit it."
       inactive_block_cannot_be_reactivated: "This block is inactive and cannot be reactivated."
       success: "Block updated."

test/abilities/abilities_test.rb

@@ -95,6 +95,53 @@ class ModeratorAbilityTest < AbilityTest
       assert ability.can?(action, DiaryComment), "should be able to #{action} DiaryComment"
     end
   end
+
+  test "Active block update permissions" do
+    creator_user = create(:moderator_user)
+    other_moderator_user = create(:moderator_user)
+    block = create(:user_block, :creator => creator_user)
+
+    creator_ability = Ability.new creator_user
+    assert creator_ability.can?(:edit, block)
+    assert creator_ability.can?(:update, block)
+
+    other_moderator_ability = Ability.new other_moderator_user
+    assert other_moderator_ability.can?(:edit, block)
+    assert other_moderator_ability.can?(:update, block)
+  end
+
+  test "Expired block update permissions" do
+    creator_user = create(:moderator_user)
+    other_moderator_user = create(:moderator_user)
+    block = create(:user_block, :expired, :creator => creator_user)
+
+    creator_ability = Ability.new creator_user
+    assert creator_ability.can?(:edit, block)
+    assert creator_ability.can?(:update, block)
+
+    other_moderator_ability = Ability.new other_moderator_user
+    assert other_moderator_ability.cannot?(:edit, block)
+    assert other_moderator_ability.cannot?(:update, block)
+  end
+
+  test "Revoked block update permissions" do
+    creator_user = create(:moderator_user)
+    revoker_user = create(:moderator_user)
+    other_moderator_user = create(:moderator_user)
+    block = create(:user_block, :revoked, :creator => creator_user, :revoker => revoker_user)
+
+    creator_ability = Ability.new creator_user
+    assert creator_ability.can?(:edit, block)
+    assert creator_ability.can?(:update, block)
+
+    revoker_ability = Ability.new revoker_user
+    assert revoker_ability.can?(:edit, block)
+    assert revoker_ability.can?(:update, block)
+
+    other_moderator_ability = Ability.new other_moderator_user
+    assert other_moderator_ability.cannot?(:edit, block)
+    assert other_moderator_ability.cannot?(:update, block)
+  end
 end
 
 class AdministratorAbilityTest < AbilityTest

test/controllers/user_blocks_controller_test.rb

@@ -173,7 +173,7 @@ def test_active_block_buttons
     block = create(:user_block, :creator => creator_user)
 
     session_for(other_moderator_user)
-    check_block_buttons block, :revoke => 1
+    check_block_buttons block, :edit => 1, :revoke => 1
 
     session_for(creator_user)
     check_block_buttons block, :edit => 1, :revoke => 1
@@ -252,7 +252,9 @@ def test_new
   ##
   # test the edit action
   def test_edit
-    active_block = create(:user_block)
+    creator_user = create(:moderator_user)
+    other_moderator_user = create(:moderator_user)
+    active_block = create(:user_block, :creator => creator_user)
 
     # Check that the block edit page requires us to login
     get edit_user_block_path(:id => active_block)
@@ -266,17 +268,37 @@ def test_edit
     assert_redirected_to :controller => "errors", :action => "forbidden"
 
     # Login as a moderator
-    session_for(create(:moderator_user))
+    session_for(other_moderator_user)
 
     # Check that the block edit page loads for moderators
     get edit_user_block_path(:id => active_block)
     assert_response :success
     assert_select "h1 a[href='#{user_path active_block.user}']", :text => active_block.user.display_name
+    assert_select "form#edit_user_block_#{active_block.id}", :count => 1 do
+      assert_select "textarea#user_block_reason", :count => 1
+      assert_select "select#user_block_period", :count => 0
+      assert_select "input#user_block_needs_view[type='checkbox']", :count => 0
+      assert_select "input[type='submit'][value='Update block']", :count => 0
+      assert_select "input#user_block_period[type='hidden']", :count => 1
+      assert_select "input#user_block_needs_view[type='hidden']", :count => 1
+      assert_select "input[type='submit'][value='Revoke block']", :count => 1
+    end
+
+    # Login as the block creator
+    session_for(creator_user)
+
+    # Check that the block edit page loads for the creator
+    get edit_user_block_path(:id => active_block)
+    assert_response :success
+    assert_select "h1 a[href='#{user_path active_block.user}']", :text => active_block.user.display_name
     assert_select "form#edit_user_block_#{active_block.id}", :count => 1 do
       assert_select "textarea#user_block_reason", :count => 1
       assert_select "select#user_block_period", :count => 1
       assert_select "input#user_block_needs_view[type='checkbox']", :count => 1
       assert_select "input[type='submit'][value='Update block']", :count => 1
+      assert_select "input#user_block_period[type='hidden']", :count => 0
+      assert_select "input#user_block_needs_view[type='hidden']", :count => 0
+      assert_select "input[type='submit'][value='Revoke block']", :count => 0
     end
 
     # We should get an error if the user doesn't exist
@@ -391,7 +413,6 @@ def test_create_duration
   # test the update action
   def test_update
     moderator_user = create(:moderator_user)
-    second_moderator_user = create(:moderator_user)
     active_block = create(:user_block, :creator => moderator_user)
 
     # Not logged in yet, so updating a block should fail
@@ -405,19 +426,7 @@ def test_update
     put user_block_path(:id => active_block)
     assert_redirected_to :controller => "errors", :action => "forbidden"
 
-    # Login as the wrong moderator
-    session_for(second_moderator_user)
-
-    # Check that only the person who created a block can update it
-    assert_no_difference "UserBlock.count" do
-      put user_block_path(:id => active_block,
-                          :user_block_period => "12",
-                          :user_block => { :needs_view => true, :reason => "Vandalism" })
-    end
-    assert_redirected_to edit_user_block_path(active_block)
-    assert_equal "Only the moderator who created this block can edit it.", flash[:error]
-
-    # Login as the correct moderator
+    # Login as the moderator
     session_for(moderator_user)
 
     # A bogus block period should result in an error
@@ -495,26 +504,55 @@ def test_update_revoked
 
   ##
   # test the update action revoking the block
-  def test_revoke_using_update
+  def test_revoke_using_update_by_creator
     moderator_user = create(:moderator_user)
     block = create(:user_block, :creator => moderator_user)
 
     session_for(moderator_user)
     put user_block_path(block,
                         :user_block_period => "24",
                         :user_block => { :needs_view => false, :reason => "Updated Reason" })
+    assert_redirected_to user_block_path(block)
+    assert_equal "Block updated.", flash[:notice]
     block.reload
     assert_predicate block, :active?
     assert_nil block.revoker
 
     put user_block_path(block,
                         :user_block_period => "0",
                         :user_block => { :needs_view => false, :reason => "Updated Reason" })
+    assert_redirected_to user_block_path(block)
+    assert_equal "Block updated.", flash[:notice]
     block.reload
     assert_not_predicate block, :active?
     assert_equal moderator_user, block.revoker
   end
 
+  def test_revoke_using_update_by_other_moderator
+    creator_user = create(:moderator_user)
+    other_moderator_user = create(:moderator_user)
+    block = create(:user_block, :creator => creator_user)
+
+    session_for(other_moderator_user)
+    put user_block_path(block,
+                        :user_block_period => "24",
+                        :user_block => { :needs_view => false, :reason => "Updated Reason" })
+    assert_response :success
+    assert_equal "Only the moderator who created this block can edit it without revoking.", flash[:error]
+    block.reload
+    assert_predicate block, :active?
+    assert_nil block.revoker
+
+    put user_block_path(block,
+                        :user_block_period => "0",
+                        :user_block => { :needs_view => false, :reason => "Updated Reason" })
+    assert_redirected_to user_block_path(block)
+    assert_equal "Block updated.", flash[:notice]
+    block.reload
+    assert_not_predicate block, :active?
+    assert_equal other_moderator_user, block.revoker
+  end
+
   ##
   # test the revoke action
   def test_revoke

test/factories/user_blocks.rb

@@ -7,6 +7,12 @@
     user
     creator :factory => :moderator_user
 
+    trait :zero_hour do
+      now = Time.now.utc
+      created_at { now }
+      ends_at { now }
+    end
+
     trait :needs_view do
       needs_view { true }
       deactivates_at { nil }

test/system/user_blocks_test.rb

@@ -98,4 +98,22 @@ class UserBlocksSystemTest < ApplicationSystemTestCase
     click_on "Update block"
     assert_text(/Reason for block:\s+Editing expired blocks works/)
   end
+
+  test "other moderator can revoke 0-hour blocks" do
+    creator_user = create(:moderator_user)
+    other_moderator_user = create(:moderator_user)
+    block = create(:user_block, :zero_hour, :needs_view, :creator => creator_user, :reason => "Testing revoking 0-hour blocks")
+    sign_in_as(other_moderator_user)
+
+    visit edit_user_block_path(block)
+    assert_field "Reason", :with => "Testing revoking 0-hour blocks"
+    assert_no_select "user_block_period"
+    assert_no_field "Needs view"
+
+    fill_in "Reason", :with => "Revoking 0-hour blocks works"
+    click_on "Revoke block"
+    assert_text(/Revoker:\s+#{Regexp.escape other_moderator_user.display_name}/)
+    assert_text(/Status:\s+Ended/)
+    assert_text(/Reason for block:\s+Revoking 0-hour blocks works/)
+  end
 end