From 2b89e457b682fdc4bbc170578d5c84e6ae39fb69 Mon Sep 17 00:00:00 2001 From: Mike Bryant Date: Fri, 23 Aug 2024 17:29:19 +0100 Subject: [PATCH] Tweak the Google Docs external page feature to support native titles The Docs title is used if no i18n one is defined --- modules/portal/app/assets/css/portal/_style.scss | 5 +++++ modules/portal/app/controllers/portal/Portal.scala | 10 ++++++---- .../app/services/htmlpages/GoogleDocsHtmlPages.scala | 9 ++++++--- modules/portal/app/services/htmlpages/HtmlPages.scala | 4 ++-- .../app/services/search/AkkaStreamsIndexMediator.scala | 6 +++--- modules/portal/conf/portal.routes | 2 +- test/integration/portal/PortalSpec.scala | 2 +- test/mockdata/package.scala | 2 +- test/services/htmlpages/MockHtmlPages.scala | 6 +++--- 9 files changed, 28 insertions(+), 18 deletions(-) diff --git a/modules/portal/app/assets/css/portal/_style.scss b/modules/portal/app/assets/css/portal/_style.scss index 0c525f9888..fa0db80258 100644 --- a/modules/portal/app/assets/css/portal/_style.scss +++ b/modules/portal/app/assets/css/portal/_style.scss @@ -2157,3 +2157,8 @@ section:last-child { @include make-container(); @include make-container-max-widths(); } + +// External page styling +.external-page table { + @extend .table, .table-bordered, .table-striped; +} diff --git a/modules/portal/app/controllers/portal/Portal.scala b/modules/portal/app/controllers/portal/Portal.scala index 1e6d74eef2..4d38e36ff5 100644 --- a/modules/portal/app/controllers/portal/Portal.scala +++ b/modules/portal/app/controllers/portal/Portal.scala @@ -185,10 +185,12 @@ case class Portal @Inject()( def externalPage(key: String): Action[AnyContent] = OptionalUserAction.async { implicit request => futurePageOr404 { htmlPages.get(key, noCache = request.getQueryString("noCache").isDefined).map { futureData => - futureData.map { case (css, html) => - val title = Messages(s"pages.external.$key.title") - val meta = Map("description" -> Messages(s"pages.external.$key.description")) - Ok(views.html.layout.textLayout(title, meta = meta, styles = css)(html)) + futureData.map { case (title, css, html) => + val titleKey = s"pages.external.$key.title" + val descKey = s"pages.external.$key.description" + val i18nTitle = if (Messages.isDefinedAt(titleKey)) Messages(titleKey) else title + val i18nMeta = Map("description" -> (if(Messages.isDefinedAt(descKey)) Messages(descKey) else "")) + Ok(views.html.layout.textLayout(i18nTitle, meta = i18nMeta, styles = css)(html)) } } } diff --git a/modules/portal/app/services/htmlpages/GoogleDocsHtmlPages.scala b/modules/portal/app/services/htmlpages/GoogleDocsHtmlPages.scala index 4b658be2d9..c1cc291d65 100644 --- a/modules/portal/app/services/htmlpages/GoogleDocsHtmlPages.scala +++ b/modules/portal/app/services/htmlpages/GoogleDocsHtmlPages.scala @@ -22,7 +22,7 @@ import scala.concurrent.{ExecutionContext, Future} case class GoogleDocsHtmlPages @Inject ()(ws: WSClient, config: play.api.Configuration)( implicit cache: AsyncCacheApi, executionContext: ExecutionContext) extends HtmlPages { - private def googleDocBody(url: String): Future[(Html, Html)] = { + private def googleDocBody(url: String): Future[(String, Html, Html)] = { ws.url(url).addQueryStringParameters( "e" -> "download", "exportFormat" -> "html", @@ -38,6 +38,9 @@ case class GoogleDocsHtmlPages @Inject ()(ws: WSClient, config: play.api.Configu import org.jsoup.Jsoup val doc = Jsoup.parse(r.body) + + val title = doc.title().replace(" - Google Docs", "") + val body = doc .body() .tagName("div") @@ -51,11 +54,11 @@ case class GoogleDocsHtmlPages @Inject ()(ws: WSClient, config: play.api.Configu body.select("div#banners").remove() body.select("div#footer").remove() - Html("") -> Html(body.outerHtml()) + (title, Html(""), Html(body.outerHtml())) } } - override def get(key: String, noCache: Boolean = false)(implicit messages: Messages): Option[Future[(Html, Html)]] = { + override def get(key: String, noCache: Boolean = false)(implicit messages: Messages): Option[Future[(String, Html, Html)]] = { def getUrl: Option[String] = config.getOptional[String](s"pages.external.google.$key.${messages.lang.code}") orElse config.getOptional[String](s"pages.external.google.$key.default") diff --git a/modules/portal/app/services/htmlpages/HtmlPages.scala b/modules/portal/app/services/htmlpages/HtmlPages.scala index 2b98d5a5d8..109496c4aa 100644 --- a/modules/portal/app/services/htmlpages/HtmlPages.scala +++ b/modules/portal/app/services/htmlpages/HtmlPages.scala @@ -17,7 +17,7 @@ trait HtmlPages { * @param key the identifier for this fragment * @param noCache whether to prevent the fragment coming from the cache * @param messages an implicit messages instance containing the current language - * @return An optional, future of Css -> Body HTML + * @return An optional, future of (Title, Css, Body HTML) */ - def get(key: String, noCache: Boolean = false)(implicit messages: Messages): Option[Future[(Html, Html)]] + def get(key: String, noCache: Boolean = false)(implicit messages: Messages): Option[Future[(String, Html, Html)]] } diff --git a/modules/portal/app/services/search/AkkaStreamsIndexMediator.scala b/modules/portal/app/services/search/AkkaStreamsIndexMediator.scala index 1de36cbb57..25f9b2d37a 100644 --- a/modules/portal/app/services/search/AkkaStreamsIndexMediator.scala +++ b/modules/portal/app/services/search/AkkaStreamsIndexMediator.scala @@ -45,7 +45,7 @@ case class AkkaStreamsIndexMediatorHandle( override def withChannel(actorRef: ActorRef, formatter: String => String, filter: Int => Boolean = _ % 100 == 0): AkkaStreamsIndexMediatorHandle = copy(chan = Some(actorRef), processFunc = formatter, progressFilter = filter) - import scala.collection.JavaConverters._ + import scala.jdk.CollectionConverters._ import scala.concurrent.duration._ private val logger = Logger(classOf[AkkaStreamsIndexMediator]) @@ -79,8 +79,8 @@ case class AkkaStreamsIndexMediatorHandle( } .via(httpSinkFlow) .collect { - case (Success(response), tag) => response - case (Failure(e), tag) => throw e + case (Success(response), _) => response + case (Failure(e), _) => throw e } .flatMapConcat(_.entity.withoutSizeLimit().dataBytes) .toMat(Sink.fold(ByteString.empty)(_ ++ _))(Keep.right) diff --git a/modules/portal/conf/portal.routes b/modules/portal/conf/portal.routes index 40d0202d24..adf9c7ccd3 100644 --- a/modules/portal/conf/portal.routes +++ b/modules/portal/conf/portal.routes @@ -15,7 +15,7 @@ GET /about @controllers.portal.Portal.abo GET /terms @controllers.portal.Portal.terms() GET /help/datamodel @controllers.portal.Portal.dataModel() -GET /help/$page<\w+> @controllers.portal.Portal.externalPage(page: String) +GET /help/$page<\w[\w-_]+> @controllers.portal.Portal.externalPage(page: String) # Change locale GET /prefs @controllers.portal.Portal.prefs() diff --git a/test/integration/portal/PortalSpec.scala b/test/integration/portal/PortalSpec.scala index c7a00f2960..417fb08b5e 100644 --- a/test/integration/portal/PortalSpec.scala +++ b/test/integration/portal/PortalSpec.scala @@ -164,7 +164,7 @@ class PortalSpec extends IntegrationTestRunner { "fetch external pages" in new ITestApp { val faq = FakeRequest(portalRoutes.externalPage("faq")).call() status(faq) must equalTo(OK) - contentAsString(faq) must contain(mockdata.externalPages("faq").toString()) + contentAsString(faq) must contain(mockdata.externalPages("faq")._3.toString()) } "return 404 for external pages with a malformed id (bug #635)" in new ITestApp { diff --git a/test/mockdata/package.scala b/test/mockdata/package.scala index 60fdedffc9..7b70420f14 100644 --- a/test/mockdata/package.scala +++ b/test/mockdata/package.scala @@ -14,7 +14,7 @@ package object mockdata { val yahooOpenId = OpenIDAssociation("linda", "https://yahoo.com/openid", Some(moderator)) val externalPages = Map( - "faq" -> Html("

FAQ

") + "faq" -> ("Faq", Html(""), Html("

FAQ

")) ) // Users... diff --git a/test/services/htmlpages/MockHtmlPages.scala b/test/services/htmlpages/MockHtmlPages.scala index 3014267804..497a1bf8ff 100644 --- a/test/services/htmlpages/MockHtmlPages.scala +++ b/test/services/htmlpages/MockHtmlPages.scala @@ -10,9 +10,9 @@ import scala.concurrent.Future * Mock external pages. */ case class MockHtmlPages() extends HtmlPages { - override def get(key: String, noCache: Boolean)(implicit messages: Messages): Option[Future[(Html, Html)]] = { - mockdata.externalPages.get(key).map { html => - Some(Future.successful(Html("") -> html)) + override def get(key: String, noCache: Boolean)(implicit messages: Messages): Option[Future[(String, Html, Html)]] = { + mockdata.externalPages.get(key).map { data => + Some(Future.successful(data)) }.getOrElse(throw new ItemNotFound()) } }