artists: improve prefilling of new artist form (#4028)

* When creating an artist by clicking the '?' next to the artist tag in
  the tag list, prefill the new artist form by finding the artist's last
  upload and fetching its source data.

  Previously we filled the urls with the source of the artist's last
  upload, which was wrong because it was usually a direct image URL (#3078).

* Fix the other names field not escaping spaces within names to underscores.

* Fix the other names field being potentially prefilled with duplicate names.

app/controllers/artists_controller.rb

@@ -6,7 +6,7 @@ class ArtistsController < ApplicationController
   before_action :load_artist, :only => [:ban, :unban, :show, :edit, :update, :destroy, :undelete]
 
   def new
-    @artist = Artist.new_with_defaults(artist_params)
+    @artist = Artist.new_with_defaults(artist_params(:new))
     respond_with(@artist)
   end
 
@@ -104,9 +104,10 @@ private
     sp.permit!
   end
 
-  def artist_params
+  def artist_params(context = nil)
     permitted_params = %i[name other_names other_names_string group_name url_string notes]
     permitted_params << :is_active if CurrentUser.is_builder?
+    permitted_params << :source if context == :new
 
     params.fetch(:artist, {}).permit(permitted_params)
   end

app/logical/sources/strategies/base.rb

@@ -166,6 +166,16 @@ module Sources
         Artist.find_artists(normalize_for_artist_finder)
       end
 
+      # A new artist entry with suggested defaults for when the artist doesn't
+      # exist. Used in Artist.new_with_defaults to prefill the new artist form.
+      def new_artist
+        Artist.new(
+          name: unique_id,
+          other_names: [artist_name],
+          url_string: [profile_url, normalize_for_artist_finder].sort.uniq.join("\n")
+        )
+      end
+
       def file_url
         image_url
       end

app/models/artist.rb

@@ -310,17 +310,26 @@ class Artist < ApplicationRecord
   end
 
   module FactoryMethods
+    # Make a new artist, fetching the defaults either from the given source, or
+    # from the source of the artist's last upload.
     def new_with_defaults(params)
-      Artist.new(params).tap do |artist|
+      source = params.delete(:source)
-        if artist.name.present?
+
-          post = CurrentUser.without_safe_mode do
+      if source.blank? && params[:name].present?
-            Post.tag_match("source:http #{artist.name}").where("true /* Artist.new_with_defaults */").first
+        CurrentUser.without_safe_mode do
-          end
+          post = Post.tag_match("source:http* #{params[:name]}").first
-          unless post.nil? || post.source.blank?
+          source = post.try(:source)
-            artist.url_string = post.source
-          end
         end
       end
+
+      if source.present?
+        artist = Sources::Strategies.find(source).new_artist
+        artist.attributes = params
+      else
+        artist = Artist.new(params)
+      end
+
+      artist.tap(&:validate) # run before_validation callbacks to normalize the names
     end
   end
 

app/views/sources/_info.html.erb

@@ -15,7 +15,7 @@
             <%= link_to @source.artist_name, @source.profile_url, id: "source-info-artist-profile" %>
 
             <% if @source.artists.empty? %>
-              (<%= link_to "Create new artist", new_artist_path(artist: { name: @source.unique_id, other_names: @source.artist_name, url_string: [@source.profile_url, @source.normalize_for_artist_finder].uniq.sort.join("\n") }), id: "source-info-create-new-artist" %>)
+              (<%= link_to "Create new artist", new_artist_path(artist: { source: @source.canonical_url }), id: "source-info-create-new-artist" %>)
             <% else %>
               (<ul id="source-info-translated-artists">
                 <% @source.artists.each do |artist| %>

test/unit/artist_test.rb

@@ -550,5 +550,26 @@ class ArtistTest < ActiveSupport::TestCase
         assert_equal(1, @artist.urls.count)
       end
     end
+
+    context "#new_with_defaults" do
+      should "fetch the defaults from the given source" do
+        source = "https://i.pximg.net/img-original/img/2018/01/28/23/56/50/67014762_p0.jpg"
+        artist = Artist.new_with_defaults(source: source)
+
+        assert_equal("niceandcool", artist.name)
+        assert_equal("nice_and_cool", artist.other_names_string)
+        assert_equal("https://www.pixiv.net/member.php?id=906442", artist.url_string)
+      end
+
+      should "fetch the defaults from the given tag" do
+        source = "https://i.pximg.net/img-original/img/2018/01/28/23/56/50/67014762_p0.jpg"
+        FactoryBot.create(:post, source: source, tag_string: "test_artist")
+        artist = Artist.new_with_defaults(name: "test_artist")
+
+        assert_equal("test_artist", artist.name)
+        assert_equal("nice_and_cool", artist.other_names_string)
+        assert_equal("https://www.pixiv.net/member.php?id=906442", artist.url_string)
+      end
+    end
   end
 end