Remove the pending marker on some modeling tests

We're leaning more into Rails collection management and autosave stuff!

app/models/pet.rb

@@ -3,71 +3,16 @@ class Pet < ApplicationRecord
 
   attr_reader :items, :pet_state, :alt_style
 
-  scope :with_pet_type_color_ids, ->(color_ids) {
-    joins(:pet_type).where(PetType.arel_table[:id].in(color_ids))
-  }
-
   def load!(timeout: nil)
-    viewer_data = Neopets::CustomPets.fetch_viewer_data(name, timeout:)
-    use_viewer_data(viewer_data)
+    viewer_data_hash = Neopets::CustomPets.fetch_viewer_data(name, timeout:)
+    use_viewer_data(ViewerData.new(viewer_data_hash))
   end
 
   def use_viewer_data(viewer_data)
-    pet_data = viewer_data[:custom_pet]
-
-    raise UnexpectedDataFormat unless pet_data[:species_id]
-    raise UnexpectedDataFormat unless pet_data[:color_id]
-    raise UnexpectedDataFormat unless pet_data[:body_id]
-
-    has_alt_style = pet_data[:alt_style].present?
-
-    self.pet_type = PetType.find_or_initialize_by(
-      species_id: pet_data[:species_id].to_i,
-      color_id: pet_data[:color_id].to_i
-    )
-
-    begin
-      new_image_hash = Neopets::CustomPets.fetch_image_hash(self.name)
-    rescue => error
-      Rails.logger.warn "Failed to load image hash: #{error.full_message}"
-    end
-    self.pet_type.image_hash = new_image_hash if new_image_hash.present?
-
-    # With an alt style, `body_id` in the biology data refers to the body ID of
-    # the *alt* style, not the usual pet type. (We have `original_biology` for
-    # *some* of the pet type's situation, but not it's body ID!)
-    #
-    # So, in the alt style case, don't update `body_id` - but if this is our
-    # first time seeing this pet type and it doesn't *have* a `body_id` yet,
-    # let's not be creating it without one. We'll need to model it without the
-    # alt style first. (I don't bother with a clear error message though 😅)
-    self.pet_type.body_id = pet_data[:body_id] unless has_alt_style
-    if self.pet_type.body_id.nil?
-      raise UnexpectedDataFormat,
-        "can't process alt style on first occurrence of pet type"
-    end
-
-    pet_state_biology = has_alt_style ? pet_data[:original_biology] :
-      pet_data[:biology_by_zone]
-    raise UnexpectedDataFormat if pet_state_biology.empty?
-    pet_state_biology[0] = nil # remove effects if present
-    @pet_state = self.pet_type.add_pet_state_from_biology! pet_state_biology
-
-    if has_alt_style
-      raise UnexpectedDataFormat unless pet_data[:alt_color]
-      raise UnexpectedDataFormat if pet_data[:biology_by_zone].empty?
-
-      @alt_style = AltStyle.find_or_initialize_by(id: pet_data[:alt_style].to_i)
-      @alt_style.assign_attributes(
-        color_id: pet_data[:alt_color].to_i,
-        species_id: pet_data[:species_id].to_i,
-        body_id: pet_data[:body_id].to_i,
-        biology: pet_data[:biology_by_zone],
-      )
-    end
-
-    @items = Item.collection_from_pet_type_and_registries(self.pet_type,
-      viewer_data[:object_info_registry], viewer_data[:object_asset_registry])
+    self.pet_type = viewer_data.pet_type
+    @pet_state = viewer_data.pet_state
+    @alt_style = viewer_data.alt_style
+    @items = viewer_data.items
   end
 
   def wardrobe_query
@@ -93,11 +38,8 @@ class Pet < ApplicationRecord
 
   before_validation do
     pet_type.save!
-    if @pet_state
-      @pet_state.save!
-      @pet_state.handle_assets!
-    end
-    
+    @pet_state.save! if @pet_state
+
     if @items
       @items.each do |item|
         item.save! if item.changed?
@@ -117,5 +59,91 @@ class Pet < ApplicationRecord
   end
 
   class UnexpectedDataFormat < RuntimeError;end
+
+  # A representation of a Neopets::CustomPets viewer data response, translated
+  # to DTI's database models!
+  class ViewerData
+    def initialize(viewer_data_hash)
+      @custom_pet = viewer_data_hash[:custom_pet]
+      @object_info_registry = viewer_data_hash[:object_info_registry]
+      @object_asset_registry = viewer_data_hash[:object_asset_registry]
+    end
+
+    def pet_type
+      @pet_type ||= begin
+        raise UnexpectedDataFormat unless @custom_pet[:species_id]
+        raise UnexpectedDataFormat unless @custom_pet[:color_id]
+        raise UnexpectedDataFormat unless @custom_pet[:body_id]
+
+        @custom_pet => {species_id:, color_id:}
+        PetType.find_or_initialize_by(species_id:, color_id:).tap do |pet_type|
+          # Apply the pet's body ID to the pet type, unless it's wearing an alt
+          # style, in which case ignore it, because it's the *alt style*'s body ID.
+          # (This can theoretically cause a problem saving a new pet type when
+          # there's an alt style too!)
+          pet_type.body_id = @custom_pet[:body_id] unless @custom_pet[:alt_style]
+          if pet_type.body_id.nil?
+            raise UnexpectedDataFormat,
+              "can't process alt style on first occurrence of pet type"
+          end
+
+          # Try using this pet for the pet type's thumbnail, but don't worry
+          # if it fails.
+          begin
+            pet_type.consider_pet_image(@custom_pet[:name])
+          rescue => error
+            Rails.logger.warn "Failed to load pet image: #{error.full_message}"
+          end
+        end
+      end
+    end
+
+    def pet_state
+      @pet_state ||= begin
+        # Choose the right biology, depending on if there's an alt style.
+        pet_state_biology = @custom_pet[:alt_style].present? ?
+          @custom_pet[:original_biology] :
+          @custom_pet[:biology_by_zone]
+        raise UnexpectedDataFormat if pet_state_biology.empty?
+
+        # Then, set up the biology assets.
+        body_id = @custom_pet[:body_id].to_i
+        swf_assets = pet_state_biology.values.map do |biology_data|
+          SwfAsset.from_biology_data(body_id, biology_data)
+        end
+        swf_asset_ids = swf_assets.map(&:remote_id).sort.join(",")
+
+        # Then, set up the pet state, using these biology assets.
+        pet_type.pet_states.find_or_initialize_by(swf_asset_ids:).tap do |pet_state|
+          pet_state.swf_assets = swf_assets
+        end
+      end
+    end
+
+    def alt_style
+      @alt_style ||= begin
+        return nil unless @custom_pet[:alt_style]
+
+        raise UnexpectedDataFormat unless @custom_pet[:alt_color]
+        raise UnexpectedDataFormat if @custom_pet[:biology_by_zone].empty?
+
+        id = @custom_pet[:alt_style].to_i
+        AltStyle.find_or_initialize_by(id:).tap do |alt_style|
+          alt_style.assign_attributes(
+            color_id: @custom_pet[:alt_color].to_i,
+            species_id: @custom_pet[:species_id].to_i,
+            body_id: @custom_pet[:body_id].to_i,
+            biology: @custom_pet[:biology_by_zone],
+          )
+        end
+      end
+    end
+
+    def items
+      @items ||= Item.collection_from_pet_type_and_registries(
+        pet_type, @object_info_registry, @object_asset_registry
+      )
+    end
+  end
 end
 

app/models/pet_state.rb

@@ -6,8 +6,7 @@ class PetState < ApplicationRecord
   has_many :contributions, :as => :contributed,
     :inverse_of => :contributed # in case of duplicates being merged
   has_many :outfits
-  has_many :parent_swf_asset_relationships, :as => :parent,
-    :autosave => false
+  has_many :parent_swf_asset_relationships, :as => :parent
   has_many :swf_assets, :through => :parent_swf_asset_relationships
 
   belongs_to :pet_type
@@ -15,8 +14,6 @@ class PetState < ApplicationRecord
   delegate :species_id, :species, :color_id, :color, to: :pet_type
 
   alias_method :swf_asset_ids_from_association, :swf_asset_ids
-  
-  attr_writer :parent_swf_asset_relationships_to_update
 
   # A simple ordering that tries to bring reliable pet states to the front.
   scope :emotion_order, -> {
@@ -135,71 +132,10 @@ class PetState < ApplicationRecord
     self['swf_asset_ids'] = ids
   end
 
-  def handle_assets!
-    @parent_swf_asset_relationships_to_update.each do |rel|
-      rel.swf_asset.save!
-      rel.save!
-    end
-  end
-
   def to_param
     "#{id}-#{pose.split('_').map(&:capitalize).join('-')}"
   end
 
-  def self.from_pet_type_and_biology_info(pet_type, info)
-    swf_asset_ids = []
-    info.each do |zone_id, asset_info|
-      if zone_id.present? && asset_info
-        swf_asset_ids << asset_info[:part_id].to_i
-      end
-    end
-    swf_asset_ids_str = swf_asset_ids.sort.join(',')
-    if pet_type.new_record?
-      pet_state = self.new :swf_asset_ids => swf_asset_ids_str
-    else
-      pet_state = self.find_or_initialize_by(
-        pet_type_id: pet_type.id,
-        swf_asset_ids: swf_asset_ids_str
-      )
-    end
-    existing_swf_assets = SwfAsset.biology_assets.includes(:zone).
-      where(remote_id: swf_asset_ids)
-    existing_swf_assets_by_id = {}
-    existing_swf_assets.each do |swf_asset|
-      existing_swf_assets_by_id[swf_asset.remote_id] = swf_asset
-    end
-    existing_relationships_by_swf_asset_id = {}
-    unless pet_state.new_record?
-      pet_state.parent_swf_asset_relationships.each do |relationship|
-        existing_relationships_by_swf_asset_id[relationship.swf_asset_id] = relationship
-      end
-    end
-    pet_state.pet_type = pet_type # save the second case from having to look it up by ID
-    relationships = []
-    info.each do |zone_id, asset_info|
-      if zone_id.present? && asset_info
-        swf_asset_id = asset_info[:part_id].to_i
-        swf_asset = existing_swf_assets_by_id[swf_asset_id]
-        unless swf_asset
-          swf_asset = SwfAsset.new
-          swf_asset.remote_id = swf_asset_id
-        end
-        swf_asset.origin_biology_data = asset_info
-        swf_asset.origin_pet_type = pet_type
-        relationship = existing_relationships_by_swf_asset_id[swf_asset.id]
-        unless relationship
-          relationship ||= ParentSwfAssetRelationship.new
-          relationship.parent = pet_state
-          relationship.swf_asset_id = swf_asset.id
-        end
-        relationship.swf_asset = swf_asset
-        relationships << relationship
-      end
-    end
-    pet_state.parent_swf_asset_relationships_to_update = relationships
-    pet_state
-  end
-
   private
 
   # A helper for the `pose=` method.

app/models/pet_type.rb

@@ -57,6 +57,14 @@ class PetType < ApplicationRecord
     basic_image_hash || self['image_hash'] || 'deadbeef'
   end
 
+  def consider_pet_image(pet_name)
+    # If we already have a basic image hash, don't worry about it!
+    return if basic_image_hash?
+
+    # Otherwise, use this as the new image hash for this pet type.
+    self.image_hash = Neopets::CustomPets.fetch_image_hash(pet_name)
+  end
+
   def possibly_new_color
     self.color || Color.new(id: self.color_id)
   end
@@ -71,11 +79,6 @@ class PetType < ApplicationRecord
                    species_human_name: possibly_new_species.human_name)
   end
 
-  def add_pet_state_from_biology!(biology)
-    pet_state = PetState.from_pet_type_and_biology_info(self, biology)
-    pet_state
-  end
-
   def canonical_pet_state
     # For consistency (randomness is always scary!), we use the PetType ID to
     # determine which gender to prefer, if it's not built into the color. That

spec/models/pet_spec.rb

@@ -32,23 +32,16 @@ RSpec.describe Pet, type: :model do
         it("is saved when saving the pet") { pet.save!; should be_persisted }
 
         describe "its biology assets" do
-          # TODO: I wish biology assets were set up before saving.
-          # Once we change this, we can un-mark some tests as pending.
-          before { pet.save! }
-
           subject(:biology_assets) { pet_state.swf_assets }
           let(:asset_ids) { biology_assets.map(&:remote_id) }
 
-          they("are all new") do
-            pending("Currently, pets must be saved before assets are assigned.")
-            should all be_new_record
-          end
+          they("are all new") { should all be_new_record }
           they("match the expected IDs") do
             expect(asset_ids).to contain_exactly(10083, 11613, 14187, 14189)
           end
           they("are saved when saving the pet") { pet.save!; should all be_persisted }
           they("have the expected asset metadata") do
-            expect(pet_state.swf_assets).to contain_exactly(
+            should contain_exactly(
               a_record_matching(
                 type: "biology",
                 remote_id: 10083,
@@ -144,23 +137,25 @@ RSpec.describe Pet, type: :model do
           it("is saved when saving the pet") { new_pet.save!; should be_persisted }
 
           describe "its biology assets" do
-            # TODO: I wish biology assets were set up before saving.
-            # Once we change this, we can un-mark some tests as pending.
-            before { new_pet.save! }
-
             subject(:biology_assets) { pet_state.swf_assets }
             let(:asset_ids) { biology_assets.map(&:remote_id) }
+            let(:persisted_asset_ids) {
+              biology_assets.select(&:persisted?).map(&:remote_id)
+            }
+            let(:new_asset_ids) {
+              biology_assets.select(&:new_record?).map(&:remote_id)
+            }
 
             they("are partially new, partially existing") do
-              pending("Currently, pets must be saved before assets are assigned.")
-              fail # TODO: Write this test once we have the ability to see it pass!
+              expect(persisted_asset_ids).to contain_exactly(10083, 11613)
+              expect(new_asset_ids).to contain_exactly(10448, 10451)
             end
             they("match the expected IDs") do
               expect(asset_ids).to contain_exactly(10083, 11613, 10448, 10451)
             end
             they("are saved when saving the pet") { new_pet.save!; should all be_persisted }
             they("have the expected asset metadata") do
-              expect(pet_state.swf_assets).to contain_exactly(
+              should contain_exactly(
                 a_record_matching(
                   type: "biology",
                   remote_id: 10083,
@@ -208,17 +203,10 @@ RSpec.describe Pet, type: :model do
       it("is a Striped Blumaroo") { expect(pet.pet_type.human_name).to eq "Striped Blumaroo" }
 
       describe "its biology assets" do
-        # TODO: I wish biology assets were set up before saving.
-        # Once we change this, we can un-mark some tests as pending.
-        before { pet.save! }
-
         subject(:biology_assets) { pet.pet_state.swf_assets }
         let(:asset_ids) { biology_assets.map(&:remote_id) }
 
-        they("are all new") do
-          pending("Currently, pets must be saved before assets are assigned.")
-          should all be_new_record
-        end
+        they("are all new") { should all be_new_record }
         they("match the expected IDs") do
           expect(asset_ids).to contain_exactly(331, 332, 333, 23760, 23411)
         end
@@ -416,9 +404,30 @@ RSpec.describe Pet, type: :model do
           it("has no series name yet") { expect(alt_style.real_series_name?).to be false }
           it("has no thumbnail yet") { expect(alt_style.thumbnail_url?).to be false }
           it("is saved when saving the pet") { pet.save!; should be_persisted }
-        end
 
-        # TODO: Alt style assets!
+          describe "its assets" do
+            subject(:assets) { alt_style.swf_assets }
+            let(:asset_ids) { assets.map(&:remote_id) }
+
+            they("are all new") { should all be_new_record }
+            they("match the expected IDs") do
+              expect(asset_ids).to contain_exactly(56223)
+            end
+            they("are saved when saving the pet") { pet.save!; should all be_persisted }
+            they("have the expected asset metadata") do
+              should contain_exactly(
+                a_record_matching(
+                  type: "biology",
+                  remote_id: 56223,
+                  zone_id: 15,
+                  url: "https://images.neopets.com/cp/bio/swf/000/000/056/56223_dc26edc764.swf",
+                  manifest_url: "https://images.neopets.com/cp/bio/data/000/000/056/56223_dc26edc764/manifest.json",
+                  zones_restrict: "0000000000000000000000000000000000000000000000000000",
+                )
+              )
+            end
+          end
+        end
 
         context "when modeled a second time" do
           before { pet.save! }
@@ -432,6 +441,16 @@ RSpec.describe Pet, type: :model do
             it "is not changed when saving the pet" do
               expect { new_pet.save! }.not_to change { alt_style.attributes }
             end
+
+            describe "its assets" do
+              subject(:assets) { alt_style.swf_assets }
+
+              they("already exist") { should all be_persisted }
+              they("are the same as before") { should eq pet.alt_style.swf_assets }
+              they("are not changed when saving the pet") do
+                expect { new_pet.save! }.not_to change { assets.map(&:attributes) }
+              end
+            end
           end
         end
       end