Summary

Don't overwrite modifications to ModificationTimestampField

Review Request #9816 — Created March 21, 2018 and submitted July 5, 2018, 4:30 p.m.

Information

Owner

brennie

Repository

Djblets

Branch

release-1.0.x

Bugs

Depends On

Blocks

8800

Commit

a0aee27...

Reviewers

Groups

djblets

People

Description

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.

Testing Done

Ran unit tests.

Issues

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, …	chipx86	April 18, 2018, 7:25 p.m.
A bunch of work was done prior to this change on release-1.0.x of Djblets to clean up documentation and such. …	chipx86	April 18, 2018, 7:25 p.m.
This does exactly the same thing as django.utils.timezone.now()	david	April 6, 2018, 4 p.m.
Can we add a blank line so this doesn't seem grouped with the comment about i18n?	david	April 6, 2018, 4:30 p.m.
typo: ustom	david	April 9, 2018, 2:41 p.m.
No blank line.	chipx86	April 18, 2018, 6:41 p.m.
This function isn't getting anything. It's returning something.	chipx86	April 18, 2018, 6:42 p.m.
, optional	chipx86	April 18, 2018, 6:42 p.m.
This has a lot of duplicate code paths. This would clear those up: existing_value = getattr(model, self.attnmae) if ((not add …	chipx86	April 18, 2018, 6:43 p.m.
This isn't the format we use for test class docstrings.	chipx86	April 18, 2018, 6:43 p.m.
Should be in alphabetical order.	chipx86	April 30, 2018, 2:57 p.m.
:py:meth:	chipx86	April 30, 2018, 2:59 p.m.
Same here.	chipx86	April 30, 2018, 2:59 p.m.
Can this be get_attr_name or maybe make_attr_name?	chipx86	April 30, 2018, 2:59 p.m.
Blank lines around this.	chipx86	April 30, 2018, 3 p.m.
This is in the wrong import group.	chipx86	April 30, 2018, 3 p.m.
You can just use timezone.utc. Actually, I think I pushed a change that already did this, so you'll just need …	chipx86	April 30, 2018, 3:02 p.m.
Does .objects.create() work for this, instead of doing a .save()?	chipx86	April 30, 2018, 3:03 p.m.
.objects.create() here and below.	chipx86	May 2, 2018, 3:55 p.m.

flake8 passed.

JSHint passed.

Description:

		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_raw` etc.
	~	`save_base`, etc.

Change Summary:

Simplify unit tests

Commit:

-	cc35cf8f53064dacd162d48d24b532081e2ef529
+	515acbc161447a74b8d23fd3b7743beb9048c60c

Diff:

Revision 2 (+341 -14)

Show changes

	djblets/db/fields.py
	djblets/db/tests/test_modification_timestamp_field.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

djblets/db/fields.py (Diff revision 2)

The issue has been resolved. Show all issues

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

Change Summary:

Addressed David's issues. Now set USE_TZ in tests.

Commit:

-	515acbc161447a74b8d23fd3b7743beb9048c60c
+	357f2a52319c8110e2ec094dcf791787acbe0c60

Diff:

Revision 3 (+323 -17)

Show changes

	djblets/db/fields.py
	djblets/db/tests/test_modification_timestamp_field.py
	tests/settings.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

tests/settings.py (Diff revision 3)

The issue has been resolved. Show all issues

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

Commit:

-	357f2a52319c8110e2ec094dcf791787acbe0c60
+	9ede2d0b7fd3ae7ce1e845925743d81d96a8e162

Diff:

Revision 4 (+324 -17)

Show changes

	djblets/db/fields.py
	djblets/db/tests/test_modification_timestamp_field.py
	tests/settings.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

djblets/db/fields.py (Diff revision 4)
The issue has been resolved. Show all issues
```
typo: ustom
```

Commit:

-	9ede2d0b7fd3ae7ce1e845925743d81d96a8e162
+	28c4496226b4e4c5fb5682623f2453dce548fb2a

Diff:

Revision 5 (+324 -17)

Show changes

	djblets/db/fields.py
	djblets/db/tests/test_modification_timestamp_field.py
	tests/settings.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been resolved. 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?

The issue has been resolved. 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.

djblets/db/fields.py (Diff revision 5)
The issue has been resolved. Show all issues
```
No blank line.
```
djblets/db/fields.py (Diff revision 5)
The issue has been resolved. Show all issues
```
This function isn't getting anything. It's returning something.
```
djblets/db/fields.py (Diff revision 5)
The issue has been resolved. Show all issues
```
, optional
```

djblets/db/fields.py (Diff revision 5)

The issue has been resolved. 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)

djblets/db/tests/test_modification_timestamp_field.py (Diff revision 5)
The issue has been resolved. Show all issues
```
This isn't the format we use for test class docstrings.
```

Change Summary:

Migrated to release-1.0.x

Branch:

-	release-0.9.x
+	release-1.0.x

Commit:

-	28c4496226b4e4c5fb5682623f2453dce548fb2a
+	3f65633cf7379a216d23ceb20d5de62a662b971a

Diff:

Revision 6 (+258 -7)

Show changes

	djblets/db/fields/modification_timestamp_field.py
	djblets/db/tests/test_modification_timestamp_field.py
	tests/settings.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

djblets/db/fields/modification_timestamp_field.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Should be in alphabetical order.
```
djblets/db/fields/modification_timestamp_field.py (Diff revision 6)
The issue has been dropped. Show all issues
```
:py:meth:
```
djblets/db/fields/modification_timestamp_field.py (Diff revision 6)
The issue has been dropped. Show all issues
```
Same here.
```
djblets/db/fields/modification_timestamp_field.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Can this be get_attr_name or maybe make_attr_name?
```
djblets/db/fields/modification_timestamp_field.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Blank lines around this.
```
djblets/db/tests/test_modification_timestamp_field.py (Diff revision 6)
The issue has been resolved. Show all issues
```
This is in the wrong import group.
```

djblets/db/tests/test_modification_timestamp_field.py (Diff revision 6)

The issue has been resolved. 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.

djblets/db/tests/test_modification_timestamp_field.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Does .objects.create() work for this, instead of doing a .save()?
```

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.

Change Summary:

Addressed Christian's feedback.

Commit:

-	3f65633cf7379a216d23ceb20d5de62a662b971a
+	a91a504b00abe5b339258d8948b1d30c278381b3

Diff:

Revision 7 (+253 -4)

Show changes

	djblets/db/fields/modification_timestamp_field.py
	djblets/db/tests/test_modification_timestamp_field.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

djblets/db/tests/test_modification_timestamp_field.py (Diff revision 7)
The issue has been resolved. Show all issues
```
.objects.create() here and below.
```

Change Summary:

Addressed Christian's feedback.

Commit:

-	a91a504b00abe5b339258d8948b1d30c278381b3
+	a0aee27f8579f4261ad34f5f6f6a82c4fed313ba

Diff:

Revision 8 (+250 -4)

Show changes

	djblets/db/fields/modification_timestamp_field.py
	djblets/db/tests/test_modification_timestamp_field.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-1.0.x (c409735)