{"action":"closed","issue":{"url":"https://api.github.com/repos/mozilla/fxa-content-server/issues/4396","repository_url":"https://api.github.com/repos/mozilla/fxa-content-server","labels_url":"https://api.github.com/repos/mozilla/fxa-content-server/issues/4396/labels{/name}","comments_url":"https://api.github.com/repos/mozilla/fxa-content-server/issues/4396/comments","events_url":"https://api.github.com/repos/mozilla/fxa-content-server/issues/4396/events","html_url":"https://github.com/mozilla/fxa-content-server/issues/4396","id":188760778,"number":4396,"title":"#4383 was prematurely merged","user":{"login":"philbooth","id":64367,"avatar_url":"https://avatars.githubusercontent.com/u/64367?v=3","gravatar_id":"","url":"https://api.github.com/users/philbooth","html_url":"https://github.com/philbooth","followers_url":"https://api.github.com/users/philbooth/followers","following_url":"https://api.github.com/users/philbooth/following{/other_user}","gists_url":"https://api.github.com/users/philbooth/gists{/gist_id}","starred_url":"https://api.github.com/users/philbooth/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/philbooth/subscriptions","organizations_url":"https://api.github.com/users/philbooth/orgs","repos_url":"https://api.github.com/users/philbooth/repos","events_url":"https://api.github.com/users/philbooth/events{/privacy}","received_events_url":"https://api.github.com/users/philbooth/received_events","type":"User","site_admin":false},"labels":[{"id":235579159,"url":"https://api.github.com/repos/mozilla/fxa-content-server/labels/waffle:review","name":"waffle:review","color":"ededed","default":false}],"state":"closed","locked":false,"assignee":{"login":"philbooth","id":64367,"avatar_url":"https://avatars.githubusercontent.com/u/64367?v=3","gravatar_id":"","url":"https://api.github.com/users/philbooth","html_url":"https://github.com/philbooth","followers_url":"https://api.github.com/users/philbooth/followers","following_url":"https://api.github.com/users/philbooth/following{/other_user}","gists_url":"https://api.github.com/users/philbooth/gists{/gist_id}","starred_url":"https://api.github.com/users/philbooth/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/philbooth/subscriptions","organizations_url":"https://api.github.com/users/philbooth/orgs","repos_url":"https://api.github.com/users/philbooth/repos","events_url":"https://api.github.com/users/philbooth/events{/privacy}","received_events_url":"https://api.github.com/users/philbooth/received_events","type":"User","site_admin":false},"assignees":[{"login":"philbooth","id":64367,"avatar_url":"https://avatars.githubusercontent.com/u/64367?v=3","gravatar_id":"","url":"https://api.github.com/users/philbooth","html_url":"https://github.com/philbooth","followers_url":"https://api.github.com/users/philbooth/followers","following_url":"https://api.github.com/users/philbooth/following{/other_user}","gists_url":"https://api.github.com/users/philbooth/gists{/gist_id}","starred_url":"https://api.github.com/users/philbooth/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/philbooth/subscriptions","organizations_url":"https://api.github.com/users/philbooth/orgs","repos_url":"https://api.github.com/users/philbooth/repos","events_url":"https://api.github.com/users/philbooth/events{/privacy}","received_events_url":"https://api.github.com/users/philbooth/received_events","type":"User","site_admin":false}],"milestone":null,"comments":2,"created_at":"2016-11-11T13:32:06Z","updated_at":"2016-11-18T10:46:03Z","closed_at":"2016-11-18T10:46:03Z","body":"I green-buttoned it before @shane-tomlinson was able to file his comments. From IRC:\r\n\r\n```\r\n13:25:10 <@stomlinson> I was just commenting on your PR pb, I had changes requested!\r\n13:25:38 <pb> stomlinson: oh, sorry man, i didn't realise\r\n13:25:50 <pb> stomlinson: make them anyway, i'll file a follow-up pr\r\n13:26:33 <pb> stomlinson: or i can back it out and re-submit?\r\n13:27:10 <@stomlinson> pb: it won't let me submit now\r\n13:27:20 <pb> stomlinson: no worries, i'll revert\r\n13:27:33 <@stomlinson> pb: nah, I'll copy the line and comment here\r\n13:27:35 <@stomlinson> https://github.com/mozilla/fxa-content-server/pull/4383/files#diff-a0470533ce10924d396c0e7774ca1afeR21\r\n13:27:45 <@stomlinson> VALID_METRICS_PROPERTIES are only checked for flow events, can/should the name indicate that somehow?\r\n13:27:54 <@stomlinson> https://github.com/mozilla/fxa-content-server/pull/4383/files#diff-a0470533ce10924d396c0e7774ca1afeR76\r\n13:28:03 <@stomlinson> With so much logic related to flow events in this module, I wonder if we'd be better served extracting the logic into a new collector or module in lib. Not a blocker for this PR, but something to keep in mind.\r\n13:28:14 <@stomlinson> https://github.com/mozilla/fxa-content-server/pull/4383/files#diff-a0470533ce10924d396c0e7774ca1afeR140\r\n13:28:24 <@stomlinson> property here is actually a property value, the name confused me a bit. If the property value is falsey, then isValidProperty will always return true? That seems like all falsey values will be let through, even if invalid. Should if (property) { be if (typeof property !== 'undefined') {?\r\n13:28:41 <@stomlinson> https://github.com/mozilla/fxa-content-server/pull/4383/files#diff-ec516925eac7c185ffa5d54e296cf6f0R171\r\n13:28:48 <@stomlinson> The next 41 lines are tedious to read, would it be easier to do an assert.deepEqual on an object that contains the expected output? @vladikoff used fixtures in the statsd-collector tests and I find that a bit easier to comprehend.\r\n13:31:28 <@stomlinson> https://github.com/mozilla/fxa-content-server/pull/4383/files#diff-ec516925eac7c185ffa5d54e296cf6f0R388\r\n13:31:50 <@stomlinson> I think @vbudhram mentioned it in an earlier review, it seems like these tests largely duplicate setup except for one metrics value changing. Can all of the data be extracted into setupMetricsHandlerTests, by default known good values are used, and you pass in overrides to the function?\r\n13:32:28 <@stomlinson> pb: The most important is the third\r\n```\r\n"}}
{"actor":{"display_login":"philbooth"}}