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.

Sign up for GitHub

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

Reinstate affiliate links for all eligible DCR articles behind a 0% test #27150

Merged
merged 6 commits into from
May 16, 2024
Merged

Reinstate affiliate links for all eligible DCR articles behind a 0% test #27150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
71fe2fa
Check if first two elements are paragraphs for DCR articles
emma-imber May 14, 2024
6f41a02
Add 0% test for affiliate links on DCR
emma-imber May 14, 2024
ddf35b7
Check if user is in test variant
emma-imber May 14, 2024
8220330
Remove unnecessary var
emma-imber May 14, 2024
0c31a5f
Remove variant test
emma-imber May 14, 2024
c7b0702
Add minimum character count for second p element
emma-imber May 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions common/app/experiments/Experiments.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ object ActiveExperiments extends ExperimentsDefinition {
DCRVideoPages,
UpdatedHeaderDesign,
MastheadWithHighlights,
AffiliateLinksDCR,
)
implicit val canCheckExperiment: CanCheckExperiment = new CanCheckExperiment(this)
}
Expand Down Expand Up @@ -48,6 +49,15 @@ object DarkModeWeb
participationGroup = Perc0D,
)

object AffiliateLinksDCR
extends Experiment(
name = "affiliate-links-dcr",
description = "Display affiliate links on all eligible DCR articles",
owners = Seq(Owner.withGithub("[email protected]")),
sellByDate = LocalDate.of(2024, 7, 30),
participationGroup = Perc0E,
)

object DCRVideoPages
extends Experiment(
name = "dcr-video-pages",
Expand Down
2 changes: 1 addition & 1 deletion common/app/model/dotcomrendering/DotcomBlocksRenderingDataModel.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ object DotcomBlocksRenderingDataModel {
bodyBlocks: Seq[APIBlock],
): DotcomBlocksRenderingDataModel = {
val content = page.item
val shouldAddAffiliateLinks = DotcomRenderingUtils.shouldAddAffiliateLinks(content)
val shouldAddAffiliateLinks = DotcomRenderingUtils.shouldAddAffiliateLinks(content)(request)
val contentDateTimes = DotcomRenderingUtils.contentDateTimes(content)

val edition = Edition(request)
Expand Down
2 changes: 1 addition & 1 deletion common/app/model/dotcomrendering/DotcomRenderingDataModel.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,7 +487,7 @@ object DotcomRenderingDataModel {
blocks.exists(block => DotcomRenderingUtils.stringContainsAffiliateableLinks(block.bodyHtml))
}

val shouldAddAffiliateLinks = DotcomRenderingUtils.shouldAddAffiliateLinks(content)
val shouldAddAffiliateLinks = DotcomRenderingUtils.shouldAddAffiliateLinks(content)(request)
val shouldAddDisclaimer = hasAffiliateLinks(bodyBlocks)

val contentDateTimes: ArticleDateTimes = ArticleDateTimes(
Expand Down
35 changes: 22 additions & 13 deletions common/app/model/dotcomrendering/DotcomRenderingUtils.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import model.{
Pillar,
}
import org.joda.time.format.DateTimeFormat
import org.jsoup.Jsoup
import play.api.libs.json._
import play.api.mvc.RequestHeader
import views.support.AffiliateLinksCleaner
Expand Down Expand Up @@ -241,19 +242,27 @@ object DotcomRenderingUtils {
}
}

def shouldAddAffiliateLinks(content: ContentType): Boolean = {
AffiliateLinksCleaner.shouldAddAffiliateLinks(
switchedOn = Switches.AffiliateLinks.isSwitchedOn,
section = content.metadata.sectionId,
showAffiliateLinks = content.content.fields.showAffiliateLinks,
supportedSections = Configuration.affiliateLinks.affiliateLinkSections,
defaultOffTags = Configuration.affiliateLinks.defaultOffTags,
alwaysOffTags = Configuration.affiliateLinks.alwaysOffTags,
tagPaths = content.content.tags.tags.map(_.id),
firstPublishedDate = content.content.fields.firstPublicationDate,
pageUrl = content.metadata.id,
contentType = "article",
)
def shouldAddAffiliateLinks(content: ContentType)(implicit request: RequestHeader): Boolean = {
val contentHtml = Jsoup.parse(content.fields.body)
val bodyElements = contentHtml.select("body").first().children()
Copy link
Contributor

@SiAdcock SiAdcock May 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Non blocking: I'm not wedded to this suggestion, and I'm not sure of the best place, but would an explanatory comment help?

Suggested change
val bodyElements = contentHtml.select("body").first().children()
val bodyElements = contentHtml.select("body").first().children()
/**
* On smaller devices, the disclaimer is inserted before paragraph 2 of the article body and floats left.
* This logic ensures there are two clear paragraphs of text at the top of the article.
* We don't support inserting the disclaimer next to other element types.
* It also ensures the second paragraph is long enough to accommodate the disclaimer appearing alongside it.
*/

if (bodyElements.size >= 2) {
val firstEl = bodyElements.get(0)
val secondEl = bodyElements.get(1)
if (firstEl.tagName == "p" && secondEl.tagName == "p" && secondEl.text().length >= 250) {
AffiliateLinksCleaner.shouldAddAffiliateLinks(
switchedOn = Switches.AffiliateLinks.isSwitchedOn,
section = content.metadata.sectionId,
showAffiliateLinks = content.content.fields.showAffiliateLinks,
supportedSections = Configuration.affiliateLinks.affiliateLinkSections,
defaultOffTags = Configuration.affiliateLinks.defaultOffTags,
alwaysOffTags = Configuration.affiliateLinks.alwaysOffTags,
tagPaths = content.content.tags.tags.map(_.id),
firstPublishedDate = content.content.fields.firstPublicationDate,
pageUrl = content.metadata.id,
contentType = "article",
)
} else false
} else false
}

def contentDateTimes(content: ContentType): ArticleDateTimes = {
Expand Down
10 changes: 6 additions & 4 deletions common/app/views/support/HtmlCleaner.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import common.{Edition, GuLogging, LinkTo}
import conf.Configuration.affiliateLinks._
import conf.Configuration.site.host
import conf.switches.Switches._
import experiments.{ActiveExperiments, AffiliateLinksDCR}
import layout.ContentWidths
import layout.ContentWidths._
import model._
Expand Down Expand Up @@ -873,7 +874,8 @@ case class AffiliateLinksCleaner(
tags: List[String],
publishedDate: Option[DateTime],
contentType: String,
) extends HtmlCleaner
)(implicit request: RequestHeader)
extends HtmlCleaner
with GuLogging {

override def clean(document: Document): Document = {
Expand Down Expand Up @@ -946,7 +948,7 @@ object AffiliateLinksCleaner {
firstPublishedDate: Option[DateTime],
pageUrl: String,
contentType: String,
): Boolean = {
)(implicit request: RequestHeader): Boolean = {
val publishedCutOffDate = new DateTime(2020, 8, 14, 0, 0)

val cleanedPageUrl = if (pageUrl.charAt(0) == '/') {
Expand All @@ -973,11 +975,11 @@ object AffiliateLinksCleaner {

// Never include affiliate links if it is tagged with an always off tag, or if it was published before our cut off date.
// The cut off date is temporary while we are working on improving the compliance of affiliate links.
// The cut off date does not apply to any URL on the allow list
// The cut off date does not apply to any URL on the allow list or to galleries
if (
!contentHasAlwaysOffTag(tagPaths, alwaysOffTags) && (firstPublishedDate.exists(
_.isBefore(publishedCutOffDate),
) || urlIsInAllowList || contentType == "gallery")
) || urlIsInAllowList || contentType == "gallery" || ActiveExperiments.isParticipating(AffiliateLinksDCR))
) {
if (showAffiliateLinks.isDefined) {
showAffiliateLinks.contains(true)
Expand Down
25 changes: 14 additions & 11 deletions common/test/views/support/cleaner/AffiliateLinksCleanerTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package views.support.cleaner
import conf.Configuration
import conf.switches.Switches.ServerSideExperiments
import org.joda.time.DateTime
import org.scalatest.flatspec.AnyFlatSpec
import org.scalatest.matchers.should.Matchers
import play.api.test.FakeRequest
import views.support.AffiliateLinksCleaner._

class AffiliateLinksCleanerTest extends AnyFlatSpec with Matchers {
Expand All @@ -16,6 +18,7 @@ class AffiliateLinksCleanerTest extends AnyFlatSpec with Matchers {
}

"shouldAddAffiliateLinks" should "correctly determine when to add affiliate links" in {
val fakeTestControlRequest = FakeRequest().withHeaders("X-GU-Experiment-0perc-E" -> "control")
val supportedSections = Set("film", "books", "fashion")
val oldPublishedDate = Some(new DateTime(2020, 8, 13, 0, 0))
val newPublishedDate = Some(new DateTime(2020, 8, 15, 0, 0))
Expand All @@ -32,7 +35,7 @@ class AffiliateLinksCleanerTest extends AnyFlatSpec with Matchers {
oldPublishedDate,
deniedPageUrl,
"article",
) should be(false)
)(fakeTestControlRequest) should be(false)
shouldAddAffiliateLinks(
switchedOn = true,
"film",
Expand All @@ -44,7 +47,7 @@ class AffiliateLinksCleanerTest extends AnyFlatSpec with Matchers {
oldPublishedDate,
deniedPageUrl,
"article",
) should be(true)
)(fakeTestControlRequest) should be(true)
shouldAddAffiliateLinks(
switchedOn = true,
"film",
Expand All @@ -56,7 +59,7 @@ class AffiliateLinksCleanerTest extends AnyFlatSpec with Matchers {
oldPublishedDate,
deniedPageUrl,
"article",
) should be(false)
)(fakeTestControlRequest) should be(false)
shouldAddAffiliateLinks(
switchedOn = true,
"news",
Expand All @@ -68,7 +71,7 @@ class AffiliateLinksCleanerTest extends AnyFlatSpec with Matchers {
oldPublishedDate,
deniedPageUrl,
"article",
) should be(true)
)(fakeTestControlRequest) should be(true)
shouldAddAffiliateLinks(
switchedOn = true,
"news",
Expand All @@ -80,7 +83,7 @@ class AffiliateLinksCleanerTest extends AnyFlatSpec with Matchers {
oldPublishedDate,
deniedPageUrl,
"article",
) should be(false)
)(fakeTestControlRequest) should be(false)
shouldAddAffiliateLinks(
switchedOn = true,
"news",
Expand All @@ -92,7 +95,7 @@ class AffiliateLinksCleanerTest extends AnyFlatSpec with Matchers {
oldPublishedDate,
deniedPageUrl,
"article",
) should be(false)
)(fakeTestControlRequest) should be(false)
shouldAddAffiliateLinks(
switchedOn = true,
"fashion",
Expand All @@ -104,7 +107,7 @@ class AffiliateLinksCleanerTest extends AnyFlatSpec with Matchers {
oldPublishedDate,
deniedPageUrl,
"article",
) should be(true)
)(fakeTestControlRequest) should be(true)
shouldAddAffiliateLinks(
switchedOn = true,
"fashion",
Expand All @@ -116,7 +119,7 @@ class AffiliateLinksCleanerTest extends AnyFlatSpec with Matchers {
oldPublishedDate,
deniedPageUrl,
"article",
) should be(false)
)(fakeTestControlRequest) should be(false)
shouldAddAffiliateLinks(
switchedOn = true,
"fashion",
Expand All @@ -128,7 +131,7 @@ class AffiliateLinksCleanerTest extends AnyFlatSpec with Matchers {
oldPublishedDate,
deniedPageUrl,
"article",
) should be(true)
)(fakeTestControlRequest) should be(true)
shouldAddAffiliateLinks(
switchedOn = true,
"film",
Expand All @@ -140,7 +143,7 @@ class AffiliateLinksCleanerTest extends AnyFlatSpec with Matchers {
newPublishedDate,
deniedPageUrl,
"article",
) should be(false)
)(fakeTestControlRequest) should be(false)
shouldAddAffiliateLinks(
switchedOn = true,
"film",
Expand All @@ -152,6 +155,6 @@ class AffiliateLinksCleanerTest extends AnyFlatSpec with Matchers {
newPublishedDate,
deniedPageUrl,
"gallery",
) should be(true)
)(fakeTestControlRequest) should be(true)
}
}