Merge pull request #25118 from guardian/jc/filter-capi-data-auto-filters

filter blog by topics on render article route

article/app/controllers/LiveBlogController.scala

@@ -19,7 +19,7 @@ import play.twirl.api.Html
 import renderers.DotcomRenderingService
 import services.CAPILookup
 import services.dotcomponents.DotcomponentsLogger
-import topmentions.{TopMentionsService}
+import topmentions.TopMentionsService
 import views.support.RenderOtherStatus
 import scala.concurrent.Future
 
@@ -45,7 +45,7 @@ class LiveBlogController(
 
   def renderEmail(path: String): Action[AnyContent] = {
     Action.async { implicit request =>
-      mapModel(path, ArticleBlocks) {
+      mapModel(path, ArticleBlocks, topMentionResult = None) {
         case (minute: MinutePage, _) =>
           Future.successful(common.renderEmail(ArticleEmailHtmlPage.html(minute), minute))
         case (blog: LiveBlogPage, _) => Future.successful(common.renderEmail(LiveBlogHtmlPage.html(blog), blog))
@@ -62,7 +62,7 @@ class LiveBlogController(
   ): Action[AnyContent] = {
     Action.async { implicit request =>
       val filter = shouldFilter(filterKeyEvents)
-      val topMentions = getTopMentionsByTopics(path, topics)
+      val topMentions = if (filter) None else getTopMentions(path, topics)
       val topicList = topMentionsService.getTopics(path)
       page.map(ParseBlockId.fromPageParam) match {
         case Some(ParsedBlockId(id)) =>
@@ -71,7 +71,13 @@ class LiveBlogController(
           Future.successful(
             Cached(10)(WithoutRevalidationResult(NotFound)),
           ) // page param there but couldn't extract a block id
-        case None => renderWithRange(path, CanonicalLiveBlog, filter, topMentions, topicList) // no page param
+        case None => {
+          topMentions match {
+            case Some(value) =>
+              renderWithRange(path, TopicsLiveBlog, filter, Some(value), topicList) // no page param
+            case None => renderWithRange(path, CanonicalLiveBlog, filter, None, topicList) // no page param
+          }
+        }
       }
     }
   }
@@ -87,7 +93,8 @@ class LiveBlogController(
     Action.async { implicit request: Request[AnyContent] =>
       val filter = shouldFilter(filterKeyEvents)
       val range = getRange(lastUpdate, page)
-      mapModel(path, range, filter) {
+
+      mapModel(path, range, filter, None) {
         case (blog: LiveBlogPage, _) if rendered.contains(false) => getJsonForFronts(blog)
         case (blog: LiveBlogPage, blocks) if request.forceDCR && lastUpdate.isEmpty =>
           Future.successful(renderGuuiJson(blog, blocks, filter))
@@ -112,7 +119,7 @@ class LiveBlogController(
   )(implicit
       request: RequestHeader,
   ): Future[Result] = {
-    mapModel(path, range, filterKeyEvents) { (page, blocks) =>
+    mapModel(path, range, filterKeyEvents, topMentionResult) { (page, blocks) =>
       {
         val isAmpSupported = page.article.content.shouldAmplify
         val pageType: PageType = PageType(page, request, context)
@@ -307,12 +314,17 @@ class LiveBlogController(
     common.renderJson(json, blog).as("application/json")
   }
 
-  private[this] def mapModel(path: String, range: BlockRange, filterKeyEvents: Boolean = false)(
+  private[this] def mapModel(
+      path: String,
+      range: BlockRange,
+      filterKeyEvents: Boolean = false,
+      topMentionResult: Option[TopMentionsResult],
+  )(
       render: (PageWithStoryPackage, Blocks) => Future[Result],
   )(implicit request: RequestHeader): Future[Result] = {
     capiLookup
       .lookup(path, Some(range))
-      .map(responseToModelOrResult(range, filterKeyEvents))
+      .map(responseToModelOrResult(range, filterKeyEvents, topMentionResult))
       .recover(convertApiExceptions)
       .flatMap {
         case Left((model, blocks)) => render(model, blocks)
@@ -323,6 +335,7 @@ class LiveBlogController(
   private[this] def responseToModelOrResult(
       range: BlockRange,
       filterKeyEvents: Boolean,
+      topMentionResult: Option[TopMentionsResult],
   )(response: ItemResponse)(implicit request: RequestHeader): Either[(PageWithStoryPackage, Blocks), Result] = {
     val supportedContent: Option[ContentType] = response.content.filter(isSupported).map(Content(_))
     val supportedContentResult: Either[ContentType, Result] = ModelOrResult(supportedContent, response)
@@ -339,6 +352,7 @@ class LiveBlogController(
           response,
           range,
           filterKeyEvents,
+          topMentionResult,
         ).left
           .map(_ -> blocks)
       case unknown =>
@@ -353,7 +367,7 @@ class LiveBlogController(
     filterKeyEvents.getOrElse(false)
   }
 
-  def getTopMentionsByTopics(blogId: String, topics: Option[String]) = {
+  def getTopMentions(blogId: String, topics: Option[String]) = {
     val topMentionsResult = for {
       topMentionTopic <- TopMentionsTopic.fromString(topics)
       topMentions <- topMentionsService.getTopMentionsByTopic(blogId, topMentionTopic)

article/app/model/LiveBlogHelpers.scala

@@ -1,14 +1,15 @@
 package model
 
 import com.gu.contentapi.client.model.v1.ItemResponse
+import common.GuLogging
 import common.`package`._
 import model.liveblog.BodyBlock
 import model.ParseBlockId.ParsedBlockId
 import org.joda.time.DateTime
 import play.api.libs.functional.syntax._
 import play.api.libs.json.{JsValue, Json, _}
 
-object LiveBlogHelpers {
+object LiveBlogHelpers extends GuLogging {
 
   // Get a Seq[BodyBlock] given an article and the "page" request parameter on live-blog pages.
 
@@ -19,7 +20,7 @@ object LiveBlogHelpers {
   ): Seq[BodyBlock] = {
 
     def modelWithRange(range: BlockRange) =
-      LiveBlogHelpers.createLiveBlogModel(article, range, filterKeyEvents)
+      LiveBlogHelpers.createLiveBlogModel(article, range, filterKeyEvents, None)
 
     val lbcp = param.map(ParseBlockId.fromPageParam) match {
       case Some(ParsedBlockId(id)) => modelWithRange(PageWithBlock(id))
@@ -39,6 +40,7 @@ object LiveBlogHelpers {
       liveBlog: Article,
       range: BlockRange,
       filterKeyEvents: Boolean,
+      topMentionResult: Option[TopMentionsResult],
   ): Option[LiveBlogCurrentPage] = {
 
     val pageSize = if (liveBlog.content.tags.tags.map(_.id).contains("sport/sport")) 30 else 10
@@ -49,6 +51,7 @@ object LiveBlogHelpers {
         _,
         range,
         filterKeyEvents,
+        topMentionResult,
       ),
     )
 
@@ -61,6 +64,7 @@ object LiveBlogHelpers {
       response: ItemResponse,
       range: BlockRange,
       filterKeyEvents: Boolean,
+      topMentionResult: Option[TopMentionsResult],
   ): Either[LiveBlogPage, Status] = {
 
     val pageSize = if (liveBlog.content.tags.tags.map(_.id).contains("sport/sport")) 30 else 10
@@ -72,6 +76,7 @@ object LiveBlogHelpers {
           blocks,
           range,
           filterKeyEvents,
+          topMentionResult,
         )
       } getOrElse None
 

article/test/DCRFake.scala

@@ -8,10 +8,15 @@ import play.api.libs.ws.WSClient
 import play.api.mvc.{RequestHeader, Result}
 import play.twirl.api.Html
 
+import scala.collection.mutable
+import scala.collection.mutable.Queue
 import scala.concurrent.{ExecutionContext, Future}
 
 // It is always a mistake to rely on actual DCR output for tests.
 class DCRFake(implicit context: ApplicationContext) extends renderers.DotcomRenderingService {
+
+  val requestedBlogs: Queue[PageWithStoryPackage] = new Queue[PageWithStoryPackage]()
+
   override def getArticle(
       ws: WSClient,
       article: PageWithStoryPackage,
@@ -22,6 +27,7 @@ class DCRFake(implicit context: ApplicationContext) extends renderers.DotcomRend
       topics: Option[Seq[TopicWithCount]],
   )(implicit request: RequestHeader): Future[Result] = {
     implicit val ec = ExecutionContext.global
+    requestedBlogs.enqueue(article)
     Future(
       Cached(article)(RevalidatableResult.Ok(Html("FakeRemoteRender has found you out if you rely on this markup!"))),
     )

article/test/LiveBlogControllerTest.scala

@@ -2,14 +2,14 @@ package test
 
 import controllers.LiveBlogController
 import org.mockito.Mockito._
-import org.mockito.Matchers.any
+import org.mockito.Matchers.{any, anyObject, anyString}
 import org.scalatest.flatspec.AnyFlatSpec
 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 model.{TopMentionsResult, TopMentionsTopic, TopMentionsTopicType}
+import model.{LiveBlogPage, TopMentionsResult, TopMentionsTopic, TopMentionsTopicType, TopicsLiveBlog}
 import topmentions.{TopMentionsS3Client, TopMentionsService}
 
 import scala.concurrent.Future
@@ -28,28 +28,38 @@ import scala.concurrent.Future
   val liveBlogUrl = "global/middle-east-live/2013/sep/09/syria-crisis-russia-kerry-us-live"
   val path = "/football/live/2016/feb/26/fifa-election-who-will-succeed-sepp-blatter-president-live"
 
-  val fakeTopMentionsService = mock[TopMentionsService]
+  var fakeTopMentionsService = mock[TopMentionsService]
   val topMentionResult = TopMentionsResult(
-    name = "nhs",
+    name = "Fifa",
     `type` = TopMentionsTopicType.Org,
-    blocks = Seq("blockId1"),
+    blocks = Seq("56d02bd2e4b0d38537b1f5fa"),
     count = 1,
     percentage_blocks = 1.2f,
   )
-  when(
-    fakeTopMentionsService.getTopMentionsByTopic(path, TopMentionsTopic(TopMentionsTopicType.Org, "nhs")),
-  ) thenReturn Some(
-    topMentionResult,
-  )
 
+  var fakeDcr = new DCRFake()
   lazy val liveBlogController = new LiveBlogController(
     testContentApiClient,
     play.api.test.Helpers.stubControllerComponents(),
     wsClient,
-    new DCRFake(),
+    fakeDcr,
     fakeTopMentionsService,
   )
 
+  override def beforeAll(): Unit = {
+    // This is to assure on every test we have a fresh instance of DCR
+    // and requestedBlogs is only having the calls for the relevant test
+    fakeDcr = new DCRFake()
+
+    // This is to assure on every test we have a fresh instance of TopicService
+    fakeTopMentionsService = mock[TopMentionsService]
+    when(
+      fakeTopMentionsService.getTopMentionsByTopic(path, TopMentionsTopic(TopMentionsTopicType.Org, "Fifa")),
+    ) thenReturn Some(
+      topMentionResult,
+    )
+  }
+
   it should "return the latest blocks of a live blog" in {
     val lastUpdateBlock = "block-56d03169e4b074a9f6b35baa"
     val fakeRequest = FakeRequest(
@@ -82,7 +92,7 @@ import scala.concurrent.Future
 
   }
 
-  it should "return the latest blocks of a live blog using DCR" in {
+  "renderJson" should "return the latest blocks of a live blog using DCR" in {
     val lastUpdateBlock = "block-56d03169e4b074a9f6b35baa"
     val fakeRequest = FakeRequest(
       GET,
@@ -229,15 +239,58 @@ import scala.concurrent.Future
     liveBlogController.shouldFilter(None) should be(false)
   }
 
-  "getTopMentionsForFilters" should "return none given no automatic filter query parameter" in {
-    liveBlogController.getTopMentionsByTopics(path, None) should be(None)
+  "getTopMentionsForFilters" should "returns none given no automatic filter query parameter" in {
+    liveBlogController.getTopMentions(path, None) should be(None)
   }
 
-  "getTopMentionsForFilters" should "return none given an incorrect automatic filter query parameter" in {
-    liveBlogController.getTopMentionsByTopics(path, Some("orgnhs")) should be(None)
+  "getTopMentionsForFilters" should "returns none given an incorrect automatic filter query parameter" in {
+    liveBlogController.getTopMentions(path, Some("orgFifa")) should be(None)
   }
 
-  "getTopMentionsForFilters" should "return correct topMentionResult given a correct automatic filter query parameter" in {
-    liveBlogController.getTopMentionsByTopics(path, Some("org:nhs")) should be(Some(topMentionResult))
+  "getTopMentionsForFilters" should "returns correct topMentionResult given a correct automatic filter query parameter" in {
+    liveBlogController.getTopMentions(path, Some("org:Fifa")) should be(Some(topMentionResult))
+  }
+
+  "renderArticle" should "returns the first page of filtered blog by topics" in {
+    val fakeRequest = FakeRequest(
+      GET,
+      s"${path}",
+    ).withHeaders("host" -> "localhost:9000")
+
+    val result = liveBlogController.renderArticle(
+      path,
+      page = None,
+      filterKeyEvents = Some(false),
+      topics = Some("org:Fifa"),
+    )(fakeRequest)
+
+    status(result) should be(200)
+    assertDcrCalledForLiveBlogWithBlocks(expectedBlocks = Seq("56d02bd2e4b0d38537b1f5fa"))
+  }
+
+  "renderArticle" should "doesn't call getTopMentionsByTopic given filterKeyEvents and topics query params are provided" in {
+    reset(fakeTopMentionsService)
+    val fakeRequest = FakeRequest(GET, s"${path}").withHeaders("host" -> "localhost:9000")
+
+    val result = liveBlogController.renderArticle(
+      path,
+      page = None,
+      filterKeyEvents = Some(true),
+      topics = Some("org:Fifa"),
+    )(fakeRequest)
+
+    verify(fakeTopMentionsService, times(0)).getTopMentionsByTopic(anyString(), anyObject())
+    status(result) should be(200)
+  }
+
+  private def assertDcrCalledForLiveBlogWithBlocks(expectedBlocks: Seq[String]) = {
+    val liveblog = fakeDcr.requestedBlogs.dequeue()
+
+    liveblog match {
+      case LiveBlogPage(_, currentPage, _, _) => {
+        currentPage.currentPage.blocks.map(_.id) should be(expectedBlocks)
+      }
+      case _ => fail("DCR was not called with a LiveBlogPage")
+    }
   }
 }