From 4ab202ab0a1149ebef69d049de9d833e1b792e65 Mon Sep 17 00:00:00 2001 From: Molly Struve Date: Fri, 24 Apr 2020 09:30:17 -0500 Subject: [PATCH] [deploy] Execute Reading List searches in Elasticsearch (#7440) * Execute Reading List searches in Elasticsearch * add additional readinglist front tests --- app/assets/javascripts/base.js.erb | 1 - .../initializers/initScrolling.js.erb | 2 +- .../reading_list_items_controller.rb | 10 +-- app/javascript/.eslintrc.js | 1 - .../elements/__mocks__/algoliasearch.js | 62 ----------------- app/javascript/readingList/readingList.jsx | 19 ++---- .../searchableItemList/searchableItemList.js | 56 +++++++-------- .../src/components/ItemList/ItemListItem.jsx | 61 +++++++---------- .../ItemList/__tests__/ItemListItem.test.jsx | 68 ++++++++++--------- .../__snapshots__/ItemListItem.test.jsx.snap | 41 ----------- app/javascript/src/utils/algolia.js | 7 -- .../search/query_builders/reaction.rb | 4 +- spec/services/search/reaction_spec.rb | 2 +- 13 files changed, 101 insertions(+), 233 deletions(-) delete mode 100644 app/javascript/article-form/elements/__mocks__/algoliasearch.js delete mode 100644 app/javascript/src/utils/algolia.js diff --git a/app/assets/javascripts/base.js.erb b/app/assets/javascripts/base.js.erb index dbd0abe73..220e133cf 100644 --- a/app/assets/javascripts/base.js.erb +++ b/app/assets/javascripts/base.js.erb @@ -5,7 +5,6 @@ //= require utilities/getImageForLink //= require honeybadger-js/dist/honeybadger.js //= require serviceworker-companion -//= require algolia/v3/algoliasearch.min var instantClick , InstantClick = instantClick = function(document, location, $userAgent) { diff --git a/app/assets/javascripts/initializers/initScrolling.js.erb b/app/assets/javascripts/initializers/initScrolling.js.erb index ad86e213a..663d9d353 100644 --- a/app/assets/javascripts/initializers/initScrolling.js.erb +++ b/app/assets/javascripts/initializers/initScrolling.js.erb @@ -1,6 +1,6 @@ 'use strict'; -/* global algoliasearch, insertAfter, insertArticles, buildArticleHTML, nextPage:writable, fetching:writable, done:writable */ +/* global insertAfter, insertArticles, buildArticleHTML, nextPage:writable, fetching:writable, done:writable */ var client; diff --git a/app/controllers/reading_list_items_controller.rb b/app/controllers/reading_list_items_controller.rb index 211d0ce30..f17c1e813 100644 --- a/app/controllers/reading_list_items_controller.rb +++ b/app/controllers/reading_list_items_controller.rb @@ -2,7 +2,6 @@ class ReadingListItemsController < ApplicationController def index @reading_list_items_index = true set_view - generate_algolia_search_key end def update @@ -16,18 +15,11 @@ class ReadingListItemsController < ApplicationController private - def generate_algolia_search_key - params = { filters: "viewable_by:#{session_current_user_id}" } - @secured_algolia_key = Algolia.generate_secured_api_key( - ApplicationConfig["ALGOLIASEARCH_SEARCH_ONLY_KEY"], params - ) - end - def set_view @view = if params[:view] == "archive" "archived" else - "valid" + "valid,confirmed" end end end diff --git a/app/javascript/.eslintrc.js b/app/javascript/.eslintrc.js index c4b111c0b..8020fc0b8 100644 --- a/app/javascript/.eslintrc.js +++ b/app/javascript/.eslintrc.js @@ -45,7 +45,6 @@ module.exports = { InstantClick: false, filterXSS: false, Pusher: false, - algoliasearch: false, ga: false, Honeybadger: false, AndroidBridge: false, diff --git a/app/javascript/article-form/elements/__mocks__/algoliasearch.js b/app/javascript/article-form/elements/__mocks__/algoliasearch.js deleted file mode 100644 index 28693ea0b..000000000 --- a/app/javascript/article-form/elements/__mocks__/algoliasearch.js +++ /dev/null @@ -1,62 +0,0 @@ -global.document.head.innerHTML = - "" + - "" + - ""; - -const mockIndex = { - search: (query, _options) => - new Promise((resolve, _reject) => { - process.nextTick(() => { - const searchResults = { - gi: { - hits: [ - { - name: 'git', - bg_color_hex: '#888751', - text_color_hex: '#56c938', - hotness_score: 0, - supported: true, - objectID: '4', - _highlightResult: { - name: { - value: 'git', - matchLevel: 'full', - fullyHighlighted: false, - matchedWords: ['gi'], - }, - bg_color_hex: { - value: '#888751', - matchLevel: 'none', - matchedWords: [], - }, - text_color_hex: { - value: '#56c938', - matchLevel: 'none', - matchedWords: [], - }, - }, - }, - ], - nbHits: 1, - page: 0, - nbPages: 1, - hitsPerPage: 10, - processingTimeMS: 1, - exhaustiveNbHits: true, - query: 'gi', - params: - 'query=gi&hitsPerPage=10&filters=supported%3Atrue&restrictIndices=searchables_development%2CTag_development%2Cordered_articles_development%2Cordered_articles_by_published_at_development%2Cordered_articles_by_positive_reactions_count_development', - }, - }; - - const results = searchResults[query] || { hits: [] }; - - resolve(results); - }); - }), -}; -const client = { - initIndex: _index => mockIndex, -}; - -export default jest.fn().mockImplementation((_id, _key) => client); diff --git a/app/javascript/readingList/readingList.jsx b/app/javascript/readingList/readingList.jsx index 5d5fb2914..ff059640c 100644 --- a/app/javascript/readingList/readingList.jsx +++ b/app/javascript/readingList/readingList.jsx @@ -16,7 +16,7 @@ import { ItemListItemArchiveButton } from '../src/components/ItemList/ItemListIt import { ItemListLoadMoreButton } from '../src/components/ItemList/ItemListLoadMoreButton'; import { ItemListTags } from '../src/components/ItemList/ItemListTags'; -const STATUS_VIEW_VALID = 'valid'; +const STATUS_VIEW_VALID = 'valid,confirmed'; const STATUS_VIEW_ARCHIVED = 'archived'; const READING_LIST_ARCHIVE_PATH = '/readinglist/archive'; const READING_LIST_PATH = '/readinglist'; @@ -50,19 +50,14 @@ export class ReadingList extends Component { } componentDidMount() { - const { hitsPerPage, statusView } = this.state; + const { statusView } = this.state; this.performInitialSearch({ - containerId: 'reading-list', - indexName: 'SecuredReactions', - searchOptions: { - hitsPerPage, - filters: `status:${statusView}`, - }, + searchOptions: { status: `${statusView}` }, }); } - toggleStatusView = event => { + toggleStatusView = (event) => { event.preventDefault(); const { query, selectedTags } = this.state; @@ -174,12 +169,12 @@ export class ReadingList extends Component { const isStatusViewValid = this.statusViewValid(); const archiveButtonLabel = isStatusViewValid ? 'archive' : 'unarchive'; - const itemsToRender = items.map(item => { + const itemsToRender = items.map((item) => { return ( this.toggleArchiveStatus(e, item)} + onClick={(e) => this.toggleArchiveStatus(e, item)} /> ); @@ -226,7 +221,7 @@ export class ReadingList extends Component {
this.toggleStatusView(e)} + onClick={(e) => this.toggleStatusView(e)} data-no-instant > {isStatusViewValid ? 'View Archive' : 'View Reading List'} diff --git a/app/javascript/searchableItemList/searchableItemList.js b/app/javascript/searchableItemList/searchableItemList.js index dca05149d..109866bcd 100644 --- a/app/javascript/searchableItemList/searchableItemList.js +++ b/app/javascript/searchableItemList/searchableItemList.js @@ -1,5 +1,5 @@ // Shared behavior between the reading list and history pages -import setupAlgoliaIndex from '../src/utils/algolia'; +import { fetchSearch } from '../src/utils/search'; // Provides the initial state for the component export function defaultState(options) { @@ -59,25 +59,24 @@ export function clearSelectedTags(event) { } // Perform the initial search -export function performInitialSearch({ - containerId, - indexName, - searchOptions = {}, -}) { +export function performInitialSearch({ searchOptions = {} }) { const component = this; const { hitsPerPage } = component.state; + const dataHash = { page: 0, per_page: hitsPerPage }; - const index = setupAlgoliaIndex({ containerId, indexName }); + if (searchOptions.status) { + dataHash.status = searchOptions.status.split(','); + } - index.search('', searchOptions).then(result => { + const responsePromise = fetchSearch('reactions', dataHash); + return responsePromise.then((response) => { + const reactions = response.result; component.setState({ - items: result.hits, - totalCount: result.nbHits, - index, // set the index in the component state, to be retrieved later + page: 0, + items: reactions, itemsLoaded: true, - // show the button if the number of total results is greater - // than the number of results for the current page - showLoadMoreButton: result.nbHits > hitsPerPage, + totalCount: response.total, + showLoadMoreButton: hitsPerPage < response.total, }); }); } @@ -90,28 +89,31 @@ export function search(query, { page, tags, statusView }) { // we check `undefined` because page can be 0 const newPage = page === undefined ? component.state.page : page; - const { index, hitsPerPage, items } = component.state; + const { hitsPerPage } = component.state; + const dataHash = { + search_fields: query, + page: newPage, + per_page: hitsPerPage, + }; - const filters = { hitsPerPage, page: newPage }; if (tags && tags.length > 0) { - filters.tagFilters = tags; + dataHash.tag_names = tags; + dataHash.tag_boolean_mode = 'all'; } if (statusView) { - filters.filters = `status:${statusView}`; + dataHash.status = statusView.split(','); } - index.search(query, filters).then(result => { - // append new items at the end - const allItems = - page === undefined ? result.hits : [...items, ...result.hits]; + + const responsePromise = fetchSearch('reactions', dataHash); + return responsePromise.then((response) => { + const reactions = response.result; component.setState({ query, page: newPage, - items: result.hits, - totalCount: allItems.length, - // show the button if the number of items is lower than the number - // of available results - showLoadMoreButton: allItems.length < result.nbHits, + items: reactions, + totalCount: response.total, + showLoadMoreButton: reactions.length < response.total, }); }); } diff --git a/app/javascript/src/components/ItemList/ItemListItem.jsx b/app/javascript/src/components/ItemList/ItemListItem.jsx index d755bfd97..6e9cfd515 100644 --- a/app/javascript/src/components/ItemList/ItemListItem.jsx +++ b/app/javascript/src/components/ItemList/ItemListItem.jsx @@ -4,13 +4,12 @@ import { PropTypes } from 'preact-compat'; export const ItemListItem = ({ item, children }) => { const adaptedItem = { - path: item.article_path || item.searchable_reactable_path, - title: item.article_title || item.searchable_reactable_title, - user: item.article_user || item.reactable_user, - publishedDate: item.reactable_published_date, - visitedDate: item.readable_visited_at, - readingTime: item.article_reading_time || item.reading_time, - tags: item.article_tags || item.reactable_tags, + path: item.reactable.path, + title: item.reactable.title, + user: item.reactable.user, + publishedDate: item.reactable.published_date_string, + readingTime: item.reactable.reading_time, + tags: item.reactable.tags, }; // update readingTime to 1 min if the reading time is less than 1 min @@ -24,17 +23,15 @@ export const ItemListItem = ({ item, children }) => { Profile Pic {`${adaptedItem.user.name}・`} - {adaptedItem.visitedDate - ? `visited on ${adaptedItem.visitedDate}・` - : `${adaptedItem.publishedDate}・`} + {`${adaptedItem.publishedDate}・`} {`${adaptedItem.readingTime} min read・`} {adaptedItem.tags.length > 0 ? ( - {adaptedItem.tags.map(tag => ( - - {`#${tag}`} + {adaptedItem.tags.map((tag) => ( + + {`#${tag.name}`} ))} @@ -49,30 +46,19 @@ export const ItemListItem = ({ item, children }) => { ); }; -const historyItemPropTypes = PropTypes.shape({ - article_path: PropTypes.string.isRequired, - article_title: PropTypes.string.isRequired, - article_user: PropTypes.shape({ - username: PropTypes.string.isRequired, - profile_image_90: PropTypes.string.isRequired, - name: PropTypes.string.isRequired, - }), - article_reading_time: PropTypes.number.isRequired, - readable_visited_at: PropTypes.string.isRequired, - article_tags: PropTypes.arrayOf(PropTypes.string).isRequired, -}); - const readingListItemPropTypes = PropTypes.shape({ - searchable_reactable_path: PropTypes.string.isRequired, - searchable_reactable_title: PropTypes.string.isRequired, - reactable_user: PropTypes.shape({ - username: PropTypes.string.isRequired, - profile_image_90: PropTypes.string.isRequired, - name: PropTypes.string.isRequired, - }), - reading_time: PropTypes.number.isRequired, - reactable_published_date: PropTypes.string.isRequired, - reactable_tags: PropTypes.arrayOf(PropTypes.string).isRequired, + reactable: { + path: PropTypes.string.isRequired, + title: PropTypes.string.isRequired, + reading_time: PropTypes.number.isRequired, + published_date_string: PropTypes.string.isRequired, + user: PropTypes.shape({ + username: PropTypes.string.isRequired, + profile_image_90: PropTypes.string.isRequired, + name: PropTypes.string.isRequired, + }), + tags: PropTypes.arrayOf(PropTypes.string).isRequired, + }, }); ItemListItem.defaultProps = { @@ -80,7 +66,6 @@ ItemListItem.defaultProps = { }; ItemListItem.propTypes = { - item: PropTypes.oneOfType([historyItemPropTypes, readingListItemPropTypes]) - .isRequired, + item: PropTypes.oneOfType([readingListItemPropTypes]).isRequired, children: PropTypes.element, }; diff --git a/app/javascript/src/components/ItemList/__tests__/ItemListItem.test.jsx b/app/javascript/src/components/ItemList/__tests__/ItemListItem.test.jsx index f3f090700..c194ece8e 100644 --- a/app/javascript/src/components/ItemList/__tests__/ItemListItem.test.jsx +++ b/app/javascript/src/components/ItemList/__tests__/ItemListItem.test.jsx @@ -3,55 +3,61 @@ import render from 'preact-render-to-json'; import { shallow } from 'preact-render-spy'; import { ItemListItem } from '../ItemListItem'; -const historyItem = { - article_path: '/article', - article_title: 'Title', - article_user: { - username: 'bob', - profile_image_90: 'https://dummyimage.com/90x90', - name: 'Bob', - }, - article_reading_time: 1, - readable_visited_at: 'Jun 29', - article_tags: ['discuss'], -}; - const item = { - searchable_reactable_path: '/article', - searchable_reactable_title: 'Title', - reactable_user: { - username: 'bob', - profile_image_90: 'https://dummyimage.com/90x90', - name: 'Bob', + reactable: { + path: '/article', + title: 'Title', + published_date_string: 'Jun 29', + reading_time: 1, + user: { + username: 'bob', + profile_image_90: 'https://dummyimage.com/90x90', + name: 'Bob', + }, + tags: [{ name: 'discuss' }], }, - reading_time: 1, - reactable_published_date: 'Jun 29', - reactable_tags: ['discuss'], }; describe('', () => { - it('renders properly with a history item', () => { - const tree = render(); - expect(tree).toMatchSnapshot(); - }); - it('renders properly with a readinglist item', () => { const tree = render(); expect(tree).toMatchSnapshot(); }); it('renders with readingtime of 1 min if reading time is less than 1 min.', () => { - const wrapper = shallow(); + item.reactable.reading_time = 0.5; + const wrapper = shallow(); expect(wrapper.find('.item-user').text()).toContain('1 min read'); }); it('renders with readingtime of 1 min if reading time is null.', () => { - const wrapper = shallow(); + item.reactable.reading_time = null; + const wrapper = shallow(); expect(wrapper.find('.item-user').text()).toContain('1 min read'); }); + it('renders correct readingtime.', () => { + item.reactable.reading_time = 10; + const wrapper = shallow(); + expect(wrapper.find('.item-user').text()).toContain('10 min read'); + }); + it('renders without any tags if the tags array is empty.', () => { - const wrapper = shallow(); - expect(wrapper.find('.item-user').text()).toContain('1 min read'); + item.reactable.tags = []; + const wrapper = shallow(); + expect(wrapper.find('.item-tags').exists()).toEqual(false); + }); + + it('renders tags with links if present.', () => { + item.reactable.tags = [{ name: 'discuss' }]; + const wrapper = shallow(); + expect(wrapper.find('.item-tag')[0].attributes.href).toEqual('/t/discuss'); + expect(wrapper.find('.item-tag').text()).toContain('discuss'); + }); + + it('renders user information', () => { + const wrapper = shallow(); + expect(wrapper.find('.item-user')[0].attributes.href).toEqual('/bob'); + expect(wrapper.find('.item-user').text()).toContain('Bob'); }); }); diff --git a/app/javascript/src/components/ItemList/__tests__/__snapshots__/ItemListItem.test.jsx.snap b/app/javascript/src/components/ItemList/__tests__/__snapshots__/ItemListItem.test.jsx.snap index 4c324d433..8595095ad 100644 --- a/app/javascript/src/components/ItemList/__tests__/__snapshots__/ItemListItem.test.jsx.snap +++ b/app/javascript/src/components/ItemList/__tests__/__snapshots__/ItemListItem.test.jsx.snap @@ -1,46 +1,5 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[` renders properly with a history item 1`] = ` -
- -
- Title -
-
- - Profile Pic - Bob・visited on Jun 29・1 min read・ - - - - #discuss - - -
- -
-`; - exports[` renders properly with a readinglist item 1`] = `
val } } } else { terms: { search_key => values } } diff --git a/spec/services/search/reaction_spec.rb b/spec/services/search/reaction_spec.rb index 05a407be8..0528d1c80 100644 --- a/spec/services/search/reaction_spec.rb +++ b/spec/services/search/reaction_spec.rb @@ -92,7 +92,7 @@ RSpec.describe Search::Reaction, type: :service do it "filters by status" do reaction1.update(status: "invalid") index_documents([reaction1, reaction2]) - query_params[:status] = ["valid"] + query_params[:status] = %w[valid confirmed] reaction_docs = described_class.search_documents(params: query_params)["reactions"] expect(reaction_docs.count).to eq(1)