Add a "Last Updated By" column to the dashboard.

Review Request #12018 — Created Jan. 28, 2022 and updated

kylemclean
Review Board
master
reviewboard

These changes add a new column to the review request dashboard called "Last Updated By" which displays the user that made the last update to each review request. Similar to the existing "Last Updated" column, which shows the time that the last update was made, this column is updated whenever a review request is created, edited, receives a review, is closed, or reopened.

Changes

"Last Updated By" column

  • Added a "Last Updated By" column
  • Added information about the new column to manual
  • Added tests for the new column

ReviewRequest last activity info functions

  • ReviewRequest.get_last_activity_info() also returns the user that last updated the review request
  • Review requests cache their last activity info (last updated object, changedesc, and user) in ReviewRequest.extra_data
  • Added ReviewRequest.set_last_activity_info(timestamp, updated_object, changedesc, user)
  • Modified existing uses of ReviewRequest.get_last_activity_info() to take advantage of the user field
  • Added and modified existing tests of ReviewRequest.get_last_activity_info()

Other

  • Made the WebAPI ReviewRequestLastUpdateResource return the correct user now that ReviewRequest.get_last_activity_info() includes it
  • Added tests for the column itself as well as the new last activity-related functions.
  • Ran all backend tests and observed that all passed.
  • Performed manual testing of the functionality of the column by creating, editing, reviewing, closing, and reopening review requests and observing how the column changes.
Summary
Add a "Last Updated By" column to the dashboard
First try at optimization of get_last_activity_info
Continue work on last_activity_info caching
Fix import error in review_request.py
Fix incorrect argument order
None handling for changedesc
Fetch users in Last Updated By column in one query
Add LastUpdatedByColumnTests based on UsernameColumnTests
Fix ReviewRequest.set_last_activity_info() failing when user is None
Add info on Last Updated By column to dashboard documentation
Fix issues with review request closing updating last activity info
Handle user being None in ReviewRequest.get_last_activity_user_id()
Make ReviewRequest.publish() set updated_object to diffset if it exists
Update last activity info when review request is reopened
Fix last_updated being set incorrectly after publishing ReviewRequest
Send the correct user in ReviewRequestLastUpdateResource
Fix ReviewRequest.reopen() setting last activity info after no status change
Make changes to address feedback
Description From Last Updated

E501 line too long (94 > 79 characters)

reviewbotreviewbot

F401 'reviewboard.diffviewer.models.diffset.DiffSet' imported but unused

reviewbotreviewbot

F401 'reviewboard.reviews.models.review.Review' imported but unused

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

E501 line too long (94 > 79 characters)

reviewbotreviewbot

E501 line too long (84 > 79 characters)

reviewbotreviewbot

E501 line too long (84 > 79 characters)

reviewbotreviewbot

We need to document the new sortable arg in here.

daviddavid

This needs an "Args" section.

daviddavid

This needs an "Args" section.

daviddavid

We can use a set comprehension here: ids = { review_request.get_last_activity_user_id() for review_request in object_list }

daviddavid

You're using this for every test case, so you can just add a class attribute: class LastUpdatedByColumnTests(BaseColumnTestCase): ... fixtures = …

daviddavid

Let's wrap this a little differently. Here and in the other test cases below: review_requests = [ self.create_review_request( summary='Test %d' …

daviddavid

If Review is used multiple times, wouldn't it be better to move this import statement to the global import at …

sheenaNgsheenaNg

This should be in the imperative mood ("Set" instead of "Sets")

daviddavid

Double backticks aren't necessary around the argument names here.

daviddavid

The first line of the docstring should be only one line, and should be in the imperative mood. If you …

daviddavid

This should be in the imperative mood ("Return" instead of "Returns"). This docstring also needs a "Returns" section.

daviddavid

id is a built-in for python. Can we use a different variable name?

daviddavid

Add a blank line between these two

daviddavid

We generally avoid the python ternary syntax because it's awkward and confusing. Please just use an if/else here.

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

flake8

KY
Review request changed

Commits:

Summary
-
[WIP] Add a "Last Updated By" column to the dashboard
+
[WIP] Add a "Last Updated By" column to the dashboard
+
[WIP] First try at optimization of get_last_activity_info

Diff:

Revision 2 (+324 -18)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

KY
KY
Review request changed

Change Summary:

Continue work on last_activity_info caching

Commits:

Summary
-
[WIP] Add a "Last Updated By" column to the dashboard
-
[WIP] First try at optimization of get_last_activity_info
+
[WIP] Add a "Last Updated By" column to the dashboard
+
[WIP] First try at optimization of get_last_activity_info
+
Continue work on last_activity_info caching

Diff:

Revision 4 (+563 -71)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

KY
KY
KY
Review request changed

Change Summary:

Add tests for LastUpdatedByColumn

Commits:

Summary
-
[WIP] Add a "Last Updated By" column to the dashboard
-
[WIP] First try at optimization of get_last_activity_info
-
Continue work on last_activity_info caching
-
Fix import error in review_request.py
-
Fix incorrect argument order
-
None handling for changedesc
-
Fetch users in Last Updated By column in one query
+
[WIP] Add a "Last Updated By" column to the dashboard
+
[WIP] First try at optimization of get_last_activity_info
+
Continue work on last_activity_info caching
+
Fix import error in review_request.py
+
Fix incorrect argument order
+
None handling for changedesc
+
Fetch users in Last Updated By column in one query
+
Add LastUpdatedByColumnTests based on UsernameColumnTests

Diff:

Revision 7 (+720 -90)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

KY
Review request changed

Commits:

Summary
-
[WIP] Add a "Last Updated By" column to the dashboard
-
[WIP] First try at optimization of get_last_activity_info
-
Continue work on last_activity_info caching
-
Fix import error in review_request.py
-
Fix incorrect argument order
-
None handling for changedesc
-
Fetch users in Last Updated By column in one query
-
Add LastUpdatedByColumnTests based on UsernameColumnTests
+
[WIP] Add a "Last Updated By" column to the dashboard
+
[WIP] First try at optimization of get_last_activity_info
+
Continue work on last_activity_info caching
+
Fix import error in review_request.py
+
Fix incorrect argument order
+
None handling for changedesc
+
Fetch users in Last Updated By column in one query
+
Add LastUpdatedByColumnTests based on UsernameColumnTests
+
Fix ReviewRequest.set_last_activity_info() failing when user is None

Diff:

Revision 8 (+776 -94)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

KY
KY
KY
KY
david
  1. 
      
  2. reviewboard/datagrids/columns.py (Diff revision 11)
     
     
     
     
     
     
     

    We need to document the new sortable arg in here.

  3. reviewboard/datagrids/columns.py (Diff revision 11)
     
     
     

    This needs an "Args" section.

  4. reviewboard/datagrids/columns.py (Diff revision 11)
     
     
     

    This needs an "Args" section.

  5. reviewboard/datagrids/columns.py (Diff revision 11)
     
     
     
     
     

    We can use a set comprehension here:

    ids = {
        review_request.get_last_activity_user_id()
        for review_request in object_list
    }
    
  6. reviewboard/datagrids/tests.py (Diff revision 11)
     
     

    You're using this for every test case, so you can just add a class attribute:

    class LastUpdatedByColumnTests(BaseColumnTestCase):
        ...
        fixtures = ['test_site']
        ...
        def test_render(self):
    
  7. reviewboard/datagrids/tests.py (Diff revision 11)
     
     
     
     

    Let's wrap this a little differently. Here and in the other test cases below:

    review_requests = [
        self.create_review_request(
            summary='Test %d' % index,
            publish=True,
            submitter=username)
        for (index, username) in enumerate(usernames)
    ]
    

    In fact, since it looks like you are doing that for all the test methods, perhaps move this into a helper function?

  8. This should be in the imperative mood ("Set" instead of "Sets")

  9. reviewboard/reviews/models/review_request.py (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Double backticks aren't necessary around the argument names here.

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

    The first line of the docstring should be only one line, and should be in the imperative mood. If you need more detail for completenes, add additional paragraphs.

  11. This should be in the imperative mood ("Return" instead of "Returns"). This docstring also needs a "Returns" section.

  12. id is a built-in for python. Can we use a different variable name?

  13. reviewboard/reviews/models/review_request.py (Diff revision 11)
     
     
     

    Add a blank line between these two

  14. We generally avoid the python ternary syntax because it's awkward and confusing. Please just use an if/else here.

  15. 
      
sheenaNg
  1. 
      
  2. reviewboard/datagrids/columns.py (Diff revision 11)
     
     

    I wonder why is "state" being passed as a parameter in this method but it is not being used?

    1. I decided to add a state argument because the LastUpdatedByColumn, which inherits from UsernameColumn, needs it in this method, and there does not appear to be another way of getting the column state.

  3. If Review is used multiple times, wouldn't it be better to move this import statement to the global import at the top of the file so there is no need to repeat this import statement multiple times?

    1. I believe the reason that Review is imported in this method and not the entire module is because it would otherwise result in a circular import between these modules.

  4. 
      
KY
Review request changed

Change Summary:

Make changes to address feedback

Commits:

Summary
-
[WIP] Add a "Last Updated By" column to the dashboard
-
[WIP] First try at optimization of get_last_activity_info
-
Continue work on last_activity_info caching
-
Fix import error in review_request.py
-
Fix incorrect argument order
-
None handling for changedesc
-
Fetch users in Last Updated By column in one query
-
Add LastUpdatedByColumnTests based on UsernameColumnTests
-
Fix ReviewRequest.set_last_activity_info() failing when user is None
-
Add info on Last Updated By column to dashboard documentation
-
Fix issues with review request closing updating last activity info
-
Handle user being None in ReviewRequest.get_last_activity_user_id()
-
Make ReviewRequest.publish() set updated_object to diffset if it exists
-
Update last activity info when review request is reopened
-
Fix last_updated being set incorrectly after publishing ReviewRequest
-
Send the correct user in ReviewRequestLastUpdateResource
-
Fix ReviewRequest.reopen() setting last activity info after no status change
+
Add a "Last Updated By" column to the dashboard
+
First try at optimization of get_last_activity_info
+
Continue work on last_activity_info caching
+
Fix import error in review_request.py
+
Fix incorrect argument order
+
None handling for changedesc
+
Fetch users in Last Updated By column in one query
+
Add LastUpdatedByColumnTests based on UsernameColumnTests
+
Fix ReviewRequest.set_last_activity_info() failing when user is None
+
Add info on Last Updated By column to dashboard documentation
+
Fix issues with review request closing updating last activity info
+
Handle user being None in ReviewRequest.get_last_activity_user_id()
+
Make ReviewRequest.publish() set updated_object to diffset if it exists
+
Update last activity info when review request is reopened
+
Fix last_updated being set incorrectly after publishing ReviewRequest
+
Send the correct user in ReviewRequestLastUpdateResource
+
Fix ReviewRequest.reopen() setting last activity info after no status change
+
Make changes to address feedback

Diff:

Revision 12 (+952 -186)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...