Properly implemented sessionTrackingEnabled #1452
Conversation
package.json
Outdated
| @@ -24,7 +24,7 @@ | |||
| ], | |||
| "dependencies": { | |||
| "@mapbox/mapbox-gl-language": "^0.10.1", | |||
| "@yext/answers-core": "^1.2.0", | |||
| "@yext/answers-core": "file:../answers-core", | |||
There was a problem hiding this comment.
Once you merge in your core PR, we should publish to NPM and have this PR point to that version
| let sessionId = window.sessionStorage.getItem('sessionId'); | ||
| if (!sessionId) { | ||
| sessionId = uuid(); | ||
| window.sessionStorage.setItem('sessionId', sessionId); |
There was a problem hiding this comment.
nit: I generally try to avoid putting setters inside of a getters because that means the getter has side effects. What do you think of renaming this function to getOrSetupSessionId to try and avoid this confusion?
There was a problem hiding this comment.
I agree that the name of the function could be better. getSessionId() is definitely misleading since it's not just a getter.
src/core/utils/uuid.js
Outdated
| * | ||
| * @returns {String} | ||
| */ | ||
| export function uuid() { |
There was a problem hiding this comment.
nit: This function name is a noun, and I think it's easier to read code when function names start with verbs. Would generateUUID work here instead?
There was a problem hiding this comment.
yeah we can do that.
There was a problem hiding this comment.
In the case where sessionTrackingEnabled is true, but we fail to use session storage, the back-end generates a cookie. Did we have to do anything special to make sure that cookie is passed back on all subsequent requests? Or is that just handled by the browser?
src/core/core.js
Outdated
| @@ -345,6 +346,7 @@ export default class Core { | |||
| location: this._getLocationPayload(), | |||
| skipSpellCheck: this.storage.get(StorageKeys.SKIP_SPELL_CHECK), | |||
| queryTrigger: queryTriggerForApi, | |||
| sessionId: this.getSessionId(), | |||
There was a problem hiding this comment.
One thing the spec is a little unclear on is which requests we need to include sessionId with. Is it only the search requests or on autocomplete as well? Rose may need to weigh in there.
There was a problem hiding this comment.
The browser handles cookies in the case of session storage failing.
When sessionTrackingEnabled is false, getSessionId() returns null. When that happens, in the browser, I don't see a sessionId query parameter which indicates to me that setting it to null results in it being filtered out.
I assumed we would only use search requests because a search request is called whenever we refresh the page. Since a search request occurs when the page loads for the first time and we generate a sessionId then. It might be redundant to include it in the autocomplete as well since we don't update our sessionId if a sessionId already exists. So for example, the page loads and a search query happens and we generate our first sessionID. another search happens and our sessionID doesn't change because we already have an existing one.
There was a problem hiding this comment.
For tracking purposes, they may want to associate various auto-complete calls or question submission calls with a particular sessionId. Here, I think it's a question of what the back-end expects. If we don't include a sessionId on an auto-complete request, I'm not sure the back-end has any way of knowing which session to associate it with. @mdavish, do you know what the back-end expects here? Should all requests made by the SDK (excluding Analytics) have a sessionId?
There was a problem hiding this comment.
Okay, heard back from Product. We only want sessionId for the searches!
J=SLAP-1408 TEST=manual Checked on test page to make sure that session_id and cookies are being passed as expected. When sessionTrackingEnabled is true, the sessionID is passed to the test site and no cookie for sessionID is dropped. When sessionTrackingEnabled is false, the sessionID is generated by the API and no cookie is dropped. When no sessionID is passed and sessionTrackingEnabled is true, a cookie is dropped.
cjiang2000
force-pushed
the
dev/sessionstorage
branch
from
fb5213d
to
bfaf504
Compare
### New Features * The SearchBar now supports displaying a loading indicator while a search is running. * The SearchBar also now has support for voice search. * We now support several new languages, including simplified/traditional Chinese and several variants of Arabic. * `universalLimit` config option (#1444) * Session tracking using the browser's session storage (#1452) * `querySource` can now be set at runtime using `ANSWERS.setQuerySource()` (#1464) * Display email validation error in QuestionSubmission (#1505) * Support for DirectAnswers on vertical searches (#1483) * Support providerOptions on the map (#1484) * Create entry points for multi-lang SDK bundles (#1501) ### Speed Improvements * replace IconComponent usages with handlebars partials (#1445) (#1451) (#1458) ### Infrastructure * Change CircleCI node_modules caching strategy to use package-lock.json instead of package.json (#1457) * `canary/latest` assets are now built with i18n support (#1459) * More tests + acceptance testing improvements * Automated publishing for the standalone SearchBar assets (#1529) ### SearchBar-only release notes * The SearchBar now supports displaying a loading indicator while a search is running. * The SearchBar also now has support for voice search.
J=SLAP-1408
TEST=manual
Checked on test page to make sure that session_id and cookies are being passed as expected. When sessionTrackingEnabled is true, the sessionID is passed to the test site and no cookie for sessionID is dropped. When sessionTrackingEnabled is false, the sessionID is generated by the API and no cookie is dropped. When no sessionID is passed and sessionTrackingEnabled is true, a cookie is dropped.