Making Discourse more pluggable by adding more callback Triggers

api
plugins

(SigurĂ°ur GuĂ°brandsson) #1

How about we add more callback triggers so people can create more plugins?

I spent the last weekend looking at the Discourse code to figure out how it works so I can easily extend it with plugins.
(I am using this as an excuse to learn Ruby+Rails + understand the Discourse code base)

Unfortunately, I found it really difficult to figure out where and how to add my callbacks so I really dug in the code and used the plugin discourse-akismet to help me figure out how things work in Discourse.

To my surprise, I only found 8 callback triggers in the whole code … so I figure that the code needs more triggers so it’s easier to extend the core functionality.

Right now, the only callback triggers I can find in the code are:

  • app/models/site_setting.rb

  • Trigger - :site_setting_saved

    • Trigger variable - site_setting
  • lib/post_revisor.rb

  • Trigger - :before_edit_post

    • Trigger variable - @post
  • Trigger - :validate_post

    • Trigger variable - @post
  • lib/post_creator.rb

  • Trigger - :before_create_post

    • Trigger variable - @post
  • Trigger - :validate_post

    • Trigger variable - @post
  • app/controllers/posts_controller.rb

  • Trigger - :topic_created

    • Trigger variable - post.topic
    • Trigger variable - params
    • Trigger variable - current_user
  • Trigger - :post_created

    • Trigger variable - post
    • Trigger variable - params
    • Trigger variable - current_user
  • app/models/post_action.rb

  • Trigger - :confirmed_spam_post

    • Trigger variable - post

I don’t mind adding the triggers in myself and doing a PU, I just want feedback from you guys first.

What do you think? Should we add more triggers?

(and then there is the problem of documenting the triggers … but that’s a topic for another thread)


Group Assignment SQL Query
(Robin Ward) #2

Yes, this has come up before. I’m more than happy to add triggers for anything that needs it, I’d just need a use case to explain why it’s good. So if you have a plugin that needs to do something, the chances of us accepting a PR to support that are very high.


(SigurĂ°ur GuĂ°brandsson) #3

AWESOME!

I am working on a plugin that will help onboarding users to a forum.
Basically a plugin that will send API calls to getdrip.com for e-mail automation.

The simplest and easiest method of integrating this would be to add actions to “badge received” and “badge removed” in addition to “user created”, “user suspended” and “user deleted”.

The uses I have in mind are:

  • When a user is created but hasn’t created his first post or reply.
  • When a user gets a badge, give him more rewards than just a badge …

(SigurĂ°ur GuĂ°brandsson) #4

Well, I created two PR’s to add triggers.

PR: Badge granted/removed triggers

  • app/models/user_badge.rb
  • Trigger - :user_badge_granted
    • Trigger variable - badge_id
    • Trigger variable - user_id
  • Trigger - :user_badge_removed
    • Trigger variable - badge_id
    • Trigger variable - user_id

PR: User created/updated/deleted

  • app/models/user.rb
  • Trigger - :user_created
    • Trigger variable - username
    • Trigger variable - email
  • Trigger - :user_updated
    • Trigger variable - username
    • Trigger variable - email
  • Trigger - :user_deleted
    • Trigger variable - username
    • Trigger variable - email

(Thomas Purchas) #5

Would it not be better to pass the user_id in the user triggers, rather than username or email.

Using username and email could create a race condition if the user quickly changed either (I know its unlikely, but if you introduce job queues to the equation at some point it could be a problem).

Also username and email could be read from the user object.


(SigurĂ°ur GuĂ°brandsson) #6

Clever … I thought about doing lookups by using the username if the triggered action needs anything else… but using user_id is probably the best way, no race conditions…

… as for allowing access to the username and email, I believe that can save some db lookup time, but I’m unsure of how much it actually saves … probably not a whole lot - any thoughts @sam / @zogstrip?

DiscourseEvent.trigger(:event_trigger, user_id: id)
...
user = User.find_by_id(user_id)
email = user.email

vs

DiscourseEvent.trigger(:event_trigger, user_id: id, username, email)

(RĂ©gis Hanol) #7

Well, if you already have the information handy (eg. username, email) when you’re triggering the event, I don’t see any drawbacks not to pass it to as a parameter :wink:

But yeah, I agree with @thomaspurchas, you should at the very least pass the user_id.


(Robin Ward) #8

Why not pass the user object?


(SigurĂ°ur GuĂ°brandsson) #9

That was my original thought … but since I’m such a RoR n00b that I felt intimidated …

@eviltrout - would something like this work?

DiscourseEvent.trigger(:event_trigger, user: self)

(Robin Ward) #10

Yup that should work fine. It’s only weird in the case of the user being deleted. For that trigger you might want to extract whatever the event would want to know in advance.


(SigurĂ°ur GuĂ°brandsson) #11

Finally … after having to rebuild my dev machine, I finally managed to write the tests and triggers.

PR 3236
PR 3237

Hopefully this meets the standard. :smile:


(system) #12

What happened to this PR? I need those triggers for my plugin.
Also, how are you handling the case when BadgeGranter grants a badge from the backfill function? The Badge is being granted via raw SQL and not UserBadge class, so the after_create wouldn’t work.


(SigurĂ°ur GuĂ°brandsson) #13

They’re in the decision queue


(Sam Saffron) #14

In particular, I want to see either a lighter weight mechanism of adding hooks (zero work when nothing is subscribed) or a real world demand for a particular hook


(SigurĂ°ur GuĂ°brandsson) #15

Hmm, I thought the weight of a trigger was next to none if nothing was hooked into the trigger.

Do I understand the code incorrectly?


(Jens Maier) #16

A dynamic method call and hash lookup is not “zero” work… :smile: Altho I’m not sure how this could be improved short of overriding Kernel::load and messing with the loaded code. But good luck getting the load order right.


(Sam Saffron) #17

One technique could be monkey patching on demand.

That said, I just want consumers of a feature before building this, there is a non-zero cost maintaining and documenting a bunch of unused hooks.


(Thomas Purchas) #18

Well both myself and @sigurdur what to build plugins that require these hooks.

In my case I want to be able to assign groups based on custom fields passed through SSO.

@sigurdur wants to add people to external mailing lists based on various stats.

Is that enough for these hooks?? Or have I misunderstood what you mean by consumers?


Integration with Smyte for Spam protection
(Thomas Purchas) #19

@sam could you provide us with some clear instructions on how best to move forward with this issue?

I understand that you want consumers of this feature, and I think @sigurdur and myself fulfil that goal. Do you want someone to design a monkey patch system for triggers to minimise overhead on unused calls, or is the current system of detecting consumers light enough?


(Sam Saffron) #20

You need all of them? What are the minimal number of new callbacks you need?