Skip to content

Commit a111c6f

Browse files
author
Eric Sethna
authored
Merge pull request mattermost#719 from mattermost/jasonblais-patch-2
Clarify steps in managing open pull requests
2 parents 51b1860 + 6b55e9c commit a111c6f

File tree

1 file changed

+22
-18
lines changed

1 file changed

+22
-18
lines changed

source/developer/contribution-guide.md

Lines changed: 22 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,9 @@ Thank you for your interest in contributing to Mattermost. Here's the process:
44

55
## Choose a Ticket
66

7-
1. Choose a ticket from the [Help Wanted GitHub Issues](https://github.com/mattermost/platform/issues?utf8=%E2%9C%93&q=is%3Aissue%20is%3Aopen%20%5BHelp%20Wanted%5D) or the [Accepting Pull Requests](https://mattermost.atlassian.net/issues/?filter=10101) (APR Tickets) list
7+
1. Choose a ticket from the [Help Wanted GitHub Issues](https://github.com/mattermost/platform/issues?utf8=%E2%9C%93&q=is%3Aissue%20is%3Aopen%20%5BHelp%20Wanted%5D) or the [Accepting Pull Requests](https://mattermost.atlassian.net/issues/?filter=10101) (APR Tickets) list.
88
- Choose any ticket marked "Open", even if it's assigned, and comment to let people know you're working on it.
9-
- If you're looking for a quick ticket, pick something from the [Good First Contribution](https://mattermost.atlassian.net/issues/?filter=10206) list instead
9+
- If you're looking for a quick ticket, pick something from the [Good First Contribution](https://mattermost.atlassian.net/issues/?filter=10206) list instead.
1010

1111
2. If you have questions post in [Mattermost forum](http://forum.mattermost.org/) or join the [Contributors](https://pre-release.mattermost.com/core/channels/tickets) channel and announce the ticket you'd like to work on so it can be assigned to you.
1212

@@ -18,11 +18,11 @@ The best way to discuss opening an APR ticket with the core team is by [starting
1818

1919
Once you have a ticket:
2020

21-
1. Follow [developer setup instructions](http://docs.mattermost.com/developer/developer-setup.html) to install Mattermost
21+
1. Follow the [developer setup instructions](http://docs.mattermost.com/developer/developer-setup.html) to install Mattermost.
2222

23-
2. Create a branch with `<branch name>` set to the ID of the ticket you're working on, for example `PLT-394`, using command: `git checkout -b <branch name>`
23+
2. Create a branch with `<branch name>` set to the ID of the ticket you're working on, for example `PLT-394`, using the command: `git checkout -b <branch name>`
2424

25-
3. Take a look at the [developer flow](https://docs.mattermost.com/developer/developer-flow.html) to learn how to work with the Mattermost codebase
25+
3. Take a look at the [developer flow](https://docs.mattermost.com/developer/developer-flow.html) to learn how to work with the Mattermost codebase.
2626

2727
## Preparing a Pull Request
2828

@@ -42,27 +42,31 @@ Before submitting a pull request (PR), check that:
4242

4343
When submitting a PR, check that:
4444

45-
1. PR is submitted against `master`
46-
2. PR title begins with the Jira Ticket ID (eg `PLT-394:`, [see examples](https://github.com/mattermost/platform/pulls?q=is%3Apr+is%3Aclosed))
47-
3. PR comment describes the changes and how the feature is supposed to work
45+
1. PR is submitted against `master`
46+
2. PR title begins with the Jira Ticket ID (eg `PLT-394:`, [see examples](https://github.com/mattermost/platform/pulls?q=is%3Apr+is%3Aclosed))
47+
3. PR comment describes the changes and how the feature is supposed to work
4848

4949
## Managing an Open Pull Request
5050

5151
After submitting a PR, before it is merged:
5252

5353
1. Automated build process must pass
54-
- If the build fails, check the error log to narrow down the reason
54+
- If the build fails, check the error log to narrow down the reason.
5555
- Sometimes one of the multiple build tests will randomly fail due to issues in Travis CI. If you see just one build failure and no clear error message, it may be a random issue. Add a comment so the reviewer for your change can re-run the build for you, or close the PR and re-submit and that typically clears the issue.
56-
2. Pull requests require review by an approved reviewer and an approved seconder.
57-
3. Feedback from reviewers needs to be addressed
58-
3. If user interface changes do not match specification in Jira ticket, PM approval is needed.
59-
60-
If you've included your mailing address in Step 1, you may receive a [Limited Edition Mattermost Mug](https://forum.mattermost.org/t/limited-edition-mattermost-mugs/143) as a thank you gift after your first pull request is merged.
56+
2. PM review must pass
57+
- A Product Manager will review the pull request to make sure it:
58+
- Fits with our product roadmap
59+
- Works as expected
60+
- Meets [UX guidelines](https://docs.mattermost.com/developer/fx-guidelines.html)
61+
- The Product Manager may come back with some bugs or UI improvements to fix before the pull request moves on to dev review.
62+
3. Dev review must pass
63+
- Two developers will review the pull request and either give feedback or approve the PR.
64+
- Any comments will need to be addressed before the pull request is ready to merge.
65+
66+
If you've included your mailing address in the signed [Contributor License Agreement](https://www.mattermost.org/mattermost-contributor-agreement/), you may receive a [Limited Edition Mattermost Mug](https://forum.mattermost.org/t/limited-edition-mattermost-mugs/143) as a thank you gift after your first pull request is merged.
6167

6268
### Approved Reviewers
6369

64-
_Please DO NOT @-mention reviewers if they haven't yet commented on your PR._
65-
_Pull requests are reviewed in sequence._
70+
**Approved Dev reviewers include**: coreyhulen, crspeller, enahum, grundleborg, hmhealey, jwilander
6671

67-
**Approved reviewers include**: coreyhulen, crspeller, hmhealey, jwilander
68-
**Approved seconders include**: coreyhulen, crspeller, hmhealey, jwilander
72+
**Approved PM reviewers include**: asaadmahmood, esethna, it33, jasonblais, lfbrock, yangchen1

0 commit comments

Comments
 (0)