Merge pull request #622 from PRX/feat/async_validations

Async validations

Gemfile

@@ -37,7 +37,6 @@ gem "roar-rails", github: "PRX/roar-rails", branch: "feat/rails_7"
 # models
 gem "addressable"
 gem "countries"
-gem "fastimage"
 gem "paranoia"
 gem "sanitize"
 

Gemfile.lock

@@ -180,7 +180,6 @@ GEM
       railties (>= 5.0.0)
     faraday (0.17.6)
       multipart-post (>= 1.2, < 3)
-    fastimage (2.2.6)
     ffi (1.15.5)
     fuzzyurl (0.2.2)
     globalid (1.0.1)
@@ -482,7 +481,6 @@ DEPENDENCIES
   excon
   factory_bot_rails
   faraday (~> 0.17.4)
-  fastimage
   hal_api-rails (~> 1.2.0)
   hyperresource
   importmap-rails

app/controllers/application_controller.rb

@@ -45,4 +45,9 @@ def prx_auth_needs_refresh?(jwt_ttl)
   def skip_session
     request.session_options[:skip] = true
   end
+
+  # TODO: hacky, but this method is private in turbo-rails
+  def turbo_frame_request?
+    request.headers["Turbo-Frame"].present?
+  end
 end

app/controllers/episodes_controller.rb

@@ -125,9 +125,4 @@ def episode_params
       images_attributes: %i[id original_url size alt_text caption credit _destroy]
     )
   end
-
-  # TODO: hacky, but this method is private in turbo-rails
-  def turbo_frame_request?
-    request.headers["Turbo-Frame"].present?
-  end
 end

app/helpers/uploads_helper.rb

@@ -74,4 +74,15 @@ def uploads_resolve_dev_credentials
       raise e
     end
   end
+
+  def upload_invalid_messages(rec)
+    if rec.status_invalid?
+      rec.status = "complete"
+      rec.valid?
+      msgs = rec.errors.full_messages
+      rec.status = "invalid"
+      rec.valid?
+      msgs.present? && msgs.join(", ")
+    end
+  end
 end

app/models/concerns/image_file.rb

@@ -12,17 +12,15 @@ module ImageFile
 
     before_validation :initialize_attributes, on: :create
 
-    before_validation :detect_image_attributes
-
     validates :original_url, presence: true
 
-    validates :format, inclusion: {in: ["jpeg", "png", "gif", nil]}
+    validates :format, inclusion: {in: %w[jpeg png gif]}, if: :status_complete?
 
-    enum status: [:started, :created, :processing, :complete, :error, :retrying, :cancelled]
+    enum :status, [:started, :created, :processing, :complete, :error, :retrying, :cancelled, :invalid], prefix: true
 
     scope :complete_or_replaced, -> do
       with_deleted
-        .complete
+        .status_complete
         .where("deleted_at IS NULL OR replaced_at IS NOT NULL")
         .order("created_at DESC")
     end
@@ -63,7 +61,9 @@ def guid
   end
 
   def file_name
-    File.basename(URI.parse(original_url).path)
+    if original_url.present?
+      File.basename(URI.parse(original_url).path)
+    end
   end
 
   def copy_media(force = false)
@@ -75,11 +75,11 @@ def copy_media(force = false)
   end
 
   def url
-    complete? ? self[:url] : self[:original_url]
+    self[:url] ||= published_url
   end
 
   def href
-    complete? ? url : original_url
+    status_complete? ? url : original_url
   end
 
   def href=(h)
@@ -105,39 +105,6 @@ def reset_image_attributes
     self.status = :created
   end
 
-  def detect_image_attributes
-    # skip if we've already detected width/height/format
-    return if width.present? && height.present? && format.present?
-
-    # s3 urls cannot be fastimage'd - must wait for async porter inspection
-    return if original_url.blank? || original_url.starts_with?("s3://")
-
-    info = nil
-    begin
-      fastimage_options = {
-        timeout: 10,
-        raise_on_failure: true,
-        http_header: {"User-Agent" => "Mozilla/5.0 (Macintosh; Intel Mac OS X) PRX Feeder/1.0"}
-      }
-      info = FastImage.new(original_url, fastimage_options)
-    rescue FastImage::FastImageException => err
-      logger.error(err)
-      NewRelic::Agent.notice_error(err)
-      raise
-    end
-    self.dimensions = info.size
-    self.format = info.type
-    self.size = info.content_length
-  end
-
-  def dimensions
-    [width, height]
-  end
-
-  def dimensions=(s)
-    self.width, self.height = s
-  end
-
   def replace?(img)
     original_url != img.try(:original_url)
   end

app/models/concerns/porter_parser.rb

@@ -63,45 +63,52 @@ def porter_key(msg)
 
   def porter_callback_media_meta
     info = self.class.porter_callback_inspect(result).try(:[], :Inspection)
-    if info
-      mime = porter_callback_mime(info)
-      meta = {
-        mime_type: mime,
-        medium: (mime || "").split("/").first,
-        file_size: info[:Size].to_i
-      }
-      audio_meta = porter_callback_audio_meta(mime, info)
-      video_meta = porter_callback_video_meta(mime, info)
-
-      meta.merge(audio_meta).merge(video_meta)
-    end
+    mime = porter_callback_mime(info)
+    meta = {
+      mime_type: mime,
+      medium: (mime || "").split("/").first,
+      file_size: porter_callback_size(info)
+    }
+    audio_meta = porter_callback_audio_meta(mime, info)
+    video_meta = porter_callback_video_meta(mime, info)
+
+    meta.merge(audio_meta).merge(video_meta)
   end
 
   def porter_callback_image_meta
     info = self.class.porter_callback_inspect(result).try(:[], :Inspection)
-    mime = porter_callback_mime(info || {})
+    mime = porter_callback_mime(info)
+    meta = {
+      size: porter_callback_size(info)
+    }
 
     # only return for actual images - not detected images in id3 tags
     if info && info[:Image] && mime.starts_with?("image/")
-      {
+      meta.merge(
         format: info[:Image][:Format],
         height: info[:Image][:Height].to_i,
         size: info[:Size].to_i,
         width: info[:Image][:Width].to_i
-      }
+      )
+    else
+      meta
     end
   end
 
   def porter_callback_mime(info)
-    if info[:MIME]
+    if info && info[:MIME]
       info[:MIME]
-    elsif info[:Audio]
+    elsif info && info[:Audio]
       "audio/mpeg"
     end
   end
 
+  def porter_callback_size(info)
+    info[:Size].to_i if info
+  end
+
   def porter_callback_audio_meta(mime, info)
-    if info[:Audio]
+    if info && info[:Audio]
       {
         sample_rate: info[:Audio][:Frequency].to_i,
         channels: info[:Audio][:Channels].to_i,
@@ -115,7 +122,7 @@ def porter_callback_audio_meta(mime, info)
 
   def porter_callback_video_meta(mime, info)
     # only return for actual videos - not detected images in id3 tags
-    if info[:Video] && mime.starts_with?("video")
+    if info && info[:Video] && mime.starts_with?("video")
       {
         duration: info[:Video][:Duration].to_f / 1000,
         bit_rate: info[:Video][:Bitrate].to_i / 1000,

app/models/enclosure.rb

@@ -15,6 +15,7 @@ def update_with_enclosure!(enclosure)
   def update_enclosure_attributes(enclosure)
     self.file_size = enclosure["length"].to_i
     self.mime_type = enclosure["type"]
+    self.medium = (enclosure["type"] || "").split("/").first
     self.href = enclosure["url"]
     self
   end

app/models/episode_image.rb

@@ -1,7 +1,10 @@
 class EpisodeImage < ActiveRecord::Base
+  include ImageFile
+
   belongs_to :episode, touch: true, optional: true
 
-  include ImageFile
+  validates :height, :width, numericality: {greater_than_or_equal_to: 1400, less_than_or_equal_to: 3000}, if: :status_complete?
+  validates :width, comparison: {equal_to: :height}, if: :status_complete?
 
   def destination_path
     "#{episode.path}/#{image_path}"

app/models/itunes_image.rb

@@ -2,12 +2,8 @@ class ITunesImage < ApplicationRecord
   include ImageFile
   include FeedImageFile
 
-  validates :height, :width, numericality: {
-    less_than_or_equal_to: 3000,
-    greater_than_or_equal_to: 1400
-  }, if: ->(i) { i.height && i.width }
-
-  validates :height, numericality: {equal_to: ->(image) { image.width }}, if: ->(i) { i.height }
+  validates :height, :width, numericality: {greater_than_or_equal_to: 1400, less_than_or_equal_to: 3000}, if: :status_complete?
+  validates :width, comparison: {equal_to: :height}, if: :status_complete?
 
   def replace_resources!
     ITunesImage.where(feed_id: feed_id).where.not(id: id).touch_all(:replaced_at, :deleted_at)

app/models/media_resource.rb

@@ -6,15 +6,19 @@ class MediaResource < ApplicationRecord
 
   acts_as_paranoid
 
-  enum status: [:started, :created, :processing, :complete, :error, :retrying, :cancelled]
+  enum :status, [:started, :created, :processing, :complete, :error, :retrying, :cancelled, :invalid], prefix: true
 
   before_validation :initialize_attributes, on: :create
 
+  validates :original_url, presence: true
+
+  validates :medium, inclusion: {in: %w[audio video]}, if: :status_complete?
+
   after_create :replace_resources!
 
   scope :complete_or_replaced, -> do
     with_deleted
-      .complete
+      .status_complete
       .where("deleted_at IS NULL OR replaced_at IS NOT NULL")
       .order("created_at DESC")
   end
@@ -53,7 +57,7 @@ def replace_resources!
   end
 
   def href
-    complete? ? url : original_url
+    status_complete? ? url : original_url
   end
 
   def href=(h)

app/models/tasks/copy_image_task.rb

@@ -1,18 +1,15 @@
 class Tasks::CopyImageTask < ::Task
   before_save do
     if image_resource && status_changed?
+      image_resource.status = status
+
       if complete?
-        meta = porter_callback_image_meta
-        if meta
-          update_image!(meta.merge(url: image_resource.published_url))
-          podcast.try(:publish!)
-        else
-          update_image!
-          Rails.logger.warn("No image meta found in result: #{JSON.generate(result)}")
-        end
-      else
-        update_image!
+        image_resource.assign_attributes(porter_callback_image_meta)
+        image_resource.status = "invalid" if image_resource.invalid?
       end
+
+      image_resource.save!
+      podcast.try(:publish!) if image_resource.status_complete?
     end
   end
 
@@ -46,14 +43,4 @@ def podcast
   def image_resource
     owner
   end
-
-  private
-
-  def update_image!(attrs = {})
-    image_resource.update!(attrs.merge(status: status))
-  rescue ActiveRecord::RecordInvalid
-    # TODO: handle/display async validation issues
-    image_resource.restore_attributes
-    image_resource.update!(status: "error")
-  end
 end

app/models/tasks/copy_media_task.rb

@@ -1,19 +1,15 @@
 class Tasks::CopyMediaTask < ::Task
   before_save do
     if media_resource && status_changed?
+      media_resource.status = status
+
       if complete?
-        meta = porter_callback_media_meta
-        if meta
-          media_resource.update!(meta.merge(status: status))
-        else
-          # TODO: should non-audio be an error or something?
-          media_resource.update!(status: status)
-          Rails.logger.warn("No audio meta found in result: #{JSON.generate(result)}")
-        end
-        episode.try(:podcast).try(:publish!)
-      else
-        media_resource.update!(status: status)
+        media_resource.assign_attributes(porter_callback_media_meta)
+        media_resource.status = "invalid" if media_resource.invalid?
       end
+
+      media_resource.save!
+      episode.try(:podcast).try(:publish!) if media_resource.status_complete?
     end
   end
 

app/representers/api/episode_representer.rb

@@ -60,7 +60,7 @@ class Api::EpisodeRepresenter < Api::BaseRepresenter
     decorator: Api::MediaResourceRepresenter,
     class: MediaResource,
     writeable: false,
-    if: ->(_o) { !media_resources&.all?(&:complete?) }
+    if: ->(_o) { !media_resources&.all?(&:status_complete?) }
 
   property :image, decorator: Api::ImageRepresenter, class: EpisodeImage
 

app/representers/api/image_representer.rb

@@ -3,7 +3,6 @@ class Api::ImageRepresenter < Roar::Decorator
   include HalApi::Representer::FormatKeys
 
   property :href
-  property :url, writeable: false # TODO: deprecate in favor of href ... but Castle scrapes this field
   property :original_url, writeable: false
   property :alt_text
   property :caption

app/views/episodes/audio/_error.html.erb

@@ -11,7 +11,7 @@
       <% end %>
     </div>
 
-    <div class="invalid-feedback">There was a problem processing your audio</div>
+    <div class="invalid-feedback"><%= upload_invalid_messages(content) || "There was a problem processing your audio" %></div>
 
     <label class="is-invalid">Segment <%= content.position %></label>
   </div>

app/views/images/_error.html.erb

@@ -15,7 +15,7 @@
         <small class="text-muted">(<%= number_to_human_size(image.size) %>)</small>
       </div>
 
-      <div class="invalid-feedback">There was a problem processing your image</div>
+      <div class="invalid-feedback"><%= upload_invalid_messages(image) || "There was a problem processing your image" %></div>
 
       <label class="is-invalid">Image File</label>
     </div>