Skip to content

Conversation

@davidtruong
Copy link
Contributor

Base Android SDK ready for code review.

Key Classes to look at:
IterableApi
IterableConstants
IterableHelper
IterableNotification
IterablePushOpenReceiver
IterablePushReceiver
IterablePushRegistrationGCM
IterableRequest

…context. Added in new map of additionalParams to track.
…name and added a default constructor with now optional dataFields.
…y handling the null check for a ghost push silently rather than explicitly having the app null check the NotifCompat.builder.
Abstracted out GCM receiver into its own class
… which handles both push gcm registration and push notification received intent calls.
…n an Activity.

Updated notification builder to pull the main activity from the application context.
Bundle extras = intent.getExtras();
if (extras != null && !extras.isEmpty() && extras.containsKey("itbl"))
{
String iterableData = extras.getString("itbl");
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a string? Not an object/dictionary already?

@dontgitit
Copy link
Contributor

Added a bunch of comments. Also, looks like you have random comments/notes/TODOs throughout, see if you can clean those up! lgtm so far

{
sharedInstance = new IterableApi(context, apiKey, email);
Intent calledIntent = ((Activity) context).getIntent();
sharedInstance.trackAppOpen(calledIntent);
Copy link

Choose a reason for hiding this comment

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

is this function always guaranteed to be called as a result of opening the app? if not, and only by convention, then trackAppOpen could lead to overcounting perhaps?

also, it would be an interesting notion to explore bulking networking calls together

Copy link
Contributor

Choose a reason for hiding this comment

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

also, it would be an interesting notion to explore bulking networking calls together probably not worth it

Copy link

Choose a reason for hiding this comment

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

well, i know that's what Parse did, kinda?

Anyway, does the Iterable SDK do a best effort save (e.g. if a network call times out, it times out, oh well?), or does it do a save eventually? if the latter, this would lend to bulking networking calls... but i guess that also means changing the server APIs. so maybe not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sharedInstanceWithApiKey is usually called in the onCreate method of the main application. That being said, it can also be called at other times within an app. The trackAppOpen call is actually more of a tryTrackAppOpen call since it takes the calledIntent and determines if it was opened from a notification before calling the server endpoint to track a pushOpen.

I'll change the function name (to tryTrackOpen) to reflect that it is an attempt to track a pushOpen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One thing we might need to account for down the road is a queued retry mechanism.

Moved keys to the constants class
Changed async call to use a java object instead of an arbitrary list of params.
* @param campaignId
* @param templateId
*/
public void trackConversion(int campaignId, int templateId) {
Copy link

Choose a reason for hiding this comment

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

we removed the conversion data type, so we can get rid of the conversion calls

Temporarily making api calls private so they wont be used yet.
Changed sharedInstance to be a singleton which can update its data.
Adds in error handling for Server requests. Added in timeouts
Included retry mechanism for failed http requests.

Added in a debug mode to route requests to a local server

Added Disable Device call to the SDK

Abstracted the bundle from intent when creating a new notification.

Generated a unique id from notifications so they do not overwrite the old notification.
@davidtruong davidtruong merged commit 5b5721b into master Jun 20, 2016
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.

6 participants