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