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

[WIP] Order owners alphabetically #6259

wants to merge 1 commit into
base: dev


Copy link

loic-sharma commented Aug 2, 2018

Part of #6258

⚠️ TODO:

  • The manage owners page
  • Verify on DEV that this package's owners are ordered.

@loic-sharma loic-sharma changed the title Order owners on display package page [WIP] Order owners alphabetically Aug 3, 2018

foreach (var owner in Model.Owners.OrderBy(o => o.Username, StringComparer.InvariantCultureIgnoreCase))

This comment has been minimized.


chenriksson Aug 3, 2018


Should we OrderBy from the view model, so that any views that work with owners get the same ordering?

This comment has been minimized.


loic-sharma Aug 3, 2018

Author Member

I considered that, but the view model does queries off of the owners field (for example, see HasSingleOwner). This would affect all these queries as well.

This comment has been minimized.


scottbommarito Aug 3, 2018


I think as long as you do a .ToList() when you OrderBy in the view model there shouldn't be any impact on the other queries. Also then we'd get the added bonus that all of those queries will then be ordered as well if we show them in the UI.

This comment has been minimized.


shishirx34 Aug 3, 2018


HasSingleOwner does a Union operation and that would kill of any orderby done previously. We will need to make sure we do orderby at all places where you are returning owners list. I would leave it as is if there are whole lot of places where you might have to order usernames.


This comment has been minimized.

Copy link
Member Author

loic-sharma commented Mar 13, 2019

Closing this PR as it has gotten stale and we have no plans of finishing this work in the short term

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.