Merge pull request #5376 from edx/sarina/new-contributor-docs

Update Contributing documentation with new PR process. OPEN-111

CONTRIBUTING.rst

@@ -13,9 +13,30 @@ Step 0: Join the Conversation
 
 Got an idea for how to improve the codebase? Fantastic, we'd love to hear about
 it! Before you dive in and spend a lot of time and effort making a pull request,
-it's a good idea to discuss your idea with other interested developers. You may
-get some valuable feedback that changes how you think about your idea, or you
-may find other developers who have the same idea and want to work together.
+it's a good idea to discuss your idea with other interested developers and/or the 
+edX product team. You may get some valuable feedback that changes how you think
+about your idea, or you may find other developers who have the same idea and want
+to work together.
+
+If you've got an idea for a new feature or new functionality for an existing feature,
+please start a discussion on the `edx-code`_ mailing list to get feedback from
+the community about the idea and your implementation choices.
+
+.. _edx-code: https://groups.google.com/forum/#!forum/edx-code
+
+If you then plan to contribute your code upstream, please `start a discussion on JIRA`_
+(you may first need to `create a free JIRA account`_).
+Start a discussion by visiting the JIRA website and clicking the "Create" button at the
+top of the page. Choose the project "Open Source Pull Requests" and the issue type
+"Feature Proposal". In the description give us as much detail as you can for the feature
+or functionality you are thinking about implementing. Include a link to any relevant
+edx-code mailing list discussions about your idea. We encourage you to do this before
+you begin implementing your feature, in order to get valuable feedback from the edX
+product team early on in your journey and increase the likelihood of a successful
+pull request.
+
+.. _start a discussion on JIRA: https://openedx.atlassian.net/secure/Dashboard.jspa
+.. _create a free JIRA account: https://openedx.atlassian.net/admin/users/sign-up
 
 For real-time conversation, we use `IRC`_: we all hang out in the
 `#edx-code channel on Freenode`_. Come join us! The channel tends to be most

docs/en_us/developers/source/process/community-manager.rst

@@ -10,10 +10,11 @@ pull requests. For each pull request, a community manager should:
 #. Read the description of the pull request to understand the idea behind it
    and what parts of the code it impacts. If the description is absent or
    unclear, inform the author that the pull request cannot be reviewed until
-   the description is clearer.
+   the description is clearer. Guide them to the :doc:`pull request cover letter <cover-letter>`
+   guidelines.
 
-#. Evaluate the idea behind the pull request. Is this something that
-   Open edX wants? Discuss with product owners as necessary. If you and the
+#. Help the product team evaluate the idea behind the pull request.
+   Is this something that Open edX wants? If you and the
    product owner(s) all believe that Open edX does not want this pull request,
    add a comment to the pull request explaining the reasoning behind that
    decision. Be polite, and remind them that they are welcome to fork the code
@@ -24,8 +25,8 @@ pull requests. For each pull request, a community manager should:
 
 #. Check that the author of the pull requests has submitted a
    `contributor's agreement`_, added name to AUTHORS file, and any other
-   necessary administrivia. If not, inform author of problems
-   and wait for them to fix it.
+   necessary administrivia (our bot will make an automated comment if this is not
+   properly in place). If not, inform author of problems and wait for them to fix it.
 
 #. Once you’ve verified that the code change is not malicious,
    run a Jenkins job on the pull request and check the result.
@@ -77,25 +78,39 @@ meaningful way.
 If the pull request is small, it can be reviewed immediately. If the community
 manager that is handling this pull request feels comfortable doing the code
 review, then he or she should do so rather than handing it off to a core
-committer. If not, he or she should add a comment to the PR tagging a core
-committer to do the review, and informing the author that it might take a few
-days for the reviewer to get around to it.
+committer. If not, he or she should move the JIRA ticket for the PR review
+into the "Awaiting Prioritization" state and add enough detail on the ticket for
+the product team to understand the size and scope of the changes.
+Inform the author that it might take a few days for the engineering team to review the PR.
 
-If the pull request is not small, it will be handled by the core contributor
-Scrum process. The community manager should:
+If the pull request is not small, it will be handled by the full pull request process:
 
-* Determine which teams of core committers this pull request impacts.
-  At least one developer from each of these teams should review this pull request.
+.. image:: pr-process.png
+   :align: center
+   :alt: A visualization of the pull request process
 
-* For each team, create a story on the team’s JIRA board that links to the
-  pull request in question. In the story description, include any relevant
-  information about which contributor organization is behind the pull request,
-  any known political factors, etc. Tag the story as a contributor pull
-  request in whatever manner the team prefers: adding it to an epic, perhaps.
+The community manager should:
 
-* Inform the product owner of the team that there is a contributor pull request
-  that their team is responsible for reviewing, and send them a link to the
-  JIRA story that you’ve just created. Ask the product owner for an estimate
+* Make sure the pull request is ready for Product Review, if that has not yet happened.
+  That means getting enough detail out of the contributor for the product owner
+  to properly do a product review. Once this is done, move the JIRA ticket to the
+  "Product Review" state.
+
+* If questions arise from product owners during review, work with the contributor to
+  get those questions answered before the next round of review.
+
+* Once a PR has passed product review, do a first-round review of the PR with the
+  contributor. That is, make sure quality and test coverage is up to par, and that
+  the code generally meets our style guidelines. Once this has happened, move the
+  ticket to the "Awaiting Prioritization" state.
+
+* At each of these junctures, try to update the author with an estimate of how long
+  the next steps will take. The product team will meet biweekly to review new
+  proposals and prioritize PRs for team review. Direct the contributor to the JIRA ticket
+  as well; the state of the JIRA ticket reflect the above diagram and can give a good
+  sense of where in the process the pull request is.
+
+* Once a PR has been prioritized for team review, ask the product owner for an estimate
   of how many sprints it will take for the pull request to be reviewed:
   if its more than one, try to push back and advocate for the contributor.
   However, the estimate is ultimately up to the product owner, and if he/she
@@ -105,9 +120,8 @@ Scrum process. The community manager should:
   is queued to be reviewed. Give them an estimate of when the pull request
   will be reviewed: if you’re not sure what to say, tell them it will be in
   two weeks. If the product owner has estimated that it will take more than
-  one sprint before the pull request can be reviewed, tag the product owner
-  in the comment so that they will be notified of further comments on
-  the pull request.
+  one sprint before the pull request can be reviewed, direct the contributor to
+  JIRA to monitor progress.
 
 For determining which teams that the pull request impacts, use common sense --
 but in addition, there are a few guidelines:
@@ -124,7 +138,7 @@ but in addition, there are a few guidelines:
 * If any logging events are modified,
   include the analytics team.
 
-* Include the doc team on every contributor pull request.
+* Include the doc team on every contributor pull request that has a user-facing change.
 
 Once the code review process has started, the community managers are also
 responsible for keeping the pull request unblocked during the review process. If

docs/en_us/developers/source/process/contributor.rst

@@ -15,17 +15,36 @@ track -- before you spend a lot of time and effort making a pull request.
 .. _chat on the IRC channel: http://webchat.freenode.net?channels=edx-code
 .. _open an issue in our JIRA issue tracker: https://openedx.atlassian.net
 
+If you've got an idea for a new feature or new functionality for an existing feature,
+and wish to contribute your code upstream, please `start a discussion on JIRA`_
+(you may first need to `create a free JIRA account`_).
+Do this by visiting the JIRA website and clicking the "Create" button at the top.
+Choose the project "Open Source Pull Requests" and the issue type "Feature Proposal";
+in the description give us as much detail as you can for the feature or functionality
+you are thinking about implementing. We encourage you to do this before
+you begin implementing your feature, in order to get valuable feedback from the edX
+product team early on in your journey and increase the likelihood of a successful
+pull request.
+
+.. _start a discussion on JIRA: https://openedx.atlassian.net/secure/Dashboard.jspa
+.. _create a free JIRA account: https://openedx.atlassian.net/admin/users/sign-up
+
 It’s also sometimes useful to submit a pull request even before the code is
 working properly, to make it easier to collect early feedback. To indicate to
 others that your pull request is not yet in a functional state, just prefix the
-pull request title with "(WIP)" (which stands for Work In Progress).
+pull request title with "(WIP)" (which stands for Work In Progress). Please do
+include a link to a WIP pull request in your JIRA ticket, if you have one.
 
 Once you’re ready to submit your changes in a pull request, check the following
 list of requirements to be sure that your pull request is ready to be reviewed:
 
+#. Prepare a :doc:`pull request cover letter <cover-letter>`. When you open
+   up your pull request, put your cover letter into the "Description" field on Github.
+
 #. The code should be clear and understandable.
    Comments in code, detailed docstrings, and good variable naming conventions
-   are expected.
+   are expected. The `edx-platform Github wiki`_ contains many great links to
+   style guides for Python, Javascript, and internationalization (i18n) conventions.
 
 #. The pull request should be as small as possible.
    Each pull request should encompass only one idea: one bugfix, one feature,
@@ -37,7 +56,7 @@ list of requirements to be sure that your pull request is ready to be reviewed:
    "Fixup" commits should be squashed together. The best pull requests contain
    only a single, logical change -- which means only a single, logical commit.
 
-#. All code in the pull request must be compatible with edX’s AGPL license.
+#. All code in the pull request must be compatible with edX's AGPL license.
    This means that the author of the pull request must sign a `contributor's
    agreement with edX`_, and all libraries included or referenced in
    the pull request must have `compatible licenses`_.
@@ -49,13 +68,16 @@ list of requirements to be sure that your pull request is ready to be reviewed:
    (edX’s continuous integration server will verify this for your pull request,
    and point out any failing tests.)
 
-#. The author of the pull request should provide a test plan for verifying
+#. The author of the pull request should provide a test plan for manually verifying
    the change in this pull request. The test plan should include details
    of what should be checked, how to check it, and what the correct behavior
-   should be.
+   should be. When it makes sense to do so, a good test plan includes a tarball
+   of a small edX test course that has a unit which triggers the bug or illustrates
+   the new feature.
 
 #. For pull requests that make changes to the user interface,
-   it’s very helpful if you can include screenshots of what you changed.
+   please include screenshots of what you changed. Github will allow
+   you to upload images directly from your computer.
    In the future, the core committers will produce a style guide that
    contains more requirements around how pages should appear and how
    front-end code should be structured.
@@ -95,6 +117,24 @@ if we do reject your pull request, we will explain why we aren’t taking it, an
 try to suggest other ways that you can accomplish the same result in a way that
 we will accept.
 
+Once A PR is Open
+-----------------
+
+Once a pull request is open, our faithful robot "Botbro" will open up a JIRA ticket
+in our system to track review of your pull request. The JIRA ticket is a way for
+non-engineers (particularly, product owners) to understand your change and prioritize
+your pull request for team review.
+
+If you open up your pull request with a solid description, following the
+:doc:`pull request cover letter <cover-letter>` guidelines, the product owners will be able
+to quickly understand your change and prioritize it for review. However, they may have
+some questions about your intention, need, and/or approach that they will ask about
+on the JIRA ticket. A community manager will ping you on Github to clarify these questions if
+they arise; you are not required to monitor the JIRA discussion.
+
+Once the product team has sent your pull request to the engineering teams for review, all
+technical discussion regarding your change will occur on Github, inline with your code.
+
 Further Information
 -------------------
 For futher information on the pull request requirements, please see the following
@@ -107,5 +147,6 @@ links:
 * `Python Guidelines <https://github.com/edx/edx-platform/wiki/Python-Guidelines>`_
 * `Javascript Guidelines <https://github.com/edx/edx-platform/wiki/Javascript-Guidelines>`_
 
+.. _edx-platform Github wiki: https://github.com/edx/edx-platform/wiki#development
 .. _contributor's agreement with edX: http://code.edx.org/individual-contributor-agreement.pdf
 .. _compatible licenses: https://github.com/edx/edx-platform/wiki/Licensing

docs/en_us/developers/source/process/core-committer.rst

@@ -49,8 +49,8 @@ many contributor pull request reviews they want to commit to.
 Once a pull request from a contributor passes all required code reviews, a core
 committer will need to merge the pull request into the project. The core
 committer who merges the pull request will be responsible for verifying those
-changes on the staging server prior to release, using the test plan provided by
-the author of the pull request.
+changes on the staging server prior to release, using the manual test plan provided
+by the author of the pull request.
 
 In addition to reviewing contributor requests as part of sprint work, core
 committers should expect to spend about one hour per week doing other tasks

docs/en_us/developers/source/process/cover-letter.rst

@@ -0,0 +1,58 @@
+*************************
+Pull Request Cover Letter
+*************************
+
+When opening up a pull request, please prepare a "cover letter" to place into
+the "Description" field on Github. A good cover letter concisely answers as
+many of the following questions as possible. Not all pull requests will have
+answers to every one of these questions, which is okay!
+
+* What JIRA ticket does this address (if any)? Please provide a link to the JIRA ticket
+  representing the bug you are fixing or the feature discussion you've already
+  had with the edX product owners.
+
+* Who have you talked to at edX about this work? Design, architecture, previous PRs,
+  course project manager, IRC, mailing list, etc. Please include links to relevant
+  discussions.
+
+* Why do you need this change? It's important for us to understand what problem your
+  change is trying to solve, so please describe fully why you feel this change is needed.
+
+* What components are affected? (LMS, Studio, a specific app in the system, etc)
+
+* What users are affected?  For example, is this a new component intended for use
+  in just one course, or is this a system wide change affecting all edX students?
+
+* Test instructions for manual testing. When it makes sense to do so, a good test
+  plan includes a tarball of a small test course that has a unit which triggers
+  the bug or illustrates the new feature. Another option would be to provide
+  explicit, numbered steps (ideally with screenshots!) to walk the reviewer
+  through your feature or fix.
+
+* Please provide screenshots for all user-facing changes.
+
+* Indicate the urgency of your request. If this is a pull request for a course
+  running or about to run on edx.org, we need to understand your time constraints.
+  Good pieces of information to provide are the course(s) that need this feature
+  and the date that the feature needed by.
+
+* What are your concerns (the author’s) about the PR? Is there a corner case you
+  don't know how to address or some tests you aren't sure how to add? Please bring
+  these concerns up in your cover letter so we can help!
+
+
+Example Of A Good PR Cover Letter
+---------------------------------
+
+`Pull Request 4675`_ is one of the first edX pull requests to include a cover
+letter, and it is great! It clearly explains what the bug is, what system is
+affected (just the LMS), includes a tarball of a course that demonstrates the
+issue, and provides clear manual testing instructions.
+
+`Pull Request 4983`_ is another great example. This pull request's cover letter
+includes before and after screenshots, so the UX team can quickly understand
+what changes were made and make suggestions. Further, the pull request indicates
+how to manually test the feature and what date it is needed by.
+
+.. _Pull Request 4675: https://github.com/edx/edx-platform/pull/4675
+.. _Pull Request 4983: https://github.com/edx/edx-platform/pull/4983

docs/en_us/developers/source/process/index.rst

@@ -8,8 +8,8 @@ Contributing to Open edX
     :maxdepth: 2
 
     overview
-    core-committer
-    product-owner
-    community-manager
     contributor
-    
+    cover-letter
+    community-manager
+    product-owner
+    core-committer

docs/en_us/developers/source/process/overview.rst

@@ -30,10 +30,10 @@ different jobs and responsibilities:
     Prioritizes the work of core committers.
 
 :doc:`community-manager`
-    helps keep the community healthy and working smoothly.
+    Helps keep the community healthy and working smoothly.
 
 :doc:`contributor`
-    submits pull requests for eventual committing to an Open edX repository.
+    Submits pull requests for eventual committing to an Open edX repository.
 
 .. note::
   At the moment, developers who work for edX are core committers, and other
@@ -49,10 +49,13 @@ Overview
 If you are a :doc:`contributor <contributor>` submitting a pull request, expect that it will
 take a few weeks before it can be merged. The earlier you can start talking
 with the rest of the Open edX community about the changes you want to make,
-before you even start changing code if possible, the better the whole process
-will go.  Follow the guidelines in the document for a high-quality pull
-request: include a detailed description, keep the code clear and readable,
-make sure the tests pass, be responsive to code review comments.
+before you even start changing code, the better the whole process
+will go. 
+
+Follow the guidelines in this document for a high-quality pull request: include a detailed
+description of your pull request when you open it on Github (we recommend using a
+:doc:`pull request cover letter <cover-letter>` to guide your description),
+keep the code clear and readable, make sure the tests pass, be responsive to code review comments.
 Small pull requests are easier to review than large pull requests, so
 split up your changes into several small pull requests when possible --
 it will make everything go faster.  See the full :doc:`contributor guidelines <contributor>`

docs/en_us/developers/source/process/product-owner.rst

@@ -2,11 +2,25 @@
 Product Owner
 *************
 
-The product owner is responsible for prioritizing pull requests from
-contributors, and keeping them informed when prioritization slips. The community
-managers will inform product owners of incoming pull requests that are relevant
-to their team. For every sprint, each incoming pull request should either be
-included in the sprint as a commitment to get the pull request reviewed, or the
+The product owner has two main responsibilities: approving user-facing features
+and improvements from a product point of view, and prioritizing pull request
+reviews.
+
+When a contributor is interested in developing a new feature, or enhancing
+an existing one, they can engage in a dialogue with the product team about
+the feature: why it is needed, what does it do, etc. Product owners are expected
+to fully engage in this process and treat contributors like customers. If
+the idea is good but the implementation idea is poor, direct them to a better
+solution. If the feature is not something we can support at this time, provide
+a detailed explanation of why that is.
+
+A product owner is responsible for prioritizing pull requests from
+contributors, and keeping them informed when prioritization slips. Pull
+requests that are ready to be prioritized in the next sprint will have a
+"Awaiting Prioritization" label on their JIRA review tickets. At every
+product review meeting (which should happen each sprint), pull requests awaiting
+prioritization should either be included in the sprint for the appropriate team
+as a commitment to get the pull request reviewed, or the
 product owner must inform the author of the pull request that the pull request
 is still queued and is not being ignored. Contributors should be treated as
 customers, and if their pull requests are delayed then they should be informed