ChiliProject is not maintained anymore. Please be advised that there will be no more updates.

We do not recommend that you setup new ChiliProject instances and we urge all existing users to migrate their data to a maintained system, e.g. Redmine. We will provide a migration script later. In the meantime, you can use the instructions by Christian Daehn.

Development »

Code Review

When reviewing code to be included there are a few goals to keep in mind:

Code quality questions

  • does the code work?
  • is the code readable?
  • is the code better than the code around it?
  • are boundary conditions handled?
  • is nil handled?
  • does it follow MVC?
  • user input sanitized?
  • any potential security problems? Rails Security Guide
  • follow the Code Standards?

Testing questions

  • all of the tests pass?
  • new tests added?
  • existing tests changed?
  • using fixtures (bad) or factories (good) used?
  • do tests have a clear name? def test_this_thing is bad, test "User#activate should activate the user" is good.
  • edge cases tested? (See below)

Documentation questions

  • is there user documentation that needs to be created?
  • are there rdocs/yardocs for the code?

User interface

  • all user interface strings using the I18N system?
  • all labels attached to the form fields?
  • will the code work with JavaScript disabled? (this is a judgement call)
  • does the JavaScript work in the latest versions of Firefox, Chrome, Safari, and IE?

Compatibility

  • runs on Ruby 1.8.7 and 1.9.2?
  • runs on Rails 2.3.5?
  • change public internal APIs that haven't be deprecated yet? (e.g. changing User#current's behavior)
  • add deprecation warnings when changing change public internal APIs?
  • add or change any external dependencies? (judgement call, we don't want to write everything ourselves and should rely on stable and high quality third party code if it helps)

Testing edge cases

It's difficult to test all of the edge cases but check that the major ones are handled:

  • most common action
  • low boundary conditions (e.g. test 0 if a field should have a positive number)
  • high boundary conditions (e.g. test 99 if a field should have a two digit positive number)
  • just outside the boundary conditions (e.g. test -1 if a field should be positive)
  • nil/null/empty array/empty hash conditions

Also if the code is about user accounts, permissions, or authentication then all of the different user states need to be tested:

  • Anonymous user
  • Normal user, without project membership
  • Normal user, with project membership
  • Admin user, without project membership
  • Admin user, with project membership
  • Archived user

Code review process

  • TODO: github pull request review
  • TODO: issue attachment review
  • TODO: github forks patch review
  • TODO: upstream code reviews
  • TODO: security code reviews and audits