From 03cc3dfa5011aa788e71d41c32fa1a640c1a1098 Mon Sep 17 00:00:00 2001 From: evazion Date: Thu, 4 Oct 2018 17:38:44 -0500 Subject: [PATCH] artists: fix editing invalid urls in artist entries (fix #3720, #3927, #3781) Convert to an autosave association on urls. This ensures that when we save the artist we only validate the added urls, not bad urls that we're trying to remove, and that url validation errors are propagated up to the artist object. This also fixes invalid urls being saved in the artist history despite validation failing (#3720). --- app/models/artist.rb | 47 +++++++++++++++++------------------- app/models/artist_url.rb | 22 +++++------------ test/unit/artist_test.rb | 14 +++++++++-- test/unit/artist_url_test.rb | 11 +++++++-- 4 files changed, 49 insertions(+), 45 deletions(-) diff --git a/app/models/artist.rb b/app/models/artist.rb index e35b9cfff..97aab0095 100644 --- a/app/models/artist.rb +++ b/app/models/artist.rb @@ -2,19 +2,17 @@ class Artist < ApplicationRecord extend Memoist class RevertError < Exception ; end - attribute :url_string, :string, default: nil + attr_accessor :url_string_was + before_validation :normalize_name after_save :create_version after_save :categorize_tag after_save :update_wiki - after_save :save_urls - validates_associated :urls validates :name, tag_name: true, uniqueness: true validate :validate_wiki, :on => :create - after_validation :merge_validation_errors belongs_to_creator has_many :members, :class_name => "Artist", :foreign_key => "group_name", :primary_key => "name" - has_many :urls, :dependent => :destroy, :class_name => "ArtistUrl" + has_many :urls, :dependent => :destroy, :class_name => "ArtistUrl", :autosave => true has_many :versions, -> {order("artist_versions.id ASC")}, :class_name => "ArtistVersion" has_one :wiki_page, :foreign_key => "title", :primary_key => "name" has_one :tag_alias, :foreign_key => "antecedent_name", :primary_key => "name" @@ -175,18 +173,24 @@ class Artist < ApplicationRecord end def url_array - urls.map(&:url) + urls.map(&:to_s) end - def save_urls - if url_string && saved_change_to_url_string? - Artist.transaction do - self.urls.clear - self.urls = url_string.scan(/[^[:space:]]+/).uniq.map do |url| - self.urls.find_or_create_by(url: url) - end - end - end + def url_string + url_array.sort.join("\n") + end + + def url_string=(string) + self.url_string_was = url_string + + self.urls = string.to_s.scan(/[^[:space:]]+/).map do |url| + is_active, url = ArtistUrl.parse_prefix(url) + self.urls.find_or_initialize_by(url: url, is_active: is_active) + end.uniq(&:url) + end + + def url_string_changed? + url_string_was != url_string end def map_domain(x) @@ -253,7 +257,7 @@ class Artist < ApplicationRecord module VersionMethods def create_version(force=false) - if saved_change_to_name? || saved_change_to_url_string? || saved_change_to_is_active? || saved_change_to_is_banned? || saved_change_to_other_names? || saved_change_to_group_name? || saved_change_to_notes? || force + if saved_change_to_name? || url_string_changed? || saved_change_to_is_active? || saved_change_to_is_banned? || saved_change_to_other_names? || saved_change_to_group_name? || saved_change_to_notes? || force if merge_version? merge_version else @@ -268,7 +272,7 @@ class Artist < ApplicationRecord :name => name, :updater_id => CurrentUser.id, :updater_ip_addr => CurrentUser.ip_addr, - :url_string => url_string || url_array.join("\n"), + :url_string => url_string, :is_active => is_active, :is_banned => is_banned, :other_names => other_names, @@ -280,7 +284,7 @@ class Artist < ApplicationRecord prev = versions.last prev.update_attributes( :name => name, - :url_string => url_string || url_array.join("\n"), + :url_string => url_string, :is_active => is_active, :is_banned => is_banned, :other_names => other_names, @@ -546,13 +550,6 @@ class Artist < ApplicationRecord extend SearchMethods include ApiMethods - def merge_validation_errors - errors[:urls].clear - urls.select(&:invalid?).each do |url| - errors[:url] << url.errors.full_messages.join("; ") - end - end - def status if is_banned? && is_active? "Banned" diff --git a/app/models/artist_url.rb b/app/models/artist_url.rb index 14953b796..47e310f96 100644 --- a/app/models/artist_url.rb +++ b/app/models/artist_url.rb @@ -1,5 +1,4 @@ class ArtistUrl < ApplicationRecord - before_validation :parse_prefix before_validation :initialize_normalized_url, on: :create before_validation :normalize validates :url, presence: true, uniqueness: { scope: :artist_id } @@ -9,12 +8,11 @@ class ArtistUrl < ApplicationRecord scope :url_matches, ->(url) { url_attribute_matches(:url, url) } scope :normalized_url_matches, ->(url) { url_attribute_matches(:normalized_url, url) } - def self.strip_prefixes(url) - url.sub(/^[-]+/, "") - end + def self.parse_prefix(url) + prefix, url = url.match(/\A(-)?(.*)/)[1, 2] + is_active = prefix.nil? - def self.is_active?(url) - url !~ /^-/ + [is_active, url] end def self.normalize(url) @@ -88,14 +86,6 @@ class ArtistUrl < ApplicationRecord end end - def parse_prefix - case url - when /^-/ - self.url = url[1..-1] - self.is_active = false - end - end - def priority if normalized_url =~ /pixiv\.net\/member\.php/ 10 @@ -132,8 +122,8 @@ class ArtistUrl < ApplicationRecord def validate_url_format uri = Addressable::URI.parse(url) - errors[:url] << "#{uri} must begin with http:// or https://" if !uri.scheme.in?(%w[http https]) + errors[:url] << "'#{uri}' must begin with http:// or https:// " if !uri.scheme.in?(%w[http https]) rescue Addressable::URI::InvalidURIError => error - errors[:url] << "is malformed: #{error}" + errors[:url] << "'#{uri}' is malformed: #{error}" end end diff --git a/test/unit/artist_test.rb b/test/unit/artist_test.rb index 36c1b7313..307e919ed 100644 --- a/test/unit/artist_test.rb +++ b/test/unit/artist_test.rb @@ -166,9 +166,19 @@ class ArtistTest < ActiveSupport::TestCase end should "not allow invalid urls" do - artist = FactoryBot.create(:artist, :url_string => "blah") + artist = FactoryBot.build(:artist, :url_string => "blah") assert_equal(false, artist.valid?) - assert_equal([" blah must begin with http:// or https://"], artist.errors[:url]) + assert_equal(["'blah' must begin with http:// or https:// "], artist.errors["urls.url"]) + end + + should "allow fixing invalid urls" do + artist = FactoryBot.build(:artist) + artist.urls << FactoryBot.build(:artist_url, url: "www.example.com", normalized_url: "www.example.com") + artist.save(validate: false) + + artist.update(url_string: "http://www.example.com") + assert_equal(true, artist.valid?) + assert_equal("http://www.example.com", artist.urls.map(&:to_s).join) end should "make sure old urls are deleted" do diff --git a/test/unit/artist_url_test.rb b/test/unit/artist_url_test.rb index 3403a8f39..75b8c963f 100644 --- a/test/unit/artist_url_test.rb +++ b/test/unit/artist_url_test.rb @@ -17,10 +17,17 @@ class ArtistUrlTest < ActiveSupport::TestCase end should "allow urls to be marked as inactive" do - url = FactoryBot.create(:artist_url, :url => "-http://monet.com") + url = FactoryBot.create(:artist_url, url: "http://monet.com", is_active: false) assert_equal("http://monet.com", url.url) assert_equal("http://monet.com/", url.normalized_url) - refute(url.is_active?) + assert_equal("-http://monet.com", url.to_s) + end + + should "disallow invalid urls" do + url = FactoryBot.build(:artist_url, url: "www.example.com") + + assert_equal(false, url.valid?) + assert_match(/must begin with http/, url.errors.full_messages.join) end should "always add a trailing slash when normalized" do