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

Review Request #10168 — Created Sept. 22, 2018 and submitted

Information

Djblets
release-2.0.x
8867d05...

Reviewers

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.

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.

Description From Last Updated

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

brenniebrennie

Something to note is that, while this says these specific test suites passed, you're actually going to need to run …

chipx86chipx86

nit: Your summary does not need a period.

brenniebrennie

These should use get_sort_field, since it's not guaranteed to be db_field. (As a heads up, and you probably know this …

chipx86chipx86

We don't really use .format() in our codebase. Instead, we generally use %-formatted strings. I know this seems backwards, depending …

chipx86chipx86

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

chipx86chipx86

E501 line too long (83 > 79 characters)

reviewbotreviewbot

E501 line too long (84 > 79 characters)

reviewbotreviewbot

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) …

brenniebrennie

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

chipx86chipx86

You don't need the parents around column.get_sort_field()

brenniebrennie

The trailing """ should always be on its own line. Same with the others below.

chipx86chipx86

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

brenniebrennie

Something that occurs to me... We should explicitly check to make sure get_sort_field() returned a value, so we don't subtly …

chipx86chipx86

nit: no blank line here.

brenniebrennie

nit: no blank line here.

brenniebrennie

Add a blank line between these two.

daviddavid

Please use single quotes instead of double.

daviddavid
gojeffcho
chipx86
  1. 
      
  2. djblets/datagrid/grids.py (Diff revision 2)
     
     
     
    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.)

  3. djblets/datagrid/grids.py (Diff revision 2)
     
     
    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.

  4. djblets/datagrid/tests.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

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

  5. 
      
gojeffcho
Review request changed
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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

gojeffcho
chipx86
  1. 
      
  2. djblets/datagrid/grids.py (Diff revision 4)
     
     
     
    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.

  3. djblets/datagrid/tests.py (Diff revision 4)
     
     
    Show all issues

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

    Same with the others below.

  4. 
      
gojeffcho
brennie
  1. 
      
  2. Show all issues

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

  3. djblets/datagrid/grids.py (Diff revision 4)
     
     
     
     
    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)
    
  4. djblets/datagrid/grids.py (Diff revision 4)
     
     
    Show all issues

    You don't need the parents around column.get_sort_field()

  5. 
      
gojeffcho
gojeffcho
brennie
  1. 
      
  2. djblets/datagrid/grids.py (Diff revision 6)
     
     
    Show all issues

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

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

  2. 
      
gojeffcho
chipx86
  1. 
      
  2. 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.

  3. djblets/datagrid/grids.py (Diff revision 8)
     
     
     
     
    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.

  4. 
      
gojeffcho
brennie
  1. 
      
  2. djblets/datagrid/grids.py (Diff revisions 8 - 9)
     
     
    Show all issues

    nit: no blank line here.

  3. djblets/datagrid/grids.py (Diff revisions 8 - 9)
     
     
    Show all issues

    nit: no blank line here.

  4. 
      
gojeffcho
david
  1. Just a couple nits:

  2. djblets/datagrid/grids.py (Diff revision 10)
     
     
     
    Show all issues

    Add a blank line between these two.

  3. djblets/datagrid/grids.py (Diff revision 10)
     
     
    Show all issues

    Please use single quotes instead of double.

  4. 
      
gojeffcho
brennie
  1. 
      
  2. Show all issues

    nit: Your summary does not need a period.

  3. 
      
gojeffcho
brennie
  1. Ship It!
  2. 
      
david
  1. Ship It!
  2. 
      
gojeffcho
Review request changed
Status:
Completed
Change Summary:
Pushed to release-1.0.x (4a094ea)