Conversation
|
The latest push to PR-114 is running on SAN-4053-intercom-engagement-navi |
| user_id: 'navi-' + req.naviEntry.ownerUsername, | ||
| update_last_request_at: true, | ||
| companies: [{ | ||
| company_id: req.naviEntry.ownerUsername, |
There was a problem hiding this comment.
Should we use owner githubId or something?
There was a problem hiding this comment.
company_id is intercom syntax. We already have it as ownerUsername everywhere.
|
@Myztiq - See comment about defensive programming. Other than that, do we want to add a test to ensure we are doing the reporting? Basically I'd feel better if we were testing that if statement above. 🍻 |
| @@ -14,6 +14,8 @@ var pluck = require('101/pluck'); | |||
| var put = require('101/put'); | |||
| var url = require('url'); | |||
|
|
|||
There was a problem hiding this comment.
nit why the space?
|
|
||
| // This runs async, but we don't care about the results. FIRE + FORGET! | ||
| var ownerUsername = keypather.get(req, 'naviEntry.ownerUsername') | ||
| if (ownerUsername && self.intercomClient) { |
There was a problem hiding this comment.
Good catch on the && self.intercomClient. Have you considered just pulling this functionality out into its own method? For instance,
// Returns true if we should report via intercom
Proxy.prototype.shouldUseIntercom = function () {
return process.env.INTERCOM_APP_ID && process.env.INTERCOM_API_KEY
}
// Reports user information to intercom
Proxy.prototype.reportToIntercom = function (req) {
if (!this.shouldUseIntercom()) { return }
}If we suspect there are other places where we would want this specific behavior I suggest pulling the whole intercom client out into its own module. That way you can just require the internal intercom module and don't have to repeat defensive checks.
There was a problem hiding this comment.
Yea, I could see extracting this out if we end up needing to use it in another spot. I don't feel like we should do it now for the sake of "engineering" a better system. I can't think of a spot off the top of my head where we'll be reporting from navi anything to intercom other than this right now.
|
this LGTM |
5 participants
Added logic to create a navi user for intercom to be able to know when the last request was made on a company.