Merge pull request #3434 from edx/db/dev-process

Document our development/review process

CONTRIBUTING.rst

@@ -4,9 +4,11 @@ Contributing to edx-platform
 
 Contributions to edx-platform are very welcome, and strongly encouraged! The
 easiest way is to fork the repo and then make a pull request from your fork.
-Read on for details on how to become a contributor, edx-platform code quality,
-testing, making a pull request, and more.
+Check out our `process documentation`_, or read on for details on how to
+become a contributor, edx-platform code quality, testing, making a pull
+request, and more.
 
+.. _process documentation: https://github.com/edx/edx-platform/blob/master/docs/en_us/developers/source/process/index.rst
 
 Becoming a Contributor
 ======================

docs/en_us/developers/source/index.rst

@@ -17,6 +17,7 @@ Contents:
     change_log
     overview.rst
     extending_platform/index
+    process/index
 
 APIs
 -----

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

+*****************
+Community Manager
+*****************
+
+Community managers handle the first part of the process of responding to pull
+requests, before they are reviewed by core committers. Community managers are
+responsible for monitoring the Github project so that they are aware of incoming
+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.
+
+#. Evaluate the idea behind the pull request. Is this something that
+   Open edX wants? Discuss with product owners as necessary. 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
+   and run their own fork on their own servers, without needing permission
+   from edX. Try to suggest ways that they can build something that Open edX
+   *does* want: for example, perhaps an API that would allow the contributor
+   to build their own component separately. Then close the pull request.
+
+#. 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.
+
+#. Once you’ve verified that the code change is not malicious,
+   run a Jenkins job on the pull request and check the result.
+   If there are failing tests (and they are real failures, not flaky tests),
+   inform the author that the pull request cannot be reviewed until the tests
+   are passing.
+
+#. When all the tests pass, check the diff coverage and diff quality.
+   If they are too low, inform the author of how to check these metrics,
+   and ask the author to write unit tests to increase coverage and quality
+   Diff quality should be 100%, and diff coverage should be at least 95% unless
+   there are exceptional circumstances.
+
+#. Skim the contents of the pull request and suggest obvious fixes/improvements
+   to the pull request. Note that this is *not* a thorough code review --
+   this is simply to catch obvious issues and low-hanging fruit.
+   The point is to avoid interrupting core committers for trivial issues.
+
+#. Ask the author of the pull request for a test plan:
+   once this code is merged, how can we test that it’s working properly?
+   Whichever core committer merges this pull request will need to test it
+   on a staging server before the code is deployed to production, so be sure
+   that the test plan is clear enough for a core committer to follow.
+
+#. If the PR includes any visual changes, or changes in user interaction,
+   ask the author of the pull request to provide some screenshots.
+   (For interaction changes, GIFs are awesome!) When a core committer starts
+   reviewing the changes, it is often helpful to deploy the pull request to a
+   sandbox server, so that the reviewer can click around and verify that the
+   changes look good.
+
+#. The core committers will put together a style guide.
+   Pull requests that have visual/UX changes will be expected to respect this
+   style guide -- if they don’t, point the author to the style guide and tell
+   them to resubmit the pull request when it does.
+
+.. _contributor's agreement: http://code.edx.org/individual-contributor-agreement.pdf
+
+At this point, the pull request is ready for code review. There are two
+different options: small PR review and large PR review. A PR is “small” if it
+can be read and understood in less than 15 minutes, including time spent
+context-switching, reading the description of the pull request, reading any
+necessary code context, etc. Typically, “small” PRs consist of fixing typos,
+improving documentation, adding comments, changing strings to unicode, marking
+strings that need to be translated, adding tests, and other chores. A “small”
+pull request doesn’t modify the code that will be run in production in any
+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.
+
+If the pull request is not small, it will be handled by the core contributor
+Scrum process. The community manager should:
+
+* Determine which teams of core committers this pull request impacts.
+  At least one developer from each of these teams should review this pull request.
+
+* 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.
+
+* 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
+  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
+  says it will really be more than one sprint, respect that.
+
+* Add a comment to the pull request and inform the author that the pull request
+  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.
+
+For determining which teams that the pull request impacts, use common sense --
+but in addition, there are a few guidelines:
+
+* If any SASS files are modified, or any HTML in templates,
+  include the UX (user experience) team.
+
+* If any settings files or requirements files are modified,
+  include the devops team.
+
+* If any XModules are modified,
+  include the blades team.
+
+* If any logging events are modified,
+  include the analytics team.
+
+* Include the doc team on every contributor pull request.
+
+Once the code review process has started, the community managers are also
+responsible for keeping the pull request unblocked during the review process. If
+a pull request has been waiting on a core committer for a few days, a community
+manager should remind the core committer to re-review the pull request. If a
+pull request has been waiting on a contributor for a few days, a community
+manager should add a comment to the pull request, informing the contributor that
+if they want the pull request merged, they need to address the review comments.
+If the contributor still has not responded after a few more days, a community
+manager should close the pull request. Note that if a contributor adds a comment
+saying something along the lines of “I can’t do this right now, but I’ll come
+back to it in X amount of time”, that’s fine, and the PR can remain open -- but
+a community manager should come back after X amount of time, and if the PR still
+hasn’t been addressed, he or she should warn the contributor again.

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

+***********
+Contributor
+***********
+
+Before you make a pull request, it’s a good idea to reach out to the edX
+developers and the rest of the Open edX community to discuss your ideas. There
+might well be someone else already working on the same change you want to make,
+and it’s much better to collaborate than to submit incompatible pull requests.
+You can `send an email to the mailing list`_, `chat on the IRC channel`_, or
+open an issue on our Github issue tracker. (We prefer email or IRC rather than
+Github issues.) The earlier you start the conversation, the easier it will be to
+make sure that everyone’s on the right track -- before you spend a lot of time
+and effort making a pull request.
+
+.. _send an email to the mailing list: https://groups.google.com/forum/#!forum/edx-code
+.. _chat on the IRC channel: http://webchat.freenode.net?channels=edx-code
+
+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).
+
+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:
+
+#. The code should be clear and understandable.
+   Comments in code, detailed docstrings, and good variable naming conventions
+   are expected.
+
+#. The pull request should be as small as possible.
+   Each pull request should encompass only one idea: one bugfix, one feature,
+   etc. Multiple features (or multiple bugfixes) should not be bundled into
+   one pull request. A handful of small pull requests is much better than
+   one large pull request.
+
+#. Structure your pull request into logical commits.
+   "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.
+   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`_.
+
+#. All of the tests must pass.
+   If a pull request contains a new feature, it should also contain
+   new tests for that feature. If the pull request fixes a bug, it should
+   also contain a test for that bug to be sure that it stays fixed.
+   (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 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.
+
+#. For pull requests that make changes to the user interface,
+   it’s very helpful if you can include screenshots of what you changed.
+   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.
+
+#. The pull request should contain some documentation for the feature or bugfix,
+   either in a README file or in a comment on the pull request.
+   A well-written description for the pull request may be sufficient.
+
+#. The pull request should integrate with existing infrastructure as much
+   as possible, rather than reinventing the wheel.  In a project as large as
+   Open edX, there are many foundational components that might be hard to find,
+   but it is important not to duplicate functionality, even if small,
+   that already exists.
+
+#. The author of the pull request should be receptive to feedback and
+   constructive criticism.
+   The pull request will not be accepted until all feedback from reviewers
+   is addressed. Once a core committer has reviewed a pull request from a
+   contributor, no further review is required from the core committer until
+   the contributor has addressed all of the core committer’s feedback:
+   either making changes to the pull request, or adding another comment
+   explaining why the contributor has chosen not make any change
+   based on that feedback.
+
+It’s also important to realize that you and the core committers may have
+different ideas of what is important in the codebase. The power and freedom of
+open source software comes from the fact that you can fork our software and make
+any modifications that you like, without permission from us; however, the core
+committers are similarly empowered and free to decide what modifications to pull
+in from other contributors, and what not to pull in. While your code might work
+great for you on a small installation, it might not work as well on a large
+installation, have problems with performance or security, not be compatible with
+internationalization or accessibility guidelines, and so on. There are many,
+many reasons why the core committers may decide not to accept your pull request,
+even for reasons that are unrelated to the quality of your code change. However,
+if we do reject your pull request, we will explain why we aren’t taking it, and
+try to suggest other ways that you can accomplish the same result in a way that
+we will accept.
+
+.. _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

+**************
+Core Committer
+**************
+
+Core committers are responsible for doing code review on pull requests from
+contributors, once the pull request has passed through a community manager and
+been prioritized by a product owner. As much as possible, the code review
+process should be treated identically to the process of reviewing a pull request
+from another core committer: we’re all part of the same community. However,
+there are a few ways that the process is different:
+
+* The contributor cannot see when conflicts occur in the branch.
+  These conflicts prevent the pull request from being merged,
+  so you should ask the contributor to rebase their pull request,
+  and point them to `the documentation for doing so`_.
+
+* Jenkins may not run on the contributor’s pull request automatically.
+  Be sure to start new Jenkins jobs for the PR as necessary -- do not approve
+  a pull request unless Jenkins has run, and passed, on the last commit
+  in the pull request. If this contributor has already contributed a few
+  good pull requests, that contributor can be added to the Jenkins whitelist,
+  so that jobs are run automatically.
+
+* The contributor may not respond to comments in a timely manner.
+  This is not your concern: you can move on to other things while waiting.
+  If there is no response after a few days, a community manager will warn the
+  contributor that if the comments are not addressed, the pull request will
+  be closed. (You can also warn the contributor yourself, if you wish.)
+  Do not close the pull request merely because the contributor hasn’t responded
+  -- if you think the pull request should be closed, inform the
+  community managers, and they will handle it.
+
+.. _the documentation for doing so: https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request
+
+Each Scrum team should decide for themselves how to estimate stories related to
+reviewing external pull requests, and how to claim points for those stories,
+keeping in mind that an unresponsive contributor may block the story in ways
+that the team can’t control. When deciding how many contributor pull request
+reviews to commit to in the upcoming iteration, teams should plan to spend about
+two hours per week per developer on the team -- larger teams can plan to spend
+more time than smaller teams. For example, a team with two developers should plan
+to spend about four hours per week on pull request review, while a team with
+four developers should plan to spend about eight hours per week on pull request
+review -- these hours can be spread out among multiple developers, or one
+developer can do all the review for the whole team in that iteration.
+However, this is just a guideline: the teams can decide for themselves how
+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.
+
+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
+related to the open source community: reading/responding to questions on the
+mailing list and/or IRC channel, disseminating information about what edX is
+working on, and so on.

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

+###########################
+Contributing to Open edX
+###########################
+
+.. toctree::
+    :maxdepth: 2
+
+    overview
+    core-committer
+    product-owner
+    community-manager
+    contributor
+    
\ No newline at end of file

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

+*****************************
+Process for Contributing Code
+*****************************
+
+Open edX is a massive project, and we would love you to help us build
+the best online education system in the world -- we can’t do it alone!
+However, the core committers on the project are also developing features
+and creating pull requests, so we need to balance reviewing time with
+development time. To help manage our time and keep everyone as happy as
+possible, we’ve developed this document that explains what core committers
+and other contributors can expect from each other. The goals are:
+
+* Keep pull requests unblocked and flowing as much as possible,
+  while respecting developer time and product owner prioritization.
+* Maintain a high standard for code quality, while avoiding hurt feelings
+  as much as possible.
+
+Roles
+-----
+
+:doc:`core-committer`
+    Can commit changes to an Open edX repository.  Core committers are responsible for the quality of the code, and for supporting the code in the future.  Core committers are also developers in their own right.
+
+:doc:`product-owner`
+    Prioritizes the work of core committers.
+
+:doc:`community-manager`
+    helps keep the community healthy and working smoothly.
+
+:doc:`contributor`
+    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
+  developers are contributors. This may change in the future.
+
+Overview
+--------
+
+.. image:: pr-process.png
+   :align: center
+   :alt: A visualization of the pull request process
+
+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.
+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.
+
+If you are a :doc:`product owner <product-owner>`, treat pull requests
+from contributors like feature requests from a customer.
+Keep the lines of communication open -- if there are delays or unexpected
+problems, add a comment to the pull request informing the author of the
+pull request of what’s going on. No one likes to feel like they’re being ignored!
+
+If you are a :doc:`core committer <core-committer>`, allocate some time
+in every two-week sprint to review pull requests from other contributors.
+The community managers will make sure that these pull requests meet a
+basic standard for quality before asking you spend time reviewing them.
+
+Feel free to read the other documentation specific to each individual role in the
+process, but you don’t need to read everything to get started! If you're not
+sure where to start, check out the :doc:`contributor <contributor>` documentation. Thanks
+for helping us grow the project smoothly! :)

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

+*************
+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
+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
+of that, just as a product owner would inform any customer when that customer’s
+requests are delayed.