Summary:

Added filtering of sorting columns that are not actual database fields (e.x. fullname)

Review Request #9497 — Created Jan. 19, 2018 and discarded Oct. 29, 2018, 2:16 p.m.

Information
Owner:	rpolyano
Repository:	Djblets
Branch:	master
Bugs:	4607
Depends On:
Reviewers
Groups:	djblets, students
People:

Description

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 called get_full_name exists).

DataGrid now ignores these columns and logs a warning to the console (just like it does when sorting by non-existing columns).

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_column on 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.

Issues

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	Jan. 19, 2018, 12:08 p.m.
The bug number should go in the Bugs field, instead of having a link in the description.	chipx86	Jan. 19, 2018, 12:20 p.m.
The text for the summary, description, and testing done need to be more in line with our standards described at …	chipx86	Jan. 19, 2018, 1:52 p.m.
Make sure you close any open issues as you fix them in your code, so that you don't leave any …	chipx86	Jan. 19, 2018, 1:53 p.m.
E501 line too long (140 > 79 characters)	reviewbot	Jan. 20, 2018, 6:40 a.m.
E501 line too long (84 > 79 characters)	reviewbot	Jan. 20, 2018, 6:40 a.m.
W293 blank line contains whitespace	reviewbot	Jan. 20, 2018, 6:40 a.m.
W293 blank line contains whitespace	reviewbot	Jan. 20, 2018, 6:40 a.m.
E501 line too long (86 > 79 characters)	reviewbot	Jan. 20, 2018, 6:40 a.m.
W291 trailing whitespace	reviewbot	Jan. 20, 2018, 6:40 a.m.
W293 blank line contains whitespace	reviewbot	Jan. 20, 2018, 6:40 a.m.
E501 line too long (107 > 79 characters)	reviewbot	Jan. 20, 2018, 6:40 a.m.
E101 indentation contains mixed spaces and tabs	reviewbot	Jan. 20, 2018, 6:40 a.m.
W191 indentation contains tabs	reviewbot	Jan. 20, 2018, 6:40 a.m.
W291 trailing whitespace	reviewbot	Jan. 20, 2018, 6:40 a.m.
W191 indentation contains tabs	reviewbot	Jan. 20, 2018, 6:40 a.m.
W191 indentation contains tabs	reviewbot	Jan. 20, 2018, 6:40 a.m.
W291 trailing whitespace	reviewbot	Jan. 20, 2018, 6:40 a.m.
I know the old code was like this, but there should be a blank line before the start of a …	chipx86	Jan. 19, 2018, 12:23 p.m.
No blank line here.	chipx86	Jan. 19, 2018, 12:23 p.m.
Much more can fit on each line.	chipx86	Jan. 19, 2018, 12:24 p.m.
This doesn't match the docstring format for our tests. It should be more like the old docstring. They're in this …	chipx86	Jan. 19, 2018, 12:26 p.m.
Ideally, we should always use single quotes. We can also import the logger we care about directly from the module, …	chipx86	Jan. 19, 2018, 12:29 p.m.
Instead of calls[-1], use last_call.	chipx86	Jan. 19, 2018, 12:29 p.m.

flake8 failed.

JSHint passed.

flake8

djblets/datagrid/grids.py (Diff revision 1)
Show all issues
```
E501 line too long (140 > 79 characters)
```
djblets/datagrid/tests.py (Diff revision 1)
Show all issues
```
E501 line too long (84 > 79 characters)
```
djblets/datagrid/tests.py (Diff revision 1)
Show all issues
```
W293 blank line contains whitespace
```
djblets/datagrid/tests.py (Diff revision 1)
Show all issues
```
W293 blank line contains whitespace
```
djblets/datagrid/tests.py (Diff revision 1)
Show all issues
```
E501 line too long (86 > 79 characters)
```
djblets/datagrid/tests.py (Diff revision 1)
Show all issues
```
W291 trailing whitespace
```
djblets/datagrid/tests.py (Diff revision 1)
Show all issues
```
W293 blank line contains whitespace
```

Change Summary:

Fixed flake8 errors

Commit:

-	d074501728857ad187cbbc25c21a5d491cfbe007
+	4f4b644b8a7ac63bae53351847306d05fd64d103

Diff:

Revision 2 (+23 -8)

Show changes

	djblets/datagrid/grids.py
	djblets/datagrid/tests.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

djblets/datagrid/grids.py (Diff revision 2)
Show all issues
```
E501 line too long (107 > 79 characters)
```
djblets/datagrid/tests.py (Diff revision 2)
Show all issues
```
E101 indentation contains mixed spaces and tabs
```
djblets/datagrid/tests.py (Diff revision 2)
Show all issues
```
W191 indentation contains tabs
```
djblets/datagrid/tests.py (Diff revision 2)
Show all issues
```
W291 trailing whitespace
```
djblets/datagrid/tests.py (Diff revision 2)
Show all issues
```
W191 indentation contains tabs
```
djblets/datagrid/tests.py (Diff revision 2)
Show all issues
```
W191 indentation contains tabs
```
djblets/datagrid/tests.py (Diff revision 2)
Show all issues
```
W291 trailing whitespace
```

Commit:

-	4f4b644b8a7ac63bae53351847306d05fd64d103
+	453afb2a308bc8b4a3c3f06a76d6d3565371a3b2

Diff:

Revision 3 (+24 -8)

Show changes

	djblets/datagrid/grids.py
	djblets/datagrid/tests.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Fix it!

Show all issues

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.username

In 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.)

rpolyano Jan. 19, 2018, 1:38 p.m.

Is this allowed currently? It does not appear to be implemented as self.get_column('owner.username') returns None (and is thus ignored) in /r/?sort=owner.username

Show all issues

The bug number should go in the Bugs field, instead of having a link in the description.

Show all issues

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/.

Show all issues

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.

djblets/datagrid/grids.py (Diff revision 3)

Show all issues

I know the old code was like this, but there should be a blank line before the start of a new block (like for).

djblets/datagrid/grids.py (Diff revision 3)
Show all issues
```
No blank line here.
```
djblets/datagrid/grids.py (Diff revision 3)
Show all issues
```
Much more can fit on each line.
```

djblets/datagrid/tests.py (Diff revision 3)

Show all issues

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.

djblets/datagrid/tests.py (Diff revision 3)

Show all issues

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.

djblets/datagrid/tests.py (Diff revision 3)
Show all issues
```
Instead of calls[-1], use last_call.
```

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 called `get_full_name` exists).

~
	~	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_column` on 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.

Bugs:

+	4607

Diff:

Revision 4 (+24 -8)

Show changes

	djblets/datagrid/grids.py
	djblets/datagrid/tests.py

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_column` on 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_column` on 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.

Commit:

-	453afb2a308bc8b4a3c3f06a76d6d3565371a3b2
+	f1b1f82a0b3dae6a4e90e91455911f4e7e01c301

Diff:

Revision 5 (+22 -8)

Show changes

	djblets/datagrid/grids.py
	djblets/datagrid/tests.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Review Board 6.0 beta 3 (dev)

Added filtering of sorting columns that are not actual database fields (e.x. fullname)

flake8

Change Summary:

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commit:

Diff:

Checks run (2 succeeded)

Description:

Testing Done:

Bugs:

Diff:

Checks run (2 succeeded)

Testing Done:

Commit:

Diff:

Checks run (2 succeeded)

Status: Discarded