FIX: Store custom attributes that are needed by plugins in queuedpost payload (#8009)

FIX: Store custom attributes that are needed by plugins in queuedpost payload (#8009)

diff --git a/lib/new_post_manager.rb b/lib/new_post_manager.rb
index a9a1278..5db5cd5 100644
--- a/lib/new_post_manager.rb
+++ b/lib/new_post_manager.rb
@@ -21,6 +21,14 @@ class NewPostManager
     sorted_handlers.map { |h| h[:proc] }
   end
 
+  def self.plugin_payload_attributes
+    @payload_attributes ||= []
+  end
+
+  def self.add_plugin_payload_attribute(attribute)
+    plugin_payload_attributes << attribute
+  end
+
   def self.clear_handlers!
     @sorted_handlers = []
   end
@@ -208,6 +216,9 @@ class NewPostManager
     %w(typing_duration_msecs composer_open_duration_msecs reply_to_post_number).each do |a|
       payload[a] = @args[a].to_i if @args[a]
     end
+
+    self.class.plugin_payload_attributes.each { |a| payload[a] = @args[a] if @args[a].present? }
+
     payload[:via_email] = true if !!@args[:via_email]
     payload[:raw_email] = @args[:raw_email] if @args[:raw_email].present?
 
diff --git a/plugins/poll/plugin.rb b/plugins/poll/plugin.rb
index ca59482..29c365a 100644
--- a/plugins/poll/plugin.rb
+++ b/plugins/poll/plugin.rb
@@ -402,6 +402,8 @@ after_initialize do
     true
   end
 
+  NewPostManager.add_plugin_payload_attribute("is_poll")
+
   NewPostManager.add_handler(1) do |manager|
     post = Post.new(raw: manager.args[:raw])
 
diff --git a/plugins/poll/spec/lib/new_post_manager_spec.rb b/plugins/poll/spec/lib/new_post_manager_spec.rb
index 9517e4f..4bee4c5 100644
--- a/plugins/poll/spec/lib/new_post_manager_spec.rb
+++ b/plugins/poll/spec/lib/new_post_manager_spec.rb
@@ -11,8 +11,8 @@ describe NewPostManager do
       SiteSetting.poll_minimum_trust_level_to_create = 0
     end
 
-    it "should render the poll upon approval" do
-      params = {
+    let(:params) do
+      {
         raw: "[poll]\n* 1\n* 2\n* 3\n[/poll]",
         archetype: "regular",
         category: "",
@@ -27,7 +27,9 @@ describe NewPostManager do
         referrer: "http://localhost:3000/",
         first_post_checks: true
       }
+    end
 
+    it "should render the poll upon approval" do
       result = NewPostManager.new(user, params).perform
       expect(result.action).to eq(:enqueued)
       expect(result.reviewable).to be_present
@@ -35,5 +37,23 @@ describe NewPostManager do
       review_result = result.reviewable.perform(admin, :approve_post)
       expect(Poll.where(post: review_result.created_post).exists?).to eq(true)
     end
+
+    it 're-validates the poll when the approve_post event is triggered' do
+      invalid_raw_poll = <<~RAW
+        [poll type=multiple min=0]
+        * 1
+        * 2
+        [/poll]
+      RAW
+
+      result = NewPostManager.new(user, params).perform
+
+      reviewable = result.reviewable
+      reviewable.payload["raw"] = invalid_raw_poll
+      reviewable.save!
+
+      review_result = result.reviewable.perform(admin, :approve_post)
+      expect(Poll.where(post: review_result.created_post).exists?).to eq(false)
+    end
   end
 end

GitHub sha: 79957706

1 Like

What I wonder here is when do we add extensibility points like this vs wrappers like this:

I think it makes sense to always add wrappers in Plugin::Instance to signal we have a public API, I know I have not done this every time in the past but I think it makes sense to add and makes our API more discoverable.

@eviltrout / @romanrizzi ?

I think adding a wrapper totally makes sense in terms of discoverability. :+1:

Yes I agree it almost always makes sense to do this.

I do wish we’d used a plugin object on the server side like we have on the client side, but that :ship: has probably sailed.

Followed up here:

1 Like