From bb5f291112a5521cd779efda362e9b533cdf3d87 Mon Sep 17 00:00:00 2001
From: evazion <noizave@gmail.com>
Date: Thu, 4 Oct 2018 19:37:18 -0500
Subject: [PATCH] artists: don't create new version when nothing changed.

Fix an issue where saving an artist entry without changing anything
would create a new artist version.
---
 app/models/artist.rb     | 13 ++++++++-----
 test/unit/artist_test.rb | 37 +++++++++++++++++++++++++++++++++----
 2 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/app/models/artist.rb b/app/models/artist.rb
index 97aab0095..af2b08ce8 100644
--- a/app/models/artist.rb
+++ b/app/models/artist.rb
@@ -2,12 +2,13 @@ class Artist < ApplicationRecord
   extend Memoist
   class RevertError < Exception ; end
 
-  attr_accessor :url_string_was
+  attr_accessor :url_string_changed
 
   before_validation :normalize_name
   after_save :create_version
   after_save :categorize_tag
   after_save :update_wiki
+  after_save :clear_url_string_changed
   validates :name, tag_name: true, uniqueness: true
   validate :validate_wiki, :on => :create
   belongs_to_creator
@@ -181,16 +182,18 @@ class Artist < ApplicationRecord
     end
 
     def url_string=(string)
-      self.url_string_was = url_string
+      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)
+
+      self.url_string_changed = (url_string_was != url_string)
     end
 
-    def url_string_changed?
-      url_string_was != url_string
+    def clear_url_string_changed
+      self.url_string_changed = false
     end
 
     def map_domain(x)
@@ -257,7 +260,7 @@ class Artist < ApplicationRecord
 
   module VersionMethods
     def create_version(force=false)
-      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 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
diff --git a/test/unit/artist_test.rb b/test/unit/artist_test.rb
index 307e919ed..5e88dd85a 100644
--- a/test/unit/artist_test.rb
+++ b/test/unit/artist_test.rb
@@ -493,15 +493,44 @@ class ArtistTest < ActiveSupport::TestCase
       assert_equal(Tag.categories.artist, tag.category)
     end
 
-    context "when updated" do
+    context "when saving" do
       setup do
-        @artist = FactoryBot.create(:artist)
+        @artist = FactoryBot.create(:artist, url_string: "http://foo.com")
         @artist.stubs(:merge_version?).returns(false)
       end
 
-      should "create a new version" do
+      should "create a new version when an url is added" do
         assert_difference("ArtistVersion.count") do
-          @artist.update(:url_string => "http://foo.com")
+          @artist.update(:url_string => "http://foo.com http://bar.com")
+          assert_equal(%w[http://bar.com http://foo.com], @artist.versions.last.url_array)
+        end
+      end
+
+      should "create a new version when an url is removed" do
+        assert_difference("ArtistVersion.count") do
+          @artist.update(:url_string => "")
+          assert_equal(%w[], @artist.versions.last.url_array)
+        end
+      end
+
+      should "create a new version when an url is marked inactive" do
+        assert_difference("ArtistVersion.count") do
+          @artist.update(:url_string => "-http://foo.com")
+          assert_equal(%w[-http://foo.com], @artist.versions.last.url_array)
+        end
+      end
+
+      should "not create a new version when nothing has changed" do
+        assert_no_difference("ArtistVersion.count") do
+          @artist.save
+          assert_equal(%w[http://foo.com], @artist.versions.last.url_array)
+        end
+      end
+
+      should "not save invalid urls" do
+        assert_no_difference("ArtistVersion.count") do
+          @artist.update(:url_string => "http://foo.com www.example.com")
+          assert_equal(%w[http://foo.com], @artist.versions.last.url_array)
         end
       end
     end