intercom-java icon indicating copy to clipboard operation
intercom-java copied to clipboard

Make Intercom Credentials configurable on a Per - Object Basis

Open prismec opened this issue 9 years ago • 8 comments

Due to the fact that static methods are used in order to list, create, update etc... entities, we are forced to either talk to 1 Intercom app per Java application or use Java synchronization in order to switch credentials on a per-method call (which is awkward since remote method calls take time to finish).

In order to send events to multiple Intercom apps within a single Java application, it would be helpful to extend the API in order to take credentials on a per-operation level, e.g.

def app = new IntercomApp(apiKey: "...", appId: "...") User.list(app) User.create(app, user)

prismec avatar Dec 21 '15 10:12 prismec

@prismec thanks for raising this. This looks like a good idea at first glance; will want to think through the API signatures a bit - guessing based on the example, your preference is to supply an app context object?

dehora avatar Dec 21 '15 11:12 dehora

Also, using static methods is generally bad practice. It makes things like this ^ and testing very hard.

From an API point of view it would be easier to use if you could create instances, here is an idea:

IntercomSession intercom = new IntercomSession(apiKey, apiId,...);
UserResource user = intercom.userResource();
user.list()
user.create(new User());

With this approach consumers of your library could:

  • Pick which intercom app to talk to
  • Mock the connection to intercom making testing easier

martina-if avatar Jun 28 '16 08:06 martina-if

The example uses the session (IntercomSession) as a factory for further resource accessors (e.g. UserResource). This is a common pattern found in many APIs. From a testing point of view it would be good if the factory, as well as all resources are encapsulated using interfaces, so that it’s much easier to mock it for testing purposes.


Smarter Ecommerce GmbH

Spittelwiese 15 4020 Linz

T +43 732 997002-100 [email protected] http://www.smarter-ecommerce.com http://www.smarter-ecommerce.com/

UID: ATU 63760825

smec – we automate ecommerce success

On 21 Dec 2015, at 12:22 , Bill de hÓra [email protected] wrote:

@prismec https://github.com/prismec thanks for raising this. This looks like a good idea at first glance; will want to think through the API signatures a bit - guessing based on the example, your preference to supply an app context object?

— Reply to this email directly or view it on GitHub https://github.com/intercom/intercom-java/issues/89#issuecomment-166275797.

prismec avatar Jul 04 '16 07:07 prismec

Hi @prismec, We were just reviewing some older issue and wanted to check in on this one. Unfortunately we have not been able to make any specific changes with regard to this issue. So as far as I can see I think this is still a valid issue to remain open. I just wanted to check in with you to see if you had any other thoughts on it in the meantime or were there any changes that might have been related to this issue? Like I said, it sounds like it is still valid to leave open as an issue but wanted to confirm with you.

Thanks Cathal

choran avatar Aug 24 '17 10:08 choran

This still seems like a good feature to have to me, it's something I'd find very useful.

danielcompton avatar Oct 04 '17 23:10 danielcompton

The current very static-based api makes it nearly impossible to use this SDK with multiple app-ids, requiring a separate java application for each api key. This makes it very awkward to use, so please consider giving a builder-based api.

Something like:

Intercom app1 = Intercom.create()
        .withToken(API_TOKEN_APP1)
        .withAppId(appid1)
        //... whatever more
        .build()

Intercom app2 = Intercom.create()
        .withToken(API_TOKEN_APP2)
        .withAppId(appid2)
        //... whatever more
        .build()

or maybe require the non-optional settings directly as paramters to the create or build method.

esiqveland avatar Nov 10 '17 10:11 esiqveland

I also had the problem of using this lib for multiple appIDs and tokens (on behalf of OAuth'd customers). I made a version of the library which eliminates the static config, and passes the config object to every service method that requires it.

My fork is here: https://github.com/xgp/intercom-java

You can use it like this example:

Intercom intercom = new Intercom();
intercom.setAppID("put_app_id_here");
intercom.setToken("put_token_here");

AdminCollection admins = Admin.list(intercom);
while(admins.hasNext()) {
    Admin admin = admins.next();
    System.err.println("Admin: "+admin.toString());
}

Note, this is not a drop-in replacement for the official library from Intercom, as all of the service methods require the Intercom config object.

xgp avatar Jun 09 '18 12:06 xgp

Thanks so much for sharing @xgp 👏 Definitely a good starting point but as you mentioned it isn't a drop-in replacement due to a breaking change in the code

I've tagged this issue as "help wanted" as I don't think I'd have the resources to look into this right now but definitely open to ideas and PRs to see how we can get this moving forward 👍

thewheat avatar Jun 11 '18 01:06 thewheat