-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Event stats #192
Event stats #192
Conversation
4f6c3ca
to
e804f1a
Compare
const updateChannelPrice = async ev => { | ||
const price = ev.type === 'UPDATE_IMPRESSION_PRICE' ? ev.price : '0' | ||
await channelsCol.updateOne({ id: channelId }, { $set: { currentPricePerImpression: price } }) | ||
if (ev.type === eventTypes.IMPRESSION_PRICE_PER_CASE) { |
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.
later improvement: consider merging IMPRESSION_PRICE_PER_CASE
and UPDATE_IMPRESSION_PRICE
into one event (considering that this is still not frozen in spec)
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.
Ohh okay, which event name should it be UPDATE_IMPRESSION_PRICE
?
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.
Yes, but start with a PR to the documentation with the proposed interface
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.
see this: #203
// for the event stat | ||
// it can be publisher prefixed for a specific publisher | ||
const result = channel.pricePerImpressionCase.find( | ||
priceCase => priceCase.stat === ev.stat || priceCase.stat === `${ev.publisher}:${ev.stat}` |
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.
this allows setting the price per-publisher, right?
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.
Yes
Fixes #190 and #191