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

Feature Enhancement: Display PR status in the PT list. #286

Closed
particthistle opened this issue Jul 18, 2020 · 18 comments
Closed

Feature Enhancement: Display PR status in the PT list. #286

particthistle opened this issue Jul 18, 2020 · 18 comments
Assignees
Labels

Comments

@particthistle
Copy link

Steps to reproduce the issue

Patch tester doesn't display additional statuses of PRs that may impact their viability for testing.

It makes using the PT a little frustrating (especially for new users) to be playing Patch Roulette and end up coming up with a string of patches that perhaps aren't ones to be displayed as they're draft or otherwise not really a patch for testing.

Expected result

Show status or filter out PR statuses such as Draft where there's not actually something to test.

Actual result

You can't tell what the status of a PR is until you view it on J! Issue Tracker or Github.

Additional comments

Example 1:
joomla/joomla-cms#29196 was closed June 3
joomla/joomla-cms#29198 was the PR related to 29196 which was switched to draft May 26 by George Wilson. So there's not much point continuing testing it while it's a draft.

Example 2:
https://issues.joomla.org/tracker/joomla-cms/26722 is another in the Patch Tester that's a Draft, but it also is in the Unit Tests category, and isn't a typical PR - looks more like a note from George Wilson adding to a team task list.

@richard67
Copy link

Display draft status makes sense. Not sure if that's possible though.

Display the "NPM Resource Change" GitHub label (maybe with a better title "Requires NPM" or just "NPM") might make sense, too, because those can't be tested with PT 4 as long as the CI method is not working reliably.

The same might apply to GitHub label "Composer Dependency Changed".

The "Unit Tests" GitHub label should be ignored. It is automatically set if some code for unit tests is touched or created. This might be a PR dealing ONLY with unit tests, but it also can be a PR adding a new feature including the necessary unit tests. You never know if it makes sense to test that using Patch Tester or not.

@roland-d
Copy link
Collaborator

I have no idea what I can do in terms of filtering issues but I will take a look at it and let you know.

@roland-d roland-d self-assigned this Jul 24, 2020
@roland-d
Copy link
Collaborator

The labels as can be seen in Github have now been added to the list:
image

Further I added a filter for NPM PRs:
image

So you can filter on the non-NPM PRs, those should be usable via the Patch Tester. I am not sure yet but I may just make this the default.

@richard67
Copy link

@roland-d Unfortunately the "Draft" status on GitHub is not signalled by a label, it is a status. Do we have the chance to show that, too?

@richard67
Copy link

@roland-d Next question: The "Filter NPM patches", does it mean only NPM patches are shown, or does it mean all PRs except of NPM patches are shown? The text doesn't really make it clear, it can mean both, as far as my English knowledge reaches.

@roland-d
Copy link
Collaborator

@richard67 I checked the status of the example PR of George, but I don't see in the data we get back from Github that it is Draft, so I do not know how Github knows that.

The Filter NPM patches works the same as Filter RTC status.

@richard67
Copy link

@roland-d In the issue tracker we show the draft status meanwhile, too, George recently added that, so there must be a way.

@roland-d
Copy link
Collaborator

@wilsonge How do you identify a draft issue?

@richard67
Copy link

@roland-d Does this help joomla/jissues#1079? Or this joomla/jissues#1080? I think the first one should be the one.

@roland-d
Copy link
Collaborator

As for draft statuses we cannot retrieve them with the current implementation of the Patch Tester.

@richard67
Copy link

Yes, we have to use the "pulls" API end point, e.g. https://api.github.com/repos/joomla/joomla-cms/pulls/26722?page=1&per_page=1 instead of or in addition to the "issues" end point, e.g. https://api.github.com/repos/joomla/joomla-cms/issues/26722?page=1&per_page=1, and that provides different data in the json.

@particthistle
Copy link
Author

As there's only 16 or so drafts at any time, to improve testing short term as you've implemented all of the other suggestions @roland-d could you look at making RC3 what you put in the BFH chat last weekend (made testing so much easier), and splitting off the draft issue to a new issue for RC4?

@roland-d
Copy link
Collaborator

roland-d commented Aug 1, 2020

@particthistle The PT 4.0.0 RC 3 release is out and I think we should create a new issue for the draft issue. Care to create the issue?

@richard67
Copy link

@roland-d @particthistle Maybe a new issue not only for draft status but also for showing if a PR has conflicts or not? Could be the same change necessary for both.

@roland-d
Copy link
Collaborator

roland-d commented Aug 3, 2020

I prefer 1 issue/request per ticket.

@richard67
Copy link

Then title and description of this issue have to be changed, because title talks about PR status, not about PR's draft status, and the description talks about statuses.

@roland-d
Copy link
Collaborator

roland-d commented Aug 3, 2020

This issue will be closed and new ones created.

@roland-d
Copy link
Collaborator

@particthistle I am going to close this issue as I have implemented the draft status as well. For any other requests, please create a new issue.

image

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

No branches or pull requests

3 participants