WIP: Reviewer & Contributor guides for docs #704

it33 · 2016-12-05T00:14:33Z

High-level, I believe:

We need explicit goal and principles to get everyone on the same page
We need to verify PRs after they're merged
We need a clear contributor's guide and reviewer's guide so everyone is using the same process and criteria
When a PR comes in, we should be explicit about special reviews required (technical review, structural review, Sphinx-specific review, TOC-specific review, testing required, etc.) and the appropriate reviewers to make those approvals

I hope there's a change to review and get people's feedback on this draft, and test it out on some real PRs, e.g.:

WIP:

Need links to GitHub label queries plus pages that don't yet exist.
Needs place in TOC

lfbrock · 2016-12-05T15:28:32Z

source/process/docs-contributors-guide.rst

+Create a Ticket 
+---------------
+
+1. **Select a Help Wanted ticket** - Please select a `HELP WANTED ticket <https://github.com/mattermost/docs/issues?q=is%3Aopen+is%3Aissue+label%3A%22Help+Wanted%22>`_ that you'd like address with your contribution, or create a new ticket to discuss an improvement you'd like to make and wait for a project Member to approve and add the `Help Wanted` label so the change will be accepted.


"you'd like address" is incorrect.
"project Member" seems weird.

Fixed: 311bc77

lfbrock · 2016-12-05T15:29:09Z

source/process/docs-contributors-guide.rst

+
+If your update is small and an obvious improvement, you can skip this step and submit a PR directly. 
+
+2. **Become an Approved Contributor** - Please complete the `contributor license agreement <https://www.mattermost.org/mattermost-contributor-agreement/>`_ (a.k.a. "CLA"). This is a standard form for most open source projects and it adds your the approved contributor's list for the Mattermost open source project. There are answers to frequently asked questions in the link above. 


"approved contributor's list" is incorrect. It should be "contributors"

Fixed: 311bc77

lfbrock · 2016-12-05T15:30:45Z

source/process/docs-contributors-guide.rst

+
+2. **Become an Approved Contributor** - Please complete the `contributor license agreement <https://www.mattermost.org/mattermost-contributor-agreement/>`_ (a.k.a. "CLA"). This is a standard form for most open source projects and it adds your the approved contributor's list for the Mattermost open source project. There are answers to frequently asked questions in the link above. 
+
+3. **Create your Pull Request** - Please create a pull request for the documentation change you would like to make. Make sure to review and follow the `Style Guide <https://docs.mattermost.com/process/sg_mattermost-doc-style.html>`_. Please include a link to the Help Wanted ticket if you have one. 


We should update the style guide URL before linking to it elsewhere, I think "sg" is confusing/"not obvious".

lfbrock · 2016-12-05T15:31:36Z

source/process/docs-contributors-guide.rst

+
+3. **Create your Pull Request** - Please create a pull request for the documentation change you would like to make. Make sure to review and follow the `Style Guide <https://docs.mattermost.com/process/sg_mattermost-doc-style.html>`_. Please include a link to the Help Wanted ticket if you have one. 
+
+4. **(Optional) Meet the Documentation Community** - You can join the Mattermost Documentation channel on the community server and introduce yourself to other contributors and project members. 


Why is the link to the server not included here?

Fixed: 311bc77

lfbrock · 2016-12-05T15:32:23Z

source/process/docs-contributors-guide.rst

+
+1. **New PR submitted** - Each PR created sends a notification to the `Documentation Channel <https://pre-release.mattermost.com/core/channels/documentation>`_ on the Mattermost contributors server, alerting reviewers. 
+
+2. **Triage** - Project Member assigned to triage reviews doc repo within 1-2 working days and applies labels appropriate "approval required" labels. Use **Saved Reply** template linked to this page to let submitter know what to expect.


"applies labels appropriate "approval required" labels" is incorrect.

Also the "approval required" labels is not very clear, we should link to somewhere explaining the labels.

Fixed: 311bc77

Will add link later XXX

lfbrock · 2016-12-05T15:45:31Z

source/process/docs-reviewers-guide.rst

+
+- Please read `Contributor's Guide for docs.mattermost.com XXXXXX <XXXX>`_ for the process by which Approved Contributors submit PRs and engage with reviewers. 
+
+Review Labels 


I'm not really sure I agree with the labeling concept, it seems very confusing for people external to the team. But I guess you've considered that and still think it's important to do?

Also do we only need one review then?
And what about "Content review"/"Accuracy review", is that covered by the Style Guide review....? If so, Style Guide review seems like a weird name and a little limiting.

Also I'm a little confused by the "Post-Merge Editing Required" label, why would we merge something that still needs to be edited? In what cases would it be appropriate to use this?

What would a minor correction be? Do we want to be merging docs with spelling errors and fixing them afterwards?

While I don't think the labels system is perfect, I think reviewers have different areas of expertise and levels of skill and aren't interchangeable. Some reviewers may be more technical, some have stronger grammar, and we need a system that delivers a process as the sum of our strengths.

I think we should review the system, implement it as a pilot and evaluate the pros and cons vs. other options.

I'm fine changing Style Guide to another name, as well as changing Needs ... Approval labels, etc. The goal is "sum of our strengths" in my mind.

Editing would be after a merge--or maybe they're able to edit in a staging environment of some kind. I think the role is traditional editor

lfbrock · 2016-12-05T15:46:40Z

source/process/docs-reviewers-guide.rst

+
+- **Approved Contributor** - Anyone who's completed the CLA. This is the minimum role to submit a PR for review. 
+- **Project Member** - Approved Contributor granted ability to apply labels. Often a staff member from Mattermost, Inc. 
+- **Project Member Reviewer on call** - Member assigned to review PRs in certain approval categories. 


Why is it "on call", are people assigned this role over time periods? It's not very clear.

lfbrock · 2016-12-05T15:51:15Z

source/process/docs-reviewers-guide.rst

+PR Review Process for Reviewers 
+---------------------------------------
+
+The following process is used determine how reviewers on-call leave feedback on doc PRs submitted: 


why is it "on-call" here and "on call" above? If you're using it as an adjective, I think it would be "on-call reviewers"?

Fixed to "on rotation"

lfbrock · 2016-12-05T15:57:40Z

source/process/docs-reviewers-guide.rst

+
+The following process is used determine how reviewers on-call leave feedback on doc PRs submitted: 
+
+1. **Reviewers** - Reviewers on call should `browse open doc PRs <https://github.com/mattermost/docs/pulls>`_ at least once daily while on-call, and leave feedback, approve or merge any open PRs.


Are you going for "while on-call" as an adjective? It's a little weird when you use it as a verb above.

lfbrock · 2016-12-05T15:58:58Z

source/process/docs-reviewers-guide.rst

+
+1. **Reviewers** - Reviewers on call should `browse open doc PRs <https://github.com/mattermost/docs/pulls>`_ at least once daily while on-call, and leave feedback, approve or merge any open PRs.
+2. **Verifiers** - Verifier on call should `browse query of PRs needing verification XXX<linked_needed>`_ at least once daily while on-call and verify and acknowledge PRs as appropiate. If there's an issue, Verifier should message Reviewer to submit a PR to correct issues. 
+3. **Editors** - Editor on call should `browse query of PRs needing editing XXX<XXX>`_ daily and submit a PR with appropriate edits. 


How will editors know what to edit? Does this need to be stated in the PR comments somehow? This should be made clearer.

it33 · 2016-12-12T20:38:54Z

source/process/docs-reviewers-guide.rst

+
+The following labels are used to indicate the type of review required for each doc PR: 
+
+- **Needs Style Guide Approval** - Review for `style guide requirements <https://docs.mattermost.com/process/sg_mattermost-doc-style.html>`_, and specifically: 


Need to add "Needs accuracy approval" for checking functionality matches documentation

it33 · 2016-12-12T20:40:05Z

source/process/docs-reviewers-guide.rst

+
+- **Needs Verification & Acknowledgement** - docs.mattermost.com needs to be verified and acknowledgement added to PR.
+
+- **Post-Merge Editing Required** - PR can be merged and someone will follow-up with edits for minor corrections.


Change to pre-merge edit, and clarify use case--add a clear comment of what is not matching style guide

JeffSchering · 2017-01-05T18:36:44Z

Just curious... how does the "Edit on GitHub" link figure into the process for contributors?

JeffSchering · 2017-01-11T18:59:28Z

source/process/docs-reviewers-guide.rst

+- **Level 4** - 95%+ track record of correct reviews or maintainer of doc repo.
+
+Extended periods of consistent performance are recognized by level increases. Repeated inconsistency reduces an reviwers level. 
+


A few questions about the levels:

What's the difference between a "good review" in Level 2, and a "correct review" in Levels 3 and 4?

Do you keep a record of reviewers and their ratings and their progress towards the next level? Is that record public?

How does everyone know who has what level?

Do higher ratings have greater responsibilities? Any perks? Special powers?

JeffSchering · 2017-01-12T17:34:00Z

source/process/docs-contributors-guide.rst

+2. **Triage** - Project Member assigned to triage reviews doc repo within 1-2 working days and applies the `appropriate "Needs ... Approval" labels to indicate required approvals XXX <XXX>`_. Use **Saved Reply** template linked to this page to let submitter know what to expect.
+
+3. **Reviews** - Within 5-10 working days, for each `"Needs ... Approval" XXX <link to labels explaination>`_ label, reviewers on rotation either a) leave feedback or b) mark PR "Approved" and remove the appropriate label. The reviewer requiring changes adds "Awaiting Submitter Action" label if it is not yet applied. After submitter addresses feedback, the 5-10 working day clock restarts.
+


I think in practice, "5-10 working days" will end up being "10 working days", which seems kind of long.

JeffSchering · 2017-01-17T04:55:44Z

source/process/docs-reviewers-guide.rst

+- **Post-Merge Editing Required** - PR can be merged and someone will follow-up with edits for minor corrections.
+
+- **Awaiting Submitter Action** - Merge should not happen until feedback is addressed. Note: This label should be applied for any work-in-progress PR, a separate label is not needed. 
+


I think the labels might be too granular. I think if you create roles such as Reviewer, Technical Reviewer, and Editor, the people with those roles should be responsible for making sure that, for example, the changes are tested or the Sphinx output is verified. And then the labels would be of a different nature... "Review Required", "Technical Review Required", "Copy Edit Required", and the like.

amyblais · 2019-01-03T23:42:57Z

Closing this as this has been open with no activity for a while - it can always be re-opened if needed.

it33 added 5 commits December 4, 2016 16:14

WIP: Reviewer & Contributor guides for docs

fa20807

Removing Reviewer & Contributor Guides

79122cb

Contributor's Guide for docs

355af78

Reviewer's Guide for docs

d521120

Rename docs-reviewer-guide.rst to docs-contributors-guide.rst

d91c2da

lindy65 added the Work In Progress Not yet ready for review label Dec 5, 2016

lfbrock reviewed Dec 5, 2016

View reviewed changes

lfbrock added the Awaiting Submitter Action Blocked on the author label Dec 5, 2016

it33 added 6 commits December 5, 2016 11:20

Updating based on feedback from lfbrock

311bc77

Changing Style Guide link to XXX placeholder

c8c3b76

Changes based on lfbrock feedback

a5cc700

Adding link to labels explaination

54662b5

Changing on call to "on rotation"

0ebceb0

Updates based on lfbrock feedback

affdc2e

it33 removed the Awaiting Submitter Action Blocked on the author label Dec 9, 2016

it33 commented Dec 12, 2016

View reviewed changes

JeffSchering reviewed Jan 11, 2017

View reviewed changes

JeffSchering reviewed Jan 12, 2017

View reviewed changes

JeffSchering reviewed Jan 17, 2017

View reviewed changes

amyblais closed this Jan 3, 2019

cpanato deleted the docs branch January 12, 2019 15:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Reviewer & Contributor guides for docs #704

WIP: Reviewer & Contributor guides for docs #704

it33 commented Dec 5, 2016 •

edited

Loading

lfbrock Dec 5, 2016

it33 Dec 5, 2016

lfbrock Dec 5, 2016

it33 Dec 5, 2016

lfbrock Dec 5, 2016

it33 Dec 5, 2016

lfbrock Dec 5, 2016

it33 Dec 5, 2016

lfbrock Dec 5, 2016

it33 Dec 5, 2016

lfbrock Dec 5, 2016

lfbrock Dec 5, 2016

lfbrock Dec 5, 2016

it33 Dec 5, 2016

lfbrock Dec 5, 2016

lfbrock Dec 5, 2016

it33 Dec 9, 2016

lfbrock Dec 5, 2016

lfbrock Dec 5, 2016

it33 Dec 12, 2016

it33 Dec 12, 2016 •

edited

Loading

JeffSchering commented Jan 5, 2017

JeffSchering Jan 11, 2017

JeffSchering Jan 12, 2017

JeffSchering Jan 17, 2017

amyblais commented Jan 3, 2019


		If your update is small and an obvious improvement, you can skip this step and submit a PR directly.

		2. Become an Approved Contributor - Please complete the `contributor license agreement <https://www.mattermost.org/mattermost-contributor-agreement/>`_ (a.k.a. "CLA"). This is a standard form for most open source projects and it adds your the approved contributor's list for the Mattermost open source project. There are answers to frequently asked questions in the link above.


		2. Become an Approved Contributor - Please complete the `contributor license agreement <https://www.mattermost.org/mattermost-contributor-agreement/>`_ (a.k.a. "CLA"). This is a standard form for most open source projects and it adds your the approved contributor's list for the Mattermost open source project. There are answers to frequently asked questions in the link above.

		3. Create your Pull Request - Please create a pull request for the documentation change you would like to make. Make sure to review and follow the `Style Guide <https://docs.mattermost.com/process/sg_mattermost-doc-style.html>`_. Please include a link to the Help Wanted ticket if you have one.


		3. Create your Pull Request - Please create a pull request for the documentation change you would like to make. Make sure to review and follow the `Style Guide <https://docs.mattermost.com/process/sg_mattermost-doc-style.html>`_. Please include a link to the Help Wanted ticket if you have one.

		4. (Optional) Meet the Documentation Community - You can join the Mattermost Documentation channel on the community server and introduce yourself to other contributors and project members.


		1. New PR submitted - Each PR created sends a notification to the `Documentation Channel <https://pre-release.mattermost.com/core/channels/documentation>`_ on the Mattermost contributors server, alerting reviewers.

		2. Triage - Project Member assigned to triage reviews doc repo within 1-2 working days and applies labels appropriate "approval required" labels. Use Saved Reply template linked to this page to let submitter know what to expect.


		- Please read `Contributor's Guide for docs.mattermost.com XXXXXX <XXXX>`_ for the process by which Approved Contributors submit PRs and engage with reviewers.

		Review Labels


		The following process is used determine how reviewers on-call leave feedback on doc PRs submitted:

		1. Reviewers - Reviewers on call should `browse open doc PRs <https://github.com/mattermost/docs/pulls>`_ at least once daily while on-call, and leave feedback, approve or merge any open PRs.


		The following labels are used to indicate the type of review required for each doc PR:

		- Needs Style Guide Approval - Review for `style guide requirements <https://docs.mattermost.com/process/sg_mattermost-doc-style.html>`_, and specifically:


		- Needs Verification & Acknowledgement - docs.mattermost.com needs to be verified and acknowledgement added to PR.

		- Post-Merge Editing Required - PR can be merged and someone will follow-up with edits for minor corrections.

		- Level 4 - 95%+ track record of correct reviews or maintainer of doc repo.

		Extended periods of consistent performance are recognized by level increases. Repeated inconsistency reduces an reviwers level.

		2. Triage - Project Member assigned to triage reviews doc repo within 1-2 working days and applies the `appropriate "Needs ... Approval" labels to indicate required approvals XXX <XXX>`_. Use Saved Reply template linked to this page to let submitter know what to expect.

		3. Reviews - Within 5-10 working days, for each `"Needs ... Approval" XXX <link to labels explaination>`_ label, reviewers on rotation either a) leave feedback or b) mark PR "Approved" and remove the appropriate label. The reviewer requiring changes adds "Awaiting Submitter Action" label if it is not yet applied. After submitter addresses feedback, the 5-10 working day clock restarts.

		- Post-Merge Editing Required - PR can be merged and someone will follow-up with edits for minor corrections.

		- Awaiting Submitter Action - Merge should not happen until feedback is addressed. Note: This label should be applied for any work-in-progress PR, a separate label is not needed.

WIP: Reviewer & Contributor guides for docs #704

WIP: Reviewer & Contributor guides for docs #704

Conversation

it33 commented Dec 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

it33 Dec 12, 2016 • edited Loading

Choose a reason for hiding this comment

JeffSchering commented Jan 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amyblais commented Jan 3, 2019

it33 commented Dec 5, 2016 •

edited

Loading

it33 Dec 12, 2016 •

edited

Loading