Label campaigns for the web version and the WC Mobile app #2514
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| <?php | ||
| declare( strict_types=1 ); | ||
|
|
||
| namespace Automattic\WooCommerce\GoogleListingsAndAds\API\Site\Controllers; | ||
|
|
||
| use WP_REST_Request; | ||
|
|
||
| defined( 'ABSPATH' ) || exit; | ||
|
|
||
| /** | ||
| * Trait MobileTrait | ||
| * | ||
| * @package Automattic\WooCommerce\GoogleListingsAndAds\API | ||
| */ | ||
| trait MobileTrait { | ||
|
|
||
| /** | ||
| * Check if the request is from WooCommerce iOS app. | ||
| * | ||
| * @param WP_REST_Request $request | ||
| * | ||
| * @return bool | ||
| */ | ||
| public function is_wc_ios( WP_REST_Request $request ) { | ||
| return $request->get_header( 'User-Agent' ) && stristr( $request->get_header( 'User-Agent' ), 'wc-ios' ); | ||
| } | ||
|
|
||
| /** | ||
| * Check if the request is from WooCommerce Android app. | ||
| * | ||
| * @param WP_REST_Request $request | ||
| * | ||
| * @return bool | ||
| */ | ||
| public function is_wc_android( WP_REST_Request $request ) { | ||
| return $request->get_header( 'User-Agent' ) && stristr( $request->get_header( 'User-Agent' ), 'wc-android' ); | ||
| } | ||
|
|
||
| /** | ||
| * Check if the request is from WooCommerce mobile app. | ||
| * | ||
| * @param WP_REST_Request $request | ||
| * | ||
| * @return bool | ||
| */ | ||
| public function is_wc_mobile_app( WP_REST_Request $request ) { | ||
| return $this->is_wc_ios( $request ) || $this->is_wc_android( $request ); | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,10 +76,11 @@ public function tearDown(): void { | |
| * @param string $endpoint Endpoint to hit. | ||
| * @param string $type Type of request e.g GET or POST. | ||
| * @param array $params Request body or query. | ||
| * @param array $headers Request headers in format key => value. | ||
| * | ||
| * @return Response | ||
| */ | ||
| protected function do_request( string $endpoint, string $type = 'GET', array $params = [], array $headers = [] ): object { | ||
| $request = new Request( $type, $endpoint ); | ||
|
|
||
| if ( 'GET' === $type ) { | ||
|
|
@@ -90,6 +91,10 @@ protected function do_request( string $endpoint, string $type = 'GET', array $pa | |
| $request->set_body( wp_json_encode( $params ) ); | ||
| } | ||
|
|
||
| foreach ( $headers as $key => $value ) { | ||
| $request->set_header( $key, $value ); | ||
| } | ||
|
|
||
| return $this->server->dispatch_request( $request ); | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -332,6 +332,99 @@ public function test_create_campaign() { | |||||||
| $this->assertEquals( 200, $response->get_status() ); | ||||||||
| } | ||||||||
|
|
||||||||
| public function test_create_campaign_with_label() { | ||||||||
| $campaign_data = [ | ||||||||
| 'name' => 'New Campaign', | ||||||||
| 'amount' => 20, | ||||||||
| 'targeted_locations' => [ 'US', 'GB', 'TW' ], | ||||||||
| 'label' => 'wc-web', | ||||||||
| ]; | ||||||||
|
|
||||||||
| $expected = [ | ||||||||
| 'id' => self::TEST_CAMPAIGN_ID, | ||||||||
| 'status' => 'enabled', | ||||||||
| 'type' => 'performance_max', | ||||||||
| 'country' => self::BASE_COUNTRY, | ||||||||
| 'name' => 'New Campaign', | ||||||||
| 'amount' => 20, | ||||||||
| 'targeted_locations' => [ 'US', 'GB', 'TW' ], | ||||||||
| ]; | ||||||||
|
|
||||||||
| $this->ads_campaign->expects( $this->once() ) | ||||||||
| ->method( 'create_campaign' ) | ||||||||
| ->with( $campaign_data ) | ||||||||
| ->willReturn( $expected ); | ||||||||
|
|
||||||||
| $this->expect_track_event( | ||||||||
| 'created_campaign', | ||||||||
| [ | ||||||||
| 'id' => self::TEST_CAMPAIGN_ID, | ||||||||
| 'status' => 'enabled', | ||||||||
| 'name' => 'New Campaign', | ||||||||
| 'amount' => 20, | ||||||||
| 'country' => self::BASE_COUNTRY, | ||||||||
| 'targeted_locations' => 'US,GB,TW', | ||||||||
| ] | ||||||||
| ); | ||||||||
|
There was a problem hiding this comment. Just looking at this track here. Wouldn't it be useful to also pass the source? Or is it going to still include the user agent string so it can be extracted from there? |
||||||||
|
|
||||||||
| $response = $this->do_request( self::ROUTE_CAMPAIGNS, 'POST', $campaign_data ); | ||||||||
|
|
||||||||
| $this->assertEquals( $expected, $response->get_data() ); | ||||||||
| $this->assertEquals( 200, $response->get_status() ); | ||||||||
| } | ||||||||
|
|
||||||||
| public function test_create_campaign_with_mobile() { | ||||||||
| $user_agents = [ | ||||||||
| 'Mozilla/5.0 (iPhone; CPU iPhone OS 17_5_1 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Mobile/15E148 wc-ios/19.7.1', | ||||||||
| 'Mozilla/5.0 (Linux; Android 10; K) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/114.0.0.0 Mobile Safari/537.36 wc-android/19.7.1', | ||||||||
| ]; | ||||||||
|
|
||||||||
| $campaign_data = [ | ||||||||
| 'name' => 'New Campaign', | ||||||||
| 'amount' => 20, | ||||||||
| 'targeted_locations' => [ 'US', 'GB', 'TW' ], | ||||||||
| ]; | ||||||||
|
|
||||||||
| $expected = [ | ||||||||
| 'id' => self::TEST_CAMPAIGN_ID, | ||||||||
| 'status' => 'enabled', | ||||||||
| 'type' => 'performance_max', | ||||||||
| 'country' => self::BASE_COUNTRY, | ||||||||
| 'name' => 'New Campaign', | ||||||||
| 'amount' => 20, | ||||||||
| 'targeted_locations' => [ 'US', 'GB', 'TW' ], | ||||||||
| ]; | ||||||||
|
|
||||||||
| $matcher = $this->exactly( 2 ); | ||||||||
| $this->ads_campaign->expects( $matcher ) | ||||||||
| ->method( 'create_campaign' ) | ||||||||
| ->willReturnCallback( | ||||||||
| function ( $data ) use ( $campaign_data, $matcher, $expected ) { | ||||||||
| $invokation = $matcher->getInvocationCount(); | ||||||||
| if ( $invokation === 1 ) { | ||||||||
|
There was a problem hiding this comment. Not a big deal, but I would spell the variable with a
Suggested change
|
||||||||
| $this->assertEquals( $data, $campaign_data + [ 'label' => 'wc-ios' ] ); | ||||||||
| } else { | ||||||||
| $this->assertEquals( $data, $campaign_data + [ 'label' => 'wc-android' ] ); | ||||||||
| } | ||||||||
| return $expected; | ||||||||
| } | ||||||||
| ); | ||||||||
|
|
||||||||
| foreach ( $user_agents as $user_agent ) { | ||||||||
| $response = $this->do_request( | ||||||||
| self::ROUTE_CAMPAIGNS, | ||||||||
| 'POST', | ||||||||
| $campaign_data, | ||||||||
| [ | ||||||||
| 'User-Agent' => $user_agent, | ||||||||
| ] | ||||||||
| ); | ||||||||
|
|
||||||||
| $this->assertEquals( $expected, $response->get_data() ); | ||||||||
| $this->assertEquals( 200, $response->get_status() ); | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| public function test_create_campaign_without_name() { | ||||||||
| $campaign_data = [ | ||||||||
| 'amount' => 30, | ||||||||
|
|
||||||||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| <?php | ||
| declare( strict_types=1 ); | ||
|
|
||
| namespace Automattic\WooCommerce\GoogleListingsAndAds\Tests\Unit\API\Site\Controllers; | ||
|
|
||
| use Automattic\WooCommerce\GoogleListingsAndAds\API\Site\Controllers\MobileTrait; | ||
| use PHPUnit\Framework\TestCase; | ||
| use WP_REST_Request; | ||
|
|
||
| /** | ||
| * Class MobileTraitTest | ||
| * | ||
| * @package Automattic\WooCommerce\GoogleListingsAndAds\Tests\Unit\API\Site\Controllers | ||
| */ | ||
| class MobileTraitTest extends TestCase { | ||
|
|
||
| /** @var MobileTrait $trait */ | ||
| protected $trait; | ||
|
|
||
| /** | ||
| * Runs before each test is executed. | ||
| */ | ||
| public function setUp(): void { | ||
| parent::setUp(); | ||
|
|
||
| // phpcs:ignore Universal.Classes.RequireAnonClassParentheses.Missing | ||
| $this->trait = new class { | ||
| use MobileTrait; | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Test is a WooCommerce iOS app. | ||
| */ | ||
| public function test_is_wc_ios() { | ||
| $request = new WP_REST_Request(); | ||
| $request->set_header( 'User-Agent', 'Mozilla/5.0 AppleWebKit/605.1.15 (KHTML, like Gecko) Mobile/15E148 wc-ios/19.7.1' ); | ||
| $this->assertTrue( $this->trait->is_wc_ios( $request ) ); | ||
| $this->assertFalse( $this->trait->is_wc_android( $request ) ); | ||
| $this->assertTrue( $this->trait->is_wc_mobile_app( $request ) ); | ||
| } | ||
|
|
||
| /** | ||
| * Test is a WooCommerce Android app. | ||
| */ | ||
| public function test_is_wc_android() { | ||
| $request = new WP_REST_Request(); | ||
| $request->set_header( 'User-Agent', 'Mozilla/5.0 (Linux; Android 10; K) Chrome/114.0.0.0 Mobile Safari/537.36 wc-android/19.7.1' ); | ||
| $this->assertTrue( $this->trait->is_wc_android( $request ) ); | ||
| $this->assertFalse( $this->trait->is_wc_ios( $request ) ); | ||
| $this->assertTrue( $this->trait->is_wc_mobile_app( $request ) ); | ||
| } | ||
|
|
||
| /** | ||
| * Test is not a WooCommerce mobile app. | ||
| */ | ||
| public function test_is_not_wc_app() { | ||
| $request = new WP_REST_Request(); | ||
| $request->set_header( 'User-Agent', 'Mozilla/5.0 AppleWebKit/605.1.15 (KHTML, like Gecko)' ); | ||
| $this->assertFalse( $this->trait->is_wc_ios( $request ) ); | ||
| $this->assertFalse( $this->trait->is_wc_android( $request ) ); | ||
| $this->assertFalse( $this->trait->is_wc_mobile_app( $request ) ); | ||
| } | ||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I'm assuming this means we are defaulting the label to
wc-webeven on mobile, and then only overwriting the value once we process the request in PHP. It also means that when testing for mobile if I view the requests it's sending a request withwc-webbut the final campaign gets a different label.Is that in preparation for the mobile eventually calling the API directly? I was imagining that in this implementation we'd fetch the user agent in JS and then populate this label accordingly. Fine either approach though if it's better to detect the user agent in PHP.
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.
Thanks for the suggestion! My initial thought was to detect the header directly on the server, but I like the idea of moving the logic to the client. It keeps the API agnostic to the labels and lets the client handle setting them. If the mobile team wants to use the API to create campaigns, they can simply add the label parameter in the body.