-
Notifications
You must be signed in to change notification settings - Fork 309
Added Application State to analytics #1601
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
Added Application State to analytics #1601
Conversation
76de656
to
5265b44
Compare
@baubrey91 I have added the do not review tag, please ensure CI is passing before we re-review. |
6bce253
to
1cdef7f
Compare
The two tests failing for me are |
Yes, these tests are flaky in CI. Try "re-run failed tests" |
1cdef7f
to
25e39e9
Compare
edbc07a
to
7f1502e
Compare
let payPalInstalled: Bool = application.isPayPalAppInstalled() | ||
|
||
let platform = "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 need this newline:
ae7ebaa
to
470efb8
Compare
f50b4eb
to
547b8e4
Compare
953ee3f
to
e7e5f92
Compare
/// Encapsulates a single event by it's name and timestamp. | ||
struct Event: Codable { | ||
|
||
/// The app's state within the device's lifecycle (ie active, inactive, background) |
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.
small nit:
/// The app's state within the device's lifecycle (ie active, inactive, background) | |
/// The app's state within the device's lifecycle (i.e active, inactive, background) |
looking good - ty for addressing the comments above! two quick notes - and happy to pair on this:
|
054119f
to
d50ff8f
Compare
nit: can we also add a unit test in |
d50ff8f
to
64c3b8b
Compare
I added a test in BTPayPalClient_Tests but am not positive if its the right naming convention or if you wanted me to just add an assert in another test. May need help. Thanks! |
XCTAssertEqual(mockAPIClient.postedShopperSessionID, "fake-shopper-session-id") | ||
} | ||
|
||
func testTokenize_applicationStateAnalytics() 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.
small nit:
func testTokenize_applicationStateAnalytics() async { | |
func testTokenize_whenSuccess_ sendsapplicationStateInAnalytics() async { |
64c3b8b
to
7db62da
Compare
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.
Lets add a changelog entry for this. There should be several similar entries when adding new tags to reference.
Also to confirm we have tested that doing something like backgrounding the app while sending an event fetches the correct state and does not cause a crash?
@jaxdesmarais I updated the changelog, wasn't sure if we wanted to do that as this is an internal feature for FPTI. For testing I set a repeated task to continue send up an analytic and checked to see if it was sent while background. Are you asking to test if the state changes while we are sending? Do you have an idea how to test that? Would this require setting up an async task in the app delegate. Thanks!
|
fed2f4c
to
4b02d57
Compare
It's generally helpful for us to know what version a FPTI change was made in, so still worth having one.
Sorry should have been more clear. I more meant backgrounding the app and ensuring the application state is correct since it's sent on main. So something like:
|
CHANGELOG.md
Outdated
* Analytics updates for PayPal's analytics service (FPTI) | ||
* Add `space_key` and `product_name` to `batch_params` | ||
* Add `context_type` to `event_params` | ||
* Add `application_state` to PayPal analytics for tracking UIApplication state |
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 needs to be moved under ## unreleased
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.
Aw man I did that initially, must have broke it when I rebased
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.
Aw man I did that initially, must have broke it when I rebased
|
||
let eventParams = [ | ||
FPTIBatchData.Event( | ||
applicationState: UIApplication.shared.applicationStateString, |
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 think we can hardcode this value for the test
4b02d57
to
5917c97
Compare
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 addressing comments and doing thorough testing! 🚀
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.
lgtm ~ just left a small nit comment 🚀
Co-authored-by: agedd <[email protected]>
Thank you for your contribution to Braintree.
Summary of changes
When sending up analytics we will also record the application state ie background, active, inactive.
Checklist
Authors
FPTI