- Allow users to specify identifiers for scheduled jobs, so they can create identifiers like "node_3" or "n_{nid}u_{uid}" with input evaluators.
- Add an action to allow cancelation by of jobs by id.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

klausi’s picture

Assigned: Unassigned » klausi
Issue tags: +gsoc:rulesmonkey
klausi’s picture

FileSize
7.24 KB

OK, here is a first patch to do a listing of scheduled jobs using Views.

Next steps:
* add DB field "identifier" to rules_scheduler table with DB schema API
* add input field for "identifier" in the schedule action form
* update the View to incorporate the new field
* add cancel action for scheduled rule sets
* add link to cancel scheduled rule sets in the View

klausi’s picture

FileSize
10.13 KB

Next updated patch: includes new DB field and updated View.

At the moment token replacements (like "node_[node:nid]") in this identifier field do not work.

Remaining work:
* add cancel action for scheduled rule sets
* add link to cancel scheduled rule sets in the View
* solve the token replacement problem

klausi’s picture

FileSize
12.15 KB

Another updated patch: a delete action of scheduled tasks has been added.

The whole modified rules code is now also available in the rulesmonkey CVS repository:
http://drupal.org/project/rulesmonkey
http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/rulesmonkey/

Remaining work:
* create menu path and delete form for identifiers for scheduled tasks
* add link to cancel scheduled rule sets in the View
* solve the token replacement problem

klausi’s picture

FileSize
15.86 KB

Further progress: menu path for deletion is now available, single tasks can be deleted from the view. Token replacements are working, too.
This is getting finished soon.

Remaining work:
* deletion (cancellation) by rule set name on the scheduling page is not implemented yet
* input validation on the rules action for deletion of tasks
* more code comments

klausi’s picture

FileSize
21.69 KB

And some further improvements: deletion is now possible per rule action or manually, manual scheduling of rule sets is now possible, too. At first I wanted to separate the issue of manual scheduling from this topic, but it is closely related and difficult to keep standalone. So here is a patch that implements all necessary features, some small enhancements are still open and will be done next week:
* input validation on the rules action for deletion of tasks
* more code comments, description texts
* move some files into an include subfolder?

klausi’s picture

Status: Active » Needs review
FileSize
23.24 KB

Finally, here is a patch that is ready for review now.

klausi’s picture

FileSize
23.77 KB

Fixed some coding standards issues.

fago’s picture

Status: Needs review » Needs work

Great! Some points:
(For this being the first patch this is a really short list.. Good work!)

>require_once('rules_scheduler.rules.inc');

This won't work for some configurations, as it depends on a set include_path of php. Use drupal_get_path() to get the relative path to the drupal install or best, just use module_load_include().

* Use format_date() for printing dates: -> %date, $task['date'] in the confirm form.

* Put hook_views_data() in the views.inc include.

* Please add a comment to rules_scheduler_action(), why these shifting is necessary. Yep this should be already there.

> $form_state['values']['settings']['task_identifier'] == ""

Better use empty() for checking for an empty string.

@rules_scheduler_datetime:

- Hm, the name of the class should reflect what it is: a views handler. So perhaps call it rules_scheduler_views_handler_datetime.
- Why is those complicated gmmktime() line necessary? The result of str_to_time() should be already in UTC?

* Include files: Please put the views files into a "includes" folder. The admin form shouldn't sit in the main module, best create a new rules_scheduler.admin.inc and put all admin-form stuff in there. So this isn't included in every page load. Also the form of the delete action can go into a module.rules_forms.inc - best also to so, even if it's not much code now.

klausi’s picture

Status: Needs work » Needs review
FileSize
25.79 KB

I tried to cover all comments in this patch. The complicated date handling is necessary to calculate the local time from datetime DB field.

criz’s picture

great work! will test it as soon as possible!

aeronox’s picture

I'm looking forward to this; I will often need to delete instances of a scheduled rule.

Rules is fast becoming my one-module-fits-all preference over other contrib modules, which is perfect.

Great work klausi :D

I'm still new to patch/CVS etc. When will this be a part of the stable version?

fago’s picture

Status: Needs review » Needs work
FileSize
28.41 KB

@#12: Once it's ready.

@patch:
Code looks good now. Great that you already fixed the timezone issue for the rules date handling!

I tested it and fixed some small things I found during testing:
* the menu arg loading functions needs to be in the .module - fixed
* added "in GMT" to the description of the date text + changed the default to be the current date as this fits better for the manual scheduling case.
* Fixed the date in the help text to be GMT too.

However I tested values like "+1 day" and "now" and notices that they aren't working correctly.. For our case (CEST) it adds 2h as it interprets the output as GMT, which isn't *really* the case. Perhaps the ini_set() method could be the solution? (Read the old value, set to GMT and set it back again afterwards?)

Apart from the odd timezone problem, when we switch to GMT I think it would we should notice the admin about that when upgrading - please add such a notice. You can just use drupal_set_message() during the upgrade for this.

updated patch attached

klausi’s picture

Status: Needs work » Needs review
FileSize
31.17 KB

Alright - the timezone handling of PHP strtotime() and date() is really annoying. I switched to your suggestion and simply turn the default timezone to UTC during date computation.

Patch is attached.

Code is also completely available in CVS (rulesmonkey module).

klausi’s picture

FileSize
31.03 KB

Sorry, I forgot to remove two debug statements in the previous patch, new one is attached.

klausi’s picture

FileSize
26.19 KB

Sorry for spamming again, but the previous patch contains updates that shouldn't be there and reflect changes already done in the CVS repository. I use SVN to provide these patches and didn't have an up to date code basis. CVS really, really sucks and forces me to use SVN :-(

fago’s picture

Status: Needs review » Needs work

oh date_default_timezone_set() is php5 only, so we can't use that :( -> We have to use ini_set() instead. Also you have a typ "rember" ;)

klausi’s picture

Status: Needs work » Needs review
FileSize
26.19 KB

fixed.

fago’s picture

Status: Needs review » Needs work

As we've already talked, this setting is also php5 only :(

klausi’s picture

Status: Needs work » Needs review
FileSize
26.7 KB

New approach taken from http://at.php.net/manual/en/function.strtotime.php#87900

It does not work for time strings like "now" and "+1 day", but I did a workaround using the RULES_DATE_REGEX_LOOSE constant to detect those cases.

fago’s picture

Status: Needs review » Needs work

Oh, that's a good clean method. Nice catch! I tested it and it's working fine now, however I noticed another issue (arg!). The "update" operation isn't working right yet: If one uses the same identifier twice the task should be updated, but currently it inserts another task - this way it isn't possible to update an existing task (e.g. update the time). Basically you'd just need to remove any possible previous tasks (but only if there is an identifier of course).

Also we should mention this somewhere, perhaps in the description of the identifier textfield?

klausi’s picture

Status: Needs work » Needs review
FileSize
28.67 KB

OK, new strategy when tasks are inserted: if a task with the identifier already exists for the rule set, then it is replaced. Description of the identifier field is also extended with this information.

I also did some refactoring to avoid having the same code twice: insertion of new tasks lives now as an API function in rules_scheduler.module (doxygen comments included).

klausi’s picture

FileSize
28.69 KB

small bugfix for rules_scheduler_schedule_task()

fago’s picture

Status: Needs review » Fixed

Everything is fine now, committed. :)

Status: Fixed » Closed (fixed)
Issue tags: -gsoc:rulesmonkey

Automatically closed -- issue fixed for 2 weeks with no activity.