[#598] - Add the label that match to the current issue state (POC)
- Closed
- 2 Feb 2015
- Medium
- Build: master
- # 598
- Diff
- zero-24:patch-1
- Success continuous-integration/travis-ci The Travis CI build passed Details
User tests: Successful: Unsuccessful:
Based on 08be5c5 this will add some code that do it generaly for all status that we have on jissues. This will run on every save action and add the status as label on github. (Ref #318)
I hope @b2z @mbabker or @elkuku can have a look into the code and the questions
Open questions
- do we need to create the label bevor adding it via the
$github->issues->labels->add(function or will this done automatic? - here: 08be5c5 we use some logger messages. How can i add a new logger instance or is this not needed?
@zero-24 I agree to @mbabker about putting it into the main controller. But currently we do not have other place, though #596 could be the saver if I am correct about its usage...
As for the code I have some questions and will comment inline, but not now. Why? If #596 will be used then your code should be refactored to match its pattern.
Regarding exceptions - I think that we should do not throw them. Saving an issue should not break because of the labels. May be we can enqueueMessage to let the user know that something went wrong.
@b2z can you have a look into the last commits regarding enqueueMessage? zero-24@d4a6855 and ff23ba9
Should be good now for testing. Thanks 
But I pushed my changes to this repo. See 
| Status | New | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2015-02-02 19:32:35 |
Just a small side note(s).... You would need some code to run this only for specific projects and I guess you should use the defined edit bot credentials for the current project when instantiating the GitHub object.
You would need some code to run this only for specific projects
@elkuku can you tell my what you mean? What kind of specific?
The code only run on projekts that on github see: https://github.com/joomla/jissues/blob/zero-24-patch-1/src/App/Tracker/Controller/Issue/Save.php#L98-130
What he's saying is the same as I did above, there really needs to be something set up so that we aren't coding project specific tasks into the main controller classes. Otherwise, if things are merged as is, every project using this app is going to have the ~status labels applied to it or we'll have to use hardcoded checks for specific projects.
The AbstractHookController for our webhook events uses the initialize() method to set up the event dispatcher and figure out if there are any event listeners for the project and uses that to perform project specific tasks. It looks like #596 is another way to handle the same thinking. The main point is that between those two approaches, it keeps project specific tasks out of an area where code is executing for all projects.
Either approach would work as long as it can access the needed data and services.
I think I'd still rather see whatever's calling the code to do this in a project specific executor. And I think that's something that #596 would address it looks like (we write a common task then somewhere in the UI we update a project to use that action).
Is it a useful feature? Absolutely. But, I don't think that we should be forcing it on each GitHub repo that is hooked up to the application. The parameter addresses it, but now we're back at my original concern of adding tasks that are project centric in our main code which kind of goes against making these parts of the app extensible. I'd scream just as loudly if we tried making JControllerForm in the CMS dependent on a parameter from com_newsfeeds.
making JControllerForm in the CMS dependent on a parameter from com_newsfeeds.
That'd be a decent example 
At a glance, things look OK. This just highlights the need for a plugin system like the GitHub hooks have so we aren't putting code that's really only going to apply to one project into the main controllers.
As for the logging, it doesn't look like we inject a logger into the controllers (the hooks create a Monolog\Logger instance specific within the initialize method), so one option is to call
$this->getContainer()->get('app')->getLogger();to get the one being set in the application class or to create an instance within the controller. For the hooks, it's more important to log that data since there's no human interaction at any point in those request cycles; in this instance it may not be something we need.