From e3d196fe876ea1efb31b8a5cd907600050efcdb2 Mon Sep 17 00:00:00 2001 From: Emi Matchu Date: Wed, 6 Nov 2024 14:31:16 -0800 Subject: [PATCH] Revert modeling refactors to the old modeling that worked! Because we ended up with such a big error, and it doesn't have an easy fix, I'm wrapping up today by reverting the entire set of refactors we've done lately, so modeling in production can continue while we improve this code further over time. I generated this commit by hand-picking the refactor-y commits recently, running `git revert --no-commit ` in reverse order, then manually updating `pet_spec.rb` to reflect the state of the code: passing the most important behavioral tests, but no longer passing one of the kinds of annoyances I *did* fix in the new code. ```shell git revert --no-commit 48c1a58df9a170a2d9ad213f5424fda91177fcba git revert --no-commit 42e7eabdd8fb8640dd71c38b42a1b515ec572dfd git revert --no-commit d82c7f817ae43f95db473b6d3c66bae53b16ae00 git revert --no-commit 5264947608c81176320981e9bccd9b02f2089db6 git revert --no-commit 90407403baf0b504fca99fe3a9a6a90ed8b9a66d git revert --no-commit 242b85470d1e5e161f2a49c16faaad38f052b74f git revert --no-commit 9eaee4a2d4cdf1a956c3710a3a307e8f0c383ecb git revert --no-commit 52ca41dbff4c08165012afa774677c938bdfcaf3 git revert --no-commit c03e7446e3d5e1323414039555d11a76abce2efe git revert --no-commit f81415d3270e7bfee17101475c8b98b79e32fde1 git revert --no-commit 13ceec8fcc866fcc0a81e86a74b6e79b5f7d4f6c ``` --- app/controllers/pets_controller.rb | 4 +- app/models/alt_style.rb | 9 ++ app/models/item.rb | 133 ++++++++++++++++-- app/models/parent_swf_asset_relationship.rb | 4 +- app/models/pet.rb | 87 ++++++++++-- app/models/pet/modeling_snapshot.rb | 103 -------------- app/models/pet_state.rb | 88 ++++++++++-- app/models/pet_type.rb | 13 +- app/models/swf_asset.rb | 81 +++++------ config/environments/development.rb | 4 - config/environments/production.rb | 4 - config/environments/test.rb | 4 - spec/models/pet_spec.rb | 146 ++++++++++++++++++-- 13 files changed, 470 insertions(+), 210 deletions(-) delete mode 100644 app/models/pet/modeling_snapshot.rb diff --git a/app/controllers/pets_controller.rb b/app/controllers/pets_controller.rb index 97b25329..8dbaf3e4 100644 --- a/app/controllers/pets_controller.rb +++ b/app/controllers/pets_controller.rb @@ -1,10 +1,12 @@ class PetsController < ApplicationController rescue_from Neopets::CustomPets::PetNotFound, with: :pet_not_found rescue_from Neopets::CustomPets::DownloadError, with: :pet_download_error - rescue_from Pet::ModelingDisabled, with: :modeling_disabled rescue_from Pet::UnexpectedDataFormat, with: :unexpected_data_format def load + # Uncomment this to temporarily disable modeling for most users. + # return modeling_disabled unless user_signed_in? && current_user.admin? + raise Neopets::CustomPets::PetNotFound unless params[:name] @pet = Pet.load(params[:name]) points = contribute(current_user, @pet) diff --git a/app/models/alt_style.rb b/app/models/alt_style.rb index b822a2dd..0e9a47bf 100644 --- a/app/models/alt_style.rb +++ b/app/models/alt_style.rb @@ -74,6 +74,15 @@ class AltStyle < ApplicationRecord Item.appearances_for(items, self, ...) end + def biology=(biology) + # TODO: This is very similar to what `PetState` does, but like… much much + # more compact? Idk if I'm missing something, or if I was just that much + # more clueless back when I wrote it, lol 😅 + self.swf_assets = biology.values.map do |asset_data| + SwfAsset.from_biology_data(self.body_id, asset_data) + end + end + # At time of writing, most batches of Alt Styles thumbnails used a simple # pattern for the item thumbnail URL, but that's not always the case anymore. # For now, let's keep using this format as the default value when creating a diff --git a/app/models/item.rb b/app/models/item.rb index c58f2074..8be75718 100644 --- a/app/models/item.rb +++ b/app/models/item.rb @@ -26,8 +26,6 @@ class Item < ApplicationRecord validates_presence_of :name, :description, :thumbnail_url, :rarity, :price, :zones_restrict - before_validation :update_cached_fields - attr_writer :current_body_id, :owned, :wanted NCRarities = [0, 500] @@ -267,11 +265,7 @@ class Item < ApplicationRecord def update_cached_fields self.cached_occupied_zone_ids = occupied_zone_ids self.cached_compatible_body_ids = compatible_body_ids(use_cached: false) - end - - def update_cached_fields! - update_cached_fields - save! + self.save! end def species_support_ids @@ -410,6 +404,33 @@ class Item < ApplicationRecord return PetType.all if compatible_body_ids.include?(0) PetType.where(body_id: compatible_body_ids) end + + def handle_assets! + if @parent_swf_asset_relationships_to_update && @current_body_id + new_swf_asset_ids = @parent_swf_asset_relationships_to_update.map(&:swf_asset_id) + rels = ParentSwfAssetRelationship.arel_table + swf_assets = SwfAsset.arel_table + + # If a relationship used to bind an item and asset for this body type, + # but doesn't in this sample, the two have been unbound. Delete the + # relationship. + ids_to_delete = self.parent_swf_asset_relationships. + select(rels[:id]). + joins(:swf_asset). + where(rels[:swf_asset_id].not_in(new_swf_asset_ids)). + where(swf_assets[:body_id].in([@current_body_id, 0])). + map(&:id) + + unless ids_to_delete.empty? + ParentSwfAssetRelationship.where(:id => ids_to_delete).delete_all + end + + @parent_swf_asset_relationships_to_update.each do |rel| + rel.save! + rel.swf_asset.save! + end + end + end def body_specific? # If there are species support IDs (it's not empty), the item is @@ -417,7 +438,7 @@ class Item < ApplicationRecord explicitly_body_specific? || !species_support_ids.empty? end - def add_origin_registry_info(info) + def add_origin_registry_info(info, locale) # bear in mind that numbers from registries are floats species_support_strs = info['species_support'] || [] self.species_support_ids = species_support_strs.map(&:to_i) @@ -436,6 +457,16 @@ class Item < ApplicationRecord self.zones_restrict = info['zones_restrict'] end + def pending_swf_assets + @parent_swf_asset_relationships_to_update.inject([]) do |all_swf_assets, relationship| + all_swf_assets << relationship.swf_asset + end + end + + def parent_swf_asset_relationships_to_update=(rels) + @parent_swf_asset_relationships_to_update = rels + end + # NOTE: Adding the JSON serializer makes `as_json` treat this like a model # instead of like a hash, so you can target its children with things like # the `include` option. This feels clunky though, I wish I had something a @@ -607,4 +638,90 @@ class Item < ApplicationRecord items end + + def self.collection_from_pet_type_and_registries(pet_type, info_registry, asset_registry, scope=Item.all) + # bear in mind that registries are arrays with many nil elements, + # due to how the parser works + + # Collect existing items + items = {} + item_ids = [] + info_registry.each do |item_id, info| + if info && info[:is_compatible] + item_ids << item_id.to_i + end + end + + # Collect existing relationships + existing_relationships_by_item_id_and_swf_asset_id = {} + existing_items = scope.where(id: item_ids). + includes(:parent_swf_asset_relationships) + existing_items.each do |item| + items[item.id] = item + relationships_by_swf_asset_id = {} + item.parent_swf_asset_relationships.each do |relationship| + relationships_by_swf_asset_id[relationship.swf_asset_id] = relationship + end + existing_relationships_by_item_id_and_swf_asset_id[item.id] = + relationships_by_swf_asset_id + end + + # Collect existing assets + swf_asset_ids = [] + asset_registry.each do |asset_id, asset_data| + swf_asset_ids << asset_id.to_i if asset_data + end + existing_swf_assets = SwfAsset.object_assets.includes(:zone). + where(remote_id: swf_asset_ids) + existing_swf_assets_by_remote_id = {} + existing_swf_assets.each do |swf_asset| + existing_swf_assets_by_remote_id[swf_asset.remote_id] = swf_asset + end + + # With each asset in the registry, + relationships_by_item_id = {} + asset_registry.each do |asset_id, asset_data| + if asset_data + # Build and update the item + item_id = asset_data[:obj_info_id].to_i + next unless item_ids.include?(item_id) # skip incompatible (Uni Bug) + item = items[item_id] + unless item + item = Item.new + item.id = item_id + items[item_id] = item + end + item.add_origin_registry_info info_registry[item.id.to_s], I18n.default_locale + item.current_body_id = pet_type.body_id + + # Build and update the SWF + swf_asset_remote_id = asset_data[:asset_id].to_i + swf_asset = existing_swf_assets_by_remote_id[swf_asset_remote_id] + unless swf_asset + swf_asset = SwfAsset.new + swf_asset.remote_id = swf_asset_remote_id + end + swf_asset.origin_object_data = asset_data + swf_asset.origin_pet_type = pet_type + swf_asset.item = item + + # Build and update the relationship + relationship = existing_relationships_by_item_id_and_swf_asset_id[item.id][swf_asset.id] rescue nil + unless relationship + relationship = ParentSwfAssetRelationship.new + relationship.parent = item + end + relationship.swf_asset = swf_asset + relationships_by_item_id[item_id] ||= [] + relationships_by_item_id[item_id] << relationship + end + end + + # Set up the relationships to be updated on item save + relationships_by_item_id.each do |item_id, relationships| + items[item_id].parent_swf_asset_relationships_to_update = relationships + end + + items.values + end end diff --git a/app/models/parent_swf_asset_relationship.rb b/app/models/parent_swf_asset_relationship.rb index 71d8b0cc..6f24bfad 100644 --- a/app/models/parent_swf_asset_relationship.rb +++ b/app/models/parent_swf_asset_relationship.rb @@ -1,7 +1,7 @@ class ParentSwfAssetRelationship < ApplicationRecord self.table_name = 'parents_swf_assets' - belongs_to :parent, polymorphic: true + belongs_to :parent, :polymorphic => true belongs_to :swf_asset @@ -21,6 +21,6 @@ class ParentSwfAssetRelationship < ApplicationRecord end def update_parent_cached_fields - parent.try(:update_cached_fields!) + parent.try(:update_cached_fields) end end diff --git a/app/models/pet.rb b/app/models/pet.rb index 40eeb57c..3765bec9 100644 --- a/app/models/pet.rb +++ b/app/models/pet.rb @@ -4,17 +4,66 @@ class Pet < ApplicationRecord attr_reader :items, :pet_state, :alt_style def load!(timeout: nil) - raise ModelingDisabled unless Rails.configuration.modeling_enabled - - viewer_data_hash = Neopets::CustomPets.fetch_viewer_data(name, timeout:) - use_modeling_snapshot(ModelingSnapshot.new(viewer_data_hash)) + viewer_data = Neopets::CustomPets.fetch_viewer_data(name, timeout:) + use_viewer_data(viewer_data) end - def use_modeling_snapshot(snapshot) - self.pet_type = snapshot.pet_type - @pet_state = snapshot.pet_state - @alt_style = snapshot.alt_style - @items = snapshot.items + 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]) end def wardrobe_query @@ -40,9 +89,21 @@ class Pet < ApplicationRecord before_validation do pet_type.save! - @pet_state.save! if @pet_state - @alt_style.save! if @alt_style - (@items || []).each(&:save!) + if @pet_state + @pet_state.save! + @pet_state.handle_assets! + end + + if @items + @items.each do |item| + item.save! if item.changed? + item.handle_assets! + end + end + + if @alt_style + @alt_style.save! + end end def self.load(name, **options) @@ -52,5 +113,5 @@ class Pet < ApplicationRecord end class UnexpectedDataFormat < RuntimeError;end - class ModelingDisabled < RuntimeError;end end + diff --git a/app/models/pet/modeling_snapshot.rb b/app/models/pet/modeling_snapshot.rb deleted file mode 100644 index 0d500836..00000000 --- a/app/models/pet/modeling_snapshot.rb +++ /dev/null @@ -1,103 +0,0 @@ -# A representation of a Neopets::CustomPets viewer data response, translated -# to DTI's database models! -class Pet::ModelingSnapshot - 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 Pet::UnexpectedDataFormat unless @custom_pet[:species_id] - raise Pet::UnexpectedDataFormat unless @custom_pet[:color_id] - raise Pet::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 Pet::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 - swf_asset_ids = biology_assets.map(&:remote_id) - pet_type.pet_states.find_or_initialize_by(swf_asset_ids:).tap do |pet_state| - pet_state.swf_assets = biology_assets - end - end - end - - def alt_style - @alt_style ||= begin - return nil unless @custom_pet[:alt_style] - raise Pet::UnexpectedDataFormat unless @custom_pet[:alt_color] - - 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, - swf_assets: alt_style_assets, - ) - end - end - end - - def items - @items ||= @object_info_registry.map do |id, item_data| - Item.find_or_initialize_by(id:).tap do |item| - item.add_origin_registry_info item_data - item.swf_assets = item_assets_for id - end - end - end - - private - - def biology_assets - @biology_assets ||= begin - biology = @custom_pet[:alt_style].present? ? - @custom_pet[:original_biology] : - @custom_pet[:biology_by_zone] - assets_from_biology(biology) - end - end - - def item_assets_for(item_id) - all_infos = @object_asset_registry.values - infos = all_infos.select { |a| a[:obj_info_id].to_i == item_id.to_i } - infos.map do |asset_data| - SwfAsset.from_object_data(@custom_pet[:body_id], asset_data) - end - end - - def alt_style_assets - raise Pet::UnexpectedDataFormat if @custom_pet[:biology_by_zone].empty? - @alt_style_assets ||= assets_from_biology(@custom_pet[:biology_by_zone]) - end - - def assets_from_biology(biology) - raise Pet::UnexpectedDataFormat if biology.empty? - body_id = @custom_pet[:body_id].to_i - biology.values.map { |b| SwfAsset.from_biology_data(body_id, b) } - end -end diff --git a/app/models/pet_state.rb b/app/models/pet_state.rb index f3e33f9f..61671777 100644 --- a/app/models/pet_state.rb +++ b/app/models/pet_state.rb @@ -6,16 +6,17 @@ 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 + has_many :parent_swf_asset_relationships, :as => :parent, + :autosave => false has_many :swf_assets, :through => :parent_swf_asset_relationships - serialize :swf_asset_ids, coder: Serializers::IntegerSet, type: Array - belongs_to :pet_type 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, -> { @@ -94,16 +95,85 @@ class PetState < ApplicationRecord end end + def sort_swf_asset_ids! + self.swf_asset_ids = swf_asset_ids_array.sort.join(',') + end + + def swf_asset_ids + self['swf_asset_ids'] + end + + def swf_asset_ids_array + swf_asset_ids.split(',').map(&:to_i) + end + + def swf_asset_ids=(ids) + 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 - # Because our column is named `swf_asset_ids`, we need to ensure writes to - # it go to the attribute, and not the thing ActiveRecord does of finding the - # relevant `swf_assets`. - # TODO: Consider renaming the column to `cached_swf_asset_ids`? - def swf_asset_ids=(new_swf_asset_ids) - write_attribute(:swf_asset_ids, new_swf_asset_ids) + 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 diff --git a/app/models/pet_type.rb b/app/models/pet_type.rb index a73f510d..055cca2d 100644 --- a/app/models/pet_type.rb +++ b/app/models/pet_type.rb @@ -57,14 +57,6 @@ 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 @@ -79,6 +71,11 @@ 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 diff --git a/app/models/swf_asset.rb b/app/models/swf_asset.rb index ba911ece..8b2b283d 100644 --- a/app/models/swf_asset.rb +++ b/app/models/swf_asset.rb @@ -276,25 +276,26 @@ class SwfAsset < ApplicationRecord return items.any? { |i| i.body_specific? } end - def add_compatible_body_id(new_body_id) - if !body_specific? - # If this asset is already known to not be body-specific (e.g. by its - # zone), use 0 as our body ID. - self.body_id = 0 - elsif body_id.nil? - # If this asset has not body ID yet, assume this is it. (It's possible it - # might also fit other pet bodies, which we'll discover soon enough!) - self.body_id = new_body_id - elsif body_id != new_body_id - # If this asset already has a body ID, but this one is different, assume - # it actually fits all bodies, and use 0. (Note that we're talking about - # *asset* compatibility here, not *item* compatibility. Seeing the same - # *asset* on two bodies is a pretty sure sign it fits them all!) - self.body_id = 0 - else - # Otherwise, we already have a body ID, and it matches the incoming one. - # No change! - end + def origin_pet_type=(pet_type) + self.body_id = pet_type.body_id + end + + def origin_biology_data=(data) + Rails.logger.debug("my biology data is: #{data.inspect}") + self.type = 'biology' + self.zone_id = data[:zone_id].to_i + self.url = data[:asset_url] + self.zones_restrict = data[:zones_restrict] + self.manifest_url = data[:manifest] + end + + def origin_object_data=(data) + Rails.logger.debug("my object data is: #{data.inspect}") + self.type = 'object' + self.zone_id = data[:zone_id].to_i + self.url = data[:asset_url] + self.zones_restrict = "" + self.manifest_url = data[:manifest] end def normalize_manifest_url @@ -303,32 +304,20 @@ class SwfAsset < ApplicationRecord self.manifest_url = parsed_manifest_url.to_s end - def self.from_biology_data(body_id, data) - type = "biology" - remote_id = data[:part_id].to_i - SwfAsset.find_or_initialize_by(type:, remote_id:).tap do |swf_asset| - swf_asset.assign_attributes( - body_id:, - zone_id: data[:zone_id].to_i, - url: data[:asset_url], - manifest_url: data[:manifest], - zones_restrict: data[:zones_restrict], - ) - end + # To manually change the body ID without triggering the usual change to 0, + # use this override method. (This is intended for use from the console.) + def override_body_id(new_body_id) + @body_id_overridden = true + self.body_id = new_body_id end - def self.from_object_data(body_id, data) - type = "object" - remote_id = data[:asset_id].to_i - SwfAsset.find_or_initialize_by(type:, remote_id:).tap do |swf_asset| - swf_asset.assign_attributes( - zone_id: data[:zone_id].to_i, - url: data[:asset_url], - manifest_url: data[:manifest], - zones_restrict: "", - ) - swf_asset.add_compatible_body_id(body_id) - end + def self.from_biology_data(body_id, data) + remote_id = data[:part_id].to_i + swf_asset = SwfAsset.find_or_initialize_by type: 'biology', + remote_id: remote_id + swf_asset.body_id = body_id + swf_asset.origin_biology_data = data + swf_asset end # Given a list of SWF assets, ensure all of their manifests are loaded, with @@ -368,4 +357,10 @@ class SwfAsset < ApplicationRecord swf_assets.select(&:changed?).each(&:save!) end end + + before_save do + # If an asset body ID changes, that means more than one body ID has been + # linked to it, meaning that it's probably wearable by all bodies. + self.body_id = 0 if !@body_id_overridden && (!self.body_specific? || (!self.new_record? && self.body_id_changed?)) + end end diff --git a/config/environments/development.rb b/config/environments/development.rb index 8f374d14..41211e87 100644 --- a/config/environments/development.rb +++ b/config/environments/development.rb @@ -103,10 +103,6 @@ Rails.application.configure do # Allow connections on Vagrant's private network. config.web_console.permissions = '10.0.2.2' - # Allow pets to model new data. (If modeling is ever broken, disable this in - # production while we fix it!) - config.modeling_enabled = true - # Use a local copy of Impress 2020, presumably running on port 4000. (Can # override this with the IMPRESS_2020_ORIGIN environment variable!) config.impress_2020_origin = ENV.fetch("IMPRESS_2020_ORIGIN", diff --git a/config/environments/production.rb b/config/environments/production.rb index 58eae953..370e0246 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -122,10 +122,6 @@ Rails.application.configure do # Skip DNS rebinding protection for the default health check endpoint. # config.host_authorization = { exclude: ->(request) { request.path == "/up" } } - # Allow pets to model new data. (If modeling is ever broken, disable this - # here while we fix it!) - config.modeling_enabled = false - # Use the live copy of Impress 2020. (Can override this with the # IMPRESS_2020_ORIGIN environment variable!) config.impress_2020_origin = ENV.fetch("IMPRESS_2020_ORIGIN", diff --git a/config/environments/test.rb b/config/environments/test.rb index e57cbb53..f677ede2 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -62,10 +62,6 @@ Rails.application.configure do # Raise error when a before_action's only/except options reference missing actions config.action_controller.raise_on_missing_callback_actions = true - # Allow pets to model new data. (If modeling is ever broken, disable this in - # production while we fix it!) - config.modeling_enabled = true - # Use a local copy of Impress 2020, presumably running on port 4000. (Can # override this with the IMPRESS_2020_ORIGIN environment variable!) config.impress_2020_origin = ENV.fetch("IMPRESS_2020_ORIGIN", diff --git a/spec/models/pet_spec.rb b/spec/models/pet_spec.rb index b13ba65f..a6eb874e 100644 --- a/spec/models/pet_spec.rb +++ b/spec/models/pet_spec.rb @@ -36,11 +36,52 @@ RSpec.describe Pet, type: :model do let(:asset_ids) { biology_assets.map(&:remote_id) } they("are all new") { should all be_new_record } - they("match the expected IDs") do + pending("match the expected IDs (before saving)") do + expect(asset_ids).to contain_exactly(10083, 11613, 14187, 14189) + end + they("match the expected IDs (after saving)") do + pet.save! # TODO: Remove this test once the above passes. 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 + pending("have the expected asset metadata (before saving)") do + should contain_exactly( + a_record_matching( + type: "biology", + remote_id: 10083, + zone_id: 37, + url: "https://images.neopets.com/cp/bio/swf/000/000/010/10083_8a1111a13f.swf", + manifest_url: "https://images.neopets.com/cp/bio/data/000/000/010/10083_8a1111a13f/manifest.json", + zones_restrict: "0000000000000000000000000000000000000000000000000000", + ), + a_record_matching( + type: "biology", + remote_id: 11613, + zone_id: 15, + url: "https://images.neopets.com/cp/bio/swf/000/000/011/11613_f7d8d377ab.swf", + manifest_url: "https://images.neopets.com/cp/bio/data/000/000/011/11613_f7d8d377ab/manifest.json", + zones_restrict: "0000000000000000000000000000000000000000000000000000", + ), + a_record_matching( + type: "biology", + remote_id: 14187, + zone_id: 34, + url: "https://images.neopets.com/cp/bio/swf/000/000/014/14187_0e65c2082f.swf", + manifest_url: "https://images.neopets.com/cp/bio/data/000/000/014/14187_0e65c2082f/manifest.json", + zones_restrict: "0000000000000000000000000000000000000000000000000000", + ), + a_record_matching( + type: "biology", + remote_id: 14189, + zone_id: 33, + url: "https://images.neopets.com/cp/bio/swf/000/000/014/14189_102e4991e9.swf", + manifest_url: "https://images.neopets.com/cp/bio/data/000/000/014/14189_102e4991e9/manifest.json", + zones_restrict: "0000000000000000000000000000000000000000000000000000", + ) + ) + end + they("have the expected asset metadata (after saving)") do + pet.save! # TODO: Remove this test once the above passes. should contain_exactly( a_record_matching( type: "biology", @@ -146,15 +187,56 @@ RSpec.describe Pet, type: :model do biology_assets.select(&:new_record?).map(&:remote_id) } - they("are partially new, partially existing") do + pending("are partially new, partially existing") do 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 + pending("match the expected IDs (before saving)") do + expect(asset_ids).to contain_exactly(10083, 11613, 10448, 10451) + end + they("match the expected IDs (after saving)") do + new_pet.save! # TODO: Remove this test once the above passes. 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 + pending("have the expected asset metadata (before saving)") do + should contain_exactly( + a_record_matching( + type: "biology", + remote_id: 10083, + zone_id: 37, + url: "https://images.neopets.com/cp/bio/swf/000/000/010/10083_8a1111a13f.swf", + manifest_url: "https://images.neopets.com/cp/bio/data/000/000/010/10083_8a1111a13f/manifest.json", + zones_restrict: "0000000000000000000000000000000000000000000000000000", + ), + a_record_matching( + type: "biology", + remote_id: 11613, + zone_id: 15, + url: "https://images.neopets.com/cp/bio/swf/000/000/011/11613_f7d8d377ab.swf", + manifest_url: "https://images.neopets.com/cp/bio/data/000/000/011/11613_f7d8d377ab/manifest.json", + zones_restrict: "0000000000000000000000000000000000000000000000000000", + ), + a_record_matching( + type: "biology", + remote_id: 10448, + zone_id: 34, + url: "https://images.neopets.com/cp/bio/swf/000/000/010/10448_0b238e79e2.swf", + manifest_url: "https://images.neopets.com/cp/bio/data/000/000/010/10448_0b238e79e2/manifest.json", + zones_restrict: "0000000000000000000000000000000000000000000000000000", + ), + a_record_matching( + type: "biology", + remote_id: 10451, + zone_id: 33, + url: "https://images.neopets.com/cp/bio/swf/000/000/010/10451_cd4a8a8e47.swf", + manifest_url: "https://images.neopets.com/cp/bio/data/000/000/010/10451_cd4a8a8e47/manifest.json", + zones_restrict: "0000000000000000000000000000000000000000000000000000", + ) + ) + end + they("have the expected asset metadata (after saving)") do + new_pet.save! # TODO: Remove this test once the above passes. should contain_exactly( a_record_matching( type: "biology", @@ -207,7 +289,11 @@ RSpec.describe Pet, type: :model do let(:asset_ids) { biology_assets.map(&:remote_id) } they("are all new") { should all be_new_record } - they("match the expected IDs") do + pending("match the expected IDs (before saving)") do + expect(asset_ids).to contain_exactly(331, 332, 333, 23760, 23411) + end + they("match the expected IDs (after saving)") do + pet.save! # TODO: Remove this test once the above passes. expect(asset_ids).to contain_exactly(331, 332, 333, 23760, 23411) end they("are saved when saving the pet") { pet.save!; should all be_persisted } @@ -223,7 +309,7 @@ RSpec.describe Pet, type: :model do expect(item_ids).to contain_exactly(39552, 53874, 71706) end they("are saved when saving the pet") { pet.save! ; should all be_persisted } - they("have the expected item metadata") do + they("have the expected item metadata (without even saving first)") do should contain_exactly( a_record_matching( id: 39552, @@ -285,11 +371,50 @@ RSpec.describe Pet, type: :model do let(:asset_ids) { item_assets.map(&:remote_id) } they("are all new") { should all be_new_record } - they("match the expected IDs") do + pending("match the expected IDs (before saving)") do + expect(asset_ids).to contain_exactly(16933, 108567, 410722) + end + they("match the expected IDs (after saving)") do + pet.save! # TODO: Remove this test once the above passes. expect(asset_ids).to contain_exactly(16933, 108567, 410722) end they("are saved when saving the pet") { pet.save! ; should all be_persisted } - they("match the expected metadata") do + pending("match the expected metadata (before saving)") do + expect(assets_by_item).to match( + 39552 => a_collection_containing_exactly( + a_record_matching( + type: "object", + remote_id: 16933, + zone_id: 35, + url: "https://images.neopets.com/cp/items/swf/000/000/016/16933_0833353c4f.swf", + manifest_url: "https://images.neopets.com/cp/items/data/000/000/016/16933_0833353c4f/manifest.json?v=1706", + zones_restrict: "", + ) + ), + 53874 => a_collection_containing_exactly( + a_record_matching( + type: "object", + remote_id: 108567, + zone_id: 23, + url: "https://images.neopets.com/cp/items/swf/000/000/108/108567_ee88141325.swf", + manifest_url: "https://images.neopets.com/cp/items/data/000/000/108/108567_ee88141325/manifest.json?v=1706", + zones_restrict: "", + ) + ), + 71706 => a_collection_containing_exactly( + a_record_matching( + type: "object", + remote_id: 410722, + zone_id: 3, + url: "https://images.neopets.com/cp/items/swf/000/000/410/410722_3bcd2f5e11.swf", + manifest_url: "https://images.neopets.com/cp/items/data/000/000/410/410722_3bcd2f5e11/manifest.json?v=1706", + zones_restrict: "", + ) + ), + ) + end + they("match the expected metadata (after saving)") do + pet.save! # TODO: Remove this test after the above passes. expect(assets_by_item).to match( 39552 => a_collection_containing_exactly( a_record_matching( @@ -389,7 +514,6 @@ RSpec.describe Pet, type: :model do let(:compatible_body_ids) { items.to_h { |i| [i.id, i.compatible_body_ids] } } they("should be marked compatible with both pets' body IDs") do - pending("Whuh oh, we're currently replacing the assets altogether!") new_pet.save! expect(compatible_body_ids).to eq( 39552 => [47, 93], @@ -481,7 +605,7 @@ RSpec.describe Pet, type: :model do context "when modeling is disabled" do before { allow(Rails.configuration).to receive(:modeling_enabled) { false } } - it("raises an error") do + pending("raises an error") do expect { Pet.load("matts_bat") }.to raise_error(Pet::ModelingDisabled) end end