Fix bug where ModificationTimestampField would not automatically update its value.

Review Request #13261 — Created Sept. 6, 2023 and submitted

Information

Djblets
release-4.x

Reviewers

As discussed in /r/13260, it was discovered that the
ModificationTimestampField had a bug where sometimes the field wouldn't
automatically update to a new value when it was supposed to. Review Board uses
this field for its "last updated" timestamp on review request and drafts, and
this timestamp sometimes wasn't being updated when we would save changes on a
review request or draft. Specifically, this would happen whenever we tried to
save changes on objects that were fetched through a model manager (i.e. objects
from Model.objects).

ModificationTimeStampField takes care to not overwrite manually set values by
tracking modification state in its __set__ method. If __set__ was called,
then we assume that the field value was manually set and we give the field a
modified state. But when an object is fetched through a manager, the
__set__ method for the field gets called when creating the object instance.
This makes the ModificationTimestampField think that the value has been
manually set, even though it hasn't been. Previously we were using a
first_set state to handle the case when __set__ is called during object
initialization, but this wasn't hooked up correctly and didn't actually work.
This change addresses the case by checking if the field value actually exists
in the object's attributes dictionary, and if not then we know that the field
is being set for the first time during object creation and its not being
manually set. We also add unit tests for this case.

  • Ran unit tests.
  • Ran unit tests (including my newly added ones) without the fix to
    ModificationTimestampField, saw that the cases where we fetch objects
    through a model manager and save those would not properly update the
    timestamp.
  • Used in a Review Board change.
Summary ID
Fix the ModificationTimestampField
b4c0892169016f91d9feec869ea910047b8eba52
Description From Last Updated

Can you add some more context in the summary?

chipx86chipx86

We can just check against __dict__, which is faster as it'll check by hash, whereas checking within .keys() will require …

chipx86chipx86

Can you add -> None: to new test methods, so we opt into typing?

chipx86chipx86
chipx86
  1. 
      
  2. Can you add some more context in the summary?

    1. The new description looks good, but the summary's a bit vague still. Can we fit in the condition/symptom that was fixed?

    2. Oh woops, totally missed that you were talking about the summary and not the description. I'll update the summary :).

  3. We can just check against __dict__, which is faster as it'll check by hash, whereas checking within .keys() will require iterating through all keys one-by-one.

  4. Can you add -> None: to new test methods, so we opt into typing?

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

Status: Closed (submitted)

Change Summary:

Pushed to release-4.x (76ded09)
Loading...