Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Harmonize publisher visits #3852

Merged
merged 2 commits into from
Sep 9, 2016
Merged

Harmonize publisher visits #3852

merged 2 commits into from
Sep 9, 2016

Conversation

mrose17
Copy link
Member

@mrose17 mrose17 commented Sep 9, 2016

Fixes #3830

test plan:

open two windows. in one window, put the payments pane; in the other two tabs on the same publisher but different URLs. observer the visits and duration counts in the first window and then switch back and forth between tabs (waiting one or two minutes between each switch). the visits should stay constant and the duration should increase.

repeats of a location/tabId pair should result in a `revisitP=true`
call to `synopsis.addPublisher` so the `visits` counter is not
incremented.

auditor: @diracdeltas
@mrose17 mrose17 added this to the 0.12.1dev milestone Sep 9, 2016
@mrose17 mrose17 self-assigned this Sep 9, 2016
@@ -470,7 +470,8 @@ var updatePublisherInfo = () => {
var entries = []

underscore.keys(publishers[publisher]).forEach((location) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can just do underscore.each(publishers, ...)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in this case, no... because in the function i need access to both the key and value, not just the value...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both key and value are accessible http://underscorejs.org/#each

@diracdeltas
Copy link
Member

please include a test plan in the commit message
@bridiver should also audit this

@mrose17
Copy link
Member Author

mrose17 commented Sep 9, 2016

i will add a test plan. i had a brief conversation with @bridiver about this...

@bridiver
Copy link
Collaborator

bridiver commented Sep 9, 2016

the eventStore changes look good to me. Discussed the definition of a visit with @mrose17 and that sounds ok too. @diracdeltas I think you are in a better position than I am to review the ledger code.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants