From 1c3808da3265d00440b073d06dc4af0b15a2bdb4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ant=C3=B4nio=20Moreira?= Date: Fri, 10 Sep 2021 12:02:32 -0300 Subject: [PATCH 1/6] Throw Exception if the offline conversion name can't be found in the Ads account Change-Id: I33bdb7cbfbbbae580a5f4068c456e49d6bf0a8ad --- .../conversions/google_ads_offline_conversions_uploader.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/megalista_dataflow/uploaders/google_ads/conversions/google_ads_offline_conversions_uploader.py b/megalista_dataflow/uploaders/google_ads/conversions/google_ads_offline_conversions_uploader.py index a439f506..2e9d2878 100644 --- a/megalista_dataflow/uploaders/google_ads/conversions/google_ads_offline_conversions_uploader.py +++ b/megalista_dataflow/uploaders/google_ads/conversions/google_ads_offline_conversions_uploader.py @@ -93,16 +93,14 @@ def _do_upload(oc_service, conversion_resource_name, customer_id, rows): 'conversions': conversions } - response = oc_service.upload_click_conversions(request=upload_data) utils.print_partial_error_messages(_DEFAULT_LOGGER, 'uploading offline conversions', response) def _get_resource_name(self, customer_id: str, name: str): - resource_name = None service = self._get_ads_service(customer_id) query = f"SELECT conversion_action.resource_name FROM conversion_action WHERE conversion_action.name = '{name}'" response_query = service.search_stream(customer_id=customer_id, query=query) for batch in response_query: for row in batch.results: - resource_name = row.conversion_action.resource_name - return resource_name + return row.conversion_action.resource_name + raise Exception(f'Conversion "{name}" could not be found on account {customer_id}') From ce46fed0730dd1e13ac1c6e53272513918270c6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ant=C3=B4nio=20Moreira?= Date: Fri, 10 Sep 2021 12:02:55 -0300 Subject: [PATCH 2/6] Enable partial failures to better log erros Change-Id: I0bf127ac64eca7727996e072f226036af78e49f8 --- .../conversions/google_ads_offline_conversions_uploader.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/megalista_dataflow/uploaders/google_ads/conversions/google_ads_offline_conversions_uploader.py b/megalista_dataflow/uploaders/google_ads/conversions/google_ads_offline_conversions_uploader.py index 2e9d2878..21654a20 100644 --- a/megalista_dataflow/uploaders/google_ads/conversions/google_ads_offline_conversions_uploader.py +++ b/megalista_dataflow/uploaders/google_ads/conversions/google_ads_offline_conversions_uploader.py @@ -88,7 +88,7 @@ def _do_upload(oc_service, conversion_resource_name, customer_id, rows): upload_data = { 'customer_id': customer_id, - 'partial_failure': False, + 'partial_failure': True, 'validate_only': False, 'conversions': conversions } From c1b05c4429d0f5e8f386aaba026346db53135695 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ant=C3=B4nio=20Moreira?= Date: Fri, 10 Sep 2021 12:10:20 -0300 Subject: [PATCH 3/6] Doesn't make sense in the current version of Megalista. Uploaders are activated by configuration nowadays not by the presence of parameters. Change-Id: Ia1ef3f68b142ac9c932770e73b3e546945daa705 --- .../google_ads_offline_conversions_uploader.py | 5 ----- .../google_ads_offline_conversions_uploader_test.py | 12 ------------ 2 files changed, 17 deletions(-) diff --git a/megalista_dataflow/uploaders/google_ads/conversions/google_ads_offline_conversions_uploader.py b/megalista_dataflow/uploaders/google_ads/conversions/google_ads_offline_conversions_uploader.py index 21654a20..a8b8e143 100644 --- a/megalista_dataflow/uploaders/google_ads/conversions/google_ads_offline_conversions_uploader.py +++ b/megalista_dataflow/uploaders/google_ads/conversions/google_ads_offline_conversions_uploader.py @@ -28,7 +28,6 @@ def __init__(self, oauth_credentials, developer_token): super().__init__() self.oauth_credentials = oauth_credentials self.developer_token = developer_token - self.active = self.developer_token is not None def _get_ads_service(self, customer_id: str): return utils.get_ads_service('GoogleAdsService', ADS_API_VERSION, @@ -59,10 +58,6 @@ def _assert_conversion_name_is_present(execution: Execution): @utils.safe_process( logger=logging.getLogger('megalista.GoogleAdsOfflineUploader')) def process(self, batch: Batch, **kwargs): - if not self.active: - logging.getLogger().warning( - 'Skipping upload, parameters not configured.') - return execution = batch.execution self._assert_conversion_name_is_present(execution) diff --git a/megalista_dataflow/uploaders/google_ads/conversions/google_ads_offline_conversions_uploader_test.py b/megalista_dataflow/uploaders/google_ads/conversions/google_ads_offline_conversions_uploader_test.py index fade617a..9963ddcb 100644 --- a/megalista_dataflow/uploaders/google_ads/conversions/google_ads_offline_conversions_uploader_test.py +++ b/megalista_dataflow/uploaders/google_ads/conversions/google_ads_offline_conversions_uploader_test.py @@ -44,18 +44,6 @@ def test_get_service(mocker, uploader): assert uploader._get_oc_service(mocker.ANY) is not None -def test_not_active(mocker, caplog): - credential_id = StaticValueProvider(str, 'id') - secret = StaticValueProvider(str, 'secret') - access = StaticValueProvider(str, 'access') - refresh = StaticValueProvider(str, 'refresh') - credentials = OAuthCredentials(credential_id, secret, access, refresh) - uploader_dofn = GoogleAdsOfflineUploaderDoFn(credentials, None) - mocker.patch.object(uploader_dofn, '_get_oc_service') - uploader_dofn.process(Batch(None, [])) - uploader_dofn._get_oc_service.assert_not_called() - assert 'Skipping upload, parameters not configured.' in caplog.text - def test_conversion_upload(mocker, uploader): mocker.patch.object(uploader, '_get_oc_service') conversion_name = 'user_list' From 77bbcf70ebffff28dc74d6c34ba94fb9dc8c7917 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ant=C3=B4nio=20Moreira?= Date: Fri, 10 Sep 2021 12:48:51 -0300 Subject: [PATCH 4/6] Add resource name lookup to test Change-Id: I059010ab5ef65f81ab927d2da9b0d4e6da1f32c3 --- ...e_ads_offline_conversions_uploader_test.py | 33 +++++++++++++++---- 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/megalista_dataflow/uploaders/google_ads/conversions/google_ads_offline_conversions_uploader_test.py b/megalista_dataflow/uploaders/google_ads/conversions/google_ads_offline_conversions_uploader_test.py index 9963ddcb..ffcaee0b 100644 --- a/megalista_dataflow/uploaders/google_ads/conversions/google_ads_offline_conversions_uploader_test.py +++ b/megalista_dataflow/uploaders/google_ads/conversions/google_ads_offline_conversions_uploader_test.py @@ -11,9 +11,11 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +from unittest.mock import MagicMock from apache_beam.options.value_provider import StaticValueProvider import pytest + from uploaders.google_ads.conversions.google_ads_offline_conversions_uploader import GoogleAdsOfflineUploaderDoFn from models.execution import AccountConfig from models.execution import Destination @@ -24,7 +26,7 @@ from models.execution import Batch from models.oauth_credentials import OAuthCredentials -_account_config = AccountConfig('account_id', False, 'ga_account_id', '', '') +_account_config = AccountConfig('123-45567-890', False, 'ga_account_id', '', '') @pytest.fixture @@ -45,6 +47,19 @@ def test_get_service(mocker, uploader): def test_conversion_upload(mocker, uploader): + # arrange + mocker.patch.object(uploader, '_get_ads_service') + + conversion_resource_name = 'user_list_resouce' + resource_name_result = MagicMock() + resource_name_result.conversion_action.resource_name = conversion_resource_name + + resource_name_batch_response = MagicMock() + resource_name_batch_response.results = [resource_name_result] + + uploader._get_ads_service.return_value.search_stream.return_value = [resource_name_batch_response] + + # act mocker.patch.object(uploader, '_get_oc_service') conversion_name = 'user_list' destination = Destination( @@ -69,17 +84,23 @@ def test_conversion_upload(mocker, uploader): }]) uploader.process(batch) - uploader._get_oc_service.return_value.upload_click_conversions.assert_any_call(request = { - 'customer_id': 'account_id', - 'partial_failure': False, + #assert + uploader._get_ads_service.return_value.search_stream.assert_called_once_with( + customer_id='12345567890', + query=f"SELECT conversion_action.resource_name FROM conversion_action WHERE conversion_action.name = '{conversion_name}'" + ) + + uploader._get_oc_service.return_value.upload_click_conversions.assert_called_once_with(request={ + 'customer_id': '12345567890', + 'partial_failure': True, 'validate_only': False, 'conversions': [{ - 'conversion_action': None, + 'conversion_action': conversion_resource_name, 'conversion_date_time': time1_result, 'conversion_value': 123, 'gclid': '456' }, { - 'conversion_action': None, + 'conversion_action': conversion_resource_name, 'conversion_date_time': time2_result, 'conversion_value': 234, 'gclid': '567' From 8d83512e308bffe651dc7aaf6ee06cd4cf655853 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ant=C3=B4nio=20Moreira?= Date: Mon, 13 Sep 2021 11:59:27 -0300 Subject: [PATCH 5/6] Add possibility to specify a Ads Account ID as METADATA 2 for Ads Offline Conversions Change-Id: Iedb18d23679d2665c0915a67344ac55a830cd0ff --- ...google_ads_offline_conversions_uploader.py | 15 ++++- ...e_ads_offline_conversions_uploader_test.py | 67 +++++++++++++++++-- 2 files changed, 75 insertions(+), 7 deletions(-) diff --git a/megalista_dataflow/uploaders/google_ads/conversions/google_ads_offline_conversions_uploader.py b/megalista_dataflow/uploaders/google_ads/conversions/google_ads_offline_conversions_uploader.py index a8b8e143..391c9253 100644 --- a/megalista_dataflow/uploaders/google_ads/conversions/google_ads_offline_conversions_uploader.py +++ b/megalista_dataflow/uploaders/google_ads/conversions/google_ads_offline_conversions_uploader.py @@ -15,7 +15,7 @@ import logging import apache_beam as beam -from models.execution import Batch, Execution +from models.execution import Batch, Execution, AccountConfig, Destination from uploaders import utils from uploaders.google_ads import ADS_API_VERSION @@ -44,10 +44,19 @@ def _get_oc_service(self, customer_id): def start_bundle(self): pass + def _get_customer_id(self, account_config:AccountConfig, destination:Destination) -> str: + """ + If the customer_id is present on the destination, returns it, otherwise defaults to the account_config info. + """ + if len(destination.destination_metadata) >= 2 and len(destination.destination_metadata[1]) > 0: + return destination.destination_metadata[1].replace('-', '') + return account_config.google_ads_account_id.replace('-', '') + + @staticmethod def _assert_conversion_name_is_present(execution: Execution): destination = execution.destination.destination_metadata - if len(destination) != 1: + if len(destination) is 0: raise ValueError('Missing destination information. Found {}'.format( len(destination))) @@ -61,7 +70,7 @@ def process(self, batch: Batch, **kwargs): execution = batch.execution self._assert_conversion_name_is_present(execution) - customer_id = execution.account_config.google_ads_account_id.replace('-', '') + customer_id = self._get_customer_id(execution.account_config, execution.destination) oc_service = self._get_oc_service(customer_id) resource_name = self._get_resource_name(customer_id, execution.destination.destination_metadata[0]) diff --git a/megalista_dataflow/uploaders/google_ads/conversions/google_ads_offline_conversions_uploader_test.py b/megalista_dataflow/uploaders/google_ads/conversions/google_ads_offline_conversions_uploader_test.py index ffcaee0b..f8415dff 100644 --- a/megalista_dataflow/uploaders/google_ads/conversions/google_ads_offline_conversions_uploader_test.py +++ b/megalista_dataflow/uploaders/google_ads/conversions/google_ads_offline_conversions_uploader_test.py @@ -46,11 +46,9 @@ def test_get_service(mocker, uploader): assert uploader._get_oc_service(mocker.ANY) is not None -def test_conversion_upload(mocker, uploader): - # arrange +def arrange_conversion_resource_name_api_call(mocker, uploader, conversion_resource_name): mocker.patch.object(uploader, '_get_ads_service') - conversion_resource_name = 'user_list_resouce' resource_name_result = MagicMock() resource_name_result.conversion_action.resource_name = conversion_resource_name @@ -59,7 +57,11 @@ def test_conversion_upload(mocker, uploader): uploader._get_ads_service.return_value.search_stream.return_value = [resource_name_batch_response] - # act +def test_conversion_upload(mocker, uploader): + # arrange + conversion_resource_name = 'user_list_resouce' + arrange_conversion_resource_name_api_call(mocker, uploader, conversion_resource_name) + mocker.patch.object(uploader, '_get_oc_service') conversion_name = 'user_list' destination = Destination( @@ -82,6 +84,8 @@ def test_conversion_upload(mocker, uploader): 'amount': '234', 'gclid': '567' }]) + + # act uploader.process(batch) #assert @@ -106,3 +110,58 @@ def test_conversion_upload(mocker, uploader): 'gclid': '567' }] }) + + +def test_upload_with_ads_account_override(mocker, uploader): + # arrange + conversion_resource_name = 'user_list_resouce' + arrange_conversion_resource_name_api_call(mocker, uploader, conversion_resource_name) + + mocker.patch.object(uploader, '_get_oc_service') + conversion_name = 'user_list' + destination = Destination( + 'dest1', DestinationType.ADS_OFFLINE_CONVERSION, ['user_list', '987-7654-123']) + source = Source('orig1', SourceType.BIG_QUERY, ['dt1', 'buyers']) + execution = Execution(_account_config, source, destination) + + time1 = '2020-04-09T14:13:55.0005' + time1_result = '2020-04-09 14:13:55-03:00' + + time2 = '2020-04-09T13:13:55.0005' + time2_result = '2020-04-09 13:13:55-03:00' + + batch = Batch(execution, [{ + 'time': time1, + 'amount': '123', + 'gclid': '456' + }, { + 'time': time2, + 'amount': '234', + 'gclid': '567' + }]) + + # act + uploader.process(batch) + + # assert + uploader._get_ads_service.return_value.search_stream.assert_called_once_with( + customer_id='9877654123', + query=f"SELECT conversion_action.resource_name FROM conversion_action WHERE conversion_action.name = '{conversion_name}'" + ) + + uploader._get_oc_service.return_value.upload_click_conversions.assert_called_once_with(request={ + 'customer_id': '9877654123', + 'partial_failure': True, + 'validate_only': False, + 'conversions': [{ + 'conversion_action': conversion_resource_name, + 'conversion_date_time': time1_result, + 'conversion_value': 123, + 'gclid': '456' + }, { + 'conversion_action': conversion_resource_name, + 'conversion_date_time': time2_result, + 'conversion_value': 234, + 'gclid': '567' + }] + }) \ No newline at end of file From fbaa57d10aedfd45b6a3466fd33ba43d35126e5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ant=C3=B4nio=20Moreira?= Date: Mon, 13 Sep 2021 17:06:54 -0300 Subject: [PATCH 6/6] Delete copied test in wrong place Change-Id: I9e32b447523a24d4d474f08e09e21b4221497e23 --- .../contact_info_uploader_test.py | 99 ------------------- 1 file changed, 99 deletions(-) delete mode 100644 megalista_dataflow/uploaders/google_ads/customer_match/contact_info_uploader_test.py diff --git a/megalista_dataflow/uploaders/google_ads/customer_match/contact_info_uploader_test.py b/megalista_dataflow/uploaders/google_ads/customer_match/contact_info_uploader_test.py deleted file mode 100644 index fade617a..00000000 --- a/megalista_dataflow/uploaders/google_ads/customer_match/contact_info_uploader_test.py +++ /dev/null @@ -1,99 +0,0 @@ -# Copyright 2021 Google LLC -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# https://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -from apache_beam.options.value_provider import StaticValueProvider -import pytest -from uploaders.google_ads.conversions.google_ads_offline_conversions_uploader import GoogleAdsOfflineUploaderDoFn -from models.execution import AccountConfig -from models.execution import Destination -from models.execution import DestinationType -from models.execution import Execution -from models.execution import Source -from models.execution import SourceType -from models.execution import Batch -from models.oauth_credentials import OAuthCredentials - -_account_config = AccountConfig('account_id', False, 'ga_account_id', '', '') - - -@pytest.fixture -def uploader(mocker): - mocker.patch('google.ads.googleads.client.GoogleAdsClient') - mocker.patch('google.ads.googleads.oauth2') - credential_id = StaticValueProvider(str, 'id') - secret = StaticValueProvider(str, 'secret') - access = StaticValueProvider(str, 'access') - refresh = StaticValueProvider(str, 'refresh') - credentials = OAuthCredentials(credential_id, secret, access, refresh) - return GoogleAdsOfflineUploaderDoFn(credentials, - StaticValueProvider(str, 'devtoken')) - - -def test_get_service(mocker, uploader): - assert uploader._get_oc_service(mocker.ANY) is not None - - -def test_not_active(mocker, caplog): - credential_id = StaticValueProvider(str, 'id') - secret = StaticValueProvider(str, 'secret') - access = StaticValueProvider(str, 'access') - refresh = StaticValueProvider(str, 'refresh') - credentials = OAuthCredentials(credential_id, secret, access, refresh) - uploader_dofn = GoogleAdsOfflineUploaderDoFn(credentials, None) - mocker.patch.object(uploader_dofn, '_get_oc_service') - uploader_dofn.process(Batch(None, [])) - uploader_dofn._get_oc_service.assert_not_called() - assert 'Skipping upload, parameters not configured.' in caplog.text - -def test_conversion_upload(mocker, uploader): - mocker.patch.object(uploader, '_get_oc_service') - conversion_name = 'user_list' - destination = Destination( - 'dest1', DestinationType.ADS_OFFLINE_CONVERSION, ['user_list']) - source = Source('orig1', SourceType.BIG_QUERY, ['dt1', 'buyers']) - execution = Execution(_account_config, source, destination) - - time1 = '2020-04-09T14:13:55.0005' - time1_result = '2020-04-09 14:13:55-03:00' - - time2 = '2020-04-09T13:13:55.0005' - time2_result = '2020-04-09 13:13:55-03:00' - - batch = Batch(execution, [{ - 'time': time1, - 'amount': '123', - 'gclid': '456' - },{ - 'time': time2, - 'amount': '234', - 'gclid': '567' - }]) - uploader.process(batch) - - uploader._get_oc_service.return_value.upload_click_conversions.assert_any_call(request = { - 'customer_id': 'account_id', - 'partial_failure': False, - 'validate_only': False, - 'conversions': [{ - 'conversion_action': None, - 'conversion_date_time': time1_result, - 'conversion_value': 123, - 'gclid': '456' - }, { - 'conversion_action': None, - 'conversion_date_time': time2_result, - 'conversion_value': 234, - 'gclid': '567' - }] - })