TDL-16008 add new search types #39
Conversation
CHANGELOG.md
Outdated
| @@ -1,5 +1,8 @@ | |||
| # Changelog | |||
|
|
|||
| ## 1.0.0 | |||
| * Add support for new search types news, googleNews, discover [#39](https://github.com/singer-io/tap-google-search-console/pull/39) | |||
There was a problem hiding this comment.
Can we also list (as another point here) that we're major version changing for general release?
| @@ -89,7 +89,7 @@ class IncrementalTableStream(BaseStream, ABC): | |||
| forced_replication_method = "INCREMENTAL" | |||
| replication_key = "date" | |||
| pagination = "body" | |||
| sub_types = ["web", "image", "video"] | |||
| sub_types = ["web", "image", "video", "news", "googleNews", "discover"] | |||
There was a problem hiding this comment.
any reason not to alphabetize these for long-term maintainability? If there's not a specific reason, I think we should given that it's used in multiple places in performance_reports as well
| # Excluding discover sub_type since Requests for Discover cannot be grouped by device. | ||
| # this also overwrites the sub_types attribute declared for class IncrementalTableStream | ||
| # in abstract.py | ||
| sub_types = ["web", "image", "video", "news", "googleNews"] |
| # grouping data for discover and googleNews | ||
| # this also overwrites the sub_types attribute declared for class IncrementalTableStream | ||
| # in abstract.py | ||
| sub_types = ["web", "image", "video", "news"] |
| # Skip the extraction for type discover | ||
| # Requests for Discover cannot be grouped by device | ||
| if sub_type == "discover" and "device" in self.body_params["dimensions"]: | ||
| self.body_params["dimensions"].remove("device") |
There was a problem hiding this comment.
This comment is confusing. We remove device and don't appear to skip extraction of discover type; is there something I'm missing? Is there documentation that shows discovery can't be grouped by device, and if so, can we add that to the relevant Jira or the code comment?
There was a problem hiding this comment.
my bad, updated the comment. I couldn't find any documentation on webmasters but these exceptions were found while testing. I have seen few issues reported on stackoverflow for the same
| # Skip the extraction for type discover | ||
| # Requests for Discover cannot be grouped by device | ||
| if sub_type == "discover" and "device" in self.body_params["dimensions"]: | ||
| self.body_params["dimensions"].remove("device") |
There was a problem hiding this comment.
Is this removing a selected field for the purpose of the request? If so, I think we should at a minimum write a log line that indicates that we're doing this.
There was a problem hiding this comment.
This isn't removing the selected field. So, for custom reports we consider all the dimensions(page, country, device, query, date) where as for performance_report_device we consider only device as dimension.
If user selects the device field in performance_report_custom and while replicating data for discover sub_type we end up encountering the error Requests for Discover cannot be grouped by device, to avoid this we simple remove device dimension from dimensions_list for discover sub_type. and this dimension_list is passed as param in API request
There was a problem hiding this comment.
this is very similar to not considering discover sub_type for performance_report_device stream here
Description of change
This PR adds support for new search type
news,googleNewsanddiscoverdocI thought this is quite simple to add these search_types by appending them to sub_types attribute defined in abstract.py but there are few exceptions for below streams:
discoversearch_type since Requests for Discover cannot be grouped by devicediscoverandgoogleNewssearch_types since Query is an invalid argument while grouping data for discover and googleNewsdiscoverandgoogleNewssearch_types when user selectsdiscoverandqueryfields for this stream. So, these are being filtered out during making payload for this stream.Note: This change will be released as major release because of two reasons
Manual QA steps
Risks
Rollback steps