Added filtering of sorting columns that are not actual database fields (e.x. fullname)
Review Request #9497 — Created Jan. 19, 2018 and discarded
This fixes a crash throwing a FieldError in DataGrid. The crash is caused by specifying a sort column in the url that is a method (rather than a field; For example "fullname" is a method on User (as
get_full_name()) and no table column for this field actually exists). The method is resolved from the field name, and added to the queryset's orderby call, which eventually returns with a FieldError (since no column calledget_full_nameexists).DataGrid now ignores these columns and logs a warning to the console (just like it does when sorting by non-existing columns).
Did manual testing on a mock database with various sort columns.
Removed a unit test that no longer was relevant (since it tested the behaviour of
get_sort_columnon these invalid columns, and it no longer gets called).
Added a (passing) unit test that makes sure no exception is thrown and a warning is printed about trying to sort the grid by a column that isn't in the db table.
| Description | From | Last Updated |
|---|---|---|
|
I realized there's a flaw with the approach here. It's assuming we're only able to sort by fields on the … |
 chipx86
|
|
|
The bug number should go in the Bugs field, instead of having a link in the description. |
chipx86
|
|
|
The text for the summary, description, and testing done need to be more in line with our standards described at … |
chipx86
|
|
|
Make sure you close any open issues as you fix them in your code, so that you don't leave any … |
chipx86
|
|
|
E501 line too long (140 > 79 characters) |
 reviewbot
|
|
|
E501 line too long (84 > 79 characters) |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
E501 line too long (86 > 79 characters) |
reviewbot
|
|
|
W291 trailing whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
E501 line too long (107 > 79 characters) |
reviewbot
|
|
|
E101 indentation contains mixed spaces and tabs |
reviewbot
|
|
|
W191 indentation contains tabs |
reviewbot
|
|
|
W291 trailing whitespace |
reviewbot
|
|
|
W191 indentation contains tabs |
reviewbot
|
|
|
W191 indentation contains tabs |
reviewbot
|
|
|
W291 trailing whitespace |
reviewbot
|
|
|
I know the old code was like this, but there should be a blank line before the start of a … |
chipx86
|
|
|
No blank line here. |
chipx86
|
|
|
Much more can fit on each line. |
chipx86
|
|
|
This doesn't match the docstring format for our tests. It should be more like the old docstring. They're in this … |
chipx86
|
|
|
Ideally, we should always use single quotes. We can also import the logger we care about directly from the module, … |
chipx86
|
|
|
Instead of calls[-1], use last_call. |
chipx86
|
- Change Summary:
-
Fixed flake8 errors
- Commit:
-
d074501728857ad187cbbc25c21a5d491cfbe0074f4b644b8a7ac63bae53351847306d05fd64d103
Checks run (1 failed, 1 succeeded)
flake8
- Commit:
-
4f4b644b8a7ac63bae53351847306d05fd64d103453afb2a308bc8b4a3c3f06a76d6d3565371a3b2
Checks run (2 succeeded)
-
-
I realized there's a flaw with the approach here. It's assuming we're only able to sort by fields on the main model being worked with, but that's not the case. We can sort by related fields as well. For instance, if we were working with review requests:
?sort=submitter.usernameIn this case, we'd be sorting based on the username of the submitter, which clearly is not a field directly on the model. That case is important, and must be allowed. (Unit tests should probably be added to make sure it continues to work correctly.)
-
-
The text for the summary, description, and testing done need to be more in line with our standards described at https://www.reviewboard.org/docs/codebase/dev/writing-good-descriptions/.
-
Make sure you close any open issues as you fix them in your code, so that you don't leave any open or accidentally close any that turned out not to be fixed. Very important while going through code review.
-
I know the old code was like this, but there should be a blank line before the start of a new block (like
for). -
-
-
This doesn't match the docstring format for our tests. It should be more like the old docstring. They're in this format:
"""One-line-summary."""Or:
"""One-line summary broken across multiple lines. """Note that this format is only for unit test docstrings. Normal docstrings follow a differnet format, documented in Notion.
-
Ideally, we should always use single quotes.
We can also import the logger we care about directly from the module, instaed of calling
logging.getLogger()here. -
- Description:
-
~ Added filtering of sorting columns that are not actual database fields (e.x. fullname)
~ This fixes a crash throwing a FieldError in DataGrid. The crash is caused by specifying a sort column in the url that is a method (rather than a field; For example "fullname" is a method on User (as
get_full_name()) and no table column for this field actually exists). The method is resolved from the field name, and added to the queryset's orderby call, which eventually returns with a FieldError (since no column calledget_full_nameexists).~ ~ DataGrid now ignores these columns and logs a warning to the console (just like it does when sorting by non-existing columns).
- - Fixed bug #4607.
- When trying to be "smart" and enter bad columns into the sortfield (bad being columns that are not actual columns in the db table, but rather functions such as get_full_name), there DataGrid would raise a FieldError. It now ignores these columns and logs a warning to the console (just like it does when sorting by non-existing columns). - Testing Done:
-
~ - Manual:
- Mock database sorting of users and requests grid by acceptable and unacceptable column names
~ - Automated
- Fixed and reran failing test
~ Did manual testing on a mock database with various sort columns.
~ + Removed a unit test that no longer was relevant (since it tested the behaviour of
get_sort_columnon these invalid columns, and it no longer gets called)+ Added a (passing) unit test that makes sure no exception is thrown and a warning is printed about trying to sort the grid by a column that isn't in the db table. - Manual:
- Bugs:
-
Checks run (2 succeeded)
flake8 passed.JSHint passed.
- Testing Done:
-
Did manual testing on a mock database with various sort columns.
~ Removed a unit test that no longer was relevant (since it tested the behaviour of
get_sort_columnon these invalid columns, and it no longer gets called)~ Removed a unit test that no longer was relevant (since it tested the behaviour of
get_sort_columnon these invalid columns, and it no longer gets called).Added a (passing) unit test that makes sure no exception is thrown and a warning is printed about trying to sort the grid by a column that isn't in the db table.
