-
Notifications
You must be signed in to change notification settings - Fork 264
Conversation
@@ -97,6 +107,10 @@ BrowserID.State = (function() { | |||
handleState("new_user", function(msg, info) { | |||
self.newUserEmail = info.email; | |||
|
|||
// Add new_account to the KPIs *before* the staging occurs allows us to | |||
// know when we are losing users due to the email verification. | |||
mediator.publish("kpi_data", { new_account: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Yes, we have to publish data pretty early so we don't have nay blind spots. Good call.
Just one question before r+... Otherwise, Code looks great. I didn't run locally. I can't find test output on Travis CI. |
@@ -95,7 +95,12 @@ | |||
timestamp: now, | |||
local_timestamp: now, | |||
lang: "bar", | |||
secret: "Attack at dawn!!!" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sigh.
afaict we need one more commit on this PR to address the "new var 'current' instead of 'controller.getCurrent()' here". I'll re-read, but I can't see any other major issues. I'll add a patch, test and merge in about 2 hrs if there's no response before then. |
with the last patch, r+ - checking tests and merging.. |
Getting a bunch of new KPIs ready
issue #1667
issue #1730