Merge pull request #3230 from woocommerce/fix/GOOWOO-321

[GOOWOO-321] Fix error editing asset group when Brand Guidelines is enabled

Author: joemcgill

src/API/Google/AdsAssetGroup.php

@@ -68,6 +68,13 @@ class AdsAssetGroup implements OptionsAwareInterface, ContainerAwareInterface {
 	 */
 	protected $asset_group_asset;
 
+	/**
+	 * The AdsCampaign class.
+	 *
+	 * @var AdsCampaign
+	 */
+	protected $campaign;
+
 	/**
 	 * List of asset group resource names.
 	 *
@@ -80,10 +87,12 @@ class AdsAssetGroup implements OptionsAwareInterface, ContainerAwareInterface {
 	 *
 	 * @param GoogleAdsClient    $client
 	 * @param AdsAssetGroupAsset $asset_group_asset
+	 * @param AdsCampaign        $campaign
 	 */
-	public function __construct( GoogleAdsClient $client, AdsAssetGroupAsset $asset_group_asset ) {
+	public function __construct( GoogleAdsClient $client, AdsAssetGroupAsset $asset_group_asset, AdsCampaign $campaign ) {
 		$this->client            = $client;
 		$this->asset_group_asset = $asset_group_asset;
+		$this->campaign          = $campaign;
 	}
 
 	/**
@@ -345,11 +354,56 @@ protected function get_assets( array $asset_groups ): array {
 
 		foreach ( $asset_group_ids as $asset_group_id ) {
 			$asset_groups[ $asset_group_id ]['assets'] = $assets[ $asset_group_id ] ?? (object) [];
+
+			// When Brand Guidelines is enabled, business name and logo are at campaign level; merge them for display.
+			$campaign_info = $this->get_campaign_info_by_asset_group_id( $asset_group_id );
+			if ( ! empty( $campaign_info['brand_guidelines_enabled'] ) && ! empty( $campaign_info['id'] ) ) {
+				$campaign_brand = $this->campaign->get_campaign_brand_assets_for_display( (int) $campaign_info['id'] );
+				$existing       = $asset_groups[ $asset_group_id ]['assets'];
+				$existing       = is_object( $existing ) ? (array) $existing : $existing;
+				if ( $campaign_brand['business_name'] !== null ) {
+					$existing['business_name'] = $campaign_brand['business_name'];
+				}
+				if ( ! empty( $campaign_brand['logo'] ) ) {
+					$existing['logo'] = $campaign_brand['logo'];
+				}
+				$asset_groups[ $asset_group_id ]['assets'] = $existing;
+			}
 		}
 
 		return $asset_groups;
 	}
 
+	/**
+	 * Get campaign information from asset group ID.
+	 *
+	 * @param int $asset_group_id The asset group ID.
+	 *
+	 * @return array Campaign information with 'id' and 'brand_guidelines_enabled' keys.
+	 */
+	protected function get_campaign_info_by_asset_group_id( int $asset_group_id ): array {
+		try {
+			$results = ( new AdsAssetGroupQuery() )
+				->set_client( $this->client, $this->options->get_ads_id() )
+				->add_columns( [ 'campaign.id', 'campaign.brand_guidelines_enabled' ] )
+				->where( 'asset_group.id', $asset_group_id, '=' )
+				->get_results();
+
+			foreach ( $results->iterateAllElements() as $row ) {
+				$campaign = $row->getCampaign();
+				return [
+					'id'                       => $campaign->getId(),
+					'brand_guidelines_enabled' => $campaign->getBrandGuidelinesEnabled(),
+				];
+			}
+
+			return [];
+		} catch ( ApiException $e ) {
+			do_action( 'woocommerce_gla_ads_client_exception', $e, __METHOD__ );
+			return [];
+		}
+	}
+
 	/**
 	 * Edit an asset group.
 	 *
@@ -362,7 +416,17 @@ protected function get_assets( array $asset_groups ): array {
 	 */
 	public function edit_asset_group( int $asset_group_id, array $data, array $assets = [] ): int {
 		try {
-			$operations = $this->asset_group_asset->edit_operations( $asset_group_id, $assets );
+			$is_brand_guidelines_enabled = false;
+			// Check if Brand Guidelines is enabled for this asset group's campaign.
+			$campaign_info = $this->get_campaign_info_by_asset_group_id( $asset_group_id );
+			if ( ! empty( $campaign_info['brand_guidelines_enabled'] ) && ! empty( $campaign_info['id'] ) ) {
+				$is_brand_guidelines_enabled = true;
+			}
+
+			$edit_result                  = $this->asset_group_asset->edit_operations( $asset_group_id, $assets, $is_brand_guidelines_enabled );
+			$operations                   = $edit_result['operations'];
+			$assets_for_creation          = $edit_result['assets_for_creation'] ?? [];
+			$created_asset_resource_names = $edit_result['created_asset_resource_names'] ?? [];
 
 			// PMax only supports one final URL but it is required to be an array.
 			if ( ! empty( $data['final_url'] ) ) {
@@ -375,6 +439,16 @@ public function edit_asset_group( int $asset_group_id, array $data, array $asset
 				$operations = [ $this->edit_operation( $asset_group_id, $data ), ...$operations ];
 			}
 
+			if ( ! empty( $campaign_info['brand_guidelines_enabled'] ) && ! empty( $campaign_info['id'] ) ) {
+				$brand_operations = $this->campaign->get_brand_asset_link_operations(
+					$campaign_info['id'],
+					$assets,
+					$assets_for_creation,
+					$created_asset_resource_names
+				);
+				$operations       = array_merge( $brand_operations, $operations );
+			}
+
 			if ( ! empty( $operations ) ) {
 				$this->mutate( $operations );
 			}

src/API/Google/AdsAssetGroupAsset.php

@@ -308,20 +308,30 @@ function ( $asset ) use ( $asset_group_id ) {
 	/**
 	 * Edit assets group assets.
 	 *
-	 * @param int   $asset_group_id The asset group id.
-	 * @param array $assets The assets to create.
+	 * When brand guidelines is enabled, business name and logo must NOT be linked at asset group level
+	 * (they must be CampaignAssets only, per Google Ads API requirements). This method skips creating
+	 * AssetGroupAsset operations for brand assets and returns the created asset data so the caller can
+	 * link them at campaign level instead.
 	 *
-	 * @return array The asset group asset operations.
+	 * @param int   $asset_group_id              The asset group id.
+	 * @param array $assets                      The assets to create.
+	 * @param bool  $is_brand_guidelines_enabled Whether brand guidelines is enabled for the asset group's campaign.
+	 *
+	 * @return array{operations: MutateOperation[], assets_for_creation: array, created_asset_resource_names: array} Asset group operations and creation data for campaign-level linking.
 	 * @throws Exception If the asset type is not supported.
 	 */
-	public function edit_operations( int $asset_group_id, array $assets ): array {
+	public function edit_operations( int $asset_group_id, array $assets, bool $is_brand_guidelines_enabled ): array {
 		if ( empty( $assets ) ) {
-			return [];
+			return [
+				'operations'                   => [],
+				'assets_for_creation'          => [],
+				'created_asset_resource_names' => [],
+			];
 		}
 
 		$asset_group_assets_operations        = [];
 		$assets_for_creation                  = $this->get_assets_to_be_created( $assets );
-		$asset_arns                           = $this->asset->create_assets( $assets_for_creation );
+		$asset_resource_names                 = $this->asset->create_assets( $assets_for_creation );
 		$total_assets                         = count( $assets_for_creation );
 		$delete_asset_group_assets_operations = [];
 
@@ -333,33 +343,54 @@ public function edit_operations( int $asset_group_id, array $assets ): array {
 		// The asset mutation operation results (ARNs) are returned in the same order as the operations are specified.
 		// See: https://youtu.be/9KaVjqW5tVM?t=103
 		for ( $i = 0; $i < $total_assets; $i++ ) {
-			$asset_group_assets_operations[] = $this->create_operation( $asset_group_id, $assets_for_creation[ $i ]['field_type'], $asset_arns[ $i ] );
+			$field_type = $assets_for_creation[ $i ]['field_type'];
+
+			// When brand guidelines is enabled, do NOT add asset group asset create for business_name or logo.
+			// Google Ads API requires them as CampaignAssets only, not AssetGroupAssets.
+			if ( $is_brand_guidelines_enabled && ( 'business_name' === $field_type || 'logo' === $field_type ) ) {
+				continue;
+			}
+
+			$asset_group_assets_operations[] = $this->create_operation( $asset_group_id, $field_type, $asset_resource_names[ $i ] );
 		}
 
 		foreach ( $this->get_assets_to_be_deleted( $assets ) as $asset ) {
+			// When Brand Guidelines is enabled, skip deleting business_name/logo from asset group level.
+			// They may not exist there (original implementation didn't link them at asset group level).
+			// They're managed at campaign level via CampaignAsset links.
+			if ( $is_brand_guidelines_enabled && isset( $asset['field_type'] ) && ( 'business_name' === $asset['field_type'] || 'logo' === $asset['field_type'] ) ) {
+				continue;
+			}
 			$delete_asset_group_assets_operations[] = $this->delete_operation( $asset_group_id, $asset['field_type'], $asset['id'] );
 		}
 
 		// The delete operations must be executed first otherwise will cause a conflict with existing assets with identical content.
 		// See here: https://github.com/woocommerce/google-listings-and-ads/pull/1870
-		return array_merge( $delete_asset_group_assets_operations, $asset_group_assets_operations );
-	}
+		$operations = array_merge( $delete_asset_group_assets_operations, $asset_group_assets_operations );
 
+		// Resource names (ARNs) from AdsAsset::create_assets() mutate response; same order as assets_for_creation.
+		// Used when brand guidelines is enabled to link business name/logo at campaign level.
+		return [
+			'operations'                   => $operations,
+			'assets_for_creation'          => $assets_for_creation,
+			'created_asset_resource_names' => $asset_resource_names,
+		];
+	}
 
 	/**
 	 * Creates an operation for an asset group asset.
 	 *
 	 * @param int    $asset_group_id The ID of the asset group.
 	 * @param string $asset_field_type The field type of the asset.
-	 * @param string $asset_arn The the asset ARN.
+	 * @param string $asset_resource_name The asset resource name.
 	 *
 	 * @return MutateOperation The mutate create operation for the asset group asset.
 	 */
-	protected function create_operation( int $asset_group_id, string $asset_field_type, string $asset_arn ): MutateOperation {
+	protected function create_operation( int $asset_group_id, string $asset_field_type, string $asset_resource_name ): MutateOperation {
 		$operation             = new AssetGroupAssetOperation();
 		$new_asset_group_asset = new AssetGroupAsset(
 			[
-				'asset'       => $asset_arn,
+				'asset'       => $asset_resource_name,
 				'asset_group' => ResourceNames::forAssetGroup( $this->options->get_ads_id(), $asset_group_id ),
 				'field_type'  => AssetFieldType::number( $asset_field_type ),
 			]