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

Problem with Adsense reload and DFP cmd push #9

Merged
merged 5 commits into from Sep 7, 2015

Conversation

2 participants
@ladydanger
Copy link
Contributor

commented Sep 4, 2015

Hi Coaches,

Recently, we refactored the Adsense reload code because DFP ads did not load in the presence of an Adsense Ads. See 50ee40c

This worked because the original if(key.indexOf("google") !== -1) was undefining everything starting with google, even the googletag which is part of DFP.

However, it brought 2 issues that were discovered recently.

1) DFP Ad not showing on page change

The google cmd is part of the DFP code and the error is that it is not defined. It runs initially on first reload, but when navigating to a post and then back, it stops pushing the ad and the command becomes undefined. We think is is because it's conflicting with the adsense reload in: assets/javascripts/discourse/components/google-adsense.js.es6 because this error doesn't occur when there are nil adsense ads. See image below.

This is the DFP ad not loading - you can see the adsense ad there though - next picture shows what the dfp ad (orange) and the adsense ad together

screen shot 2015-09-04 at 10 48 30 am

This is the DFP and Adsense Ad

screen shot 2015-09-04 at 10 47 18 am

2) Adsense Ad not showing

A 400 Error appears after 3-5 minutes of navigating to posts and back (without refreshing the page). We think this is a reloading error. It only happened after 50ee40c was put in. The error appears regardless of whether DFP ads are present or not.

Adsense ad not showing - 400 error

screen shot 2015-09-04 at 10 50 25 am

To replicate this in your local machine, download the latest version of this repo and checkout to the development branch (remember to pull the latest version of Discourse-main) and put the following settings in:

DFP Plugin Inputs
screen shot 2015-09-04 at 10 47 33 am

Adsense Plugin Inputs
screen shot 2015-09-04 at 10 47 42 am

Your help would be greatly appreciated!

Thanks in advance!

@cyberkoi

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2015

Issue #1 seems to be fixed after we added in this line here && key.indexOf("googletag") == -1 -> here

@ladydanger

This comment has been minimized.

Copy link
Contributor Author

commented on assets/javascripts/discourse/components/google-adsense.js.es6 in 7062254 Sep 7, 2015

key.indexOf('google') !== -1 means that any element in the window starting will google will be not false, and therefore true. key.indexOf('googletag') === -1 means that any element starting within googletag will be false. Therefore, the keys that will be undefined (see next line) and therefore reloaded in the following lines will be those that start with google but not googletag. This means that DFP can load and Adsense will reload.

There is a problem with this in that everything starting with google will reload and while it is a good patch for now, it may present issues if google changes its element names, another google ad platform is developed that uses etc.... (for noting)

@ladydanger

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2015

This issue has now been fixed - see commit 7062254 (7062254).

The problem was that the post Google and pre Google vars were loading elements starting with google for both dfp and adsense at the same time as opposed to loading the adsense elements first and undefining them. This was also messing up the reload.

Solution was to default back to original working reload code and add key.indexOf('googletag') === -1) - see detailed comment on this here.

Thank you again to Myles for his help!!!!!

@ladydanger ladydanger merged commit bdc8a2d into master Sep 7, 2015

@ladydanger ladydanger deleted the development branch Sep 7, 2015

@cyberkoi

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2015

cool, that makes sense. (read).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.