GitHub sync enhancement Success

User tests: Successful: Unsuccessful:

avatar elkuku
elkuku
18 Aug 2014

This will add some more label management.

Note: It could be that there will be some "duplicate events" on the live site because I assume that changing a label will also fire a hook event - not sure.

avatar elkuku elkuku - open - 18 Aug 2014
avatar elkuku elkuku - change - 18 Aug 2014
Labels Added: enhancement
avatar b2z
b2z - comment - 18 Aug 2014

When I edit an issue labels are wrong. For example issue 410 - it has 2 labels: enhancement and GitHub Sync. But when I edit it has 4 labels: enhancement, GitHub Sync, bug and reoccurring.

Well this happens in some random manner - sometimes it shows 4 instead of 2, but sometimes everything is correct :confused:

avatar elkuku
elkuku - comment - 18 Aug 2014

Should be fixed now - silly mistake :wink:

avatar b2z
b2z - comment - 19 Aug 2014

Thanks fixed. Another problem - the labels are not saving for new item.

avatar elkuku
elkuku - comment - 19 Aug 2014

OK... Thanks for testing :wink:

avatar elkuku
elkuku - comment - 19 Aug 2014

Ummm, there is a problem.
GitHub won't accept label changes if you are not authorized to do so but will fail silently only changing the "allowed parts".

So the only solution I see here is to fire another request after saving the item checking if the labels have been successfully added and if not, use the bot account.

Not sure if I really like this so suggestions are very welcome.

avatar mbabker
mbabker - comment - 19 Aug 2014

I'd just do all the label stuff as the bot account so you don't need to get into complicated conditionals for that.

avatar b2z
b2z - comment - 19 Aug 2014

I'd just do all the label stuff as the bot account so you don't need to get into complicated conditionals for that.

Yeap. But the main question is - will we allow to add labels for general users?

avatar mbabker
mbabker - comment - 19 Aug 2014

I wouldn't allow everyone free reign to add labels or that will get out of control rather quickly. Let's try to limit them down to whatever is already in the system.

avatar elkuku
elkuku - comment - 19 Aug 2014

I'd just do all the label stuff as the bot account

I think we should add a new comment stating that the labels have been changed on behalf of user X?

I wouldn't allow everyone free reign to add labels or that will get out of control rather quickly. Let's try to limit them down to whatever is already in the system.

We are talking about adding (or removing) labels to an issue, right?
Not sure who should be allowed to do that either..
Should we limit that to a user group with "edit" rights (aka JBS)?

avatar b2z
b2z - comment - 20 Aug 2014

Should we limit that to a user group with "edit" rights (aka JBS)?

I'd say yes, to JBS. BTW what will be the main purpose for labels? We have statuses and categories. They do not cover all we need? @brianteeman

avatar brianteeman
brianteeman - comment - 20 Aug 2014

Sorry I'm confused - what are labels? I dont see them
You may blame the J!Tracker Application at http://issues.joomla.org/ for transmitting this comment.

avatar b2z
b2z - comment - 20 Aug 2014

Sorry I'm confused - what are labels? I dont see them

Labels - like on GitHub (Needs JoomlaCode Tracker Item, PR-staging and so on). You can see them on the tracker too, but can not manage currently.

avatar brianteeman
brianteeman - comment - 20 Aug 2014

IMHO these should be issue-bot labels only and therefore not for JBS to edit
On 20 Aug 2014 16:14, "Dmitry Rekun" notifications@github.com wrote:

Sorry I'm confused - what are labels? I dont see them

Labels - like on GitHub (Needs JoomlaCode Tracker Item,PR-staging` and so
on). You can see them on the tracker too, but can not manage currently.


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

avatar zero-24
zero-24 - comment - 20 Aug 2014

IMHO these should be issue-bot labels only and therefore not for JBS to edit

@brianteeman What is with the "RTC", "New Feature" , 'Unit testing', ... label?

iirc atm only a Owner or Collaborator can add / change it. Should this expected for the future?

avatar zero-24
zero-24 - comment - 20 Aug 2014
avatar nicksavov
nicksavov - comment - 12 Sep 2014

If I label an item in JIssues, it doesn't label it in github. Example:
http://issues.joomla.org/tracker/joomla-cms/4229
joomla/joomla-cms#4229

Let me know if I should open up a different issue report for it.

avatar elkuku
elkuku - comment - 12 Sep 2014

If I label an item in JIssues

This is currently not possible. It is what this PR is about (in parts), so you have not assigned a label but a category to the issue... :wink:

Currently

  • on GitHub you can create and display labels and no categories
  • on JTracker you can only display labels and additionally create and display categories

Hope this helps a bit..

If you feel that we should change something here, please say so.

avatar nicksavov
nicksavov - comment - 13 Sep 2014

Thanks, it does help!

Why are there labels and categories? Why not just have labels (even if those labels are called "categories") in jissues?

That way if I assign a category called "Code style" in jissues, it adds the label "Code style" in github?

avatar brianteeman
brianteeman - comment - 13 Sep 2014

It would have been helpful if you had been testing the tracker while it was
in development and not waiting until it was already live and in use for
months before suggesting that the developers change the way that it
fundamentally works.

On 13 September 2014 02:21, Nick Savov notifications@github.com wrote:

Thanks, it does help!

Why are there labels and categories? Why not just have labels (even if
those labels are called "categories") in jissues?

That way if I assign a category called "Code style" in jissues, it adds
the label "Code style" in github?


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

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

avatar nicksavov
nicksavov - comment - 15 Sep 2014

Yes, it would have. If only time weren't so limited, then I could do all the things I want to do and can't currently do.

avatar brianteeman
brianteeman - comment - 15 Sep 2014

Well thats life.

On 15 September 2014 17:29, Nick Savov notifications@github.com wrote:

Yes, it would have. If only time weren't so limited, then I could do all
the things I want to do and can't currently do.


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

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

avatar b2z
b2z - comment - 22 Sep 2014

Why are there labels and categories? Why not just have labels (even if those labels are called "categories") in jissues?

My vision is that Categories are more for issues workflow and Labels are for code workflow. Like you do not need to filter on PR-2.5.x, PR-staging or PR-3.4-dev, but you want to mark some items and that's what labels are for.

avatar nicksavov
nicksavov - comment - 22 Sep 2014

You could still do that with github labels, but filter ones that you don't want to appear in the "Category" list in JIssues. The advantage of this approach is that you don't have to duplicate effort and you'd have github integration built-in.

avatar b2z
b2z - comment - 22 Sep 2014

@nicksavov but we are developing "Issue tracking application" and GitHub is like a feature for it. So we are supporting categories locally and do not rely on GitHub's labels.

avatar nicksavov
nicksavov - comment - 22 Sep 2014

Then, as a feature, make the github labels dependent on the JIssues categories :) I don't see the need to have two different labeling mechanisms though.

avatar b2z
b2z - comment - 22 Sep 2014

Then, as a feature, make the github labels dependent on the JIssues categories

That's closer to truth... We need to refactor a lot now to support it. Let see what others think about it.

avatar nicksavov
nicksavov - comment - 22 Sep 2014

Cool, sounds good.

I can test whatever solution you guys decide on.

avatar b2z
b2z - comment - 19 Mar 2015

@elkuku will you able to update this branch? I think that the current implementation is enough if nobody against?

avatar elkuku
elkuku - comment - 19 Mar 2015

@elkuku will you able to update this branch?

Sure, let me find some time...

avatar b2z
b2z - comment - 19 Mar 2015

Yeah time is luckuing currently :(

чт, 19 марта 2015, 17:11, Nikolai Plath notifications@github.com:

@elkuku https://github.com/elkuku will you able to update this branch?

Sure, let me find some time...


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

avatar b2z b2z - change - 22 Mar 2015
Title
Labels
Labels & milestones
avatar b2z
b2z - comment - 22 Mar 2015

So I have updated the branch and improved it with the milestones. Comments are welcome :wink:

Basically everything is working fine (I was testing it hard today). The only thing that concerns me is syncing of changes. See issue 74 - there are many changes on the GitHub (made on my local environment), but nothing on the tracker after the sync. Not sure why :(

If you need to test it live, I will provide my testing server tomorrow.

P.S.
Currently all users with the EDIT rights (JBS and PLT) can add/edit milestones and labels. Please let me know if we should change this.

avatar zero-24
zero-24 - comment - 22 Mar 2015

@b2z

If you need to test it live, I will provide my testing server tomorrow.

Great i will test it if i get some time.

Basically everything is working fine (I was testing it hard today). The only thing that concerns me is syncing of changes. See issue 74 - there are many changes on the GitHub (made on my local environment), but nothing on the tracker after the sync. Not sure why :(

Maybe as we never trigger the #__activities table on sync?

Can you try the following:

  • create a issue with JTracker
  • add the milestone and a label (with JTracker)
  • see if it is synced to github
  • see the activity

After this ty the following

  • create a issue with JTracker
  • add the milestone and a label (on Github)
  • see if it is synced to jtracker
  • see the activity

If the label is changed on Github it will show at the activity in jtracker (current behavior see: http://issues.joomla.org/tracker/joomla-cms/6513; Brian adds a label)

avatar b2z
b2z - comment - 23 Mar 2015

Do you mean JTracker on http://issues.joomla.org ? You can not add labels there...
It was everything in sync for me locally. Seems that the cron job does not sync these activities, but it should. Need to test it on my local environment also.

avatar b2z
b2z - assign - 23 Mar 2015
Assigned to b2z
avatar b2z b2z - change - 23 Mar 2015
Labels Added: GitHub sync
avatar b2z
b2z - comment - 23 Mar 2015

OK regarding sync, it sounds like we are not supporting these events currently - see #610. I will not implement it in this PR (want to keep it clean). That will be the next step.

avatar b2z
b2z - comment - 28 Mar 2015

Finally my test server is ready :) Sorry for the delay...

avatar zero-24
zero-24 - comment - 28 Mar 2015

Finally my test server is ready :) Sorry for the delay...

Thanks @b2z !

I have just tested here.
First of all is it expected that everyone can edit and add milstones / labels? (as i can do it without in any group in your tracker?)

Some things i have notice:
on existings the sync works good see: http://jissues.j4devs.ru/tracker/jtests/75 and jtester/tests#75

excepted the removing of the milstone (set to non milstone)

On a new issue i open:
http://jissues.j4devs.ru/tracker/jtests/76
jtester/tests#76

The sync don't work see the changes at JTracker and Github.

Maybe unrealted but new PRs and issues don't get synced to your tracker:
jtester/tests#77
jtester/tests#78

avatar zero-24
zero-24 - comment - 28 Mar 2015

A i have missed something. Also closed milestones like: Grrr11 can be assigned and will be shown:
milestone_grrr11

Is this expected?

avatar b2z
b2z - comment - 28 Mar 2015

First of all is it expected that everyone can edit and add milstones / labels? (as i can do it without in any group in your tracker?)

Yes I've made it to test easier.

On a new issue i open:
http://jissues.j4devs.ru/tracker/jtests/76
jtester/tests#76
The sync don't work see the changes at JTracker and Github.

Looks like a bug. Not sure why at the moment.

Maybe unrealted but new PRs and issues don't get synced to your tracker:
jtester/tests#77
jtester/tests#78

They will not, because webhooks does not exist.

Also closed milestones like: Grrr11 can be assigned and will be shown:

That's a bug. Will fix.

avatar b2z
b2z - comment - 28 Mar 2015

All issues are fixed.

avatar b2z
b2z - comment - 28 Mar 2015

Please not that GitHub => Tracker sync still will not work.

avatar zero-24
zero-24 - comment - 28 Mar 2015

Great work @b2z ! Works good here also for #76 and #81 Yery much thanks! :+1:

avatar b2z
b2z - comment - 28 Mar 2015

Need a code review now @mbabker @elkuku

avatar b2z
b2z - comment - 9 Apr 2015

Will we make it in ? ;)

avatar mbabker
mbabker - comment - 9 Apr 2015

I'll take a look tomorrow. Getting ready right now to head to the airport.

avatar b2z
b2z - comment - 9 Apr 2015

Safe flight

чт, 9 апр. 2015, 16:03, Michael Babker notifications@github.com:

I'll take a look tomorrow. Getting ready right now to head to the airport.


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

avatar zero-24
zero-24 - comment - 9 Apr 2015

P.S.
Currently all users with the EDIT rights (JBS and PLT) can add/edit milestones and labels. Please let me know if we should change this.

Sorry @b2z i have just notice that this question is not answered and I'm not sure if we should give all JBS members access to the milestones and labels.

I think the idea of the CMS Maintainer (like: #268) is a good one for the task? So this Group and also PLT can do the changes.

avatar mbabker
mbabker - comment - 9 Apr 2015

Well that's just adding a new ACL role for the tracker then. That wouldn't change the underlying code.

avatar b2z
b2z - comment - 9 Apr 2015

@mbabker actually you are not correct. For this we should add new ACL action like edit.something, because we authorize against actions and not roles.

avatar mbabker
mbabker - comment - 9 Apr 2015

Err... Ya, I just realized it. Even if we added that maintainer group, they'd still have the same edit permission as JBS. So it would need a new action.

avatar b2z
b2z - comment - 9 Apr 2015

The question is - should we wait for more coding on adding action or merge it as it is. Then these people will have access to this feature:

    MAT978
    vdespa
    drmmr763
    SniperSister
    Hackwar
    infograf768
    gwsdesk
    pjwiseman
    zero-24
    Harmageddon
    roland-d
    nibra
    topwebs
    rvbgnu
    gunjanpatel
    anupkumarmishra
    mrunalpittalia

Of course I will add an additional action, but not now. Desperately lucking free time these days.

avatar zero-24
zero-24 - comment - 9 Apr 2015

@b2z is it difficult to limit it for PLT until we have the CMS Maintainer Group for now?

avatar zero-24
zero-24 - comment - 9 Apr 2015

If you tell me what needs to be done i can make a PR to fix it.

I guess it is only:
{% if user.check("admin") %}
insted of
{% if user.check("edit") %}

For now?

avatar b2z
b2z - comment - 10 Apr 2015

You are right - we can limit it to PLT checking as admin. But it's not only checking in templates. Checks also should be done in the Save controller. I will look into it today.

avatar b2z b2z - change - 11 Apr 2015
Status New Closed
Closed_Date 0000-00-00 00:00:00 2015-04-11 04:16:34
avatar b2z b2z - close - 11 Apr 2015
avatar b2z b2z - close - 11 Apr 2015
avatar b2z b2z - change - 11 Apr 2015
Status Closed New
avatar b2z b2z - reopen - 11 Apr 2015
avatar b2z b2z - reopen - 11 Apr 2015
avatar b2z
b2z - comment - 11 Apr 2015

Check are added and tested. Now only users who can perform 'admin' action can change the labels / milestones.

avatar zero-24
zero-24 - comment - 11 Apr 2015

Great thanks @b2z !

avatar mbabker mbabker - reference | cb3b21c - 12 Apr 15
avatar mbabker mbabker - merge - 12 Apr 2015
avatar mbabker mbabker - close - 12 Apr 2015
avatar mbabker mbabker - change - 12 Apr 2015
Status New Closed
Closed_Date 2015-04-11 04:16:34 2015-04-12 10:42:16
avatar mbabker mbabker - close - 12 Apr 2015
avatar b2z b2z - head_ref_deleted - 12 Apr 2015

Add a Comment

Login with GitHub to post a comment