Fix ReviewRequest.get_last_activity() to return the last update activity for a review request

Review Request #8800 - Created March 7, 2017 and updated

Anni Cao
Review Board
release-2.5.x
3445
2b7aac3...
reviewboard, students

ReviewRequest.get_last_activity() always returns the ReviewRequest itself. The bug is caused by timestamp discrepancy between ReviewRequest.last_updated and review.timestamp. There is a minor difference in millisecond between these two timestamps although they are logically supposed to be same. What's really happening is that, when we publish/save an update with a timestamp of "now," and then when we save the review request, we allow it to re-calculate what "now" is. The fix should be forcing the two to be in sync.
After change, ReviewRequest.get_last_activity() returns the last update activity for a review request.

Unit tests for following update type have been added and passed in ReviewRequestTests:
- Reviews
- Diffsets
- ChangeDescription

  • 0
  • 8
  • 2
  • 10
Description From Last Updated
Review Bot
Mike Conley
Anni Cao
Anni Cao
Review Bot
Simon Zhang
Anni Cao
David Trowbridge
Anni Cao
Review Bot
Anni Cao
Anni Cao
Christian Hammond
Anni Cao
Review Bot
Anni Cao
Anni Cao
Review request changed

Description:

~  

ReviewRequest.get_last_activity() always returns the ReviewRequest itself. The bug is caused by timestamp discrepancy between ReviewRequest.last_updated and review.timestamp. There is a minor difference in millisecond between these two timestamps although they are logically supposed to be same. I use replace(microsecond=0) to trim the microsecond part of the timestamps to remove the discrepancy.

  ~

ReviewRequest.get_last_activity() always returns the ReviewRequest itself. The bug is caused by timestamp discrepancy between ReviewRequest.last_updated and review.timestamp. There is a minor difference in millisecond between these two timestamps although they are logically supposed to be same. What's really happening is that, when we publish/save an update with a timestamp of "now," and then when we save the review request, we allow it to re-calculate what "now" is. The fix should be forcing the two to be in sync.

    After change, ReviewRequest.get_last_activity() returns the last update activity for a review request.

Simon Zhang
  1. 
      
  2. 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."

    1. That's the whole purpose of this function, review_request.last_updated should store the same timestamp of the last update object. Yes, the last_updated is updated whenever a Review is publish or some other update change happens.

  3. reviewboard/reviews/tests/test_review.py (Diff revision 4)
     
     
     
     

    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?

    1. Thanks for good suggestion

    2. But it might be a bit long, also the following comment gives the detail. Maybe something like "test_sync_of_last_updated_timestamp"

  4. reviewboard/reviews/tests/test_review.py (Diff revision 4)
     
     
     

    Why are you checking this twice?

    1. Just want to make sure that when the second review is published, the last_updated field will be updated to overwrite the first one.

  5. reviewboard/reviews/tests/test_review.py (Diff revision 4)
     
     
     
     
     
     

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

    1. I actually put them together on purpose to test if the last_updated will be updated when a new type of review is created and published

  6. 
      
Loading...