Skip to content
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

precision should honor 0 if it is passed in #1479

Closed
brianxieseattle opened this issue Aug 10, 2017 · 3 comments · Fixed by #1591
Closed

precision should honor 0 if it is passed in #1479

brianxieseattle opened this issue Aug 10, 2017 · 3 comments · Fixed by #1591
Assignees
Labels

Comments

@brianxieseattle
Copy link

Type of issue

Bug

Description

If I give a bucket with precision to 0 and call getPriceBucketString. getPriceBucketString will treat 0 to the default 2 precision.
'buckets': [
{
'precision': 0,
'min': 3,
'max': 18,
'increment': 0.05,
}

Steps to reproduce

let cpm = 16.50908;
let customConfig = {
  'buckets': [
  {
    'precision': 0,
    'min': 3,
    'max': 18,
    'increment': 0.05,
  }
  ]
};

let output = getPriceBucketString(cpm, customConfig);

Expected results

cpm.custom should be 17 as I set precision to 0

Actual results

cpm.custom is emit as 16.51.

Other information

Root cause seems to be around in cpmBucketManager
function getCpmTarget(cpm, increment, precision) {
if (!precision) {
precision = _defaultPrecision;
}

!precision will evaluate 0 as true and defaultPrecision is used.

Shall we consider
(!precision && precision != 0)

@mkendall07
Copy link
Member

Nice catch! We accept PRs if you want or we will fix next sprint. Thanks

@brianxieseattle
Copy link
Author

Great.. I seem to have some issues in pushing the changes remotely. You can feel free to include the fix in the next sprint.. BTW, when is the estimate time for the next sprint?

@brianxieseattle
Copy link
Author

BTW, here are the two unit tests you can consider adding to cpmBucketManager_spec.js when you fix this issue.

it('gets custom bucket strings and it should honor 0', () => {
let cpm = 16.50908;
let customConfig = {
'buckets': [
{
'precision': 0,
'min': 3,
'max': 18,
'increment': 0.05,
}
]
};
let expected = '{"low":"5.00","med":"16.50","high":"16.50","auto":"16.50","dense":"16.50","custom":"17"}';
let output = getPriceBucketString(cpm, customConfig);
expect(JSON.stringify(output)).to.deep.equal(expected);
});

it('gets the custom bucket strings without passing precision and it should honor the default precision', () => {
let cpm = 16.50908;
let customConfig = {
'buckets': [
{
'min': 3,
'max': 18,
'increment': 0.05,
}
]
};
let expected = '{"low":"5.00","med":"16.50","high":"16.50","auto":"16.50","dense":"16.50","custom":"16.50"}';
let output = getPriceBucketString(cpm, customConfig);
expect(JSON.stringify(output)).to.deep.equal(expected);
});

@ghost ghost assigned mkendall07 Sep 15, 2017
@ghost ghost added the in progress label Sep 15, 2017
@ghost ghost removed the in progress label Sep 15, 2017
outoftime pushed a commit to Genius/Prebid.js that referenced this issue Sep 18, 2017
…built

* 'master' of https://github.com/prebid/Prebid.js: (46 commits)
  Serverbid alias (prebid#1560)
  Add user sync to process for approving adapter PRs (prebid#1457)
  fix travis build (prebid#1595)
  Rubicon project improvement/user sync (prebid#1549)
  Adding Orbitsoft adapter (prebid#1378)
  Fix renderer test for new validation rule (prebid#1592)
  Allow SET_TARGETING to be used in AnalyticsAdapter (prebid#1577)
  Add support for video stream context (prebid#1483)
  Invalidate bid if matching bid request not found (prebid#1575)
  allow adapters to set default adserverTargeting for specific bid (prebid#1568)
  Custom granularity precision should honor 0 if it is passed in closes prebid#1479 (prebid#1591)
  BaseAdapter for the Prebid 0.x -> 1.x transition  (prebid#1494)
  Add a version to the Criteo adapter (prebid#1573)
  Allow bundling from node.js or with new gulp task bundle-to-stdout  (prebid#1570)
  Add url.parse option to not decode the whole URL (prebid#1480)
  Tremor Video Bid Adapter (prebid#1552)
  Yieldmo bid adapter (prebid#1415)
  Switch `gulp docs` to build its output using documentation.js (prebid#1545)
  Increment pre version
  Prebid 0.28.0 Release
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants