FIX: un-prioritise inactive users in user search (#11838)

When doing a user search (eg. when mentioning a user) we will not prioritie
users who hasn't been seen in over a year.

REFACTOR the user-search specs to be more precise regarding the ordering

app/models/user_search.rb

@@ -64,10 +64,11 @@ class UserSearch
   def search_ids
     users = Set.new
 
-    # 1. exact username matches
+    # 1. exact username matches active in the past year
     if @term.present?
       scoped_users
         .where(username_lower: @term)
+        .where('last_seen_at > ?', 1.year.ago)
         .limit(@limit)
         .pluck(:id)
         .each { |id| users << id }

spec/models/user_search_spec.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-require 'rails_helper'
+require "rails_helper"
 
 describe UserSearch do
 
@@ -11,170 +11,163 @@ describe UserSearch do
   fab!(:topic2)    { Fabricate :topic }
   fab!(:topic3)    { Fabricate :topic }
   fab!(:topic4)    { Fabricate :topic }
-  fab!(:user1)     { Fabricate :user, username: "mrb", name: "Michael Madsen", last_seen_at: 10.days.ago }
+  fab!(:mr_b)      { Fabricate :user, username: "mrb",      name: "Michael Madsen",    last_seen_at: 10.days.ago }
-  fab!(:user2)     { Fabricate :user, username: "mrblue",   name: "Eddie Code", last_seen_at: 9.days.ago }
+  fab!(:mr_blue)   { Fabricate :user, username: "mrblue",   name: "Eddie Code",        last_seen_at: 9.days.ago }
-  fab!(:user3)     { Fabricate :user, username: "mrorange", name: "Tim Roth", last_seen_at: 8.days.ago }
+  fab!(:mr_orange) { Fabricate :user, username: "mrorange", name: "Tim Roth",          last_seen_at: 8.days.ago }
-  fab!(:user4)     { Fabricate :user, username: "mrpink",   name: "Steve Buscemi",  last_seen_at: 7.days.ago }
+  fab!(:mr_pink)   { Fabricate :user, username: "mrpink",   name: "Steve Buscemi",     last_seen_at: 7.days.ago }
-  fab!(:user5)     { Fabricate :user, username: "mrbrown",  name: "Quentin Tarantino", last_seen_at: 6.days.ago }
+  fab!(:mr_brown)  { Fabricate :user, username: "mrbrown",  name: "Quentin Tarantino", last_seen_at: 6.days.ago }
-  fab!(:user6)     { Fabricate :user, username: "mrwhite",  name: "Harvey Keitel",  last_seen_at: 5.days.ago }
+  fab!(:mr_white)  { Fabricate :user, username: "mrwhite",  name: "Harvey Keitel",     last_seen_at: 5.days.ago }
-  fab!(:inactive) { Fabricate :user, username: "Ghost", active: false }
+  fab!(:inactive)  { Fabricate :user, username: "Ghost", active: false }
   fab!(:admin)     { Fabricate :admin, username: "theadmin" }
   fab!(:moderator) { Fabricate :moderator, username: "themod" }
   fab!(:staged)    { Fabricate :staged }
 
   def search_for(*args)
-    UserSearch.new(*args).search
+    # mapping "username" so it's easier to debug
+    UserSearch.new(*args).search.map(&:username)
   end
 
-  context 'with a secure category' do
+  context "with a secure category" do
-    fab!(:group) { Fabricate :group }
+    fab!(:user) { Fabricate(:user) }
-    fab!(:user) { Fabricate :user }
+    fab!(:searching_user) { Fabricate(:user) }
-    fab!(:searching_user) { Fabricate :user }
+    fab!(:group) { Fabricate(:group) }
+    fab!(:category) { Fabricate(:category, read_restricted: true, user: user) }
+
     before_all do
+      Fabricate(:category_group, category: category, group: group)
+
       group.add(user)
       group.add(searching_user)
       group.save
     end
-    fab!(:category) { Fabricate(:category,
-                        read_restricted: true,
-                        user: user)
-    }
-    before_all { Fabricate(:category_group, category: category, group: group) }
 
-    it 'autocompletes with people in the category' do
+    it "autocompletes with people in the category" do
       results = search_for("", searching_user: searching_user, category_id: category.id)
-
+      expect(results).to eq [user.username]
-      expect(user.username).to eq(results[0].username)
-      expect(results.length).to eq(1)
     end
 
-    it 'will lookup the category from the topic id' do
+    it "will lookup the category from the topic id" do
       topic = Fabricate(:topic, category: category)
-      _post = Fabricate(:post, user: topic.user, topic: topic)
+      Fabricate(:post, user: topic.user, topic: topic)
 
       results = search_for("", searching_user: searching_user, topic_id: topic.id)
 
-      expect(results.length).to eq(2)
+      expect(results).to eq [topic.user, user].map(&:username)
-
-      expect(results.map(&:username)).to contain_exactly(
-        user.username, topic.user.username
-      )
     end
 
-    it 'will raise an error if the user cannot see the category' do
+    it "will raise an error if the user cannot see the category" do
       expect do
         search_for("", searching_user: Fabricate(:user), category_id: category.id)
       end.to raise_error(Discourse::InvalidAccess)
     end
 
-    it 'will respect the group member visibility setting' do
+    it "will respect the group member visibility setting" do
       group.update(members_visibility_level: Group.visibility_levels[:owners])
       results = search_for("", searching_user: searching_user, category_id: category.id)
-      expect(results.length).to eq(0)
+      expect(results).to be_blank
 
       group.add_owner(searching_user)
       results = search_for("", searching_user: searching_user, category_id: category.id)
-      expect(results.length).to eq(1)
+      expect(results).to eq [user.username]
     end
 
   end
 
-  it 'allows for correct underscore searching' do
+  it "allows for correct underscore searching" do
-    Fabricate(:user, username: 'Under_Score')
+    Fabricate(:user, username: "undertaker")
-    Fabricate(:user, username: 'undertaker')
+    under_score = Fabricate(:user, username: "Under_Score")
 
-    expect(search_for("under_sc").length).to eq(1)
+    expect(search_for("under_sc")).to eq [under_score.username]
-    expect(search_for("under_").length).to eq(1)
+    expect(search_for("under_")).to eq [under_score.username]
   end
 
-  it 'allows filtering by group' do
+  it "allows filtering by group" do
+    sam = Fabricate(:user, username: "sam")
+    Fabricate(:user, username: "samantha")
+
     group = Fabricate(:group)
-    sam = Fabricate(:user, username: 'sam')
-    _samantha = Fabricate(:user, username: 'samantha')
     group.add(sam)
 
     results = search_for("sam", groups: [group])
-    expect(results.count).to eq(1)
+    expect(results).to eq [sam.username]
   end
 
-  it 'allows filtering by multiple groups' do
+  it "allows filtering by multiple groups" do
+    sam = Fabricate(:user, username: "sam")
+    samantha = Fabricate(:user, username: "samantha")
+
     group_1 = Fabricate(:group)
-    sam = Fabricate(:user, username: 'sam')
-    group_2 = Fabricate(:group)
-    samantha = Fabricate(:user, username: 'samantha')
     group_1.add(sam)
+
+    group_2 = Fabricate(:group)
     group_2.add(samantha)
 
     results = search_for("sam", groups: [group_1, group_2])
-    expect(results.count).to eq(2)
+    expect(results).to eq [sam, samantha].map(&:username)
   end
 
   context "with seed data" do
-    fab!(:post1) { Fabricate :post, user: user1, topic: topic }
+    fab!(:post1) { Fabricate :post, user: mr_b, topic: topic }
-    fab!(:post2) { Fabricate :post, user: user2, topic: topic2 }
+    fab!(:post2) { Fabricate :post, user: mr_blue, topic: topic2 }
-    fab!(:post3) { Fabricate :post, user: user3, topic: topic }
+    fab!(:post3) { Fabricate :post, user: mr_orange, topic: topic }
-    fab!(:post4) { Fabricate :post, user: user4, topic: topic }
+    fab!(:post4) { Fabricate :post, user: mr_pink, topic: topic }
-    fab!(:post5) { Fabricate :post, user: user5, topic: topic3 }
+    fab!(:post5) { Fabricate :post, user: mr_brown, topic: topic3 }
-    fab!(:post6) { Fabricate :post, user: user6, topic: topic }
+    fab!(:post6) { Fabricate :post, user: mr_white, topic: topic }
     fab!(:post7) { Fabricate :post, user: staged, topic: topic4 }
 
-    before { user6.update(suspended_at: 1.day.ago, suspended_till: 1.year.from_now) }
+    before { mr_white.update(suspended_at: 1.day.ago, suspended_till: 1.year.from_now) }
 
     it "can search by name and username" do
       # normal search
-      results = search_for(user1.name.split(" ").first)
+      results = search_for(mr_b.name.split.first)
-      expect(results.size).to eq(1)
+      expect(results).to eq [mr_b.username]
-      expect(results.first.username).to eq(user1.username)
 
       # lower case
-      results = search_for(user1.name.split(" ").first.downcase)
+      results = search_for(mr_b.name.split.first.downcase)
-      expect(results.size).to eq(1)
+      expect(results).to eq [mr_b.username]
-      expect(results.first).to eq(user1)
 
       # username
-      results = search_for(user4.username)
+      results = search_for(mr_pink.username)
-      expect(results.size).to eq(1)
+      expect(results).to eq [mr_pink.username]
-      expect(results.first).to eq(user4)
 
       # case insensitive
-      results = search_for(user4.username.upcase)
+      results = search_for(mr_pink.username.upcase)
-      expect(results.size).to eq(1)
+      expect(results).to eq [mr_pink.username]
-      expect(results.first).to eq(user4)
     end
 
     it "handles substring search correctly" do
-      # substrings
-      # only staff members see suspended users in results
       results = search_for("mr")
-      expect(results.size).to eq(5)
+      expect(results).to eq [mr_brown, mr_pink, mr_orange, mr_blue, mr_b].map(&:username)
-      expect(results).not_to include(user6)
+
-      expect(search_for("mr", searching_user: user1).size).to eq(5)
+      results = search_for("mr", searching_user: mr_b)
+      expect(results).to eq [mr_brown, mr_pink, mr_orange, mr_blue, mr_b].map(&:username)
+
+      # only staff members see suspended users in results
+      results = search_for("mr", searching_user: moderator)
+      expect(results).to eq [mr_white, mr_brown, mr_pink, mr_orange, mr_blue, mr_b].map(&:username)
 
       results = search_for("mr", searching_user: admin)
-      expect(results.size).to eq(6)
+      expect(results).to eq [mr_white, mr_brown, mr_pink, mr_orange, mr_blue, mr_b].map(&:username)
-      expect(results).to include(user6)
-      expect(search_for("mr", searching_user: moderator).size).to eq(6)
 
-      results = search_for(user1.username, searching_user: admin)
+      results = search_for(mr_b.username, searching_user: admin)
-      expect(results.size).to eq(3)
+      expect(results).to eq [mr_b, mr_brown, mr_blue].map(&:username)
 
       results = search_for("MR", searching_user: admin)
-      expect(results.size).to eq(6)
+      expect(results).to eq [mr_white, mr_brown, mr_pink, mr_orange, mr_blue, mr_b].map(&:username)
 
       results = search_for("MRB", searching_user: admin, limit: 2)
-      expect(results.size).to eq(2)
+      expect(results).to eq [mr_b, mr_brown].map(&:username)
     end
 
     it "prioritises topic participants" do
-      # topic priority
+      results = search_for(mr_b.username, topic_id: topic.id)
-      results = search_for(user1.username, topic_id: topic.id)
+      expect(results).to eq [mr_b, mr_brown, mr_blue].map(&:username)
-      expect(results.first).to eq(user1)
 
-      results = search_for(user1.username, topic_id: topic2.id)
+      results = search_for(mr_b.username, topic_id: topic2.id)
-      expect(results[1]).to eq(user2)
+      expect(results).to eq [mr_b, mr_blue, mr_brown].map(&:username)
 
-      results = search_for(user1.username, topic_id: topic3.id)
+      results = search_for(mr_b.username, topic_id: topic3.id)
-      expect(results[1]).to eq(user5)
+      expect(results).to eq [mr_b, mr_brown, mr_blue].map(&:username)
     end
 
     it "only reveals topic participants to people with permission" do
@@ -187,37 +180,46 @@ describe UserSearch do
 
       # Random user, does not have access
       expect do
-        search_for("", topic_id: pm_topic.id, searching_user: user1)
+        search_for("", topic_id: pm_topic.id, searching_user: mr_b)
       end.to raise_error(Discourse::InvalidAccess)
 
-      pm_topic.invite(pm_topic.user, user1.username)
+      pm_topic.invite(pm_topic.user, mr_b.username)
-      results = search_for("", topic_id: pm_topic.id, searching_user: user1)
+
-      expect(results.length).to eq(1)
+      results = search_for("", topic_id: pm_topic.id, searching_user: mr_b)
-      expect(results[0]).to eq(pm_topic.user)
+      expect(results).to eq [pm_topic.user.username]
     end
 
     it "only searches by name when enabled" do
       # When searching by name is enabled, it returns the record
       SiteSetting.enable_names = true
       results = search_for("Tarantino")
-      expect(results.size).to eq(1)
+      expect(results).to eq [mr_brown.username]
 
       results = search_for("coding")
-      expect(results.size).to eq(0)
+      expect(results).to be_blank
 
       results = search_for("z")
-      expect(results.size).to eq(0)
+      expect(results).to be_blank
 
       # When searching by name is disabled, it will not return the record
       SiteSetting.enable_names = false
       results = search_for("Tarantino")
-      expect(results.size).to eq(0)
+      expect(results).to be_blank
     end
 
     it "prioritises exact matches" do
-      # find an exact match first
       results = search_for("mrB")
-      expect(results.first.username).to eq(user1.username)
+      expect(results).to eq [mr_b, mr_brown, mr_blue].map(&:username)
+    end
+
+    it "doesn't prioritises exact matches for users who haven't been seen in more than 1 year" do
+      abcdef = Fabricate(:user, username: "abcdef", last_seen_at: 2.days.ago)
+      abcde  = Fabricate(:user, username: "abcde", last_seen_at: 2.weeks.ago)
+      abcd   = Fabricate(:user, username: "abcd", last_seen_at: 2.months.ago)
+      abc    = Fabricate(:user, username: "abc", last_seen_at: 2.years.ago)
+
+      results = search_for("abc")
+      expect(results).to eq [abcdef, abcde, abcd, abc].map(&:username)
     end
 
     it "does not include self, staged or inactive" do
@@ -230,12 +232,11 @@ describe UserSearch do
       expect(results).to be_blank
 
       results = search_for(staged.username, include_staged_users: true)
-      expect(results.first.username).to eq(staged.username)
+      expect(results).to eq [staged.username]
 
-      results = search_for("", topic_id: topic.id, searching_user: user1)
+      # mrb is omitted since they're the searching user
-
+      results = search_for("", topic_id: topic.id, searching_user: mr_b)
-      # mrb is omitted, mrb is current user
+      expect(results).to eq [mr_pink, mr_orange].map(&:username)
-      expect(results.map(&:username)).to eq(["mrpink", "mrorange"])
     end
   end
 end