From 8a89a772487654a20121425e0e9b727f9aea7fe3 Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Thu, 5 Dec 2024 15:41:13 +1000
Subject: [PATCH] FIX: Discard empty bundles for reviewables (#30121)

Followup c7e471d35a0d394899eda5af26de64511e199cc6

It is currently possible to add a bundle (which is a collection
of actions used for a dropdown on the client) for a reviewable
via actions.add_bundle and then never add any actions to it.

This causes the client to explode, as seen in the referenced
commit, because of the way our store expects to resolve objects
referenced by ID that are passed down by the serializer, which
then causes Ember to have an unrecoverable render error.

Fixing this on the serializer level is not really possible because
of all the ActiveModel::Serializer magic that serializes
objects by ID reference when doing things like has_many.
`Reviewable#actions_for` is a better place to do this anyway,
because this is the main location where the bundles and actions
are built for every action via the serializer.
---
 app/models/reviewable.rb       |  9 +++++++-
 lib/reviewable/actions.rb      |  4 ++++
 spec/models/reviewable_spec.rb | 39 ++++++++++++++++++++++++++++++++++
 3 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/app/models/reviewable.rb b/app/models/reviewable.rb
index 5535ca375f3..a9e957b7f34 100644
--- a/app/models/reviewable.rb
+++ b/app/models/reviewable.rb
@@ -270,8 +270,15 @@ class Reviewable < ActiveRecord::Base
 
   def actions_for(guardian, args = nil)
     args ||= {}
+    built_actions =
+      Actions.new(self, guardian).tap { |actions| build_actions(actions, guardian, args) }
 
-    Actions.new(self, guardian).tap { |actions| build_actions(actions, guardian, args) }
+    # Empty bundles can cause big issues on the client side, so we remove them
+    # here. It's not valid anyway to have a bundle with no actions, but you can
+    # add a bundle via actions.add_bundle and then not add any actions to it.
+    built_actions.bundles.reject!(&:empty?)
+
+    built_actions
   end
 
   def editable_for(guardian, args = nil)
diff --git a/lib/reviewable/actions.rb b/lib/reviewable/actions.rb
index 4df0eaee70f..5714035eeea 100644
--- a/lib/reviewable/actions.rb
+++ b/lib/reviewable/actions.rb
@@ -30,6 +30,10 @@ class Reviewable < ActiveRecord::Base
         @label = label
         @actions = []
       end
+
+      def empty?
+        @actions.empty?
+      end
     end
 
     class Action < Item
diff --git a/spec/models/reviewable_spec.rb b/spec/models/reviewable_spec.rb
index de197304323..387e4f65115 100644
--- a/spec/models/reviewable_spec.rb
+++ b/spec/models/reviewable_spec.rb
@@ -621,6 +621,45 @@ RSpec.describe Reviewable, type: :model do
     end
   end
 
+  describe "#actions_for" do
+    fab!(:reviewable) { Fabricate(:reviewable_queued_post) }
+    fab!(:user)
+
+    it "gets the bundles and actions for a reviewable" do
+      actions = reviewable.actions_for(user.guardian)
+      expect(actions.bundles.map(&:id)).to eq(%w[approve_post reject_post revise_and_reject_post])
+      expect(actions.bundles.find { |b| b.id == "approve_post" }.actions.map(&:id)).to eq(
+        ["approve_post"],
+      )
+      expect(actions.bundles.find { |b| b.id == "reject_post" }.actions.map(&:id)).to eq(
+        ["reject_post"],
+      )
+      expect(actions.bundles.find { |b| b.id == "revise_and_reject_post" }.actions.map(&:id)).to eq(
+        ["revise_and_reject_post"],
+      )
+    end
+
+    describe "handling empty bundles" do
+      class ReviewableTestRecord < Reviewable
+        def build_actions(actions, guardian, args)
+          actions.add(:approve_post) do |action|
+            action.icon = "check"
+            action.label = "reviewables.actions.approve_post.title"
+          end
+          actions.add_bundle("empty_bundle", icon: "xmark", label: "Empty Bundle")
+        end
+      end
+
+      it "does not return empty bundles with no actions" do
+        actions = ReviewableTestRecord.new.actions_for(user.guardian)
+        expect(actions.bundles.map(&:id)).to eq(%w[approve_post])
+        expect(actions.bundles.find { |b| b.id == "approve_post" }.actions.map(&:id)).to eq(
+          ["approve_post"],
+        )
+      end
+    end
+  end
+
   describe "default actions" do
     let(:reviewable) { Reviewable.new }
     let(:actions) { Reviewable::Actions.new(reviewable, Guardian.new) }