Don't overwrite modifications to ModificationTimestampField

Review Request #9816 — Created March 21, 2018 and submitted

Information

Djblets
release-1.0.x
a0aee27...

Reviewers

The ModificationTimestampField now tracks manual modifications to the
value so that if code manually sets a timestamp it will not be
overwritten during model save. This allows models to manage timestamps
manually in some cases without having to resort to mucking about with
save_base, etc.

Ran unit tests.

Description From Last Updated

There's also a bunch of unit tests in release-1.0.x that you're probably going to want to bring over. In fact, …

chipx86chipx86

A bunch of work was done prior to this change on release-1.0.x of Djblets to clean up documentation and such. …

chipx86chipx86

This does exactly the same thing as django.utils.timezone.now()

daviddavid

Can we add a blank line so this doesn't seem grouped with the comment about i18n?

daviddavid

typo: ustom

daviddavid

No blank line.

chipx86chipx86

This function isn't getting anything. It's returning something.

chipx86chipx86

, optional

chipx86chipx86

This has a lot of duplicate code paths. This would clear those up: existing_value = getattr(model, self.attnmae) if ((not add …

chipx86chipx86

This isn't the format we use for test class docstrings.

chipx86chipx86

Should be in alphabetical order.

chipx86chipx86

:py:meth:

chipx86chipx86

Same here.

chipx86chipx86

Can this be get_attr_name or maybe make_attr_name?

chipx86chipx86

Blank lines around this.

chipx86chipx86

This is in the wrong import group.

chipx86chipx86

You can just use timezone.utc. Actually, I think I pushed a change that already did this, so you'll just need …

chipx86chipx86

Does .objects.create() work for this, instead of doing a .save()?

chipx86chipx86

.objects.create() here and below.

chipx86chipx86
brennie
brennie
david
  1. 
      
  2. djblets/db/fields.py (Diff revision 2)
     
     
     
     
     
    Show all issues

    This does exactly the same thing as django.utils.timezone.now()

  3. 
      
brennie
david
  1. 
      
  2. tests/settings.py (Diff revision 3)
     
     
     
    Show all issues

    Can we add a blank line so this doesn't seem grouped with the comment about i18n?

  3. 
      
brennie
david
  1. 
      
  2. djblets/db/fields.py (Diff revision 4)
     
     
    Show all issues

    typo: ustom

  3. 
      
brennie
chipx86
  1. 
      
  2. Show all issues

    A bunch of work was done prior to this change on release-1.0.x of Djblets to clean up documentation and such. Rather than rewrite the docstrings again, can you pull in the exact text from that branch, to minimize conflicts?

  3. 
      
chipx86
  1. 
      
  2. Show all issues

    There's also a bunch of unit tests in release-1.0.x that you're probably going to want to bring over.

    In fact, what I'd suggest is having a precursor change that ports those unit tests to release-0.9.x and ports the docstrings. This change can then be based on those. As it is, it's right now going to be a bit of a mess trying to merge the branches without that work.

  3. djblets/db/fields.py (Diff revision 5)
     
     
     
     
    Show all issues

    No blank line.

  4. djblets/db/fields.py (Diff revision 5)
     
     
    Show all issues

    This function isn't getting anything. It's returning something.

  5. djblets/db/fields.py (Diff revision 5)
     
     
     
    Show all issues

    , optional

  6. djblets/db/fields.py (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    This has a lot of duplicate code paths. This would clear those up:

    existing_value = getattr(model, self.attnmae)
    
    if ((not add and not state.modified) or
        (add and existing_value is None)):
        value = timezone.now()
        setattr(model, self.attname, value)
    elif ((not add and state.modified) or
          (add and existing_value is not None)):
        value = super(...).pre_save(model, add)
    
  7. Show all issues

    This isn't the format we use for test class docstrings.

  8. 
      
brennie
chipx86
  1. 
      
  2. djblets/db/fields/modification_timestamp_field.py (Diff revision 6)
     
     
     
     
     
    Show all issues

    Should be in alphabetical order.

  3. Show all issues

    :py:meth:

  4. Show all issues

    Same here.

  5. Show all issues

    Can this be get_attr_name or maybe make_attr_name?

  6. Show all issues

    Blank lines around this.

  7. Show all issues

    This is in the wrong import group.

  8. Show all issues

    You can just use timezone.utc.

    Actually, I think I pushed a change that already did this, so you'll just need to update.

  9. Show all issues

    Does .objects.create() work for this, instead of doing a .save()?

  10. tests/settings.py (Diff revision 6)
     
     

    This was recently added as well, so you'll probably see this go away or conflict when you update.

  11. 
      
brennie
chipx86
  1. 
      
  2. Show all issues

    .objects.create() here and below.

  3. 
      
brennie
david
  1. Ship It!
  2. 
      
brennie
Review request changed
Status:
Completed
Change Summary:
Pushed to release-1.0.x (c409735)