Revert validation of album artists separator (#998)

This reverts a small part of #722, where I moved the separator to the album artist model. This worked inside the model tests, but does not work in the controller since when updating (since we fully replace the album artists, during the validation `album.album_artists.size` is 0.

Author: robbevp

app/models/album.rb

@@ -40,6 +40,7 @@ class Album < ApplicationRecord
   before_validation :normalize_artist_order
 
   validates :title, presence: true
+  validate :album_artist_separators
 
   normalized_col_generator :title
   nilify_blank_values :edition_description, :review_comment
@@ -80,4 +81,14 @@ def normalize_artist_order
       aa.save unless aa.new_record?
     end
   end
+
+  def album_artist_separators
+    album_artists.each do |aa|
+      if aa.order == album_artists.to_a.count
+        errors.add(:album_artists, :last_separator_present) unless aa.separator.nil?
+      elsif aa.separator.nil?
+        errors.add(:album_artists, :separator_blank)
+      end
+    end
+  end
 end

app/models/album_artist.rb

@@ -33,18 +33,6 @@ class AlbumArtist < ApplicationRecord
 
   validates :name, presence: true
   validates :order, presence: true
-  validate :separator_not_nil, unless: :last_item?
-  validates :separator, absence: { allow_blank: false }, if: :last_item?
 
   normalized_col_generator :name
-
-  private
-
-  def last_item?
-    order == album.album_artists.size
-  end
-
-  def separator_not_nil
-    errors.add(:separator, :blank) if separator.nil?
-  end
 end

test/controllers/albums_controller_test.rb

@@ -185,6 +185,18 @@ class AlbumsControllerTest < ActionDispatch::IntegrationTest
     assert_response :success
   end
 
+  test 'should update album and replace associated album_artists' do
+    sign_in_as create(:moderator)
+    create_list(:album_artist, 2, album: @album)
+    artist = create(:artist)
+
+    assert_difference '@album.album_artists.reload.count', -1 do
+      patch album_url(@album), params: { album: { album_artists: [{ artist_id: artist.id, name: artist.name, order: 1 }] } }
+    end
+
+    assert_response :success
+  end
+
   test 'should destroy previous image when image is replaced' do
     sign_in_as create(:moderator)
     album = create(:album, :with_image)

test/models/album_artist_test.rb

@@ -35,40 +35,4 @@ class AlbumArtistTest < ActiveSupport::TestCase
     assert_not album_artist.normalized_name.nil?
     assert_equal 'iouaaa', album_artist.normalized_name
   end
-
-  test 'should be valid if all except last have separator' do
-    album = build(:album, album_artists: [build(:album_artist, separator: ' / ', order: 1), build(:album_artist, separator: nil, order: 2)])
-    album_artist1 = album.album_artists.first
-    album_artist2 = album.album_artists.second
-
-    assert_predicate album_artist1, :valid?
-    assert_predicate album_artist2, :valid?
-  end
-
-  test 'should reject if album artists except last has no separator' do
-    album = build(:album, album_artists: [build(:album_artist, separator: nil, order: 1), build(:album_artist, separator: nil, order: 2)])
-    album_artist1 = album.album_artists.first
-    album_artist2 = album.album_artists.second
-
-    assert_not_predicate album_artist1, :valid?
-    assert_predicate album_artist2, :valid?
-    assert_error_of_kind album_artist1, :separator, :blank
-  end
-
-  test 'should allow album artists with empty string as separator' do
-    album = build(:album, album_artists: [build(:album_artist, separator: '', order: 1), build(:album_artist, separator: nil, order: 2)])
-    album_artist1 = album.album_artists.first
-    album_artist2 = album.album_artists.second
-
-    assert_predicate album_artist1, :valid?
-    assert_predicate album_artist2, :valid?
-  end
-
-  test 'should reject if last album artists has separator' do
-    album = build(:album, album_artists: [build(:album_artist, separator: ' / ')])
-    album_artist1 = album.album_artists.first
-
-    assert_not_predicate album_artist1, :valid?
-    assert_error_of_kind album_artist1, :separator, :present
-  end
 end

test/models/album_test.rb

@@ -35,6 +35,32 @@ class AlbumTest < ActiveSupport::TestCase
     assert_equal 'iouaaa', album.normalized_title
   end
 
+  test 'should pass if all but last album artists have separator' do
+    album = build(:album, album_artists: [build(:album_artist, separator: ' / ', order: 1), build(:album_artist, separator: nil, order: 2)])
+
+    assert_predicate album, :valid?
+  end
+
+  test 'should reject if album artists except last has no separator' do
+    album = build(:album, album_artists: [build(:album_artist, separator: nil, order: 1), build(:album_artist, separator: nil, order: 2)])
+
+    assert_not_predicate album, :valid?
+    assert_error_of_kind album, :album_artists, :separator_blank
+  end
+
+  test 'should allow album artists with empty string as separator' do
+    album = build(:album, album_artists: [build(:album_artist, separator: '', order: 1), build(:album_artist, separator: nil, order: 2)])
+
+    assert_predicate album, :valid?
+  end
+
+  test 'should reject if last album artists has separator' do
+    album = build(:album, album_artists: [build(:album_artist, separator: ' / ')])
+
+    assert_not_predicate album, :valid?
+    assert_error_of_kind album, :album_artists, :last_separator_present
+  end
+
   test 'should normalize order of album artists' do
     aa1 = build(:album_artist, order: 5)
     aa2 = build(:album_artist, order: 2)