FreeUKGen / Systemwide

repository for issues that affect all systems within the Free UK Gen portfolio
0 stars 0 forks source link

All adverts appearing incorrectly sized #59

Closed richpomfret closed 5 years ago

richpomfret commented 5 years ago

Across each project, including test sites. May well be 'bad' settings in Adsense made by external advert team. They are apparently 'responsive', but I wonder if our code actually supports that or if we previously had fixed resolution settings?

Captainkirkdawson commented 5 years ago

I note that the advert time added a large number of ads on Aug 27; These changes are independent of the changes that I have been asked to make to reg itself ad-units.JPG

Captainkirkdawson commented 5 years ago

From what I see none are responsive except the one we added a year ago on August 15 2018

richpomfret commented 5 years ago

Yes, I spotted the same info re: advert change on the 27th. It looks like the configuration is incorrect. I've asked them to revert to previous adverts until the issue has been resolved - it should all have been tested on 'test' before going into production..

Captainkirkdawson commented 5 years ago

But as far as I understand it these are purely code snippets that one places in the body of our html to make use of them. Hence this is simply a repository of those snippets. The addition and deletion of snippets should be irrelevant. We have code snippets in our app that is unchanged in some months. See the code base at https://github.com/FreeUKGen/MyopicVicar/blob/master/app/helpers/application_helper.rb#L428-L584 That code is somewhat cumbersome and difficult to follow but their changes should not have impacted it UNLESS they ALSO changed the data-ad-client or data-ad-slot that we use

richpomfret commented 5 years ago

I see what you mean - any new adverts would need to be called in the page code - although that might explain why the ads are not appearing on BMD (the code hasn't been implemented on our side), but ads (new?) ARE appearing on FR/FC and no additional code has been added there. Some kind of config change must have occured, as there is now a huge space (for a large leaderboard advert I think) at the top of the BMD page and we haven't altered the page code at all, so this must be due to an advert setting...hopefully reverting settings on their side will fix it.

Captainkirkdawson commented 5 years ago

@richpomfret From what I see the data_ad_clients are OK BUT I am deeply concerned that the data_ad_slots we use have been removed. I cannot find the current freereg one in the list. 8870759291 Cen uses "7868124617" "2003577939" "9056426667" "9391036375" I cannot find my access to google adsense for CEN so cannot confirm

Nor do I know what FreeBMD1 uses

richpomfret commented 5 years ago

Still looking into data_ad_slots.

BMD uses 3099203613 for the homepage leaderboard and I can see 'responsive' in the html page code, which may explain the size change.

I've also found the current FR one you mention as an existing ad unit. I don't have access to FC either..

Captainkirkdawson commented 5 years ago

You found the reg one? which snippet?

richpomfret commented 5 years ago

Found it within the Adsense search - 'Responsive Top FreeREG' ad has that ID.

richpomfret commented 5 years ago

It seems to be connected just fine to the home/search page code (see line 152): view-source:https://www.freereg.org.uk/

Although I do wonder about our sizing restrictions code and whether there is a clash between the advert settings in Adsense and our restrictions.

PatReynolds commented 5 years ago

FreeREG responsive top) is data-ad-slot="8870759291"

NB, while I have made adverts for FreeBMD which are responsive, and have put the word 'responsive' in their name, this does not mean they have been implemented as responsive.

PatReynolds commented 5 years ago

Data-ad-slot="8870759291"

Captainkirkdawson commented 5 years ago

Thanks guys I had failed to notice that there was a second page and it is our old one. There is no way that FreeBMD1 had changed their code. Their update only completed today. Wrt to your email @richpomfret there is nothing wrong with the ads.txt file in production. The code base uses 1 or the other depending on the app that is running. HOWEVER I DO NOT BELIEVE WE USE THIS FILE. @vino can confirm. The apps select their ads depending on the settings of the advert_key in https://github.com/FreeUKGen/MyopicVicar/blob/master/config/application.rb#L57-L60 Hence the recent changes made to ads.txt are likely irrelevant as they are unused by the app

Captainkirkdawson commented 5 years ago

Summary. Our applications are using unchanged calls to their original adsense code snippets. Our apps have not changed their dimensions which for reg and cen call for a box height of 90 px. All apps are affected on both production and test sites Hence something has been changed in the google side

richpomfret commented 5 years ago

From their dev team: 'We have feedback from our dev team. Our dev team we see that the ad request is being made for the size 728x280 while the header height is 170px. As a result, we are seeing large ads on the page. We can suggest the publisher update the ad tags by changing the height to 90px or less than 170px.'

And.

'Based on the screenshots: http://prntscr.com/oz6vjg http://prntscr.com/oz6vum

The ad is being requested for the size 728x280 -- Which is too big for the site. Here, the height set in the media query is being overridden by the size being passed in the ad request.'

Captainkirkdawson commented 5 years ago

I have changed the ads.txt file in each app so it only refers to the publisher for that app ie have removed the cen add from reg and the reg add from cen in the ads.txt file. test3 is running this deployment. reg continues to show a large add. cen shows only an empty box in the header and a blank advertisement in the body of a successful search the same as in production

Captainkirkdawson commented 5 years ago

BUT @richpomfret why is this happening. We have not changed our code and why no ads at all on cen I presume within this context we are the publisher?

Vino-S commented 5 years ago

@richpomfret the advert code we use doesn't request for an advert of height 280. Not sure from where this request is made.

Captainkirkdawson commented 5 years ago

But @Vino-S look at the actual request as shown below ad_call.JPG

Vino-S commented 5 years ago

Yes Kirk. Thats what I am unsure of. The google adsense script is making an advert request of height 280 px. But we have not set this in our code.

richpomfret commented 5 years ago

@Vino-S that request must be made within the Adsense/publisher side. They are suggesting it is from one of our old adverts, yet we didn't encounter this issue before (my baffle-o-meter is now broken).

@Captainkirkdawson there must be errors in the codes we use for FreeCEN Adsense (I have no access to this so cannot check) - either the ID or publisher itself. I am a bit surprised by it working on Production though (I might speculate that it is related, somehow, to our cookie policy 'show personalised ads or not' code, but that might be a rabbit-hole).

Vino-S commented 5 years ago

I had removed the data-ad-format="auto" from the script. We are now getting usual size

Vino-S commented 5 years ago

But the bad news is I could not see any adverts in my local

Captainkirkdawson commented 5 years ago

But the key question is not reg it is cen and bmd where we are showing NO ads in production bmd ad.JPGcen ad.JPG

Captainkirkdawson commented 5 years ago

I say forget reg and track why cen and bmd have gone AWOL and bmd has nothing to do with all our code

Off to golf have fun

richpomfret commented 5 years ago

@Captainkirkdawson I am quite confident that BMD will be resolved once the update to the ads.txt is processed by Google (can take 24hrs apparently). The mystery remains with CEN. Although I am glad removing the unnecessary 'data-ad-format' has helped in some way. Must be some weird setting in Adsense for the specific advert on the CEN homepage, as ads are being served on other pages.

Captainkirkdawson commented 5 years ago

test3 cen shows no ad! so the change made no difference

richpomfret commented 5 years ago

An advert DOES pop up on the cookie policy page (https://test3.freecen.org.uk/cms/about/cookie-policy). It's a terrible advert, but shows 'some' adverts are getting through.

Vino-S commented 5 years ago

@richpomfret I have tried to hard code the width and height in adsense script but no adverts are served after the change both in REG and CEN.

richpomfret commented 5 years ago

Note that adverts ARE BACK on FreeBMD!! Which is excellent. Less excellent is the news that they take up 1/3 of the screen and link to other genealogy sites.

Captainkirkdawson commented 5 years ago

Have experimented with various settings to no avail. Over to the UK people

Captainkirkdawson commented 5 years ago

I had one last thought. I eliminated the style. Looked OK on test so deployed to colobus. The sizing now appears correct. There are no ads though. Is this because the update redeploys the ads.txt and that needs time to percolate through? However I note that the reg search form is corrupted why I do not know and bed calls There were no changes to the search form so where it has gone wonky is beyond me

richpomfret commented 5 years ago

Adverts appearing cause the issue - you'll note that if you switch on an adblocker than the advert block size is fine, the size increases when served with an extra large advert. Perhaps the solution will be in providing a new advert with the right size via Adsense - then working out what has happened to break things later (it remains a weird mystery).

As an additional point, I wonder if it would help IF we deployed the change we made for FreeBMD2 in terms of moving the logo into the navi-bar and thus, freeing up the top section for a banner advert. It would presumably allow responsive adverts function more effectively.

Captainkirkdawson commented 5 years ago

Disagree with your conclusion @richpomfret The box had a height of 280px before any ad was displayed. If we detect an adblocker we deploy our message with a different piece of code.

Captainkirkdawson commented 5 years ago

As noted 3 comments above I eliminated the style in our code snippet on colobus https://github.com/FreeUKGen/MyopicVicar/blob/c0aee670269338f2cc5557083f67435f3bbcb311/app/helpers/application_helper.rb#L560 As it might be overriding our style specs earlier in the code (This is effectively what their developers noted) BUT it is only a partial style and has no height or width. This gives a box of the correct dimensions on colobus PS this is similar to what @Vino-S tried by hard coding the height and width HOWEVER no ads. Now the deployment rewrites the ads.txt; do we need to wait for a day for this to be picked up??

Captainkirkdawson commented 5 years ago

I have reverted colobus to the original code as my change had broken the search page on reg likely something to do with a misplaced script closure in the changes I had made to the application helper. Reg is now instantly delivering ads as before with a large box. so clearly one does not need to wait on ads.txt as there are no changes. Cen continues to not deliver ads even with the larger box.

Captainkirkdawson commented 5 years ago

Without further feedback from @Vino-S or @richpomfret I am going to stop further work on the issue. I have no background in ads and adsense. The code in the application helper in this area is extremely hard to follow. What seems to me is that we may be breaking the rules for the code snippet we use where they emphasize time and again the need for the code snippet on adsense to match the code in our application. Perhaps it only worked previously by pure luck. Looking at the adsense documentation Display ad units are responsive by default. They allow you to support a wide range of devices (i.e., computers, phones, tablets, etc.) by automatically adapting the size of the ads to fit your page layout. Regardless of which device users use to visit your site, display ad units can help you provide a great ad experience. BUT there is a caution If your site uses scripts that are executed before our responsive ad code, for example, to hide the ads in your page until the page has fully loaded, then our ad code won’t be able to calculate the required size for the responsive ad unit. In this case, you’ll need to modify your code and use CSS media queries to set the size of the ad unit. Find out how to modify your responsive ad code. We do that but?

PatReynolds commented 5 years ago

I have searched for others reporting the same issue with no results.

I have tried asking Adsense directly (doesn't seem to be a way) and their community forum (again, seems to no longer be taking questions).

Not sure if it helps: no adverts showing on FreeBMD in Commodo, but correctly sized ones on FreeREG and FreeCEN.

richpomfret commented 5 years ago

YES! Great stuff, thanks @Captainkirkdawson and indeed @Vino-S. We might need to go in and check the Adsense code on FC and ensure it is hooked up properly (we assume not but neither Kirk or I have access yet I think).

Captainkirkdawson commented 5 years ago

@richpomfret @PatReynolds After 2 solid days of research have concluded: Our use of ad units is an absolute mess and impossible to follow. It has multiple definitions of classes in many different places that are modified with js including detecting ad blockers and personalization in cookies. The responsive nature is limited and semi broken. Mobile ads are not displayed on either reg or cen. After much experimentation which are noted in https://docs.google.com/document/d/1exiX0ijQsiPKJ9EERpLpjJeLQ1zJyne3X5dmjE0AzIU/edit

I NOW BELIEVE MUCH OF THAT NOTE IS IRRELEVANT added 10 Sept I was able to get reg and cen to display the header in the correct size and cen to display its body placement adverts on the Search; Search results and Database coverage pages. I was unable to determine where a section of code full size width was in fact used. The js code appears to manipulate classes called cen_unit and cen_unit_header but these never appear to be defined We should NOT touch CEN ads until we have expanded REG This code in deployed in branch systemwide_67 and is running on test3 REG and CEN. I recommend its deployment to master and into production

PatReynolds commented 5 years ago

Agreed @captain kirk, no change on FreeCEN.

FreeCEN on mobile (android, large screen): no adverts/advert spaces showing.

FreeCEN on Chrome: and Commodo advert spaces showing, not filled with adverts. Changing to incognito does not change situation. Changing "ad balance" so that 100% of advert spaces are sold, and should be showing adverts makes no difference.

FreeREG on mobile: no adverts / advert spaces showing.

FreeREG on chromse and Commodo advert spaces showing, not filled with adverts.

Captainkirkdawson commented 5 years ago

I have compared the ad code running on both chrome and firefox and they are identical difference.JPG Only remaining comment is can it be that are no adverts for 9303635649 on test3?

The fundamental problem with our current banner code in production is that it has a style of display="block" rather than "block-insert". The latter forces the display to be within the box boundary. The former permits it to go outside the box boundary. Our box boundary is 728x90. It means that bigger adverts can be displayed. ie adverts of 728x280 will be displayed with a block setting and not with the block-inline.

richpomfret commented 5 years ago

Thanks for the in-depth research and findings @Captainkirkdawson. I think our external ad collaborators will ask us to switch from responsive to fixed - seeing as the new ads they have created are all fixed. This may result in a bit of a rewrite to our code - although not to the JS code which controls the personalised cookies or not, nor our adblocker detection code which then provides our own 'donate to freeukgen' message.

We should, for now, deploy the fix to production ("block-insert"). We can then look at implementing new and fixed adverts across both FR and FC - note that these can take a few hours for adverts to start being served (apparently).

richpomfret commented 5 years ago

@Vino-S can you shed any light on the JS scripts ('The js code appears to manipulate classes called cen_unit and cen_unit_header but these never appear to be defined') Kirk has pointed out?

Captainkirkdawson commented 5 years ago

Following the scrum review have abandoned systemwide_67 and reverted to master. Going to try minimal changes into branch systemwide_59

Captainkirkdawson commented 5 years ago

(This note is being edited as I keep clearing cookies) @richpomfret @PatReynolds @Vino-S Current state of play. Added cookie policy link to footer. Simply changing to inline-block did not help Changed to fixed, removed the attempt at responsiveness and made style style="display:inline-block;width:728px;height:90px" Experimented first with Chrome Cen. Cleared ALL cookies. This is important. Accepted cookies. No ads. Clicked cookie policy in footer. ads appeared immediately and the personalized setting changed itself. Go to another page and the ads disappeared. went to cookie page and the ads appeared immediately and the personalized setting changed itself for this one page. quite repeatable Reg. Never got the cookie acceptance page!!!. No ads. go to cookie policy. personalized ads are not accepted. While it can be changed nothing happens and it is not being retained. Experimented with Firefox Cleared ALL cookies. Cen. Cookie acceptance come up. accepted. No ads. The personalized setting was set. nothing happened if changed or went to different page. Cannot get ads Reg. NO COOKIE acceptance. no ads. cleared all cookies again and ads appeared!!!!!! cookie policy says no personalized ads and while it can be changed nothing happens and it is not being retained. This whole thing makes no sense my suspicion is we have an unset variable which we are not saving Unless @Vino-S has suggestions only way forward seems that we need to get rid of the js associated with personalized ads

richpomfret commented 5 years ago

Thanks for your perseverance here @Captainkirkdawson, I know this is rather infuriating. Hopefully @Vino-S can assist as she helped implement the original JS code (although I think Scott also modified it). I've also asked our advert guys for their GDPR code/cookie solution in case we need it.

The unset variable hypothesis makes sense - although I also note similar findings in terms of FC seemingly saving user cookie selection but FR is not. The user doesn't even get offered the Accept Cookie notice on FR but does over on FC. Cookie selection is then NOT saved on FR when changes are made. I say 'saved' in the sense that it appears that way to the user within the interface.

I've taken a look and I can see that the cookie acceptance code differs between FR and FC - which may (I might be straw-grasping here) account for the cookie notice NOT initially appearing on FR when visiting for the first time (cookies cleared). FR does not have the following code: Screen Shot 2019-09-12 at 15 02 02

FR also does not appear to be saving the cookie choice when selecting on/off and navigating between pages. FC does. But no adverts coming through on FC ASIDE FROM on the cookie policy page itself, just as you have found. As you say, commenting out the cookie code might help troubleshoot where it is going wrong.

Baffled, Surbiton.

Captainkirkdawson commented 5 years ago

@richpomfret it is clear that we are not going to resolve this anytime soon. So I propose to move the footer access to the cookie control into a separate branch so it can be merged into master and deployed so we can at least see things easier in production and vervet.

Then will commit my changes to systemwide_59 so others can see and change.

My knowledge of js on a scale of 1 to 100 is 2 so I am highly dangerous. I have come to realize my comments about .cen_unit and .cen_unit_header were TOTAL nonsense. They were the way the js is asking if we were using are using a class of that name cen_unit or . cen_unit_header So maybe I have gone to a 3 now

Captainkirkdawson commented 5 years ago

@richpomfret the code snippet is nothing but a diagnostic message as far as my limited knowledge is concerned Have done as proposed in the previous post. (Master updates have NOT been deployed)

PS I note that the cookie policy has no way of declining cookies may be worth a separate story

Vino-S commented 5 years ago

After experimenting on adsense code, figured out the issue was in data-ad-format="auto". According to google help documentation: data-ad-format tag with the value of "auto" enables the auto-sizing behavior for the responsive ad unit. Which in my opinion means google auto re-sizes the advert irrespective of size we provide in our CSS.

One possible solution to the issue can be to remove data-ad-format="auto" so that our custom sizing of advert through CSS in various media types can become effective. Example of implementation: https://support.google.com/adsense/answer/9183363