Amp access toggle for ads #6079

thekp · 2020-04-02T15:35:43Z

Resolves #5924

Overall change:
Use amp access to conditionally render amp-ads by fetching data from the toggles endpoint.

Code changes:

Following the same implementation as the current AMP page, using amp-access to conditionally render amp-ads
Update snapshots
Added some comments, to clarify the ads work is currently just on Pidgin
enabled Ads for the following services:

arabic
hindi
mundo
portuguese
russian
zhongwen

I have assigned myself to this PR and the corresponding issues
I have added labels to this PR for the relevant pod(s) affected by these changes
I have assigned this PR to the Simorgh project

Testing:

Automated (jest and/or cypress) tests added (for new features) or updated (for existing features)
If necessary, I have run the local E2E non-smoke tests relevant to my changes (CYPRESS_APP_ENV=local CYPRESS_SMOKE=false npm run test:e2e:interactive)
This PR requires manual testing

thekp · 2020-04-02T15:47:12Z

src/app/containers/Ad/Amp/index.jsx

+      </Helmet>
+      <div
+        amp-access="toggles.ads.enabled AND geoIp.advertiseCombined"
+        amp-access-hide="true"


I've added a string here because when I run the unit tests for Ads I get this console warning...

And this attribute hides the div by default, until we get a successful authorization response right?

Yep that is correct 💪 noted in this doc, I should've linked... https://amp.dev/documentation/components/amp-access/

The amp-access-hide attribute can be used to optimistically hide the element before the Authorization response has been received, which can show it. It provides the semantics of “invisible by default”. The authorization response returned by the Authorization later may rescind this default and make section visible.

I've just gone down a bit of a rabbit hole looking into this - it looks like react is buggy around boolean, non-standard, html attributes (i.e. amp-access-hide isn't an html attribute). See here: facebook/react#9230 - they recommend using amp-access-hide="" instead, which is rendered as amp-access-hide. This is a bit weird, not sure which I prefer, as amp-access-hide="true" isn't quite what we should be passing to the AMP component (and amp-access-hide="false" wouldn't have the opposite effect, due to how boolean attributes work)

thekp · 2020-04-02T16:07:10Z

src/app/containers/Ad/Amp/index.jsx

+        {AMP_ACCESS_JS}
+        {AMP_ACCESS_FETCH(service)}
+      </Helmet>
+      <div


I've created a div here to pass the amp attributes to (amp-access/amp-access-hide), as styled components strip out the attributes that are not on their whitelist.

thekp · 2020-04-02T16:12:36Z

src/app/containers/Ad/Amp/index.jsx

@@ -36,29 +63,54 @@ const ampAdPropsDesktop = ({ service }) => ({
  json: JSON.stringify(constructAdJsonData({ service })),
 });

+const AMP_ACCESS_DATA = (endpoint) => ({
+  authorization: endpoint,
+  noPingback: true,


I don't think we need a pingback to happen so I've disabled it
pingback doc: https://amp.dev/documentation/components/amp-access/#pingback-endpoint

thekp · 2020-04-02T16:12:57Z

src/app/containers/Ad/Amp/index.jsx

@@ -36,29 +63,54 @@ const ampAdPropsDesktop = ({ service }) => ({
  json: JSON.stringify(constructAdJsonData({ service })),
 });

+const AMP_ACCESS_DATA = (endpoint) => ({
+  authorization: endpoint,


authorization doc: https://amp.dev/documentation/components/amp-access/#authorization-endpoint

…into AmpAccessToggleForAds

thekp · 2020-04-03T09:09:41Z

scripts/bundleSizeConfig.js

@@ -2,5 +2,5 @@ module.exports = {
  // Size limit for all bundles used by each service (K)
  // Keep these +/- 5K and update frequently!
  MIN_SIZE: 743,
-  MAX_SIZE: 793,
+  MAX_SIZE: 800,


Highest current bundle:

DenisHdz · 2020-04-03T13:30:25Z

src/app/containers/Ad/Amp/index.jsx

+        {AMP_ACCESS_FETCH(service)}
+      </Helmet>
+      <div
+        amp-access="toggles.ads.enabled AND geoIp.advertiseCombined"


So this will be either true or false depending on the response returned by the endpoint, right?

Yep that is correct.

This is an example of the endpoint data response btw:

{ "toggles": { "ads": { "enabled": true, "value": "" } }, "geoIp": { "ukCombined": true, "advertiseCombined": false, "countryCode": "gb" } }

DenisHdz

This looks good to me, thanks for the comments @thekp 👍

tochwill

Looks good to me, just one little discussion point! Would be worth someone who was more involved in the canonical toggles work taking a look too :)

tochwill · 2020-04-03T13:57:03Z

src/app/containers/Ad/Amp/index.jsx

+      </Helmet>
+      <div
+        amp-access="toggles.ads.enabled AND geoIp.advertiseCombined"
+        amp-access-hide="true"


I've just gone down a bit of a rabbit hole looking into this - it looks like react is buggy around boolean, non-standard, html attributes (i.e. amp-access-hide isn't an html attribute). See here: facebook/react#9230 - they recommend using amp-access-hide="" instead, which is rendered as amp-access-hide. This is a bit weird, not sure which I prefer, as amp-access-hide="true" isn't quite what we should be passing to the AMP component (and amp-access-hide="false" wouldn't have the opposite effect, due to how boolean attributes work)

PriyaKR · 2020-04-09T09:29:36Z

This PR is waiting for testing as the GeoIp shows error status in isite. Once this is fixed then it could be picked up for testing.

PriyaKR · 2020-04-14T13:25:26Z

Testing:
I tried using VPN to set the geo location as outside UK and looking at the http://localhost:7080/mundo.amp i dont see any ads being displayed.

PriyaKR · 2020-04-14T13:48:02Z

This is again blocked because the geoIP endpoint seem to have changed and we got to update our toggle API to use the new endpoint.

@thekp is creating a new issue to do this work.

thekp · 2020-04-15T09:27:13Z

This is again blocked because the geoIP endpoint seem to have changed and we got to update our toggle API to use the new endpoint.
@thekp is creating a new issue to do this work.

Seems like the geoIP endpoint has been fixed.

As a team, we decided to get this work merged since this is not shown on LIVE and test this on our TEST env.

PriyaKR · 2020-04-16T14:20:25Z

Testing on Test env:
https://www.test.bbc.com/pidgin.amp
The ads are displayed across all browsers and devices when the toggle is enabled(for non-uk) and ads are not shown when the toggle is disabled(for non-uk).

Testing on latest opera mini couldn't be performed as this browser doesn't have proxy settings.

PriyaKR · 2020-04-17T08:47:59Z

@EinsteinNjoroge verified in opera mini and it looks good.

thekp added 4 commits April 2, 2020 16:14

move styled ad logic to amp file

91fa08f

add styledAd, add amp access to conditionally load ads client side

0ae1b25

update amp-access-hide prop to pass string true

737650b

update snapshots

d791cbb

thekp added AMP Work related to AMP ws-home ads labels Apr 2, 2020

thekp added this to the Simorgh 3.0 milestone Apr 2, 2020

thekp self-assigned this Apr 2, 2020

thekp added this to PR in Progress in Simorgh via automation Apr 2, 2020

thekp commented Apr 2, 2020

View reviewed changes

thekp and others added 3 commits April 2, 2020 17:21

update endpoint url to respect environment

90ff782

update tests

b7aa0d3

Merge branch 'latest' into AmpAccessToggleForAds

d934e71

thekp marked this pull request as ready for review April 2, 2020 16:33

thekp added 2 commits April 2, 2020 17:39

increase bundle size

50f5bd2

Merge branch 'AmpAccessToggleForAds' of https://github.com/bbc/simorgh …

390bbad

…into AmpAccessToggleForAds

thekp moved this from PR in Progress to Code review in Simorgh Apr 2, 2020

resolve conflicts

f98d263

thekp commented Apr 3, 2020

View reviewed changes

DenisHdz reviewed Apr 3, 2020

View reviewed changes

DenisHdz approved these changes Apr 3, 2020

View reviewed changes

tochwill reviewed Apr 3, 2020

View reviewed changes

thekp and others added 2 commits April 3, 2020 15:06

resolve conflicts

7da7288

Merge branch 'latest' into AmpAccessToggleForAds

936ae82

thekp requested a review from sareh April 6, 2020 09:00

tochwill approved these changes Apr 7, 2020

View reviewed changes

sareh moved this from Ready for Test to In Test in Simorgh Apr 9, 2020

thekp and others added 4 commits April 9, 2020 10:01

remove comment

ca4533e

update config to enable ads

40721a5

update integration snapshots

f94400d

Merge branch 'latest' into AmpAccessToggleForAds

5e921ac

PriyaKR added the blocked This issue should not be worked on until another internal issue is completed - see desc for details label Apr 9, 2020

PriyaKR moved this from In Test to Ready for Test in Simorgh Apr 9, 2020

PriyaKR moved this from Ready for Test to In Test in Simorgh Apr 14, 2020

PriyaKR removed the blocked This issue should not be worked on until another internal issue is completed - see desc for details label Apr 14, 2020

thekp mentioned this pull request Apr 14, 2020

Ads fallback behaviour #6176

Closed

1 task

Merge branch 'latest' into AmpAccessToggleForAds

79d4982

PriyaKR added the blocked This issue should not be worked on until another internal issue is completed - see desc for details label Apr 14, 2020

PriyaKR moved this from In Test to Ready for Test in Simorgh Apr 14, 2020

PriyaKR moved this from Ready for Test to In Test in Simorgh Apr 14, 2020

PriyaKR removed the blocked This issue should not be worked on until another internal issue is completed - see desc for details label Apr 15, 2020

Merge branch 'latest' into AmpAccessToggleForAds

3215600

thekp merged commit 064769b into latest Apr 15, 2020

Simorgh automation moved this from In Test to Done Apr 15, 2020

thekp deleted the AmpAccessToggleForAds branch April 15, 2020 09:58

thekp moved this from Done to In Test in Simorgh Apr 15, 2020

PriyaKR moved this from In Test to Ready for Test in Simorgh Apr 16, 2020

PriyaKR moved this from Ready for Test to In Test in Simorgh Apr 16, 2020

PriyaKR moved this from In Test to Done in Simorgh Apr 17, 2020

PriyaKR mentioned this pull request Apr 22, 2020

Integration tests for ads #6255

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Amp access toggle for ads #6079

Amp access toggle for ads #6079

thekp commented Apr 2, 2020 •

edited

thekp Apr 2, 2020

DenisHdz Apr 3, 2020

thekp Apr 3, 2020 •

edited

tochwill Apr 3, 2020 •

edited

thekp Apr 2, 2020

thekp Apr 2, 2020 •

edited

thekp Apr 2, 2020

thekp Apr 3, 2020 •

edited

DenisHdz Apr 3, 2020

thekp Apr 3, 2020

DenisHdz left a comment

tochwill left a comment

tochwill Apr 3, 2020 •

edited

PriyaKR commented Apr 9, 2020

PriyaKR commented Apr 14, 2020

PriyaKR commented Apr 14, 2020

thekp commented Apr 15, 2020 •

edited

PriyaKR commented Apr 16, 2020

PriyaKR commented Apr 17, 2020

Amp access toggle for ads #6079

Amp access toggle for ads #6079

Conversation

thekp commented Apr 2, 2020 • edited

thekp Apr 2, 2020

Choose a reason for hiding this comment

DenisHdz Apr 3, 2020

Choose a reason for hiding this comment

thekp Apr 3, 2020 • edited

Choose a reason for hiding this comment

tochwill Apr 3, 2020 • edited

Choose a reason for hiding this comment

thekp Apr 2, 2020

Choose a reason for hiding this comment

thekp Apr 2, 2020 • edited

Choose a reason for hiding this comment

thekp Apr 2, 2020

Choose a reason for hiding this comment

thekp Apr 3, 2020 • edited

Choose a reason for hiding this comment

DenisHdz Apr 3, 2020

Choose a reason for hiding this comment

thekp Apr 3, 2020

Choose a reason for hiding this comment

DenisHdz left a comment

Choose a reason for hiding this comment

tochwill left a comment

Choose a reason for hiding this comment

tochwill Apr 3, 2020 • edited

Choose a reason for hiding this comment

PriyaKR commented Apr 9, 2020

PriyaKR commented Apr 14, 2020

PriyaKR commented Apr 14, 2020

thekp commented Apr 15, 2020 • edited

PriyaKR commented Apr 16, 2020

PriyaKR commented Apr 17, 2020

thekp commented Apr 2, 2020 •

edited

thekp Apr 3, 2020 •

edited

tochwill Apr 3, 2020 •

edited

thekp Apr 2, 2020 •

edited

thekp Apr 3, 2020 •

edited

tochwill Apr 3, 2020 •

edited

thekp commented Apr 15, 2020 •

edited