From 97926427745bb1f1f5782d4ad1531fb378638a74 Mon Sep 17 00:00:00 2001 From: creotutar Date: Wed, 2 Sep 2020 19:55:40 -0400 Subject: [PATCH] pagination: Fix disappearing pagination on back nav and refresh Previously, there were two pagination persistence bugs. 1. Land on a vertical page with a query with results. Go to page 2. Go to page 3. Try to navigate backwards in browser history. You will go to page 1 instead of page 3. 2. Land on a vertical page w a query w results. Go to page 2. Refresh the page. You will be on page 1 instead of page 2. Both of these related bugs were because we were defaulting to resetting pagination on any searchbar search. In 1, backwards navigation updates the query and the searchbar takes the updated query and searches. In 2, landing with a `query` query parameter is determined in the SearchBar component onCreate. To fix this, we add another value for the pre-existing `queryTrigger` global storage key, QUERY_PARAMETER, meaning the query was triggered by a query parameter. Note: as a result of this fix, we were no longer able to go from Page 2 to Page 1. This is because of another bug, where navigating backwards in history from presence of a query param to absence of the param will not erase the parameter in storage. J=SLAP-610 TEST=manual Test on a local Jambo HH Theme site attached to a local dist of the SDK. Test on a local site using the SDK directly. Test that navigating on a vertical to Page 2 and then Page 3 allows you to navigate back in browser history to Page 2 and Page 1 respectively. Test that navigating to a page and then refreshing maintains pagination offset. (IE Test that query params for search-offset work as intended). Test that navigating to a page and then searching another query in the searchbar /will/ reset pagination (and remove the associated query parameter). Test that queryTrigger in the liveapi request in all the situations are null. Test that with defaultInitialSearch set, queryTrigger still sends as 'initialize' in the liveapi request. --- src/answers-umd.js | 12 +++++- src/core/core.js | 28 ++++++++++--- src/core/models/querytriggers.js | 12 ++++++ src/core/storage/storagekeys.js | 3 +- src/ui/components/search/searchcomponent.js | 40 ++++++++++--------- .../components/search/spellcheckcomponent.js | 4 +- 6 files changed, 72 insertions(+), 27 deletions(-) create mode 100644 src/core/models/querytriggers.js diff --git a/src/answers-umd.js b/src/answers-umd.js index c78dbb94d..e66354f00 100644 --- a/src/answers-umd.js +++ b/src/answers-umd.js @@ -19,6 +19,7 @@ import GlobalStorage from './core/storage/globalstorage'; import { AnswersComponentError } from './core/errors/errors'; import AnalyticsEvent from './core/analytics/analyticsevent'; import StorageKeys from './core/storage/storagekeys'; +import QueryTriggers from './core/models/querytriggers'; import SearchConfig from './core/models/searchconfig'; import AutoCompleteApi from './core/search/autocompleteapi'; import MockAutoCompleteService from './core/search/mockautocompleteservice'; @@ -163,6 +164,12 @@ class Answers { resetListener: data => { if (!data[StorageKeys.QUERY]) { this.core.clearResults(); + } else { + this.core.globalStorage.set(StorageKeys.QUERY_TRIGGER, QueryTriggers.QUERY_PARAMETER); + } + + if (!data[StorageKeys.SEARCH_OFFSET]) { + this.core.globalStorage.delete(StorageKeys.SEARCH_OFFSET); } globalStorage.setAll(data); } @@ -173,6 +180,9 @@ class Answers { globalStorage.set(StorageKeys.LOCALE, parsedConfig.locale); globalStorage.set(StorageKeys.SESSIONS_OPT_IN, parsedConfig.sessionTrackingEnabled); parsedConfig.noResults && globalStorage.set(StorageKeys.NO_RESULTS_CONFIG, parsedConfig.noResults); + if (globalStorage.getState(StorageKeys.QUERY)) { + globalStorage.set(StorageKeys.QUERY_TRIGGER, QueryTriggers.QUERY_PARAMETER); + } const context = globalStorage.getState(StorageKeys.API_CONTEXT); if (context && !isValidContext(context)) { @@ -434,7 +444,7 @@ class Answers { if (prepopulatedQuery != null) { return; } - this.core.globalStorage.set('queryTrigger', 'initialize'); + this.core.globalStorage.set(StorageKeys.QUERY_TRIGGER, QueryTriggers.INITIALIZE); this.core.setQuery(searchConfig.defaultInitialSearch); } diff --git a/src/core/core.js b/src/core/core.js index c883f3e8d..cc12491a1 100644 --- a/src/core/core.js +++ b/src/core/core.js @@ -10,6 +10,7 @@ import Navigation from './models/navigation'; import AlternativeVerticals from './models/alternativeverticals'; import DirectAnswer from './models/directanswer'; import LocationBias from './models/locationbias'; +import QueryTriggers from './models/querytriggers'; import StorageKeys from './storage/storagekeys'; import AnalyticsEvent from './analytics/analyticsevent'; @@ -172,7 +173,9 @@ export default class Core { } const locationRadiusFilterNode = this.getLocationRadiusFilterNode(); - + const queryTrigger = this.getQueryTriggerForSearchApi( + this.globalStorage.getState(StorageKeys.QUERY_TRIGGER) + ); return this._searcher .verticalSearch(verticalKey, { limit: this.globalStorage.getState(StorageKeys.SEARCH_CONFIG).limit, @@ -184,7 +187,7 @@ export default class Core { offset: this.globalStorage.getState(StorageKeys.SEARCH_OFFSET) || 0, isDynamicFiltersEnabled: this._isDynamicFiltersEnabled, skipSpellCheck: this.globalStorage.getState('skipSpellCheck'), - queryTrigger: this.globalStorage.getState('queryTrigger'), + queryTrigger: queryTrigger, sessionTrackingEnabled: this.globalStorage.getState(StorageKeys.SESSIONS_OPT_IN), sortBys: this.globalStorage.getState(StorageKeys.SORT_BYS), locationRadius: locationRadiusFilterNode ? locationRadiusFilterNode.getFilter().value : null, @@ -217,7 +220,7 @@ export default class Core { this.globalStorage.set(StorageKeys.LOCATION_BIAS, data[StorageKeys.LOCATION_BIAS]); } this.globalStorage.delete('skipSpellCheck'); - this.globalStorage.delete('queryTrigger'); + this.globalStorage.delete(StorageKeys.QUERY_TRIGGER); const exposedParams = { verticalKey: verticalKey, @@ -280,11 +283,14 @@ export default class Core { this.globalStorage.set(StorageKeys.SPELL_CHECK, {}); this.globalStorage.set(StorageKeys.LOCATION_BIAS, {}); + const queryTrigger = this.getQueryTriggerForSearchApi( + this.globalStorage.getState(StorageKeys.QUERY_TRIGGER) + ); return this._searcher .universalSearch(queryString, { geolocation: this.globalStorage.getState(StorageKeys.GEOLOCATION), skipSpellCheck: this.globalStorage.getState('skipSpellCheck'), - queryTrigger: this.globalStorage.getState('queryTrigger'), + queryTrigger: queryTrigger, sessionTrackingEnabled: this.globalStorage.getState(StorageKeys.SESSIONS_OPT_IN), context: context, referrerPageUrl: referrerPageUrl @@ -299,7 +305,7 @@ export default class Core { this.globalStorage.set(StorageKeys.SPELL_CHECK, data[StorageKeys.SPELL_CHECK]); this.globalStorage.set(StorageKeys.LOCATION_BIAS, data[StorageKeys.LOCATION_BIAS]); this.globalStorage.delete('skipSpellCheck'); - this.globalStorage.delete('queryTrigger'); + this.globalStorage.delete(StorageKeys.QUERY_TRIGGER); const exposedParams = { queryString: queryString, @@ -497,6 +503,18 @@ export default class Core { this.filterRegistry.clearLocationRadiusFilterNode(); } + /** + * Returns the query trigger for the search API given the SDK query trigger + * @param {QueryTriggers} queryTrigger SDK query trigger + * @returns {QueryTriggers} query trigger if accepted by the search API, null o/w + */ + getQueryTriggerForSearchApi (queryTrigger) { + if (queryTrigger === QueryTriggers.QUERY_PARAMETER) { + return null; + } + return queryTrigger; + } + enableDynamicFilters () { this._isDynamicFiltersEnabled = true; } diff --git a/src/core/models/querytriggers.js b/src/core/models/querytriggers.js new file mode 100644 index 000000000..6d181355b --- /dev/null +++ b/src/core/models/querytriggers.js @@ -0,0 +1,12 @@ +/** @module QueryTriggers */ + +/** + * QueryTriggers is an ENUM of the possible triggers for a + * query update. + * + * @enum {string} + */ +export default { + INITIALIZE: 'initialize', + QUERY_PARAMETER: 'query-parameter' +} diff --git a/src/core/storage/storagekeys.js b/src/core/storage/storagekeys.js index fb8a9fe4a..9230092e8 100644 --- a/src/core/storage/storagekeys.js +++ b/src/core/storage/storagekeys.js @@ -35,5 +35,6 @@ export default { LOCATION_RADIUS: 'location-radius', RESULTS_HEADER: 'results-header', API_CONTEXT: 'context', - REFERRER_PAGE_URL: 'referrerPageUrl' + REFERRER_PAGE_URL: 'referrerPageUrl', + QUERY_TRIGGER: 'queryTrigger' }; diff --git a/src/ui/components/search/searchcomponent.js b/src/ui/components/search/searchcomponent.js index f423e6ffd..8b99cc26a 100644 --- a/src/ui/components/search/searchcomponent.js +++ b/src/ui/components/search/searchcomponent.js @@ -3,6 +3,7 @@ import Component from '../component'; import DOM from '../../dom/dom'; import StorageKeys from '../../../core/storage/storagekeys'; +import QueryTriggers from '../../../core/models/querytriggers'; import SearchParams from '../../dom/searchparams'; const IconState = { @@ -154,7 +155,10 @@ export default class SearchComponent extends Component { } return; } - this.debouncedSearch(q); + + const queryTrigger = this.core.globalStorage.getState(StorageKeys.QUERY_TRIGGER); + const resetPagination = this._verticalKey && queryTrigger !== QueryTriggers.QUERY_PARAMETER; + this.debouncedSearch(q, { resetPagination: resetPagination }); }); /** @@ -462,7 +466,7 @@ export default class SearchComponent extends Component { this.core.persistentStorage.delete(StorageKeys.SEARCH_OFFSET); this.core.globalStorage.delete(StorageKeys.SEARCH_OFFSET); this.core.setQuery(query); - this.debouncedSearch(query); + this.debouncedSearch(query, {}); return false; } @@ -505,9 +509,10 @@ export default class SearchComponent extends Component { * performed if we recently searched, if there's no query for universal search, or if this * is a twin searchbar. * @param {string} query The string to query against. + * @param {Object} searchOptionsOverrides The options to pass for core search * @returns {Promise} A promise that will perform the query and update globalStorage accordingly. */ - debouncedSearch (query) { + debouncedSearch (query, searchOptionsOverrides) { if (this._throttled || (!query && !this._verticalKey) || (!query && this._verticalKey && !this._allowEmptySearch) || @@ -535,10 +540,10 @@ export default class SearchComponent extends Component { lng: position.coords.longitude, radius: position.coords.accuracy }); - resolve(this.search(query)); + resolve(this.search(query, searchOptionsOverrides)); }, () => { - resolve(this.search(query)); + resolve(this.search(query, searchOptionsOverrides)); const { enabled, message } = this._geolocationTimeoutAlert; if (enabled) { window.alert(message); @@ -547,29 +552,28 @@ export default class SearchComponent extends Component { this._geolocationOptions) ); } else { - return this.search(query); + return this.search(query, searchOptionsOverrides); } }); } else { - return this.search(query); + return this.search(query, searchOptionsOverrides); } } /** * Performs a query using the provided string input. * @param {string} query The string to query against. + * @param {Object} searchOptionsOverrides The options to pass for core search * @returns {Promise} A promise that will perform the query and update globalStorage accordingly. */ - search (query) { + search (query, searchOptionsOverrides) { + const defaultSearchOptions = { + setQueryParams: true, + resetPagination: !!this._verticalKey + }; if (this._verticalKey) { - this.core.verticalSearch( - this._config.verticalKey, - { - resetPagination: true, - setQueryParams: true - }, - { input: query } - ); + const searchOptions = Object.assign({}, defaultSearchOptions, searchOptionsOverrides); + this.core.verticalSearch(this._config.verticalKey, searchOptions, { input: query }); } else { // NOTE(billy) Temporary hack for DEMO // Remove me after the demo @@ -592,10 +596,10 @@ export default class SearchComponent extends Component { urls[tabs[i].configId] = url; } } - return this.core.search(query, urls, { setQueryParams: true }); + return this.core.search(query, urls, defaultSearchOptions); } - return this.core.search(query, undefined, { setQueryParams: true }); + return this.core.search(query, undefined, defaultSearchOptions); } } diff --git a/src/ui/components/search/spellcheckcomponent.js b/src/ui/components/search/spellcheckcomponent.js index 673dfe7d5..190048545 100644 --- a/src/ui/components/search/spellcheckcomponent.js +++ b/src/ui/components/search/spellcheckcomponent.js @@ -31,7 +31,7 @@ export default class SpellCheckComponent extends Component { onCreate () { this.core.persistentStorage.delete('skipSpellCheck', true); - this.core.persistentStorage.delete('queryTrigger', true); + this.core.persistentStorage.delete(StorageKeys.QUERY_TRIGGER, true); } setState (data, val) { @@ -49,7 +49,7 @@ export default class SpellCheckComponent extends Component { let params = new SearchParams(window.location.search.substring(1)); params.set('query', query.value); params.set('skipSpellCheck', true); - params.set('queryTrigger', type.toLowerCase()); + params.set(StorageKeys.QUERY_TRIGGER, type.toLowerCase()); return '?' + params.toString(); }