lifterlms icon indicating copy to clipboard operation
lifterlms copied to clipboard

llms_proccess_certificate_engagement not firing

Open pixeldevsio opened this issue 1 year ago • 7 comments

Reproduction Steps

add this code to wherever you'd like. I have it in my functions.php file function my_llms_engagement_can_process( $can_process, $user_id, $template_id, $related_id, $engagement_id ) { return false; } add_filter( 'llms_proccess_certificate_engagement', 'my_llms_engagement_can_process', 999999999, 5 ); (can be any priority 1 - 999999) still nothing Try to generate a certificate based on an engagement. The certificate still generates no matter what.

Expected Behavior

  • Expected certificate to not generate

Actual Behavior

  • certificate generated

Error Messages / Logs

  • None

System and Environment Information

System Report ``` LifterLMS 6.11.0 WordPress 6.0.2 Nginx, MariaDB, PHP 8, Redis, Object Cache, Page Cache ```

This issue has be recreated:

  • [x] Locally
  • [x] On a staging site
  • [x] On a production website
  • [x] With only LifterLMS and a default theme

pixeldevsio avatar Sep 27 '22 16:09 pixeldevsio

Update I noticed the function counts errors instead of returning true or false. I added an error in and it still doesnt work

function my_llms_engagement_can_process( $can_process, $user_id, $template_id, $related_id, $engagement_id ) {
	return new WP_Error( 'broke', __( "I've fallen and can't get up", "my_textdomain" ) );

}
add_filter( 'llms_proccess_certificate_engagement', 'my_llms_engagement_can_process', 10, 5 );

pixeldevsio avatar Sep 27 '22 16:09 pixeldevsio

@pixeldevsio where are you adding this code? A custom plugin or a functions.php file in a theme?

A functions.php might be too late, haven't tested yet.

thomasplevy avatar Sep 27 '22 16:09 thomasplevy

@thomasplevy Adding it to functions.php in the theme. I have also tried a plugin, but not sure when that plugin loads vs lifter

pixeldevsio avatar Sep 27 '22 18:09 pixeldevsio

Just added it to an mu-plugin as well and same result

pixeldevsio avatar Sep 27 '22 18:09 pixeldevsio

hey @pixeldevsio I'm sorry for having wasted your time here... I've re-reveiewed all the related code and this particular filter only runs during non-immediate engagements. It's, perhaps, not named as descriptively as it can and without reading the whole context of the function from which it's called it's not immediately obvious. I'm sorry that I neglected to remember this when you first asked for a filter.

I don't have any good excuse here. Developers ask me for custom code all the time and I don't have the capacity to write the custom code that is requested... Due to an error in documentation (really just an ommission of some information that was probably really obvious to us when we wrote it but less obvious several months later when working on memory) it looks like this filter should run for every engagement but it actually doesn't.

See the docs on the preceeding filter: https://github.com/gocodebox/lifterlms/blob/849936e80b86c7ace7c99b450cdbc0a198f440eb/includes/class-llms-engagement-handler.php#L46-L63

This filter is used when an immediate engagement is triggered (one that does not have a delay)

The can_process() method is a catchall for a number of checks we run against delayed engagements to ensure that they still should be run. For example, if a delayed engagement is set to award a cert 30 days after enrollment but the user was deleted before the engagement triggers, the engagement shouldn't send...

You'll see here where we disable the processing check at runtime for immediately-triggered engagements: https://github.com/gocodebox/lifterlms/blob/686ed14d5068956fa5d59d83a60335b444930725/includes/class.llms.engagements.php#L605-L616

This filter might work better, though it's semantically incorrect by name (though that doesn't truly matter, I don't think): https://developer.lifterlms.com/reference/hooks/llms_earned_type_dupcheck/

Similarly to the filter I (incorrectly) originally suggested, this filter will always run for every engagement (assuming the can_process() check returns true).

I think we should add an additional filter in our can_process() method immediately within the method that adds a new check that always runs specifically for the purposes of allowing custom constraints to be placed on enagagements being triggered. This filter must run before llms_skip_engagement_processing_checks.

In the meantime I believe the dupcheck filter (again, while semantically incorrect) would satisfy your needs.

Again, I'm sorry for providing you with a headache here.... My only excuse is that I made a mistake and I didn't read all of the documentation thoroughly. I made a mistake. The code is sound and the language is perhaps a bit unclear if you don't read the code in context. I'm sorry.

thomasplevy avatar Sep 27 '22 18:09 thomasplevy

@thomasplevy thanks for taking the time to look through that! Dude, no worries. Im not hitting a deadline yet. That's next week :) Thanks again

pixeldevsio avatar Sep 27 '22 18:09 pixeldevsio

@pixeldevsio thanks for understanding... I think you said something along the lines of pulling your hair out and I missed this two or three times after you kept insisting it wasn't working. I hate that I delayed you at all. But anyway... Thank you

thomasplevy avatar Sep 28 '22 00:09 thomasplevy