Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add or update cron tasks and optional job parameters. #1

Closed
wants to merge 8 commits into from

Conversation

jasonevans1
Copy link

I have added the following functionality to the scheduler in my fork. Add or update cron tasks. When it saves the new or updated task it is saved to the core_config_data table. Support for optional parameters. This feature allows a user to set parameters in the configuration and use then when running the job. It only supports text parameters with no validation. Let me know what you think and any problems or questions you have.

Thank you,
Jason Evans

@fbrnc
Copy link
Member

fbrnc commented Aug 31, 2011

Great contribution! I also had something like this in mind for a long time now. I will review your code soon and if you don't mind I will merge it into the module.

@jasonevans1
Copy link
Author

Yes, that would be great. Thanks.

@fbrnc
Copy link
Member

fbrnc commented Aug 31, 2011

Hi Jason,

I had a short review on your code. Works fine (besides the Cds_Log. See inline comment).

I've some remarks:

  • You can add tasks, but currently it's not possible to delete them, is it?
  • Why seperate the form into "General" and "Settings"? I'd remove the tabs and show all fields in one form.
  • Enabling and Disabling should also be possible in the edit form. Maybe we should find a cleaner solution to implement this than having a comma-seperated-list in the configuration of the disabled tasks.
  • I'd like to have the model field a string field. This enables the user to create new tasks out of a random class/method without having to define the tasks previously as crontab. What do you think?
  • We could prevent saving a task if a code is inserted that already exists to prevent usage errors.

@jasonevans1
Copy link
Author

Yes the Cds_Log is debugging code. I have removed this from my fork.

Here are my answers and comments to your remarks.

  • You can add tasks, but currently it's not possible to delete them, is it?
    Yes, I did not implement a delete.
  • Why seperate the form into "General" and "Settings"? I'd remove the tabs and show all fields in one form.
    The reason I did this was to support the optional parameters. The optional parameters are driven by which model you select on the settings tab. I am using the crontab configuration to define what parameters are supported for a model.
  • Enabling and Disabling should also be possible in the edit form. Maybe we should find a cleaner solution to implement this than having a comma-seperated-list in the configuration of the disabled tasks.
    I agree, I will look into this as I find time.
  • I'd like to have the model field a string field. This enables the user to create new tasks out of a random class/method without having to define the tasks previously as crontab. What do you think?
    Yes, I agree that would be nice. I will look into changing this. The only concern is it would be easy to enter an invalid model/method and the job would fail.
  • We could prevent saving a task if a code is inserted that already exists to prevent usage errors.
    Yes, I agree. I will work on this as I find time.

Thanks for the feedback.

@fbrnc
Copy link
Member

fbrnc commented Sep 9, 2011

Hi Jason,

are you still working on your version? I'd really like to see your features merged to the original module. I think deleting tasks is the most importing missing feature.

Separating "General" and "Settings" is ok, but how could I add optional parameters to my cron task? And how would I be able to access them inside the cron task implementation? To you have an example for this feature?

About the enabling/disabling thing: Do you see an easy way to change this to match the rest of the customization of a cron task?

For the string/select field of model/method configuration: Would be nice to benefit from both: Being able to choose a predifined string from a list without needing to look it up somewhere and without the chance to add typos and having the freedom to add new cron tasks without creating a xml configuration. I bet having both in a clean way (without offering two fields) is not an easy task. So I would stick to how it is now and offer only a select box.

Bye,

Fabrizio

@jasonevans1
Copy link
Author

Yes, I am still working on my version. However, the last couple of weeks have been very busy for me and I haven't had much time.

I will work on deleting tasks in the next couple of days. I will also try changing the enabling/disabling to be another core_config_data record for each job code.

Here is an example of what I was thinking about with using the optional parameters/arguments in a task. http://stackoverflow.com/questions/5673911/magento-store-id-in-cronjob .

Yes, I agree having both a select and a text field would be beneficial. I haven't thought about how I would support both.

Thanks,
Jason

@jasonevans1
Copy link
Author

I am working on deleting tasks. I should have it done soon.

Thanks,
Jason

@jasonevans1
Copy link
Author

I have added the delete option for tasks to the edit form. Let me know if you find any problems.

Thanks,
Jason

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants