-
Notifications
You must be signed in to change notification settings - Fork 316
Card fields analytics #1817
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
Card fields analytics #1817
Changes from 4 commits
8b07605
bac4430
1dad0c4
019fa2b
1e4bc90
cffcce8
2335a9e
2922887
af4784c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| import BraintreeCard | ||
| import BraintreeCore | ||
| import Combine | ||
| import Foundation | ||
|
|
||
|
|
@@ -15,6 +16,7 @@ final class CardFieldsViewModel: ObservableObject { | |
|
|
||
| // MARK: - Private Properties | ||
|
|
||
| var apiClient: BTAPIClient | ||
| private let cardClient: BTCardClient | ||
| private let card: BTCard | ||
| private let completion: (BTCardNonce?, Error?) -> Void | ||
|
|
@@ -27,6 +29,7 @@ final class CardFieldsViewModel: ObservableObject { | |
| card: BTCard, | ||
| completion: @escaping (BTCardNonce?, Error?) -> Void | ||
| ) { | ||
| self.apiClient = BTAPIClient(authorization: authorization) | ||
| self.cardClient = BTCardClient(authorization: authorization) | ||
| self.card = card | ||
| self.completion = completion | ||
|
|
@@ -45,9 +48,15 @@ final class CardFieldsViewModel: ObservableObject { | |
|
|
||
| // MARK: - Internal Methods | ||
|
|
||
| func sendAnalyticsEvent(_ event: String) { | ||
| apiClient.sendAnalyticsEvent(event) | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: can we add a docstring to this? It took me a second to wrap my head around what was going on. Alternatively we could consider constructing the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep passing the client in from CardFields works! I briefly thought about going down that road early on, but I think it makes much more sense than having this weird phantom 2nd apiClient.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated again to remove the apiClient directly from the viewModel. I had to create a |
||
|
|
||
| func tokenize() { | ||
| guard isFormValid else { return } | ||
|
|
||
| sendAnalyticsEvent(UIComponentsAnalytics.cardFieldsSelected) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: could we call |
||
|
|
||
| let card = card.merging( | ||
| cardNumber: cardNumberViewModel.value, | ||
| expirationMonth: expirationDateViewModel.expirationMonth, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,43 @@ | ||||||
| import XCTest | ||||||
| import BraintreeCard | ||||||
| import BraintreeCore | ||||||
| @testable import BraintreeTestShared | ||||||
| @testable import BraintreeUIComponents | ||||||
|
|
||||||
| @MainActor | ||||||
| final class CardFieldsViewModelTests: XCTestCase { | ||||||
|
|
||||||
| private var mockAPIClient = MockAPIClient(authorization: "development_testing_tokenization_key_sandbox") | ||||||
| private var viewModel = CardFieldsViewModel( | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: it looks like we're initializing the same on line 19
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ope yep fixed! |
||||||
| authorization: "development_testing_tokenization_key_sandbox", | ||||||
| card: BTCard() | ||||||
| ) { _, _ in } | ||||||
|
|
||||||
| override func setUp() { | ||||||
| super.setUp() | ||||||
| mockAPIClient = MockAPIClient(authorization: "development_testing_tokenization_key_sandbox") | ||||||
| viewModel = CardFieldsViewModel( | ||||||
| authorization: "development_testing_tokenization_key_sandbox", | ||||||
| card: BTCard() | ||||||
| ) { _, _ in } | ||||||
| viewModel.apiClient = mockAPIClient | ||||||
| } | ||||||
|
|
||||||
| // MARK: - Analytics | ||||||
|
|
||||||
| func testSendAnalyticsEvent_cardFieldsPresented_postsCorrectEvent() { | ||||||
| viewModel.sendAnalyticsEvent(UIComponentsAnalytics.cardFieldsPresented) | ||||||
|
|
||||||
| XCTAssertTrue(mockAPIClient.postedAnalyticsEvents.contains(UIComponentsAnalytics.cardFieldsPresented)) | ||||||
| } | ||||||
|
|
||||||
| func testTokenize_whenFormIsValid_postsSubmittedEvent() { | ||||||
| viewModel.cardNumberViewModel.updateValue("4111111111111111") | ||||||
| viewModel.expirationDateViewModel.updateValue("12/45") | ||||||
| viewModel.cvvViewModel.updateValue("123") | ||||||
|
|
||||||
| viewModel.tokenize() | ||||||
|
|
||||||
| XCTAssertTrue(mockAPIClient.postedAnalyticsEvents.contains(UIComponentsAnalytics.cardFieldsSelected)) | ||||||
| } | ||||||
| } | ||||||
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.
does this need to be private 👀
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.
ah yep! Good call thanks
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.
Actually sorry it is exposed for testing like the current pattern for the other clients. I moved and added docstring to follow existing pattern
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.
Your comment and Jax's below sort of intersected, so ended up changing this up