Add Intercom Server Side #82
Conversation
x4d3
commented
Sep 15, 2016
x4d3
commented
- Separated event logger in 2 listener: Audit Events Listener and Intercom Events Listener
- Separated event logger in 2 listener: Audit Events Listener and Intercom Events Listener
There was a problem hiding this comment.
Thanks @x4d3 for the help on this.
A few required changes, I'll do them before merging.
See my comments and let me know if I'm mistaken.
It'd be great to have the specs before merging.
@hedudelgado FYI
| @@ -11,6 +11,8 @@ | |||
| gem 'shoulda-matchers' | |||
| gem 'fakeweb', '~> 1.3' | |||
| gem 'timecop' | |||
| gem 'timecop' | |||
| }}) | ||
| rescue Net::ReadTimeout | ||
| # Meant to fail | ||
| formated_metadata = format_metadata(metadata, object) |
| basic_auth MnoEnterprise.tenant_id, MnoEnterprise.tenant_key | ||
| @@listeners = [] | ||
| @@listeners << AuditEventsListener.new | ||
| @@listeners << IntercomEventsListener.new |
There was a problem hiding this comment.
Why not ?
@@listeners = [
AuditEventsListener.new,
IntercomEventsListener.new
]| @@ -27,7 +27,7 @@ Gem::Specification.new do |s| | |||
| s.add_runtime_dependency 'coffee-rails', '~> 4.1' | |||
| s.add_runtime_dependency 'health_check', '~> 1.5' | |||
| s.add_runtime_dependency 'httparty', '~> 0.13.7' | |||
|
|
|||
| s.add_runtime_dependency 'intercom', '~> 3.5.4' | |||
There was a problem hiding this comment.
We want an optional dependency on it (like sparkpost)
|
|
||
| def initialize | ||
| if MnoEnterprise.intercom_api_key && MnoEnterprise.intercom_api_secret | ||
| self.intercom = ::Intercom::Client.new(app_id: MnoEnterprise.intercom_api_key, api_key: MnoEnterprise.intercom_api_secret) |
There was a problem hiding this comment.
A bit of mixup between the app id and the api_key (probably because of way the tenant_id work?)
| when 'app_add' | ||
| event_name = 'added-app-' + object.app.nid | ||
| else | ||
| event_name = key.gsub!(/_/, '-') |
There was a problem hiding this comment.
I can show off a new trick I learned! I you don't use the regex capture groups in the replacement or if like here you're just doing a string substitution, #tr is order of magnitude faster than #gsub
See this benchmark:
Warming up --------------------------------------
String#gsub 49.395k i/100ms
String#tr 134.293k i/100ms
Calculating -------------------------------------
String#gsub 542.159k (±23.6%) i/s - 2.519M in 5.001670s
String#tr 2.346M (± 5.8%) i/s - 11.683M in 5.001330s
Comparison:
String#tr: 2345516.8 i/s
String#gsub: 542158.9 i/s - 4.33x slower
| def info(key, current_user_id, description, metadata, object) | ||
| return unless self.intercom | ||
| u = User.find(current_user_id) | ||
| begin |
There was a problem hiding this comment.
I don't think you need this begin, the rescue Intercom::IntercomError will apply to the whole method and it's easier to read.
|
@x4d3 see my changes. I think we still miss the finishedSignUp event and the AddApp event doesn't have the same payload. |
| read_timeout 0.1 | ||
| basic_auth MnoEnterprise.tenant_id, MnoEnterprise.tenant_key | ||
| @@listeners = [AuditEventsListener.new] | ||
| @@listeners << IntercomEventsListener.new if defined?(Intercom) |
|
@ouranos I've added specs. |
