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

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

Information

Review Board
master
4f72040...

Reviewers

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.

Description From Last Updated

Make sure your description and testing corresponds to our guidelines. They need to wrap at the 70-75 character range and …

chipx86chipx86

E501 line too long (93 > 79 characters)

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E501 line too long (88 > 79 characters)

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

F821 undefined name 'review_request'

reviewbotreviewbot

This old description doesn't really meet our standards, so we shouldn't bring it over for the new one. The summary …

chipx86chipx86

We also write docstrings in the imperitive mood. In this case it would be "Return" not "Returns".

brenniebrennie

This should also be documented using the new standards now.

chipx86chipx86

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 …

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

This is a Python Standard Library module, so it should go in the first import list (excluding the __future__ group). …

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

This is missing a docstring. It also should test the entirety of the feature, not just the timestamp.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Same here.

chipx86chipx86

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". …

chipx86chipx86

E128 continuation line under-indented for visual indent

reviewbotreviewbot

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`): …

brenniebrennie

Can you format this as: return { 'key': value, # ... }

brenniebrennie

self.assertEqual(len(w), 1)

brenniebrennie

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

brenniebrennie

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 …

brenniebrennie

""" on next line.

brenniebrennie

self.assertIn

brenniebrennie

self.assertLess

brenniebrennie

self.assertIn

brenniebrennie

self.assertEqual

brenniebrennie

self.assertIn

brenniebrennie

Undo this.

brenniebrennie

Can you add a unit test here that creates a review request, closes it, then adds a new review with …

daviddavid

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

daviddavid

E501 line too long (83 > 79 characters)

reviewbotreviewbot

Can we reformat this as a list, à la: A 2-tuple of: * The close description (:py:class:`unicode`). * Whether or …

brenniebrennie

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

brenniebrennie

E501 line too long (82 > 79 characters)

reviewbotreviewbot
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

TB
TB
TB
chipx86
  1. 
      
  2. Show all issues

    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. Show all issues

    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)
     
     
     
     
     
     
    Show all issues

    This should also be documented using the new standards now.

  5. Show all issues

    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)
     
     
     
     
     
    Show all issues

    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. Show all issues

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

  8. Show all issues

    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. Show all issues

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

  10. Show all issues

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

  11. Show all issues

    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)
     
     
     
     
    Show all issues

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

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

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

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

    Same here.

  15. Show all issues

    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. 
      
brennie
  1. 
      
  2. Show all issues

    We also write docstrings in the imperitive mood. In this case it would be "Return" not "Returns".

  3. 
      
TB
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:
Commit:
6ab73c55e7f27083d3facea816f4c45268256bbd
1be2cfe6df542934ca89bcb388d916274849910c

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

TB
TB
TB
brennie
  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)
     
     
     
     
     
     
     
    Show all issues

    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)
     
     
     
     
    Show all issues

    Can you format this as:

    return {
        'key': value,
        # ...   
    }
    
  4. Show all issues

    self.assertEqual(len(w), 1)

  5. Show all issues

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

  6. Show all issues

    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. Show all issues

    """ on next line.

  8. Show all issues

    self.assertIn

  9. Show all issues

    self.assertLess

  10. Show all issues

    self.assertIn

  11. Show all issues

    self.assertEqual

  12. Show all issues

    self.assertIn

  13. 
      
TB
brennie
  1. One more nit. Pending the following issue being fixed, this change looks good to me!

  2. Show all issues

    Undo this.

  3. 
      
TB
TB
david
  1. 
      
  2. Show all issues

    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. Show all issues

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

  4. 
      
TB
Review request changed
Commit:
6ee88d42a15f53e54a7b2ae5f3014af3e96c4aab
43e99a8b9e4e2bc77b6618c1a56bfb1dff2d312e

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

TB
mike_conley
  1. This looks good to me! Thanks Theo!

  2. 
      
TB
  1. Ship It!

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

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

    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)
     
     
     
     
    Show all issues

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

  4. 
      
TB
Review request changed
Commit:
c11e4fe783bf3a41b38e36eeb0318b4a63f882b0
a75ce653506c8a92a3026a053482d31f8fca219b

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

TB
david
  1. Ship It!
  2. 
      
TB
Review request changed
Status:
Completed
Change Summary:
Pushed to release-3.0.x (0756e8c)