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)
     
     
     
     
    Show all issues

    First line of Returns needs to be tuple:

  3. reviewboard/datagrids/tests.py (Diff revision 3)
     
     
     
     
     
     
     
    Show all issues

    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)
     
     
    Show all issues

    No period at the end of unit test docstrings.

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

    """ should be on the previous line.

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

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

    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)
     
     
     
    Show all issues

    Same docstring comments apply here.

  8. 
      
gojeffcho
gojeffcho
Review request changed
Change Summary:

Using custom users for the unit tests.

Commit:
b2db0ba5bb944e2cd56f43e3a4471199ce2968ef
2d7fb99cca66e79276c3f5f30d8e18af7a5c25fc

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

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)
     
     
    Show all issues

    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. Show all issues

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

  3. Show all issues

    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)
     
     
    Show all issues

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

  5. 
      
shoven
  1. 
      
  2. Show all issues

    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)
     
     
    Show all issues

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

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

    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)
     
     
    Show all issues

    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