Skip to content

Conversation

eihabi
Copy link
Contributor

@eihabi eihabi commented Apr 22, 2021

This PR imports the GPNF Delay Child Notifications for Parent Payment snippet.

It also fixes an issue where notifications were sent twice by filtering on gform_entry_post_save and specific parent form IDs.

#23899 / DWC.

@eihabi eihabi requested a review from claygriffiths April 22, 2021 15:18
Copy link
Contributor

@claygriffiths claygriffiths left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, just need to sync up one of the comments from moving it into the class 🙂

@spivurno
Copy link
Contributor

Solution looks good!

I'm assuming the customer must be using multiple payment gateways; some that support delayed payments and some that do not. If the user isn't using a payment gateway with delayed payments this wouldn't be necessary. If that's correct, could you add a comment to explain some of the logic here?

Only other thing is restructing the code a bit. We have a Snippet Template now that will keep a consistent experience for our customers and for us as we maintain these.

@spivurno
Copy link
Contributor

Haha, after I explained how all this worked to Clay, I realized that it might be simpler to actually remove the function bound to the gform_post_payment_completed filter after $should_send_notification return true. This would also account for the scenario where no payment feed is processed and the notification should still be sent. Admittedly, I'm not sure if that is a real-world scenario.

if ( $context === 'parent' ) {
    $parent_entry             = GFAPI::get_entry( rgar( $entry, 'gpnf_entry_parent' ) );
    $should_send_notification = in_array( rgar( $parent_entry, 'payment_status' ), array( 'Paid', 'Active' ), true );
    // Prevent double notifications for non-delayed payment gateways.
    remove_filter( 'gform_post_payment_completed', array( $this, 'whatever_you_decide_to_name_this_func' ) );
}

@eihabi
Copy link
Contributor Author

eihabi commented Apr 22, 2021

@spivurno That did the trick. I've also formatted the snippet to match the template and tested on the user's site.

@eihabi eihabi requested a review from spivurno April 23, 2021 14:15
Copy link
Contributor

@spivurno spivurno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. It doesn't look like the form_ids arg is being used anymore.

  2. Can we update this so each instance only applies to a single form? Rather than providing form_ids the user would provide a single form_id and create an instance of the class for each form.

    I know some of these details are a undocumented knowledge. Sorry about that. Writing our snippets per form gives more flexibility to control how it works for each form. Doesn't specifically apply here now but if we expand on this with more options in the future we'll be better poised for that granular control.

@eihabi
Copy link
Contributor Author

eihabi commented Apr 23, 2021

@spivurno Done. One thing to note, I had to restore the gform_entry_post_save filter removal as that seems to have caused double notifications being sent again:
remove_filter( 'gform_entry_post_save', array( gpnf_notification_processing(), 'maybe_send_child_notifications' ), 11 );

@eihabi eihabi requested a review from spivurno April 23, 2021 20:01
@spivurno
Copy link
Contributor

Code structure looks good!

And rats! I was hoping we could avoid having to remove the default functionality. Since we're keeping this:

remove_filter( 'gform_entry_post_save', array( gpnf_notification_processing(), 'maybe_send_child_notifications' ), 11 );

Let's add a comment explaining why. And remove this:

// Prevent double notifications for non-delayed payment gateways.
remove_filter( 'gform_post_payment_completed', array( $this, 'gpnf_should_send_notification' ) );

It doesn't the work the way I was hoping it would.

@eihabi
Copy link
Contributor Author

eihabi commented Apr 27, 2021

@spivurno Done!

Copy link
Contributor

@spivurno spivurno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sticking it out, Eihab! Approved. ✅

@eihabi eihabi merged commit 3359930 into master Apr 27, 2021
@eihabi eihabi deleted the eihab/add/23899-delay-child-notifications branch April 27, 2021 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants