Improve static file job handling and user avatar processing

TODO.md

@@ -10,15 +10,15 @@
 - [ ] Automatically enqueue jobs for FA users to do incremental scans of profiles
 - [ ] Fix FA posts that start with "Font size adjustment: smallerlarger"
 - [ ] Convert logger .prefix=... into .tagged(...)
-- [ ] `make_tag` should be smart about the objects it takes
+- [x] `make_tag` should be smart about the objects it takes
 - [ ] Convert all `state: string` attributes to enums in ActiveRecord models
 - [ ] Create `belongs_to_log_entry` macro for ActiveRecord models
-- [ ] Use StaticFileJobHelper for Domain::Fa::Job::ScanFileJob
+- [x] Use StaticFileJobHelper for Domain::Fa::Job::ScanFileJob
 - [ ] Unify HTTP client configs for all domains, so the same job type can be used for different domains
 - [ ] put abstract `external_url_for_view` in a module
 - [ ] backfill descriptions on inkbunny posts
 - [ ] store deep update json on inkbunny posts
-- [ ] limit number of users, or paginate for "users who favorited this post" page
+- [x] limit number of users, or paginate for "users who favorited this post" page
 - [ ] manual good job runner does not indicate if the job threw an exception - check return value of #perform, maybe?
 - [ ] FA user favs job should stop when in incremental mode when all posts on the page are already known favs (e.g. pages with only 47 posts are not a false positive)
 - [x] Factor out FA listings page enqueue logic into common location; use in Gallery and Favs jobs

app/controllers/domain/users_controller.rb

@@ -64,7 +64,12 @@ class Domain::UsersController < ApplicationController
     @post = T.must(@post)
     authorize @post
     @user_post_favs =
-      @post.user_post_favs.includes(user: :avatar).page(params[:page]).per(50)
+      @post
+        .user_post_favs
+        .includes(user: :avatar)
+        .page(params[:page])
+        .per(50)
+        .without_count
   end
 
   sig(:final) do

app/jobs/domain/fa/job/base.rb

@@ -146,9 +146,14 @@ class Domain::Fa::Job::Base < Scraper::JobBase
     params(
       page_type: ListingPageType::Type,
       page_parser: Domain::Fa::Parser::Page,
+      for_user: T.nilable(Domain::User::FaUser),
     ).returns(ListingPageScanStats)
   end
-  def update_and_enqueue_posts_from_listings_page(page_type, page_parser:)
+  def update_and_enqueue_posts_from_listings_page(
+    page_type,
+    page_parser:,
+    for_user: nil
+  )
     fatal_error("not a listing page") unless page_parser.probably_listings_page?
 
     all_posts = T.let([], T::Array[Domain::Post::FaPost])
@@ -185,10 +190,17 @@ class Domain::Fa::Job::Base < Scraper::JobBase
       end
 
       if creator = post.creator
+        creator.state_ok!
+        creator.save!
         enqueue_user_scan(creator)
       end
     end
 
+    if for_user && (user_page = page_parser.user_page) &&
+         (url = user_page.profile_thumb_url)
+      enqueue_user_avatar(for_user, url)
+    end
+
     posts_to_save.each(&:save!)
 
     logger.info(
@@ -331,15 +343,53 @@ class Domain::Fa::Job::Base < Scraper::JobBase
       user_page.profile_html.encode("UTF-8", invalid: :replace, undef: :replace)
     user.last_user_page_id = response.log_entry.id
     user.scanned_page_at = Time.current
+    user.save!
 
-    avatar = user.avatar || Domain::UserAvatar.new(user: user)
-    uri = Addressable::URI.parse(user_page.profile_thumb_url)
+    if url = user_page.profile_thumb_url
+      enqueue_user_avatar(user, url)
+    end
+  end
+
+  sig { params(user: Domain::User::FaUser, avatar_url_str: String).void }
+  def enqueue_user_avatar(user, avatar_url_str)
+    match = avatar_url_str.match(%r{/([^/]+)\.gif})
+    if match.nil?
+      logger.warn(
+        format_tags("invalid avatar url", make_tag("url", avatar_url_str)),
+      )
+      return
+    end
+
+    expected_url_name = match[1]
+    if user.url_name != expected_url_name
+      logger.warn(
+        format_tags(
+          "invalid avatar url",
+          make_tag("url", avatar_url_str),
+          make_tag("expected", expected_url_name),
+          make_tag("actual", user.url_name),
+        ),
+      )
+      return
+    end
+
+    uri = Addressable::URI.parse(avatar_url_str)
     uri.scheme ||= "https"
-    avatar.url_str = uri.to_s
+
+    avatar = user.avatar
+    if avatar.nil? || (avatar.url_str.present? && avatar.url_str != uri.to_s)
+      logger.info(format_tags("creating new avatar", make_tag("url", uri.to_s)))
+      avatar = user.avatars.build(url_str: uri.to_s)
+    elsif avatar.url_str.blank?
+      logger.info(format_tags("updating avatar", make_tag("url", uri.to_s)))
+      avatar.url_str = uri.to_s
+    end
+
     if avatar.changed?
-      avatar.state = "pending"
+      avatar.state_pending!
       avatar.save!
       defer_job(Domain::Fa::Job::UserAvatarJob, { avatar: })
+      user.association(:avatar).reload
     end
   end
 end

app/jobs/domain/fa/job/scan_file_job.rb

@@ -1,6 +1,7 @@
 # typed: strict
 class Domain::Fa::Job::ScanFileJob < Domain::Fa::Job::Base
   queue_as :static_file
+  include Domain::StaticFileJobHelper
 
   sig { override.params(args: T::Hash[Symbol, T.untyped]).returns(T.untyped) }
   def perform(args)
@@ -31,45 +32,7 @@ class Domain::Fa::Job::ScanFileJob < Domain::Fa::Job::Base
         end
 
     file = T.must(file)
-
-    logger.info "scanning file: #{file.id}, state=#{file.state}, retry_count=#{file.retry_count}"
-
-    if file.state == "terminal_error" && !force_scan?
-      logger.warn("state == terminal_error, abort without retrying")
-      return
-    end
-
-    if file.state == "retryable_error" && file.retry_count >= 3
-      logger.warn("retry_count >= 3, abort without retrying")
-      return
-    end
-
-    if file.state == "ok" && !force_scan?
-      logger.warn("already have file, skipping")
-      return
-    end
-
-    file_url_str = file.url_str
-    fatal_error("file has no url") if file_url_str.nil?
-
-    response = http_client.get(file_url_str)
-    file.log_entry = response.log_entry
-    file.last_status_code = response.status_code
-
-    if response.status_code == 200
-      file.state = "ok"
-      file.retry_count = 0
-      logger.info "response is 200, ok"
-    elsif response.status_code == 404
-      file.state = "terminal_error"
-      logger.error "response is 404, abort without retrying"
-    else
-      file.state = "retryable_error"
-      file.retry_count += 1
-      logger.warn "response is #{response.status_code}, retrying later"
-      # job runner will retry this job as it threw an exception
-      fatal_error("response #{response.status_code}, aborting")
-    end
+    download_post_file(file)
   ensure
     file.save! if file
   end

app/jobs/domain/fa/job/user_gallery_job.rb

@@ -123,6 +123,7 @@ class Domain::Fa::Job::UserGalleryJob < Domain::Fa::Job::Base
         update_and_enqueue_posts_from_listings_page(
           ListingPageType::GalleryPage.new(page_number:, folder: folder.title),
           page_parser: page,
+          for_user: user,
         )
       total_num_new_posts_seen += listing_page_stats.new_posts.count
       total_num_posts_seen += listing_page_stats.all_posts.count

app/jobs/domain/static_file_job_helper.rb

@@ -20,9 +20,16 @@ module Domain::StaticFileJobHelper
       return
     end
 
+    if post_file.state_retryable_error? && post_file.retry_count >= MAX_RETRIES
+      logger.warn(format_tags("retry count >= max retries, skipping"))
+      post_file.state_terminal_error!
+      return
+    end
+
     file_url_str = post_file.url_str
     if file_url_str.blank?
       logger.warn(format_tags("no url, skipping"))
+      post_file.state_terminal_error!
       return
     end
 
@@ -32,10 +39,10 @@ module Domain::StaticFileJobHelper
     logger.tagged(make_arg_tag(response.log_entry)) do
       if response.status_code == 200
         post_file.state_ok!
+        post_file.retry_count = 0
         logger.info(format_tags("downloaded file"))
       elsif response.status_code == 404
         post_file.state_terminal_error!
-        post_file.retry_count += 1
         logger.error(format_tags("404, terminal error state"))
       else
         post_file.retry_count += 1

app/views/domain/users/users_faving_post.erb

@@ -11,6 +11,7 @@
       </div>
     </div>
   </section>
+  <%= render "shared/pagination_controls", collection: @user_post_favs %>
   <% favs = @user_post_favs.to_a %>
   <% if favs.any? %>
     <section

sorbet/rbi/dsl/application_controller.rbi

@@ -36,12 +36,12 @@ class ApplicationController
     include ::Domain::ModelHelper
     include ::Domain::PaginationHelper
     include ::Domain::PostGroupsHelper
+    include ::LogEntriesHelper
     include ::Domain::PostsHelper
     include ::Domain::UsersHelper
     include ::DomainSourceHelper
     include ::GoodJobHelper
     include ::IndexablePostsHelper
-    include ::LogEntriesHelper
     include ::SourceHelper
     include ::DeviseHelper
     include ::ReactOnRails::Utils::Required

sorbet/rbi/dsl/devise_controller.rbi

@@ -33,12 +33,12 @@ class DeviseController
     include ::Domain::ModelHelper
     include ::Domain::PaginationHelper
     include ::Domain::PostGroupsHelper
+    include ::LogEntriesHelper
     include ::Domain::PostsHelper
     include ::Domain::UsersHelper
     include ::DomainSourceHelper
     include ::GoodJobHelper
     include ::IndexablePostsHelper
-    include ::LogEntriesHelper
     include ::SourceHelper
     include ::DeviseHelper
     include ::ReactOnRails::Utils::Required

sorbet/rbi/dsl/domain/post_file/inkbunny_post_file.rbi

@@ -591,6 +591,9 @@ class Domain::PostFile::InkbunnyPostFile
     sig { params(args: T.untyped, blk: T.untyped).returns(PrivateAssociationRelation) }
     def none(*args, &blk); end
 
+    sig { params(args: T.untyped, blk: T.untyped).returns(PrivateAssociationRelation) }
+    def not_state_file_error(*args, &blk); end
+
     sig { params(args: T.untyped, blk: T.untyped).returns(PrivateAssociationRelation) }
     def not_state_ok(*args, &blk); end
 
@@ -665,6 +668,9 @@ class Domain::PostFile::InkbunnyPostFile
     sig { params(args: T.untyped, blk: T.untyped).returns(PrivateAssociationRelation) }
     def select(*args, &blk); end
 
+    sig { params(args: T.untyped, blk: T.untyped).returns(PrivateAssociationRelation) }
+    def state_file_error(*args, &blk); end
+
     sig { params(args: T.untyped, blk: T.untyped).returns(PrivateAssociationRelation) }
     def state_ok(*args, &blk); end
 
@@ -2080,6 +2086,9 @@ class Domain::PostFile::InkbunnyPostFile
     sig { params(args: T.untyped, blk: T.untyped).returns(PrivateRelation) }
     def none(*args, &blk); end
 
+    sig { params(args: T.untyped, blk: T.untyped).returns(PrivateRelation) }
+    def not_state_file_error(*args, &blk); end
+
     sig { params(args: T.untyped, blk: T.untyped).returns(PrivateRelation) }
     def not_state_ok(*args, &blk); end
 
@@ -2154,6 +2163,9 @@ class Domain::PostFile::InkbunnyPostFile
     sig { params(args: T.untyped, blk: T.untyped).returns(PrivateRelation) }
     def select(*args, &blk); end
 
+    sig { params(args: T.untyped, blk: T.untyped).returns(PrivateRelation) }
+    def state_file_error(*args, &blk); end
+
     sig { params(args: T.untyped, blk: T.untyped).returns(PrivateRelation) }
     def state_ok(*args, &blk); end
 

sorbet/rbi/dsl/domain/user_post_fav.rbi

@@ -495,6 +495,9 @@ class Domain::UserPostFav
     sig { params(association: Symbol).returns(T::Array[T.untyped]) }
     def extract_associated(association); end
 
+    sig { params(args: T.untyped, blk: T.untyped).returns(PrivateAssociationRelation) }
+    def for_post_type(*args, &blk); end
+
     sig { params(args: T.untyped, blk: T.untyped).returns(PrivateAssociationRelation) }
     def from(*args, &blk); end
 
@@ -910,6 +913,9 @@ class Domain::UserPostFav
     sig { params(association: Symbol).returns(T::Array[T.untyped]) }
     def extract_associated(association); end
 
+    sig { params(args: T.untyped, blk: T.untyped).returns(PrivateRelation) }
+    def for_post_type(*args, &blk); end
+
     sig { params(args: T.untyped, blk: T.untyped).returns(PrivateRelation) }
     def from(*args, &blk); end
 

sorbet/rbi/dsl/good_job/jobs_controller.rbi

@@ -26,6 +26,7 @@ class GoodJob::JobsController
     include ::GoodJob::ApplicationHelper
     include ::GoodJob::ApplicationController::HelperMethods
     include ::HelpersInterface
+    include ::LogEntriesHelper
     include ::Domain::PostsHelper
     include ::Domain::UsersHelper
     include ::Domain::PostGroupsHelper

sorbet/rbi/dsl/rails/application_controller.rbi

@@ -36,12 +36,12 @@ class Rails::ApplicationController
     include ::Domain::ModelHelper
     include ::Domain::PaginationHelper
     include ::Domain::PostGroupsHelper
+    include ::LogEntriesHelper
     include ::Domain::PostsHelper
     include ::Domain::UsersHelper
     include ::DomainSourceHelper
     include ::GoodJobHelper
     include ::IndexablePostsHelper
-    include ::LogEntriesHelper
     include ::SourceHelper
     include ::DeviseHelper
     include ::ReactOnRails::Utils::Required

sorbet/rbi/dsl/rails/conductor/base_controller.rbi

@@ -36,12 +36,12 @@ class Rails::Conductor::BaseController
     include ::Domain::ModelHelper
     include ::Domain::PaginationHelper
     include ::Domain::PostGroupsHelper
+    include ::LogEntriesHelper
     include ::Domain::PostsHelper
     include ::Domain::UsersHelper
     include ::DomainSourceHelper
     include ::GoodJobHelper
     include ::IndexablePostsHelper
-    include ::LogEntriesHelper
     include ::SourceHelper
     include ::DeviseHelper
     include ::ReactOnRails::Utils::Required

sorbet/rbi/dsl/rails/health_controller.rbi

@@ -36,12 +36,12 @@ class Rails::HealthController
     include ::Domain::ModelHelper
     include ::Domain::PaginationHelper
     include ::Domain::PostGroupsHelper
+    include ::LogEntriesHelper
     include ::Domain::PostsHelper
     include ::Domain::UsersHelper
     include ::DomainSourceHelper
     include ::GoodJobHelper
     include ::IndexablePostsHelper
-    include ::LogEntriesHelper
     include ::SourceHelper
     include ::DeviseHelper
     include ::ReactOnRails::Utils::Required

spec/jobs/domain/e621/job/static_file_job_spec.rb

@@ -59,7 +59,7 @@ describe Domain::E621::Job::StaticFileJob do
     expect(post_file.state).to eq("terminal_error")
     expect(post_file.log_entry).to eq(mock_log_entries[0])
     expect(post_file.last_status_code).to eq(404)
-    expect(post_file.retry_count).to eq(1)
+    expect(post_file.retry_count).to eq(0)
   end
 
   it "retries with a retryable error" do
@@ -97,7 +97,7 @@ describe Domain::E621::Job::StaticFileJob do
             content_type: "text/html",
             contents: "test",
           },
-        ] * 4,
+        ] * 3,
       )
 
     perform_now({ post_file: post_file }, should_raise: /will retry later/)
@@ -106,8 +106,8 @@ describe Domain::E621::Job::StaticFileJob do
     perform_now({ post_file: post_file })
     post_file.reload
     expect(post_file.state).to eq("terminal_error")
-    expect(post_file.log_entry).to eq(mock_log_entries[3])
+    expect(post_file.log_entry).to eq(mock_log_entries[2])
     expect(post_file.last_status_code).to eq(500)
-    expect(post_file.retry_count).to eq(4)
+    expect(post_file.retry_count).to eq(3)
   end
 end

spec/jobs/domain/fa/job/browse_page_job_spec.rb

@@ -357,6 +357,24 @@ describe Domain::Fa::Job::BrowsePageJob do
       include_examples "enqueue user gallery scan", true
     end
 
+    context "and post is marked as removed" do
+      before do
+        post = find_post.call
+        post.state_removed!
+        post.save!
+        perform_now({})
+      end
+
+      it "marks the post as ok" do
+        expect(find_post.call.state).to eq("ok")
+      end
+
+      include_examples "enqueue post scan", true
+      include_examples "enqueue file scan", false
+      include_examples "enqueue user page scan", true
+      include_examples "enqueue user gallery scan", true
+    end
+
     context "and post scanned but file is a terminal error state" do
       before do
         post = find_post.call

spec/jobs/domain/fa/job/scan_file_job_spec.rb

@@ -79,7 +79,7 @@ RSpec.describe Domain::Fa::Job::ScanFileJob do
           post_file.update!(state: "pending")
           perform_now(
             { post_file: post_file },
-            should_raise: /response 500, aborting/,
+            should_raise: /will retry later/,
           )
 
           post_file.reload
@@ -92,7 +92,9 @@ RSpec.describe Domain::Fa::Job::ScanFileJob do
         let(:client_mock_config) { [] }
 
         it "skips processing when state is ok" do
-          post_file.update!(state: "ok")
+          post_file.log_entry = create(:http_log_entry)
+          post_file.state_ok!
+          post_file.save!
 
           perform_now({ post_file: post_file })
 
@@ -115,7 +117,7 @@ RSpec.describe Domain::Fa::Job::ScanFileJob do
           perform_now({ post_file: post_file })
 
           post_file.reload
-          expect(post_file.state).to eq("retryable_error")
+          expect(post_file.state).to eq("terminal_error")
           expect(post_file.retry_count).to eq(3)
         end
       end
@@ -168,10 +170,11 @@ RSpec.describe Domain::Fa::Job::ScanFileJob do
         )
       end
 
-      it "raises error when file has no url" do
+      it "marks the file as terminal error when it has no url" do
         post_file.update!(url_str: nil, state: "pending")
-
-        perform_now({ post_file: post_file }, should_raise: /file has no url/)
+        perform_now({ post_file: post_file })
+        post_file.reload
+        expect(post_file.state).to eq("terminal_error")
       end
     end
   end

spec/jobs/domain/fa/job/user_gallery_job_spec.rb

@@ -161,6 +161,48 @@ describe Domain::Fa::Job::UserGalleryJob do
         expect { perform_now(args) }.to(expectations)
       end
 
+      it "enqueues user avatar job" do
+        expect do
+          perform_now(args)
+          user.reload
+        end.to change(user, :avatar).from(be_nil).to(be_present)
+
+        expect(
+          SpecUtil.enqueued_job_args(Domain::Fa::Job::UserAvatarJob),
+        ).to match_array(hash_including({ avatar: user.avatar }))
+
+        expect(user.avatar.url_str).to eq(
+          "https://a.furaffinity.net/1674032155/pcraxkers.gif",
+        )
+      end
+
+      context "when the user has an avatar" do
+        before do
+          user.avatar =
+            create(
+              :domain_user_avatar,
+              state: "ok",
+              url_str: "https://a.furaffinity.net/0/pcraxkers.gif",
+            )
+        end
+
+        it "creates a new avatar if the url_str is different" do
+          expect do
+            perform_now(args)
+            user.reload
+          end.to change { user.avatars.count }.by(1)
+        end
+
+        it "enqueues user avatar job and updates the url_str" do
+          expect do
+            perform_now(args)
+            user.association(:avatar).reload
+          end.to change { user.avatar.url_str }.from(
+            "https://a.furaffinity.net/0/pcraxkers.gif",
+          ).to("https://a.furaffinity.net/1674032155/pcraxkers.gif")
+        end
+      end
+
       it "records the first log entry" do
         perform_now(args)
         user.reload

spec/jobs/domain/inkbunny/job/static_file_job_spec.rb

@@ -79,7 +79,7 @@ describe Domain::Inkbunny::Job::StaticFileJob do
 
       file.reload
       expect(file.state).to eq("terminal_error")
-      expect(file.retry_count).to eq(1)
+      expect(file.retry_count).to eq(0)
       expect(file.last_status_code).to eq(404)
       expect(file.log_entry).to eq(log_entries[0])
     end
@@ -115,7 +115,7 @@ describe Domain::Inkbunny::Job::StaticFileJob do
       file.reload
       expect(file.log_entry).to eq(log_entries[1])
       expect(file.state).to eq("ok")
-      expect(file.retry_count).to eq(1)
+      expect(file.retry_count).to eq(0)
       expect(file.last_status_code).to eq(200)
     end
 
@@ -130,7 +130,7 @@ describe Domain::Inkbunny::Job::StaticFileJob do
               content_type: "text/html",
               contents: "not found",
             },
-          ] * 4,
+          ] * 3,
         )
 
       3.times.map do |retry_num|
@@ -146,7 +146,7 @@ describe Domain::Inkbunny::Job::StaticFileJob do
       perform_now({ file: file })
       file.reload
       expect(file.state).to eq("terminal_error")
-      expect(file.log_entry).to eq(log_entries[3])
+      expect(file.log_entry).to eq(log_entries[2])
     end
 
     it "fails if file argument is missing" do