From 07a4bdcb21af33ab3b2cfa0c138ef42db965c8c7 Mon Sep 17 00:00:00 2001 From: evazion Date: Sat, 25 Nov 2017 12:42:09 -0600 Subject: [PATCH 1/8] posts: display validation warnings in flash notice. --- app/controllers/posts_controller.rb | 4 ++++ app/controllers/uploads_controller.rb | 7 ++++++- app/models/application_record.rb | 4 ++++ app/models/upload.rb | 6 +++++- app/views/layouts/default.html.erb | 2 +- 5 files changed, 20 insertions(+), 3 deletions(-) diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index eac3b30c2..b16f1fcd8 100644 --- a/app/controllers/posts_controller.rb +++ b/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 diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb index fa2655d9f..6c4466f57 100644 --- a/app/controllers/uploads_controller.rb +++ b/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 diff --git a/app/models/application_record.rb b/app/models/application_record.rb index d0bdfc17e..45af77b23 100644 --- a/app/models/application_record.rb +++ b/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 diff --git a/app/models/upload.rb b/app/models/upload.rb index 4d0704d24..0115f4c46 100644 --- a/app/models/upload.rb +++ b/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 diff --git a/app/views/layouts/default.html.erb b/app/views/layouts/default.html.erb index 968be757e..3da3aed44 100644 --- a/app/views/layouts/default.html.erb +++ b/app/views/layouts/default.html.erb @@ -112,7 +112,7 @@ <% end %>
"> - <%= flash[:notice] %> + <%= format_text(flash[:notice], inline: true) %>. close
From 963eacd849692ff4d81196e4a12717dc1e0e8293 Mon Sep 17 00:00:00 2001 From: evazion Date: Wed, 8 Nov 2017 19:33:02 -0600 Subject: [PATCH 2/8] posts: warn when adding newly created tags. --- app/models/post.rb | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/app/models/post.rb b/app/models/post.rb index 1a25add2e..ebeee8274 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -18,6 +18,7 @@ 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 :post_is_not_its_own_parent validate :updater_can_change_rating before_save :update_tag_post_counts @@ -592,6 +593,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 @@ -1707,6 +1720,17 @@ 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 } + + 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 + end end include FileMethods From d571efd7030b2fdd638da871c96e4ff2e938d6dc Mon Sep 17 00:00:00 2001 From: evazion Date: Wed, 8 Nov 2017 19:37:09 -0600 Subject: [PATCH 3/8] posts: warn when adding artist tag with no artist entry. --- app/models/post.rb | 7 +++++++ app/models/tag.rb | 1 + 2 files changed, 8 insertions(+) diff --git a/app/models/post.rb b/app/models/post.rb index ebeee8274..8795beed2 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -1724,12 +1724,19 @@ class Post < ApplicationRecord 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 end diff --git a/app/models/tag.rb b/app/models/tag.rb index 5f3a3fdc2..1cffddc6a 100644 --- a/app/models/tag.rb +++ b/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" From 4cd372296d7bc713f24dda867dda85f5af731c3a Mon Sep 17 00:00:00 2001 From: evazion Date: Sat, 25 Nov 2017 13:45:17 -0600 Subject: [PATCH 4/8] posts: warn when post is missing copyright tags. --- app/models/post.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/app/models/post.rb b/app/models/post.rb index 8795beed2..ec929b874 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -19,6 +19,7 @@ class Post < ApplicationRecord 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 :has_copyright_tag validate :post_is_not_its_own_parent validate :updater_can_change_rating before_save :update_tag_post_counts @@ -1738,6 +1739,13 @@ class Post < ApplicationRecord end end 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 From 94fa83573360b4c525b3e5331f3877d5215c58ea Mon Sep 17 00:00:00 2001 From: evazion Date: Sat, 25 Nov 2017 14:44:22 -0600 Subject: [PATCH 5/8] posts: warn when post from known source is missing an artist tag. --- app/models/post.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/app/models/post.rb b/app/models/post.rb index ec929b874..cf0ef3511 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -19,6 +19,7 @@ class Post < ApplicationRecord 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 :has_artist_tag validate :has_copyright_tag validate :post_is_not_its_own_parent validate :updater_can_change_rating @@ -1740,6 +1741,17 @@ class Post < ApplicationRecord end end + def has_artist_tag + return if !new_record? + return if source !~ %r!\Ahttps?://! + return if has_tag?("artist_request") || 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:]]. 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 } From cc1f8ab9ed58d145dd9ac5f78e9f33fd7cf384ad Mon Sep 17 00:00:00 2001 From: evazion Date: Sat, 25 Nov 2017 17:02:16 -0600 Subject: [PATCH 6/8] posts: warn when a tag cannot be removed due to implications / automatic tags. --- app/models/post.rb | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/app/models/post.rb b/app/models/post.rb index cf0ef3511..3d7e56f75 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -19,6 +19,7 @@ class Post < ApplicationRecord 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 @@ -648,12 +649,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 @@ -702,10 +709,10 @@ class Post < ApplicationRecord end def remove_negated_tags(tags) - negated_tags, tags = tags.partition {|x| x =~ /\A-/i} - negated_tags = negated_tags.map {|x| x[1..-1]} - negated_tags = TagAlias.to_aliased(negated_tags) - return tags - negated_tags + @negated_tags, tags = tags.partition {|x| x =~ /\A-/i} + @negated_tags = @negated_tags.map {|x| x[1..-1]} + @negated_tags = TagAlias.to_aliased(@negated_tags) + return tags - @negated_tags end def add_automatic_tags(tags) @@ -1741,6 +1748,16 @@ class Post < ApplicationRecord 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?://! From bc3e2438d91940383c9a8c87bac46948b31d900f Mon Sep 17 00:00:00 2001 From: evazion Date: Sat, 25 Nov 2017 17:02:58 -0600 Subject: [PATCH 7/8] posts: add tests for warning validations. --- test/unit/post_test.rb | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/test/unit/post_test.rb b/test/unit/post_test.rb index 42deb0b7f..bd58894a0 100644 --- a/test/unit/post_test.rb +++ b/test/unit/post_test.rb @@ -1546,6 +1546,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 From 6d9708a22e310d57f35d8c44a71307addce73533 Mon Sep 17 00:00:00 2001 From: evazion Date: Sun, 26 Nov 2017 11:42:00 -0600 Subject: [PATCH 8/8] posts: don't warn about missing artist tags for official_art. --- app/models/post.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/models/post.rb b/app/models/post.rb index 3d7e56f75..ff8469d90 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -1761,7 +1761,8 @@ class Post < ApplicationRecord def has_artist_tag return if !new_record? return if source !~ %r!\Ahttps?://! - return if has_tag?("artist_request") || tags.any? { |t| t.category == Tag.categories.artist } + 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:]]. Ask on the forum if you need naming help"