flake8
-
reviewboard/datagrids/columns.py (Diff revision 1) Show all issues
Review Request #12018 — Created Jan. 28, 2022 and updated
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
ReviewRequest
last activity info functionsReviewRequest.get_last_activity_info()
also returns the user that last updated the review requestReviewRequest.extra_data
ReviewRequest.set_last_activity_info(timestamp, updated_object, changedesc, user)
ReviewRequest.get_last_activity_info()
to take advantage of the user fieldReviewRequest.get_last_activity_info()
ReviewRequestLastUpdateResource
return the correct user now that ReviewRequest.get_last_activity_info()
includes itSummary | ID |
---|---|
2886dc701a7582abb5ab763c2d9c1fe7ba1d542b | |
347e5aa8a529bc8b6aaea55d331475446f4d3b63 | |
bba4df2b0cd2f382bb4eb2d88dd5a20cb92f9b22 | |
9eead2a3139e9cec4e82a13c376d33fc558582f7 | |
19d8bb48e3e7a2ae0536196102556e00918acf96 | |
f568b695db1d1f7372b8dc53e5eee94aca606785 | |
3a44a50bec97041c994a6b7b342998c9c6f942f2 | |
410e1b643422b85781aa59d6a59f005b1c898c71 | |
b076ce9a7ead039ffb03c6e1f27b6775a31c712d | |
9dfd7179ef9462d8463a230e0769f7fa3b4e7dcb | |
caf3389617783d8d0d40393f750381fba7424f8f | |
d1b2f75efb0be1df4e8ba081ed3b5616be574067 | |
c3bd1a09f5a1ac7359cf7f89628067d86cd74a70 | |
bb22a4b741f7ff917b1c4b1dce19afca681c5e95 | |
2d5599330ffb81e6e5c02fc28c17c91ff6e88487 | |
cdb0cfc28c3f678a77d1f779daa05da483a6915c | |
3120d9fe3af41f0467404c49716457599d42ac3b | |
e606ba6247e22f74d4d40b1e28cc8d34de4549e8 |
Description | From | Last Updated |
---|---|---|
E501 line too long (94 > 79 characters) |
![]() |
|
F401 'reviewboard.diffviewer.models.diffset.DiffSet' imported but unused |
![]() |
|
F401 'reviewboard.reviews.models.review.Review' imported but unused |
![]() |
|
E501 line too long (82 > 79 characters) |
![]() |
|
E501 line too long (94 > 79 characters) |
![]() |
|
E501 line too long (84 > 79 characters) |
![]() |
|
E501 line too long (84 > 79 characters) |
![]() |
|
We need to document the new sortable arg in here. |
|
|
This needs an "Args" section. |
|
|
This needs an "Args" section. |
|
|
We can use a set comprehension here: ids = { review_request.get_last_activity_user_id() for review_request in object_list } |
|
|
You're using this for every test case, so you can just add a class attribute: class LastUpdatedByColumnTests(BaseColumnTestCase): ... fixtures = … |
|
|
Let's wrap this a little differently. Here and in the other test cases below: review_requests = [ self.create_review_request( summary='Test %d' … |
|
|
If Review is used multiple times, wouldn't it be better to move this import statement to the global import at … |
|
|
This should be in the imperative mood ("Set" instead of "Sets") |
|
|
Double backticks aren't necessary around the argument names here. |
|
|
The first line of the docstring should be only one line, and should be in the imperative mood. If you … |
|
|
This should be in the imperative mood ("Return" instead of "Returns"). This docstring also needs a "Returns" section. |
|
|
id is a built-in for python. Can we use a different variable name? |
|
|
Add a blank line between these two |
|
|
We generally avoid the python ternary syntax because it's awkward and confusing. Please just use an if/else here. |
|
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+324 -18) |
reviewboard/datagrids/columns.py (Diff revision 2) |
---|
F401 'reviewboard.diffviewer.models.diffset.DiffSet' imported but unused
reviewboard/datagrids/columns.py (Diff revision 2) |
---|
F401 'reviewboard.reviews.models.review.Review' imported but unused
Commits: |
|
|||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+322 -20) |
Continue work on last_activity_info caching
Commits: |
|
|||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+563 -71) |
reviewboard/reviews/models/review_request.py (Diff revision 4) |
---|
E501 line too long (82 > 79 characters)
Fetch users in Last Updated By column in one query
Commits: |
|
|||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+2871 -4409) |
Commits: |
|
|||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+670 -88) |
Add tests for LastUpdatedByColumn
Commits: |
|
|||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+720 -90) |
Commits: |
|
|||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 8 (+776 -94) |
Commits: |
|
|||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 9 (+780 -94) |
Commits: |
|
|||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 10 (+792 -98) |
Finished
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 11 (+871 -155) |
Description: |
|
---|
reviewboard/datagrids/columns.py (Diff revision 11) |
---|
We need to document the new
sortable
arg in here.
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 }
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):
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?
reviewboard/reviews/models/review_request.py (Diff revision 11) |
---|
This should be in the imperative mood ("Set" instead of "Sets")
reviewboard/reviews/models/review_request.py (Diff revision 11) |
---|
Double backticks aren't necessary around the argument names here.
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.
reviewboard/reviews/models/review_request.py (Diff revision 11) |
---|
This should be in the imperative mood ("Return" instead of "Returns"). This docstring also needs a "Returns" section.
reviewboard/reviews/models/review_request.py (Diff revision 11) |
---|
id
is a built-in for python. Can we use a different variable name?
reviewboard/reviews/models/review_request.py (Diff revision 11) |
---|
We generally avoid the python ternary syntax because it's awkward and confusing. Please just use an if/else here.
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?
reviewboard/reviews/models/review_request.py (Diff revision 11) |
---|
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?
Make changes to address feedback
Commits: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 12 (+952 -186) |