• 
      

    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:
    Completed
    Change Summary:
    Pushed to release-4.0.x (48bdb88)