Fix a confusing crash creating a DiffCommit with no committer date.

Review Request #11317 — Created Dec. 7, 2020 and submitted

Information

Review Board
release-4.0.x

Reviewers

When creating a DiffCommit with committer_date=None, which could
happen for some commits, we'd see a confusing error:

TypeError: 'committer_date' is an invalid keyword argument for this
function

This was a bit confusing because, although committer_date is a
property and not a model field, it clearly works in most situations
(we've been creating multi-commit review requests for some time now).

The failure only manifests when passing None as the value, which led
to a crash in the property when trying to call a method on it. This
raised an AttributeError, which triggered a handler in Django's
BaseModel class, making it think that committer_date didn't exist.

This change fixes the property to work with None values. It also
applies the same logic to author_date, so that if a None is passed
in, we'll just set None in the fields, allowing Django's own
validation to occur.

Unit tests were added for both of these properties.

Unit tests pass.

Hit this in production use and verified the fix.

Summary ID
Fix a confusing crash creating a DiffCommit with no committer date.
When creating a `DiffCommit` with `committer_date=None`, which could happen for some commits, we'd see a confusing error: TypeError: 'committer_date' is an invalid keyword argument for this function This was a bit confusing because, although `committer_date` is a property and not a model field, it clearly works in most situations (we've been creating multi-commit review requests for some time now). The failure only manifests when passing `None` as the value, which led to a crash in the property when trying to call a method on it. This raised an `AttributeError`, which triggered a handler in Django's `BaseModel` class, making it think that `committer_date` didn't exist. This change fixes the property to work with `None` values. It also applies the same logic to `author_date`, so that if a `None` is passed in, we'll just set `None` in the fields, allowing Django's own validation to occur. Unit tests were added for both of these properties.
083e4ac1577706d2052fdcdbaf461917a374adda
Description From Last Updated

F841 local variable 'commit' is assigned to but never used

reviewbotreviewbot
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

david
  1. Ship It!
  2. 
      
chipx86
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (48bdb88)
Loading...