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

timelineview now shows the name and groups - better overview, if you … #340

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ghost
Copy link

@ghost ghost commented Jun 12, 2017

…have many crons with groups

@ghost
Copy link
Author

ghost commented Jun 12, 2017

better timeline-overview

@dlandmann
Copy link

dlandmann commented Jun 12, 2017

bildschirmfoto 2017-06-12 um 15 21 53

Name and Group are only shown if it set in Job-Configuration.

.idea/vcs.xml Outdated
@@ -0,0 +1,6 @@
<?xml version="1.0" encoding="UTF-8"?>
<project version="4">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove this file

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi - I just remove the dotfile. Sorry :-)

*
* @return array
*/
public function getAvailableJobCodes()
public function getAvailableJobs()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets be non breaking by keeping the old method.

@@ -55,6 +53,15 @@ public function getName()
return $name;
}

public function getGroups()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add doc comment with return type

* @method string getGroups()
* @method Aoe_Scheduler_Model_Job setOnSuccess($onSuccess)
* @method string getOnSuccess()
* @method array getGroups()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getgroups can be removed from here as it's implemented below.

@@ -17,9 +17,7 @@
* @method Aoe_Scheduler_Model_Job setParameters($parameters)
* @method string getParameters()
* @method Aoe_Scheduler_Model_Job setGroups($groups)
* @method string getGroups()
* @method Aoe_Scheduler_Model_Job setOnSuccess($onSuccess)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you remove setOnSuccess and getOnSuccess? these methods are being used

{
$groups = $this->getData('groups');
if (empty($groups)) {
$groups = $this->getJobCode();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like it. Its surprising to get jobCode from getGroups getter.
I also don't see a benefit of this change.
In the template you do

if ($_groups !== $_jobCode):
                                echo $this->__('<b>Crongroups:</b> ') . $_groups;
                            endif;

which can be simplified to "if ($_groups):" once you remove adding of $this->getJobCode(); to groups, right?

@dlandmann
Copy link

@tmotyl This PR is from 3 years ago. I will check and fix the code asap....

@tmotyl
Copy link
Contributor

tmotyl commented Apr 20, 2020

I know :)
I've got access to this repo recently. I'm trying to help out and move some things forward.
Thanks to my company macopedia.com for supporting opensource tools we use.

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.

2 participants