From 46b6cf6cd123fbf2d3fd56afd771f3676ee7b3ec Mon Sep 17 00:00:00 2001
From: Jamie B <53781962+JamieB-gu@users.noreply.github.com>
Date: Tue, 3 Dec 2024 17:28:38 +0000
Subject: [PATCH 1/3] Add test summary to CI

Updated our test config to output test summary files. Added a step to the `build` workflow in GitHub actions to read from these files and output a summary in the GitHub UI.
---
 .github/workflows/build.yml   | 6 ++++++
 .gitignore                    | 1 +
 project/ProjectSettings.scala | 3 +--
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index 0668a2e5e7df..3ec4a2059e49 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -50,6 +50,12 @@ jobs:
           -DAPP_SECRET="fake_secret" \
           -Duser.timezone=Australia/Sydney \
           -jar ./bin/sbt-launch.jar clean compile assets scalafmtCheckAll test Universal/packageBin
+      
+      - name: Test Summary
+        uses: test-summary/action@v2
+        with:
+          paths: "test-results/**/TEST-*.xml"
+        if: always()
 
       - uses: guardian/actions-riff-raff@v4
         env:
diff --git a/.gitignore b/.gitignore
index 6fbf8f6de5cd..725f9848c0dc 100644
--- a/.gitignore
+++ b/.gitignore
@@ -94,3 +94,4 @@ static/src/stylesheets/pasteup/.npmrc
 metals.sbt
 
 .java-version
+test-results/
diff --git a/project/ProjectSettings.scala b/project/ProjectSettings.scala
index f40137aa9a31..f906b47f73db 100644
--- a/project/ProjectSettings.scala
+++ b/project/ProjectSettings.scala
@@ -55,8 +55,7 @@ object ProjectSettings {
   def testStage = if (isCi) "DEVINFRA" else "LOCALTEST"
 
   val frontendTestSettings = Seq(
-    // Use ScalaTest https://groups.google.com/d/topic/play-framework/rZBfNoGtC0M/discussion
-    Test / testOptions := Nil,
+    Test / testOptions += Tests.Argument(TestFrameworks.ScalaTest, "-u", s"test-results/scala-${scalaVersion.value}", "-o"),
     concurrentRestrictions in Global := List(Tags.limit(Tags.Test, 4)),
     // Copy unit test resources https://groups.google.com/d/topic/play-framework/XD3X6R-s5Mc/discussion
     Test / unmanagedClasspath += (baseDirectory map { bd => Attributed.blank(bd / "test") }).value,

From dcd3e5cd74c6f9db83c65b4e3348b9a80f17ecca Mon Sep 17 00:00:00 2001
From: Jamie B <53781962+JamieB-gu@users.noreply.github.com>
Date: Wed, 27 Nov 2024 12:56:17 +0000
Subject: [PATCH 2/3] Use `JsValue` not `String` for DCAR

Frontend posts the data for articles, fronts, tags pages and so on to DCAR as JSON, using `WSClient`. At the moment the following steps are taken to transform from an instance of a case class in frontend to the format that `WSClient` uses (`BodyWritable`):

```
case class instance -> JsValue -> String -> BodyWritable
```

The final transformation here, `String -> BodyWritable`, is handled by `WSClient`, as it defines an implicit way to get from one type to the other. However, it also defines a way to do this for a `play-json` `JsValue`, so we do not need to convert to a `String` beforehand. We should be able to pass a `JsValue` to `WSClient` directly, removing one of the transformation steps:

```
case class instance -> JsValue -> BodyWritable
```

The way to achieve this is by modifying our methods that use `WSClient.post` to take a `JsValue` payload instead of a `String`. We are then able to update the `toJson` methods on each of our models to remove the `Json.stringify` step. However, aside from `RemoteRender` requests, these `toJson` methods are also used to provide for `JsonFormat` requests via the `renderJson` method, which at the moment requires a `String`. This method, though, makes use of Play's `Writeable`, which also includes an implicit transformation to get from a `play-json` `JsValue`. So we can update it to take a `JsValue` instead, and save the additional `String` transformation step here too.

Note that there is one exception where we're using `circe` instead of `play-json`, for crosswords, so in that case we're transforming from one to the other via `String`.
---
 .../app/controllers/CrosswordsController.scala   |  3 ++-
 .../app/controllers/GalleryController.scala      |  3 ++-
 .../app/controllers/ImageContentController.scala |  2 +-
 .../app/controllers/MediaController.scala        |  4 ++--
 article/app/controllers/ArticleController.scala  |  4 ++--
 common/app/common/package.scala                  |  7 +++++--
 .../DotcomBlocksRenderingDataModel.scala         |  4 ++--
 .../DotcomFrontsRenderingDataModel.scala         |  4 ++--
 ...DotcomNewslettersPageRenderingDataModel.scala |  4 ++--
 .../DotcomRenderingDataModel.scala               |  4 ++--
 .../DotcomTagPagesRenderingDataModel.scala       |  4 ++--
 .../EditionsCrosswordRenderingDataModel.scala    | 16 +++++++++-------
 .../app/renderers/DotcomRenderingService.scala   |  5 +++--
 13 files changed, 36 insertions(+), 28 deletions(-)

diff --git a/applications/app/controllers/CrosswordsController.scala b/applications/app/controllers/CrosswordsController.scala
index 48c16256bec4..b48801e34f05 100644
--- a/applications/app/controllers/CrosswordsController.scala
+++ b/applications/app/controllers/CrosswordsController.scala
@@ -23,6 +23,7 @@ import org.joda.time.{DateTime, LocalDate}
 import pages.{CrosswordHtmlPage, IndexHtmlPage, PrintableCrosswordHtmlPage}
 import play.api.data.Forms._
 import play.api.data._
+import play.api.libs.json.JsValue
 import play.api.libs.ws.WSClient
 import play.api.mvc._
 import renderers.DotcomRenderingService
@@ -117,7 +118,7 @@ class CrosswordPageController(
   }
   private def getDCRJson(crosswordPage: CrosswordPageWithContent, pageType: PageType)(implicit
       request: RequestHeader,
-  ): String =
+  ): JsValue =
     DotcomRenderingDataModel.toJson(
       DotcomRenderingDataModel.forCrossword(crosswordPage, request, pageType),
     )
diff --git a/applications/app/controllers/GalleryController.scala b/applications/app/controllers/GalleryController.scala
index fc749a3ab4aa..65fa2b76e6c3 100644
--- a/applications/app/controllers/GalleryController.scala
+++ b/applications/app/controllers/GalleryController.scala
@@ -7,6 +7,7 @@ import contentapi.ContentApiClient
 import model._
 import model.dotcomrendering.{DotcomRenderingDataModel, PageType}
 import pages.GalleryHtmlPage
+import play.api.libs.json.JsValue
 import play.api.libs.ws.WSClient
 import play.api.mvc._
 import play.twirl.api.Html
@@ -74,7 +75,7 @@ class GalleryController(
 
   private def getDCRJson(galleryPage: GalleryPage, pageType: PageType, blocks: Blocks)(implicit
       request: RequestHeader,
-  ): String = {
+  ): JsValue = {
     DotcomRenderingDataModel.toJson(DotcomRenderingDataModel.forGallery(galleryPage, request, pageType, blocks))
   }
 
diff --git a/applications/app/controllers/ImageContentController.scala b/applications/app/controllers/ImageContentController.scala
index daa7310efcf9..278104e22461 100644
--- a/applications/app/controllers/ImageContentController.scala
+++ b/applications/app/controllers/ImageContentController.scala
@@ -61,7 +61,7 @@ class ImageContentController(
 
   private def getDCRJson(content: ImageContentPage, pageType: PageType, mainBlock: Option[Block])(implicit
       request: RequestHeader,
-  ): String = {
+  ): JsValue = {
     DotcomRenderingDataModel.toJson(DotcomRenderingDataModel.forImageContent(content, request, pageType, mainBlock))
   }
 
diff --git a/applications/app/controllers/MediaController.scala b/applications/app/controllers/MediaController.scala
index 6d455b3af789..2e096897844f 100644
--- a/applications/app/controllers/MediaController.scala
+++ b/applications/app/controllers/MediaController.scala
@@ -9,7 +9,7 @@ import implicits.{AppsFormat, JsonFormat}
 import model._
 import model.dotcomrendering.{DotcomRenderingDataModel, PageType}
 import pages.ContentHtmlPage
-import play.api.libs.json.{Format, JsObject, Json}
+import play.api.libs.json.{Format, JsObject, Json, JsValue}
 import play.api.libs.ws.WSClient
 import play.api.mvc._
 import renderers.DotcomRenderingService
@@ -96,7 +96,7 @@ class MediaController(
 
   private def getDCRJson(content: MediaPage, pageType: PageType, blocks: Blocks)(implicit
       request: RequestHeader,
-  ): String = {
+  ): JsValue = {
     DotcomRenderingDataModel.toJson(DotcomRenderingDataModel.forMedia(content, request, pageType, blocks))
   }
 
diff --git a/article/app/controllers/ArticleController.scala b/article/app/controllers/ArticleController.scala
index 1ea08a3eb076..6ac328f52364 100644
--- a/article/app/controllers/ArticleController.scala
+++ b/article/app/controllers/ArticleController.scala
@@ -8,7 +8,7 @@ import model.Cached.{RevalidatableResult, WithoutRevalidationResult}
 import model.dotcomrendering.{DotcomRenderingDataModel, PageType}
 import model._
 import pages.{ArticleEmailHtmlPage, ArticleHtmlPage}
-import play.api.libs.json.Json
+import play.api.libs.json.{Json, JsValue}
 import play.api.libs.ws.WSClient
 import play.api.mvc._
 import renderers.DotcomRenderingService
@@ -91,7 +91,7 @@ class ArticleController(
 
   /** Returns a JSON representation of the payload that's sent to DCR when rendering the Article.
     */
-  private def getDCRJson(article: ArticlePage, blocks: Blocks)(implicit request: RequestHeader): String = {
+  private def getDCRJson(article: ArticlePage, blocks: Blocks)(implicit request: RequestHeader): JsValue = {
     val pageType: PageType = PageType(article, request, context)
     val newsletter = newsletterService.getNewsletterForArticle(article)
 
diff --git a/common/app/common/package.scala b/common/app/common/package.scala
index 51f51aeb4341..dc4ea81434e4 100644
--- a/common/app/common/package.scala
+++ b/common/app/common/package.scala
@@ -10,7 +10,7 @@ import model.CacheTime.RecentlyUpdated
 import model.Cached.RevalidatableResult
 import model.{ApplicationContext, Cached, NoCache}
 import play.api.Logger
-import play.api.libs.json.{JsObject, JsString}
+import play.api.libs.json.{JsObject, JsString, JsValue}
 import play.api.mvc.{RequestHeader, Result}
 import play.twirl.api.Html
 import model.ApplicationContext
@@ -162,7 +162,10 @@ object `package`
       JsonComponent(page, json)
     }
 
-  def renderJson(json: String, page: model.Page)(implicit request: RequestHeader, context: ApplicationContext): Result =
+  def renderJson(json: JsValue, page: model.Page)(implicit
+      request: RequestHeader,
+      context: ApplicationContext,
+  ): Result =
     Cached(page) {
       RevalidatableResult.Ok(json)
     }
diff --git a/common/app/model/dotcomrendering/DotcomBlocksRenderingDataModel.scala b/common/app/model/dotcomrendering/DotcomBlocksRenderingDataModel.scala
index 6edc08e8aa59..edba663ef04a 100644
--- a/common/app/model/dotcomrendering/DotcomBlocksRenderingDataModel.scala
+++ b/common/app/model/dotcomrendering/DotcomBlocksRenderingDataModel.scala
@@ -58,9 +58,9 @@ object DotcomBlocksRenderingDataModel {
     }
   }
 
-  def toJson(model: DotcomBlocksRenderingDataModel): String = {
+  def toJson(model: DotcomBlocksRenderingDataModel): JsValue = {
     val jsValue = Json.toJson(model)
-    Json.stringify(DotcomRenderingUtils.withoutNull(jsValue))
+    DotcomRenderingUtils.withoutNull(jsValue)
   }
 
   def apply(
diff --git a/common/app/model/dotcomrendering/DotcomFrontsRenderingDataModel.scala b/common/app/model/dotcomrendering/DotcomFrontsRenderingDataModel.scala
index dbec33a6051b..d5fc57c7206d 100644
--- a/common/app/model/dotcomrendering/DotcomFrontsRenderingDataModel.scala
+++ b/common/app/model/dotcomrendering/DotcomFrontsRenderingDataModel.scala
@@ -98,8 +98,8 @@ object DotcomFrontsRenderingDataModel {
     )
   }
 
-  def toJson(model: DotcomFrontsRenderingDataModel): String = {
+  def toJson(model: DotcomFrontsRenderingDataModel): JsValue = {
     val jsValue = Json.toJson(model)
-    Json.stringify(DotcomRenderingUtils.withoutNull(jsValue))
+    DotcomRenderingUtils.withoutNull(jsValue)
   }
 }
diff --git a/common/app/model/dotcomrendering/DotcomNewslettersPageRenderingDataModel.scala b/common/app/model/dotcomrendering/DotcomNewslettersPageRenderingDataModel.scala
index 0754e2159071..46256ce4ddae 100644
--- a/common/app/model/dotcomrendering/DotcomNewslettersPageRenderingDataModel.scala
+++ b/common/app/model/dotcomrendering/DotcomNewslettersPageRenderingDataModel.scala
@@ -97,9 +97,9 @@ object DotcomNewslettersPageRenderingDataModel {
     )
   }
 
-  def toJson(model: DotcomNewslettersPageRenderingDataModel): String = {
+  def toJson(model: DotcomNewslettersPageRenderingDataModel): JsValue = {
     val jsValue = Json.toJson(model)
-    Json.stringify(DotcomRenderingUtils.withoutNull(jsValue))
+    DotcomRenderingUtils.withoutNull(jsValue)
   }
 
   private def convertNewsletterResponseToData(response: NewsletterResponseV2): NewsletterData = {
diff --git a/common/app/model/dotcomrendering/DotcomRenderingDataModel.scala b/common/app/model/dotcomrendering/DotcomRenderingDataModel.scala
index b8771ad55e77..cce1327fef43 100644
--- a/common/app/model/dotcomrendering/DotcomRenderingDataModel.scala
+++ b/common/app/model/dotcomrendering/DotcomRenderingDataModel.scala
@@ -191,9 +191,9 @@ object DotcomRenderingDataModel {
     }
   }
 
-  def toJson(model: DotcomRenderingDataModel): String = {
+  def toJson(model: DotcomRenderingDataModel): JsValue = {
     val jsValue = Json.toJson(model)
-    Json.stringify(withoutNull(jsValue))
+    withoutNull(jsValue)
   }
 
   def forInteractive(
diff --git a/common/app/model/dotcomrendering/DotcomTagPagesRenderingDataModel.scala b/common/app/model/dotcomrendering/DotcomTagPagesRenderingDataModel.scala
index 4a76a7bf9761..2cf5dc72cf7d 100644
--- a/common/app/model/dotcomrendering/DotcomTagPagesRenderingDataModel.scala
+++ b/common/app/model/dotcomrendering/DotcomTagPagesRenderingDataModel.scala
@@ -132,8 +132,8 @@ object DotcomTagPagesRenderingDataModel {
     )
   }
 
-  def toJson(model: DotcomTagPagesRenderingDataModel): String = {
+  def toJson(model: DotcomTagPagesRenderingDataModel): JsValue = {
     val jsValue = Json.toJson(model)
-    Json.stringify(DotcomRenderingUtils.withoutNull(jsValue))
+    DotcomRenderingUtils.withoutNull(jsValue)
   }
 }
diff --git a/common/app/model/dotcomrendering/pageElements/EditionsCrosswordRenderingDataModel.scala b/common/app/model/dotcomrendering/pageElements/EditionsCrosswordRenderingDataModel.scala
index 92cfff9ec8a3..7507961520ea 100644
--- a/common/app/model/dotcomrendering/pageElements/EditionsCrosswordRenderingDataModel.scala
+++ b/common/app/model/dotcomrendering/pageElements/EditionsCrosswordRenderingDataModel.scala
@@ -2,8 +2,9 @@ package model.dotcomrendering.pageElements
 
 import com.gu.contentapi.client.model.v1.Crossword
 import com.gu.contentapi.json.CirceEncoders._
-import io.circe.JsonObject
 import io.circe.syntax._
+import model.dotcomrendering.DotcomRenderingUtils
+import play.api.libs.json.{JsObject, Json, JsValue}
 
 case class EditionsCrosswordRenderingDataModel(
     quick: Crossword,
@@ -11,10 +12,11 @@ case class EditionsCrosswordRenderingDataModel(
 )
 
 object EditionsCrosswordRenderingDataModel {
-  def toJson(model: EditionsCrosswordRenderingDataModel): String = {
-    JsonObject(
-      "quick" -> model.quick.asJson.dropNullValues,
-      "cryptic" -> model.cryptic.asJson.dropNullValues,
-    ).asJson.dropNullValues.noSpaces
-  }
+  def toJson(model: EditionsCrosswordRenderingDataModel): JsValue =
+    DotcomRenderingUtils.withoutNull(
+      Json.obj(
+        "quick" -> Json.parse(model.quick.asJson.toString()),
+        "cryptic" -> Json.parse(model.cryptic.asJson.toString()),
+      ),
+    )
 }
diff --git a/common/app/renderers/DotcomRenderingService.scala b/common/app/renderers/DotcomRenderingService.scala
index 32d7e067a59a..14d2ed7c87d3 100644
--- a/common/app/renderers/DotcomRenderingService.scala
+++ b/common/app/renderers/DotcomRenderingService.scala
@@ -25,6 +25,7 @@ import model.{
   RelatedContentItem,
   SimplePage,
 }
+import play.api.libs.json.JsValue
 import play.api.libs.ws.{WSClient, WSResponse}
 import play.api.mvc.Results.{InternalServerError, NotFound}
 import play.api.mvc.{RequestHeader, Result}
@@ -56,7 +57,7 @@ class DotcomRenderingService extends GuLogging with ResultWithPreconnectPreload
 
   private[this] def postWithoutHandler(
       ws: WSClient,
-      payload: String,
+      payload: JsValue,
       endpoint: String,
       timeout: Duration = Configuration.rendering.timeout,
   )(implicit request: RequestHeader): Future[WSResponse] = {
@@ -93,7 +94,7 @@ class DotcomRenderingService extends GuLogging with ResultWithPreconnectPreload
 
   private[this] def post(
       ws: WSClient,
-      payload: String,
+      payload: JsValue,
       endpoint: String,
       cacheTime: CacheTime,
       timeout: Duration = Configuration.rendering.timeout,

From aa37687680a6fa0371fbe109a62a5d4dbb93f531 Mon Sep 17 00:00:00 2001
From: Jamie B <53781962+JamieB-gu@users.noreply.github.com>
Date: Tue, 3 Dec 2024 18:01:55 +0000
Subject: [PATCH 3/3] Fix test to use `JsValue` not `String` for payload

---
 common/test/renderers/DotcomRenderingServiceTest.scala | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/common/test/renderers/DotcomRenderingServiceTest.scala b/common/test/renderers/DotcomRenderingServiceTest.scala
index 7e3d2f4477c9..6f4fd2158cbe 100644
--- a/common/test/renderers/DotcomRenderingServiceTest.scala
+++ b/common/test/renderers/DotcomRenderingServiceTest.scala
@@ -6,6 +6,7 @@ import org.scalatest.{BeforeAndAfterAll, DoNotDiscover, PrivateMethodTester}
 import org.scalatest.matchers.should.Matchers
 import test.{ConfiguredTestSuite, TestRequest, WithMaterializer, WithTestWsClient}
 import play.api.libs.ws.{WSClient, WSRequest, WSResponse}
+import play.api.libs.json.{JsString}
 import play.api.mvc.{Result}
 import conf.Configuration
 import org.mockito.Matchers.any
@@ -35,7 +36,7 @@ import org.scalatest.flatspec.AnyFlatSpec
   private val wsRequestMock = mock[WSRequest]
 
   "post" should "return a 404 for DCR 415 errors" in {
-    val payload = "payload"
+    val payload = JsString("payload")
     when(wsMock.url(any[String])).thenReturn(wsRequestMock)
     when(wsRequestMock.withRequestTimeout(any())).thenReturn(wsRequestMock)
     when(wsRequestMock.addHttpHeaders(any())).thenReturn(wsRequestMock)