Summary

Only sort a DataGrid if the column's sortable is set True

Review Request #10168 — Created Sept. 22, 2018 and submitted Nov. 12, 2018, 11:53 a.m.

Information

Owner

gojeffcho

Repository

Djblets

Branch

release-2.0.x

Bugs

4607

Depends On

Commit

8867d05...

Reviewers

Groups

djblets, students

People

Description

Previously, all arguments to self.request.GET.get('sort', ...) were
passed to self.sort_list even if they were invalid. This could cause
issues where columns collided with methods, such as when giving
?sort=fullname to /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_field is only added to
self.sort_list if 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 sort key and
checking that self.sort_list is empty.

Two additional unit tests sort on one valid and one invalid key in
ascending and descending order and check that self.sort_list only
contains the valid key. All four of these unit tests pass.

All tests in djblets.datagrid.tests and
reviewboard.datagrids.tests passed without problems.

Issues

Description	From	Last Updated
Can you wrap your testing done a 72 characters as well?	brennie	Sept. 22, 2018, 12:54 p.m.
Something to note is that, while this says these specific test suites passed, you're actually going to need to run …	chipx86	Oct. 1, 2018, 12:30 p.m.
nit: Your summary does not need a period.	brennie	Oct. 3, 2018, 6:13 p.m.
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	Sept. 22, 2018, 12:30 p.m.
We don't really use .format() in our codebase. Instead, we generally use %-formatted strings. I know this seems backwards, depending …	chipx86	Sept. 22, 2018, 12:30 p.m.
These all have the same docstring. The docstring should be unique in each case, describing the exact conditions being tested.	chipx86	Sept. 22, 2018, 12:30 p.m.
E501 line too long (83 > 79 characters)	reviewbot	Sept. 22, 2018, 12:31 p.m.
E501 line too long (84 > 79 characters)	reviewbot	Sept. 22, 2018, 12:31 p.m.
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	Sept. 22, 2018, 12:52 p.m.
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	Sept. 22, 2018, 12:45 p.m.
You don't need the parents around column.get_sort_field()	brennie	Sept. 22, 2018, 12:53 p.m.
The trailing """ should always be on its own line. Same with the others below.	chipx86	Sept. 22, 2018, 12:45 p.m.
Python variable names should be in lower_snake_case (yes thats really what it is called :) ), as per PEP 8.	brennie	Sept. 23, 2018, 6:48 a.m.
Something that occurs to me... We should explicitly check to make sure get_sort_field() returned a value, so we don't subtly …	chipx86	Oct. 1, 2018, 12:30 p.m.
nit: no blank line here.	brennie	Oct. 2, 2018, 9:39 a.m.
nit: no blank line here.	brennie	Oct. 2, 2018, 9:39 a.m.
Add a blank line between these two.	david	Oct. 3, 2018, 5:58 p.m.
Please use single quotes instead of double.	david	Oct. 3, 2018, 5:58 p.m.

flake8 passed.

JSHint passed.

Change Summary:

Committing the two additional unit tests.

Commit:

-	8760b3fececc2f07c4d7dfa2a72dccb3fa9ebe56
+	3afee531ea94d808441bb45cdcc60b08565be5a4

Diff:

Revision 2 (+37 -1)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

The issue has been resolved. Show all issues

These should use get_sort_field, since it's not guaranteed to be db_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.)

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

The issue has been resolved. Show all issues

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.

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

The issue has been resolved. Show all issues

These all have the same docstring. The docstring should be unique in each case, describing the exact conditions being tested.

Change Summary:

Use StatefulColumns; use get_sort_key() instead of db_field; use %-substitution; amend docstrings to be more descriptive and unique.

Commit:

-	3afee531ea94d808441bb45cdcc60b08565be5a4
+	e5c6d3d2563d86ab30f023adb598135b804193c8

Diff:

Revision 3 (+39 -1)

Show changes

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

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

djblets/datagrid/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E501 line too long (83 > 79 characters)
```
djblets/datagrid/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E501 line too long (84 > 79 characters)
```

Change Summary:

Fixed docstring length and wrapping.

Commit:

-	e5c6d3d2563d86ab30f023adb598135b804193c8
+	b12eb82def548898b965f11cc7aee0f88fcdf6f0

Diff:

Revision 4 (+41 -1)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

The issue has been resolved. Show all issues

Let's call column.get_sort_field() once, and reuse the value. That way, if the method does anything remotely expensive, it only has to do it once.

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

The issue has been resolved. Show all issues

The trailing """ should always be on its own line.

Same with the others below.

Change Summary:

Only call get_sort_field() once; fix docstring formatting.

Commit:

-	b12eb82def548898b965f11cc7aee0f88fcdf6f0
+	6a7a76e75794ba030d36aa510f2fd2577eee66c7

Diff:

Revision 5 (+46 -1)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

Can you wrap your testing done a 72 characters as well?

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

The issue has been resolved. Show all issues

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)
sort_keys.add('-%s' % sort_field)

djblets/datagrid/grids.py (Diff revision 4)
The issue has been resolved. Show all issues
```
You don't need the parents around column.get_sort_field()
```

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_list` even if they were invalid. This could cause issues where
~		columns collided with methods, such as when giving `?sort=fullname` to
~		`/users/` which resulted in an Error 500.
	~	Previously, all arguments to self.request.GET.get('sort', ...) were
	~	passed to `self.sort_list` even if they were invalid. This could cause
	~	issues where columns collided with methods, such as when giving
	~	`?sort=fullname` to `/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_field` is only added to `self.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_field` is only added to
	~	`self.sort_list` if 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 `sort` key and checking that `self.sort_list` is empty.
	~	Two unit tests have been added to exercise this functionality in
	+	ascending and descending order, passing in an invalid `sort` key and
	+	checking that `self.sort_list` is empty.

~		Two additional unit tests sort on one valid and one invalid key in ascending and descending order and check that `self.sort_list` only 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_list` only
	+	contains the valid key. All four of these unit tests pass.

Change Summary:

Removed unnecessary parentheses.

Commit:

-	6a7a76e75794ba030d36aa510f2fd2577eee66c7
+	1ec1bc6cffdce03e8b32108d959e115810112855

Diff:

Revision 6 (+46 -1)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

The issue has been resolved. Show all issues

Python variable names should be in lower_snake_case (yes thats really what it is called :) ), as per PEP 8.

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:

-	1ec1bc6cffdce03e8b32108d959e115810112855
+	52a4fdaacb460f2e5decd5c904f3622d6fdf68b0

Diff:

Revision 7 (+49 -3)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Accidentally left column.field_name insted of column.get_sort_field()

Commit:

-	52a4fdaacb460f2e5decd5c904f3622d6fdf68b0
+	5b414a19fa69b06027ffb4ecad52b754531346cb

Diff:

Revision 8 (+49 -3)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

This looks good to me! Once another mentor signs off we will land it.

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 `sort` key and
		checking that `self.sort_list` is empty.

		Two additional unit tests sort on one valid and one invalid key in
		ascending and descending order and check that `self.sort_list` only
		contains the valid key. All four of these unit tests pass.
	+
	+	All tests in `djblets.datagrid.tests` and
	+	`reviewboard.datagrids.tests` passed without problems.

The issue has been resolved. Show all issues

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.

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

The issue has been resolved. Show all issues

Something that occurs to me... We should explicitly check to make sure get_sort_field() returned a value, so we don't subtly break (add None=None and -None=None to the sort list) in the event that a column is sortable=True with custom logic that intentionally or unintentionally returns None.

You can do this with a simple if sort_field: before adding.

We'd also want a unit test to ensure None doesn't get in in any form.

Change Summary:

Updated unit tests.

Commit:

-	5b414a19fa69b06027ffb4ecad52b754531346cb
+	c865e3b4b5a514fc4e4e05e902fdd8f75dad15cb

Diff:

Revision 9 (+64 -8)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

djblets/datagrid/grids.py (Diff revisions 8 - 9)
The issue has been resolved. Show all issues
```
nit: no blank line here.
```
djblets/datagrid/grids.py (Diff revisions 8 - 9)
The issue has been resolved. Show all issues
```
nit: no blank line here.
```

Change Summary:

Removed blank lines.

Commit:

-	c865e3b4b5a514fc4e4e05e902fdd8f75dad15cb
+	fd0a4e7c527a17bfe4ca0e16c58471c774c7d3cb

Diff:

Revision 10 (+62 -8)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

```
Just a couple nits:
```
djblets/datagrid/grids.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Add a blank line between these two.
```
djblets/datagrid/grids.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Please use single quotes instead of double.
```

Change Summary:

Changed double quotes to single quotes, added linebreaks where requested.

Commit:

-	fd0a4e7c527a17bfe4ca0e16c58471c774c7d3cb
+	8867d05d9d23fb6b89bd9fc60a6d7e1b2aa5dc76

Diff:

Revision 11 (+67 -11)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues
```
nit: Your summary does not need a period.
```

Change Summary:

...

Summary:

-	Only sort a DataGrid if the column's sortable is set True.
+	Only sort a DataGrid if the column's sortable is set True

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-1.0.x (4a094ea)