Webhook course.created (and likely other ${post_type}.created) event does not fire

[thomasplevy]

Reproduction

Create a new webhook for the course.created topic.
Publish a new course

Expectation

The webhook will be delivered when the course is created

Actual

The webhook is not delivered

Information

We utilize the WP core save_post_{$post_type} hook to trigger the publication of a course. Since this event is fired by WP for both post creation and update we watch the 3rd argument passed to the hook, $update which is true during an update and false during a post creation.

If that 3rd argument is true we don't fire the webhook.

However, this is problematic because WordPress creates the post first as an auto-draft and then fires the hook with $update=false but at this stage most users would very likely NOT expect the hook to fire.

I believe we need to re-evaluate the hook utilized for publication and can maybe use the transition_post_status hooks instead.

HS-113632

[eri-trabiccolo]

@thomasplevy
I've investigated a little bit on how wc does.
Well I've just looked into a "normal" creation of a product, via the wp admin.
When a product is 'saved' this action hook is fired:
https://github.com/woocommerce/woocommerce/blob/4.0.0/includes/admin/class-wc-admin-meta-boxes.php#L222

and this is the actual hook the webhook "listens to":
https://github.com/woocommerce/woocommerce/blob/4.0.0/includes/class-wc-webhook.php#L998
(haven't investigated when woocommerce_new_product occurs to be honest, probably with rest-apis or import?)

The woocommerce_process_product_meta action hook is not fired when doing the autosave as you can see from the wc code.

So basically the 'product.created' and 'product.updated' are webhooks are both potentially fired when the woocommerce_process_product_meta action hook is fired BUT wc discriminates them by checking the product date: // A resource is considered created when the hook is executed within 10 seconds of the post creation date :
https://github.com/woocommerce/woocommerce/blob/4.0.0/includes/class-wc-webhook.php#L254-L260

I think we can then adopt a similar approach, why not?!

[thomasplevy]

@eri-trabiccolo

Thanks for the research!

If we were to consider a resource new by the same criteria (if the hook is triggered within 10 seconds of the post creation date) then we might as well reduce the amount of code we'd need to rewrite and remove the check that skips the firing of the hook if the post's status is "auto-draft":

lifterlms-rest/includes/models/class-llms-rest-webhook.php

Lines 268 to 271 in 63daaa1

	// Ignore auto-drafts.
	if ( in_array( get_post_status( absint( $args[0] ) ), array( 'new', 'auto-draft' ), true ) ) {
	return false;
	}

Right?

[eri-trabiccolo]

That might be fine,
haven't tried though what happens when you publish an "auto-draft", is $args[2] false or true?
because in that case we are not able to discriminate between creation and update.
Why don't we bail if doing autosave like:
https://github.com/woocommerce/woocommerce/blob/4.0.0/includes/admin/class-wc-admin-meta-boxes.php#L187-L195

WooCommerce also doesn't "skip" the check above:
https://github.com/woocommerce/woocommerce/blob/4.0.0/includes/class-wc-webhook.php#L278

[thomasplevy]

@eri-trabiccolo

I think you're right and using the WC time-check method probably makes the most sense... I've just run a few tests to review the data on the post object and the post_date property is updated when moving from auto-draft to published. My concern was that the date would be the initial date of the auto-draft creation but that's not the case.

So if we remove the check on the 3rd parameter in favor of a check on the post_date (within 10 seconds seem just fine like WC we should fix the issue and still ensure that this only runs when actually publishing something.

Additionally if the post is switched to a draft and then back to published later the initial post_date remains unchanged so we wouldn't accidentally trigger created hooks again in the future with this method either

[thomasplevy]

@eri-trabiccolo do you want to tackle this tomorrow or should I have at it?

[eri-trabiccolo]

@thomasplevy
I can try tackling this tomorrow before we meet, if I fail for whatever reason you take over if you like :D
how does this sound to you?

[thomasplevy]

@eri-trabiccolo Perfect thanks

[eri-trabiccolo]

@thomasplevy
I'm working on this, I think the woocommerce approach has a problem:
scheduled posts.
in that case the post date is always in the future and then the resource "is created" at each update.
I think this is on purpose? who knows.

[eri-trabiccolo]

@thomasplevy WIP here:
https://github.com/eri-trabiccolo/lifterlms-rest/tree/webhooks

I just ran some very rough tests that seem to be fine,
more manual tests need to be done and unit tests to be added.

[eri-trabiccolo]

@thomasplevy
man I'm not convinced about the 10s thing.
They're too much, think about some app doing two rest calls:

1. create a course
2. update the course title

I hope that it can do that in less than 10 seconds...
This is of course a kind of an edge case...
Consider that wc uses this method only for the 'woocommerce_process_product_*' action hooks - the ones fired in their metaboxes code...

What do you think?
we can still go with the 10s thing for now, and improve this in the future.

[eri-trabiccolo]

@thomasplevy
I ran a manual test on what happens when cloning a course with the current implementation:
save_post_course and edit_post_course action hooks are fired several times

1. the course is "created" in draft state, save_post_course is fired and the course.created is "delivered" (scheduled) only once, at this very early stage (not ideal because the course is not "complete").
2. then an edit_post_course action hook is fired, and the course.updated is "delivered" (scheduled)
   from this point several other save_post_course and edit_post_course are fired BUT, of course:
   a) the save_post_course won't produce any other valid course.created "resource", because the third param of that hook is false
   b) the edit_post_course even if producing a valid course.updated "resource" won't produce
   any weebhook delivering ONLY because the previous course.updated it's still pending delivery. But this is "just" a case, and it depends on the scheduling which can be turned off with a filter... not ideal.

I did the same test then with the WIP https://github.com/eri-trabiccolo/lifterlms-rest/tree/webhooks

1. the course is "created" in draft state, save_post_course is fired and the course.created is "delivered" (scheduled) only once, at this very early stage (not ideal because the course is not "complete").
2. then an edit_post_course action hook is fired, BUT the course.updated is NOT "delivered" because, of course, 10s are not passed by since the post has been created.

from this point several other save_post_course and edit_post_course are fired BUT, of course:
a) the save_post_course WILL produce other valid course.created "resource", because we're still within the 10 seconds ALTHOUGH it won't be delivered because the previous course.created it's still pending delivery, again this is a case.
b) edit_post_course action hook is fired, BUT the course.updated is NOT "delivered" because, of course, 10s are not passed by since the post has been created.

These (and probably also the one above with the scheduled posts) are the problems that I've found until now.
To me, then, relying on the new 10s method to distinguish newly created and updated posts, it's not a very solid solution, even if it can be "enough" for now, at least to fix the common uses - it works fine if you create a course via the wp dashboard (*).

In the future, though, we need to implement a more solid solution, and we can still get inspiration from wc - as said the 10s rule, for my understanding, is applied only to distinguish newly created and updated posts with actions triggered by the post editor (meta boxes), while it relies on more ad hoc action hooks for the rest. Translated: we need these hooks. We actually already have action hooks we can use because we have:

• the various post-type meta boxes hooks on save, that will cover the creation/update through the wp editor: 'lifterlms_process_' . $post->post_type . '_meta'
• the various post-type hooks on cloning (generators, so also when importing :D): https://github.com/gocodebox/lifterlms/blob/3.37.13/includes/class.llms.generator.php#L440
• various specific rest-api post-type hooks on creation/update (see below and the fix in Fixed "llms_rest_insert_{$this->post_type}" and `"llms_rest_insert… #153 )

What we need is some kind of unification but maybe we can already start using the hooks above - should already cover the various scenarios right? -, and adapt the code just a little bit (once again, thanks woocommerce).

Meantime, though, I open the PR with the WIP that at least fixes this very issue, and, for what I could test, doesn't create drawbacks EXCEPT for the edge cases aforementioned.

I really hope you, dear reader, liked what you read until now, as much as I liked to write it :/

(*) It kinda works with rest api creation too, but let's consider that a course is not a common post, most of its valuable information is contained in the post metas, which are "set" AFTER the post is inserted into the db - in this case, though, we can use specific rest api action hooks, see: https://github.com/gocodebox/lifterlms-rest/blob/1.0.0-beta.10/includes/abstracts/class-llms-rest-posts-controller.php#L299

[thomasplevy]

@eri-trabiccolo

While I feel that using the other hooks is probably better I think we still run into some issues...

Mainly, if we use the metabox processing hooks we still run into a situation where the post might not be really finished. From my perspective this is just as unreliable as what this WIP introduces.

I think we should add the generator hooks -- possibly. Currently the generator users WP core functions for post insertion so this would result in the core hook taking care of it. Although we'd still have the not completely finished post issue here since we inserst and then update meta...

REST is in a similar boat...

Maybe with all that said we need to introduce (into the LLMS core) a hook for each post type that runs after meta information is updated.

We don't have a great "save()" method on our data models like WooCommerce does so their in a simpler situation. For them my primitive understanding of progammatic product creation is:

1. Create data object
2. Update meta
3. Save (this writes to db)

All the hooks can be triggered on save ensuring that there's no "empty" product sent to creation webhooks.

This likely is why their 10s rule works better for them than it does for us..

In any event I've merged the WIP so we can close the pending support request and I'm going to leave this open for further improvement in the future.

[eri-trabiccolo]

@thomasplevy

Mainly, if we use the metabox processing hooks we still run into a situation where the post might not be really finished. From my perspective this is just as unreliable as what this WIP introduces.

That's true and it's true for WC too.

I think we should add the generator hooks -- possibly. Currently the generator users WP core functions for post insertion so this would result in the core hook taking care of it. Although we'd still have the not completely finished post issue here since we inserst and then update meta...

What do you mean? This generator hook is fired at the end of the process:
https://github.com/gocodebox/lifterlms/blob/3.37.13/includes/class.llms.generator.php#L440
when all the metas have been already updated. Am I wrong?

REST is in a similar boat...

Well not really, we have an hook that is fired at the very end of the creation process, again, after all the metas have been added:

https://github.com/gocodebox/lifterlms-rest/blob/1.0.0-beta.10/includes/abstracts/class-llms-rest-posts-controller.php#L299

We don't have a great "save()" method on our data models like WooCommerce does so their in a simpler situation. For them my primitive understanding of progammatic product creation is:

Create data object
Update meta
Save (this writes to db)
All the hooks can be triggered on save ensuring that there's no "empty" product sent to creation webhooks.

This likely is why their 10s rule works better for them than it does for us..

I'm not sure about this Thomas, when you create a product through the editor, for what I can see, only the woocommerce_process_product_meta is fired. And that's the only case when the apply their 10s rule. See:
https://github.com/woocommerce/woocommerce/blob/4.0.0/includes/class-wc-webhook.php#L193-L195
When reacting to other action hooks they don't use the 10s rule.

[thomasplevy]

@eri-trabiccolo I don't know. Solve the problem please.