intercom / intercom-wordpress

Intercom integration with Wordpress
28 stars 13 forks source link

Add ability to filter intercomSettings, fixes #74 #90

Closed lkraav closed 7 years ago

lkraav commented 7 years ago

@Skaelv thoughts here, please?

kant01ne commented 7 years ago

Hey @lkraav

Is the purpose of this PR to be able to modify the IntercomSnippetSettings instantiation with custom values? Such as in this comment directly in the plugin in WordPress?

I'm not sure this is a long-term solution as this means you can't upgrade to new versions of the plugin if you need to manually modify the code in the plugin. Did I understand correctly?

lkraav commented 7 years ago

Is the purpose of this PR to be able to modify the IntercomSnippetSettings instantiation with custom values?

Yes. It is actually critical for any non-trivial use-case to be able to easily send additional data structures for Intercom boot. Upstream chosen defaults are clearly not a 100% fit for everyone.

Such as in this comment directly in the plugin in WordPress?

Implementation shown in that comment is relevant only for demonstrating the need. The correct, future-proof implementation is given in my PR here.

I'm not sure this is a long-term solution as this means you can't upgrade to new versions of the plugin if you need to manually modify the code in the plugin. Did I understand correctly?

No. Are you familiar with WordPress Plugin API hook system? See http://blog.teamtreehouse.com/hooks-wordpress-actions-filters-examples

This PR gives a simple flexible way for people to override the default data set in their theme or a custom micro-plugin. After merging this apply_filters() call there is no need to further modify Intercom plugin source code.

Instead, people can put something like this in their theme to get user_id added:

add_filter( 'intercom_settings', function( $settings ) {

    if ( $user_id = get_current_user_id() ) {           
        $settings['user_id'] = $user_id;                
    }                                                   

    return $settings;                                   

} );
kant01ne commented 7 years ago

That makes sense. Nice one @lkraav! Thanks for contributing :)

lkraav commented 7 years ago

@Skaelv tyvm sir

lkraav commented 7 years ago

@Skaelv I think this needs another iteration. Apparently Identity Verification requires a specific order of having user_id and email to work with and our filter here is too late for that. Filing a new PR later today.

allaire commented 7 years ago

@lkraav About your last comment, is this causing a bug somehow? It worked great for my use case (hide_default_launcher).

lkraav commented 7 years ago

About your last comment, is this causing a bug somehow? It worked great for my use case (hide_default_launcher).

@allaire this is only specific to the order of user_id vs email appearing in the array.

EDIT 2018 yup, it's been sitting safely on the TODO list for a year now.

neilgilmour commented 5 years ago

Filing a new PR later today.

I didn't see another PR from @lkraav, and ran into the user_id vs email problem myself.

If you pass user_id using the filter on intercom-settings the hash is still performed on the email address, but because Intercom receives a user_id it automatically uses this as the identifier for the user and assumes that it will be the hashed value. Which it isn't.

I'm not a developer, so this could be the wrong way to do it, but I changed the buildSettings() function and now the ID gets hashed instead of the email.

https://github.com/motional-io/intercom-wordpress/commit/b588471dbc948c4a477ac98f43264c1abfb93048