diff --git a/app/controllers/pet_states_controller.rb b/app/controllers/pet_states_controller.rb index 38c313fd..e23ebf67 100644 --- a/app/controllers/pet_states_controller.rb +++ b/app/controllers/pet_states_controller.rb @@ -40,7 +40,8 @@ class PetStatesController < ApplicationController end def next_unlabeled_appearance_path - unlabeled_appearance = PetState.next_unlabeled_appearance + unlabeled_appearance = + PetState.next_unlabeled_appearance(after_id: params[:after]) if unlabeled_appearance edit_pet_type_pet_state_path( diff --git a/app/helpers/support_form_helper.rb b/app/helpers/support_form_helper.rb index 7d8d9a0e..00fc312b 100644 --- a/app/helpers/support_form_helper.rb +++ b/app/helpers/support_form_helper.rb @@ -1,7 +1,8 @@ module SupportFormHelper class SupportFormBuilder < ActionView::Helpers::FormBuilder attr_reader :template - delegate :capture, :check_box_tag, :content_tag, :params, :render, + delegate :capture, :check_box_tag, :concat, :content_tag, + :hidden_field_tag, :params, :render, to: :template, private: true def errors @@ -40,8 +41,11 @@ module SupportFormHelper content_tag(:section, class: "actions", &block) end - def go_to_next_field(**options, &block) - content_tag(:label, class: "go-to-next", **options, &block) + def go_to_next_field(after: nil, **options, &block) + content_tag(:label, class: "go-to-next", **options) do + concat hidden_field_tag(:after, after) if after + yield + end end def go_to_next_check_box(value) diff --git a/app/models/pet_state.rb b/app/models/pet_state.rb index fc785e11..9badfff0 100644 --- a/app/models/pet_state.rb +++ b/app/models/pet_state.rb @@ -24,6 +24,7 @@ class PetState < ApplicationRecord scope :newest, -> { order(created_at: :desc) } scope :newest_pet_type, -> { joins(:pet_type).merge(PetType.newest) } + scope :created_before, ->(time) { where(arel_table[:created_at].lt(time)) } # A simple ordering that tries to bring reliable pet states to the front. scope :emotion_order, -> { @@ -142,11 +143,39 @@ class PetState < ApplicationRecord end end - def self.next_unlabeled_appearance + def self.next_unlabeled_appearance(after_id: nil) # Rather than just getting the newest unlabeled pet state, prioritize the # newest *pet type*. This better matches the user's perception of what the # newest state is, because the Rainbow Pool UI is grouped by pet type! - needs_labeling.newest_pet_type.newest.first + pet_states = needs_labeling.newest_pet_type.newest + + # If `after_id` is given, convert it from a PetState ID to creation + # timestamps, and find the next record prior to those timestamps. This + # enables skipping past records the user doesn't want to label. + if after_id + begin + after_pet_state = PetState.find(after_id) + before_pt_created_at = after_pet_state.pet_type.created_at + before_ps_created_at = after_pet_state.created_at + rescue ActiveRecord::RecordNotFound + Rails.logger.warn "PetState.next_unlabeled_appearance: Could not " + + "find pet state ##{after_id}" + return nil + end + + # Because we sort by `newest_pet_type` first, then breaks ties by + # `newest`, our filter needs to operate the same way. Kudos to: + # https://brunoscheufler.com/blog/2022-01-01-paginating-large-ordered-datasets-with-cursor-based-pagination + pet_states.merge!( + PetType.created_before(before_pt_created_at).or( + PetType.created_at(before_pt_created_at).and( + PetState.created_before(before_ps_created_at) + ) + ) + ) + end + + pet_states.first end end diff --git a/app/models/pet_type.rb b/app/models/pet_type.rb index 08606c4a..de088ffe 100644 --- a/app/models/pet_type.rb +++ b/app/models/pet_type.rb @@ -31,6 +31,12 @@ class PetType < ApplicationRecord # We use DTI's creation timestamp as an estimate of when it was released. where('created_at <= ?', time) } + scope :created_before, ->(time) { + where(arel_table[:created_at].lt(time)) + } + scope :created_at, ->(time) { + where(arel_table[:created_at].eq(time)) + } def self.random_basic_per_species(species_ids) random_pet_types = [] diff --git a/app/views/pet_states/edit.html.haml b/app/views/pet_states/edit.html.haml index fc55f6aa..04372852 100644 --- a/app/views/pet_states/edit.html.haml +++ b/app/views/pet_states/edit.html.haml @@ -39,9 +39,10 @@ = f.actions do = f.submit "Save changes" - = f.go_to_next_field title: "If checked, takes you to the first unlabeled appearance in the database, if any. Useful for labeling in bulk!" do + = f.go_to_next_field after: @pet_state.id, + title: "If checked, takes you to the first unlabeled appearance in the database, if any. Useful for labeling in bulk!" do = f.go_to_next_check_box "unlabeled-appearance" - Then: Go to unlabeled appearance + Then: Go to next unlabeled appearance - content_for :stylesheets do = stylesheet_link_tag "application/breadcrumbs" diff --git a/spec/models/pet_state_spec.rb b/spec/models/pet_state_spec.rb new file mode 100644 index 00000000..8cac01e1 --- /dev/null +++ b/spec/models/pet_state_spec.rb @@ -0,0 +1,105 @@ +require_relative '../rails_helper' + +RSpec.describe PetState do + fixtures :colors, :species, :zones + + let(:blue) { colors(:blue) } + let(:green) { colors(:green) } + let(:red) { colors(:red) } + let(:acara) { species(:acara) } + + describe ".next_unlabeled_appearance" do + before { PetType.destroy_all } + + def create_sa + swf_asset = SwfAsset.create!( + type: "biology", remote_id: (SwfAsset.maximum(:remote_id) || 0) + 1, + url: "https://images.neopets.example/hello.swf", + zone: zones(:body), zones_restrict: [], body_id: 0) + end + + def create_pt(color, species, created_at = nil) + PetType.create! color:, species:, created_at:, + body_id: (PetType.maximum(:body_id) || 0) + 1 + end + + def create_ps(pet_type, pose, created_at = nil, **options) + # HACK: PetStates without any assets don't save correctly. + # https://github.com/rails/rails/issues/52340 + swf_assets = [create_sa] + PetState.create! pet_type:, pose:, created_at:, swf_assets:, + swf_asset_ids: swf_assets.map(&:id), **options + end + + it "returns nil where there are no pet states" do + expect(PetState.next_unlabeled_appearance).to be_nil + end + + it "returns nil where there are only labeled pet states" do + pt = PetType.create! color: blue, species: acara, body_id: 1 + ps = create_ps(pt, "HAPPY_MASC").tap(&:save!) + expect(PetState.next_unlabeled_appearance).to be_nil + end + + it "returns the only pet state when it is unlabeled" do + pt = PetType.create! color: blue, species: acara, body_id: 1 + ps = create_ps(pt, "UNKNOWN").tap(&:save!) + expect(PetState.next_unlabeled_appearance).to eq ps + end + + describe "with multiple unlabeled pet states" do + before do + # Create three pet types, with ascending order of creation date. + @pt1 = create_pt blue, acara, Date.new(2000) + @pt2 = create_pt green, acara, Date.new(2005) + @pt3 = create_pt red, acara, Date.new(2010) + + # Give each a pet state, but created in a different order. + @ps1 = create_ps @pt1, "UNKNOWN", Date.new(2020) + @ps2 = create_ps @pt2, "UNKNOWN", Date.new(2025) + @ps3 = create_ps @pt3, "UNKNOWN", Date.new(2015) + end + + it "returns the latest pet type's pet state" do + expect(PetState.next_unlabeled_appearance).to eq @ps3 + end + + it "excludes fully-labeled pet types" do + # Label the latest pet state, then see it move to the next. + @ps3.update!(pose: "HAPPY_FEM") + expect(PetState.next_unlabeled_appearance).to eq @ps2 + end + + it "excludes labeled pet states" do + # Create an older pet state on the latest pet type, than label the + # latest pet state, and see it move back to the older one. + ps3_a = create_ps @pt3, "UNKNOWN", Date.new(2014) + @ps3.update!(pose: "HAPPY_FEM") + expect(PetState.next_unlabeled_appearance).to eq ps3_a + end + + it "sorts pet states within the latest pet type by newest" do + # Create a few pet types on the latest pet type, and see that we get + # the latest back. + ps3_a = create_ps @pt3, "UNKNOWN", Date.new(2016) + ps3_b = create_ps @pt3, "UNKNOWN", Date.new(2017) + ps3_c = create_ps @pt3, "UNKNOWN", Date.new(2018) + ps3_d = create_ps @pt3, "UNKNOWN", Date.new(2019) + expect(PetState.next_unlabeled_appearance).to eq ps3_d + end + + it "can find the next after the latest pet state" do + expect(PetState.next_unlabeled_appearance(after_id: @ps3.id)).to eq @ps2 + end + + it "can find the next after any given pet state" do + expect(PetState.next_unlabeled_appearance(after_id: @ps2.id)).to eq @ps1 + end + + it "can find the next after the latest pet state, even within the same pet type" do + ps3_a = create_ps @pt3, "UNKNOWN", Date.new(2014) + expect(PetState.next_unlabeled_appearance(after_id: @ps3.id)).to eq ps3_a + end + end + end +end