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.

Sign up for GitHub

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

Ensure All Newsletters page is async #26873

Merged
merged 2 commits into from
Feb 5, 2024
Merged

Ensure All Newsletters page is async #26873

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
36b171d
refactor: make newsletters page async incl futures and swap DCR endpo…
cemms1 Feb 5, 2024
464cf02
keep email newsletters page using facia baseurl
cemms1 Feb 5, 2024
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
75 changes: 34 additions & 41 deletions applications/app/controllers/SignupPageController.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package controllers

import common.{GuLogging, ImplicitControllerExecutionContext}
import conf.switches.Switches.{UseDcrNewslettersPage}
import conf.switches.Switches.UseDcrNewslettersPage
import model.{ApplicationContext, Cached, NoCache}
import model.Cached.RevalidatableResult
import pages.NewsletterHtmlPage
Expand All @@ -13,12 +13,12 @@ import services.newsletters.GroupedNewslettersResponse.GroupedNewslettersRespons
import services.newsletters.NewsletterSignupAgent
import services.newsletters.model.NewsletterResponseV2
import staticpages.StaticPages
import implicits.{HtmlFormat, JsonFormat}
import implicits.Requests.RichRequestHeader

import scala.concurrent.{Await, ExecutionContext, Future}
import scala.concurrent.{ExecutionContext, Future}
import scala.concurrent.duration._
import model.dotcomrendering.DotcomNewslettersPageRenderingDataModel
import model.SimplePage
import model.CacheTime

class SignupPageController(
Expand All @@ -36,61 +36,48 @@ class SignupPageController(

private def localRenderNewslettersPage()(implicit
request: RequestHeader,
): Result = {
): Future[Result] = {
val groupedNewsletters: Either[String, GroupedNewslettersResponse] =
newsletterSignupAgent.getGroupedNewsletters()

groupedNewsletters match {
case Right(groupedNewsletters) =>
Cached(defaultCacheDuration)(
RevalidatableResult.Ok(
NewsletterHtmlPage.html(StaticPages.simpleNewslettersPage(request.path, groupedNewsletters)),
Future.successful(
Cached(defaultCacheDuration)(
RevalidatableResult.Ok(
NewsletterHtmlPage.html(StaticPages.simpleNewslettersPage(request.path, groupedNewsletters)),
),
),
)
case Left(e) =>
log.error(s"API call to get newsletters failed: $e")
NoCache(InternalServerError)
Future(NoCache(InternalServerError))
}
}

private def remoteRenderNewslettersPage()(implicit
request: RequestHeader,
): Result = {
): Future[Result] = {

val newsletters: Either[String, List[NewsletterResponseV2]] =
newsletterSignupAgent.getV2Newsletters()

newsletters match {
case Right(newsletters) =>
Await.result(
remoteRenderer.getEmailNewsletters(
ws = wsClient,
newsletters = newsletters,
page = StaticPages.dcrSimpleNewsletterPage(request.path),
),
3.seconds,
Comment on lines -64 to -70
Copy link
Contributor Author

@cemms1 cemms1 Feb 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think the presence of this extra Await could be the reason why the DCR circuit breaker keeps opening. The DotcomRenderingService has a timeout of 2 seconds so looks like this could be potentially blocking operations for one second after the DCR timeout and before the Await atMost duration of 3 seconds?

remoteRenderer.getEmailNewsletters(
ws = wsClient,
newsletters = newsletters,
page = StaticPages.dcrSimpleNewsletterPage(request.path),
)
case Left(e) =>
log.error(s"API call to get newsletters failed: $e")
NoCache(InternalServerError)
Future(NoCache(InternalServerError))
}
}

def renderNewslettersPage()(implicit
executionContext: ExecutionContext = this.executionContext,
): Action[AnyContent] =
csrfAddToken {
Action { implicit request =>
if (request.forceDCR || UseDcrNewslettersPage.isSwitchedOn) {
remoteRenderNewslettersPage()
} else {
localRenderNewslettersPage()
}
}
}
Comment on lines -78 to -89
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved into generic renderNewsletters function at bottom of page which handles both JSON and HTML and DCR / non DCR requests


private def renderDCRNewslettersJson()(implicit
request: RequestHeader,
): Result = {
): Future[Result] = {
val newsletters: Either[String, List[NewsletterResponseV2]] =
newsletterSignupAgent.getV2Newsletters()

Expand All @@ -100,29 +87,35 @@ class SignupPageController(
val dataModel =
DotcomNewslettersPageRenderingDataModel.apply(page, newsletters, request)
val dataJson = DotcomNewslettersPageRenderingDataModel.toJson(dataModel)
common.renderJson(dataJson, page).as("application/json")
Future.successful(common.renderJson(dataJson, page).as("application/json"))
}
case Left(e) =>
log.error(s"API call to get newsletters failed: $e")
throw new RuntimeException()
}
}

private def renderNewslettersJson()(implicit
private def notFoundPage()(implicit
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Named more appropriately, I think? Might have got the wrong end of the stick though...

request: RequestHeader,
): Result = {
Cached(CacheTime.NotFound)(Cached.WithoutRevalidationResult(NotFound))
): Future[Result] = {
Future(Cached(CacheTime.NotFound)(Cached.WithoutRevalidationResult(NotFound)))
}

def renderNewslettersJson()(implicit
def renderNewsletters()(implicit
executionContext: ExecutionContext = this.executionContext,
): Action[AnyContent] =
csrfAddToken {
Action { implicit request =>
if (request.forceDCR || UseDcrNewslettersPage.isSwitchedOn) {
renderDCRNewslettersJson()
} else {
renderNewslettersJson()
Action.async { implicit request =>
val useDCR = request.forceDCR || UseDcrNewslettersPage.isSwitchedOn
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker as this is the current behaviour, but with this logic, it’s impossible to force DCR off when the UseDcrNewslettersPage switch is on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yep you're right. I actually wonder if that's intentional as the frontend newsletters page is VERY different to the DCR one 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

🤷


request.getRequestFormat match {
case HtmlFormat if useDCR =>
remoteRenderNewslettersPage()
case HtmlFormat =>
localRenderNewslettersPage()
case JsonFormat if useDCR =>
renderDCRNewslettersJson()
case _ => notFoundPage()
Comment on lines +104 to +118
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function combines request handling for both JSON and HTML requests, removing some duplicated code above and (I believe) making it a bit easier to read.

}
}
}
Expand Down
4 changes: 2 additions & 2 deletions applications/conf/routes
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ GET /_healthcheck
GET /sitemaps/news.xml controllers.SiteMapController.renderNewsSiteMap()
GET /sitemaps/video.xml controllers.SiteMapController.renderVideoSiteMap()

GET /email-newsletters.json controllers.SignupPageController.renderNewslettersJson()
GET /email-newsletters controllers.SignupPageController.renderNewslettersPage()
GET /email-newsletters.json controllers.SignupPageController.renderNewsletters()
GET /email-newsletters controllers.SignupPageController.renderNewsletters()

GET /survey/:formName/show controllers.SurveyPageController.renderFormStackSurvey(formName)
GET /survey/thankyou controllers.SurveyPageController.thankYou()
Expand Down
2 changes: 1 addition & 1 deletion common/app/renderers/DotcomRenderingService.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,7 @@ class DotcomRenderingService extends GuLogging with ResultWithPreconnectPreload

val dataModel = DotcomNewslettersPageRenderingDataModel.apply(page, newsletters, request)
val json = DotcomNewslettersPageRenderingDataModel.toJson(dataModel)
post(ws, json, Configuration.rendering.faciaBaseURL + "/EmailNewsletters", CacheTime.Facia)
post(ws, json, Configuration.rendering.baseURL + "/EmailNewsletters", CacheTime.Facia)
}

def getImageContent(
Expand Down
Loading