diff --git a/app/controllers/artists_controller.rb b/app/controllers/artists_controller.rb index 8ba5bb468..f8581237a 100644 --- a/app/controllers/artists_controller.rb +++ b/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 diff --git a/app/models/artist.rb b/app/models/artist.rb index d0e763909..fa9750f56 100644 --- a/app/models/artist.rb +++ b/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" diff --git a/app/models/artist_url.rb b/app/models/artist_url.rb index 9a95ecda5..dc18a3d2a 100644 --- a/app/models/artist_url.rb +++ b/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 diff --git a/test/unit/artist_test.rb b/test/unit/artist_test.rb index b35adff80..afcb9ddfd 100644 --- a/test/unit/artist_test.rb +++ b/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