search

fuzionnz / nz.co.fuzion.transactional

Track delivery, open, click, and bounce for transactional email.
Other
12 stars 16 forks source link

Does transactional_civicrm_buildForm need a filter? #41

Closed michaelmcandrew closed 1 month ago

michaelmcandrew commented 4 years ago

Just doing a quick review of this extension before trying it out. It looks nice.

Looking at the implementation of hook_civicrm_buildForm, I'm not sure what the contact ID is being stashed away for but I'm not sure that it should be stashed on every form load.

Any thoughts on why this is happening?

stesi561 commented 4 years ago

@jitendrapurohit? I see that this is being used here https://github.com/fuzionnz/nz.co.fuzion.transactional/blob/7ad93aafaa2b9fc60bc42b3dfb25efc656229b07/CRM/Mailing/Transactional.php#L130 Which seems to date back to the initial set up.

michaelmcandrew commented 4 years ago

it is then used here: https://github.com/fuzionnz/nz.co.fuzion.transactional/blob/7ad93aafaa2b9fc60bc42b3dfb25efc656229b07/CRM/Mailing/Transactional.php#L292

Maybe the problem was that it is hard to identify all the places where that form is used? Or maybe it wasn't that hard and the original author just didn't get round to adding the filter.

jitendrapurohit commented 4 years ago

Yes, agree with the filters that need to be included in the buildform hook. I've included the fix in the latest update done to the extension code https://github.com/fuzionnz/nz.co.fuzion.transactional/commit/ebb9b195684c869a8d3246ad3f23ae3546f3f947#diff-93be7aa95164c9d34275cc856f8ee88dR97. Thanks for raising @michaelmcandrew

michaelmcandrew commented 4 years ago

@jitendrapurohit - great - thanks! I did try and fix this myself but was not 100% sure what forms would need to be included.