Scala 2.13: Refactor cricket match summary template

[rtyley]

This commit removes code duplication and fixes Scala 2.13 compilation issues in the cricket match summary template, which is embedded into the /sport/cricket/match/.../...-team.json endpoint, delivering a dynamically loaded HTML chunk summarising the scores on to Cricket articles like this:

(the corresponding json endpoint for that article is https://api.nextgen.guardianapps.co.uk/sport/cricket/match/2022-07-31/england-cricket-team.json ...you have to work out the date of the article - eg 2022-07-31 - and then transplant it into the url for the json endpoint. Example payload)

We're removing the duplication because in general DRY is a good thing, and the intricacies of cricket need as much clarity as possible for non-cricket-following developers! The reason we're looking at this code though (and trying to understand all the cricket-related-logic!) is because of a compilation error under Scala 2.13.

The Scala 2.13 compiler is stricter about non-exhaustive matches in case-pattern-match expression than Scala 2.12 was, and this is actually a good thing - there was recently a bugfix in this bit of code because an unanticipated case in the PA data caused HTTP 500 errors: #24499

In the case of the @score() reusable template block, the Scala 2.13 compiler couldn't verify that all cases were covered - even though !innings.closed (meaning the innings are ongoing) is the logical complement of (innings.declared + innings.forfeited + innings.allOut), the compiler couldn't verify that without a knowledge of cricket!

frontend/sport/app/cricket/views/fragments/cricketMatchSummary.scala.html

Lines 84 to 89 in 5a6f2b9

	@innings match {
	case _ if (!innings.closed) => { @innings.runsScored - @innings.wickets }
	case _ if (innings.declared) => { @innings.runsScored - @innings.wickets declared }
	case _ if (innings.forfeited) => { @innings.runsScored - @innings.wickets forfeited }
	case _ if (innings.allOut) => { @innings.runsScored all out }
	}

[error] /Users/roberto/guardian/frontend/sport/app/cricket/views/fragments/cricketMatchSummary.scala.html:84:6: match may not be exhaustive.
[error]     @innings match {
[error]      ^

It's simpler just to catch all the specific 'closed' cases we care about, and then catch the remaining 'open' case with case _ => to very obviously catch everything that isn't closed.

[ioannakok]

I paired on this!

[OllysCoding]

+1 This all makes sense to me - I remember noticing some of these issues when migrating this UI into DCR - and looking through your changes they all make sense!

[rtyley]

I remember noticing some of these issues when migrating this UI into DCR

Oh, wow, is this functionality all already migrated to DCR?! Looking now at https://www.theguardian.com/sport/live/2022/jul/31/england-v-south-africa-third-mens-t20-live-cricket, I see that it's calling the json endpoint with dcr=true on it:

https://api.nextgen.guardianapps.co.uk/sport/cricket/match/2022-07-31/england-cricket-team.json?dcr=true

I think this wasn't the case back in December 2021, because #24499 was a bugfix reacting to problems occurring the non-DCR Frontend implementation, but I guess the work in April 2022 with guardian/dotcom-rendering#4758 switched over to using the DCR implementation... I wonder if we should have just deleted this Frontend implementation rather than fixing it? Will the non-DCR version get called anymore at all do you think?

Oh well, in any case, I'm going to merge this just to get the Scala 2.13 compilation error cleared!

[prout-bot]

Seen on PROD (merged by @rtyley 16 minutes and 8 seconds ago)

• Check your changes on www.theguardian.com ✔️
• Keep an eye on the deploy dashboard 📉

[rtyley]

Having deployed this to PROD, I'm now puzzled, as the json endpoints - DCR & non DCR - are now both 404. I can't really see how this template change could have caused that?! Should I revert this PR?

% curl -I "https://api.nextgen.guardianapps.co.uk/sport/cricket/match/2022-07-31/england-cricket-team.json?dcr=true"
HTTP/2 404
content-type: text/html; charset=utf-8
etag: "62f11b48-5d5d"
server: nginx
accept-ranges: bytes
date: Tue, 09 Aug 2022 08:29:25 GMT
via: 1.1 varnish
x-served-by: cache-lcy19233-LCY
x-cache: MISS
x-cache-hits: 0
x-timer: S1660033765.298316,VS0,VE29
vary: Accept-Encoding
access-control-allow-credentials: true
access-control-allow-headers: X-Requested-With,Origin,Accept,Content-Type
access-control-allow-origin: *
x-gu-geolocation: country:GB
cache-control: max-age=10, private
age: 0
content-length: 23901

% curl -I "https://api.nextgen.guardianapps.co.uk/sport/cricket/match/2022-07-31/england-cricket-team.json?dcr=false"
HTTP/2 404
content-type: text/html; charset=utf-8
etag: "62f11b48-5d5d"
server: nginx
accept-ranges: bytes
date: Tue, 09 Aug 2022 08:29:33 GMT
via: 1.1 varnish
x-served-by: cache-lcy19230-LCY
x-cache: MISS
x-cache-hits: 0
x-timer: S1660033774.974448,VS0,VE18
vary: Accept-Encoding
access-control-allow-credentials: true
access-control-allow-headers: X-Requested-With,Origin,Accept,Content-Type
access-control-allow-origin: *
x-gu-geolocation: country:GB
cache-control: max-age=10, private
age: 0
content-length: 23901

[rtyley]

Oh... and now they're no longer 404, and everything looks fine - I guess this usually happens after deploy?!:

% curl -I "https://api.nextgen.guardianapps.co.uk/sport/cricket/match/2022-07-31/england-cricket-team.json?dcr=true"
HTTP/2 200
content-type: application/json
etag: W/"hash8085544890306034655"
server: nginx
x-gu-backend-app: sport
accept-ranges: bytes
date: Tue, 09 Aug 2022 08:32:26 GMT
via: 1.1 varnish
x-served-by: cache-lcy19281-LCY
x-cache: MISS
x-cache-hits: 0
x-timer: S1660033946.174618,VS0,VE26
vary: Origin,Accept, Accept-Encoding
access-control-allow-credentials: true
access-control-allow-headers: X-Requested-With,Origin,Accept,Content-Type
access-control-allow-origin: *
x-gu-geolocation: country:GB
cache-control: max-age=60, stale-while-revalidate=6, stale-if-error=864000, private
age: 0
content-length: 6353

% curl -I "https://api.nextgen.guardianapps.co.uk/sport/cricket/match/2022-07-31/england-cricket-team.json?dcr=false"
HTTP/2 200
content-type: application/json
etag: W/"hash-5050155989424043750"
server: nginx
x-gu-backend-app: sport
accept-ranges: bytes
date: Tue, 09 Aug 2022 08:32:31 GMT
via: 1.1 varnish
x-served-by: cache-lcy19254-LCY
x-cache: MISS
x-cache-hits: 0
x-timer: S1660033951.482608,VS0,VE23
vary: Origin,Accept, Accept-Encoding
access-control-allow-credentials: true
access-control-allow-headers: X-Requested-With,Origin,Accept,Content-Type
access-control-allow-origin: *
x-gu-geolocation: country:GB
cache-control: max-age=60, stale-while-revalidate=6, stale-if-error=864000, private
age: 0
content-length: 1619

[ioannakok]

Maybe?! Still giving 200 so I think we can keep this change. I have never noticed that before but I have never curled these endpoints after a deployment. It looks like that's the case > I guess this usually happens after deploy