Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[stable10] Sort user list by display name #31599

Closed
wants to merge 1 commit into from

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented May 31, 2018

Description

Sort the user list on the webUI by display name (full name)

Related Issue

#1948 (comment)

Motivation and Context

I think other people have asked for the users in the users list to be sorted by display name.
It is not difficult, but there are gotchas.

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@phil-davis phil-davis self-assigned this May 31, 2018
@phil-davis
Copy link
Contributor Author

phil-davis commented May 31, 2018

The issues here are:

  1. Users are delivered by the server in case-sensitive order:
  • users are actually returned in "batches" of (currently) 200 to the UI. Those are actually retrieved by the display_name field (case-sensitive), so the first 200 Aaron Adrian Anne... come in the first batch. More batches come as you scroll down. In later batches aaron adrian anne... (if there are any) come after Zoe.
  • so you might be looking for anne but you do not find her early in the list.
  • you scroll down past Zoe
  • now anne comes, but the JavaScript sort here sorts anne case-insensitive in among Aaron Adrian Anne... at the top of the list. I guess you can now scroll back up and see that both Anne and anne are listed.

A "solution" to this would be not to sort at all - display in the order the server delivers. Or to make the JavaScript sort here match the server-sorted order. Or add a lower_display_name field on the server so that it can do an SQL order by that is case-insensitive.

  1. Different admins will want the list sorted in different orders - for that, local sorting by column could be implemented.

@codecov
Copy link

codecov bot commented May 31, 2018

Codecov Report

Merging #31599 into stable10 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             stable10   #31599   +/-   ##
===========================================
  Coverage       60.46%   60.46%           
  Complexity      18225    18225           
===========================================
  Files            1195     1195           
  Lines           71867    71867           
  Branches         1248     1248           
===========================================
  Hits            43453    43453           
  Misses          28044    28044           
  Partials          370      370
Flag Coverage Δ Complexity Δ
#javascript 52.64% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 61.31% <ø> (ø) 18225 <ø> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d765b1...65845d9. Read the comment docs.

@phil-davis phil-davis changed the title Sort user list by display name [stable10] Sort user list by display name May 31, 2018
@phil-davis phil-davis requested a review from PVince81 May 31, 2018 09:45
@phil-davis phil-davis added this to the development milestone May 31, 2018
@phil-davis
Copy link
Contributor Author

phil-davis commented May 31, 2018

Actually I think this change at least makes things better than the current behavior.
Currently the first 200 users are delivered to the webUI, ordered by case-insensitive display name. Then the webUI "kindly" sorts those into uid order. That can be very confusing, because the admin will see UIDs with a whole range of values (maybe from A-Z). Then when they scroll down, another 200 users are loaded, by display name, and then sorted into uid order.

@phil-davis
Copy link
Contributor Author

Whatever happens here, it needs to be done to the user_management app also - for running with core master

@PVince81
Copy link
Contributor

PVince81 commented Jun 1, 2018

@pmaier1 are we ok with this change of behavior in the user management page ? also see #31599 (comment)

@PVince81 PVince81 requested a review from pmaier1 June 1, 2018 10:20
@pmaier1
Copy link
Contributor

pmaier1 commented Jun 1, 2018

Users are delivered by the server in case-sensitive order

Having to scroll to "Z" first is not really nice when you look for "a" or don't know that "a" exists, I think. For this I would prefer case-insensitive ordering.
How is the list ordered currently? Edit: Ah, you explained in #31599 (comment). Agree if the ordering is case-insensitive.

Different admins will want the list sorted in different orders - for that, local sorting by column could be implemented.

Yes, there are FRs for this. I would love to get this. Might still conflict with the pagination.

@phil-davis
Copy link
Contributor Author

The Javascript ordering on the webUI is case-insensitive, and locale-aware (sorts Unicode letters with umlaut, grave, accent etc correctly together. (A and a and variants of "a" are alongside each other...).

The server, unfortunately, delivers batches in case-sensitive order. That is current server behavior. To get the server to deliver in case-insensitive order could be done by adding a lower_display_name field to the table, and then orderBy('lower_display_name') - but that is a database migration! And then ideally the server would send batches in the same sort order as the JavaScript ``localeCompare()``` collation order. But that could be Swedish, Finnish, German, English... that all have slightly different alphabetical sorting. And SQL is not going to cope with that! It is a general problem when a server tries to deliver pre-sorted batches of data to a client, and wants to deliver in the same order that the client wants them sorted.

@mmattel
Copy link
Contributor

mmattel commented Jun 11, 2018

This has been created for stable10 first.
Forward porting intended ?

@phil-davis
Copy link
Contributor Author

"forward port" in this case means applying the same change in the user_management app.
This code does not exist in core master

@mmattel
Copy link
Contributor

mmattel commented Jun 11, 2018

question, pls excuse if stupid, but having the change here and possibly in UM, when the UM app gets activated, doesnt this conflict or will many things now beeing in core moved to UM so core gets cleaned up?

btw: the UM repository needs the nice pope picture too - missing any description...

@phil-davis
Copy link
Contributor Author

In core master the User Management page has been separated out in its own app. So whenever the content of core master is released (core v10.1 or v11.0 however the version numbering works out depending on the "semver" content of the release), it will be without any User Management page. The removal of the User Management page is not being backported anywhere. So old branches or core still have the User Management page in them - e.g. 10.0.* 9.1.* 9.0.. Any fixxes for the User Management page need to be applied in those branches to get to installations that still run those branches. It might be clearer to understand IF there was still support for 9.0l. and 9.1.* - in that case we would make small fixes in each of those branches also...

There will be no conflicts reported anywhere. Because the "old code" in 10.0.* and the "new code" in the user_management app are in completely different repos, they will not "notice" each other in git. So actually we have to be very careful to "manually" make the correct fixes in both places.

@mmattel
Copy link
Contributor

mmattel commented Jun 12, 2018

@phil-davis thanks for the good explanation. Could you create some readme.md text in the user_management repo like you describe it here? (currently blank). That would really help.

@phil-davis
Copy link
Contributor Author

@PVince81
Copy link
Contributor

PVince81 commented Jun 15, 2018

First: it seems that the linked comment #1948 (comment) is based on a use case for mixed LDAP users and regular users, so the admin needs to scroll down. We could say that in this one use case, the need for "sort by display name or anything meaningful but not those horrible LDAP ids" is more acute.

In a regular usage scenario we should provide both sort options by allowing the admin to click on the columns. There is some value in keeping sorting by user id because often times admins work rather with user ids, especially in situations where display names repeat many many times (see how many Thomas Müller exist in Germany) but email addresses / user ids might be different.

At this point instead of changing this behavior and risking alienating other admins who might rely on the user id, I'd rather have this done properly with clickable column (feature request here: #26170)

That said and as you already noticed, there are some technical issues with sorting currently. I've already documented a few here: #7244 #26170 (reopened)

@phil-davis
Copy link
Contributor Author

The trick is to tell the server how you want stuff sorted, so that the server can give it to you in batches that are in the order you are hoping for.

When you change the sort order on the webUI, the webUI needs to ask the server for data in the new order. e.g. you currently have in userid order students s000001 to s000600 displayed. Now you change the sort order to "display name", "Aaron Aardvark" is userid "s001000" and so is not in your current local list at all, but he should now be first in the list. So you have to re-fetch the data, unless you know that you have the full list already loaded.

And for "popular" sort orders, there needs to be an index on the relevant database table(s). And yes, one of my first sort orders to ask for as an admin would be by quota used and/or quota remaining - to quickly find those users who used the most space, and those users who are closest to running out of quota. That will be a different challenge vs database indexes.

@PVince81
Copy link
Contributor

@phil-davis making server the authority on sorting here makes sense.

We need to make the server side read from oc_accounts and sort accordingly.

A long time ago before the accounts table it would need to query multiple user backends and then sort them, and then extract whatever page you needed, which was deemed to tricky/hacky. With the account table this might work better. Added comment on #26170 (comment)

@phil-davis
Copy link
Contributor Author

The current "easy" sort-by-name server-side with database table index is not quite what people (whoever they are) might want, since it is just a "raw" index on the display-name field. Thus it is not case-insensitive, it might sort letters with grave, accent etc. out of the order that people like/expect. So there will be a bit of thought to put in about how to index the accounts table in a suitable way the both "all databases" can do "internally/efficiently" and that is closest to "what the end-user likes".

@PVince81
Copy link
Contributor

@phil-davis the tricky part is having a cross-DB solution that provides natural sort. Last I checked (a few years ago) this wasn't possible without changing the collation.
The workaround would be to sort on PHP level with natural sort, but then you lose pagination consistency...

@PVince81
Copy link
Contributor

Closing this PR.

We first need to sort out the server side vs client side sorting. Then provide an option to switch sorting between both columns in the UI.

@PVince81
Copy link
Contributor

for the server side issue, proposal here: #26170 (comment)

the same ticket can be used to track the "sort by clicked column" idea

@PVince81 PVince81 deleted the stable10-sort-by-displayName branch June 28, 2018 10:11
@lock
Copy link

lock bot commented Jul 30, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jul 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants