favgroups: prevent adding invalid posts to favgroups.

Prevent adding duplicate or nonexistent posts to favgroups. This was
possible by editing the post_ids field directly on the favgroup edit
page.

app/models/favorite_group.rb

@@ -6,6 +6,7 @@ class FavoriteGroup < ApplicationRecord
   before_validation :strip_name
   validate :creator_can_create_favorite_groups, :on => :create
   validate :validate_number_of_posts
+  validate :validate_posts
 
   array_attribute :post_ids, parse: /\d+/, cast: :to_i
 
@@ -93,6 +94,21 @@ class FavoriteGroup < ApplicationRecord
     end
   end
 
+  def validate_posts
+    added_post_ids = post_ids - post_ids_was
+    existing_post_ids = Post.where(id: added_post_ids).pluck(:id)
+    nonexisting_post_ids = added_post_ids - existing_post_ids
+
+    if nonexisting_post_ids.present?
+      errors[:base] << "Cannot add invalid post(s) to favgroup: #{nonexisting_post_ids.to_sentence}"
+    end
+
+    duplicate_post_ids = post_ids.group_by(&:itself).transform_values(&:size).select { |id, count| count > 1 }.keys
+    if duplicate_post_ids.present?
+      errors[:base] << "Favgroup already contains post #{duplicate_post_ids.to_sentence}"
+    end
+  end
+
   def self.normalize_name(name)
     name.gsub(/[[:space:]]+/, "_")
   end
@@ -126,14 +142,12 @@ class FavoriteGroup < ApplicationRecord
 
   def add!(post)
     with_lock do
-      return if contains?(post.id)
       update!(post_ids: post_ids + [post.id])
     end
   end
 
   def remove!(post)
     with_lock do
-      return unless contains?(post.id)
       update!(post_ids: post_ids - [post.id])
     end
   end

test/unit/favorite_group_test.rb

@@ -2,76 +2,65 @@ require 'test_helper'
 
 class FavoriteTest < ActiveSupport::TestCase
   def setup
-    super
+    @user = create(:user)
-    @user = FactoryBot.create(:user)
     CurrentUser.user = @user
     CurrentUser.ip_addr = "127.0.0.1"
-    @fav_group = FactoryBot.create(:favorite_group, creator: @user, name: "blah")
+    @fav_group = create(:favorite_group, creator: @user, name: "blah")
   end
 
   def teardown
-    super
     CurrentUser.user = nil
     CurrentUser.ip_addr = nil
   end
 
   context "searching by post id" do
-    context "when the id is the only one" do
-      setup do
-        @fav_group.post_ids = "1"
-        @fav_group.save
-      end
-
     should "return the fav group" do
-        assert_equal(@fav_group.id, FavoriteGroup.for_post(1).first.try(:id))
+      posts = create_list(:post, 3)
-      end
-    end
 
-    context "when the id is in the beginning" do
+      @fav_group.add!(posts[0])
-      setup do
+      assert_equal(@fav_group.id, FavoriteGroup.for_post(posts[0].id).first.id)
-        @fav_group.post_ids = "1 2"
-        @fav_group.save
-      end
 
-      should "return the fav group" do
+      @fav_group.add!(posts[1])
-        assert_equal(@fav_group.id, FavoriteGroup.for_post(1).first.try(:id))
+      assert_equal(@fav_group.id, FavoriteGroup.for_post(posts[1].id).first.id)
-      end
-    end
 
-    context "when the id is in the middle" do
+      @fav_group.add!(posts[2])
-      setup do
+      assert_equal(@fav_group.id, FavoriteGroup.for_post(posts[2].id).first.id)
-        @fav_group.post_ids = "3 1 2"
-        @fav_group.save
-      end
-
-      should "return the fav group" do
-        assert_equal(@fav_group.id, FavoriteGroup.for_post(1).first.try(:id))
-      end
-    end
-
-    context "when the id is in the end" do
-      setup do
-        @fav_group.post_ids = "2 1"
-        @fav_group.save
-      end
-
-      should "return the fav group" do
-        assert_equal(@fav_group.id, FavoriteGroup.for_post(1).first.try(:id))
-      end
     end
   end
 
   context "expunging a post" do
-    setup do
+    should "remove it from all favorite groups" do
       @post = FactoryBot.create(:post)
+
       @fav_group.add!(@post)
+      assert_equal([@post.id], @fav_group.post_ids)
+
+      @post.expunge!
+      assert_equal([], @fav_group.reload.post_ids)
+    end
   end
 
-    should "remove it from all favorite groups" do
+  context "adding a post to a favgroup" do
-      assert_equal([@post.id], @fav_group.post_ids)
+    should "not allow adding duplicate posts" do
-      @post.expunge!
+      post = create(:post)
-      @fav_group.reload
+
-      assert_equal([], @fav_group.post_ids)
+      @fav_group.add!(post)
+      assert(@fav_group.valid?)
+      assert_equal([post.id], @fav_group.reload.post_ids)
+
+      assert_raise(ActiveRecord::RecordInvalid) { @fav_group.add!(post) }
+      assert_equal([post.id], @fav_group.reload.post_ids)
+
+      @fav_group.reload.update(post_ids: [post.id, post.id])
+      refute(@fav_group.valid?)
+      assert_equal([post.id], @fav_group.reload.post_ids)
+    end
+
+    should "not allow adding nonexistent posts" do
+      @fav_group.update(post_ids: [0])
+
+      refute(@fav_group.valid?)
+      assert_equal([], @fav_group.reload.post_ids)
     end
   end
 end