Refactor pet image hash loading into the Pet model, not PetType

Just a small thing, I guess when I was a kid I did a weird thing where
I attached `origin_pet` to `PetType`, then upon saving `PetType` I
loaded the image hash for the pet to save as the pet type's new image
hash.

I guess this does have the nice property of not bothering to load that
stuff until we need it? But whatever, I'm moving this into `Pet` both
to simplify the relationship between the models, and to prepare for
another potential refactor: using `PetService.getPet` for this instead!

app/models/pet.rb

@@ -43,7 +43,9 @@ class Pet < ApplicationRecord
       species_id: pet_data[:species_id].to_i,
       color_id: pet_data[:color_id].to_i
     )
-    self.pet_type.origin_pet = self
+
+    new_image_hash = Pet.fetch_image_hash(self.name)
+    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
@@ -158,6 +160,48 @@ class Pet < ApplicationRecord
     HashWithIndifferentAccess.new(envelope.messages[0].data.body)
   end
 
+  # Given a pet's name, load its image hash, for use in `pets.neopets.com`
+  # image URLs. (This corresponds to its current biology and items.)
+  IMAGE_CPN_FORMAT = 'https://pets.neopets.com/cpn/%s/1/1.png';
+  IMAGE_CP_LOCATION_REGEX = %r{^/cp/(.+?)/[0-9]+/[0-9]+\.png$};
+  IMAGE_CPN_ACCEPTABLE_NAME = /^[A-Za-z0-9_]+$/
+  def self.fetch_image_hash(name)
+    return nil unless name.match?(IMAGE_CPN_ACCEPTABLE_NAME)
+
+    cpn_uri = URI.parse sprintf(IMAGE_CPN_FORMAT, CGI.escape(name))
+    begin
+      res = Net::HTTP.get_response(cpn_uri, {
+        'User-Agent' => Rails.configuration.user_agent_for_neopets
+      })
+    rescue Exception => e
+      raise DownloadError, e.message
+    end
+    unless res.is_a? Net::HTTPFound
+      begin
+        res.error!
+      rescue Exception => e
+        Rails.logger.warn "Error loading CPN image at #{cpn_uri}: #{e.message}"
+        return
+      else
+        Rails.logger.warn "Error loading CPN image at #{cpn_uri}. Response: #{res.inspect}"
+        return
+      end
+    end
+    new_url = res['location']
+    new_image_hash = get_hash_from_cp_path(new_url)
+    if new_image_hash
+      Rails.logger.info "Successfully loaded #{cpn_uri}, with image hash #{new_image_hash}"
+      new_image_hash
+    else
+      Rails.logger.warn "CPN image pointed to #{new_url}, which does not match CP image format"
+    end
+  end
+
+  def self.get_hash_from_cp_path(path)
+    match = path.match(IMAGE_CP_LOCATION_REGEX)
+    match ? match[1] : nil
+  end
+
   class PetNotFound < RuntimeError;end
   class DownloadError < RuntimeError;end
   class UnexpectedDataFormat < RuntimeError;end

app/models/pet_type.rb

@@ -1,8 +1,4 @@
 class PetType < ApplicationRecord
-  IMAGE_CPN_FORMAT = 'https://pets.neopets.com/cpn/%s/1/1.png';
-  IMAGE_CP_LOCATION_REGEX = %r{^/cp/(.+?)/[0-9]+/[0-9]+\.png$};
-  IMAGE_CPN_ACCEPTABLE_NAME = /^[a-z0-9_]+$/
-
   # NOTE: While a pet type does always functionally belong to a color and
   # species, sometimes we don't have that record yet, in which case color_id
   # or species_id will refer to a nonexistant record, and the site should
@@ -13,8 +9,6 @@ class PetType < ApplicationRecord
   has_many :pet_states
   has_many :pets
 
-  attr_writer :origin_pet
-
   BasicHashes = YAML::load_file(Rails.root.join('config', 'basic_type_hashes.yml'))
   
   scope :matching_name, ->(color_name, species_name) {
@@ -23,8 +17,6 @@ class PetType < ApplicationRecord
     where(color_id: color.id, species_id: species.id)
   }
 
-  before_save :load_image_hash
-
   def self.special_color_or_basic(special_color)
     color_ids = special_color ? [special_color.id] : Color.basic.select([:id]).map(&:id)
     where(color_id: color_ids)
@@ -41,11 +33,6 @@ class PetType < ApplicationRecord
     random_pet_types
   end
 
-  def self.get_hash_from_cp_path(path)
-    match = path.match(IMAGE_CP_LOCATION_REGEX)
-    match ? match[1] : nil
-  end
-
   def as_json(options={})
     super({
       only: [:id],
@@ -112,38 +99,6 @@ class PetType < ApplicationRecord
     pet_state
   end
 
-  def load_image_hash
-    if @origin_pet && @origin_pet.name =~ IMAGE_CPN_ACCEPTABLE_NAME
-      cpn_uri = URI.parse sprintf(IMAGE_CPN_FORMAT, CGI.escape(@origin_pet.name))
-      begin
-        res = Net::HTTP.get_response(cpn_uri, {
-          'User-Agent' => Rails.configuration.user_agent_for_neopets
-        })
-      rescue Exception => e
-        raise DownloadError, e.message
-      end
-      unless res.is_a? Net::HTTPFound
-        begin
-          res.error!
-        rescue Exception => e
-          Rails.logger.warn "Error loading CPN image at #{cpn_uri}: #{e.message}"
-          return
-        else
-          Rails.logger.warn "Error loading CPN image at #{cpn_uri}. Response: #{res.inspect}"
-          return
-        end
-      end
-      new_url = res['location']
-      new_image_hash = PetType.get_hash_from_cp_path(new_url)
-      if new_image_hash
-        self.image_hash = new_image_hash
-        Rails.logger.info "Successfully loaded #{cpn_uri}, saved image hash #{new_image_hash}"
-      else
-        Rails.logger.warn "CPN image pointed to #{new_url}, which does not match CP image format"
-      end
-    end
-  end
-
   def canonical_pet_state
     # For consistency (randomness is always scary!), we use the PetType ID to
     # determine which gender to prefer. That way, it'll be stable, but we'll