feedhenry / fh-ios-sdk

FeedHenry iOS (Objective-C) SDK
http://feedhenry.org
Apache License 2.0
6 stars 16 forks source link

Add AeroGear Push dependency into cocoapods #32

Closed corinnekrych closed 9 years ago

corinnekrych commented 9 years ago

This PR is related to FH-15 "iOS SDK enablement for UPS integration"

cvasilak commented 9 years ago

have an an error during build of the demo whereas the fhconfig.plist is not found, guess it was missed to be commited in the PR.

matzew commented 9 years ago

Long term, yes we might want a shim/wrapper around the AG lib.

For now, I think really it's good enough if the app, that uses push, has a AGPush as the dependency (e.g. the generated project from FH studio). see FH-130 for similar approach

Note, adding AeroGear-Push to the project, also requires the build farm to be able to compile an app, that uses AeroGear-Push.

matzew commented 9 years ago

@wei-lee mind to take a look at my comments ?

matzew commented 9 years ago

@jasonmadigan mind wanna take a look here too ?

corinnekrych commented 9 years ago

@cvasilak I've just added fhconfig.plist into FHPushDemo. Not sure why it was ignore the first time...

In short: this is a first step, eventually we'll deeper integration for configuration and for API. To be followed. This PR is required for the template app and for testing in BF.

cvasilak commented 9 years ago

@corinnekrych I believe and as described in my previous comment the merits of it, the proper way is to:

a) if we are using cocoapods, the user should add the "FH" and "AeroGearPush" pods b) if we are not using cocoapods, the user should add FH.framework and AeroGearPush.framework binary artefacts.

Later, and as you said when we have deeper integration, I can see the s.dependency being included in the library itself. For now I believe this PR should be closed.

@matzew is that what you are suggesting too from your previous comment?

wei-lee commented 9 years ago

@corinnekrych @matzew @cvasilak I think I agree with Christos. I just can't see the benefit of having the AG SDK as a dependency of FH SDK at the moment. We should do this when we decide to deeply integrate the FH SDK with the AG SDK.

In terms of integrate with the AG push SDK, I think the better way to do it is in the build farm - during the build time, the AG push SDK will be added to the project and initialised automatically when push notification is enabled in the studio (like how UA is used in the studio at the moment).

matzew commented 9 years ago

In terms of integrate with the AG push SDK, I think the better way to do it is in the build farm - during the build time, the AG push SDK will be added to the project and initialised automatically when push notification is enabled in the studio

@wei-lee @corinnekrych @cvasilak One more thought regarding the deeper integration, on a different thread @johnfriz mentioned that he would like to avoid an extra HTTP request for device registration, which sounds reasonable.

He was suggesting to add the registration to the existing "init" call that a device does on startup (currently used for analytics tracking and for the device to get the hostname of it's cloud app)

I actually like that idea and this means perhaps that we don't need to include the AG SDK for the registration, since it basically "just" sends the deviceToken to the UPS server, which would be added to the current "init". However perhaps a bit of code re-usage from AG SDK would be possible

corinnekrych commented 9 years ago

Let's carry on the discussion on related JIRAs. Closing it.

corinnekrych commented 9 years ago

This PR will be completed. Not ready to merge yet.

corinnekrych commented 9 years ago

@matzew @wei-lee Here is an updated version with the wrapper.

wdyt?

corinnekrych commented 9 years ago

@matzew I'll rework it to add analytics + api doc. I agree with you Pods folder add a lot of noise. What about removing it from git? We keep Podfile.lock.

matzew commented 9 years ago

+1 on removing, if ok w/ @wei-lee

corinnekrych commented 9 years ago

@matzew if we want to do all the call within the FH.init we needeither

I'd rather go for an API to store the deviceToken I see it less intrusive... but open to discussion.

wei-lee commented 9 years ago

Reviewed. This function here, given that the same code will be used by other developers to register for push notification, should we consider wrap it as an API call?

Yes, I am ok with removing the Pods folder. Don't really need it here.

wei-lee commented 9 years ago

@corinnekrych @matzew About sending device token in the init call, did you mean we send it in the init request if we have the device token at the time when init is invoked, and if we don't and the device is registered later for push notification, we will invoke the init call again to send the device token?

corinnekrych commented 9 years ago

indeed @wei-lee you may want to register to push notification later in life cycle (to add cetegories etc...). if it's wrapped within FH.init it gives less flexibility.

johnfriz commented 9 years ago

I think we agreed to keep it totally separate from the init call.

corinnekrych commented 9 years ago

Oops sorry @johnfriz I agree with that, it's me reading an old comment :( Agreed on separate call, I will:

corinnekrych commented 9 years ago

@wei-lee I've added pushEnabledForRemoteNotification: to contain ios7/ios8 logic https://github.com/corinnekrych/fh-ios-sdk/blob/push.demo/demos/FHPushDemo/FHPushDemo/AppDelegate.m#L13

@matzew metrics have been added and are demoed in embedded demo see: https://github.com/corinnekrych/fh-ios-sdk/blob/push.demo/demos/FHPushDemo/FHPushDemo/AppDelegate.m#L14 https://github.com/corinnekrych/fh-ios-sdk/blob/push.demo/demos/FHPushDemo/FHPushDemo/AppDelegate.m#L70

matzew commented 9 years ago

:+1:

corinnekrych commented 9 years ago

@danielpassos @edewit as discussed we need to add the ability to set categories. wdyt of :

pushRegister:withAlias:withCategories:andSuccess:andFailure:

setPushCategories:andSuccess:andFailure:

setPushAlias:andSuccess:andFailure:

edewit commented 9 years ago

@corinnekrych added that to windows: https://github.com/aerogear/aerogear-windows-push/blob/update-config/aerogear-windows-push/Registration.cs#L67-L99

danielpassos commented 9 years ago

@corinnekrych It means a new request to set a categories and a new one to set alias?

corinnekrych commented 9 years ago

@danielpassos It's open to discussion. Do we use categories and alias at the same time?

danielpassos commented 9 years ago

@corinnekrych I was thinking about create a config class and pass it (as optional) to registration. It will make our wrapper more flexible, without need to create new methods if we add more options in UPS, and send only one request for both ;)

See my commit https://github.com/danielpassos/fh-android-sdk/commit/207afac082ecae998014ad5b128502581924f8d4

Wdyt?

corinnekrych commented 9 years ago

@danielpassos Agreed. I've added PushConfig class as per Windows and android sdk. AeroGear iOS push has been released to latest. All ready to go. @wei-lee ok to merge?

wei-lee commented 9 years ago

@corinnekrych Yes, absolutely. Feel free to merge.