Moves logRevenueV2 logic into trackLogRevenue method #39
Conversation
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
|
By analyzing the blame information on this pull request, we identified and undefined to be a potential reviewer. |
ladanazita
changed the title
brennan
reviewed
Oct 19, 2017
| - (void)trackLogRevenueV2:(NSDictionary *)properties andRevenueOrTotal:(NSNumber *)revenueOrTotal | ||
| { | ||
| id price = [properties objectForKey:@"price"] ?: revenueOrTotal; | ||
| id quantity = [properties objectForKey:@"quantity"] ?: [NSNumber numberWithInt:1]; |
There was a problem hiding this comment.
In Android, we set quantity to 1 if "price" is null but either revenue or total has a value. That's because Amplitude will calculate their internal revenue with price * quantity. Maybe we should guard against this in iOS as well: https://github.com/segment-integrations/analytics-android-integration-amplitude/blob/master/src/main/java/com/segment/analytics/android/integrations/amplitude/AmplitudeIntegration.java#L203-L211
f2prateek
reviewed
Oct 19, 2017
| @@ -42,7 +42,6 @@ + (NSNumber *)extractRevenueOrTotal:(NSDictionary *)dictionary withRevenueKey:(N | |||
|
|
|||
| if ([key caseInsensitiveCompare:totalKey] == NSOrderedSame) { | |||
| revenueOrTotal = dictionary[key]; | |||
| break; | |||
There was a problem hiding this comment.
Hmmm... why did this change?
There was a problem hiding this comment.
We want revenue to be used in cases where both total and revenue are present
There was a problem hiding this comment.
You'll want to add this reason as a comment in the codebase so the next developer (e.g. future you) does not look at it and think there's a missed break and thus a bug then put it back in.
There was a problem hiding this comment.
I will say that if a break was added, tests would fail
|
This is hard to review because of the huge diff. It would be better to break this up into commits or PR that are easier to review.
|
ladanazita
force-pushed
the
refactor/logRevenueV2
branch
from
d8e50cd
to
8872932
Compare
f2prateek
approved these changes
Oct 19, 2017
ladanazita
changed the title
|
Tests succeed with: Build succeeds with: |
tonyxiao
reviewed
Oct 19, 2017
| @@ -42,7 +42,6 @@ + (NSNumber *)extractRevenueOrTotal:(NSDictionary *)dictionary withRevenueKey:(N | |||
|
|
|||
| if ([key caseInsensitiveCompare:totalKey] == NSOrderedSame) { | |||
| revenueOrTotal = dictionary[key]; | |||
| break; | |||
There was a problem hiding this comment.
You'll want to add this reason as a comment in the codebase so the next developer (e.g. future you) does not look at it and think there's a missed break and thus a bug then put it back in.
| [self.amprevenue setReceipt:receipt]; | ||
| SEGLog(@"[[AMPRevenue revenue] setReceipt:%@];", receipt); | ||
| } | ||
| id revenueType = [properties objectForKey:@"revenueType"] ?: [properties objectForKey:@"revenue_type"]; |
There was a problem hiding this comment.
Generally it's preferred to use shorthand syntax via []. is there any reason you are not?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Build Succeeds with
xcodebuild -scheme Segment-Amplitude_Example -workspace Example/Segment-Amplitude.xcworkspaceTests succeed locally
xcodebuild test -scheme Segment-Amplitude_Example -workspace Example/Segment-Amplitude.xcworkspace -destination 'platform=iOS Simulator,name=iPhone 6'But fails with
When simulator is changed to
name=iPhone 5Travis and Podfile.lock issues are fixed on a separate branch:
#38