From 0d2e31119db5e2f3a7032b7a92191d758cd9165e Mon Sep 17 00:00:00 2001
From: PeculiarE <75625011+PeculiarE@users.noreply.github.com>
Date: Fri, 1 Jul 2022 12:47:14 +0100
Subject: [PATCH] =?UTF-8?q?feat:=20=F0=9F=8E=B8=20methods=20and=20db=20cha?=
 =?UTF-8?q?nges=20to=20publish=20single=20map=20and=20batch-publish=20mult?=
 =?UTF-8?q?iple=20maps=20(no=20interface=20yet)=20(#1772)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* feat: 🎸 publish single map and batch-publish multiple maps

* style: 💄 implemented code style suggestions & refactored routes

* refactor: 💡 discarded authors' object and used count instead

* refactor: 💡 made method names more readable
---
 app/controllers/application_controller.rb     |   2 +-
 app/controllers/spam_controller.rb            |  64 ++++--
 app/models/map.rb                             |   4 +
 app/models/user.rb                            |   6 +-
 config/routes.rb                              |  11 +-
 ...8190519_add_status_update_time_to_users.rb |   5 +
 db/schema.rb                                  |   3 +-
 test/controllers/spam_controller_test.rb      | 183 ++++++++++++++++--
 test/integration/routes_test.rb               |  10 +-
 test/models/map_test.rb                       |   6 +-
 test/models/user_test.rb                      |  10 +-
 11 files changed, 264 insertions(+), 40 deletions(-)
 create mode 100644 db/migrate/20220628190519_add_status_update_time_to_users.rb

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 806fdd95..2ab3af81 100755
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -53,7 +53,7 @@ class ApplicationController < ActionController::Base
 
   def logged_in_as(roles, action)
     unless current_user && roles.any? { |role| current_user.role == role }
-      flash[:error] = "Only #{roles.collect { |role| role.pluralize }.join(' and ')} can #{action}."
+      flash[:error] = "Only #{roles.collect(&:pluralize).join(" and ")} can #{action}."
       redirect_to('/' + '?_=' + Time.now.to_i.to_s)
     end
   end
diff --git a/app/controllers/spam_controller.rb b/app/controllers/spam_controller.rb
index db05d881..604a19c4 100644
--- a/app/controllers/spam_controller.rb
+++ b/app/controllers/spam_controller.rb
@@ -1,28 +1,36 @@
 class SpamController < ApplicationController
   module ModerationGuards
-    def spam_check(map)
-      #check and spam only unspammed maps
+    def check_and_spam(map)
+      # check and spam only unspammed maps
       map.spam unless map.status == Map::Status::BANNED
     end
-  
-    def ban_check(map)
-      #check and ban only unbanned non-anonymous authors
+
+    def check_and_ban(map)
+      # check and ban only unbanned non-anonymous authors
       map.user.ban unless map.anonymous? || map.user.status == User::Status::BANNED
     end
+
+    def check_and_publish(map)
+      # check and publish only spammed or moderated maps
+      map.publish unless map.status == Map::Status::NORMAL
+    end
+
+    def check_and_unban(map)
+      # check and unban only banned non-anonymous authors
+      map.user.unban unless map.anonymous? || map.user.status != User::Status::BANNED
+    end
   end
 
   include ModerationGuards
 
-  require 'set'
-
   before_action :require_login
   before_action { logged_in_as(['admin', 'moderator'], 'moderate maps and users') }
-  
+
   def spam_map
     @map = Map.find(params[:id])
-    if spam_check(@map)
+    if check_and_spam(@map)
       notice_text = 'Map marked as spam.'
-      notice_text.chop! << ' and author banned.' if ban_check(@map)
+      notice_text.chop! << ' and author banned.' if check_and_ban(@map)
     else
       notice_text = 'Map already marked as spam.'
     end
@@ -30,17 +38,43 @@ class SpamController < ApplicationController
     redirect_back(fallback_location: root_path)
   end
 
-  def batch_spam_map
+  def batch_spam_maps
     spammed_maps = 0
-    banned_authors = Set.new
+    banned_authors = 0
     params[:ids].split(',').uniq.each do |id|
       map = Map.find(id)
-      if spam_check(map)
+      if check_and_spam(map)
         spammed_maps += 1
-        banned_authors << map.user.id if ban_check(map)
+        banned_authors += 1 if check_and_ban(map)
       end
     end
-    flash[:notice] = helpers.pluralize(spammed_maps, 'map') + ' spammed and ' + helpers.pluralize(banned_authors.size, 'author') + ' banned.'
+    flash[:notice] = helpers.pluralize(spammed_maps, 'map') + ' spammed and ' + helpers.pluralize(banned_authors, 'author') + ' banned.'
+    redirect_back(fallback_location: root_path)
+  end
+
+  def publish_map
+    @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)
+    else
+      notice_text = 'Map already published.'
+    end
+    flash[:notice] = notice_text
+    redirect_back(fallback_location: root_path)
+  end
+
+  def batch_publish_maps
+    published_maps = 0
+    unbanned_authors = 0
+    params[:ids].split(',').uniq.each do |id|
+      map = Map.find(id)
+      if check_and_publish(map)
+        published_maps += 1
+        unbanned_authors += 1 if check_and_unban(map)
+      end
+    end
+    flash[:notice] = helpers.pluralize(published_maps, 'map') + ' published and ' + helpers.pluralize(unbanned_authors, 'author') + ' unbanned.'
     redirect_back(fallback_location: root_path)
   end
 end
diff --git a/app/models/map.rb b/app/models/map.rb
index 3443bf8f..bb799b8c 100755
--- a/app/models/map.rb
+++ b/app/models/map.rb
@@ -299,4 +299,8 @@ class Map < ApplicationRecord
   def spam
     update!(status: Status::BANNED)
   end
+
+  def publish
+    update!(status: Status::NORMAL)
+  end
 end
diff --git a/app/models/user.rb b/app/models/user.rb
index cf7539bd..0547ef06 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -65,6 +65,10 @@ class User < ApplicationRecord
   end
 
   def ban
-    update!(status: Status::BANNED)
+    update!({ status: Status::BANNED, status_updated_at: Time.now })
+  end
+
+  def unban
+    update!({ status: Status::NORMAL, status_updated_at: Time.now })
   end
 end
diff --git a/config/routes.rb b/config/routes.rb
index ed4d775e..f395f648 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -123,8 +123,15 @@ Mapknitter::Application.routes.draw do
 
   get '/warps/:map/:file(.:format)', to: redirect('https://archive.publiclab.org/warps/%{map}/%{file}.%{format}')
 
-  patch 'moderate/spam_map/:id' => 'spam#spam_map', as: 'spam_map'
-  patch 'moderate/batch_spam_map/:ids' => 'spam#batch_spam_map', as: 'batch_spam_map'
+  scope 'moderate', module: 'spam' do
+    %w(spam_map publish_map).each do |action|
+      patch action + '/:id', action: action, as: action
+    end
+
+    %w(batch_spam_maps batch_publish_maps).each do |action|
+      patch action + '/:ids', action: action, as: action
+    end
+  end
   
   # See how all your routes lay out with 'rails routes'
 
diff --git a/db/migrate/20220628190519_add_status_update_time_to_users.rb b/db/migrate/20220628190519_add_status_update_time_to_users.rb
new file mode 100644
index 00000000..c74bf694
--- /dev/null
+++ b/db/migrate/20220628190519_add_status_update_time_to_users.rb
@@ -0,0 +1,5 @@
+class AddStatusUpdateTimeToUsers < ActiveRecord::Migration[5.2]
+  def change
+    add_column :users, :status_updated_at, :datetime, null: true
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 3b3cd48d..9b4afba6 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2022_06_19_221926) do
+ActiveRecord::Schema.define(version: 2022_06_28_190519) do
 
   create_table "annotations", options: "ENGINE=InnoDB DEFAULT CHARSET=utf8", force: :cascade do |t|
     t.integer "map_id"
@@ -124,6 +124,7 @@ ActiveRecord::Schema.define(version: 2022_06_19_221926) do
     t.string "remember_token", limit: 40
     t.datetime "remember_token_expires_at"
     t.integer "status", default: 1, null: false
+    t.datetime "status_updated_at"
     t.index ["login"], name: "index_users_on_login", unique: true
   end
 
diff --git a/test/controllers/spam_controller_test.rb b/test/controllers/spam_controller_test.rb
index 17361d0b..23e693df 100644
--- a/test/controllers/spam_controller_test.rb
+++ b/test/controllers/spam_controller_test.rb
@@ -6,8 +6,14 @@ class SpamControllerTest < ActionController::TestCase
     @maps = [maps(:cubbon)]
   end
 
-  def custom_setup
-    @map_ids = @maps.collect { |map| map['id'] }.join(',')
+  def custom_setup(spam_maps)
+    if spam_maps
+      @maps.collect { |map|
+        map.spam
+        map.user.ban
+      }
+    end
+    @map_ids = @maps.collect(&:id).join(',')
   end
 
   test 'should not moderate a map if user not logged in' do
@@ -20,9 +26,9 @@ class SpamControllerTest < ActionController::TestCase
   end
 
   test 'should not moderate maps if user is not an admin or a moderator' do
-    custom_setup
+    custom_setup(false)
     session[:user_id] = 1
-    patch(:batch_spam_map, params: { ids: @map_ids })
+    patch(:batch_spam_maps, params: { ids: @map_ids })
 
     assert_equal 'Only admins and moderators can moderate maps and users.', flash[:error]
     assert_redirected_to ('/' + '?_=' + Time.now.to_i.to_s)
@@ -82,9 +88,9 @@ class SpamControllerTest < ActionController::TestCase
 
   test 'should batch-spam maps and ban non-anonymous authors' do
     @maps << maps(:village)
-    custom_setup
+    custom_setup(false)
     session[:user_id] = 2
-    patch(:batch_spam_map, params: { ids: @map_ids })
+    patch(:batch_spam_maps, params: { ids: @map_ids })
     
     assert_equal @maps.length, 2
     assert_equal @maps.uniq.length, 2
@@ -96,9 +102,9 @@ class SpamControllerTest < ActionController::TestCase
 
   test 'should batch-spam maps and not ban anonymous authors' do
     @maps << maps(:yaya)
-    custom_setup
+    custom_setup(false)
     session[:user_id] = 2
-    patch(:batch_spam_map, params: { ids: @map_ids })
+    patch(:batch_spam_maps, params: { ids: @map_ids })
     
     assert_equal @maps.length, 2
     assert_equal @maps.uniq.length, 2
@@ -110,9 +116,9 @@ class SpamControllerTest < ActionController::TestCase
 
   test 'should not batch-spam a duplicate map' do
     @maps << maps(:cubbon)
-    custom_setup
+    custom_setup(false)
     session[:user_id] = 2
-    patch(:batch_spam_map, params: { ids: @map_ids })
+    patch(:batch_spam_maps, params: { ids: @map_ids })
     
     assert_equal @maps.length, 2
     assert_equal @maps.uniq.length, 1
@@ -123,15 +129,12 @@ class SpamControllerTest < ActionController::TestCase
   end
 
   test 'should not batch-spam already-spammed maps' do
-    @maps[0].spam
-    @maps[0].user.ban
-
+    custom_setup(true)
     assert_equal 0, @maps[0].status
     assert_equal 0, @maps[0].user.status
 
-    custom_setup
     session[:user_id] = 2
-    patch(:batch_spam_map, params: { ids: @map_ids })
+    patch(:batch_spam_maps, params: { ids: @map_ids })
     
     assert_equal @maps.length, 1
     assert_equal @maps.uniq.length, 1
@@ -141,9 +144,9 @@ class SpamControllerTest < ActionController::TestCase
 
   test 'should batch-spam maps and skip banning of authors already banned' do
     @maps << @map
-    custom_setup
+    custom_setup(false)
     session[:user_id] = 2
-    patch(:batch_spam_map, params: { ids: @map_ids })
+    patch(:batch_spam_maps, params: { ids: @map_ids })
     
     assert_equal @maps.length, 2
     assert_equal @maps.uniq.length, 2
@@ -152,4 +155,150 @@ class SpamControllerTest < ActionController::TestCase
     assert @maps.all? { |map| map.user.status == 0 }
     assert_redirected_to root_path
   end
+
+  test 'should publish a spammed map owned by a banned non-anonymous author and unban the author' do
+    @map.spam
+    @map.user.ban
+
+    assert_equal 0, @map.status
+    assert_equal 0, @map.user.status
+
+    session[:user_id] = 2
+    patch(:publish_map, params: { id: @map.id })
+    @map.reload
+  
+    assert_equal 'Map published and author unbanned.', flash[:notice]
+    assert_redirected_to root_path
+    assert_equal 1, @map.status
+    assert_equal 1, @map.user.status
+  end
+
+  test 'should publish a spammed map owned by an unbanned non-anonymous author and skip the unbanning process' do
+    @map.spam
+
+    assert_equal 0, @map.status
+    assert_equal 1, @map.user.status
+
+    session[:user_id] = 2
+    patch(:publish_map, params: { id: @map.id })
+    @map.reload
+  
+    assert_equal 'Map published.', flash[:notice]
+    assert_redirected_to root_path
+    assert_equal 1, @map.status
+    assert_equal 1, @map.user.status
+  end
+
+  test 'should publish a spammed map owned by an anonymous author and skip the unbanning process' do
+    anon_map = maps(:yaya)
+    assert_equal 1, anon_map.status
+
+    anon_map.spam
+    assert_equal 0, anon_map.status
+
+    session[:user_id] = 2
+    patch(:publish_map, params: { id: anon_map.id })
+    anon_map.reload
+    
+    assert_equal 'Map published.', flash[:notice]
+    assert_redirected_to root_path
+    assert_equal 1, anon_map.status
+  end
+
+  test 'should not publish an already published map' do
+    assert_equal 1, @map.status
+
+    session[:user_id] = 2
+    patch(:publish_map, params: { id: @map.id })
+  
+    assert_equal 'Map already published.', flash[:notice]
+    assert_redirected_to root_path
+  end
+
+  test 'should batch-publish maps and unban non-anonymous banned authors' do
+    @maps << maps(:village)
+    custom_setup(true)
+
+    assert @maps.all? { |map| map.status == 0 }
+    assert @maps.all? { |map| map.user.status == 0 }
+
+    session[:user_id] = 2
+    patch(:batch_publish_maps, params: { ids: @map_ids })
+    
+    assert_equal @maps.length, 2
+    assert_equal @maps.uniq.length, 2
+    assert_equal '2 maps published and 2 authors unbanned.', flash[:notice]
+    assert @maps.all? { |map| map.reload.status == 1 }
+    assert @maps.all? { |map| map.user.status == 1 }
+    assert_redirected_to root_path
+  end
+
+  test 'should batch-publish maps and skip unbanning of anonymous authors' do
+    @maps << maps(:yaya)
+    @maps.collect { |map| map.spam }
+    @maps[0].user.ban
+
+    assert @maps.all? { |map| map.status == 0 }
+
+    custom_setup(false)
+    session[:user_id] = 2
+    patch(:batch_publish_maps, params: { ids: @map_ids })
+    
+    assert_equal @maps.length, 2
+    assert_equal @maps.uniq.length, 2
+    assert_equal '2 maps published and 1 author unbanned.', flash[:notice]
+    assert_redirected_to root_path
+    assert @maps.all? { |map| map.reload.status == 1 }
+    assert @maps.one? { |map| map.user.nil? }
+  end
+
+  test 'should batch-publish maps and skip banning of authors already unbanned' do
+    @maps << @map
+    custom_setup(true)
+
+    assert @maps.all? { |map| map.status == 0 }
+    assert @maps.all? { |map| map.user.status == 0 }
+
+    session[:user_id] = 2
+    patch(:batch_publish_maps, params: { ids: @map_ids })
+    
+    assert_equal @maps.length, 2
+    assert_equal @maps.uniq.length, 2
+    assert_equal '2 maps published and 1 author unbanned.', flash[:notice]
+    assert @maps.all? { |map| map.reload.status == 1 }
+    assert @maps.all? { |map| map.user.status == 1 }
+    assert_redirected_to root_path
+  end
+
+  test 'should not batch-publish a duplicate map' do
+    @maps << maps(:cubbon)
+    custom_setup(true)
+
+    assert @maps.uniq.one? { |map| map.status == 0 }
+    assert @maps.uniq.one? { |map| map.user.status == 0 }
+
+    session[:user_id] = 2
+    patch(:batch_publish_maps, params: { ids: @map_ids })
+    
+    assert_equal @maps.length, 2
+    assert_equal @maps.uniq.length, 1
+    assert_equal '1 map published and 1 author unbanned.', flash[:notice]
+    assert @maps.uniq.one? { |map| map.reload.status == 1 }
+    assert @maps.uniq.one? { |map| map.user.status == 1 }
+    assert_redirected_to root_path
+  end
+
+  test 'should not batch-publish already-published maps' do
+    custom_setup(false)
+    assert_equal 1, @maps[0].status
+    assert_equal 1, @maps[0].user.status
+
+    session[:user_id] = 2
+    patch(:batch_publish_maps, params: { ids: @map_ids })
+    
+    assert_equal @maps.length, 1
+    assert_equal @maps.uniq.length, 1
+    assert_equal '0 maps published and 0 authors unbanned.', flash[:notice]
+    assert_redirected_to root_path
+  end
 end
diff --git a/test/integration/routes_test.rb b/test/integration/routes_test.rb
index 1bf74539..f089c6e5 100644
--- a/test/integration/routes_test.rb
+++ b/test/integration/routes_test.rb
@@ -7,6 +7,14 @@ class RoutesTest < ActionDispatch::IntegrationTest
   end
 
   test "test batch-spam-maps route" do
-    assert_routing({ path: '/moderate/batch_spam_map/1,2', method: :patch }, { controller: 'spam', action: 'batch_spam_map', ids: '1,2' })
+    assert_routing({ path: '/moderate/batch_spam_maps/1,2', method: :patch }, { controller: 'spam', action: 'batch_spam_maps', ids: '1,2' })
+  end
+
+  test "test single-publish-map route" do
+    assert_routing({ path: '/moderate/publish_map/2', method: :patch }, { controller: 'spam', action: 'publish_map', id: '2' })
+  end
+
+  test "test batch-publish-maps route" do
+    assert_routing({ path: '/moderate/batch_publish_maps/1,2', method: :patch }, { controller: 'spam', action: 'batch_publish_maps', ids: '1,2' })
   end
 end
diff --git a/test/models/map_test.rb b/test/models/map_test.rb
index 0c0101bc..7b5b8f59 100644
--- a/test/models/map_test.rb
+++ b/test/models/map_test.rb
@@ -98,10 +98,14 @@ class MapTest < ActiveSupport::TestCase
     assert maps.collect(&:name).include?('Saugus Landfill Incinerator')
   end
 
-  test 'should spam map' do
+  test 'should spam and publish map' do
     map = maps(:saugus)
     assert_equal Map::Status::NORMAL, map.status
+
     map.spam
     assert_equal Map::Status::BANNED, map.status
+    
+    map.publish
+    assert_equal Map::Status::NORMAL, map.status
   end
 end
diff --git a/test/models/user_test.rb b/test/models/user_test.rb
index 5f915f03..3aadddec 100644
--- a/test/models/user_test.rb
+++ b/test/models/user_test.rb
@@ -52,11 +52,19 @@ class UserTest < ActiveSupport::TestCase
     assert_equal map_images.flatten, user.warpables
   end
 
-  test 'should ban user' do
+  test 'should ban and unban user' do
     user = users(:quentin)
     assert_equal User::Status::NORMAL, user.status
+    assert_nil user.status_updated_at
+
     user.ban
     assert_equal User::Status::BANNED, user.status
+    assert_not_nil user.status_updated_at
+    old_time = user.status_updated_at
+
+    user.unban
+    assert_equal User::Status::NORMAL, user.status
+    assert user.status_updated_at.to_f >= old_time.to_f
   end
 
   # def test_should_authenticate_user