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

calling for currency file even when default rates are specified #3470

Merged
merged 3 commits into from
Jan 22, 2019

Conversation

bretg
Copy link
Collaborator

@bretg bretg commented Jan 22, 2019

Type of change

  • Bugfix

Description of change

We introduced a bug with the last release of currency module -- when the defaultRates parameter is specified, no call is made to obtain the latest rates.

With this config:

      pbjs.setConfig({
        "currency": {
                "adServerCurrency": "UNK",
                "defaultRates": { "USD": { "UNK": 50 }}
            }
      });

Expected results are that a call should go out to jsdelivr to pull the currency file.

Actual results: it doesn't currently with 1.37+.

Copy link
Contributor

@idettman idettman left a comment

Choose a reason for hiding this comment

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

LGTM (verified with added unit test checks that currency file is requested even when default rates are set)

@idettman idettman added the needs 2nd review Core module updates require two approvals from the core team label Jan 22, 2019
@bretg bretg requested a review from mkendall07 January 22, 2019 20:19
@bretg bretg assigned mkendall07 and unassigned idettman Jan 22, 2019
@@ -11,6 +11,7 @@ const CURRENCY_RATE_PRECISION = 4;
var bidResponseQueue = [];
var conversionCache = {};
var currencyRatesLoaded = false;
var needToCallForCurrencyFile = true;
Copy link
Member

Choose a reason for hiding this comment

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

global state is a bad practice and should be refactored but I won't block the PR as it's already all over this module...

@mkendall07 mkendall07 merged commit f26003d into master Jan 22, 2019
amuraco pushed a commit to amuraco/Prebid.js that referenced this pull request Jan 28, 2019
…id#3470)

* calling for currency file even when default rates are specified

* fixed broken unit tests

* added unit test to check that currency file is still requested even when default rates are set
pedrolopezmrf pushed a commit to Marfeel/Prebid.js that referenced this pull request Mar 18, 2019
…id#3470)

* calling for currency file even when default rates are specified

* fixed broken unit tests

* added unit test to check that currency file is still requested even when default rates are set
jacekburys-quantcast pushed a commit to jacekburys-quantcast/Prebid.js that referenced this pull request May 15, 2019
…id#3470)

* calling for currency file even when default rates are specified

* fixed broken unit tests

* added unit test to check that currency file is still requested even when default rates are set
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs 2nd review Core module updates require two approvals from the core team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants