From 88b4008e63983521a76e270a9fb9e724f503fdd2 Mon Sep 17 00:00:00 2001 From: Robert Jambrecic Date: Wed, 22 May 2024 15:51:00 +0200 Subject: [PATCH] Error handling in the create_ad_group_with_ad_and_keywords function implemented --- .../tools/_campaign_creation_team_tools.py | 136 +++++++++++++----- .../test_campaign_creation_team_tools.py | 112 +++++++++++---- 2 files changed, 186 insertions(+), 62 deletions(-) diff --git a/captn/captn_agents/backend/tools/_campaign_creation_team_tools.py b/captn/captn_agents/backend/tools/_campaign_creation_team_tools.py index ef7f73b8..4535e82d 100644 --- a/captn/captn_agents/backend/tools/_campaign_creation_team_tools.py +++ b/captn/captn_agents/backend/tools/_campaign_creation_team_tools.py @@ -4,6 +4,8 @@ from pydantic import BaseModel, Field, StringConstraints from typing_extensions import Annotated +from google_ads.model import RemoveResource + from ....google_ads.client import check_for_client_approval, google_ads_create_update from ..toolboxes import Toolbox from ._functions import ( @@ -96,7 +98,7 @@ class AdGroupWithAdAndKeywords(BaseModel): def _get_resource_id_from_response(response: str) -> str: - return response.split("/")[-1].replace(".", "") + return response.split("/")[-1].split("~")[-1].replace(".", "").strip() def _create_ad_group( @@ -178,6 +180,7 @@ def _create_ad_group_keywords( ], ad_group_with_ad_and_keywords: AdGroupWithAdAndKeywords, ad_group_id: str, + list_of_resources: List[RemoveResource], ) -> Union[Dict[str, Any], str]: response = "" for keyword in ad_group_with_ad_and_keywords.keywords: @@ -189,10 +192,101 @@ def _create_ad_group_keywords( ad_group_id=ad_group_id, customer_id=ad_group_with_ad_and_keywords.customer_id, ) + keyword_id = _get_resource_id_from_response(keyword_response) # type: ignore[arg-type] + list_of_resources.append( + RemoveResource( + customer_id=ad_group_with_ad_and_keywords.customer_id, + parent_id=ad_group_id, + resource_id=keyword_id, + resource_type="ad_group_criterion", + ) + ) response += f"Keyword '{keyword.keyword_text}': {keyword_response}\n" return response +def _remove_resources( + user_id: int, conv_id: int, list_of_resources: List[RemoveResource] +) -> None: + for remove_resource in list_of_resources[::-1]: + remove_response = google_ads_create_update( + user_id=user_id, + conv_id=conv_id, + recommended_modifications_and_answer_list=[], + ad=remove_resource, + endpoint="/remove-google-ads-resource", + already_checked_clients_approval=True, + ) + print(remove_response) + print("Resources removed successfully") + + +def _create_ad_group_with_ad_and_keywords( + ad_group_with_ad_and_keywords: Annotated[ + AdGroupWithAdAndKeywords, "An ad group with an ad and a list of keywords" + ], + context: Context, +) -> Union[Dict[str, Any], str]: + list_of_resources = [] + try: + ad_group_response = _create_ad_group( + user_id=context.user_id, + conv_id=context.conv_id, + recommended_modifications_and_answer_list=context.recommended_modifications_and_answer_list, + ad_group_with_ad_and_keywords=ad_group_with_ad_and_keywords, + ) + if isinstance(ad_group_response, dict): + return ad_group_response + response = f"Ad group '{ad_group_with_ad_and_keywords.ad_group.name}': {ad_group_response}\n" + ad_group_id = _get_resource_id_from_response(ad_group_response) + list_of_resources.append( + RemoveResource( + customer_id=ad_group_with_ad_and_keywords.customer_id, + resource_id=ad_group_id, + resource_type="ad_group", + ) + ) + + ad_group_ad_response = _create_ad_group_ad( + user_id=context.user_id, + conv_id=context.conv_id, + recommended_modifications_and_answer_list=context.recommended_modifications_and_answer_list, + ad_group_with_ad_and_keywords=ad_group_with_ad_and_keywords, + ad_group_id=ad_group_id, + ) + ad_group_ad_id = _get_resource_id_from_response(ad_group_ad_response) # type: ignore[arg-type] + list_of_resources.append( + RemoveResource( + customer_id=ad_group_with_ad_and_keywords.customer_id, + parent_id=ad_group_id, + resource_id=ad_group_ad_id, + resource_type="ad", + ) + ) + + response += f"Ad group ad with final url - '{ad_group_with_ad_and_keywords.ad_group_ad.final_url}': {ad_group_ad_response}\n" + + ad_group_keywords_response = _create_ad_group_keywords( + user_id=context.user_id, + conv_id=context.conv_id, + recommended_modifications_and_answer_list=context.recommended_modifications_and_answer_list, + ad_group_with_ad_and_keywords=ad_group_with_ad_and_keywords, + ad_group_id=ad_group_id, + list_of_resources=list_of_resources, + ) + response += ad_group_keywords_response # type: ignore + context.changes_made += f"\n{response}" + except Exception as e: + _remove_resources( + user_id=context.user_id, + conv_id=context.conv_id, + list_of_resources=list_of_resources, + ) + raise e + + return response + + def create_campaign_creation_team_toolbox( user_id: int, conv_id: int, @@ -230,55 +324,21 @@ def create_ad_group_with_ad_and_keywords( """ # TODO: If any of the creation fails, we should rollback the changes # and return the error message (delete ad_group, other resources will be deleted automatically) - - user_id = context.user_id - conv_id = context.conv_id - recommended_modifications_and_answer_list = ( - context.recommended_modifications_and_answer_list - ) - modification_function_parameters = {} modification_function_parameters["ad_group_with_ad_and_keywords"] = ( ad_group_with_ad_and_keywords.model_dump() ) error_msg = check_for_client_approval( modification_function_parameters=modification_function_parameters, - recommended_modifications_and_answer_list=recommended_modifications_and_answer_list, + recommended_modifications_and_answer_list=context.recommended_modifications_and_answer_list, ) if error_msg: raise ValueError(error_msg) - ad_group_response = _create_ad_group( - user_id=user_id, - conv_id=conv_id, - recommended_modifications_and_answer_list=recommended_modifications_and_answer_list, - ad_group_with_ad_and_keywords=ad_group_with_ad_and_keywords, - ) - if isinstance(ad_group_response, dict): - return ad_group_response - response = f"Ad group '{ad_group_with_ad_and_keywords.ad_group.name}': {ad_group_response}\n" - ad_group_id = _get_resource_id_from_response(ad_group_response) - - ad_group_ad_response = _create_ad_group_ad( - user_id=user_id, - conv_id=conv_id, - recommended_modifications_and_answer_list=recommended_modifications_and_answer_list, + return _create_ad_group_with_ad_and_keywords( ad_group_with_ad_and_keywords=ad_group_with_ad_and_keywords, - ad_group_id=ad_group_id, + context=context, ) - response += f"Ad group ad with final url - '{ad_group_with_ad_and_keywords.ad_group_ad.final_url}': {ad_group_ad_response}\n" - - ad_group_keywords_response = _create_ad_group_keywords( - user_id=user_id, - conv_id=conv_id, - recommended_modifications_and_answer_list=recommended_modifications_and_answer_list, - ad_group_with_ad_and_keywords=ad_group_with_ad_and_keywords, - ad_group_id=ad_group_id, - ) - response += ad_group_keywords_response # type: ignore - context.changes_made += f"\n{response}" - - return response add_shared_functions(toolbox) diff --git a/tests/ci/captn/captn_agents/backend/tools/test_campaign_creation_team_tools.py b/tests/ci/captn/captn_agents/backend/tools/test_campaign_creation_team_tools.py index 300eec84..a7350a7c 100644 --- a/tests/ci/captn/captn_agents/backend/tools/test_campaign_creation_team_tools.py +++ b/tests/ci/captn/captn_agents/backend/tools/test_campaign_creation_team_tools.py @@ -11,10 +11,13 @@ AdGroupCriterionForCreation, AdGroupForCreation, AdGroupWithAdAndKeywords, + _get_resource_id_from_response, + _remove_resources, create_campaign_creation_team_toolbox, ) from captn.captn_agents.backend.tools._functions import Context from captn.google_ads.client import clean_nones +from google_ads.model import RemoveResource from .helpers import check_llm_config_descriptions, check_llm_config_total_tools @@ -52,7 +55,38 @@ def test_llm_config(self) -> None: }, ) - def test_create_ad_group_with_ad_and_keywords(self) -> None: + @pytest.mark.parametrize( + "response", + [ + "Created customers/1212/adGroups/3434.", + "Created customers/7119828439/adGroupCriteria/1212~3434.", + ], + ) + def test_get_resource_id_from_response(self, response) -> None: + resource_id = _get_resource_id_from_response(response) + assert resource_id == "3434", resource_id + + @pytest.mark.parametrize( + "side_effect", + [ + [ + "Created customers/1212/adGroups/3434.", + "Created customers/1212/adGroupAds/3434~5656.", + "Created customers/1212/adGroupCriteria/3434~7878.", + "Created customers/1212/adGroupCriteria/3434~8989.", + ], + [ + "Created customers/1212/adGroups/3434.", + "Created customers/1212/adGroupAds/3434~5656.", + "Created customers/1212/adGroupCriteria/3434~7878.", + ValueError("Error: 400: The keyword text 'keyword1' is invalid."), + "Removed customers/1212/adGroupCriteria/3434~7878.", + "Removed customers/1212/adGroupAds/3434~5656.", + "Removed customers/1212/adGroups/3434.", + ], + ], + ) + def test_create_ad_group_with_ad_and_keywords(self, side_effect) -> None: ad_group_ad = AdGroupAdForCreation( final_url="https://www.example.com", headlines=["headline1", "headline2", "headline3"], @@ -86,17 +120,16 @@ def test_create_ad_group_with_ad_and_keywords(self) -> None: keywords=[keyword1, keyword2], ) - with unittest.mock.patch( - "captn.captn_agents.backend.tools._campaign_creation_team_tools.google_ads_create_update" - ) as mock_google_ads_create_update: - side_effect = [ - "Created customers/1212/adGroups/3434.", - "Created customers/1212/adGroupAds/3434~5656.", - "Created customers/1212/adGroupCriteria/3434~7878.", - "Created customers/1212/adGroupCriteria/3434~8989.", - ] - mock_google_ads_create_update.side_effect = side_effect - + with ( + unittest.mock.patch( + "captn.captn_agents.backend.tools._campaign_creation_team_tools.google_ads_create_update", + side_effect=side_effect, + ) as mock_google_ads_create_update, + unittest.mock.patch( + "captn.captn_agents.backend.tools._campaign_creation_team_tools._remove_resources", + wraps=_remove_resources, + ) as mock_remove_resources, + ): modification_function_params = clean_nones( { "ad_group_with_ad_and_keywords": ad_group_with_ad_and_keywords.model_dump() @@ -114,23 +147,54 @@ def test_create_ad_group_with_ad_and_keywords(self) -> None: "create_ad_group_with_ad_and_keywords" ) - response = create_ad_group_with_ad_and_keywords( - ad_group_with_ad_and_keywords=ad_group_with_ad_and_keywords, - context=context, - ) - - expected_response = f"""Ad group '{ad_group.name}': {side_effect[0]} + if isinstance(side_effect[3], ValueError): + with pytest.raises(ValueError): + create_ad_group_with_ad_and_keywords( + ad_group_with_ad_and_keywords=ad_group_with_ad_and_keywords, + context=context, + ) + + # 4 create calls (the last one failed) + 3 remove calls + assert mock_google_ads_create_update.call_count == 7 + mock_remove_resources.assert_called_once_with( + user_id=1, + conv_id=1, + list_of_resources=[ + RemoveResource( + customer_id="1111", + resource_id="3434", + resource_type="ad_group", + ), + RemoveResource( + customer_id="1111", + parent_id="3434", + resource_id="5656", + resource_type="ad", + ), + RemoveResource( + customer_id="1111", + parent_id="3434", + resource_id="7878", + resource_type="ad_group_criterion", + ), + ], + ) + + else: + response = create_ad_group_with_ad_and_keywords( + ad_group_with_ad_and_keywords=ad_group_with_ad_and_keywords, + context=context, + ) + + expected_response = f"""Ad group '{ad_group.name}': {side_effect[0]} Ad group ad with final url - '{ad_group_ad.final_url}': {side_effect[1]} Keyword '{keyword1.keyword_text}': {side_effect[2]} Keyword '{keyword2.keyword_text}': {side_effect[3]} """ - print() - print(f"{response=}") - print(f"{expected_response=}") - - assert mock_google_ads_create_update.call_count == 4 - assert response == expected_response, response + assert mock_google_ads_create_update.call_count == 4 + assert response == expected_response, response + mock_remove_resources.assert_not_called() class TestContext: