[ENHANCEMENT] Add PR template

[Firesphere]

By submitting this Pull Request, please confirm the following

Description of the issue:

There is currently no default Pull request template. I believe it would be good to have one. I've compiled this basic list from multiple PR templates that float around in Open Source projects like pi-hole and projects worked on in the last few months.

note
This must be merged in to master to work

How to replicate:

After accepting, make a Pull Request on GitHub and notice the pre-populated description box.

By making this pull request, I confirm that:

• My understanding of the code: 7
• I have read and understand the Contributions guide
• I have checked that another pull request for this purpose does not exist.
• I give this submission freely, and claim no ownership to its content.

This pull request includes the following:

• Code comments where needed

[Firesphere]

This will probably get a bunch of feedback from the maintainers, so until all those are resolved, I'm not going to make a similar PR against other core modules.

[Firesphere]

Also, I don't believe this PR is causing Travis breakage ;)

[tractorcow]

Just my personal opinion, but I would change the tone of the message to be written for the user who will read the comitted PR, rather than the user submitting the PR, as 90% of submitters will leave those fields unchanged.

I.e. I confirm the following rather than please confirm the following

You also mention tests three times, but only in-code documentation once. How about referencing the user docs and suggest the appropriate docs page is updated, if applicable?

The section ## By making this pull request, I confirm that: is an interesting idea. I'll give it some thought, but I'd be interested in the feedback from others on this.

[Firesphere]

Yeah, tone and intention needs adjusting, you're free to adjust wording and I'm happy to adjust things as well. I'd love to hear what others think as well :{)

[dhensby]

1. If we are talking about giving up rights to the code in the PRs this probably needs to come as a more consistent and concerted effort across silverstripe projects and not just in framework and as a simple checkbox that can be edited after the fact.
2. I'm weary of making PRs more daunting and difficult to submit - are we actually addressing a problem that exists by implementing this template?

My personal view is that our project isn't inundated with bad PRs that are duplicate or don't describe issues well. Do other members of the @silverstripe/core-team feel there is a problem that needs solving?

If there is a problem that needs solving can we articulate the problems first and then we can construct a template we think will solve those issues.

[kinglozzer]

I have to admit my first thought was that this looks a bit intimidating, especially for smaller PRs (typos, docs etc). We very rarely struggle to discern what a PR is fixing or which version it’s targeted at

[jonom]

A general issue template might be more helpful than a PR one. I suggested something a while back offline but think I forgot to follow up, whoops!

Framework new Issue template

Framework version affected:

(Provide a full version number [major.minor.patch], or range if known)

Description of issue:

(Screenshots or recordings may be helpful)

How to reproduce issue:

(Provide detailed steps, if known)

Before submitting please confirm:

• I've reviewed the contribution guidelines for issues and bugs.

[dhensby]

Rejecting until we can get a consensus on this

[tractorcow]

To be honest, the biggest issue I have is with vague issues being raised with lack of version info / reproducibility, rather than those who take the initiative to create PRs.

[sminnee]

There are a contributors agreement plugins for github — perhaps one of those would be a better place to start?

[sminnee]

#7487

[sminnee]

I would probably simplify the PR template to show a checklist of common reasons for rejecting a PR. We'd want to come up with that list first.

E.g.

• Have you run composer lint and npm lint to check for style guide issues?
• Have you run tests with vendor/bin/phpunit?
• Have you written unit tests for the new behaviour?

[flamerohr]

I like the initiative, but it's quite daunting for a pull request creation.

If this was a template for creating issues, I think that'd give us a bigger win than a pull request template - e.g. travis runs linting to catch those for us, albeit yes we can ask users to run those before creating, it is covered.

The most important message is probably the "allow maintainers to make changes" permission to be set, so that maintainers could tailor parts accordingly if they have time - as opposed to wait 4 weeks for the creator to have time to get back to this.

[dhensby]

@sminnee as @flamerohr says, even those questions are covered by travis and it's legitimate to rely on travis to run those rather than say to people they can't submit a PR until they have...

The most important message is probably the "allow maintainers to make changes" permission to be set, so that maintainers could tailor parts accordingly if they have time - as opposed to wait 4 weeks for the creator to have time to get back to this.

I think this is enabled by default - I regularly will fix up or rebase PRs for people and the only ones I can't push to tend to be PRs that are from before the feature was added.

[chillu]

Thanks for starting this discussion Simon :) I think the CLA tooling is a better way to solve this than checkboxes in PR templates. I'd prefer to avoid overly prescriptive (borderline intimidating) messaging in those early touch points with the community - a bit of nudging can go a long way there, we don't need "I confirm" style approvals. In this context, here's my proposals for PR and issue templates.

PR template:

Thanks for contributing, you're awesome! :star:
Please describe expected and observed behaviour, and what you're fixing.
For visual fixes, please include tested browsers and screenshots.
Search for related existing issues and link to them if possible.
Please read https://docs.silverstripe.org/en/contributing/code/

Issue template:

## Affected Version

Show version numbers by pasting the output of `composer info --direct`.
Alternatively, hover over the SilverStripe logo in the CMS to basic version information.

## Description

Describe expected and observed behaviour.
For visual issues, please include browser version and screenshots.
Please read https://docs.silverstripe.org/en/contributing/issues_and_bugs/

## Steps to Reproduce

Help us with step-by-step instructions.

[dhensby]

I'd still like us to outline what problems we experience that we are trying to solve :/

[chillu]

Well, during bug triage I often have to guess what version something relates to. Half the time you can guess the major version, but sometimes people just use ancient versions of 3.x and you waste time trying to validate something that's already been fixed in newer releases. In particular, if somebody brings up bugs in unsupported releases, I'd be inclined to close it until they can demonstrate the existence of the bug in a supported release. Getting to that point quicker saves us all time.

Otherwise, I think the descriptions I've proposed are pretty much an outline of what problems we're trying to fix (e.g. "missing screenshots"). What other info do you need? Do you not see any of these as a problem?

[dhensby]

Version number and/or git commit/composer info seem reasonable. Steps to reproduce are good too.

I want to make any instructions short and simple so they aren't daunting or a barrier to reporting issues.

[sminnee]

I like Ingo's recommendations.

[chillu]

Sweet, I've shortened my templates a bit (in the comment above). @Firesphere Do you want to update your PR with them? Unless anyone else has comments?

[Firesphere]

👍 I'm happy to update them :)

[chillu]

Sweet, no more feedback from others, so @Firesphere feel free to update the PR. I guess we now have to replicate that PR to a bazillion core repos as well, sigh.

[Firesphere]

Changes as per @chillu's example done.

PR against master as that's a requirement for templates to work.

[tractorcow]

@dhensby can you approve this? I'm happy from my end.

[kinglozzer]

hover over the SilverStripe logo in the CMS to basic version information

"get" plz :) (probably easiest for whoever merges to just manually edit afterward)

[dhensby]

fixed on merge

[tractorcow]

I just realised why this wasn't working.

Tip: You must create your template in your repository's default branch. Templates created in other branches are not available for collaborators to use.

from https://help.github.com/articles/creating-an-issue-template-for-your-repository/

[Firesphere]

I made the PR against master, as I assumed it was the default branch. My bad! Sorry!

[tractorcow]

Not really your fault. :)

[tractorcow]

Back-ported and fixed. :)