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

Cleans up revenue logic #40

Merged
merged 2 commits into from
Oct 19, 2017
Merged

Cleans up revenue logic #40

merged 2 commits into from
Oct 19, 2017

Conversation

ladanazita
Copy link
Contributor

@ladanazita ladanazita commented Oct 19, 2017

  • To reduce nesting, return if useLogRevenueV2 is enabled after running through trackLogRevenueV2
  • Consolidate variable assignment with ternary conditional.
  • Remove type check on variables since users should pass in type as outlined in our ECommerce spec.

@ladanazita
Copy link
Contributor Author

Build succeeds with
xcodebuild -scheme Segment-Amplitude_Example -workspace Example/Segment-Amplitude.xcworkspace

Tests pass with xcodebuild test -scheme Segment-Amplitude_Example -workspace Example/Segment-Amplitude.xcworkspace -destination 'platform=iOS Simulator,name=iPhone 5':

Test Suite 'All tests' started at 2017-10-19 11:47:19.245
Test Suite 'Segment-Amplitude_Tests.xctest' started at 2017-10-19 11:47:19.246
Test Suite 'InitialSpecsSpec' started at 2017-10-19 11:47:19.246
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegrationFactory__factory_creates_integration_with_basic_settings]' started.
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegrationFactory__factory_creates_integration_with_basic_settings]' passed (0.020 seconds).
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegrationFactory__factory_creates_integration_with_trackSessionEvents]' started.
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegrationFactory__factory_creates_integration_with_trackSessionEvents]' passed (0.001 seconds).
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Identify__identify_without_traits]' started.
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Identify__identify_without_traits]' passed (0.003 seconds).
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Identify__identify_with_traits]' started.
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Identify__identify_with_traits]' passed (0.002 seconds).
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Identify__identify_with_groups]' started.
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Identify__identify_with_groups]' passed (0.001 seconds).
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Screen__does_not_call_screen_if_trackAllPages_false]' started.
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Screen__does_not_call_screen_if_trackAllPages_false]' passed (0.001 seconds).
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Screen__calls_basic_screen]' started.
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Screen__calls_basic_screen]' passed (0.001 seconds).
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Group__sets_groupId]' started.
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Group__sets_groupId]' passed (0.001 seconds).
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Flush__calls_uploadEvents]' started.
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Flush__calls_uploadEvents]' passed (0.001 seconds).
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Reset__calls_regenerateDeviceId]' started.
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Reset__calls_regenerateDeviceId]' passed (0.001 seconds).
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Track__tracks_a_basic_event_without_props]' started.
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Track__tracks_a_basic_event_without_props]' passed (0.001 seconds).
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Track__tracks_a_basic_event_with_props]' started.
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Track__tracks_a_basic_event_with_props]' passed (0.001 seconds).
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Track__tracks_a_basic_event_with_groups]' started.
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Track__tracks_a_basic_event_with_groups]' passed (0.001 seconds).
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Track__tracks_Order_Completed_with_revenue_if_both_total_and_revenue_are_present]' started.
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Track__tracks_Order_Completed_with_revenue_if_both_total_and_revenue_are_present]' passed (0.002 seconds).
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Track__tracks_Order_Completed_with_total_if_revenue_is_not_present]' started.
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Track__tracks_Order_Completed_with_total_if_revenue_is_not_present]' passed (0.001 seconds).
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Track__tracks_Order_Completed_with_revenue_of_type_String]' started.
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Track__tracks_Order_Completed_with_revenue_of_type_String]' passed (0.002 seconds).
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Track__tracks_with_top_level_price_and_quantity]' started.
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Track__tracks_with_top_level_price_and_quantity]' passed (0.001 seconds).
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Track__tracks_Amplitude_ecommerce_fields]' started.
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Track__tracks_Amplitude_ecommerce_fields]' passed (0.002 seconds).
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Track__fallsback_to_logRevenue_v1]' started.
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Track__fallsback_to_logRevenue_v1]' passed (0.002 seconds).
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Track__fallsback_to_logRevenue_v1_with_default_values]' started.
Test Case '-[InitialSpecsSpec test_SEGAmplitudeIntegration__Track__fallsback_to_logRevenue_v1_with_default_values]' passed (0.002 seconds).
Test Suite 'InitialSpecsSpec' passed at 2017-10-19 11:47:19.332.
	 Executed 20 tests, with 0 failures (0 unexpected) in 0.046 (0.086) seconds
Test Suite 'Segment-Amplitude_Tests.xctest' passed at 2017-10-19 11:47:19.333.
	 Executed 20 tests, with 0 failures (0 unexpected) in 0.046 (0.087) seconds
Test Suite 'All tests' passed at 2017-10-19 11:47:19.334.
	 Executed 20 tests, with 0 failures (0 unexpected) in 0.046 (0.088) seconds
** TEST SUCCEEDED **

Copy link
Contributor

@brennan brennan left a comment

Choose a reason for hiding this comment

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

LGTM!

@ladanazita ladanazita merged commit b282af1 into master Oct 19, 2017
@ladanazita ladanazita deleted the cleanup/logRevenueV2 branch October 19, 2017 20:35
Copy link
Contributor

@tonyxiao tonyxiao left a comment

Choose a reason for hiding this comment

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

There's one potentially crash scenario that you should put some attention on.

quantity = [NSNumber numberWithInt:1];
}

NSNumber *price = [properties objectForKey:@"price"] ?: revenueOrTotal;
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use the shorthand properties[@"price"] syntax here

id productId = [properties objectForKey:@"productId"] ?: [properties objectForKey:@"product_id"];
if (productId && [productId isKindOfClass:[NSString class]] && ![productId isEqualToString:@""]) {
NSString *productId = [properties objectForKey:@"productId"] ?: [properties objectForKey:@"product_id"];
if (productId && ![productId isEqualToString:@""]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is gonna crash if productId is not a NSString for some reason. You don't have compile time guarantee, so you're better off protecting against it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants