flake8
passed.
JSHint
passed.
Only sort a DataGrid if the column's sortable is set True
Review Request #10168 — Created Sept. 22, 2018 and submitted
Previously, all arguments to self.request.GET.get('sort', ...) were
passed toself.sort_listeven if they were invalid. This could cause
issues where columns collided with methods, such as when giving
?sort=fullnameto/users/which resulted in an Error 500.Now, in
load_state(), a set of sort keys are computed based on what
columns are set sortable, and the column'sdb_fieldis only added to
self.sort_listif the corresponding column is in this set.
Two unit tests have been added to exercise this functionality in
ascending and descending order, passing in an invalidsortkey and
checking thatself.sort_listis empty.Two additional unit tests sort on one valid and one invalid key in
ascending and descending order and check thatself.sort_listonly
contains the valid key. All four of these unit tests pass.All tests in
djblets.datagrid.testsand
reviewboard.datagrids.testspassed without problems.
| Description | From | Last Updated |
|---|---|---|
|
Can you wrap your testing done a 72 characters as well? |
 brennie
|
|
|
Something to note is that, while this says these specific test suites passed, you're actually going to need to run … |
 chipx86
|
|
|
nit: Your summary does not need a period. |
brennie
|
|
|
These should use get_sort_field, since it's not guaranteed to be db_field. (As a heads up, and you probably know this … |
chipx86
|
|
|
We don't really use .format() in our codebase. Instead, we generally use %-formatted strings. I know this seems backwards, depending … |
chipx86
|
|
|
These all have the same docstring. The docstring should be unique in each case, describing the exact conditions being tested. |
chipx86
|
|
|
E501 line too long (83 > 79 characters) |
 reviewbot
|
|
|
E501 line too long (84 > 79 characters) |
reviewbot
|
|
|
Since this is a function, we should probably pull this out into a temporary, à la: sort_field = column.get_sort_field() sort_keys.add(sort_field) … |
brennie
|
|
|
Let's call column.get_sort_field() once, and reuse the value. That way, if the method does anything remotely expensive, it only has … |
chipx86
|
|
|
You don't need the parents around column.get_sort_field() |
brennie
|
|
|
The trailing """ should always be on its own line. Same with the others below. |
chipx86
|
|
|
Python variable names should be in lower_snake_case (yes thats really what it is called :) ), as per PEP 8. |
brennie
|
|
|
Something that occurs to me... We should explicitly check to make sure get_sort_field() returned a value, so we don't subtly … |
chipx86
|
|
|
nit: no blank line here. |
brennie
|
|
|
nit: no blank line here. |
brennie
|
|
|
Add a blank line between these two. |
 david
|
|
|
Please use single quotes instead of double. |
david
|
- Change Summary:
-
Committing the two additional unit tests.
- Commit:
-
8760b3fececc2f07c4d7dfa2a72dccb3fa9ebe563afee531ea94d808441bb45cdcc60b08565be5a4
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
These should use
get_sort_field, since it's not guaranteed to bedb_field.(As a heads up, and you probably know this but just to be thorough, you'd want to update this as well when supporting lists later.)
-
We don't really use
.format()in our codebase. Instead, we generally use%-formatted strings. I know this seems backwards, depending on who you learn from, but it's a bit faster and is just what we have in the codebase.Also, you'll want to use single quotes where possible.
-
These all have the same docstring. The docstring should be unique in each case, describing the exact conditions being tested.
- Change Summary:
-
Use
StatefulColumns; useget_sort_key()instead ofdb_field; use %-substitution; amend docstrings to be more descriptive and unique. - Commit:
-
3afee531ea94d808441bb45cdcc60b08565be5a4e5c6d3d2563d86ab30f023adb598135b804193c8
- Change Summary:
-
Fixed docstring length and wrapping.
- Commit:
-
e5c6d3d2563d86ab30f023adb598135b804193c8b12eb82def548898b965f11cc7aee0f88fcdf6f0
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Change Summary:
-
Only call get_sort_field() once; fix docstring formatting.
- Commit:
-
b12eb82def548898b965f11cc7aee0f88fcdf6f06a7a76e75794ba030d36aa510f2fd2577eee66c7
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Change Summary:
-
Fixing the description and testing to 72 hard wrap.
- Description:
-
~ Previously, all arguments to self.request.GET.get('sort', ...) were passed to
~ self.sort_listeven if they were invalid. This could cause issues where~ columns collided with methods, such as when giving ?sort=fullnameto~ /users/which resulted in an Error 500.~ Previously, all arguments to self.request.GET.get('sort', ...) were
~ passed to self.sort_listeven if they were invalid. This could cause~ issues where columns collided with methods, such as when giving ~ ?sort=fullnameto/users/which resulted in an Error 500.~ Now, in
load_state(), a set of sort keys are computed based on what columns~ are set sortable, and the column's db_fieldis only added toself.sort_list~ if the corresponding column is in this set. ~ Now, in
load_state(), a set of sort keys are computed based on what~ columns are set sortable, and the column's db_fieldis only added to~ self.sort_listif the corresponding column is in this set. - Testing Done:
-
~ Two unit tests have been added to exercise this functionality in ascending and descending order, passing in an invalid
sortkey and checking thatself.sort_listis empty.~ Two unit tests have been added to exercise this functionality in
+ ascending and descending order, passing in an invalid sortkey and+ checking that self.sort_listis empty.~ Two additional unit tests sort on one valid and one invalid key in ascending and descending order and check that
self.sort_listonly contains the valid key. All four of these unit tests pass.~ Two additional unit tests sort on one valid and one invalid key in
+ ascending and descending order and check that self.sort_listonly+ contains the valid key. All four of these unit tests pass.
- Change Summary:
-
Removed unnecessary parentheses.
- Commit:
-
6a7a76e75794ba030d36aa510f2fd2577eee66c71ec1bc6cffdce03e8b32108d959e115810112855
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Change Summary:
-
One of the unit tests were failing due to my recent changes which required updating the sort algorithm to map the column's id to its sort_field. All Datagrid tests now pass.
- Commit:
-
1ec1bc6cffdce03e8b32108d959e11581011285552a4fdaacb460f2e5decd5c904f3622d6fdf68b0
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Change Summary:
-
Accidentally left column.field_name insted of column.get_sort_field()
- Commit:
-
52a4fdaacb460f2e5decd5c904f3622d6fdf68b05b414a19fa69b06027ffb4ecad52b754531346cb
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Change Summary:
-
Tests updated
- Testing Done:
-
Two unit tests have been added to exercise this functionality in
ascending and descending order, passing in an invalid sortkey andchecking that self.sort_listis empty.Two additional unit tests sort on one valid and one invalid key in
ascending and descending order and check that self.sort_listonlycontains the valid key. All four of these unit tests pass. + + All tests in
djblets.datagrid.testsand+ reviewboard.datagrids.testspassed without problems.
-
-
Something to note is that, while this says these specific test suites passed, you're actually going to need to run the entire test suite for Djblets and Review Board. Changes can often affect things other than in the module they're a part of, and we've had breakages in the past because it was assumed small changes were confined to their suite. While I don't think this change affects much else, it's good habit to run the whole test suite for both and mention that instead.
-
Something that occurs to me... We should explicitly check to make sure
get_sort_field()returned a value, so we don't subtly break (addNone=Noneand-None=Noneto the sort list) in the event that a column issortable=Truewith custom logic that intentionally or unintentionally returnsNone.You can do this with a simple
if sort_field:before adding.We'd also want a unit test to ensure
Nonedoesn't get in in any form.
- Change Summary:
-
Updated unit tests.
- Commit:
-
5b414a19fa69b06027ffb4ecad52b754531346cbc865e3b4b5a514fc4e4e05e902fdd8f75dad15cb
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Change Summary:
-
Removed blank lines.
- Commit:
-
c865e3b4b5a514fc4e4e05e902fdd8f75dad15cbfd0a4e7c527a17bfe4ca0e16c58471c774c7d3cb
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Change Summary:
-
Changed double quotes to single quotes, added linebreaks where requested.
- Commit:
-
fd0a4e7c527a17bfe4ca0e16c58471c774c7d3cb8867d05d9d23fb6b89bd9fc60a6d7e1b2aa5dc76
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.