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

FIX: limited search results #4981

Merged
merged 7 commits into from Aug 1, 2017

Conversation

dmacjam
Copy link
Contributor

@dmacjam dmacjam commented Jul 21, 2017

If there are more full-page search results available, inform users about that.

  • Added flag indicating if there are more search results (by querying for limit + 1).
  • Added support for backend search pagination (using offset), which will be needed for infinite loading of more search results.
  • Added infinite loading of search results to advanced search frontend - in total 10 pages of 50 results are loaded when scrolling which results in 500 search results (not more because of the offset performance). If there are still more results, it tells users to narrow down their search criteria.

Related discussion:
https://meta.discourse.org/t/results-missing-from-search/64969
https://meta.discourse.org/t/search-results-limited-to-50-each-time/34270

@discoursebot
Copy link

You've signed the CLA, dmacjam. Thank you! This pull request is ready for review.

@SamSaffron
Copy link
Member

The tests need to be a bit smarter here, they should ensure the offset is correct by checking to see that when you ask for page 2 you get proper page 2 results in expected order.


expect(results.posts.length).to eq(number_of_results)
expect(results.more_full_page_results).to eq(true)
expect(results2.posts.length).to eq(number_of_results)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add check here that post ids aren't same as the first page. This test will pass if the page param is ignored I think.

@dmacjam
Copy link
Contributor Author

dmacjam commented Jul 26, 2017

Added infinite loading of search results and rspec tests for pagination are fixed. Thanks for suggestion.

@@ -158,6 +162,11 @@ export default Ember.Controller.extend({
return iconHTML(expanded ? "caret-down" : "caret-right");
},

@computed('page')
isLastPage(page) {
return page == PAGE_LIMIT;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be === instead of ==. js tests are failing.

@dmacjam dmacjam force-pushed the fix_limited_search_results branch from 968eb06 to f8edf26 Compare July 31, 2017 10:16
@discoursebot
Copy link

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/search-results-limited-to-50-each-time/34270/12

@coding-horror
Copy link
Contributor

when can we merge this?

@nlalonde nlalonde merged commit fa3c240 into discourse:master Aug 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants