CRM-17669 and CRM-17686, flexible scheduled jobs #7505

konadave · 2015-12-28T18:26:13Z

CRM-17669: Add additional run frequencies for scheduled jobs
- CRM-17686: Add field to scheduled job form to specify first/next run

civicrm-builder · 2015-12-28T18:26:16Z

Can one of the admins verify this patch?

colemanw · 2015-12-29T01:53:42Z

jenkins, add to whitelist

colemanw · 2015-12-29T02:30:44Z

CRM/Admin/Form/Job.php

@@ -156,6 +159,13 @@ public function setDefaultValues() {

    CRM_Core_DAO::storeValues($dao, $defaults);

+    // CRM-17686
+    if (!empty($dao->scheduled_run_date)) {
+      $ts = strtotime($dao->scheduled_run_date);


Isn't this value already a timestamp?

timestamp is UTC adjusted and stored internally as a timestamp, but is returned by a query in the same format as datetime.

konadave · 2015-12-29T13:10:57Z

You're right, I screwed up on the disabling of the extension. I have fixed it on my end per your comments above. I think disabling is fine. I will be updating the extension and will add comment to info.xml that it's incompatible with 4.7, so if someone attempts to re-enable it later they should see that. If there is a way for the extension to determine the current version and fail a re-enable attempt, let me know and I'll get that added.

colemanw · 2015-12-29T13:58:54Z

I have fixed it on my end per your comments above.

I don't see that change reflected in this PR. Once you commit your change can you push it to this branch to update the PR.

konadave · 2015-12-29T14:06:25Z

I was working with a beta4 install for writing/testing, then copying files over to my core repo. So I don't have the datepicker at the moment. I will be gitifying my beta4 install later, but for now I must go shovel some snow.

konadave · 2015-12-29T22:45:08Z

@colemanw I believe I've addressed all your comments from yesterday. Thanks for your help. Let me know if you see anything else that you need me to take care of.

colemanw · 2015-12-29T23:02:28Z

templates/CRM/Admin/Form/Job.tpl

+        <td class="label">{$form.scheduled_run_date.label}</td>
+        <td>{$form.scheduled_run_date.html}<br />
+            <div dlass="description">{ts}Do not run this job before this date / time. The run frequency selected above will apply thereafter.{/ts}<br />
+              {ts}Leave blank to run{/ts} {if $action eq 1}{ts}as soon as possible{/ts}{else}{ts}at next run frequency{/ts}{/if}.


Sentence fragments are hard (sometimes impossible) to translate.
I'd suggest doing the full sentence both times. Might feel "less efficient" to write it out twice, but will make translators happier.

konadave · 2015-12-29T23:48:09Z

I have found out how to determine the currently running version of CiviCRM. I would like to use this information in flexiblejobs_civicrm_install and/or flexiblejobs_civicrm_enable to gracefully fail if not 4.6.x. Is this possible?

colemanw · 2015-12-30T00:20:14Z

I think that might be overkill. Marking it as compat with 4.6 only in the xml, plus disabling it during 4.7 upgrade in this PR, ought to cover all bases.

colemanw · 2015-12-30T00:21:29Z

Whoops looks like the test suite is encountering a fatal error when testing this PR. Click "details" and then "console output" to see what's happening. The most relevant line from the console I see is:

ERROR 1067 (42000) at line 2067: Invalid default value for 'scheduled_run_date'

konadave · 2015-12-30T01:03:24Z

That's a MySQL error, which leads me to believe it's an issue with CRM/Upgrade/Incremental/sql/4.7.beta5.mysql.tpl

Here are a couple of examples of timestamp fields being declared in the exact same way...

https://github.com/civicrm/civicrm-core/blob/master/CRM/Upgrade/Incremental/sql/4.3.alpha1.mysql.tpl#L26

https://github.com/civicrm/civicrm-core/blob/master/CRM/Upgrade/Incremental/sql/4.6.5.mysql.tpl#L13

I don't understand why those would be OK and mine isn't?

konadave · 2015-12-30T01:05:43Z

If I delete scheduled_run_date from my test DB and then run the query from 4.7.beta5.mysql.tpl, it executes without error and adds the column.

konadave · 2015-12-30T16:10:50Z

If I change the default value to CURRENT_TIMESTAMP, that should generally be acceptable. When a job is added without a scheduled date specified it would get the default value. The next time that cron runs the job would pass the needsRunning test (as any new job would) and the scheduled date will get reset to NULL. So it really won't affect anything

But, it would set scheduled_run_date for all existing jobs when the column is added, which would then cause all jobs to run on the next run of cron. I could fix this problem by adding a UPDATE civicrm_job SET scheduled_run_date = NULL after the ALTER TABLE statement. Is that acceptable?

konadave · 2015-12-30T23:41:37Z

@colemanw Please see my comment from this morning. Can / should I do that?

colemanw · 2015-12-31T03:09:38Z

jenkins, retest this please

colemanw · 2015-12-31T03:10:29Z

Sorry I also don't see any problem with your sql and why it would crash the tests. Maybe @eileenmcnaughton or @totten can spot it.

eileenmcnaughton · 2015-12-31T04:45:54Z

xml/schema/Core/Job.xml

@@ -61,6 +61,13 @@
    <add>4.1</add>
  </field>
  <field>
+    <name>scheduled_run_date</name>
+    <type>timestamp</type>
+    <default>NULL</default>


Try removing this - I think required=FALSE (or omitting required) is equivalent to default is NULL - but I'm not sure it's valid to explicitly set default to NULL

@eileenmcnaughton Both of the other timestamp NULL DEFAULT NULL fields I referenced both have <default>NULL</default> in their schema definition, but they both also have <required>false</required> which my definition does not. Do you think if I add that to Job.xml that it would pass the test?

Worth trying I guess!

konadave · 2015-12-31T15:55:34Z

@colemanw Just noticed that beta5 has been released. That means I need to add my alter table statement to 4.7.beta6.mysql.tpl correct? How can I remove 4.7.beat5.mysql.tpl from my PR, or can you selectively not include it when you merge?

konadave · 2015-12-31T17:32:13Z

I can just edit out my alter table statement in 4.7.beta5.mysq.tpl. I can download 4.7.beta6.mysql.tpl and add it to my working copy, but when I commit it will be an added file. Will that cause issues when you merge, given that it already exists in master?

colemanw · 2015-12-31T17:45:20Z

I'll let @totten field that question about upgrades and merges since he's been working on making that process less of a headache.

totten · 2015-12-31T22:06:53Z

Yeah, I've been working off-and-on on #7440. At the moment, it's still WIP (getting my head around multilingual upgrades), and there's no ETA, so let's continue with business-as-usual until the PR is merged.

In the case, my suggestion would be:

Update your branch (e.g. git pull --rebase origin master or git pull --rebase upstream master, depending on how your remotes are named). This will give you the current 4.7.beta6.mysql.tpl.
Add a new commit which moves the SQL from 4.7.beta5.mysql.tpl to 4.7.beta6.mysql.tpl.
Push the branch. Since it was rebased, you'll need to force it (e.g. git push konadave flexiblejobs -f).

(Note: Rebasing works well as long as the flexiblejobs branch is the only one with this change. If there are other branches or people juggling the same commits, then omit the --rebase option. The history won't be as pretty, but it'll merge better.)

...to specify first/next run

jobs. Also removes run frequencies added in CRM-16500 as they are no longer needed.

...when upgrading.

konadave · 2016-01-01T00:10:49Z

@colemanw When I made commit 777717c this morning the test passed. After following totten's instructions and committing 902e557, the test is now failing. I looked at the console output and don't understand what's going on, but it is failing differently than yesterday. I don't know if it's something I did or what.

colemanw · 2016-01-01T02:38:10Z

might be unrelated. jenkins, retest this please.

colemanw · 2016-01-01T13:55:18Z

still looks unrelated. jenkins retest this please

colemanw · 2016-01-01T17:18:34Z

retest this please

eileenmcnaughton · 2016-01-01T21:24:44Z

This test fail is also affecting Brian's PR

CRM-17669 and CRM-17686, flexible scheduled jobs

colemanw · 2016-01-01T22:29:58Z

I'm merging this because the test failures are not due to this PR.

eileenmcnaughton · 2016-01-01T22:44:11Z

Yay - I think this is a great improvement. Thanks Dave for persevering!!

konadave · 2016-01-01T23:19:02Z

Thank you all for your patience and willingness to work with the newbie. I learned a lot.

agh1 · 2016-05-24T19:07:38Z

Unfortunately, this PR caused a regression of CRM-16276. I've documented the situation at CRM-18671.

civicrm-builder added the master label Dec 28, 2015

colemanw reviewed Dec 29, 2015
View reviewed changes

eileenmcnaughton reviewed Dec 31, 2015
View reviewed changes

konadave added 4 commits December 31, 2015 17:23

CRM-17686 Add field to scheduled job form...

e8f6181

...to specify first/next run

CRM-17669 Add new run frequencies to scheduled...

bda41fc

jobs. Also removes run frequencies added in CRM-16500 as they are no longer needed.

CRM-17686 change database column name

d7bace2

CRM-17686 disable flexible jobs extension...

fb1f385

...when upgrading.

konadave added 5 commits December 31, 2015 17:23

CRM-17686 fix disabling of extension on upgrade

1359940

CRM-17686 switch to datepicker

9ee4786

CRM-17686 extension disable fix, tpl improvement

d357221

CRM-17686 fix next_shceduled_run xml

777717c

CRM-17686 move from beta5 to beta6

902e557

konadave force-pushed the flexiblejobs branch from 4a276c4 to 902e557 Compare December 31, 2015 23:28

colemanw added a commit that referenced this pull request Jan 1, 2016

Merge pull request #7505 from konadave/flexiblejobs

1fd3516

CRM-17669 and CRM-17686, flexible scheduled jobs

colemanw merged commit 1fd3516 into civicrm:master Jan 1, 2016

konadave deleted the flexiblejobs branch February 11, 2016 14:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRM-17669 and CRM-17686, flexible scheduled jobs #7505

CRM-17669 and CRM-17686, flexible scheduled jobs #7505

konadave commented Dec 28, 2015

civicrm-builder commented Dec 28, 2015

colemanw commented Dec 29, 2015

colemanw Dec 29, 2015

konadave Dec 29, 2015

konadave commented Dec 29, 2015

colemanw commented Dec 29, 2015

konadave commented Dec 29, 2015

konadave commented Dec 29, 2015

colemanw Dec 29, 2015

konadave commented Dec 29, 2015

colemanw commented Dec 30, 2015

colemanw commented Dec 30, 2015

konadave commented Dec 30, 2015

konadave commented Dec 30, 2015

konadave commented Dec 30, 2015

konadave commented Dec 30, 2015

colemanw commented Dec 31, 2015

colemanw commented Dec 31, 2015

eileenmcnaughton Dec 31, 2015

konadave Dec 31, 2015

eileenmcnaughton Dec 31, 2015

konadave commented Dec 31, 2015

konadave commented Dec 31, 2015

colemanw commented Dec 31, 2015

totten commented Dec 31, 2015

konadave commented Jan 1, 2016

colemanw commented Jan 1, 2016

colemanw commented Jan 1, 2016

colemanw commented Jan 1, 2016

eileenmcnaughton commented Jan 1, 2016

colemanw commented Jan 1, 2016

eileenmcnaughton commented Jan 1, 2016

konadave commented Jan 1, 2016

agh1 commented May 24, 2016

CRM-17669 and CRM-17686, flexible scheduled jobs #7505

CRM-17669 and CRM-17686, flexible scheduled jobs #7505

Conversation

konadave commented Dec 28, 2015

civicrm-builder commented Dec 28, 2015

colemanw commented Dec 29, 2015

colemanw Dec 29, 2015

Choose a reason for hiding this comment

konadave Dec 29, 2015

Choose a reason for hiding this comment

konadave commented Dec 29, 2015

colemanw commented Dec 29, 2015

konadave commented Dec 29, 2015

konadave commented Dec 29, 2015

colemanw Dec 29, 2015

Choose a reason for hiding this comment

konadave commented Dec 29, 2015

colemanw commented Dec 30, 2015

colemanw commented Dec 30, 2015

konadave commented Dec 30, 2015

konadave commented Dec 30, 2015

konadave commented Dec 30, 2015

konadave commented Dec 30, 2015

colemanw commented Dec 31, 2015

colemanw commented Dec 31, 2015

eileenmcnaughton Dec 31, 2015

Choose a reason for hiding this comment

konadave Dec 31, 2015

Choose a reason for hiding this comment

eileenmcnaughton Dec 31, 2015

Choose a reason for hiding this comment

konadave commented Dec 31, 2015

konadave commented Dec 31, 2015

colemanw commented Dec 31, 2015

totten commented Dec 31, 2015

konadave commented Jan 1, 2016

colemanw commented Jan 1, 2016

colemanw commented Jan 1, 2016

colemanw commented Jan 1, 2016

eileenmcnaughton commented Jan 1, 2016

colemanw commented Jan 1, 2016

eileenmcnaughton commented Jan 1, 2016

konadave commented Jan 1, 2016

agh1 commented May 24, 2016