dev/mail#11 Add pre/post hook for CRM_Mailing_BAO_MailingJob #12231

monishdeb · 2018-05-29T20:18:35Z

Overview

This PR adds pre/post hook for CRM_Mailing_BAO_MailingJob

Before

No pre/post hook call on CRM_Mailing_BAO_MailingJob create or delete
No delete fn present
Directly call CRM_Mailing_BAO_MailingJob object to save/delete mailingJob record.

After

Call the pre/post hook inside CRM_Mailing_BAO_MailingJob::create()
Add CRM_Mailing_BAO_MailingJob::deleteMailingJob to delete Mailing Jobs and add pre/post delete hook
Replace all the DAO call with corresponding create and delete fn in the codebase.

eileenmcnaughton · 2018-05-29T22:28:46Z

@monishdeb your PR conflicts with #12221 - which extends unit testing to MailingJob.

I've added your hook into that PR so assuming the tests pass on it if we can get that merged you can drop your changes to MailingJob.create from this PR

eileenmcnaughton · 2018-05-29T22:42:03Z

I also added a couple of your changes in where they are in the same class & are changing from save to create

eileenmcnaughton · 2018-05-29T22:42:41Z

CRM/Mailing/BAO/MailingJob.php

-        $saveJob->save();
+        $jobParams = array(
+          'id' => $job->id,
+          'start_date' => date('YmdHis'),


this is wrong - but fixed in my PR where I incorporated this change (start_date vs end_date)

monishdeb · 2018-05-30T04:37:27Z

@eileenmcnaughton I have rebased my PR, can you please check now?

eileenmcnaughton · 2018-05-30T04:37:57Z

CRM/Mailing/BAO/MailingJob.php

+   *
+   * @return mixed
+   */
+  public static function deleteMailingJob($id) {


I can see on Mailing the function is called 'del' - I think that is kind of the standard & what the api calls?

oops that was my mistake, changed it now :)

monishdeb · 2018-05-30T04:59:55Z

Thanks @eileenmcnaughton for notifying the delete function, renamed it to del

eileenmcnaughton · 2018-05-30T05:01:00Z

OK - I don't have any concerns about this now - I'll go through it more carefully once the tests pass

monishdeb · 2018-06-01T17:05:29Z

CRM/Mailing/BAO/Mailing.php

+        // Keeping former behaviour when an id is present is precautionary and may warrant reconsideration later.
+        'status' => ((empty($params['is_completed']) || !empty($params['id'])) ? 'Scheduled' : 'Complete'),
+        'is_test' => 0,
+        '_skip_evil_bao_auto_recipients_' => TRUE,


@eileenmcnaughton I need to introduce a new parameter here because there need to be some flag which should prevent mailingJob::create to trigger recipient rebuild, as required here.

@monishdeb arg. @totten do you think we can start to grandfather to building recipients being opt-in not opt out?

what about we go with is_create_recipients (ideally if not set it's false, but at least a deprecated warning if not set & set in the api). I feel like we could probably find the places where MailingJob::create are currently called?

Just a note - I checked the already merged changes & there is no case where 'mailing_id' is passed in our 'newly-calling create function' params - so we should be safe from any issues in the already-merged changes

eileenmcnaughton · 2018-06-06T09:58:06Z

CRM/Mailing/BAO/Mailing.php

-   * Delete Jobss and all its associated records
-   * related to test Mailings
+   * @deprecated
+   *  Use CRM_Mailing_BAO_MailingJob::del($id)


If you pull this delete part into a PR I should be able to merge that pretty quickly

Done, extracted del codes in separate PR #12275

ok -that is merged - we can rebase this

eileenmcnaughton · 2018-06-06T10:01:04Z

is this rebased?

seamuslee001 · 2018-07-15T04:11:03Z

@eileenmcnaughton i think this has been rebased now and are you planing to review this one?

monishdeb · 2018-10-10T11:37:30Z

@eileenmcnaughton I have rebased it earlier, sorry for notifying late. Can you please review and merge this PR?

seamuslee001 · 2018-10-10T21:38:16Z

Jenkins re test this please

eileenmcnaughton · 2019-02-17T21:32:09Z

CRM/Mailing/BAO/MailingJob.php

-        $saveJob->end_date = date('YmdHis');
-        $saveJob->status = 'Complete';
-        $saveJob->save();
+        self::create([


so this WILL generate a recipients list...

eileenmcnaughton · 2019-02-17T21:40:51Z

@monishdeb I took another look at this - I can see a spot where recipients are being generated where they shouldn't be - I think we should figure out how to grandfather

  CRM_Mailing_BAO_Mailing::getRecipients($params['mailing_id']);

out of MailingJob ::create - given it's done by the BAO_Mailing anyway

I've added this PR to try to deprecate it out Deprecate generating recipients from MailingJob #13627

eileenmcnaughton · 2019-02-17T21:45:48Z

OK - so this WILL create unwanted getRecipient calls as it stands - unless you are excited about bringing this back to life after so long I think we should try to deprecate that getRecipient stuff for a few cycles & then remove it & then re-attempt this

eileenmcnaughton · 2019-02-24T05:32:09Z

@monishdeb I'm going to close this as I think we need to deprecate that call to create mailing recipients out of MailingJob create first - I think this is the next step

https://github.com/civicrm/civicrm-core/compare/master...eileenmcnaughton:mailin_test?expand=1

monishdeb · 2019-02-24T05:52:01Z

@eileenmcnaughton can I reopen this PR after that other PR is merged? because this is a paid issue for NYSS

eileenmcnaughton · 2019-02-24T07:00:06Z

@monishdeb sure - I just think this PR as is is stalled & won't really be unstalled until we resolve the other - i didn't actually turn the above into a PR but if you want to I'll merge it - it's the best approach I can think of to grandfather the other out - I think we can get to the point of deprecating the functionality for 5.12 & then in some later version resolve the rest

civicrm-builder added the master label May 29, 2018

eileenmcnaughton reviewed May 29, 2018

View reviewed changes

monishdeb force-pushed the dev_mail_11 branch 2 times, most recently from 4b8ce27 to 088eb27 Compare May 30, 2018 04:36

eileenmcnaughton reviewed May 30, 2018

View reviewed changes

monishdeb force-pushed the dev_mail_11 branch from 088eb27 to 4fddf45 Compare May 30, 2018 04:57

monishdeb force-pushed the dev_mail_11 branch 2 times, most recently from 9eba82d to 84b65d6 Compare May 31, 2018 19:31

monishdeb commented Jun 1, 2018

View reviewed changes

eileenmcnaughton reviewed Jun 6, 2018

View reviewed changes

dev/mail#11 Add pre/post hook for CRM_Mailing_BAO_MailingJob

0596cd3

monishdeb force-pushed the dev_mail_11 branch from 84b65d6 to 0596cd3 Compare June 7, 2018 03:48

eileenmcnaughton added the sig:extension support Supports being able to integrate extensions with CiviCRM label Jun 8, 2018

eileenmcnaughton reviewed Feb 17, 2019

View reviewed changes

eileenmcnaughton mentioned this pull request Feb 17, 2019

Deprecate generating recipients from MailingJob #13627

Closed

eileenmcnaughton closed this Feb 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev/mail#11 Add pre/post hook for CRM_Mailing_BAO_MailingJob #12231

dev/mail#11 Add pre/post hook for CRM_Mailing_BAO_MailingJob #12231

monishdeb commented May 29, 2018 •

edited

Loading

eileenmcnaughton commented May 29, 2018

eileenmcnaughton commented May 29, 2018

eileenmcnaughton May 29, 2018

monishdeb commented May 30, 2018

eileenmcnaughton May 30, 2018

monishdeb May 30, 2018

monishdeb commented May 30, 2018

eileenmcnaughton commented May 30, 2018

monishdeb Jun 1, 2018 •

edited

Loading

eileenmcnaughton Jun 3, 2018

eileenmcnaughton Jun 6, 2018

eileenmcnaughton Jun 6, 2018

eileenmcnaughton Jun 6, 2018

monishdeb Jun 6, 2018

eileenmcnaughton Jun 6, 2018

eileenmcnaughton commented Jun 6, 2018

seamuslee001 commented Jul 15, 2018

monishdeb commented Oct 10, 2018 •

edited

Loading

seamuslee001 commented Oct 10, 2018

eileenmcnaughton Feb 17, 2019

eileenmcnaughton commented Feb 17, 2019

eileenmcnaughton commented Feb 17, 2019

eileenmcnaughton commented Feb 24, 2019

monishdeb commented Feb 24, 2019 •

edited

Loading

eileenmcnaughton commented Feb 24, 2019

dev/mail#11 Add pre/post hook for CRM_Mailing_BAO_MailingJob #12231

dev/mail#11 Add pre/post hook for CRM_Mailing_BAO_MailingJob #12231

Conversation

monishdeb commented May 29, 2018 • edited Loading

Overview

Before

After

eileenmcnaughton commented May 29, 2018

eileenmcnaughton commented May 29, 2018

Choose a reason for hiding this comment

monishdeb commented May 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monishdeb commented May 30, 2018

eileenmcnaughton commented May 30, 2018

monishdeb Jun 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Jun 6, 2018

seamuslee001 commented Jul 15, 2018

monishdeb commented Oct 10, 2018 • edited Loading

seamuslee001 commented Oct 10, 2018

Choose a reason for hiding this comment

eileenmcnaughton commented Feb 17, 2019

eileenmcnaughton commented Feb 17, 2019

eileenmcnaughton commented Feb 24, 2019

monishdeb commented Feb 24, 2019 • edited Loading

eileenmcnaughton commented Feb 24, 2019

monishdeb commented May 29, 2018 •

edited

Loading

monishdeb Jun 1, 2018 •

edited

Loading

monishdeb commented Oct 10, 2018 •

edited

Loading

monishdeb commented Feb 24, 2019 •

edited

Loading