Merge pull request #4981 from dmacjam/fix_limited_search_results

FIX: limited search results

app/assets/javascripts/discourse/controllers/full-page-search.js.es6

@@ -14,12 +14,13 @@ const SortOrders = [
   {name: I18n.t('search.latest_topic'), id: 4, term: 'order:latest_topic'},
 
 ];
+const PAGE_LIMIT = 10;
 
 export default Ember.Controller.extend({
   application: Ember.inject.controller(),
   bulkSelectEnabled: null,
 
-  loading: Em.computed.not("model"),
+  loading: false,
   queryParams: ["q", "expanded", "context_id", "context", "skip_context"],
   q: null,
   selected: [],
@@ -30,11 +31,8 @@ export default Ember.Controller.extend({
   sortOrder: 0,
   sortOrders: SortOrders,
   invalidSearch: false,
-
-  @computed('model.posts')
-  resultCount(posts) {
-    return posts && posts.length;
-  },
+  page: 1,
+  resultCount: null,
 
   @computed('resultCount')
   hasResults(resultCount) {
@@ -113,6 +111,7 @@ export default Ember.Controller.extend({
   @observes('sortOrder')
   triggerSearch() {
     if (this._searchOnSortChange) {
+      this.set("page", 1);
       this._search();
     }
   },
@@ -143,6 +142,11 @@ export default Ember.Controller.extend({
     this.set("application.showFooter", !this.get("loading"));
   },
 
+  @observes('model.posts.length')
+  resultCountChanged() {
+    this.set("resultCount", this.get("model.posts.length"));
+  },
+
   @computed('hasResults')
   canBulkSelect(hasResults) {
     return this.currentUser && this.currentUser.staff && hasResults;
@@ -158,6 +162,11 @@ export default Ember.Controller.extend({
     return iconHTML(expanded ? "caret-down" : "caret-right");
   },
 
+  @computed('page')
+  isLastPage(page) {
+    return page === PAGE_LIMIT;
+  },
+
   _search() {
     if (this.get("searching")) { return; }
 
@@ -169,10 +178,11 @@ export default Ember.Controller.extend({
     }
 
     this.set("searching", true);
+    this.set("loading", true);
     this.set('bulkSelectEnabled', false);
     this.get('selected').clear();
 
-    var args = { q: searchTerm };
+    var args = { q: searchTerm, page: this.get('page') };
 
     const sortOrder = this.get("sortOrder");
     if (sortOrder && SortOrders[sortOrder].term) {
@@ -180,7 +190,6 @@ export default Ember.Controller.extend({
     }
 
     this.set("q", args.q);
-    this.set("model", null);
 
     const skip = this.get("skip_context");
     if ((!skip && this.get('context')) || skip==="false"){
@@ -199,9 +208,20 @@ export default Ember.Controller.extend({
         this.set('q', results.grouped_search_result.term);
       }
 
-      setTransient('lastSearch', { searchKey, model }, 5);
-      this.set("model", model);
-    }).finally(() => this.set("searching", false));
+      if(args.page > 1){
+        if (model){
+          this.get("model").posts.pushObjects(model.posts);
+          this.get("model").topics.pushObjects(model.topics);
+          this.get("model").set('grouped_search_result', results.grouped_search_result);
+        }
+      }else{
+        setTransient('lastSearch', { searchKey, model }, 5);
+        this.set("model", model);
+      }
+    }).finally(() => {
+      this.set("searching", false);
+      this.set("loading", false);
+    });
   },
 
   actions: {
@@ -227,11 +247,20 @@ export default Ember.Controller.extend({
     },
 
     search() {
+      this.set("page", 1);
       this._search();
     },
 
     toggleAdvancedSearch() {
       this.toggleProperty('expanded');
-    }
+    },
+
+    loadMore() {
+      var page = this.get('page');
+      if (this.get('model.grouped_search_result.more_full_page_results') && !this.get("loading") && page < PAGE_LIMIT){
+        this.incrementProperty("page");
+        this._search();
+      }
+    },
   }
 });

app/assets/javascripts/discourse/templates/full-page-search.hbs

@@ -45,32 +45,23 @@
     </div>
   {{/if}}
 
-  {{#conditional-loading-spinner condition=loading}}
-
-    {{#unless hasResults}}
-      <h3>
-        {{#if searchActive}}
-          {{i18n "search.no_results"}}
-        {{/if}}
-      </h3>
-    {{/unless}}
-
-    {{#if hasResults}}
-      <div class='search-title clearfix'>
-        <div class='result-count'>
-          <span>
-            {{{i18n "search.result_count" count=resultCount term=noSortQ}}}
-          </span>
-        </div>
-        <div class='sort-by'>
-          <span class='desc'>
-            {{i18n "search.sort_by"}}
-          </span>
-          {{combo-box value=sortOrder content=sortOrders castInteger="true"}}
-        </div>
+  {{#if hasResults}}
+    <div class='search-title clearfix'>
+      <div class='result-count'>
+        <span>
+          {{{i18n "search.result_count" count=resultCount term=noSortQ}}}
+        </span>
       </div>
-    {{/if}}
+      <div class='sort-by'>
+        <span class='desc'>
+          {{i18n "search.sort_by"}}
+        </span>
+        {{combo-box value=sortOrder content=sortOrders castInteger="true"}}
+      </div>
+    </div>
+  {{/if}}
 
+  {{#load-more selector=".fps-result" action="loadMore"}}
     {{#each model.posts as |result|}}
       <div class='fps-result'>
         <div class='author'>
@@ -124,11 +115,30 @@
       </div>
     {{/each}}
 
-    {{#if hasResults}}
-      <h3 class="search-footer">
-        {{i18n "search.no_more_results"}}
-      </h3>
-    {{/if}}
+    {{#conditional-loading-spinner condition=loading }}
+      {{#unless hasResults}}
+        <h3>
+          {{#if searchActive}}
+              {{i18n "search.no_results"}}
+          {{/if}}
+        </h3>
+      {{/unless}}
+
+      {{#if hasResults}}
+        {{#unless loading}}
+          <h3 class="search-footer">
+            {{#if model.grouped_search_result.more_full_page_results}}
+              {{#if isLastPage }}
+                {{i18n "search.more_results"}}
+              {{/if}}
+            {{else}}
+              {{i18n "search.no_more_results"}}
+            {{/if}}
+          </h3>
+        {{/unless}}
+      {{/if}}
+    {{/conditional-loading-spinner}}
+
+  {{/load-more}}
 
-  {{/conditional-loading-spinner}}
 {{/d-section}}

app/controllers/search_controller.rb

@@ -13,7 +13,10 @@ class SearchController < ApplicationController
       type_filter: 'topic',
       guardian: guardian,
       include_blurbs: true,
-      blurb_length: 300
+      blurb_length: 300,
+      page: if params[:page].to_i <= 10
+              [params[:page].to_i, 1].max
+            end
     }
 
     context, type = lookup_search_context

app/serializers/grouped_search_result_serializer.rb

@@ -2,7 +2,7 @@ class GroupedSearchResultSerializer < ApplicationSerializer
   has_many :posts, serializer: SearchPostSerializer
   has_many :users, serializer: SearchResultUserSerializer
   has_many :categories, serializer: BasicCategorySerializer
-  attributes :more_posts, :more_users, :more_categories, :term, :search_log_id
+  attributes :more_posts, :more_users, :more_categories, :term, :search_log_id, :more_full_page_results
 
   def search_log_id
     object.search_log_id

config/locales/client.en.yml

@@ -1341,6 +1341,7 @@ en:
       searching: "Searching ..."
       post_format: "#{{post_number}} by {{username}}"
       results_page: "Search Results"
+      more_results: "There are more results. Please narrow your search criteria."
 
       context:
         user: "Search posts by @{{username}}"

lib/search.rb

@@ -155,8 +155,8 @@ class Search
     @search_context = @opts[:search_context]
     @include_blurbs = @opts[:include_blurbs] || false
     @blurb_length = @opts[:blurb_length]
-    @limit = Search.per_facet
     @valid = true
+    @page = @opts[:page]
 
     # Removes any zero-width characters from search terms
     term.to_s.gsub!(/[\u200B-\u200D\uFEFF]/, '')
@@ -174,10 +174,6 @@ class Search
       @search_context = @guardian.user
     end
 
-    if @opts[:type_filter].present?
-      @limit = Search.per_filter
-    end
-
     @results = GroupedSearchResults.new(
       @opts[:type_filter],
       clean_term,
@@ -187,6 +183,22 @@ class Search
     )
   end
 
+  def limit
+    if @opts[:type_filter].present?
+      Search.per_filter + 1
+    else
+      Search.per_facet + 1
+    end
+  end
+
+  def offset
+    if @page && @opts[:type_filter].present?
+      (@page - 1) * Search.per_filter
+    else
+      0
+    end
+  end
+
   def valid?
     @valid
   end
@@ -557,7 +569,6 @@ class Search
         raise Discourse::InvalidAccess.new("invalid type filter") unless Search.facets.include?(@results.type_filter)
         send("#{@results.type_filter}_search")
       else
-        @limit = Search.per_facet + 1
         unless @search_context
           user_search if @term.present?
           category_search if @term.present?
@@ -753,6 +764,7 @@ class Search
         posts = posts.where("(categories.id IS NULL) OR (NOT categories.read_restricted)").references(:categories)
       end
 
+      posts = posts.offset(offset)
       posts.limit(limit)
     end
 
@@ -795,10 +807,10 @@ class Search
       query =
         if @order == :likes
           # likes are a pain to aggregate so skip
-          posts_query(@limit, private_messages: opts[:private_messages])
+          posts_query(limit, private_messages: opts[:private_messages])
             .select('topics.id', "posts.post_number")
         else
-          posts_query(@limit, aggregate_search: true, private_messages: opts[:private_messages])
+          posts_query(limit, aggregate_search: true, private_messages: opts[:private_messages])
             .select('topics.id', "#{min_or_max}(posts.post_number) post_number")
             .group('topics.id')
         end
@@ -846,7 +858,7 @@ class Search
 
     def topic_search
       if @search_context.is_a?(Topic)
-        posts = posts_eager_loads(posts_query(@limit))
+        posts = posts_eager_loads(posts_query(limit))
           .where('posts.topic_id = ?', @search_context.id)
 
         posts.each do |post|

lib/search/grouped_search_results.rb

@@ -19,7 +19,8 @@ class Search
       :more_users,
       :term,
       :search_context,
-      :include_blurbs
+      :include_blurbs,
+      :more_full_page_results
     )
 
     attr_accessor :search_log_id
@@ -50,7 +51,9 @@ class Search
     def add(object)
       type = object.class.to_s.downcase.pluralize
 
-      if !@type_filter.present? && send(type).length == Search.per_facet
+      if @type_filter.present? && send(type).length == Search.per_filter
+        @more_full_page_results = true
+      elsif !@type_filter.present? && send(type).length == Search.per_facet
         instance_variable_set("@more_#{type}".to_sym, true)
       else
         (send type) << object

spec/components/search_spec.rb

@@ -843,4 +843,39 @@ describe Search do
     end
   end
 
+  context 'pagination' do
+    let(:number_of_results) { 2 }
+    let!(:post1) { Fabricate(:post, raw: 'hello hello hello hello hello')}
+    let!(:post2) { Fabricate(:post, raw: 'hello hello hello hello')}
+    let!(:post3) { Fabricate(:post, raw: 'hello hello hello')}
+    let!(:post4) { Fabricate(:post, raw: 'hello hello')}
+    let!(:post5) { Fabricate(:post, raw: 'hello')}
+    before do
+      Search.stubs(:per_filter).returns(number_of_results)
+    end
+
+    it 'returns more results flag' do
+      results = Search.execute('hello', type_filter: 'topic')
+      results2 = Search.execute('hello', type_filter: 'topic', page: 2)
+
+      expect(results.posts.length).to eq(number_of_results)
+      expect(results.posts.map(&:id)).to eq([post1.id, post2.id])
+      expect(results.more_full_page_results).to eq(true)
+      expect(results2.posts.length).to eq(number_of_results)
+      expect(results2.posts.map(&:id)).to eq([post3.id, post4.id])
+      expect(results2.more_full_page_results).to eq(true)
+    end
+
+    it 'correctly search with page parameter' do
+      search = Search.new('hello', type_filter: 'topic', page: 3)
+      results = search.execute
+
+      expect(search.offset).to eq(2 * number_of_results)
+      expect(results.posts.length).to eq(1)
+      expect(results.posts).to eq([post5])
+      expect(results.more_full_page_results).to eq(nil)
+    end
+
+  end
+
 end