-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Core: New activity control - load external script #12207
Conversation
src/adloader.js
Outdated
@@ -50,6 +54,11 @@ const _approvedLoadExternalJSList = [ | |||
* @param {object} attributes an object of attributes to be added to the script with setAttribute by [key] and [value]; Only the attributes passed in the first request of a url will be added. | |||
*/ | |||
export function loadExternalScript(url, moduleCode, callback, doc, attributes) { | |||
if (!isActivityAllowed(LOAD_EXTERNAL_SCRIPT, activityParams(MODULE_TYPE_PREBID, 'adLoader'))) { |
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.
I think to be useful the module should be the one attempting to load the script (rather than prebid/adloader). Otherwise I can only disable all scripts or none - the activity parameters are always the same. It does require some refactoring to get the caller's module type.
src/adloader.js
Outdated
@@ -50,6 +54,11 @@ const _approvedLoadExternalJSList = [ | |||
* @param {object} attributes an object of attributes to be added to the script with setAttribute by [key] and [value]; Only the attributes passed in the first request of a url will be added. | |||
*/ | |||
export function loadExternalScript(url, moduleCode, callback, doc, attributes) { | |||
if (!isActivityAllowed(LOAD_EXTERNAL_SCRIPT, activityParams(MODULE_TYPE_PREBID, 'adLoader'))) { | |||
logError('cannot load external script as it\'s disabled by activity controls'); |
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.
isActivityAllowed
logs a warning when denied, so this is unnecessary IMO.
test/spec/adloader_spec.js
Outdated
const unregisterRule = registerActivityControl(LOAD_EXTERNAL_SCRIPT, 'loadExternalScript config', () => ({allow: false})); | ||
adLoader.loadExternalScript('someURL2', 'debugging'); | ||
expect(utilsLogErrorStub.called).to.be.true; | ||
unregisterRule(); |
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.
Test teardown is better done in a teardown hook (after
/afterEach
), or in a finally
block. This is fine if the test passes, but if at some point some code change makes it fail, the teardown would not run and can potentially affect other tests, which makes it more difficult to find the failure.
5a613d9
to
13418b0
Compare
13418b0
to
ba89a50
Compare
Hi, https://docs.prebid.org/dev-docs/activity-controls.html#activities |
Type of change
Bugfix
Feature
New bidder adapter
Updated bidder adapter
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Does this change affect user-facing APIs or examples documented on http://prebid.org?
Other
Description of change
Other information
#11010