Fixed review request submitted time being changed by new comments/activity

Review Request #9214 - Created Sept. 23, 2017 and submitted

Theodore Brockman
Review Board
master
4492
4f72040...
reviewboard, students

Closed review requests were having their "submitted at" timestamps updated by
subsequent comments on the request. This led to situations where requests that
had long since been submitted were updated and erroneously listed as having been
submitted at time of the most recent comment/activity. This error was caused
due to the "submitted at" section using the "last updated" field of the
review request.

The submitted fix involves re-writing the method get_close_description of
the review request model into a new method get_close_info, which returns a
python dictionary including the values returned by get_close_description,
as well as a timestamp indicating when the review request had been closed.
This timestamp was then used instead of the "last updated" timestamp for
submitted review requests. get_close_description was also re-written to
wrap the new get_close_info method, and now raises a deprecation
warning on use.

Manually tested bug by taking steps to reproduce bug after fix, which no longer
seems to cause the same issue after applying the patch.

Added a test to ensure that the review request member function get_close_info
returns a dictionary with the proper keys (including the added timestamp), and
that closing a review request does cause the timestamp to be created.

Also added a test to ensure that deprecated function get_close_description
now throws a deprecation warning.

  • 0
  • 0
  • 39
  • 1
  • 40
Description From Last Updated
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

Theodore Brockman
Theodore Brockman
Theodore Brockman
Christian Hammond
  1. 
      
  2. Make sure your description and testing corresponds to our guidelines. They need to wrap at the 70-75 character range and shouldn't reference bug IDs (that's what the Bugs field is for).

    You can use backticks for any function references, which will cause them to show up as string literals.

    You should also rework the first paragraph to state the problem (rather than saying it's fixed outright). Also no need to say "Per Christian's suggestion," because that's not really going to be relevant to the change. The description should focus on the why and how of what your change is doing.

  3. This old description doesn't really meet our standards, so we shouldn't bring it over for the new one. The summary shouldn't list any data types or anything, and should instead be higher-level.

    The description is also wrong. It's not amending get_close_description at all. Even if it did, it should stand alone and describe the purpose of this function.

    You'll also need to include a Returns: section.

    Grep around for "Returns" and "Args" for examples. We have a whole guide to writing documentation on Notion. Another mentor can link you to it.

  4. reviewboard/reviews/models/review_request.py (Diff revision 2)
     
     
     
     
     
     

    This should also be documented using the new standards now.

  5. Blank line before the description, or the docs won't render correctly.

    When referencing other methods, do:

    :py:meth:`get_close_info`
    

    (We can skip the class name because it's in the same class. Otherwise you'd include the class, or even the full module path if in a different module.)

  6. reviewboard/reviews/models/review_request.py (Diff revision 2)
     
     
     
     
     

    This is referencing the wrong path and the wrong replacement function. You should simply say ReviewRequest.get_close_description() and ReviewRequest.get_close_info().

  7. Blank line between these. Let the warning stand alone. It's its own "paragraph."

  8. This is a Python Standard Library module, so it should go in the first import list (excluding the __future__ group). Plain imports go before from ... import.

  9. "warns deprecation" doesn't sound right. How "causes a deprecation warning"

  10. Blank line between these. Block-creating statements (like with or if/else) should be separated from other statements.

  11. This is missing a docstring.

    It also should test the entirety of the feature, not just the timestamp.

  12. reviewboard/reviews/ui/base.py (Diff revision 2)
     
     
     
     

    You can use the dictionary item references directly below instead of pulling out into variables.

  13. reviewboard/reviews/views.py (Diff revision 2)
     
     
     
     

    These can all be put into the dictionary below without pulling them out into variables first.

  14. reviewboard/reviews/views.py (Diff revision 2)
     
     
     
     

    Same here.

  15. Can you call this closed_date_raw and closed_date?

    I also don't tihnk you want to use |date:"N d, Y, g:i a". We should be consistent with all existing date uses (which are just using |date here).

    1. Yep!

      I definitely didn't want to, but using the regular |date filter on closed_date strips the time information (for whatever reason?) turning the hover into this, so explicitly stating the format instead of using the default was a work-around.

  16. 
      
Barret Rennie
  1. 
      
  2. We also write docstrings in the imperitive mood. In this case it would be "Return" not "Returns".

  3. 
      
Theodore Brockman
Review request changed

Description:

~  

Fixed an issue (#4492) with closed review requests "submitted at" timestamp (actually using the "last updated" field) being updated by subsequent comments (and possibly other activity), resulting in review requests being listed as having been submitted/closed recently, when in actuality the submission had been closed further in the past.

  ~

Closed review requests were having their "submitted at" timestamps updated by

  + subsequent comments on the request. This led to situations where requests that
  + had been long since submitted were updated and erroneously listed as having been
  + submitted relative to the timestamp of the most recent comment/activity. This
  + error was caused due to the "submitted at" section using the "last updated"
  + field of the review request.

   
~  

Per Christian's suggestion, the fix involved re-writing the method "get_close_description" of the review request model into a new method "get_close_info", which returns a python dictionary including the values returned by "get_close_description", as well as a timestamp indicating when the review request had been closed. This timestamp was then used instead of the "last updated" timestamp for submitted review requests. "get_close_description" was also re-written to wrap the new "get_close_info" method, and now raises deprecation warnings on use.

  ~

The submitted fix involves re-writing the method get_close_description of

  + the review request model into a new method get_close_info, which returns a
  + python dictionary including the values returned by get_close_description,
  + as well as a timestamp indicating when the review request had been closed.
  + This timestamp was then used instead of the "last updated" timestamp for
  + submitted review requests. get_close_description was also re-written to
  + wrap the new get_close_info method, and now raises a deprecation
  + warning on use.

Testing Done:

~  

Manually tested bug by taking steps to reproduce bug after fix, which no longer seemed to cause the same issue.

  ~

Manually tested bug by taking steps to reproduce bug after fix, which no longer

  + seems to cause the same issue after applying the patch.

   
~  

Added a test to ensure that the review request member function "get_close_info" returns a dictionary with the proper keys (including the added timestamp), and that closing a review request does cause the timestamp to be created.

  ~

Added a test to ensure that the review request member function get_close_info

  + returns a dictionary with the proper keys (including the added timestamp), and
  + that closing a review request does cause the timestamp to be created.

   
~  

Also added a test to ensure that deprecated function "get_close_description" now throws a deprecation warning.

  ~

Also added a test to ensure that deprecated function get_close_description

  + now throws a deprecation warning.

Bugs:

+4492

Commit:

-6ab73c55e7f27083d3facea816f4c45268256bbd
+1be2cfe6df542934ca89bcb388d916274849910c

Diff:

Revision 3 (+87 -26)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Theodore Brockman
Theodore Brockman
Theodore Brockman
Barret Rennie
  1. A couple of style nits here

    Note that we use self.assertEqual(x, y) over self.assertTrue(x == y) becuase the former will generate better error message (e.g., showing where lists or dicts differ) instead of just saying False is not True.

  2. reviewboard/reviews/models/review_request.py (Diff revision 5)
     
     
     
     
     
     
     

    Instead of this, we can format as:

    A dictionary with the following keys:
    
    ``'close_description'`` (:py:class:`unicode`):
        description of close_description
    
    ``'is_rich_text'`` (:py:class:`bool`):
        description of is_rich_text
    
    ``'timestamp'`` (:py:class:`datetime.datetime`):
        description of timestamp
    

    This will cause it to be rendered as a definition list sort of like how we render our Args, etc. in the docs.

  3. reviewboard/reviews/models/review_request.py (Diff revision 5)
     
     
     
     

    Can you format this as:

    return {
        'key': value,
        # ...   
    }
    
  4. self.assertEqual(len(w), 1)

  5. If len(w) == 1, then w[-1] is w[0]. We should just pull out w[0].

  6. self.assertIn('deprecated', six.text_type(w[0].message)))

    We use six.text_type (from django.utils import six) becuase in Python 2, the text type is unicode (str represents binary data) and in Python 3 the text type is str (and bytes represents binary data).

  7. """ on next line.

  8. self.assertLess

  9. self.assertEqual

  10. 
      
Theodore Brockman
Barret Rennie
  1. One more nit. Pending the following issue being fixed, this change looks good to me!

  2. 
      
Theodore Brockman
Theodore Brockman
David Trowbridge
  1. 
      
  2. Can you add a unit test here that creates a review request, closes it, then adds a new review with a later timestamp? The test should verify that the timestamp returned by get_close_info is that of the close operation and not the review.

  3. We don't need to define lasteupdatedraw and lastupdated in here anymore.

  4. 
      
Theodore Brockman
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Theodore Brockman
Mike Conley
  1. This looks good to me! Thanks Theo!

  2. 
      
Theodore Brockman
  1. Ship It!

  2. 
      
Barret Rennie
  1. Two minor nits and then I'm happy with it!

  2. reviewboard/reviews/models/review_request.py (Diff revision 10)
     
     
     
     

    Can we reformat this as a list, à la:

    A 2-tuple of:
    
    * The close description (:py:class:`unicode`).
    * Whether or not the close description is rich text (:py:class:`bool`).
    
  3. reviewboard/reviews/tests/test_review_request.py (Diff revision 10)
     
     
     
     

    These don't need to be in the with block.

  4. 
      
Theodore Brockman
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Theodore Brockman
David Trowbridge
  1. Ship It!
  2. 
      
Theodore Brockman
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (0756e8c)
Loading...