User directory items are sorted by some activity metric. If those metrics have the same value, postgres does not guarantee the order in which they will be returned. This can cause issues in pagination - some users may appear twice, and some may be missed. To illustrate
```
pry(main)> query = DirectoryItem.where(period_type: DirectoryItem.period_types[:weekly]).order(:likes_received).limit(50);
pry(main)> page1 = query.offset(0).pluck(:id);
pry(main)> page2 = query.offset(50).pluck(:id);
pry(main)> (page1 & page2).count # users on both pages
=> 29
```
If we use the primary key to tie-break matching metrics, things are much more reliable
```
pry(main)> query = DirectoryItem.where(period_type: DirectoryItem.period_types[:weekly]).order(:likes_received, :id).limit(50);
pry(main)> page1 = query.offset(0).pluck(:id);
pry(main)> page2 = query.offset(50).pluck(:id);
pry(main)> (page1 & page2).count # users on both pages
=> 0
```
This most commonly effects new sites where all the directory metrics are zero.
The fact that the ordering is indeterminate makes it difficult to write a reliable test case for this.
At the moment, when filtering by group, the directory will unconditionally return the current user at the top of the list. This is quite unexpected, given that the user is deliberately trying to filter the list. This commit makes sure the 'include current user' logic only triggers for unfiltered directories
The `/directory_items` route needs to have a .json url, but the rails
url helper `_path` doesn't return the format of the route.
I tried passing in a format options to `directory_items_path`. Which
works in the rails console
```
[8] pry(main)> directory_items_path(params.merge(:format => :json))
=> "/directory_items.json?page=1"
```
but when I added that some logic to the controller it comes out as
```
/directory_items?format=json&page=1
```
(which is actually how I expect it to work based on how you pass in the
format param). Anyways, because I couldn't figure out how to pass a
format to the `_path` helper I just used URI.parse to append `.json`
manually.
Doing .pluck(:column).first is a very common pattern in Discourse and in
most cases, a limit cause isn't being added. Instead of adding a limit
clause to all these callsites, this commit adds two new methods to
ActiveRecord::Relation:
pluck_first, equivalent to limit(1).pluck(*columns).first
and pluck_first! which, like other finder methods, raises an exception
when no record is found
This reduces chances of errors where consumers of strings mutate inputs
and reduces memory usage of the app.
Test suite passes now, but there may be some stuff left, so we will run
a few sites on a branch prior to merging
- Move user directory from `/directory` to `/users/`
- Defaults to 'weekly' time period
- Don't include deleted topics/posts in the results
- Move heart icon to header instead of on each row
- "Users" instead of "Users found"
David Taylor
jahan-ggn
Blake Erickson
Vinoth Kannan
Robin Ward
Bianca Nenciu
Daniel Waterworth
Sam Saffron
Maja Komel
Régis Hanol
Joe Buhlig
Gerhard Schlager
Guo Xiang Tan
Guo Xiang Tan
junwen yang