Merge pull request #3357 from evazion/feat-soft-post-validations

Post editing: add warning when creating new tags (#3352)

app/controllers/posts_controller.rb

@@ -110,6 +110,10 @@ private
   def respond_with_post_after_update(post)
     respond_with(post) do |format|
       format.html do
+        if post.warnings.any?
+          flash[:notice] = post.warnings.full_messages.join(".\n \n")
+        end
+
         if post.errors.any?
           @error_message = post.errors.full_messages.join("; ")
           render :template => "static/error", :status => 500

app/controllers/uploads_controller.rb

@@ -53,7 +53,12 @@ class UploadsController < ApplicationController
 
   def create
     @upload = Upload.create(params[:upload].merge(:server => Socket.gethostname))
-    @upload.process! if @upload.errors.empty?
+
+    if @upload.errors.empty?
+      post = @upload.process!
+      flash[:notice] = post.warnings.full_messages.join(".\n \n") if post.present? && post.warnings.any?
+    end
+
     save_recent_tags
     respond_with(@upload)
   end

app/models/application_record.rb

@@ -43,5 +43,9 @@ class ApplicationRecord < ActiveRecord::Base
     end
   end
 
+  def warnings
+    @warnings ||= ActiveModel::Errors.new(self)
+  end
+
   include ApiMethods
 end

app/models/post.rb

@@ -18,6 +18,10 @@ class Post < ApplicationRecord
   validates_uniqueness_of :md5, :on => :create
   validates_inclusion_of :rating, in: %w(s q e), message: "rating must be s, q, or e"
   validate :tag_names_are_valid
+  validate :added_tags_are_valid
+  validate :removed_tags_are_valid
+  validate :has_artist_tag
+  validate :has_copyright_tag
   validate :post_is_not_its_own_parent
   validate :updater_can_change_rating
   before_save :update_tag_post_counts
@@ -600,6 +604,18 @@ class Post < ApplicationRecord
       @tag_array_was ||= Tag.scan_tags(tag_string_was)
     end
 
+    def tags
+      Tag.where(name: tag_array)
+    end
+
+    def tags_was
+      Tag.where(name: tag_array_was)
+    end
+
+    def added_tags
+      tags - tags_was
+    end
+
     def decrement_tag_post_counts
       Tag.where(:name => tag_array).update_all("post_count = post_count - 1") if tag_array.any?
     end
@@ -641,12 +657,18 @@ class Post < ApplicationRecord
     end
 
     def merge_old_changes
+      @removed_tags = []
+
       if old_tag_string
         # If someone else committed changes to this post before we did,
         # then try to merge the tag changes together.
         current_tags = tag_array_was()
         new_tags = tag_array()
         old_tags = Tag.scan_tags(old_tag_string)
+
+        kept_tags = current_tags & new_tags
+        @removed_tags = old_tags - kept_tags
+
         set_tag_string(((current_tags + new_tags) - old_tags + (current_tags & new_tags)).uniq.sort.join(" "))
       end
 
@@ -695,10 +717,10 @@ class Post < ApplicationRecord
     end
 
     def remove_negated_tags(tags)
-      negated_tags, tags = tags.partition {|x| x =~ /\A-/i}
+      @negated_tags, tags = tags.partition {|x| x =~ /\A-/i}
-      negated_tags = negated_tags.map {|x| x[1..-1]}
+      @negated_tags = @negated_tags.map {|x| x[1..-1]}
-      negated_tags = TagAlias.to_aliased(negated_tags)
+      @negated_tags = TagAlias.to_aliased(@negated_tags)
-      return tags - negated_tags
+      return tags - @negated_tags
     end
 
     def add_automatic_tags(tags)
@@ -1713,6 +1735,53 @@ class Post < ApplicationRecord
         end
       end
     end
+
+    def added_tags_are_valid
+      new_tags = added_tags.select { |t| t.post_count <= 1 }
+      new_general_tags = new_tags.select { |t| t.category == Tag.categories.general }
+      new_artist_tags = new_tags.select { |t| t.category == Tag.categories.artist }
+
+      if new_general_tags.present?
+        n = new_general_tags.size
+        tag_wiki_links = new_general_tags.map { |tag| "[[#{tag.name}]]" }
+        self.warnings[:base] << "Created #{n} new #{n == 1 ? "tag" : "tags"}: #{tag_wiki_links.join(", ")}"
+      end
+
+      new_artist_tags.each do |tag|
+        if tag.artist.blank?
+          self.warnings[:base] << "Artist [[#{tag.name}]] requires an artist entry. \"Create new artist entry\":[/artists/new?name=#{CGI::escape(tag.name)}]"
+        end
+      end
+    end
+
+    def removed_tags_are_valid
+      attempted_removed_tags = @removed_tags + @negated_tags
+      unremoved_tags = tag_array & attempted_removed_tags
+
+      if unremoved_tags.present?
+        unremoved_tags_list = unremoved_tags.map { |t| "[[#{t}]]" }.to_sentence
+        self.warnings[:base] << "#{unremoved_tags_list} could not be removed. Check for implications and try again"
+      end
+    end
+
+    def has_artist_tag
+      return if !new_record?
+      return if source !~ %r!\Ahttps?://!
+      return if has_tag?("artist_request") || has_tag?("official_art")
+      return if tags.any? { |t| t.category == Tag.categories.artist }
+
+      site = Sources::Site.new(source)
+      self.warnings[:base] << "Artist tag is required. Create a new tag with [[artist:<artist_name>]]. Ask on the forum if you need naming help"
+    rescue Sources::Site::NoStrategyError => e
+      # unrecognized source; do nothing.
+    end
+
+    def has_copyright_tag
+      return if !new_record?
+      return if has_tag?("copyright_request") || tags.any? { |t| t.category == Tag.categories.copyright }
+
+      self.warnings[:base] << "Copyright tag is required. Consider adding [[copyright request]] or [[original]]"
+    end
   end
   
   include FileMethods

app/models/tag.rb

@@ -6,6 +6,7 @@ class Tag < ApplicationRecord
   attr_accessible :category, :as => [:moderator, :gold, :platinum, :member, :anonymous, :default, :builder, :admin]
   attr_accessible :is_locked, :as => [:moderator, :admin]
   has_one :wiki_page, :foreign_key => "title", :primary_key => "name"
+  has_one :artist, :foreign_key => "name", :primary_key => "name"
   has_one :antecedent_alias, lambda {active}, :class_name => "TagAlias", :foreign_key => "antecedent_name", :primary_key => "name"
   has_many :consequent_aliases, lambda {active}, :class_name => "TagAlias", :foreign_key => "consequent_name", :primary_key => "name"
   has_many :antecedent_implications, lambda {active}, :class_name => "TagImplication", :foreign_key => "antecedent_name", :primary_key => "name"

app/models/upload.rb

@@ -148,6 +148,8 @@ class Upload < ApplicationRecord
       else
         update_attribute(:status, "error: " + post.errors.full_messages.join(", "))
       end
+
+      post
     end
 
     def process!(force = false)
@@ -155,7 +157,7 @@ class Upload < ApplicationRecord
       return if !force && status =~ /processing|completed|error/
 
       process_upload
-      create_post_from_upload
+      post = create_post_from_upload
 
     rescue Timeout::Error, Net::HTTP::Persistent::Error => x
       if @tries > 3
@@ -164,9 +166,11 @@ class Upload < ApplicationRecord
         @tries += 1
         retry
       end
+      nil
 
     rescue Exception => x
       update_attributes(:status => "error: #{x.class} - #{x.message}", :backtrace => x.backtrace.join("\n"))
+      nil
       
     ensure
       delete_temp_file

app/views/layouts/default.html.erb

@@ -112,7 +112,7 @@
     <% end %>
 
     <div class="ui-corner-all ui-state-highlight" id="notice" style="<%= "display: none;" unless flash[:notice] %>">
-      <span><%= flash[:notice] %></span>
+      <span><%= format_text(flash[:notice], inline: true) %>.</span>
       <a href="#" id="close-notice-link">close</a>
     </div>
 

test/unit/post_test.rb

@@ -1568,6 +1568,42 @@ class PostTest < ActiveSupport::TestCase
           assert_equal("http://www.hentai-foundry.com/pictures/user/AnimeFlux/219123", @post.normalized_source)
         end
       end
+
+      context "when validating tags" do
+        should "warn when creating a new general tag" do
+          @post.add_tag("tag")
+          @post.save
+
+          assert_match(/Created 1 new tag: \[\[tag\]\]/, @post.warnings.full_messages.join)
+        end
+
+        should "warn when adding an artist tag without an artist entry" do
+          @post.add_tag("artist:bkub")
+          @post.save
+
+          assert_match(/Artist \[\[bkub\]\] requires an artist entry./, @post.warnings.full_messages.join)
+        end
+
+        should "warn when a tag removal failed due to implications or automatic tags" do
+          ti = FactoryGirl.create(:tag_implication, antecedent_name: "cat", consequent_name: "animal")
+          @post.reload
+          @post.update(old_tag_string: @post.tag_string, tag_string: "chen_(cosplay) chen cosplay cat animal")
+          @post.reload
+          @post.update(old_tag_string: @post.tag_string, tag_string: "chen_(cosplay) chen cosplay cat -cosplay")
+
+          assert_match(/\[\[animal\]\] and \[\[cosplay\]\] could not be removed./, @post.warnings.full_messages.join)
+        end
+
+        should "warn when a post from a known source is missing an artist tag" do
+          post = FactoryGirl.build(:post, source: "https://www.pixiv.net/member_illust.php?mode=medium&illust_id=65985331")
+          post.save
+          assert_match(/Artist tag is required/, post.warnings.full_messages.join)
+        end
+
+        should "warn when missing a copyright tag" do
+          assert_match(/Copyright tag is required/, @post.warnings.full_messages.join)
+        end
+      end
     end
   end