-
Notifications
You must be signed in to change notification settings - Fork 4
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
Support autocompleteSessionId #54
base: main
Are you sure you want to change the base?
Conversation
J=SLAP-1723 TEST=auto use updated core in headless, see that jest tests passed.
src/answers-headless.ts
Outdated
@@ -162,13 +173,18 @@ export default class AnswersHeadless { | |||
this.stateManager.dispatchEvent('searchStatus/setIsLoading', false); | |||
this.stateManager.dispatchEvent('meta/setUUID', response.uuid); | |||
this.stateManager.dispatchEvent('directAnswer/setResult', response.directAnswer); | |||
if (this.state.query.searchAggregation?.enabled) { | |||
this.stateManager.dispatchEvent('query/setSearchAggregationId', uuidv4()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you expand a little on how searches will be grouped together? It looks like here we would generate a new autocompleteSessionId for every single universal search made? Or maybe even a jest automock? I don't have any experience using automock, but it looks like here we're just counting the number of function calls so I think it could work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After discussing with product, the usage of autocompleteSessionId is different from the functionality added in this pr. autocompleteSessionId should be used in vertical/universal search only when it's executed for the purpose of visual autocomplete (the search term aggregation and this id doesn't apply for normal autocomplete). My original thought was to update uuid after every final search is triggered, and the visual autocomplete would have another function to execute autocomplete + search endpoints. Updated to always include autocompleteSessionId, and user can set that if they want a uuid to be included in the request
…eadless into dev/autocompleteSessionId
src/answers-headless.ts
Outdated
@@ -48,6 +48,10 @@ export default class AnswersHeadless { | |||
this.stateManager.dispatchEvent('query/setSource', source); | |||
} | |||
|
|||
setSearchAggregationId(uuid: string): void { | |||
this.stateManager.dispatchEvent('query/setSearchAggregationId', uuid); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if somebody wanted to clear the id would they just set it to blank string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, do you think undefined/null is better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eh think it's fine!
…eadless into dev/autocompleteSessionId
Update headless state and network calls to support autocompleteSessionId
QueryState
to include SearchAggregation, which includes 'enabled' and 'id' field. This is similar to SessionTrackingState, and would give user better context in what it is use for compare to the backend name of 'autocompleteSessionId'.NOTE: product wants to hold off on publishing this in core latest or beta version until backend work is done.
J=SLAP-1723
TEST=auto
use updated core in headless, see that jest tests passed.