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

DCR flag and picker for Image Content #26169

Merged
merged 5 commits into from
May 31, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
47 changes: 39 additions & 8 deletions applications/app/controllers/ImageContentController.scala
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package controllers

import com.gu.contentapi.client.model.{Direction, FollowingSearchQuery, SearchQuery}
import com.gu.contentapi.client.model.v1.{ItemResponse, Content => ApiContent}
import com.gu.contentapi.client.model.v1.{Block, ItemResponse, Content => ApiContent}
import common._
import conf.switches.Switches
import contentapi.ContentApiClient
Expand All @@ -13,17 +13,17 @@ import services.ImageQuery
import views.support.RenderOtherStatus
import play.api.libs.json._
import conf.Configuration.contentApi
import model.dotcomrendering.{DotcomRenderingDataModel, PageType}
import renderers.DotcomRenderingService
import services.dotcomrendering.{ImageContentPicker, RemoteRender}

import scala.concurrent.Future

case class ImageContentPage(image: ImageContent, related: RelatedContent) extends ContentPage {
override lazy val item: ImageContent = image
}

class ImageContentController(
val contentApiClient: ContentApiClient,
val controllerComponents: ControllerComponents,
wsClient: WSClient,
remoteRenderer: renderers.DotcomRenderingService = DotcomRenderingService(),
)(implicit context: ApplicationContext)
extends BaseController
with RendersItemResponse
Expand All @@ -42,11 +42,42 @@ class ImageContentController(
}

override def renderItem(path: String)(implicit request: RequestHeader): Future[Result] =
image(Edition(request), path).map {
case Right(content) => renderImageContent(content)
case Left(result) => RenderOtherStatus(result)
image(Edition(request), path).flatMap {
case Right((content, mainBlock)) =>
val tier = ImageContentPicker.getTier(content)

tier match {
case RemoteRender => remoteRender(content, mainBlock)
case _ => Future.successful(renderImageContent(content))
}
case Left(result) => Future.successful(RenderOtherStatus(result))
}

private def getDCRJson(content: ImageContentPage, pageType: PageType, mainBlock: Option[Block])(implicit
request: RequestHeader,
): String = {
DotcomRenderingDataModel.toJson(DotcomRenderingDataModel.forImageContent(content, request, pageType, mainBlock))
}

private def remoteRender(content: ImageContentPage, mainBlock: Option[Block])(implicit
request: RequestHeader,
): Future[Result] = {
val pageType = PageType(content, request, context)

if (request.isJson) {
Future.successful(
common.renderJson(getDCRJson(content, pageType, mainBlock), content).as("application/json"),
)
} else {
remoteRenderer.getImageContent(
wsClient,
content,
pageType,
mainBlock,
)
}
}

private def isSupported(c: ApiContent) = c.isImageContent
override def canRender(i: ItemResponse): Boolean = i.content.exists(isSupported)

Expand Down
1 change: 1 addition & 0 deletions applications/app/controllers/InteractiveController.scala
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import pages.InteractiveHtmlPage
import play.api.libs.ws.WSClient
import play.api.mvc._
import renderers.DotcomRenderingService
import services.dotcomrendering.{DotcomRendering, InteractivePicker, PressedInteractive}
import services.{CAPILookup, USElection2020AmpPages, _}

import scala.concurrent.Future
Expand Down
4 changes: 2 additions & 2 deletions applications/app/pages/ContentHtmlPage.scala
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
package pages

import common.Edition
import controllers.{ImageContentPage, MediaPage, QuizAnswersPage, TodayNewspaper}
import controllers.{MediaPage, QuizAnswersPage, TodayNewspaper}
import html.HtmlPageHelpers._
import html.{HtmlPage, Styles}
import model.{ApplicationContext, Audio, Page}
import model.{ApplicationContext, Audio, ImageContentPage, Page}
import play.api.mvc.RequestHeader
import play.twirl.api.Html
import views.html.fragments._
Expand Down
17 changes: 11 additions & 6 deletions applications/app/services/ImageQuery.scala
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
package services

import com.gu.contentapi.client.model.v1.ItemResponse
import com.gu.contentapi.client.model.v1.{Block, Blocks, ItemResponse}
import common.{Edition, _}
import contentapi.ContentApiClient
import controllers.ImageContentPage
import model.{ApiContent2Is, ApplicationContext, Content, ImageContent, StoryPackages}
import model.{ApiContent2Is, ApplicationContext, Content, ImageContent, ImageContentPage, StoryPackages}
import play.api.mvc.{RequestHeader, Result => PlayResult}

import scala.concurrent.Future
Expand All @@ -16,18 +15,24 @@ trait ImageQuery extends ConciergeRepository {
def image(edition: Edition, path: String)(implicit
request: RequestHeader,
context: ApplicationContext,
): Future[Either[PlayResult, ImageContentPage]] = {
): Future[Either[PlayResult, (ImageContentPage, Option[Block])]] = {
log.info(s"Fetching image content: $path for edition ${edition.id}")
val response = contentApiClient.getResponse(
contentApiClient
.item(path, edition)
.showBlocks("main")
.showFields("all"),
) map { response: ItemResponse =>
val mainContent = response.content.filter(_.isImageContent).map(Content(_))
val mainBlock = response.content.flatMap(_.blocks).getOrElse(Blocks()).main
mainContent
.map {
case content: ImageContent => Right(ImageContentPage(content, StoryPackages(content.metadata.id, response)))
case _ => Left(NotFound)
case content: ImageContent =>
Right(
ImageContentPage(content, StoryPackages(content.metadata.id, response)),
mainBlock,
)
case _ => Left(NotFound)
}
.getOrElse(Left(NotFound))
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
package services.dotcomrendering

import common.GuLogging
import experiments.{ActiveExperiments, DCRImageContent}
import model.Cors.RichRequestHeader
import model.ImageContentPage
import play.api.mvc.RequestHeader
import utils.DotcomponentsLogger

object ImageContentPicker extends GuLogging {

/**
*
* Add to this function any logic for including/excluding
* an image article from being rendered with DCR
*
* Currently defaulting to false until we implement image articles in DCR
*
* */
private def dcrCouldRender(imageContentPage: ImageContentPage): Boolean = {
false
}

def getTier(
imageContentPage: ImageContentPage,
)(implicit
request: RequestHeader,
): RenderType = {

val participatingInTest = ActiveExperiments.isParticipating(DCRImageContent)
val dcrCanRender = dcrCouldRender(imageContentPage)

val tier = {
if (request.forceDCROff) LocalRender
else if (request.forceDCR) RemoteRender
else if (dcrCanRender && participatingInTest) RemoteRender
else LocalRender
}

if (tier == RemoteRender) {
DotcomponentsLogger.logger.logRequest(s"path executing in dotcomponents", Map.empty, imageContentPage.image)
} else {
DotcomponentsLogger.logger.logRequest(s"path executing in web", Map.empty, imageContentPage.image)
}

tier
}
}
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
package services
package services.dotcomrendering

import conf.switches.Switches.InteractivePickerFeature
import play.api.mvc.RequestHeader
import implicits.Requests._
import services.dotcomrendering.PressedContent
import play.api.mvc.RequestHeader

sealed trait RenderingTier
object DotcomRendering extends RenderingTier
Expand Down
5 changes: 5 additions & 0 deletions applications/app/services/dotcomrendering/RenderType.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package services.dotcomrendering

sealed trait RenderType
case object RemoteRender extends RenderType
case object LocalRender extends RenderType
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
@import model.ImageContentPage
@(page: ImageContentPage)(implicit request: RequestHeader, context: model.ApplicationContext)
@import layout.ContentWidths.ImageContentMedia
@import views.support.Commercial._
Expand Down
1 change: 1 addition & 0 deletions applications/test/services/InteractivePickerTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import org.scalatest.DoNotDiscover
import test.TestRequest
import org.scalatest.flatspec.AnyFlatSpec
import org.scalatest.matchers.should.Matchers
import services.dotcomrendering.{DotcomRendering, FrontendLegacy, InteractivePicker, PressedInteractive}

@DoNotDiscover class InteractivePickerTest extends AnyFlatSpec with Matchers {
val path = "/lifeandstyle/ng-interactive/2016/mar/12/stephen-collins-cats-cartoon"
Expand Down
6 changes: 3 additions & 3 deletions article/app/controllers/LiveBlogController.scala
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ import play.api.libs.ws.WSClient
import play.api.mvc._
import play.twirl.api.Html
import renderers.DotcomRenderingService
import services.dotcomrendering.DotcomponentsLogger
import services.{CAPILookup, NewsletterService, MessageUsService}
import topics.TopicService
import utils.DotcomponentsLogger
import views.support.RenderOtherStatus

import scala.concurrent.Future
Expand Down Expand Up @@ -191,7 +191,7 @@ class LiveBlogController(

if (remoteRendering) {
DotcomponentsLogger.logger
.logRequest(s"liveblog executing in dotcomponents", properties, page)
.logRequest(s"liveblog executing in dotcomponents", properties, page.article)
val pageType: PageType = PageType(blog, request, context)
remoteRenderer.getArticle(
ws,
Expand All @@ -206,7 +206,7 @@ class LiveBlogController(
messageUs,
)
} else {
DotcomponentsLogger.logger.logRequest(s"liveblog executing in web", properties, page)
DotcomponentsLogger.logger.logRequest(s"liveblog executing in web", properties, page.article)
Future.successful(common.renderHtml(LiveBlogHtmlPage.html(blog), blog))
}
case (blog: LiveBlogPage, AmpFormat) if isAmpSupported =>
Expand Down
7 changes: 4 additions & 3 deletions article/app/services/dotcomrendering/ArticlePicker.scala
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import com.madgag.scala.collection.decorators.MapDecorator
import implicits.Requests._
import model.{ArticlePage, PageWithStoryPackage}
import play.api.mvc.RequestHeader
import utils.DotcomponentsLogger

object ArticlePageChecks {

Expand Down Expand Up @@ -69,11 +70,11 @@ object ArticlePicker {
("pageTones" -> pageTones)

if (tier == RemoteRender) {
DotcomponentsLogger.logger.logRequest(s"path executing in dotcomponents", features, page)
DotcomponentsLogger.logger.logRequest(s"path executing in dotcomponents", features, page.article)
} else if (tier == PressedArticle) {
DotcomponentsLogger.logger.logRequest(s"path executing from pressed content", features, page)
DotcomponentsLogger.logger.logRequest(s"path executing from pressed content", features, page.article)
} else {
DotcomponentsLogger.logger.logRequest(s"path executing in web", features, page)
DotcomponentsLogger.logger.logRequest(s"path executing in web", features, page.article)
}

tier
Expand Down
9 changes: 9 additions & 0 deletions common/app/experiments/Experiments.scala
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,15 @@ object DCRFronts
participationGroup = Perc50,
)

object DCRImageContent
extends Experiment(
name = "dcr-image-content",
description = "Use DCR for image content",
owners = Seq(Owner.withGithub("@guardian/dotcom-platform")),
sellByDate = LocalDate.of(2024, 1, 1),
participationGroup = Perc50,
Copy link
Member

Choose a reason for hiding this comment

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

Don't think this should be 50% 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

ha! I will change to 0

)

object OfferHttp3
extends Experiment(
name = "offer-http3",
Expand Down
5 changes: 5 additions & 0 deletions common/app/model/ImageContentPage.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package model

case class ImageContentPage(image: ImageContent, related: RelatedContent) extends ContentPage {
override lazy val item: ImageContent = image
}
Loading