Merge pull request #3247 from evazion/fix-2346

Fix #2346: Artist URLs: auto add 'http', remove non-links

app/controllers/artists_controller.rb

@@ -62,6 +62,7 @@ class ArtistsController < ApplicationController
 
   def update
     @artist.update(params[:artist], :as => CurrentUser.role)
+    flash[:notice] = @artist.valid? ? "Artist updated" : @artist.errors.full_messages.join("; ")
     respond_with(@artist)
   end
 

app/models/artist.rb

@@ -5,12 +5,12 @@ class Artist < ApplicationRecord
   before_create :initialize_creator
   before_validation :normalize_name
   after_save :create_version
-  after_save :save_url_string
   after_save :categorize_tag
   after_save :update_wiki
   validates_uniqueness_of :name
   validate :validate_name
   validate :validate_wiki, :on => :create
+  after_validation :merge_validation_errors
   belongs_to :creator, :class_name => "User"
   has_many :members, :class_name => "Artist", :foreign_key => "group_name", :primary_key => "name"
   has_many :urls, :dependent => :destroy, :class_name => "ArtistUrl"
@@ -64,37 +64,20 @@ class Artist < ApplicationRecord
       urls.map(&:url)
     end
 
-    def save_url_string
-      if @url_string
-        prev = url_array
-        curr = @url_string.scan(/\S+/).uniq
+    def url_string=(string)
+      @url_string_was = url_string
 
-        duplicates = prev.select{|url| prev.count(url) > 1}.uniq
-        duplicates.each do |url|
-          count = prev.count(url)
-          urls.where(:url => url).limit(count-1).destroy_all
-        end
-
-        (prev - curr).each do |url|
-          urls.where(:url => url).destroy_all
-        end
-
-        (curr - prev).each do |url|
-          urls.create(:url => url)
-        end
+      self.urls = string.split(/[[:space:]]+/).uniq.map do |url|
+        self.urls.find_or_initialize_by(url: url)
       end
     end
 
-    def url_string=(string)
-      @url_string = string
-    end
-
     def url_string
-      @url_string || url_array.join("\n")
+      url_array.join("\n")
     end
 
     def url_string_changed?
-      url_string.scan(/\S+/) != url_array
+      @url_string_was != url_string
     end
 
     def map_domain(x)
@@ -553,6 +536,13 @@ 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

@@ -2,6 +2,7 @@ class ArtistUrl < ApplicationRecord
   before_save :initialize_normalized_url, on: [ :create ]
   before_save :normalize
   validates_presence_of :url
+  validate :validate_url_format
   belongs_to :artist, :touch => true
   attr_accessible :url, :artist_id, :normalized_url
 
@@ -65,4 +66,11 @@ class ArtistUrl < ApplicationRecord
   def to_s
     url
   end
+
+  def validate_url_format
+    uri = Addressable::URI.parse(url)
+    errors[:base] << "'#{url}' must begin with http:// or https://" if !uri.scheme.in?(%w[http https])
+  rescue Addressable::URI::InvalidURIError => error
+    errors[:base] << "'#{url}' is malformed: #{error}"
+  end
 end

test/unit/artist_test.rb

@@ -134,6 +134,13 @@ class ArtistTest < ActiveSupport::TestCase
       assert_equal(["http://aaa.com", "http://rembrandt.com/test.jpg"], artist.urls.map(&:to_s).sort)
     end
 
+    should "not allow invalid urls" do
+      artist = FactoryGirl.build(:artist, :url_string => "blah")
+
+      assert_equal(false, artist.valid?)
+      assert_equal(["'blah' must begin with http:// or https://"], artist.errors[:url])
+    end
+
     should "make sure old urls are deleted" do
       artist = FactoryGirl.create(:artist, :name => "rembrandt", :url_string => "http://rembrandt.com/test.jpg")
       artist.url_string = "http://not.rembrandt.com/test.jpg"
@@ -168,11 +175,16 @@ class ArtistTest < ActiveSupport::TestCase
       assert_equal(["minko"], Artist.find_all_by_url("http://minko.com/x/test.jpg").map(&:name))
     end
 
-    should "not allow duplicates" do
+    should "not find duplicates" do
       FactoryGirl.create(:artist, :name => "warhol", :url_string => "http://warhol.com/x/a/image.jpg\nhttp://warhol.com/x/b/image.jpg")
       assert_equal(["warhol"], Artist.find_all_by_url("http://warhol.com/x/test.jpg").map(&:name))
     end
 
+    should "not include duplicate urls" do
+      artist = FactoryGirl.create(:artist, :url_string => "http://foo.com http://foo.com")
+      assert_equal(["http://foo.com"], artist.url_array)
+    end
+
     should "hide deleted artists" do
       FactoryGirl.create(:artist, :name => "warhol", :url_string => "http://warhol.com/a/image.jpg", :is_active => false)
       assert_equal([], Artist.find_all_by_url("http://warhol.com/a/image.jpg").map(&:name))
@@ -407,5 +419,18 @@ class ArtistTest < ActiveSupport::TestCase
       tag.reload
       assert_equal(Tag.categories.artist, tag.category)
     end
+
+    context "when updated" do
+      setup do
+        @artist = FactoryGirl.create(:artist)
+        @artist.stubs(:merge_version?).returns(false)
+      end
+
+      should "create a new version" do
+        assert_difference("@artist.versions.count") do
+          @artist.update(:url_string => "http://foo.com")
+        end
+      end
+    end
   end
 end