add missing validations for creative form (#1227)

It is currently possible to submit the creative form without images and with copy text that exceeds 85 characters in length. This commit adds enhanced validations around creatives.

app/models/creative.rb

@@ -23,6 +23,8 @@
 #
 
 class Creative < ApplicationRecord
+  MAX_COPY_LENGTH = 85
+
   # extends ...................................................................
   # includes ..................................................................
   include Eventable
@@ -46,8 +48,12 @@ class Creative < ApplicationRecord
   validates :name, length: {maximum: 255, allow_blank: false}
   validates :status, inclusion: {in: ENUMS::CREATIVE_STATUSES.values}
   validates :creative_type, inclusion: {in: ENUMS::CREATIVE_TYPES.values}
-  validate :validate_images
+  validate :validate_icon_image
+  validate :validate_small_image
+  validate :validate_wide_image
+  validate :validate_large_image
   validate :validate_status_change
+  validate :validate_copy_length
 
   # callbacks .................................................................
   after_commit :touch_campaigns, on: [:update]
@@ -191,10 +197,29 @@ def validate_status_change
     end
   end
 
-  def validate_images
-    if standard_images.exists? && sponsor_image
-      errors.add :images, "cannot include both standard and sponsor types"
-    end
+  def validate_icon_image
+    return if icon_image
+    errors.add :base, "please select an icon image"
+  end
+
+  def validate_small_image
+    return if small_image
+    errors.add :base, "please select a small image"
+  end
+
+  def validate_wide_image
+    return if wide_image
+    errors.add :base, "please select a wide image"
+  end
+
+  def validate_large_image
+    return if large_image
+    errors.add :base, "please select a large image"
+  end
+
+  def validate_copy_length
+    return unless body.length + cta.length + headline.length > MAX_COPY_LENGTH
+    errors.add :base, "headline, body, and call to action cannot exceed a combined #{MAX_COPY_LENGTH} characters."
   end
 
   def validate_destroyable

test/controllers/advertisement_clicks_controller_test.rb

@@ -73,6 +73,7 @@ class AdvertisementClicksControllerTest < ActionDispatch::IntegrationTest
 
   test "advertisement click {{creative_name}} URL variable" do
     @campaign.update url: "https://example.com?custom={{creative_name}}"
+    attach_all_images!(creative: @creative, organization: @creative.organization)
     @creative.update name: "Ad Click: Creative Name > Test"
     get advertisement_clicks_url(@impression_id), params: @params, headers: @headers
     assert_response :redirect

test/integration/advertisements_test.rb

@@ -9,6 +9,8 @@ class AdvertisementsTest < ActionDispatch::IntegrationTest
     @premium_campaign.campaign_bundle.update_columns start_date: start_date, end_date: start_date.advance(months: 3)
     @premium_campaign.save!
     @premium_campaign.organization.update balance: Monetize.parse("$10,000 USD")
+    attach_all_images!(creative: @premium_campaign.creative, organization: @premium_campaign.organization)
+    @images = @premium_campaign.creative.images
     @property = amend(properties: :website, audience_id: @premium_campaign.audience_ids.sample)
     travel_to start_date.to_time.advance(days: 15)
   end
@@ -112,7 +114,7 @@ class AdvertisementsTest < ActionDispatch::IntegrationTest
   test "js: fallback campaign with matching keywords and country is displayed before generic fallback campaigns" do
     amend campaigns: :fallback, start_date: @premium_campaign.start_date, end_date: @premium_campaign.end_date
     copy campaigns: :fallback, keywords: @property.keywords.sample(2),
-         creative_ids: [copy(creatives: :fallback, body: "This is a targeted fallback campaign").id]
+         creative_ids: [copy(creatives: :fallback, body: "This is a targeted fallback campaign", images: @images).id]
     @premium_campaign.update_columns keywords: ["No Match"]
     get advertisements_path(@property, format: :js), headers: {"REMOTE_ADDR": ip_address("US")}
     assert Campaign.fallback.count == 2
@@ -167,7 +169,7 @@ class AdvertisementsTest < ActionDispatch::IntegrationTest
   # ----------------------------------------------------------------------------------------------------------
 
   test "js: property with assigner campaign will eventually display the assigner campaign" do
-    other_campaign = copy(campaigns: :premium, creative_ids: [copy(creatives: :premium).id])
+    other_campaign = copy(campaigns: :premium, creative_ids: [copy(creatives: :premium, images: @images).id])
     @premium_campaign.update_columns assigned_property_ids: [@property.id]
     @premium_campaign.creative.update body: "This is a premium campaign that has assigned the property"
     assert other_campaign.creative.body != @premium_campaign.creative.body
@@ -180,7 +182,7 @@ class AdvertisementsTest < ActionDispatch::IntegrationTest
   end
 
   test "js: property with assigner campaign will eventually display other matching campaigns" do
-    other_campaign = Campaign.create!(@premium_campaign.attributes.except("id").merge(creative_ids: [copy(creatives: :premium, body: "This is a non assigner premium campaign").id]))
+    other_campaign = Campaign.create!(@premium_campaign.attributes.except("id").merge(creative_ids: [copy(creatives: :premium, body: "This is a non assigner premium campaign", images: @images).id]))
     @premium_campaign.update_columns assigned_property_ids: [@property.id]
     @premium_campaign.creative.update_columns body: "This is a premium campaign that has assigned the property"
     assert other_campaign.creative.body != @premium_campaign.creative.body
@@ -193,7 +195,7 @@ class AdvertisementsTest < ActionDispatch::IntegrationTest
   end
 
   test "js: property with assigner campaign will never display other matching campaigns when restrict_to_assigner_campaigns is true" do
-    other_campaign = copy(campaigns: :premium, creative_ids: [copy(creatives: :premium).id])
+    other_campaign = copy(campaigns: :premium, creative_ids: [copy(creatives: :premium, images: @images).id])
     @premium_campaign.update_columns assigned_property_ids: [@property.id]
     @premium_campaign.creative.update_columns body: "This is a premium campaign that has assigned the property"
     @property.update_columns restrict_to_assigner_campaigns: true
@@ -224,7 +226,7 @@ class AdvertisementsTest < ActionDispatch::IntegrationTest
 
   test "js: property will show targeted premium campaign over a zero balance campaign with assigned property" do
     organization = copy(organizations: :default, balance: Money.new(0, "USD"))
-
+    attach_all_images!(creative: @premium_campaign.creative, organization: organization)
     user = copy users: :advertiser,
                 email: Faker::Internet.email,
                 password: "password",
@@ -233,7 +235,8 @@ class AdvertisementsTest < ActionDispatch::IntegrationTest
     creative = copy creatives: :premium,
                     organization: organization,
                     user: user,
-                    body: "This is an assigned premium campaign"
+                    body: "This is an assigned premium campaign",
+                    images: @images
 
     Campaign.create! @premium_campaign.attributes.except("id", "campaign_bundle_id").merge(
       organization: organization,
@@ -260,10 +263,10 @@ class AdvertisementsTest < ActionDispatch::IntegrationTest
           keywords: @property.keywords,
           start_date: @premium_campaign.start_date,
           end_date: @premium_campaign.end_date,
-          creative_ids: [copy(creatives: :fallback, body: "This is a targeted fallback campaign").id]
+          creative_ids: [copy(creatives: :fallback, body: "This is a targeted fallback campaign", images: @images).id]
 
     assigned = copy campaigns: :fallback, keywords: [],
-                    creative_ids: [copy(creatives: :fallback, body: "This is an assigned fallback campaign").id]
+                    creative_ids: [copy(creatives: :fallback, body: "This is an assigned fallback campaign", images: @images).id]
 
     @property.update_columns assigned_fallback_campaign_ids: [assigned.id]
 
@@ -281,14 +284,14 @@ class AdvertisementsTest < ActionDispatch::IntegrationTest
           keywords: @property.keywords,
           start_date: @premium_campaign.start_date,
           end_date: @premium_campaign.end_date,
-          creative_ids: [copy(creatives: :fallback, body: "This is a targeted fallback campaign").id]
+          creative_ids: [copy(creatives: :fallback, body: "This is a targeted fallback campaign", images: @images).id]
 
     assigned = copy campaigns: :fallback, keywords: [],
-                    creative_ids: [copy(creatives: :fallback, body: "This is an assigned fallback campaign").id]
+                    creative_ids: [copy(creatives: :fallback, body: "This is an assigned fallback campaign", images: @images).id]
 
     assigned_and_targeted = copy campaigns: :fallback,
                                  keywords: @property.keywords,
-                                 creative_ids: [copy(creatives: :fallback, body: "This is an assigned and targeted fallback campaign").id]
+                                 creative_ids: [copy(creatives: :fallback, body: "This is an assigned and targeted fallback campaign", images: @images).id]
 
     @property.update_columns assigned_fallback_campaign_ids: [assigned.id, assigned_and_targeted.id]
 
@@ -301,12 +304,11 @@ class AdvertisementsTest < ActionDispatch::IntegrationTest
 
   test "js: fallback campaign with assigned property will not display on a different property even if the different property assigns the fallback campaign" do
     @premium_campaign.update_columns status: ENUMS::CAMPAIGN_STATUSES::PENDING
-
     fallback_campaign = amend campaigns: :fallback,
                               start_date: @premium_campaign.start_date,
                               end_date: @premium_campaign.end_date,
                               assigned_property_ids: [@property.id],
-                              creative: copy(creatives: :fallback, body: "This is a fallback campaign assigned to a different property")
+                              creative: copy(creatives: :fallback, body: "This is a fallback campaign assigned to a different property", images: @images)
 
     other_property = copy properties: :website, assigned_fallback_campaign_ids: [fallback_campaign.id]
 

test/test_helper.rb

@@ -48,6 +48,31 @@ def sign_in_user(user)
     post user_session_url
   end
 
+  def attach_all_images!(creative:, organization:)
+    creative.add_image! attach_icon_image!(organization)
+    creative.add_image! attach_small_image!(organization)
+    creative.add_image! attach_large_image!(organization)
+    creative.add_image! attach_wide_image!(organization)
+  end
+
+  def attach_icon_image!(record)
+    name = "seed-20x20.png"
+    record.images.attach(
+      io: File.open(Rails.root.join("test/assets/images/seeds/seed-20x20.png")),
+      filename: "seed-20x20.png",
+      content_type: "image/png",
+      metadata: {
+        identified: true,
+        width: 20,
+        height: 20,
+        analyzed: true,
+        name: name,
+        format: ENUMS::IMAGE_FORMATS::ICON
+      }
+    )
+    record.images.search_metadata_name(name).metadata_format(ENUMS::IMAGE_FORMATS::ICON).order(created_at: :desc).first
+  end
+
   def attach_small_image!(record)
     name = "seed-200x200.png"
     record.images.attach(