CV2-3177: fix newsletter validation (#1649)

* CV2-3177: fix newsletter validation * CV2-3177: fix time method with time is nil
meedan · Sep 12, 2023 · 0f14239 · 0f14239
1 parent c983b83
commit 0f14239
Show file tree

Hide file tree

Showing 3 changed files with 19 additions and 4 deletions.
diff --git a/app/graph/types/tipline_newsletter_type.rb b/app/graph/types/tipline_newsletter_type.rb
@@ -24,7 +24,7 @@ def send_on
   field :time, GraphQL::Types::String, null: true
 
   def time
-    object.time.strftime("%H:%M")
+    object.time ? object.time.strftime("%H:%M") : nil
   end
   field :subscribers_count, GraphQL::Types::Int, null: true
   field :footer, GraphQL::Types::String, null: true

diff --git a/app/models/tipline_newsletter.rb b/app/models/tipline_newsletter.rb
@@ -30,18 +30,18 @@ class TiplineNewsletter < ApplicationRecord
   validates_inclusion_of :language, in: ->(newsletter) { newsletter.team.get_languages.to_a }
   validates_inclusion_of :content_type, in: ['static', 'rss']
   # Should be executed only when `enabled: 1`
+  validates_presence_of :send_on, if: ->(newsletter) { newsletter.content_type == 'static' && newsletter.enabled }
   with_options if: :enabled do |obj|
     obj.validates_presence_of :time, :timezone, :introduction
-    obj.validates_presence_of :send_on, if: ->(newsletter) { newsletter.content_type == 'static' }
     obj.validates_inclusion_of :number_of_articles, in: 0..3, allow_blank: true, allow_nil: true
     obj.validates :first_article, length: { maximum: proc { |newsletter| MAXIMUM_ARTICLE_LENGTH[newsletter.number_of_articles].to_i } }, allow_blank: true, allow_nil: true, if: proc { |newsletter| newsletter.number_of_articles >= 1 }
     obj.validates :second_article, length: { maximum: proc { |newsletter| MAXIMUM_ARTICLE_LENGTH[newsletter.number_of_articles].to_i } }, allow_blank: true, allow_nil: true, if: proc { |newsletter| newsletter.number_of_articles >= 2 }
     obj.validates :third_article, length: { maximum: proc { |newsletter| MAXIMUM_ARTICLE_LENGTH[newsletter.number_of_articles].to_i } }, allow_blank: true, allow_nil: true, if: proc { |newsletter| newsletter.number_of_articles == 3 }
     obj.validate :send_every_is_a_list_of_days_of_the_week
-    obj.validate :not_scheduled_for_the_past
+    obj.validate :not_scheduled_for_the_past, unless: proc { |newsletter| newsletter.time.blank? || newsletter.timezone.blank? }
   end
 
-  after_save :reschedule_delivery
+  after_save :reschedule_delivery, unless: proc { |newsletter| newsletter.time.blank? || newsletter.timezone.blank? }
 
   def parsed_timezone
     timezone = self.timezone

diff --git a/test/models/tipline_newsletter_test.rb b/test/models/tipline_newsletter_test.rb
@@ -463,4 +463,19 @@ def teardown
     )
     assert_equal '2024-01-01 06:00', newsletter.scheduled_time.strftime("%Y-%m-%d %H:%M")
   end
+
+  test "should validate some fields only for enabled newsletter" do
+    assert_difference 'TiplineNewsletter.count' do
+      create_tipline_newsletter enabled: false, send_on: nil, time: nil, timezone: nil, send_every: nil
+    end
+    assert_raises ActiveRecord::RecordInvalid do
+      create_tipline_newsletter send_on: nil, time: nil, timezone: nil, send_every: nil
+    end
+    assert_raises ActiveRecord::RecordInvalid do
+      create_tipline_newsletter send_on: nil, send_every: nil
+    end
+    assert_raises ActiveRecord::RecordInvalid do
+      create_tipline_newsletter send_every: nil
+    end
+  end
 end