Make fullname column sortable on /users/ page

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

Information

Review Board
release-4.0.x
2555e5a...

Reviewers

This fix makes the FullNameColumn column of the /users/ page
sortable. 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.

Description From Last Updated

You do not need Review Board: in your summary since it will land in RB (and shows repository: Review Board …

brenniebrennie

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

brenniebrennie

I think the first sentence of your description is a bit confusing (I had to read it a couple times …

shovenshoven

E303 too many blank lines (2)

reviewbotreviewbot

E501 line too long (90 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

First line of Returns needs to be tuple:

chipx86chipx86

We can probably get rid of this test. There's no logic besides a fixed return value in get_sort_field, and it …

chipx86chipx86

No period at the end of unit test docstrings.

chipx86chipx86

""" should be on the previous line. Also, no period at the end of unit test docstrings.

chipx86chipx86

A few things of note here: The context object here is technically a ContextList object, lets you access individual keys …

chipx86chipx86

Same docstring comments apply here.

chipx86chipx86

F841 local variable 'fixtures' is assigned to but never used

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

Since we're making this column sortable as well, there should be a unit test testing this as well. That way, …

skaefer143skaefer143

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

shovenshoven

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

brenniebrennie

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

shovenshoven

It might be clearer to say "Testing sorting of FullNameColumn in ascending order"

bolariinwabolariinwa
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
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. 
      
bolariinwa
  1. 
      
  2. reviewboard/datagrids/tests.py (Diff revision 9)
     
     

    It might be clearer to say "Testing sorting of FullNameColumn in ascending order"

  3. 
      
gojeffcho
gojeffcho
gojeffcho
Review request changed

Change Summary:

Summary title fix.

Summary:

-Make all columns sortable on /users/ page
+Make fullname column sortable on /users/ page
Loading...