Make fullname column sortable on /users/ page
Review Request #10277 — Created Oct. 25, 2018 and updated
This fix makes the
FullNameColumncolumn of the/users/page
sortable. Previously, onlyUsernameColumnwas sortable.In order to implement this, an override method for
get_sort_field()
was added toFullNameColumnwhich returns thefirst_nameand
last_nameas a tuple. A related change in the Djblets side enables
handling of tuples as returns fromget_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 … |
 brennie
|
|
|
Your code only affects the FullNameColumn, so I would suggest slightly re-wording the summary to make that clear, e.g. Make … |
brennie
|
|
|
I think the first sentence of your description is a bit confusing (I had to read it a couple times … |
 shoven
|
|
|
E303 too many blank lines (2) |
 reviewbot
|
|
|
E501 line too long (90 > 79 characters) |
reviewbot
|
|
|
E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
First line of Returns needs to be tuple: |
 chipx86
|
|
|
We can probably get rid of this test. There's no logic besides a fixed return value in get_sort_field, and it … |
chipx86
|
|
|
No period at the end of unit test docstrings. |
chipx86
|
|
|
""" should be on the previous line. Also, no period at the end of unit test docstrings. |
chipx86
|
|
|
A few things of note here: The context object here is technically a ContextList object, lets you access individual keys … |
chipx86
|
|
|
Same docstring comments apply here. |
chipx86
|
|
|
F841 local variable 'fixtures' is assigned to but never used |
reviewbot
|
|
|
E303 too many blank lines (2) |
reviewbot
|
|
|
Since we're making this column sortable as well, there should be a unit test testing this as well. That way, … |
 skaefer143
|
|
|
It might be more clear to simply say Retrieve the first name and last name fields. |
shoven
|
|
|
A 2-tuple containing the fields to use to sort a user by their full name. |
brennie
|
|
|
It might be more clear to simply say Tuple containing the first name and last name fields. |
shoven
|
|
|
It might be clearer to say "Testing sorting of FullNameColumn in ascending order" |
 bolariinwa
|
- Change Summary:
-
Added 10278 as a dependency for this fix.
- Depends On:
-
- Commit:
4734a074748f80fc9e8dd5942c28274f6969af6f2f0b8deff5afe698dfce9f47a12eb6758587221d- Diff:
Revision 2 (+18 -1)
Checks run (2 succeeded)
flake8 passed.JSHint passed.
- Change Summary:
-
Added unit tests for sort on ascending and descending order.
- Commit:
-
2f0b8deff5afe698dfce9f47a12eb6758587221d1ce22bffe2c443531c2b23b2e8974c6980734200
Checks run (2 succeeded)
- Change Summary:
-
Description update.
- Description:
-
This fix makes all three columns of the
/users/page,UsernameColumn,FullNameColumn, andPendingCountColumn, sortableusing their values. Previously, only UsernameColumnwas sortable.In order to implement this, an override method for
get_sort_field()was added to FullNameColumnwhich returns thefirst_nameandlast_nameas a tuple. A related change in the Djblets side enableshandling of tuples as returns from get_sort_field().~ Unit tests still need to be written for this fix.
~ Two unit tests were added to test sorting of the test users in both
+ ascending and descending order.
-
-
-
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. -
-
-
A few things of note here:
- The context object here is technically a
ContextListobject, 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. - 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.
- In general, it's better to collapse nested
ifstatements like this, but we probably don't need that first one if we follow #1 and #2.
What object are those
ifstatements meant to compare with? If we pull that out and check it, this will be a much simpler, faster, safer test. - The context object here is technically a
-
- Change Summary:
-
Fixing Christian's suggestions
- Commit:
-
1ce22bffe2c443531c2b23b2e8974c6980734200b2db0ba5bb944e2cd56f43e3a4471199ce2968ef
Checks run (2 succeeded)
- Change Summary:
-
Using custom users for the unit tests.
- Commit:
-
b2db0ba5bb944e2cd56f43e3a4471199ce2968ef2d7fb99cca66e79276c3f5f30d8e18af7a5c25fc
- Change Summary:
-
Added the test_users back in because they are set globally by
BaseColumnTestCase. - Commit:
-
2d7fb99cca66e79276c3f5f30d8e18af7a5c25fcaf056b2b8d2929f14d41759a29d0bf2fc1b6c6d3
- Change Summary:
-
Took the test_users right back out. All tests pass.
- Commit:
-
af056b2b8d2929f14d41759a29d0bf2fc1b6c6d3020337f203b2607aa46a6f53deb3f93df3368415
Checks run (2 succeeded)
- Change Summary:
-
Now out of WIP.
- Summary:
-
[WIP] Review Board: Make all columns sortable on /users/ pageReview Board: Make all columns sortable on /users/ page
- Description:
-
This fix makes all three columns of the
/users/page,UsernameColumn,FullNameColumn, andPendingCountColumn, sortableusing their values. Previously, only UsernameColumnwas sortable.In order to implement this, an override method for
get_sort_field()was added to FullNameColumnwhich returns thefirst_nameandlast_nameas a tuple. A related change in the Djblets side enableshandling 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. ~ ascending and descending order using custom first and last names.
- Change Summary:
-
Reverted, now updating test_users' first_name and last_name instead of deleting them or creating new users.
- Commit:
-
020337f203b2607aa46a6f53deb3f93df3368415911ada96158f740b135a80e4f7d08230ee96054f
Checks run (2 succeeded)
- Change Summary:
-
Remove sorting on pending column - this was for my testing and is not required for this patch.
- Commit:
-
911ada96158f740b135a80e4f7d08230ee96054f2555e5a3c6c4abd384ec52b24b8dd3f8315469a4
Checks run (2 succeeded)
-
-
You do not need
Review Board:in your summary since it will land in RB (and showsrepository: Review Boardin details). -
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. -
-
-
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,FullNameColumnandPendingCountColumn)." -
-
- Summary:
-
Review Board: Make all columns sortable on /users/ pageMake all columns sortable on /users/ page
- Description:
-
~ This fix makes all three columns of the
/users/page,~ UsernameColumn,FullNameColumn, andPendingCountColumn, sortable~ This fix makes the
FullNameColumncolumn of the/users/page~ sortable. Previously, only UsernameColumnwas sortable.- using their values. Previously, only UsernameColumnwas sortable.In order to implement this, an override method for
get_sort_field()was added to FullNameColumnwhich returns thefirst_nameandlast_nameas a tuple. A related change in the Djblets side enableshandling 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.