In Part 1 we talked about why code review and what to do before a code review, in Part 2 we’ll talk about some common gotchas in Django that a thorough code review can prevent. 

Things to look out for

Models

• Have you checked the behavior of cascading delete? For example, if you have a Blog model and some Media models attached to it, should they also be deleted? Maybe you want those Media models saved on some gallery page?
• Have you looked at the feature spec, maybe a signal needs to be fired on some CRUD step?
• Are the relations as simple as possible? Maybe there’s an extra model setup that just stores one CharField… this entirely new model may not be needed if that 1 field could exist on the original model.

Migrations

• Were any new migrations added since this pull request was created? I made a simple script that should catch any new migrations that were added, if you set this as your pre/post merge git hook:

new_migrations=$(git diff-tree --name-only -r ORIG_HEAD HEAD | grep migrations/ | awk -v P="$(pwd)/" '{print P $0}')
if [ -n "$new_migrations" ]; then
    sub $new_migrations
fi

• Would a data migration be helpful, or is it needed? For example, if you add some ‘ranking’ field to a model that requires complex computations then it may be a good idea to “prime” the database by generating these ‘rankings’ in a data migration.

ORM

• Check the spec, are any Model.object.filter calls filtering on the wrong stuff? It’s not uncommon that with local data the filters are working fine, but with more complete staging/production data the filters are too broad.
• Look out for any ‘gets’ or ‘filters’ that might work in 99.9% of cases but fail in one spot, really don’t want the end-user getting 500 errors!
• A good alternative is using the shortcut ‘get_object_or_404.’

Views

• As mentioned in the pre-code review steps, views should at least be smoke tested (bare minimum test). Example:

def test_index_returns_200(self):
    resp = self.app.get(“/”)
    assert resp.status_code == 200

• Are there any special permissions required for this view? They should be included in the smoke tests, at least.
• Think about class based view vs functional view. Could a view be so much more simple if it were a more explicit functional view? Maybe this view has behavior that is included on multiple views so it should be a decorator, at least, or work well as a CBV.
• Make sure you’re handling unicode properly, you can use the Django smart_string  function in most cases.
• Maybe the view is doing too much, move some/all behavior to model methods.

Celery tasks

• Make sure when you work with objects in tasks that you pass objects via PK instead of a pickled object. When you end up finally processing a task, the original object may have become stale. For example, you could have a file processing task but when the task is finally processed the user has changed their mind and deleted the original file!

Syntax

• Problems with syntax should be caught by a linter, it’s a waste of everyone’s time if code styling problems need to be caught by a code reviewer! I don’t mean problems like maybe a list comprehension that is too complex, those things should definitely be discussed. However, you shouldn’t have to remind another dev to put 2 spaces between classes in Python or a space between () and { in JS.

Tests

• Should be named test__
• Ideally each tests has only one assertion, but that’s ridiculous sometimes for DRY. Sometimes it makes sense to assert ~10 conditions in a test, but it’s generally better to have each test cover one specific thing. That way when problems do come up, you can see just via test names what the problem is.
• Strive for entropy: unicode, random values, empty values, non existent values, malformed
• Sometimes on large PRs useful to take notes while code reviewing to make sure important functionality is tested. For example, whenever I see a model method I write it down on a checklist and take notes about what should be tested about it. Same thing with views, especially if the view has some security requirements.

Thanks for reading, please let us know in the comments if you have any suggestions or other gotchyas in Django to look out for!