Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Label campaigns for the web version and the WC Mobile app #2514

Merged
merged 12 commits into from
Aug 12, 2024
10 changes: 10 additions & 0 deletions js/src/data/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
} from './constants';
import { handleApiError } from '.~/utils/handleError';
import { adaptAdsCampaign } from './adapters';
import { isWCIos, isWCAndroid } from '.~/utils/isMobileApp';

/**
* @typedef {import('.~/data/types.js').AssetEntityGroupUpdateBody} AssetEntityGroupUpdateBody
Expand Down Expand Up @@ -798,13 +799,22 @@ export function* saveTargetAudience( targetAudience ) {
* @throws { { message: string } } Will throw an error if the campaign creation fails.
*/
export function* createAdsCampaign( amount, countryCodes ) {
let label = 'wc-web';

if ( isWCIos() ) {
label = 'wc-ios';
} else if ( isWCAndroid() ) {
label = 'wc-android';
}

try {
const createdCampaign = yield apiFetch( {
path: `${ API_NAMESPACE }/ads/campaigns`,
method: 'POST',
data: {
amount,
targeted_locations: countryCodes,
label,
},
} );

Expand Down
92 changes: 92 additions & 0 deletions js/src/data/test/createAdsCampaign.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
/**
* External dependencies
*/
import { renderHook } from '@testing-library/react-hooks';
import apiFetch from '@wordpress/api-fetch';

/**
* Internal dependencies
*/
import { useAppDispatch } from '.~/data';

jest.mock( '@wordpress/api-fetch', () => {
const impl = jest.fn().mockName( '@wordpress/api-fetch' );
impl.use = jest.fn().mockName( 'apiFetch.use' );
return impl;
} );

describe( 'createAdsCampaign', () => {
let navigatorGetter;

const mockFetch = jest
.fn()
.mockResolvedValue( { targeted_locations: [ 'ES' ] } );

beforeEach( () => {
jest.clearAllMocks();
apiFetch.mockImplementation( ( args ) => {
return mockFetch( args );
} );

navigatorGetter = jest.spyOn( window.navigator, 'userAgent', 'get' );
} );

it( 'If the user agent is not set to wc-ios or wc-android, the label should default to wc-web', async () => {
const { result } = renderHook( () => useAppDispatch() );

await result.current.createAdsCampaign( 100, [ 'ES' ] );

expect( mockFetch ).toHaveBeenCalledTimes( 1 );
expect( mockFetch ).toHaveBeenCalledWith( {
path: '/wc/gla/ads/campaigns',
method: 'POST',
data: {
amount: 100,
targeted_locations: [ 'ES' ],
label: 'wc-web',
},
} );
} );

it( 'If the user agent is set to wc-ios the label should be wc-ios', async () => {
navigatorGetter.mockReturnValue(
'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'
);

const { result } = renderHook( () => useAppDispatch() );

await result.current.createAdsCampaign( 100, [ 'ES' ] );

expect( mockFetch ).toHaveBeenCalledTimes( 1 );
expect( mockFetch ).toHaveBeenCalledWith( {
path: '/wc/gla/ads/campaigns',
method: 'POST',
data: {
amount: 100,
targeted_locations: [ 'ES' ],
label: 'wc-ios',
},
} );
} );

it( 'If the user agent is set to wc-ios the label should be wc-android', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test title seems to have a copy paste error in it.

navigatorGetter.mockReturnValue(
'Mozilla/5.0 (iPhone; CPU iPhone OS 17_5_1 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Mobile/15E148 wc-android/19.7.1'
);

const { result } = renderHook( () => useAppDispatch() );

await result.current.createAdsCampaign( 100, [ 'ES' ] );

expect( mockFetch ).toHaveBeenCalledTimes( 1 );
expect( mockFetch ).toHaveBeenCalledWith( {
path: '/wc/gla/ads/campaigns',
method: 'POST',
data: {
amount: 100,
targeted_locations: [ 'ES' ],
label: 'wc-android',
},
} );
} );
} );
38 changes: 38 additions & 0 deletions js/src/utils/isMobile.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/**
* Internal dependencies
*/
import { isWCIos, isWCAndroid } from '.~/utils/isMobileApp';

describe( 'isMobile', () => {
let navigatorGetter;

beforeEach( () => {
// To initialize `pagePaths`.
navigatorGetter = jest.spyOn( window.navigator, 'userAgent', 'get' );
} );

it( 'isWCIos', () => {
navigatorGetter.mockReturnValue(
'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'
);

expect( isWCIos() ).toBe( true );
} );

it( 'isWCAndroid', () => {
navigatorGetter.mockReturnValue(
'Mozilla/5.0 (iPhone; CPU Samsung ) AppleWebKit/605.1.15 (KHTML, like Gecko) Mobile/15E148 wc-android/19.7.1'
);

expect( isWCAndroid() ).toBe( true );
} );

it( 'is not WCAndroid or isWCIos', () => {
navigatorGetter.mockReturnValue(
'Mozilla/5.0 (iPhone; CPU ) AppleWebKit/605.1.15 (KHTML, like Gecko)'
);

expect( isWCAndroid() ).toBe( false );
expect( isWCIos() ).toBe( false );
} );
} );
20 changes: 20 additions & 0 deletions js/src/utils/isMobileApp.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/**
* Check if the WC app is running on iOS.
*
* @return {boolean} Whether the WC app is running on iOS.
*
*/
const isWCIos = () => {
return window.navigator.userAgent.includes( 'wc-ios' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think this should be a problem, but in the PHP solution a non case sensitive solution was used, this one is case sensitive. If that's acceptable then ok to leave as is.

};

/**
* Check if the WC app is running on Android.
*
* @return {boolean} Whether the WC app is running on Android.
*/
const isWCAndroid = () => {
return window.navigator.userAgent.includes( 'wc-android' );
};

export { isWCIos, isWCAndroid };
7 changes: 6 additions & 1 deletion tests/Framework/RESTControllerUnitTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [] ): object {
protected function do_request( string $endpoint, string $type = 'GET', array $params = [], array $headers = [] ): object {
$request = new Request( $type, $endpoint );

if ( 'GET' === $type ) {
Expand All @@ -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 );
}
}
41 changes: 41 additions & 0 deletions tests/Unit/API/Site/Controllers/Ads/CampaignControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,47 @@ 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',
]
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_without_name() {
$campaign_data = [
'amount' => 30,
Expand Down