dev/core#47 Add clone scheduled job functionality

[mattwire]

Overview

This adds a "clone" or "copy" function to the scheduled jobs page. It is implemented as an API (job.clone) which can be called via a link from the scheduled jobs listing.

Ref https://lab.civicrm.org/dev/core/issues/47

Before

You can't clone a scheduled job. This means you have to manually copy parameters when setting up multiple similar jobs (eg. a geocoding job with different parameters).

After

You can easily clone a scheduled job.

Technical Details

Implemented as a new API function Job.clone and a BAO function "copy".

Comments

I'm not sure what the preferred naming is, "clone" or "copy". There's not much in core to be consistent with here.

[seancolsen]

Cool idea!

I wanted to give this a full review, but I didn't get very far with r-run.

Here's what I did:

1. Go to Administer > System Settings > Scheduled Jobs

2. Find "CiviCRM Update Check" and click more > Clone

3. Observe the following exception

   CiviCRM_API3_Exception: "DB Error: unknown error"
   
   #0 /home/sean/buildkit/build/dmaster/sites/all/modules/civicrm/CRM/Admin/Page/Job.php(144): civicrm_api3("Job", "clone", (Array:2))
   #1 /home/sean/buildkit/build/dmaster/sites/all/modules/civicrm/CRM/Core/Invoke.php(309): CRM_Admin_Page_Job->run((Array:3), NULL)
   #2 /home/sean/buildkit/build/dmaster/sites/all/modules/civicrm/CRM/Core/Invoke.php(84): CRM_Core_Invoke::runItem((Array:16))
   #3 /home/sean/buildkit/build/dmaster/sites/all/modules/civicrm/CRM/Core/Invoke.php(52): CRM_Core_Invoke::_invoke((Array:3))
   #4 /home/sean/buildkit/build/dmaster/sites/all/modules/civicrm/drupal/civicrm.module(445): CRM_Core_Invoke::invoke((Array:3))
   #5 /home/sean/buildkit/build/dmaster/includes/menu.inc(527): civicrm_invoke("admin", "job")
   #6 /home/sean/buildkit/build/dmaster/index.php(21): menu_execute_active_handler()
   #7 {main}
   

I tried a new scheduled job too. Same behavior.

Also worth mentioning that I think "Copy" would be slightly more consistent than "Clone", since that the language that Events and Contributions pages use.

[mattwire]

@seanmadsen Not sure why that's failing for you? Have you got all latest DB upgrades applied on master? - sometimes I see this when I've updated dmaster and haven't run necessary DB upgrades.

I've just updated the PR to use the wording "Copy" and set the name of the new job properly (it needed the "name" instead of the "title" field to be suffixed.

[seancolsen]

@mattwire Thanks for changing "Clone" to "Copy".

I did some further troubleshooting with the error I was experiencing and am now able to submit a full review.

(CiviCRM Review Template WORD-1.1)

• (r-explain) PASS
• (r-test) PASS
• (r-code) PASS
• (r-doc) Changes (maybe) requested
• (r-maint) Changes requested
• (r-run) Changes requested
• (r-user) PASS
• (r-tech) PASS

In order of priority...

Changes requested to resolve r-run

• When copying a job, the new job should have last_run set to NULL. This will resolve two issues:

  1. As a user, I would expect a newly created job (even if it's a copy of an old job) to have never been run.

  2. This change should resolve the error message I was initially experiencing.

More details about the error message I got

I traced the root cause of the error seems do be the fact that I have the NO_ZERO_DATE MySQL mode turned on (as is the default in MySQL 5.7, I think).

By setting a breakpoint here I was able to observe a more detailed error message in the $debuginfo variable:

INSERT INTO civicrm_job (domain_id , run_frequency , last_run , scheduled_run_date , name , description , api_entity , api_action , is_active ) VALUES ( 1 , 'Daily' ,  0 ,  0 , 'CiviCRM Update Check - Copy' , 'Checks for CiviCRM version updates. Important for keeping the database secure. Also sends anonymous usage statistics to civicrm.org to to assist in prioritizing ongoing development efforts.' , 'job' , 'version_check' ,  1 )

[nativecode=1292 ** Incorrect datetime value: '0' for column 'last_run' at row 1]

Then, after running SET GLOBAL sql_mode=(SELECT REPLACE(@@sql_mode,'NO_ZERO_DATE','')); I was able to copy the scheduled job.

Changes requested to resolve r-maint

• Add unit test for Job.clone

  Worth mentioning that I'm not entirely sure where to draw the line when requesting unit tests like this, but I think a unit test might have caught the error message I received, so it seems like a good idea here.

Changes (maybe) requested to resolve r-doc

• This is pretty borderline, but the docs for hook_civicrm_copy here and here maintain a list entities that fire the hook. Personally, I'd rather we just don't try to maintain that list, unless we can auto-generate it (because that sort of documentation is just too prone to fall out of date, in my opinion). But since we do have the list, I guess we might as well either add the "Job" entity to the list, or delete the list (and let developers figure it out for themselves if they need to).

• This is also very minor, but in CRM_Core_BAO_Job::copy I think the @param tag should have the description on two lines like in CRM_Core_BAO_Job::del above it.

[mattwire]

Hey @seanmadsen Thanks for your time reviewing this one and for finding out the issue with NO_ZERO_DATE. I've now updated the PR to take account of that (we should be setting last_run and scheduled_run_date to SQL 'null'). And I've also added a unit test!
So assuming that all tests pass, would you mind taking one more look and confirming if you think it's good to merge?

[tiotsop01]

(CiviCRM Review Template WORD-1.1)

• (r-explain) PASS
• (r-test) PASS
• (r-code) REQUEST CHANGES
• (r-doc) PASS
• (r-maint) PASS
• (r-run) PASS
• (r-user) PASS
• (r-tech) PASS

@mattwire I have added some phpunit tests as described here https://github.com/mattwire/civicrm-core/pulls so you may want to handle these exceptions in your code

[eileenmcnaughton]

@mattwire there is a problem with the PR against your repo- but perhaps you could cherry-pick in or copy in the patch from @tiotsop01

[eileenmcnaughton]

I've taken another look at this & I'm OK merging without @tiotsop01's additional unit tests. They test 2 things

1. that an array must be passed - we have been phasing these out as they are well covered by the SyntaxConformanceTests
2. that the id field is required. Normally we don't test mandatory fields if they are defined i the metadata - which is the case here. I guess not being crud there is maybe a small chance that someone accidentally removes that line & we don't have a test on it, so it is possibly worth adding that test in and ALSO removing the secondary check for the presence of id (in the api function itself)

However, I don't think we need to block on 2 given the review effort & response to review that has already gone in and the fact we have a success test.