So on code review I see two issues.

1) The table instance being passed into the comment listener here is an instance of ActivitiesTable while the RTC method needs the IssuesTable (for no other reason than to have the issue's status ID).

2) The webhook data is a bit different between the pull request listener and the comment listener. For example, the pull request event uses $hookData->pull_request->number while in comments to get the pull ID you'd need to use $hookData->issue->number.

Otherwise, at a glance it looks usable.

Thanks @mbabker something like: c3c01b9 ?

should be better now? zero-24@0521bb9

thanks @mbabker now this should be fixed. And it also run on adding a comment. The TriggerEvent is moved inside the try/catch. the ID is now also fixed.

@zero-24 good job here. We can test it on my test server when PR will be ready on 100% ;)

Thanks @b2z see: zero-24@73513de

We can test it on my test server when PR will be ready on 100% ;)

BTW I've update my test server, so you can test this PR here
http://jissues.j4devs.ru/tracker/jissues-test

thanks @b2z it works but now i see what i miss. If we set it back to pending the label don't get removed. I will try to code it

@b2z @mbabker can you get again a look into the code?
It try to implement now:

• add the RTC label if we change the status to RTC
• remove the RTC label if we change the status
• DRY for removing and adding the label
• base class for the listeners.

Looks good.

Talking about DRY - may we need one more class like JoomlacmsListener which will extend AbstractListener and implement to methods that are common: checkRTCLabel and checkNoCodeLabel ? Then JoomlacmsCommentsListener can extend JoomlacmsListener :)

Or better a trait would be more suitable here?

@mbabker what do you think?

Or better a trait would be more suitable here?

hmm maybe add a method checkLabel to the AbstractListener that check if a specific is set (return true) or not (return false)?

These checks are project specific, so they cannot be in the AbstractListener. IMHO :)

Ah, sorry. I did not read your comment properly. You mean
checkLabel($labelName, $hookData, Github $github, Logger $logger, $project)?

Well this is more abstract :)

Yes. I will try to implement if i get some time today

ok so now some more DRY and the checkLabel method. Happy for a review.

Sorry for spam

Thanks @b2z i will fix that soon

@b2z i have fixed that. Can we retest on your server?

Travis is not happy :P

Candy you restart the job? Looks like he fail?

Lol i mean can you (cool auto correction in phone)

I am not sure how to do it (and may be I do not have enough permissions).

Hmm, I think I found it :)

@b2z

go to: https://travis-ci.org/joomla/jissues/builds/69345767
klick the "Sign in with GitHub" button
allow the access
switch again to https://travis-ci.org/joomla/jissues/builds/69345767
on the right site under the "Settings" dropdown there is a restart button

Yeah I did it, but again it failed... I've restarted it again.

From what I can see the build fails because of checkstyle errors so restarting may not solve this... (?)

Thanks @elkuku at the last time Travis fails on other issues. Some bad html code. Now the issues should be fixed thanks

Thanks @b2z i have just implemented that change ;)

Should be ok now @mbabker @b2z Thanks

The longest PR ever Now it seems that everything is correct. Code is deployed to my test server. Looks ok for me! @zero-24 huge

Seems that working good? :)

@b2z yes the RTC Part is ok now ;) The last commit fix also the issue with the "PR-XXX" labels that was not add bevor ;)

for the "No Code Attached" (Should be on all issues that are not pulls ;)) we need to find a solution but i think we can move this outside of this PR as this is a known issue see: #668

Code deployed to test.

Thanks it works :) Great work @b2z and @mbabker Thanks!

http://jissues.j4devs.ru/tracker/jissues-test/22

If now it is all ok please let me a bit time to write up something for the affected group of People (Commiters, PLT, JBS) to inform about the new RTC "Feature" as this is a change in there workflows ;)

I can give this a bump if this is done (hopefully this week ;)) Thanks again! You are awesome!

Great work
On 8 Jul 2015 9:54 pm, "zero-24" notifications@github.com wrote:

Thanks [image: ] [image: ] it works :) Great work @b2z
https://github.com/b2z and @mbabker https://github.com/mbabker Thanks!

http://jissues.j4devs.ru/tracker/jissues-test/22

If now it is all ok please let me a bit time to write up something for the
affected group of Prople (Commiters, PLT, JBS) to inform about the new RTC
"Feature" as this is a change in there workflows ;)

I can give this a bump if this is done (hopefully this week ;)) Thanks
again! You are awesome!

—
Reply to this email directly or view it on GitHub
#667 (comment).

Well I do not have any comments on the code If @mbabker is ok with it then we can merge

@mbabker As you can seen the info is shared (I hope i don't miss anyone) so this is ready to go if you are ok with it

The missing on are only the bots but I think they follow the code

On 10 Jul 2015 7:50 am, "zero-24" notifications@github.com wrote:

@mbabker As you can seen the info is shared (I hope i don't miss anyone)
so this is ready to go if you are ok with it

You did but I am getting used to it

The missing on are only the bots but I think they follow the code

—
Reply to this email directly or view it on GitHub.

I'll be looking it over today.

On Fri, Jul 10, 2015 at 2:50 AM, zero-24 notifications@github.com wrote:

@mbabker https://github.com/mbabker As you can seen the info is shared
(I hope i don't miss anyone) so this is ready to go if you are ok with it [image:
]

The missing on are only the bots but I think they follow the code [image:
]

—
Reply to this email directly or view it on GitHub
#667 (comment).

@mbabker let's merge it