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

Tag pages in DCR #26149

Merged
merged 14 commits into from
Jun 5, 2023
Merged
Show file tree
Hide file tree
Changes from 10 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
6 changes: 4 additions & 2 deletions applications/app/controllers/AllIndexController.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,15 @@ package controllers

import com.gu.contentapi.client.model.ContentApiError
import common.Edition.defaultEdition
import common.{Edition, ImplicitControllerExecutionContext, GuLogging}
import common.{Edition, GuLogging, ImplicitControllerExecutionContext}
import contentapi.{ContentApiClient, SectionsLookUp}
import implicits.{Dates, ItemResponses}
import model.Cached.{RevalidatableResult, WithoutRevalidationResult}
import model._
import org.joda.time.format.DateTimeFormat
import org.joda.time.{DateTime, DateTimeZone}
import pages.AllIndexHtmlPage
import play.api.libs.ws.WSClient
import play.api.mvc._
import services.{ConfigAgent, IndexPage, IndexPageItem}
import views.support.PreviousAndNext
Expand All @@ -20,14 +21,15 @@ class AllIndexController(
contentApiClient: ContentApiClient,
sectionsLookUp: SectionsLookUp,
val controllerComponents: ControllerComponents,
val ws: WSClient,
)(implicit context: ApplicationContext)
extends BaseController
with ImplicitControllerExecutionContext
with ItemResponses
with Dates
with GuLogging {

private val indexController = new IndexController(contentApiClient, sectionsLookUp, controllerComponents)
private val indexController = new IndexController(contentApiClient, sectionsLookUp, controllerComponents, ws)

// no need to set the zone here, it gets it from the date.
private val dateFormatUTC = DateTimeFormat.forPattern("yyyy/MMM/dd").withZone(DateTimeZone.UTC)
Expand Down
54 changes: 44 additions & 10 deletions applications/app/controllers/IndexController.scala
Original file line number Diff line number Diff line change
Expand Up @@ -4,26 +4,60 @@ import common._
import contentapi.{ContentApiClient, SectionsLookUp}
import model.Cached.RevalidatableResult
import model._
import model.dotcomrendering.{DotcomTagFrontsRenderingDataModel, PageType}
import pages.IndexHtmlPage
import play.api.libs.ws.WSClient
import play.api.mvc.{ControllerComponents, RequestHeader, Result}
import renderers.DotcomRenderingService
import services.IndexPage
import services.dotcomrendering.{LocalRender, RemoteRender, TagFrontPicker}

import scala.concurrent.Future

class IndexController(
val contentApiClient: ContentApiClient,
val sectionsLookUp: SectionsLookUp,
val controllerComponents: ControllerComponents,
val ws: WSClient,
)(implicit val context: ApplicationContext)
extends IndexControllerCommon {
protected def renderFaciaFront(model: IndexPage)(implicit request: RequestHeader): Result = {
Cached(model.page) {
if (request.isRss) {
val body = TrailsToRss(model.page.metadata, model.trails)
RevalidatableResult(Ok(body).as("text/xml; charset=utf-8"), body)
} else if (request.isJson) {
JsonComponent(views.html.fragments.indexBody(model))
} else {
RevalidatableResult.Ok(IndexHtmlPage.html(model))
}

val remoteRenderer: DotcomRenderingService = DotcomRenderingService()

protected def renderFaciaFront(model: IndexPage)(implicit request: RequestHeader): Future[Result] = {
TagFrontPicker.getTier(model) match {
case RemoteRender =>
if (request.isJson) {
Future {
Cached(model.page) {
JsonComponent.fromWritable(
DotcomTagFrontsRenderingDataModel(
page = model,
request = request,
pageType = PageType(model, request, context),
),
)
}
}
} else
remoteRenderer.getTagFront(
ws = ws,
page = model,
pageType = PageType(model, request, context),
)(request)
case LocalRender =>
Future {
OllysCoding marked this conversation as resolved.
Show resolved Hide resolved
Cached(model.page) {
if (request.isRss) {
val body = TrailsToRss(model.page.metadata, model.trails)
RevalidatableResult(Ok(body).as("text/xml; charset=utf-8"), body)
} else if (request.isJson) {
JsonComponent(views.html.fragments.indexBody(model))
} else {
RevalidatableResult.Ok(IndexHtmlPage.html(model))
}
}
}
}
}
}
71 changes: 71 additions & 0 deletions applications/app/services/TagFrontPicker.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
package services.dotcomrendering

import common.GuLogging
import experiments.{ActiveExperiments}
import implicits.Requests._
import play.api.mvc.RequestHeader
import services.IndexPage

object TagFrontPicker extends GuLogging {

def getTier(tagFront: IndexPage)(implicit request: RequestHeader): RenderType = {
lazy val participatingInTest = false // There's no room for a 0% test at the moment - so we're just going with false
lazy val dcrCouldRender = false

val tier = decideTier(
request.isRss,
request.isJson,
request.forceDCROff,
request.forceDCR,
participatingInTest,
dcrCouldRender,
)

logTier(tagFront, participatingInTest, dcrCouldRender, Map(), tier)

tier
}

private def decideTier(
isRss: Boolean,
isJson: Boolean,
forceDCROff: Boolean,
forceDCR: Boolean,
participatingInTest: Boolean,
dcrCouldRender: Boolean,
): RenderType = {
if (isRss) LocalRender
else if (isJson) {
// JSON requests always require forceDCR
if (forceDCR) RemoteRender
else LocalRender
} else if (forceDCROff) LocalRender
else if (forceDCR) RemoteRender
else if (dcrCouldRender && participatingInTest) RemoteRender
else LocalRender
}

private def logTier(
tagFront: IndexPage,
participatingInTest: Boolean,
dcrCouldRender: Boolean,
checks: Map[String, Boolean],
tier: RenderType,
)(implicit request: RequestHeader): Unit = {
val tierReadable = if (tier == RemoteRender) "dotcomcomponents" else "web"
val checksToString = checks.map {
case (key, value) =>
(key, value.toString)
}
val properties =
Map(
"participatingInTest" -> participatingInTest.toString,
// "testPercentage" -> DCRTagFronts.participationGroup.percentage,
"dcrCouldRender" -> dcrCouldRender.toString,
"isFront" -> "true",
Copy link
Contributor

@ioannakok ioannakok Jun 5, 2023

Choose a reason for hiding this comment

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

Should this be renamed to sth like isTagFront so that these pages don't interfere with our current Grafana dashboards / queries?

image

"tier" -> tierReadable,
) ++ checksToString

DotcomFrontsLogger.logger.logRequest(s"tag front executing in $tierReadable", properties, tagFront)
}
}
12 changes: 10 additions & 2 deletions applications/test/AllIndexControllerTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import org.joda.time.DateTimeZone
import org.scalatest.flatspec.AnyFlatSpec
import org.scalatest.matchers.should.Matchers
import org.scalatest.{BeforeAndAfterAll, DoNotDiscover}
import play.api.libs.ws.WSClient
import org.scalatestplus.mockito.MockitoSugar
import play.api.test.Helpers._

@DoNotDiscover class AllIndexControllerTest
Expand All @@ -16,7 +18,8 @@ import play.api.test.Helpers._
with WithMaterializer
with WithTestWsClient
with WithTestContentApiClient
with WithTestApplicationContext {
with WithTestApplicationContext
with MockitoSugar {

private val PermanentRedirect = 301
private val TemporaryRedirect = 302
Expand Down Expand Up @@ -58,7 +61,12 @@ import play.api.test.Helpers._

lazy val sectionsLookUp = new SectionsLookUp(testContentApiClient)
lazy val allIndexController =
new AllIndexController(testContentApiClient, sectionsLookUp, play.api.test.Helpers.stubControllerComponents())
new AllIndexController(
testContentApiClient,
sectionsLookUp,
play.api.test.Helpers.stubControllerComponents(),
mock[WSClient],
)

it should "redirect dated tag pages to the equivalent /all page" in {
val result = allIndexController.on("football/series/thefiver/2014/jan/23")(TestRequest())
Expand Down
6 changes: 5 additions & 1 deletion applications/test/IndexControllerTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import org.scalatest.matchers.should.Matchers
import play.api.test._
import play.api.test.Helpers._
import org.scalatest.{BeforeAndAfterAll, DoNotDiscover}
import org.scalatestplus.mockito.MockitoSugar
import play.api.libs.ws.WSClient

@DoNotDiscover class IndexControllerTest
extends AnyFlatSpec
Expand All @@ -16,7 +18,8 @@ import org.scalatest.{BeforeAndAfterAll, DoNotDiscover}
with WithMaterializer
with WithTestWsClient
with WithTestApplicationContext
with WithTestContentApiClient {
with WithTestContentApiClient
with MockitoSugar {

val section = "books"
lazy val sectionsLookUp = new SectionsLookUp(testContentApiClient)
Expand All @@ -29,6 +32,7 @@ import org.scalatest.{BeforeAndAfterAll, DoNotDiscover}
testContentApiClient,
sectionsLookUp,
play.api.test.Helpers.stubControllerComponents(),
mock[WSClient],
)

"Index Controller" should "200 when content type is front" in {
Expand Down
6 changes: 5 additions & 1 deletion applications/test/IndexMetaDataTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@ import org.jsoup.Jsoup
import org.scalatest.flatspec.AnyFlatSpec
import org.scalatest.matchers.should.Matchers
import org.scalatest.{BeforeAndAfterAll, DoNotDiscover}
import org.scalatestplus.mockito.MockitoSugar
import play.api.libs.json._
import play.api.libs.ws.WSClient
import play.api.test.Helpers._

@DoNotDiscover class IndexMetaDataTest
Expand All @@ -18,7 +20,8 @@ import play.api.test.Helpers._
with WithMaterializer
with WithTestWsClient
with WithTestApplicationContext
with WithTestContentApiClient {
with WithTestContentApiClient
with MockitoSugar {

val articleUrl = "money/pensions"
val crosswordsUrl = "crosswords"
Expand All @@ -28,6 +31,7 @@ import play.api.test.Helpers._
testContentApiClient,
sectionsLookUp,
play.api.test.Helpers.stubControllerComponents(),
mock[WSClient],
)

it should "Include organisation metadata" in {
Expand Down
12 changes: 10 additions & 2 deletions applications/test/common/CombinerControllerTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import org.scalatest.flatspec.AnyFlatSpec
import play.api.test.Helpers._
import org.scalatest.{BeforeAndAfterAll, DoNotDiscover}
import org.scalatest.matchers.should.Matchers
import play.api.libs.ws.WSClient
import org.scalatestplus.mockito.MockitoSugar
import test.{
ConfiguredTestSuite,
TestRequest,
Expand All @@ -23,11 +25,17 @@ import test.{
with WithMaterializer
with WithTestWsClient
with WithTestApplicationContext
with WithTestContentApiClient {
with WithTestContentApiClient
with MockitoSugar {

lazy val sectionsLookUp = new SectionsLookUp(testContentApiClient)
lazy val indexController =
new IndexController(testContentApiClient, sectionsLookUp, play.api.test.Helpers.stubControllerComponents())
new IndexController(
testContentApiClient,
sectionsLookUp,
play.api.test.Helpers.stubControllerComponents(),
mock[WSClient],
)

"Combiner" should "404 when there is no content for 2 tags" in {
val result = indexController.renderCombiner("profile/grant-klopper", "tone/reviews")(TestRequest())
Expand Down
35 changes: 19 additions & 16 deletions common/app/controllers/IndexControllerCommon.scala
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ trait IndexControllerCommon
logGoogleBot(request)
index(leftSide, rightSide, inferPage(request), request.isRss).map {
OllysCoding marked this conversation as resolved.
Show resolved Hide resolved
case Right(page) => renderFaciaFront(page)
case Left(other) => other
}
case Left(other) => Future { other }
OllysCoding marked this conversation as resolved.
Show resolved Hide resolved
}.flatten
}

private def logGoogleBot(request: RequestHeader) = {
Expand Down Expand Up @@ -63,39 +63,42 @@ trait IndexControllerCommon
}
}

protected def renderFaciaFront(model: IndexPage)(implicit request: RequestHeader): Result
protected def renderFaciaFront(model: IndexPage)(implicit request: RequestHeader): Future[Result]

private def renderTrailsFragment(model: IndexPage)(implicit request: RequestHeader) = {
val response = () =>
views.html.fragments.trailblocks.headline(model.faciaTrails, numItemsVisible = model.trails.size)
renderFormat(response, response, model.page)
}

override def renderItem(path: String)(implicit request: RequestHeader): Future[Result] =
override def renderItem(path: String)(implicit request: RequestHeader): Future[Result] = {
path match {
//if this is a section tag e.g. football/football
case TagPattern(left, right) if left == right => successful(Cached(60)(redirect(left, request.isRss)))
case _ =>
logGoogleBot(request)
index(Edition(request), path, inferPage(request), request.isRss) map {
(index(Edition(request), path, inferPage(request), request.isRss) map {
// if no content is returned (as often happens with old/expired/migrated microsites) return 404 rather than an empty page
case Right(model) =>
if (model.contents.nonEmpty) renderFaciaFront(model)
else
Cached(60)(
WithoutRevalidationResult(
Gone(
views.html.gone(
gonePage,
"Sorry - there is no content here",
"This could be, for example, because content associated with it is not yet published, or due to legal reasons such as the expiry of our rights to publish the content.",
Future {
Cached(60)(
WithoutRevalidationResult(
Gone(
views.html.gone(
gonePage,
"Sorry - there is no content here",
"This could be, for example, because content associated with it is not yet published, or due to legal reasons such as the expiry of our rights to publish the content.",
),
),
),
),
)
case Left(other) => RenderOtherStatus(other)
}
)
}
case Left(other) => Future { RenderOtherStatus(other) }
}).flatten
}
}

override def canRender(item: ItemResponse): Boolean = item.section.orElse(item.tag).isDefined
}
Loading