Skip to content

Commit

Permalink
pagination: Fix disappearing pagination on back nav and refresh
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
creotutar committed Sep 3, 2020
1 parent 1124bf2 commit 9792642
Show file tree
Hide file tree
Showing 6 changed files with 72 additions and 27 deletions.
12 changes: 11 additions & 1 deletion src/answers-umd.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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);
}
Expand All @@ -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)) {
Expand Down Expand Up @@ -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);
}

Expand Down
28 changes: 23 additions & 5 deletions src/core/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand All @@ -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,
Expand Down Expand Up @@ -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;
}
Expand Down
12 changes: 12 additions & 0 deletions src/core/models/querytriggers.js
Original file line number Diff line number Diff line change
@@ -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'
}
3 changes: 2 additions & 1 deletion src/core/storage/storagekeys.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'
};
40 changes: 22 additions & 18 deletions src/ui/components/search/searchcomponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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 });
});

/**
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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) ||
Expand Down Expand Up @@ -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);
Expand All @@ -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
Expand All @@ -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);
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/ui/components/search/spellcheckcomponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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();
}

Expand Down

0 comments on commit 9792642

Please sign in to comment.