Review Board: Make all columns sortable on /users/ page

Review Request #10277 — Created Oct. 25, 2018 and updated

gojeffcho
Review Board
release-4.0.x
10278
10278
2555e5a...
reviewboard, students

This fix makes all three columns of the /users/ page,
UsernameColumn, FullNameColumn, and PendingCountColumn, sortable
using their values. Previously, only UsernameColumn was sortable.

In order to implement this, an override method for get_sort_field()
was added to FullNameColumn which returns the first_name and
last_name as a tuple. A related change in the Djblets side enables
handling of tuples as returns from get_sort_field().

Two unit tests were added to test sorting of the test users in both
ascending and descending order using custom first and last names.

All tests pass for Review Board, except "Testing PerforceTool.connect
sets required client args", which can be disregarded for now.

  • 6
  • 0
  • 11
  • 1
  • 18
Description From Last Updated
You do not need Review Board: in your summary since it will land in RB (and shows repository: Review Board ... brennie brennie
Your code only affects the FullNameColumn, so I would suggest slightly re-wording the summary to make that clear, e.g. Make ... brennie brennie
I think the first sentence of your description is a bit confusing (I had to read it a couple times ... shoven shoven
It might be more clear to simply say Retrieve the first name and last name fields. shoven shoven
A 2-tuple containing the fields to use to sort a user by their full name. brennie brennie
It might be more clear to simply say Tuple containing the first name and last name fields. shoven shoven
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

gojeffcho
gojeffcho
gojeffcho
chipx86
  1. 
      
  2. reviewboard/datagrids/columns.py (Diff revision 3)
     
     
     
     

    First line of Returns needs to be tuple:

  3. reviewboard/datagrids/tests.py (Diff revision 3)
     
     
     
     
     
     
     

    We can probably get rid of this test. There's no logic besides a fixed return value in get_sort_field, and it won't change unless we want it to, in which case this test also has to be changed. Tests that test something that can be impacted by other changes are more valuable.

  4. reviewboard/datagrids/tests.py (Diff revision 3)
     
     

    No period at the end of unit test docstrings.

  5. reviewboard/datagrids/tests.py (Diff revision 3)
     
     
     

    """ should be on the previous line.

    Also, no period at the end of unit test docstrings.

  6. reviewboard/datagrids/tests.py (Diff revision 3)
     
     
     
     
     
     

    A few things of note here:

    1. The context object here is technically a ContextList object, lets you access individual keys without worrying about which sub-context it's in. So when we know what the exact key is that we want to look up, we can just grab it.
    2. It's not really safe or efficient to loop through all items in a context. We can get way too many false-positives through that. So that ties into #1 as well.
    3. In general, it's better to collapse nested if statements like this, but we probably don't need that first one if we follow #1 and #2.

    What object are those if statements meant to compare with? If we pull that out and check it, this will be a much simpler, faster, safer test.

  7. reviewboard/datagrids/tests.py (Diff revision 3)
     
     
     

    Same docstring comments apply here.

  8. 
      
gojeffcho
gojeffcho
Review request changed

Change Summary:

Using custom users for the unit tests.

Commit:

-b2db0ba5bb944e2cd56f43e3a4471199ce2968ef
+2d7fb99cca66e79276c3f5f30d8e18af7a5c25fc

Diff:

Revision 5 (+75 -1)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

gojeffcho
Review request changed

Change Summary:

Added the test_users back in because they are set globally by BaseColumnTestCase.

Commit:

-2d7fb99cca66e79276c3f5f30d8e18af7a5c25fc
+af056b2b8d2929f14d41759a29d0bf2fc1b6c6d3

Diff:

Revision 6 (+87 -1)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

gojeffcho
gojeffcho
gojeffcho
skaefer143
  1. The reviewboard/datagrids/tests.py file runs all tests successfully, when using the associated djblets patch.

  2. reviewboard/datagrids/grids.py (Diff revision 8)
     
     

    Since we're making this column sortable as well, there should be a unit test testing this as well. That way, you know that it's sorting the way you expect it to.

  3. 
      
gojeffcho
Review request changed

Change Summary:

Remove sorting on pending column - this was for my testing and is not required for this patch.

Commit:

-911ada96158f740b135a80e4f7d08230ee96054f
+2555e5a3c6c4abd384ec52b24b8dd3f8315469a4

Diff:

Revision 9 (+67 -1)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
brennie
  1. 
      
  2. You do not need Review Board: in your summary since it will land in RB (and shows repository: Review Board in details).

  3. Your code only affects the FullNameColumn, so I would suggest slightly re-wording the summary to make that clear, e.g.

    Make sotrable the full name column on user datagrids

    This is because another column may be added in the future that may not be sortable and could lead to a confusing git blame.

  4. reviewboard/datagrids/columns.py (Diff revision 9)
     
     

    A 2-tuple containing the fields to use to sort a user by their full name.

  5. 
      
shoven
  1. 
      
  2. I think the first sentence of your description is a bit confusing (I had to read it a couple times to make sense of it, I think because you split up the sentence with the names of the three columns so by the time I get to "sortable" I forgot what the subject of the sentence was).

    You could simplify this to be "This fix makes all three columns of the /users/ page sortable by first and last name (the columns include UsernameColumn, FullNameColumn and PendingCountColumn)."

  3. reviewboard/datagrids/columns.py (Diff revision 9)
     
     

    It might be more clear to simply say Retrieve the first name and last name fields.

  4. reviewboard/datagrids/columns.py (Diff revision 9)
     
     

    It might be more clear to simply say Tuple containing the first name and last name fields.

  5. 
      
Loading...