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

@@ -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"

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

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

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