-
Notifications
You must be signed in to change notification settings - Fork 21
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
Conversation
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 getting this added, and including some unit tests.
I tested by alternating the user agent and can confirm it gets the right label when creating the campaign. I left a few smaller comments, but none of them are blocking so I'll go ahead and approve the PR.
js/src/data/actions.js
Outdated
@@ -805,6 +805,7 @@ export function* createAdsCampaign( amount, countryCodes ) { | |||
data: { | |||
amount, | |||
targeted_locations: countryCodes, | |||
label: 'wc-web', |
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-web
even 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 with wc-web
but 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.
if ( $this->is_wc_mobile_app( $request ) ) { | ||
if ( $this->is_wc_ios( $request ) ) { | ||
$fields['label'] = 'wc-ios'; | ||
} else { | ||
$fields['label'] = 'wc-android'; | ||
} | ||
} |
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.
PHP will do some optimization but if we follow the code path this will technically fetch the user agent header and then check if it contains the string wc-ios
twice. If we aren't going to optimize it so it only needs to compare the header once, then wouldn't it look slightly cleaner to check it like this:
if ( $this->is_wc_mobile_app( $request ) ) { | |
if ( $this->is_wc_ios( $request ) ) { | |
$fields['label'] = 'wc-ios'; | |
} else { | |
$fields['label'] = 'wc-android'; | |
} | |
} | |
if ( $this->is_wc_ios( $request ) ) { | |
$fields['label'] = 'wc-ios'; | |
} elseif ( $this->is_wc_android( $request ) ) { | |
$fields['label'] = 'wc-android'; | |
} |
$invokation = $matcher->getInvocationCount(); | ||
if ( $invokation === 1 ) { |
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.
Not a big deal, but I would spell the variable with a c
, or reduce the need for storing it in a variable.
$invokation = $matcher->getInvocationCount(); | |
if ( $invokation === 1 ) { | |
if ( 1 === $matcher->getInvocationCount() ) { |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2514 +/- ##
============================================
+ Coverage 63.5% 64.7% +1.2%
- Complexity 0 4580 +4580
============================================
Files 322 798 +476
Lines 5043 22940 +17897
Branches 1220 1222 +2
============================================
+ Hits 3204 14844 +11640
- Misses 1672 7930 +6258
+ Partials 167 166 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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 changes, it looks like a much simpler implementation, and continues to work as expected.
I just left a few comments / thoughts, but none of them are blocking.
} ); | ||
} ); | ||
|
||
it( 'If the user agent is set to wc-ios the label should be wc-android', async () => { |
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.
This test title seems to have a copy paste error in it.
js/src/utils/isMobileApp.js
Outdated
* | ||
*/ | ||
const isWCIos = () => { | ||
return window.navigator.userAgent.includes( 'wc-ios' ); |
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.
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.
$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.
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?
Thanks @mikkamp for the review! I've addressed all your comments.
I think it is good to add the source in the event tracking so I've added it here: aa6185e |
Changes proposed in this Pull Request:
Part of pcTzPl-2nS-p2
This PR will automatically label campaigns created via the WC Mobile App by checking the user agent headers. The labels used will be
wc-android
orwc-ios
. If the campaign isn't created through the WC Mobile App, it will be labeled aswc-web
.Screenshots:
Detailed test instructions:
POST /wp-json/wc/gla/ads/campaigns
with theUser Agent
header set to something like: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
. Set the below body:wc-ios
orwc-android
.wc-web
Additional details:
Changelog entry