Tool: Pyflakes
Processed Files:
    reviewboard/reviews/tests/test_review_request.py
    reviewboard/reviews/models/review_request.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/tests/test_review_request.py
    reviewboard/reviews/models/review_request.py

Why is the scmtools fixture needed, out of curiosity?

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/tests/test_review_request.py
    reviewboard/reviews/models/review_request.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/tests/test_review_request.py
    reviewboard/reviews/models/review_request.py

For a bit of background, ReviewRequest.get_last_activity() is actually used in review_details() in reviewboard/reviews/views.py. So, I am trying to understand the reasoning behind the changes. 
Some more detail about the changes would be appreciated in the description. Why are the changes required?
Correct me if I am wrong--are this changes required because the comparison of timestamps (eg. on line 505 and 517 of the original file) is not working? Thus, you're using string comparisons instead?

In the changes above, you are setting
timestamp = diffset_timestamp

and
timestamp = review_timestamp

but you never end up setting server_timestamp, which should hold the timestamp of the latest activity of the review request.
Right now, server_timestamp will always be set to self.last_updated regardless of whether there is a new diffset or review newer than self.last_updated.

Instead of converting all of these to a string and then comparing strings, how about just squashing out the microseconds part of the datetimes?

timestamp = server_timestamp.replace(microsecond=0)

...

diffset_timestamp = diffset.timestamp.replace(microsecond=0)
if diffset_timestamp >= timestamp:
   ...


This should be faster and more conceptually correct than comparing string representations.

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/tests/test_review_request.py
    reviewboard/reviews/models/review_request.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/tests/test_review_request.py
    reviewboard/reviews/models/review_request.py

You should also add a unit test to ensure that saving a review results in the proper timestamps on the review request.

This change is going to appear to fix the bug most of the time, but it will only appear that way. In practice, it's still broken.
What this change is doing is compensating for the case where review.timestamp's microsecond is, say, 100000, and review_request.last_updated is, say, 100001, and is a result of the review being saved. We're knocking the microseconds down to 0 so the review wins (and it wins because the review request's timestamp is the fallback, in the event that any other is newer or equal).
I say it only appears to fix it because there are still two cases where things will go wrong (one that is just a manifestation of the current bug, and another that is introduced by this change).
1) If review.timestamp is 999999 (or thereabouts) and review_request.last_updated is 1, the review request will still win. It's the same problem as before, but the difference isn't contained within a microsecond range of 0-999999. It overlapped into the next second. We'd have to then chase this into the seconds, knocking those to 0, but I think you'll agree that opens the door easily to false positives.

2) It's possible that the review request is newer than the review. If the review was posted at microsecond=1, and the review request was updated by another user at microsecond=999999, the review request should win! This is the same problem as the seconds issue I describe above, but in a smaller window of time (but on a busy server, this isn't unlikely).
This change is what we call a "bandaid." It's trying to address the symptom of the problem without solving the core issue. If you get cut by sharp splinter of wood every time you open a drawer, a bandaid might keep blood from a cut on your finger from dripping, but it doesn't do anything to prevent that splinter from cutting you or someone else next time. Ideally, it's best to solve that core problem so the bandaid isn't ever necessary.
What's really happening with this bug is that, at some point, we publish a review with a timestamp of "now," and then when we save the review request, we allow it to re-calculate what "now" is. What we probably should be doing is forcing the two to be in sync.
Here's where this is happening, in Review.publish:

# Update the last_updated timestamp and the last review activity
# timestamp on the review request.
self.review_request.last_review_activity_timestamp = self.timestamp
self.review_request.save(
    update_fields=['last_review_activity_timestamp', 'last_updated'])


last_updated is a special field that, left to its own devices, will update itself to "now" on save. You can see this code in djblets/db/fields.py (ModificationTimestampField). What needs to happen is we need to actually set last_updated the same way we're setting last_review_activity_timestamp, so the two are in sync, and bypass ModificationTimestampField's special settings of this.
The good news is, it's really easy to bypass the special "pre-save" logic that ModificationTimestampField is doing that's causing it to set the latest timestamp. We can use save_base(raw=True, ...) instead of save(...).
Model.save() does some special stuff to prepare a model for saving, and ReviewRequest.save() does some other special stuff. In this case, though, none of that special stuff matters, at all. We're only changing two timestamps, and this is safe and reliable and simple. So you can do:

self.review_request.last_updated = self.timestamp
self.review_request.save(
    raw=True,
    update_fields=[...])


With that, you shouldn't need any of the bandaids to adjust microseconds, and it should be reliable every time (unless of course some signal handler intercepts the save and makes other modifications, but we can't do anything about that really).
Give that a try and adjust the unit tests, and see if you can make the problem go away.

There should be one of these per update type/result, rather than one big test. That way, if one part of all this regresses later, it'll help in diagnosing things.
You're also going to want to manually insert timestamps for the review and review request in these tests. (The create_* methods you use here, which live in reviewboard/testing/testcase.py, will need to be updated to optionally take last_updated values). Unit tests that test timestamps without explicitly defining them often lead to their own time precision bugs.

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/tests/test_review.py
    reviewboard/reviews/tests/test_review_request.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/reviews/models/review_request.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/tests/test_review.py
    reviewboard/reviews/tests/test_review_request.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/reviews/models/review_request.py

I'm a little confused. I wrote the following but later noticed that you have unit tests written and you said that they passed. Is this because last_updated is updated somewhere else?
"In the docstring, you should be returning 'the last object updated, along with the timestamp of that object'.
On this line, you are returning server_timestamp and it is still assigned to self.last_updated."

I think your test could use a better name here. Perhaps something more specific like test_sync_of_review_timestamp_and_review_request_last_updated?

Why are you checking this twice?

You might want to put this into its own unit test.

Prior to this change, it was possible to call draft.publish() and have it kinda sorta do the right thing. It wasn't consistent, though, and posed a problem. Looking through the test suite, we have places where we're publishing the draft, and not the review request. If we're going to change any semantics and function arguments in this change, I'd like to address this once and for all.
I think what we should do is rename the existing publish() on drafts to something like apply_changes(). We may then want a draft.publish() that in turn calls review_request.publish() and returns the change description (matching the existing return type for the method).
This would help clarify what the publishing logic on the draft does, and help avoid problems if code (including existing extension code out in the wild) calls the wrong publish method.

Blank line before comment blocks.

So I think what I'd like to see actually is a change to ModificationTimestampField that prevents overriding custom values. It would entail tracking the loaded value and seeing if a new, explicit value was set. If so, it wouldn't override.
This would also let us avoid calling save() and then save_base(), reducing the number of database writes and avoiding incorrect state if something looks up the row in-between these writes.

Needs to be the full module path.

This sentence seems off.

This may not link correctly now.

Needs to be the full module path.

This can be grouped together.

Group together.

Group together.

Can you detail what manual testing you did to verify bug 3445?

This seeems... unrelated

Needs a period.

Do we still want to do this? Does the modification timestamp work fix this?
If not, the comment above needs to say exactly why we're doing this.

This should be undone. The order was right before.

, optional

Ship It!