Add a "Last Updated By" column to ReviewBoard dashboard

Review Request #11524 — Created March 14, 2021 and updated

clarissaudrey
Review Board
master
reviewboard, students

Currently, the last updated column only shows the timestamp
of last activity that has occurred. It is desireable to show
the user who is responsible for the last activity as well.
Therefore, we need to add another column "Last Updated By" to
the dashboard.

Manual testing:
1. On the UI, activate the new Last Updated By column by selecting it.
2. It should show the user (linkable to the user's profile) that is
responsible for the last update.
Tested on Safari, Chrome, and Firefox.

Automated unit testing:
1. Run ./tests/runtests.py reviewboard.datagrids.tests.

Summary
Add class LastUpdatedByColumn and override get_user method
Fixed import module
Implemented get_user based on the last activity and added the column to the grid view
Fixed formatting
Fixed rendering column for last_updated_by
Fixed get_user
Added unit tests for Last Updated By column
Fixed line too long error
Removed unused fields in datagrids/tests.py
Fixes based on mentor's reviews
Fixed trailing whitespace
Description From Last Updated

I'm a bit confused what the difference is between Review, Review Request and Review Request Draft. Would you mind clarifying ...

mderosemderose

E501 line too long (88 > 79 characters)

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

F841 local variable 'review' is assigned to but never used

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

F841 local variable 'diffset' is assigned to but never used

reviewbotreviewbot

F841 local variable 'review' is assigned to but never used

reviewbotreviewbot

F841 local variable 'diffset' is assigned to but never used

reviewbotreviewbot

Docstrings need to conform to our documentation guidelines. Look for other functions like get_user and make sure you're following that ...

chipx86chipx86

All variables in our Python code use snake_case pattern, rather than camelCase.

chipx86chipx86

This is being set and then overridden below. Doesn't seem like this needs to be set here, because of that.

chipx86chipx86

Something important to note here is that we can't necessarily assume that the review request's owner is the user that ...

chipx86chipx86

This can cause problems down the road if we introduce more types of objects and one of them isn't owner. ...

chipx86chipx86

Imports should always be in alphabetical order.

chipx86chipx86

The last parameter of a function call shouldn't have a trailing comma. The ) should be on the same line ...

chipx86chipx86

This doesn't seem to be related to your change, so it should be reverted.

chipx86chipx86

This will also need to be in alphabetical order.

chipx86chipx86

This from .. import .. also needs to be in alphabetical order within the list of all imports.

chipx86chipx86

This is only used if the column is testing with a LocalSite. None of these are, from what I can ...

chipx86chipx86

The trailing """ must be on its own line when it's a multi-line docstring. Also, this docstring doesn't say what ...

chipx86chipx86

Several of our test objects use the same usernames, which can create false-positives in username-based tests. Your code should explicitly ...

chipx86chipx86

We're not testing get_last_activity_info(). This could would be better served testing get_user() on the column.

chipx86chipx86

For each of these, these usernames very well may be the correct ones, but there's nothing currently indicating that we ...

chipx86chipx86

These are probably wrapping prematurely. Same with the other unit tests.

chipx86chipx86

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot
CL
mderose
  1. I think this makes sense. I just was hoping you could provide a quick clarification just for my understanding. Good work on this!

  2. reviewboard/datagrids/columns.py (Diff revision 2)
     
     

    I'm a bit confused what the difference is between Review, Review Request and Review Request Draft. Would you mind clarifying this please?

    1. Hi Matthew! Based on my understanding, a review request is the component (e.g your code changes) that you hand over to be reviewed. While a review is a thread of reviews or discussions inside a review request. And lastly, the review request draft is the review request that you have not published yet (where you can decide on publishing or discarding).

  3. 
      
CL
Review request changed

Summary:

-[WIP] Add a "Last Updated By" column to ReviewBoard dashboard
+Add a "Last Updated By" column to ReviewBoard dashboard

Testing Done:

~  
  1. On the UI, activate the new Last Updated By column by selecting it.
~  
  1. It should show the user (linkable to the user's profile) that is
    responsible for the last update.
  ~

Manual testing:

  ~ 1. On the UI, activate the new Last Updated By column by selecting it.
  + 2. It should show the user (linkable to the user's profile) that is
  + responsible for the last update.

  +
  +

Automated unit testing:

  + 1. Run ./tests/runtests.py reviewboard.datagrids.tests 115 118 119.

Commits:

Summary
-
Add class LastUpdatedByColumn and override get_user method
-
Fixed import module
-
Implemented get_user based on the last activity and added the column to the grid view
-
Fixed formatting
-
Fixed rendering column for last_updated_by
-
Fixed get_user
+
Add class LastUpdatedByColumn and override get_user method
+
Fixed import module
+
Implemented get_user based on the last activity and added the column to the grid view
+
Fixed formatting
+
Fixed rendering column for last_updated_by
+
Fixed get_user
+
Added unit tests for Last Updated By column

Diff:

Revision 3 (+236 -50)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

CL
Review request changed

Commits:

Summary
-
Add class LastUpdatedByColumn and override get_user method
-
Fixed import module
-
Implemented get_user based on the last activity and added the column to the grid view
-
Fixed formatting
-
Fixed rendering column for last_updated_by
-
Fixed get_user
-
Added unit tests for Last Updated By column
+
Add class LastUpdatedByColumn and override get_user method
+
Fixed import module
+
Implemented get_user based on the last activity and added the column to the grid view
+
Fixed formatting
+
Fixed rendering column for last_updated_by
+
Fixed get_user
+
Added unit tests for Last Updated By column
+
Fixed line too long error

Diff:

Revision 4 (+247 -55)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

CL
chipx86
  1. 
      
  2. reviewboard/datagrids/columns.py (Diff revision 5)
     
     
     
     

    Docstrings need to conform to our documentation guidelines. Look for other functions like get_user and make sure you're following that pattern. This needs to have Args and Returns sections.

    A function's summary should succintly describe its purpose. The description is used for additional details. Keep this high-level: The fact that it's using get_last_activity_info() is an implementation detail that' best left to the code. The reader doesn't care. They care about hte purpose of the function.

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

    All variables in our Python code use snake_case pattern, rather than camelCase.

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

    This is being set and then overridden below. Doesn't seem like this needs to be set here, because of that.

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

    Something important to note here is that we can't necessarily assume that the review request's owner is the user that performed the update. For review requests, the changedesc object is also returned. This is the "Review request changed" box. An administrator, or special users, can make changes on a review request on another user's behalf, and the user that did that is in changedesc.user.

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

    This can cause problems down the road if we introduce more types of objects and one of them isn't owner. Each check should be very explicit about what object we're working with, and any else: should probably raise an AssertionError if the user isn't an expected type (with a useful message).

  7. reviewboard/datagrids/grids.py (Diff revision 5)
     
     
     

    Imports should always be in alphabetical order.

  8. reviewboard/datagrids/grids.py (Diff revision 5)
     
     
     
     

    The last parameter of a function call shouldn't have a trailing comma. The ) should be on the same line as the last parameter, as well. Note the other columns in this class.

  9. reviewboard/datagrids/grids.py (Diff revision 5)
     
     
     

    This doesn't seem to be related to your change, so it should be reverted.

  10. reviewboard/datagrids/tests.py (Diff revision 5)
     
     
     

    This will also need to be in alphabetical order.

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

    This from .. import .. also needs to be in alphabetical order within the list of all imports.

  12. reviewboard/datagrids/tests.py (Diff revision 5)
     
     

    This is only used if the column is testing with a LocalSite. None of these are, from what I can see.

  13. reviewboard/datagrids/tests.py (Diff revision 5)
     
     
     

    The trailing """ must be on its own line when it's a multi-line docstring.

    Also, this docstring doesn't say what is being tested. This should explicitly list the column name. Look at other unit tests in this file.

  14. reviewboard/datagrids/tests.py (Diff revision 5)
     
     
     

    Several of our test objects use the same usernames, which can create false-positives in username-based tests. Your code should explicitly create a user for each test that we want to check for, make sure the right object is being created with that user, and then use that user for any checks.

  15. reviewboard/datagrids/tests.py (Diff revision 5)
     
     
     
     

    We're not testing get_last_activity_info(). This could would be better served testing get_user() on the column.

  16. reviewboard/datagrids/tests.py (Diff revision 5)
     
     
     
     

    For each of these, these usernames very well may be the correct ones, but there's nothing currently indicating that we know for sure. The code isn't tying these to the objects the usernames are coming from. We should explicitly use those.

  17. reviewboard/datagrids/tests.py (Diff revision 5)
     
     
     
     
     
     

    These are probably wrapping prematurely.

    Same with the other unit tests.

  18. 
      
CL
Review request changed

Testing Done:

   

Manual testing:

    1. On the UI, activate the new Last Updated By column by selecting it.
    2. It should show the user (linkable to the user's profile) that is
    responsible for the last update.
    Tested on Safari, Chrome, and Firefox.

   
   

Automated unit testing:

~   1. Run ./tests/runtests.py reviewboard.datagrids.tests 115 118 119.

  ~ 1. Run ./tests/runtests.py reviewboard.datagrids.tests.

Commits:

Summary
-
Add class LastUpdatedByColumn and override get_user method
-
Fixed import module
-
Implemented get_user based on the last activity and added the column to the grid view
-
Fixed formatting
-
Fixed rendering column for last_updated_by
-
Fixed get_user
-
Added unit tests for Last Updated By column
-
Fixed line too long error
-
Removed unused fields in datagrids/tests.py
+
Add class LastUpdatedByColumn and override get_user method
+
Fixed import module
+
Implemented get_user based on the last activity and added the column to the grid view
+
Fixed formatting
+
Fixed rendering column for last_updated_by
+
Fixed get_user
+
Added unit tests for Last Updated By column
+
Fixed line too long error
+
Removed unused fields in datagrids/tests.py
+
Fixes based on mentor's reviews

Diff:

Revision 6 (+337 -107)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

CL
Review request changed

Commits:

Summary
-
Add class LastUpdatedByColumn and override get_user method
-
Fixed import module
-
Implemented get_user based on the last activity and added the column to the grid view
-
Fixed formatting
-
Fixed rendering column for last_updated_by
-
Fixed get_user
-
Added unit tests for Last Updated By column
-
Fixed line too long error
-
Removed unused fields in datagrids/tests.py
-
Fixes based on mentor's reviews
+
Add class LastUpdatedByColumn and override get_user method
+
Fixed import module
+
Implemented get_user based on the last activity and added the column to the grid view
+
Fixed formatting
+
Fixed rendering column for last_updated_by
+
Fixed get_user
+
Added unit tests for Last Updated By column
+
Fixed line too long error
+
Removed unused fields in datagrids/tests.py
+
Fixes based on mentor's reviews
+
Fixed trailing whitespace

Diff:

Revision 7 (+341 -113)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...