Merge pull request #3454 from evazion/fix-3448

Fix #3448: Lower the limit for tag category changes

app/controllers/tags_controller.rb

@@ -36,7 +36,6 @@ class TagsController < ApplicationController
     @tag = Tag.find(params[:id])
     check_privilege(@tag)
     @tag.update_attributes(params[:tag], :as => CurrentUser.role)
-    @tag.update_category_cache_for_all
     respond_with(@tag)
   end
 

app/logical/alias_and_implication_importer.rb

@@ -117,7 +117,6 @@ private
           tag = Tag.find_by_name(token[1])
           tag.category = Tag.categories.value_for(token[2])
           tag.save
-          tag.update_category_cache_for_all
 
         else
           raise "Unknown token: #{token[0]}"

app/models/tag.rb

@@ -15,6 +15,8 @@ class Tag < ApplicationRecord
   validates :name, uniqueness: true, tag_name: true, on: :create
   validates_inclusion_of :category, in: TagCategory.category_ids
 
+  after_save :update_category_cache_for_all, if: :category_changed?
+
   module ApiMethods
     def to_legacy_json
       return {
@@ -200,7 +202,7 @@ class Tag < ApplicationRecord
       names.map {|x| find_or_create_by_name(x).name}
     end
 
-    def find_or_create_by_name(name, options = {})
+    def find_or_create_by_name(name, creator: CurrentUser.user)
       name = normalize_name(name)
       category = nil
 
@@ -220,9 +222,8 @@ class Tag < ApplicationRecord
           # next few lines if the category is changed.
           tag.update_category_cache
 
-          if category_id != tag.category && !tag.is_locked? && ((CurrentUser.is_builder? && tag.post_count < 10_000) || tag.post_count <= 50)
+          if tag.editable_by?(creator)
-            tag.update_column(:category, category_id)
+            tag.update(category: category_id)
-            tag.update_category_cache_for_all
           end
         end
 
@@ -948,7 +949,10 @@ class Tag < ApplicationRecord
   end
 
   def editable_by?(user)
-    user.is_builder? || (user.is_member? && post_count <= 50)
+    return true if user.is_admin?
+    return true if !is_locked? && user.is_builder? && post_count < 1_000
+    return true if !is_locked? && user.is_member? && post_count < 50
+    return false
   end
 
   include ApiMethods

app/models/tag_alias.rb

@@ -173,7 +173,6 @@ class TagAlias < TagRelationship
   def ensure_category_consistency
     if antecedent_tag.category != consequent_tag.category && antecedent_tag.category != Tag.categories.general
       consequent_tag.update_attribute(:category, antecedent_tag.category)
-      consequent_tag.update_category_cache_for_all
     end
 
     true

test/functional/tags_controller_test.rb

@@ -73,6 +73,14 @@ class TagsControllerTest < ActionController::TestCase
         @tag.reload
         assert_equal(1, @tag.category)
       end
+
+      should "not change category when the tag is too large to be changed by a builder" do
+        @tag.update_columns(post_count: 1001)
+        post :update, {:id => @tag.id, :tag => {:category => "1"}}, {:user_id => @user.id}
+
+        assert_response :forbidden
+        assert_equal(0, @tag.reload.category)
+      end
     end
   end
 end

test/unit/tag_test.rb

@@ -2,8 +2,8 @@ require 'test_helper'
 
 class TagTest < ActiveSupport::TestCase
   setup do
-    user = FactoryGirl.create(:builder_user)
+    @builder = FactoryGirl.create(:builder_user)
-    CurrentUser.user = user
+    CurrentUser.user = @builder
     CurrentUser.ip_addr = "127.0.0.1"
   end
 
@@ -112,11 +112,9 @@ class TagTest < ActiveSupport::TestCase
 
     should "reset its category after updating" do
       tag = FactoryGirl.create(:artist_tag)
-      tag.update_category_cache_for_all
       assert_equal(Tag.categories.artist, Cache.get("tc:#{Cache.hash(tag.name)}"))
 
       tag.update_attribute(:category, Tag.categories.copyright)
-      tag.update_category_cache_for_all
       assert_equal(Tag.categories.copyright, Cache.get("tc:#{Cache.hash(tag.name)}"))
     end
 
@@ -208,6 +206,20 @@ class TagTest < ActiveSupport::TestCase
       assert_equal(0, tag.category)
     end
 
+    should "not change category when the tag is too large to be changed by a builder" do
+      tag = FactoryGirl.create(:tag, post_count: 1001)
+      Tag.find_or_create_by_name("artist:#{tag.name}", creator: @builder)
+
+      assert_equal(0, tag.reload.category)
+    end
+
+    should "not change category when the tag is too large to be changed by a member" do
+      tag = FactoryGirl.create(:tag, post_count: 51)
+      Tag.find_or_create_by_name("artist:#{tag.name}", creator: FactoryGirl.create(:member_user))
+
+      assert_equal(0, tag.reload.category)
+    end
+
     should "be created when one doesn't exist" do
       assert_difference("Tag.count", 1) do
         tag = Tag.find_or_create_by_name("hoge")