Fix sorting users by last login date #14947
Conversation
| SearchOrderByForksReverse SearchOrderBy = "num_forks DESC" | ||
| // user | ||
| SearchOrderByRecentLogin SearchOrderBy = "last_login_unix ASC" | ||
| SearchOrderByRecentLoginReverse SearchOrderBy = "last_login_unix DESC" |
There was a problem hiding this comment.
should these be here or in models/user.go? I think it's neat to have them all grouped together
There was a problem hiding this comment.
I already refactored them. So no worry now.
GiteaBot
added
the
lgtm/need 2
| @@ -12,6 +12,8 @@ | |||
| <a class="{{if eq .SortType "reversealphabetically"}}active{{end}} item" href="{{$.Link}}?sort=reversealphabetically&q={{$.Keyword}}">{{.i18n.Tr "repo.issues.label.filter_sort.reverse_alphabetically"}}</a> | |||
| <a class="{{if eq .SortType "recentupdate"}}active{{end}} item" href="{{$.Link}}?sort=recentupdate&q={{$.Keyword}}">{{.i18n.Tr "repo.issues.filter_sort.recentupdate"}}</a> | |||
| <a class="{{if eq .SortType "leastupdate"}}active{{end}} item" href="{{$.Link}}?sort=leastupdate&q={{$.Keyword}}">{{.i18n.Tr "repo.issues.filter_sort.leastupdate"}}</a> | |||
| <a class="{{if eq .SortType "recentlogin"}}active{{end}} item" href="{{$.Link}}?sort=recentlogin&q={{$.Keyword}}">{{.i18n.Tr "repo.issues.filter_sort.recentlogin"}}</a> | |||
| <a class="{{if eq .SortType "recentloginreverse"}}active{{end}} item" href="{{$.Link}}?sort=recentloginreverse&q={{$.Keyword}}">{{.i18n.Tr "repo.issues.filter_sort.recentloginreverse"}}</a> | |||
There was a problem hiding this comment.
Not sure if we should add specific sorters like this to the dropdown, it is used for orgs listing as well where it doesn't apply.
In general it's a bit weird, that this template is shared by user & org listing, while an almost identical search template explore/search.tmpl exists, that actually would benefit from being shared between admin & explore listing..
Opinions?
There was a problem hiding this comment.
Yes, please share templates if possible, we need to reduce duplication. What you could do is something like we already do for issue lists:
This would merge a additional listType=dashboard property into the root context ., making it available to the sub-template. Keep note that mergeinto does clone the context once, so it'll induce a tiny performance hit, thought that should be okay for one-time rendered things like these lists.
There was a problem hiding this comment.
We do not need the shared templates anymore. See the latest code.
And shared templates bring more problems than they resolve.
|
I think this problem can be easily fixed based on the recent code base. ps: do we need database index for |
|
It's inactive for more than one year, it could be replaced by #22081 |
On the admin user listing, sorting users by last login date was actually sorting by
last_updated_unix.As the orgs listing doesn't have a last login column, I dropped the broken sorter there (thinking about adding more sort options in a followup).
fixes #14942