Skip to content

Provide whitelisted event initializer #93

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.

Sign up for GitHub

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

Merged
rclee merged 2 commits into from
Jan 3, 2019
Merged

Provide whitelisted event initializer #93

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ddccf64
Provide whitelisted event initializer
rclee Jan 3, 2019
de8e7fe
Fix test
rclee Jan 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion MapboxMobileEvents.xcodeproj/xcshareddata/xcschemes/MapboxMobileMetrics.xcscheme
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?xml version="1.0" encoding="UTF-8"?>
<Scheme
LastUpgradeVersion = "1000"
LastUpgradeVersion = "1010"
version = "1.3">
<BuildAction
parallelizeBuildables = "YES"
Expand Down
1 change: 1 addition & 0 deletions MapboxMobileEvents/MMEEvent.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,6 @@
+ (instancetype)debugEventWithAttributes:(NSDictionary *)attributes;
+ (instancetype)searchEventWithName:(NSString *)name attributes:(NSDictionary *)attributes;
+ (instancetype)carplayEventWithName:(NSString *)name attributes:(NSDictionary *)attributes;
+ (instancetype)eventWithDateString:(NSString *)dateString name:(NSString *)name attributes:(NSDictionary *)attributes;

@end
11 changes: 11 additions & 0 deletions MapboxMobileEvents/MMEEvent.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,17 @@ + (instancetype)carplayEventWithName:(NSString *)name attributes:(NSDictionary *
return carplayEvent;
}

+ (instancetype)eventWithDateString:(NSString *)dateString name:(NSString *)name attributes:(NSDictionary *)attributes {
Copy link
Contributor

@julianrex julianrex Jan 3, 2019

Choose a reason for hiding this comment

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

Just a couple of questions here (which I think we've talked about before briefly):

  1. Why is this a class method rather than an MMEEvent initializer? Is this for future support for different MMEEvent subclasses? So does this mean -[MMEEvent init] is essentially "private"? If so, I wonder if it's worth having a specific initializer (in addition) and marking the default init as NS_UNAVAILABLE. Thoughts?
  2. Should this method take an NSDate rather than a date string? Perhaps a date transformer could be supplied (somewhere).

Copy link
Contributor Author

@rclee rclee Jan 3, 2019

Choose a reason for hiding this comment

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

This PR is somewhat of a stopgap measure until we can completely remove the whitelisting that is currently in place. The doc describes some of those plans and how we might accomplish them.

  1. I'm not sure how the decision came about to make these class methods or what the future plans were for it. I don't see any harm in a user of the library using an initializer vs any of these class methods to construct an event however. Is there something I'm missing here?
  2. I agree that this could be provided in another way and potentially better way (especially once we implement date/time correction {insert Doctor Who meme}) but I think that should be done a little further down the road. For now the date string is supplied by the eventsManager via MMENSDateWrapper and all that needs to be called by the client/host is enqueueEventWithName:attributes:.

Copy link
Contributor

@julianrex julianrex Jan 3, 2019

Choose a reason for hiding this comment

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

Is there something I'm missing here?

No - more curious about future plans 😄

[...] but I think that should be done a little further down the road

Thanks for clarifying 👍

MMEEvent *event = [[MMEEvent alloc] init];
event.name = name;
NSMutableDictionary *commonAttributes = [NSMutableDictionary dictionary];
commonAttributes[MMEEventKeyEvent] = event.name;
commonAttributes[MMEEventKeyCreated] = dateString;
[commonAttributes addEntriesFromDictionary:attributes];
event.attributes = commonAttributes;
return event;
}

+ (NSInteger)contentSizeScale {
NSInteger result = -9999;

Expand Down
4 changes: 3 additions & 1 deletion MapboxMobileEvents/MMEEventsManager.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,8 +388,10 @@ - (void)createAndPushEventBasedOnName:(NSString *)name attributes:(NSDictionary
MMEEventKeyLocalDebugDescription: [NSString stringWithFormat:@"Pushing event: %@", event]}];
[self pushEvent:event];
} else {
event = [MMEEvent eventWithDateString:[self.dateWrapper formattedDateStringForDate:[self.dateWrapper date]] name:name attributes:attributes];
[self pushDebugEventWithAttributes:@{MMEDebugEventType: MMEDebugEventTypePush,
MMEEventKeyLocalDebugDescription: [NSString stringWithFormat:@"Unknown event: %@", event]}];
MMEEventKeyLocalDebugDescription: [NSString stringWithFormat:@"Pushing generic event: %@", event]}];
[self pushEvent:event];
}
}

Expand Down
8 changes: 4 additions & 4 deletions MapboxMobileEventsTests/MMEEventsManagerTests.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -896,13 +896,13 @@ @interface MMEEventsManager (Tests) <MMELocationManagerDelegate>
});
});

context(@"when an unknown event is pushed", ^{
context(@"when a generic event is pushed", ^{
beforeEach(^{
[eventsManager enqueueEventWithName:@"invalid" attributes:attributes];
[eventsManager enqueueEventWithName:@"generic" attributes:attributes];
});

it(@"does not queue the event", ^{
eventsManager.eventQueue.count should equal(0);
it(@"does queue the event", ^{
eventsManager.eventQueue.count should equal(1);
});
});
});
Expand Down