Skip to content

Latest commit

 

History

History
227 lines (175 loc) · 10.2 KB

CONTRIBUTING.md

File metadata and controls

227 lines (175 loc) · 10.2 KB
Raw

Welcome!

We're so glad you're thinking about contributing to an 18F open source project! If you're unsure about anything, just ask — or submit the issue or pull request anyway. The worst that can happen is you'll be politely asked to change something. We love all friendly contributions.

We want to ensure a welcoming environment for all of our projects. Our staff follow the 18F Code of Conduct and all contributors should do the same.

We encourage you to read this project's CONTRIBUTING policy (you are here), its LICENSE, and its README.

If you have any questions or want to read more, check out the 18F Open Source Policy GitHub repository, or just shoot us an email.

Pull request guidelines

Below are rules we strive to follow to achieve maintainable and consistent code.

Commit message style guide:

  • Write your commit message summary in the imperative: "Fix bug" and not "Fixed bug" or "Fixes bug." This convention matches up with commit messages generated by commands like git merge and git revert.

  • Under the summary, start by explaining why this change is necessary, and add details to help the person reviewing your code understand what your pull request is about.

  • If the pull request fixes a GitHub issue, mention it at the bottom using GitHub's syntax, such as Fixes #123.

Example:

Load seed using before(:suite) in RSpec config

**Why**:
- Loading the seed in a `before(:each)` block results in an unnecessary
database call before every single test, slowing down the test suite,
and making development less efficient.

**How**:
- Use `before(:suite)` instead, since the data that is loaded is not
meant to change, and so that only one database call is made.
- To prevent the data from being wiped out after each spec, configure
Database Cleaner to ignore those static tables.

Fixes #123

Note that we use Overcommit to enforce some of the commit message rules. If this is your first time contributing to this repo, you will need to sign your Overcommit configuration by running overcommit --sign before being able to run git commit. See the Security section in the Overcommit README for more details.

Style, Readability, and OO

RESTful controllers

  • Only use CRUD methods in controllers.

  • Prefer adding a new controller with one of the CRUD methods over creating a custom method in an existing controller. For example, if your app allows a user to update their email and their password on two different pages, instead of using a single controller with methods called update_email and update_password, create two controllers and name the methods update, i.e. EmailsController#update and PasswordsController#update. See http://jeromedalbert.com/how-dhh-organizes-his-rails-controllers/ for more about this design pattern.

Lean controllers

  • Keep as much business logic as possible out of controllers.

  • Use specialized classes to handle the operations

    • These will be Form Objects for the most part, since most of what the app does is process user input via a form submission, or clicking a link in email that contains a token.

  • Form Object rules:

    • Should have a single public method called submit that returns a FormResponse object.
    • Should use ActiveModel validations to validate the user input.
    • Should be placed in app/forms.

  • Examples of Form/Service Objects:

  • The basic outline of how a controller interacts with this class is:

result = Class.new(user).submit(params) # this returns a FormResponse object
# all the necessary analytics attributes are captured inside the Form Object
analytics.track_event('Some Event Name', result.to_h)

if result.success?
  handle_success
else
  handle_failure
end
  • Only make one call to analytics.track_event after submitting the form, as opposed to one call when handling success and another when handling failure. The Form Object, when used properly, will return a FormResponse object that already tells us whether the action was successful or not.

Importance of the controller design for analytics

This design pattern was the result of many iterations, and agreed upon by all team members in early 2017. It keeps controllers clean and predictable. Having a controller interact with a Form Object or some other specialized class is not a new concept. Rails developers have been using them since at least 2012. What might seem new is the FormResponse object. The most important reason controllers expect an object that responds to success? and to_h is to define an analytics API, or a contract, if you will, between the analytics logs and the folks who query them.

For example, if someone wants to look up all events that have failed, they would run this query in Kibana: properties.event_properties.success:false. Now let's say a developer introduces a new controller that doesn't adhere to our established convention, and captures analytics in their own way, without adding success and errors keys, which are expected to be included in all analytics events. This means that any failures for this controller won't show up when running the query above, and the person running the query might not realize data is missing.

Deviating from the convention also causes confusion. The next developer to join the team will not be sure which pattern to use, and might end up picking the wrong pattern. As Sandi Metz says:

For better or for worse, the patterns you establish today will be replicated forever. When the code lies you must be alert to programmers believing and then propagating that lie.

Secure controllers

Rails by default is currently vulnerable to cache poisoning attacks through modification of the X-Forwarded-For and Host headers. In order to protect against the latter, there are two pieces that must be in place. The first one is already taken care of by defining default_url_options in ApplicationController with a host value that we control.

The other one is up to you when adding or modifying redirects:

  • Always use _url helpers (as opposed to _path) when calling redirect_to in a controller.

Additional notes on pull requests and code reviews

Please follow our Code Review guidelines. Glen Sanford's thoughts on code reviews are also well worth reading.

  • Prioritize code reviews for the current sprint above your other work
  • Review pull requests for the current sprint within 24 hours of being opened
  • Keep pull requests as small as possible, and focused on a single topic
  • Once a pull request is good to go, the person who opened it squashes related commits together, merges it, then deletes the branch.

Recommended reading, viewing, and courses

Public domain

This project is in the public domain within the United States, and copyright and related rights in the work worldwide are waived through the CC0 1.0 Universal public domain dedication.

All contributions to this project will be released under the CC0 dedication. By submitting a pull request, you are agreeing to comply with this waiver of copyright interest.