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

switch throw to console error in analytics reporter #1771

Merged
merged 10 commits into from
Aug 30, 2022

Conversation

yen-tt
Copy link
Contributor

@yen-tt yen-tt commented Aug 30, 2022

Throw error from not having ytag script tag for conversion tracking opt in would prevent a search fired on load. This is an aggressive behavior as analytics shouldn't affect search behavior. This pr updated the two throw error into console error instead.

TEST=manual

Added ANSWERS.setConversionsOptIn(true); to a local index page with Answers setup. see that the previously uncaught throw error is now a console error and a search on load is no longer blocked.

Screen Shot 2022-08-30 at 5 06 28 PM

@oshi97
Copy link
Contributor

oshi97 commented Aug 30, 2022

do we need to update support branches? (ideally we don't but wanted to check)

@yen-tt
Copy link
Contributor Author

yen-tt commented Aug 30, 2022

do we need to update support branches? (ideally we don't but wanted to check)

This hofix branch v1.13.1 will go into support branch v1.13. I will also make a hotfix branch for v1.14.2

@yen-tt yen-tt requested review from oshi97 and nmanu1 August 30, 2022 22:18
nmanu1
nmanu1 previously requested changes Aug 30, 2022
Copy link
Contributor

@nmanu1 nmanu1 left a comment

Choose a reason for hiding this comment

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

looks like tests need to be updated

@coveralls
Copy link

coveralls commented Aug 30, 2022

Coverage Status

Coverage decreased (-0.01%) to 61.737% when pulling 556b2af on dev/analytics-console-error into c8d2be6 on hotfix/v1.13.1.

Yen Truong added 2 commits August 30, 2022 18:46
@yen-tt yen-tt requested a review from nmanu1 August 30, 2022 22:50
Copy link
Contributor

@nmanu1 nmanu1 left a comment

Choose a reason for hiding this comment

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

looks like acceptance tests are failing?

@yen-tt
Copy link
Contributor Author

yen-tt commented Aug 30, 2022

looks like acceptance tests are failing?

looks unrelated, I will run again to see if it's spurious

@nmanu1
Copy link
Contributor

nmanu1 commented Aug 30, 2022

looks unrelated, I will run again to see if it's spurious

I think v1.13 is from before we mocked the acceptance test fixtures, so it might just be that the response changed and needs to be updated

@yen-tt
Copy link
Contributor Author

yen-tt commented Aug 30, 2022

looks unrelated, I will run again to see if it's spurious

I think v1.13 is from before we mocked the acceptance test fixtures, so it might just be that the response changed and needs to be updated

yea, was seeing if the answers experience match the new expected before making changes. It seems like we turned on facets for c_cellPhone and removed some entities data for c_popularities so those caused the acceptance test to fail. I was fiddling with the experience config but thought I should just update the response here anyway like you said.

@yen-tt yen-tt merged commit a8ad65d into hotfix/v1.13.1 Aug 30, 2022
@yen-tt yen-tt deleted the dev/analytics-console-error branch August 30, 2022 23:39
@yen-tt yen-tt mentioned this pull request Aug 30, 2022
yen-tt added a commit that referenced this pull request Aug 31, 2022
### Bug Fixes
- Gracefully handle analytics-related error to prevent uncaught errors from blocking the search on load (#1771)
yen-tt added a commit that referenced this pull request Aug 31, 2022
Throw error from not having `ytag` script tag for conversion tracking opt in would prevent a search fired on load. This is an aggressive behavior as analytics shouldn't affect search behavior. This pr updated the two throw error into console error instead.

Same as #1771 but without acceptance tests changes

TEST=manual

Added `ANSWERS.setConversionsOptIn(true);` to a local index page with Answers setup. see that the previously uncaught throw error is now a console error and a search on load is no longer blocked.

<img width="1786" alt="Screen Shot 2022-08-31 at 8 36 25 AM" src="https://user-images.githubusercontent.com/36055303/187679764-b5c9a6e6-bf12-4467-a42b-66fb3052846d.png">
yen-tt added a commit that referenced this pull request Sep 2, 2022
Throw error from not having `ytag` script tag for conversion tracking opt in would prevent a search fired on load. This is an aggressive behavior as analytics shouldn't affect search behavior. This pr updated the two throw error into console error instead.

TEST=manual

Added `ANSWERS.setConversionsOptIn(true);` to a local index page with Answers setup. see that the previously uncaught throw error is now a console error and a search on load is no longer blocked.

<img width="1782" alt="Screen Shot 2022-08-30 at 5 06 28 PM" src="https://user-images.githubusercontent.com/36055303/187543204-d6acca4d-9cbc-47db-a51d-2ca88b36aa34.png">
@yen-tt yen-tt mentioned this pull request Sep 2, 2022
yen-tt added a commit that referenced this pull request Sep 2, 2022
Throw error from not having `ytag` script tag for conversion tracking opt in would prevent a search fired on load. This is an aggressive behavior as analytics shouldn't affect search behavior. This pr updated the two throw error into console error instead.

Same as #1771 but without acceptance tests changes

TEST=manual

Added `ANSWERS.setConversionsOptIn(true);` to a local index page with Answers setup. see that the previously uncaught throw error is now a console error and a search on load is no longer blocked.

<img width="1786" alt="Screen Shot 2022-08-31 at 8 36 25 AM" src="https://user-images.githubusercontent.com/36055303/187679764-b5c9a6e6-bf12-4467-a42b-66fb3052846d.png">
yen-tt added a commit that referenced this pull request Sep 2, 2022
Throw error from not having `ytag` script tag for conversion tracking opt in would prevent a search fired on load. This is an aggressive behavior as analytics shouldn't affect search behavior. This pr updated the two throw error into console error instead.

Same as #1771 but without acceptance tests changes

TEST=manual

Added `ANSWERS.setConversionsOptIn(true);` to a local index page with Answers setup. see that the previously uncaught throw error is now a console error and a search on load is no longer blocked.

<img width="1786" alt="Screen Shot 2022-08-31 at 8 36 25 AM" src="https://user-images.githubusercontent.com/36055303/187679764-b5c9a6e6-bf12-4467-a42b-66fb3052846d.png">
@yen-tt yen-tt mentioned this pull request Sep 2, 2022
yen-tt added a commit that referenced this pull request Sep 2, 2022
### Bug Fixes
- Gracefully handle analytics-related error to prevent uncaught errors from blocking the search on load. (#1771 )
- Pinned the Google Maps API version to v3.47. This is the last one to officially support IE11. (#1681)
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