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

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

gojeffcho
Djblets
release-2.0.x
4607
8867d05...
djblets, students

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.

  • 0
  • 0
  • 18
  • 0
  • 18
Description From Last Updated
gojeffcho
chipx86
  1. 
      
  2. djblets/datagrid/grids.py (Diff revision 2)
     
     
     

    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)
     
     

    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)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    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

Diff:

Revision 3 (+39 -1)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

gojeffcho
chipx86
  1. 
      
  2. djblets/datagrid/grids.py (Diff revision 4)
     
     
     

    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)
     
     

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

    Same with the others below.

  4. 
      
gojeffcho
brennie
  1. 
      
  2. Can you wrap your testing done a 72 characters as well?

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

    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)
     
     

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

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

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

    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)
     
     

    nit: no blank line here.

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

    nit: no blank line here.

  4. 
      
gojeffcho
david
  1. Just a couple nits:

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

    Add a blank line between these two.

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

    Please use single quotes instead of double.

  4. 
      
gojeffcho
brennie
  1. 
      
  2. nit: Your summary does not need a period.

  3. 
      
gojeffcho
Review request changed

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
brennie
  1. Ship It!
  2. 
      
Loading...