From 10ca4dd3ad13781c52e8cdc18bbb0370cef145a3 Mon Sep 17 00:00:00 2001 From: evazion Date: Sat, 8 Sep 2018 15:39:50 -0500 Subject: [PATCH 1/2] artists: replace artist finder with fetch source data. * On posts, automatically trigger "Fetch source data" when clicking the Edit tab, instead of triggering the artist finder button. This way we find both the artist and the translated tags in one ajax call. * Remove the "Artist" finder button next to the source field. This isn't necessary given that "Fetch source data" finds the artist itself. * Remove the /artists/finder.json API endpoint. This is no longer used after removing the "Artist" finder button. --- app/controllers/artists_controller.rb | 14 -------------- app/javascript/src/javascripts/posts.js.erb | 2 +- .../src/javascripts/related_tag.js.erb | 19 ++----------------- app/views/posts/partials/show/_edit.html.erb | 1 - app/views/uploads/new.html.erb | 1 - config/routes.rb | 1 - test/functional/artists_controller_test.rb | 6 ++---- 7 files changed, 5 insertions(+), 39 deletions(-) diff --git a/app/controllers/artists_controller.rb b/app/controllers/artists_controller.rb index 1c83352fd..21deaee0d 100644 --- a/app/controllers/artists_controller.rb +++ b/app/controllers/artists_controller.rb @@ -92,20 +92,6 @@ class ArtistsController < ApplicationController end end - def finder - @artists = Artist.find_artists(params[:url], params[:referer_url]) - - respond_with(@artists) do |format| - format.xml do - render :xml => @artists.to_xml(:include => [:sorted_urls], :root => "artists") - end - format.json do - render :json => @artists.to_json(:include => [:sorted_urls]) - expires_in params[:expiry].to_i.days if params[:expiry] - end - end - end - private def load_artist diff --git a/app/javascript/src/javascripts/posts.js.erb b/app/javascript/src/javascripts/posts.js.erb index 7d617b8b1..9a1eb4b96 100644 --- a/app/javascript/src/javascripts/posts.js.erb +++ b/app/javascript/src/javascripts/posts.js.erb @@ -441,7 +441,7 @@ Post.initialize_post_sections = function() { $("#share").hide(); $("#post_tag_string").focus().selectEnd().height($("#post_tag_string")[0].scrollHeight); $("#related-tags-button").trigger("click"); - $("#find-artist-button").trigger("click"); + $("#fetch-data-manual").trigger("click"); $("#recommended").hide(); } else if (e.target.hash === "#recommended") { $("#comments").hide(); diff --git a/app/javascript/src/javascripts/related_tag.js.erb b/app/javascript/src/javascripts/related_tag.js.erb index 55acec5a0..85980c60e 100644 --- a/app/javascript/src/javascripts/related_tag.js.erb +++ b/app/javascript/src/javascripts/related_tag.js.erb @@ -1,3 +1,4 @@ +import Upload from './uploads'; import Utility from './utility'; import Cookie from './cookie'; @@ -16,7 +17,6 @@ RelatedTag.initialize_buttons = function() { <% TagCategory.related_button_list.each do |category| %> RelatedTag.common_bind("#related-<%= category %>-button", "<%= category %>"); // eslint-disable-line indent <% end %> - $("#find-artist-button").on("click.danbooru", RelatedTag.find_artist); } RelatedTag.tags_include = function(name) { @@ -281,21 +281,6 @@ RelatedTag.toggle_tag = function(e) { e.preventDefault(); } -RelatedTag.find_artist = function(e) { - $("#artist-tags").html("Loading..."); - var url = $("#upload_source,#post_source"); - var referer_url = $("#upload_referer_url"); - $.get("/artists/finder.json", {"url": url.val(), "referer_url": referer_url.val()}, RelatedTag.process_artist); - if (e) { - e.preventDefault(); - } -} - -RelatedTag.process_artist = function(data) { - RelatedTag.recent_artists = data; - RelatedTag.build_all(); -} - RelatedTag.toggle = function() { if ($("#related-tags").is(":visible")) { RelatedTag.hide(); @@ -332,7 +317,7 @@ RelatedTag.disable_artist_url = function(e) { is_active: false } } - }).done(RelatedTag.find_artist); + }).done(Upload.fetch_data_manual); } }); return false; diff --git a/app/views/posts/partials/show/_edit.html.erb b/app/views/posts/partials/show/_edit.html.erb index f70a07af4..2b5ddc82a 100644 --- a/app/views/posts/partials/show/_edit.html.erb +++ b/app/views/posts/partials/show/_edit.html.erb @@ -69,7 +69,6 @@ <%= f.label :source %> <%= f.text_field :source %> <%= button_tag "Similar", :id => "similar-button", :type => "button", :class => "ui-button ui-widget ui-corner-all sub gradient" %> - <%= button_tag "Artist", :id => "find-artist-button", :type => "button", :class => "ui-button ui-widget ui-corner-all sub gradient" %> <% if Danbooru.config.iqdbs_server %> diff --git a/app/views/uploads/new.html.erb b/app/views/uploads/new.html.erb index 077cc9cda..314735a67 100644 --- a/app/views/uploads/new.html.erb +++ b/app/views/uploads/new.html.erb @@ -43,7 +43,6 @@ <%= f.label :source %> <%= f.text_field :source, :size => 50, :value => params[:url] %> <%= button_tag "Similar", :id => "similar-button", :type => "button", :class => "ui-button ui-widget ui-corner-all sub gradient" %> - <%= button_tag "Artist", :id => "find-artist-button", :type => "button", :class => "ui-button ui-widget ui-corner-all sub gradient" %> You can enter a URL to have <%= Danbooru.config.app_name %> automatically download and process it diff --git a/config/routes.rb b/config/routes.rb index 6a8f10b87..d4a354b7a 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -79,7 +79,6 @@ Rails.application.routes.draw do collection do get :show_or_new get :banned - get :finder end end resources :artist_urls, only: [:update] diff --git a/test/functional/artists_controller_test.rb b/test/functional/artists_controller_test.rb index ad3ba74e7..99ba4e9dd 100644 --- a/test/functional/artists_controller_test.rb +++ b/test/functional/artists_controller_test.rb @@ -3,7 +3,7 @@ require 'test_helper' class ArtistsControllerTest < ActionDispatch::IntegrationTest def assert_artist_found(expected_artist, source_url = nil) if source_url - get_auth finder_artists_path(format: "json", url: source_url), @user + get_auth artists_path(format: "json", search: { url_matches: source_url }), @user if response.body =~ /Net::OpenTimeout/ skip "Remote connection to #{source_url} failed" return @@ -16,9 +16,7 @@ class ArtistsControllerTest < ActionDispatch::IntegrationTest end def assert_artist_not_found(source_url) - tries = 0 - - get_auth finder_artists_path(format: "json", url: source_url), @user + get_auth artists_path(format: "json", search: { url_matches: source_url }), @user if response.body =~ /Net::OpenTimeout/ skip "Remote connection to #{source_url} failed" return From 583f8457f06ca8b412a0283737adfcf331e7627f Mon Sep 17 00:00:00 2001 From: evazion Date: Sat, 8 Sep 2018 16:03:10 -0500 Subject: [PATCH 2/2] artists: clean up artist finding logic. Rename Artist#find_all_by_url to url_matches and drop previous url_matches method, along with find_artists and search_for_profile. Previously find_artists tried to lookup the url, referer url, and profile url in turn until an artist match was found. This was wasteful, because the source strategy already knows which url to lookup (usually the profile url). If that url doesn't find a match, then the artist doesn't exist. --- app/logical/sources/strategies/base.rb | 5 +-- app/logical/sources/strategies/twitter.rb | 8 ----- app/models/artist.rb | 38 ++--------------------- test/unit/artist_test.rb | 20 ++++++------ 4 files changed, 14 insertions(+), 57 deletions(-) diff --git a/app/logical/sources/strategies/base.rb b/app/logical/sources/strategies/base.rb index 2404e2f40..9d87b2273 100644 --- a/app/logical/sources/strategies/base.rb +++ b/app/logical/sources/strategies/base.rb @@ -129,6 +129,8 @@ module Sources normalize_for_artist_finder.present? end + # The url to use for artist finding purposes. This will be stored in the + # artist entry. Normally this will be the profile url. def normalize_for_artist_finder profile_url || url end @@ -139,8 +141,7 @@ module Sources end def artists - url = profile_url.presence || image_url.presence - Artist.find_artists(url) + Artist.url_matches(normalize_for_artist_finder) end def file_url diff --git a/app/logical/sources/strategies/twitter.rb b/app/logical/sources/strategies/twitter.rb index 99f28fba3..b1c83a261 100644 --- a/app/logical/sources/strategies/twitter.rb +++ b/app/logical/sources/strategies/twitter.rb @@ -66,14 +66,6 @@ module Sources::Strategies "" end - def artists - if profile_url.present? - Artist.find_artists(profile_url) - else - [] - end - end - def artist_name return "" if api_response.blank? api_response.attrs[:user][:screen_name] diff --git a/app/models/artist.rb b/app/models/artist.rb index 0ef989b19..3b54ee35d 100644 --- a/app/models/artist.rb +++ b/app/models/artist.rb @@ -148,7 +148,7 @@ class Artist < ApplicationRecord %r!\Ahttps?://(?:[a-zA-Z0-9_-]+\.)*#{domain}/\z!i end) - def find_all_by_url(url) + def url_matches(url) url = ArtistUrl.normalize(url) artists = [] @@ -163,7 +163,7 @@ class Artist < ApplicationRecord break if url =~ SITE_BLACKLIST_REGEXP end - artists.inject({}) {|h, x| h[x.name] = x; h}.values.slice(0, 20) + where(id: artists.uniq(&:name).take(20)) end end @@ -456,40 +456,6 @@ class Artist < ApplicationRecord end module SearchMethods - def find_artists(url, referer_url = nil) - artists = url_matches(url).order("id desc").limit(10) - - if artists.empty? && referer_url.present? && referer_url != url - artists = url_matches(referer_url).order("id desc").limit(20) - end - - artists - rescue PixivApiClient::Error => e - [] - end - - def url_matches(string) - matches = find_all_by_url(string).map(&:id) - - if matches.any? - where("id in (?)", matches) - elsif matches = search_for_profile(string) - where("id in (?)", matches) - else - where("false") - end - end - - def search_for_profile(url) - source = Sources::Strategies.find(url) - find_all_by_url(source.profile_url) - rescue Net::OpenTimeout, PixivApiClient::Error - raise if Rails.env.test? - nil - rescue Exception - nil - end - def other_names_match(string) if string =~ /\*/ && CurrentUser.is_builder? where("artists.other_names ILIKE ? ESCAPE E'\\\\'", string.to_escaped_for_sql_like) diff --git a/test/unit/artist_test.rb b/test/unit/artist_test.rb index 74925eaa4..a6df35c0a 100644 --- a/test/unit/artist_test.rb +++ b/test/unit/artist_test.rb @@ -154,9 +154,7 @@ class ArtistTest < ActiveSupport::TestCase should "resolve ambiguous urls" do bobross = FactoryBot.create(:artist, :name => "bob_ross", :url_string => "http://artists.com/bobross/image.jpg") bob = FactoryBot.create(:artist, :name => "bob", :url_string => "http://artists.com/bob/image.jpg") - matches = Artist.find_all_by_url("http://artists.com/bob/test.jpg") - assert_equal(1, matches.size) - assert_equal("bob", matches.first.name) + assert_artist_found("bob", "http://artists.com/bob/test.jpg") end should "parse urls" do @@ -190,7 +188,7 @@ class ArtistTest < ActiveSupport::TestCase should "ignore pixiv.net/ and pixiv.net/img/ url matches" do a1 = FactoryBot.create(:artist, :name => "yomosaka", :url_string => "http://i2.pixiv.net/img18/img/evazion/14901720.png") a2 = FactoryBot.create(:artist, :name => "niwatazumi_bf", :url_string => "http://i2.pixiv.net/img18/img/evazion/14901720_big_p0.png") - assert_equal([], Artist.find_all_by_url("http://i2.pixiv.net/img28/img/kyang692/35563903.jpg")) + assert_artist_not_found("http://i2.pixiv.net/img28/img/kyang692/35563903.jpg") end should "find matches by url" do @@ -198,11 +196,11 @@ class ArtistTest < ActiveSupport::TestCase a2 = FactoryBot.create(:artist, :name => "subway", :url_string => "http://subway.com/x/test.jpg") a3 = FactoryBot.create(:artist, :name => "minko", :url_string => "https://minko.com/x/test.jpg") - assert_equal(["rembrandt"], Artist.find_all_by_url("http://rembrandt.com/x/test.jpg").map(&:name)) - assert_equal(["rembrandt"], Artist.find_all_by_url("http://rembrandt.com/x/another.jpg").map(&:name)) - assert_equal([], Artist.find_all_by_url("http://nonexistent.com/test.jpg").map(&:name)) - assert_equal(["minko"], Artist.find_all_by_url("https://minko.com/x/test.jpg").map(&:name)) - assert_equal(["minko"], Artist.find_all_by_url("http://minko.com/x/test.jpg").map(&:name)) + assert_artist_found("rembrandt", "http://rembrandt.com/x/test.jpg") + assert_artist_found("rembrandt", "http://rembrandt.com/x/another.jpg") + assert_artist_not_found("http://nonexistent.com/test.jpg") + assert_artist_found("minko", "https://minko.com/x/test.jpg") + assert_artist_found("minko", "http://minko.com/x/test.jpg") end should "be case-insensitive to domains when finding matches by url" do @@ -212,7 +210,7 @@ class ArtistTest < ActiveSupport::TestCase should "not find duplicates" do FactoryBot.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)) + assert_artist_found("warhol", "http://warhol.com/x/test.jpg") end should "not include duplicate urls" do @@ -222,7 +220,7 @@ class ArtistTest < ActiveSupport::TestCase should "hide deleted artists" do FactoryBot.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)) + assert_artist_not_found("http://warhol.com/a/image.jpg") end context "when finding deviantart artists" do