-
Notifications
You must be signed in to change notification settings - Fork 256
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
[STRATCONN-5380] - Fix google enhanced conversion error parsing #2705
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2705 +/- ##
==========================================
- Coverage 78.90% 78.49% -0.41%
==========================================
Files 1089 1036 -53
Lines 21361 18774 -2587
Branches 4262 3562 -700
==========================================
- Hits 16854 14737 -2117
+ Misses 3310 2839 -471
- Partials 1197 1198 +1 ☔ View full report in Codecov by Sentry. |
throw new IntegrationError( | ||
(error as GoogleAdsError).response?.statusText, | ||
'INVALID_RESPONSE', | ||
(error as GoogleAdsError).response?.status | ||
) | ||
} |
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.
This INVALID Response is actually not useful. So, I removed it let the error bubble through in handleGoogleAdsError(error)
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.
🚀
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.
LGTM !
This PR fixes HTTP error parsing for
userList
action of Google Enhanced Conversions destination.This PR solves two key issues
data
wrapper is not needed.createOfflineJob
,addOpertions
andrun
(less likely) can throw this exception. (Previous PR)More on before and after state in Test Doc
This destination could benefit from Multistatus. I'll try to implement it in a different PR whenever I find time next.
Testing