feat: 🎸 methods to unban single user and batch-unban multiple users (no interface yet) (#1798)

* feat: 🎸 unban a single user and batch-unban multiple users

* refactor: 💡 using clearer attributes in tests methods

app/controllers/spam_controller.rb

@@ -20,9 +20,14 @@ class SpamController < ApplicationController
       map.publish unless map.status == Map::Status::NORMAL
     end
 
-    def check_and_unban(map)
+    def check_and_unban(resource, type) # toggle between directly unbanning a user or unbanning them via their map
-      # check and unban only banned non-anonymous authors
+      if type == 'map'
-      map.user.unban unless map.anonymous? || map.user.status != User::Status::BANNED
+        # check and unban a map's author as long as the author is banned and non-anonymous
+        resource.user.unban unless resource.anonymous? || resource.user.status != User::Status::BANNED
+      elsif type == 'user'
+        # check and unban only banned authors
+        resource.unban if resource.status == User::Status::BANNED
+      end
     end
   end
 
@@ -61,7 +66,7 @@ class SpamController < ApplicationController
     @map = Map.find(params[:id])
     if check_and_publish(@map)
       notice_text = 'Map published.'
-      notice_text.chop! << ' and author unbanned.' if check_and_unban(@map)
+      notice_text.chop! << ' and author unbanned.' if check_and_unban(@map, 'map')
     else
       notice_text = 'Map already published.'
     end
@@ -76,7 +81,7 @@ class SpamController < ApplicationController
       map = Map.find(id)
       if check_and_publish(map)
         published_maps += 1
-        unbanned_authors += 1 if check_and_unban(map)
+        unbanned_authors += 1 if check_and_unban(map, 'map')
       end
     end
     flash[:notice] = helpers.pluralize(published_maps, 'map') + ' published and ' + helpers.pluralize(unbanned_authors, 'author') + ' unbanned.'
@@ -115,4 +120,26 @@ class SpamController < ApplicationController
     flash[:notice] = helpers.pluralize(banned_authors, 'author') + ' banned.'
     redirect_back(fallback_location: root_path)
   end
+
+  def unban_user
+    @user = User.find(params[:id])
+    notice_text = check_and_unban(@user, 'user') ? 'Author unbanned.' : 'Only banned authors can be unbanned.'
+    flash[:notice] = notice_text
+    redirect_back(fallback_location: root_path)
+  rescue ActiveRecord::RecordNotFound
+    flash[:error] = 'Failed to unban as the user is either anonymous or does not exist on MapKnitter.'
+    redirect_back(fallback_location: root_path)
+  end
+
+  def batch_unban_users
+    unbanned_authors = 0
+    params[:ids].split(',').uniq.each do |id|
+      author = User.find_by_id(id)
+      if author && check_and_unban(author, 'user')
+        unbanned_authors += 1
+      end
+    end
+    flash[:notice] = helpers.pluralize(unbanned_authors, 'author') + ' unbanned.'
+    redirect_back(fallback_location: root_path)
+  end
 end

config/routes.rb

@@ -124,11 +124,11 @@ Mapknitter::Application.routes.draw do
   get '/warps/:map/:file(.:format)', to: redirect('https://archive.publiclab.org/warps/%{map}/%{file}.%{format}')
 
   scope 'moderate', module: 'spam' do
-    %w(spam_map publish_map ban_user).each do |action|
+    %w(spam_map publish_map ban_user unban_user).each do |action|
       patch action + '/:id', action: action, as: action
     end
 
-    %w(batch_spam_maps batch_publish_maps batch_ban_users).each do |action|
+    %w(batch_spam_maps batch_publish_maps batch_ban_users batch_unban_users).each do |action|
       patch action + '/:ids', action: action, as: action
     end
 

test/controllers/spam_controller_test.rb

@@ -8,9 +8,9 @@ class SpamControllerTest < ActionController::TestCase
     @users = [users(:chris)]
   end
 
-  def maps_custom_setup(spam_maps)
+  def maps_custom_setup(spam_maps: true)
     if spam_maps
-      @maps.collect { |map|
+      @maps.uniq.collect { |map|
         map.spam
         map.user.ban
       }
@@ -18,7 +18,12 @@ class SpamControllerTest < ActionController::TestCase
     @map_ids = @maps.collect(&:id).join(',')
   end
 
-  def users_custom_setup
+  def users_custom_setup(ban_users: true)
+    if ban_users
+      @users.uniq.collect { |user|
+        user.ban
+      }
+    end
     @user_ids = @users.collect(&:id).join(',')
   end 
 
@@ -32,7 +37,7 @@ class SpamControllerTest < ActionController::TestCase
   end
 
   test 'should not moderate maps if user is not an admin or a moderator' do
-    maps_custom_setup(false)
+    maps_custom_setup(spam_maps: false)
     session[:user_id] = 1
     patch(:batch_spam_maps, params: { ids: @map_ids })
 
@@ -94,7 +99,7 @@ class SpamControllerTest < ActionController::TestCase
 
   test 'should batch-spam maps and ban non-anonymous authors' do
     @maps << maps(:village)
-    maps_custom_setup(false)
+    maps_custom_setup(spam_maps: false)
     session[:user_id] = 2
     patch(:batch_spam_maps, params: { ids: @map_ids })
     
@@ -108,7 +113,7 @@ class SpamControllerTest < ActionController::TestCase
 
   test 'should batch-spam maps and not ban anonymous authors' do
     @maps << maps(:yaya)
-    maps_custom_setup(false)
+    maps_custom_setup(spam_maps: false)
     session[:user_id] = 2
     patch(:batch_spam_maps, params: { ids: @map_ids })
     
@@ -122,7 +127,7 @@ class SpamControllerTest < ActionController::TestCase
 
   test 'should not batch-spam a duplicate map' do
     @maps << maps(:cubbon)
-    maps_custom_setup(false)
+    maps_custom_setup(spam_maps: false)
     session[:user_id] = 2
     patch(:batch_spam_maps, params: { ids: @map_ids })
     
@@ -135,7 +140,7 @@ class SpamControllerTest < ActionController::TestCase
   end
 
   test 'should not batch-spam already-spammed maps' do
-    maps_custom_setup(true)
+    maps_custom_setup
     assert_equal 0, @maps[0].status
     assert_equal 0, @maps[0].user.status
 
@@ -150,7 +155,7 @@ class SpamControllerTest < ActionController::TestCase
 
   test 'should batch-spam maps and skip banning of authors already banned' do
     @maps << @map
-    maps_custom_setup(false)
+    maps_custom_setup(spam_maps: false)
     session[:user_id] = 2
     patch(:batch_spam_maps, params: { ids: @map_ids })
     
@@ -223,7 +228,7 @@ class SpamControllerTest < ActionController::TestCase
 
   test 'should batch-publish maps and unban non-anonymous banned authors' do
     @maps << maps(:village)
-    maps_custom_setup(true)
+    maps_custom_setup
 
     assert @maps.all? { |map| map.status == 0 }
     assert @maps.all? { |map| map.user.status == 0 }
@@ -246,7 +251,7 @@ class SpamControllerTest < ActionController::TestCase
 
     assert @maps.all? { |map| map.status == 0 }
 
-    maps_custom_setup(false)
+    maps_custom_setup(spam_maps: false)
     session[:user_id] = 2
     patch(:batch_publish_maps, params: { ids: @map_ids })
     
@@ -260,7 +265,7 @@ class SpamControllerTest < ActionController::TestCase
 
   test 'should batch-publish maps and skip banning of authors already unbanned' do
     @maps << @map
-    maps_custom_setup(true)
+    maps_custom_setup
 
     assert @maps.all? { |map| map.status == 0 }
     assert @maps.all? { |map| map.user.status == 0 }
@@ -278,7 +283,7 @@ class SpamControllerTest < ActionController::TestCase
 
   test 'should not batch-publish a duplicate map' do
     @maps << maps(:cubbon)
-    maps_custom_setup(true)
+    maps_custom_setup
 
     assert @maps.uniq.one? { |map| map.status == 0 }
     assert @maps.uniq.one? { |map| map.user.status == 0 }
@@ -295,7 +300,7 @@ class SpamControllerTest < ActionController::TestCase
   end
 
   test 'should not batch-publish already-published maps' do
-    maps_custom_setup(false)
+    maps_custom_setup(spam_maps: false)
     assert_equal 1, @maps[0].status
     assert_equal 1, @maps[0].user.status
 
@@ -312,7 +317,7 @@ class SpamControllerTest < ActionController::TestCase
     @maps << maps(:yaya)
     all_maps_count = Map.count
 
-    maps_custom_setup(false)
+    maps_custom_setup(spam_maps: false)
     session[:user_id] = 2
     delete(:batch_delete_maps, params: { ids: @map_ids })
     
@@ -327,7 +332,7 @@ class SpamControllerTest < ActionController::TestCase
     @maps << maps(:cubbon)
     all_maps_count = Map.count
 
-    maps_custom_setup(false)
+    maps_custom_setup(spam_maps: false)
     session[:user_id] = 2
     delete(:batch_delete_maps, params: { ids: @map_ids })
     
@@ -338,7 +343,7 @@ class SpamControllerTest < ActionController::TestCase
     assert_equal Map.count, all_maps_count - 1
   end
 
-  test 'should ban a user' do
+  test 'should ban an unbanned user' do
     session[:user_id] = 2
     assert_equal @user.status, User::Status::NORMAL
 
@@ -374,7 +379,7 @@ class SpamControllerTest < ActionController::TestCase
 
   test 'should batch-ban users' do
     @users << @user
-    users_custom_setup
+    users_custom_setup(ban_users: false)
 
     session[:user_id] = 2
     patch(:batch_ban_users, params: { ids: @user_ids })
@@ -388,7 +393,7 @@ class SpamControllerTest < ActionController::TestCase
 
   test 'should not batch-ban a duplicate user' do
     @users << users(:chris)
-    users_custom_setup
+    users_custom_setup(ban_users: false)
 
     session[:user_id] = 2
     patch(:batch_ban_users, params: { ids: @user_ids })
@@ -401,10 +406,9 @@ class SpamControllerTest < ActionController::TestCase
   end
 
   test 'should not batch-ban already-banned users' do
-    @users[0].ban
     users_custom_setup
 
-    assert_equal User::Status::BANNED, @users[0].status
+    assert @users.uniq.one? { |user| user.status == User::Status::BANNED }
 
     session[:user_id] = 2
     patch(:batch_ban_users, params: { ids: @user_ids })
@@ -414,4 +418,77 @@ class SpamControllerTest < ActionController::TestCase
     assert_equal '0 authors banned.', flash[:notice]
     assert_redirected_to root_path
   end
+
+  test 'should unban a banned user' do
+    @user.ban
+    assert_equal User::Status::BANNED, @user.status
+
+    session[:user_id] = 2
+    patch(:unban_user, params: { id: @user.id })
+    @user.reload
+
+    assert_equal 'Author unbanned.', flash[:notice]
+    assert_equal User::Status::NORMAL, @user.status
+    assert_redirected_to root_path
+  end
+
+  test 'should not unban a non-existent author' do
+    session[:user_id] = 2
+    patch(:unban_user, params: { id: 22 })
+    
+    assert_equal 'Failed to unban as the user is either anonymous or does not exist on MapKnitter.', flash[:error]
+    assert_redirected_to root_path
+  end
+
+  test "should not unban a user that hasn't been banned in the first place" do
+    assert_equal User::Status::NORMAL, @user.status
+
+    session[:user_id] = 2
+    patch(:unban_user, params: { id: @user.id })
+
+    assert_equal 'Only banned authors can be unbanned.', flash[:notice]
+    assert_redirected_to root_path
+  end
+
+  test 'should batch-unban users' do
+    @users << @user
+    users_custom_setup
+
+    session[:user_id] = 2
+    patch(:batch_unban_users, params: { ids: @user_ids })
+    
+    assert_equal 2, @users.length
+    assert_equal 2, @users.uniq.length
+    assert_equal '2 authors unbanned.', flash[:notice]
+    assert @users.all? { |user| user.reload.status == User::Status::NORMAL }
+    assert_redirected_to root_path
+  end
+
+  test 'should not batch-unban a duplicate user' do
+    @users << users(:chris)
+    users_custom_setup
+
+    session[:user_id] = 2
+    patch(:batch_unban_users, params: { ids: @user_ids })
+    
+    assert_equal 2, @users.length
+    assert_equal 1, @users.uniq.length
+    assert_equal '1 author unbanned.', flash[:notice]
+    assert @users.uniq.one? { |user| user.reload.status == User::Status::NORMAL }
+    assert_redirected_to root_path
+  end
+
+  test 'should not batch-unban unbanned users' do
+    users_custom_setup(ban_users: false)
+
+    assert @users.uniq.one? { |user| user.status == User::Status::NORMAL }
+
+    session[:user_id] = 2
+    patch(:batch_unban_users, params: { ids: @user_ids })
+    
+    assert_equal 1, @maps.length
+    assert_equal 1, @maps.uniq.length
+    assert_equal '0 authors unbanned.', flash[:notice]
+    assert_redirected_to root_path
+  end
 end

test/integration/routes_test.rb

@@ -29,4 +29,12 @@ class RoutesTest < ActionDispatch::IntegrationTest
   test "test batch-ban-users route" do
     assert_routing({ path: '/moderate/batch_ban_users/1,2', method: :patch }, { controller: 'spam', action: 'batch_ban_users', ids: '1,2' })
   end
+
+  test "test single-unban-user route" do
+    assert_routing({ path: '/moderate/unban_user/1', method: :patch }, { controller: 'spam', action: 'unban_user', id: '1' })
+  end
+
+  test "test batch-unban-users route" do
+    assert_routing({ path: '/moderate/batch_unban_users/1,2', method: :patch }, { controller: 'spam', action: 'batch_unban_users', ids: '1,2' })
+  end
 end