Refactor Feedback validations

This moves most of the implementation up to the Feedback base class,
ensures that checks on allowed feedback only happen if article is set,
and ensures that at most one error message is set on article.

Author: mvz

app/models/comment.rb

@@ -41,18 +41,15 @@ def send_notifications
   private
 
   def article_allows_feedback?
-    return true if article.allow_comments?
-
-    errors.add(:article, "Article is not open to comments")
-    false
+    article.allow_comments?
   end
 
   def blog_allows_feedback?
     true
   end
 
-  def check_article_closed_for_feedback
-    errors.add(:article, "Comment are closed") if article.comments_closed?
+  def article_closed_for_feedback?
+    article.comments_closed?
   end
 
   def originator

app/models/feedback.rb

@@ -11,8 +11,7 @@ class Feedback < ApplicationRecord
   include PublifyGuid
   include ContentBase
 
-  validate :article_allows_this_feedback, on: :create
-  validate :feedback_not_closed, on: :create
+  validate :feedback_allowed, on: :create
   validates :article, presence: true
 
   before_save :correct_url, :classify_content
@@ -99,8 +98,20 @@ def correct_url
     self.url = "http://#{url}" unless %r{^https?://}.match?(url)
   end
 
-  def article_allows_this_feedback
-    article && blog_allows_feedback? && article_allows_feedback?
+  def feedback_allowed
+    return unless article
+
+    unless blog_allows_feedback?
+      errors.add(:base, "#{plural_model_name} are disabled")
+      return
+    end
+
+    unless article_allows_feedback?
+      errors.add(:article, "Article is not open for #{plural_model_name.downcase}")
+      return
+    end
+
+    errors.add(:article, "#{plural_model_name} are closed") if article_closed_for_feedback?
   end
 
   def akismet_options
@@ -200,10 +211,6 @@ def report_as_ham
     end
   end
 
-  def feedback_not_closed
-    check_article_closed_for_feedback
-  end
-
   def send_notifications
     nil
   end
@@ -242,4 +249,8 @@ def akismet_client
   def blog_id
     article.blog_id if article.present?
   end
+
+  def plural_model_name
+    self.class.model_name.human.pluralize
+  end
 end

app/models/trackback.rb

@@ -14,24 +14,6 @@ def process_trackback
     end
   end
 
-  def article_allows_feedback?
-    return true if article.allow_pings?
-
-    errors.add(:article, "Article is not pingable")
-    false
-  end
-
-  def blog_allows_feedback?
-    return true unless blog.global_pings_disable
-
-    errors.add(:base, "Pings are disabled")
-    false
-  end
-
-  def check_article_closed_for_feedback
-    errors.add(:article, "Pings are closed") if article.pings_closed?
-  end
-
   def originator
     blog_name
   end
@@ -47,4 +29,18 @@ def body=(newval)
   def feed_title
     "Trackback from #{blog_name}: #{title} on #{article.title}"
   end
+
+  private
+
+  def article_allows_feedback?
+    article.allow_pings?
+  end
+
+  def blog_allows_feedback?
+    !blog.global_pings_disable
+  end
+
+  def article_closed_for_feedback?
+    article.pings_closed?
+  end
 end

spec/models/comment_spec.rb

@@ -8,9 +8,29 @@
 
   let(:published_article) { build_stubbed(:article, published_at: 1.hour.ago, blog: blog) }
 
-  def valid_comment(options = {})
-    Comment.new({ author: "Bob", article: published_article, body: "nice post",
-                  ip: "1.2.3.4" }.merge(options))
+  describe "validations" do
+    let(:comment) { described_class.new }
+
+    it "allows an article with open comment window" do
+      article = Article.new(blog: blog, allow_comments: true, state: "published",
+                            published_at: 1.day.ago)
+
+      expect(comment).to allow_value(article).for(:article)
+    end
+
+    it "requires article comment window to be open" do
+      article = Article.new(blog: blog, allow_comments: true)
+
+      expect(comment).not_to allow_value(article).for(:article).
+        with_message("Comments are closed")
+    end
+
+    it "requires article to be open to comments" do
+      article = Article.new(blog: blog, allow_comments: false)
+
+      expect(comment).not_to allow_value(article).for(:article).
+        with_message("Article is not open for comments")
+    end
   end
 
   describe "#permalink_url" do
@@ -88,6 +108,11 @@ def valid_comment(options = {})
   end
 
   describe "#classify_content" do
+    def valid_comment(options = {})
+      Comment.new({ author: "Bob", article: published_article, body: "nice post",
+                    ip: "1.2.3.4" }.merge(options))
+    end
+
     it "rejects spam rbl" do
       comment = valid_comment(
         author: "Spammer",

spec/models/trackback_spec.rb

@@ -4,57 +4,85 @@
 require "publify_core/testing_support/dns_mock"
 
 describe Trackback, type: :model do
-  let(:article) { create(:article) }
+  describe "validations" do
+    let(:blog) { build_stubbed :blog }
+    let(:trackback) { described_class.new }
 
-  before do
-    @blog = create(:blog)
-    @blog.sp_global = true
-    @blog.default_moderate_comments = false
-    @blog.save!
-  end
+    it "allows an article with open trackback window" do
+      article = Article.new(blog: blog, allow_pings: true, state: "published",
+                            published_at: 1.day.ago)
 
-  it "Incomplete trackbacks should not be accepted" do
-    tb = described_class.new(blog_name: "Blog name",
-                             title: "Title",
-                             excerpt: "Excerpt",
-                             article_id: create(:article).id)
-    expect(tb).not_to be_valid
-    expect(tb.errors["url"]).to be_any
-  end
+      expect(trackback).to allow_value(article).for(:article)
+    end
 
-  it "A valid trackback should be accepted" do
-    tb = described_class.new(blog_name: "Blog name",
-                             title: "Title",
-                             url: "http://foo.com",
-                             excerpt: "Excerpt",
-                             article_id: create(:article).id)
-    expect(tb).to be_valid
-    tb.save
-    expect(tb.guid.size).to be > 15
-    expect(tb).not_to be_spam
-  end
+    it "requires article trackback window to be open" do
+      article = Article.new(blog: blog, allow_pings: true)
 
-  it "Trackbacks with a spammy link in the excerpt should be rejected" do
-    tb = article.trackbacks.
-      build(ham_params.merge(excerpt: '<a href="http://chinaaircatering.com">spam</a>'))
-    tb.classify_content
-    expect(tb).to be_spammy
-  end
+      expect(trackback).not_to allow_value(article).for(:article).
+        with_message("Trackbacks are closed")
+    end
 
-  it "Trackbacks with a spammy source url should be rejected" do
-    tb = article.trackbacks.build(ham_params.merge(url: "http://www.chinaircatering.com"))
-    tb.classify_content
-    expect(tb).to be_spammy
-  end
+    it "requires article to be open to trackback" do
+      article = Article.new(blog: blog, allow_pings: false)
 
-  it "Trackbacks from a spammy ip address should be rejected" do
-    tb = article.trackbacks.build(ham_params.merge(ip: "212.42.230.207"))
-    tb.classify_content
-    expect(tb).to be_spammy
+      expect(trackback).not_to allow_value(article).for(:article).
+        with_message("Article is not open for trackbacks")
+    end
   end
 
-  def ham_params
-    { blog_name: "Blog", title: "trackback", excerpt: "bland",
-      url: "http://notaspammer.com", ip: "212.42.230.206" }
+  describe "spam detection" do
+    let(:article) { create(:article) }
+
+    before do
+      @blog = create(:blog)
+      @blog.sp_global = true
+      @blog.default_moderate_comments = false
+      @blog.save!
+    end
+
+    it "Incomplete trackbacks should not be accepted" do
+      tb = described_class.new(blog_name: "Blog name",
+                               title: "Title",
+                               excerpt: "Excerpt",
+                               article_id: create(:article).id)
+      expect(tb).not_to be_valid
+      expect(tb.errors["url"]).to be_any
+    end
+
+    it "A valid trackback should be accepted" do
+      tb = described_class.new(blog_name: "Blog name",
+                               title: "Title",
+                               url: "http://foo.com",
+                               excerpt: "Excerpt",
+                               article_id: create(:article).id)
+      expect(tb).to be_valid
+      tb.save
+      expect(tb.guid.size).to be > 15
+      expect(tb).not_to be_spam
+    end
+
+    it "Trackbacks with a spammy link in the excerpt should be rejected" do
+      tb = article.trackbacks.
+        build(ham_params.merge(excerpt: '<a href="http://chinaaircatering.com">spam</a>'))
+      tb.classify_content
+      expect(tb).to be_spammy
+    end
+
+    it "Trackbacks with a spammy source url should be rejected" do
+      tb = article.trackbacks.build(ham_params.merge(url: "http://www.chinaircatering.com"))
+      tb.classify_content
+      expect(tb).to be_spammy
+    end
+
+    it "Trackbacks from a spammy ip address should be rejected" do
+      tb = article.trackbacks.build(ham_params.merge(ip: "212.42.230.207"))
+      tb.classify_content
+      expect(tb).to be_spammy
+    end
+
+    def ham_params
+      { blog_name: "Blog", title: "trackback", excerpt: "bland",
+        url: "http://notaspammer.com", ip: "212.42.230.206" }
+    end
   end
 end