Skip to content
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

Update SDK to allow DA on vertical search #1438

Merged

Conversation

yen-tt
Copy link
Contributor

@yen-tt yen-tt commented Jun 17, 2021

add direct answers to vertical search in sdk core data transformer layer
Note: this sdk is using core version 1.3.0-alpha to get vertical search responses with direct answer

J=SLAP-1389
TEST=auto and manual

Update and run jest tests to see that results are formatted properly.
Verify direct answer is fetched properly from answers-core with console log.
Launched theme test site using local sdk bundle, performed a vertical search on people's page and can verify that direct answer is present in network response

add direct answers to vertical search in sdk core data transformer layer
Note: this sdk is using core version 1.3.0-alpha to get vertical search responses with direct answer

J=SLAP-1389
TEST=auto and manual

Update and run jest tests to see that results are formatted properly.
Verify direct answer is fetched properly from answers-core with console log.
Launched theme test site using local sdk bundle, performed a vertical search on people's page and can verify that direct answer is present in network response
@coveralls
Copy link

coveralls commented Jun 17, 2021

Coverage Status

Coverage decreased (-0.009%) to 56.84% when pulling e4c5721 on feature/DA-on-vertical-search into 4d7c0cb on feature/DirectAnswer-in-Vertical.

@@ -258,8 +259,12 @@ export default class Core {
const mergedResults = this.storage.get(StorageKeys.VERTICAL_RESULTS)
.append(data[StorageKeys.VERTICAL_RESULTS]);
this.storage.set(StorageKeys.VERTICAL_RESULTS, mergedResults);
if (data[StorageKeys.DIRECT_ANSWER].answer) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this instance we're guarding the setting of StorageKeys.DIRECT_ANSWER, but below we just set it without checking data[StorageKeys.DIRECT_ANSWER].answer. Do we need to check this value in both cases? If so, we could move that logic outside of the if(query.append) conditional and only have it once.

Copy link
Contributor Author

@yen-tt yen-tt Jun 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to check this value in both cases. The first result set from this vertical search may or may not have direct answers from live api, which is ok so I didn't place a check there. But with param query.append allowing for search result of new query to add on to the existing results, if this new search result contains a new directAnswer, then we would override it (since we expect only 1 directAnswer return). Otherwise, if this new search result doesn't have a directAnswer, then I think we should keep the previous directAnswer from current result set.

@yen-tt yen-tt merged commit f33b17a into feature/DirectAnswer-in-Vertical Jun 24, 2021
@yen-tt yen-tt deleted the feature/DA-on-vertical-search branch June 24, 2021 13:11
yen-tt added a commit that referenced this pull request Jun 25, 2021
* Update SDK to allow DA on vertical search

add direct answers to vertical search in sdk core data transformer layer
Note: this sdk is using core version 1.3.0-alpha to get vertical search responses with direct answer

J=SLAP-1389
TEST=auto and manual

Update and run jest tests to see that results are formatted properly.
Verify direct answer is fetched properly from answers-core with console log.
Launched theme test site using local sdk bundle, performed a vertical search on people's page and can verify that direct answer is present in network response

Co-authored-by: Yen Truong <ytruong@yext.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants