PIL-2449 Introduce domain model for financial data #563

dc-smith · 2025-10-15T13:01:58Z

The requirement in PIL-2449 to add additional states to the dynamic notification area requires nontrivial parsing of the transactions we receive from ETMP.

My view is that trying understand the new states alongside the old ones is a level of complexity which necessitates a domain model. This branch aims to introduce one for the financial data; I will raise another branch off the back of this one with the new content, and merge that into this after both are approved to support QA in testing the change. It should be easier to review as two PRs, even if it eventually all gets merged together.

Enumerate ETMP references we understand.
Model financial transactions as an ADT to support clearer use later on.
Move validation of outstanding charges from Service -> Controller to Connector -> Service. This does not introduce any extra checks.
Drop any transactions which do not pass validation. This preserves the existing behaviour.

Unrelated - made recent payment threshold configurable to stop scalastyle complaining about a magic number. After rebasing, this had been moved to a Constants file, but I thought it was worth keeping the config. Happy to roll back if we like.

platops-pr-bot · 2025-10-15T13:06:03Z

uk.gov.hmrc:play-frontend-hmrc-play-30 can be updated to 12.17.0 in project/AppDependencies.scala.
uk.gov.hmrc.mongo:hmrc-mongo-play-30 can be updated to 2.10.0 in project/AppDependencies.scala.
uk.gov.hmrc.mongo:hmrc-mongo-test-play-30 can be updated to 2.10.0 in project/AppDependencies.scala.

dc-smith · 2025-10-16T10:15:12Z

app/models/FinancialData.scala

-  outstandingAmount: Option[BigDecimal],
-  items:             Seq[FinancialItem]
-)
+case class TaxPeriod(from: LocalDate, to: LocalDate)


TaxPeriod is nearly identical to AccountingPeriod, but I didn't want to go poking around in any more places than I already have.

Maybe it's worth creating a spike to investigate further and consolidate if possible

Good idea, I'll get something into the backlog to review the model, reduce any redundancy, etc. I'll highlight this but keep the scope fairly wide.

dc-smith · 2025-10-16T10:16:36Z

app/models/FinancialData.scala

+        case EtmpMainTransactionRef.UkTaxReturnMain     => (UktrMainOutstandingCharge.apply _).tupled(fields)
+        case EtmpMainTransactionRef.LatePaymentInterest => (LatePaymentInterestOutstandingCharge.apply _).tupled(fields)
+        case EtmpMainTransactionRef.RepaymentInterest   => (RepaymentInterestOutstandingCharge.apply _).tupled(fields)


I don't particularly like constructing things like this, but I also like being able to distinguish the charges at the type level. Open to suggestions on a cleaner model.

dc-smith · 2025-10-16T10:21:37Z

app/services/FinancialDataService.scala

+        val daysAgoLatestPaymentCleared = ChronoUnit.DAYS.between(latestClearing, LocalDate.now(clock))
+        daysAgoLatestPaymentCleared <= appConfig.maxDaysAgoToConsiderPaymentAsRecent
+      }
+}


I actually move this back into FinancialData in the followup PR, which I'll merge into this one.

dc-smith · 2025-10-16T10:23:49Z

app/services/FinancialDataService.scala

+          }
+          Option.empty[FinancialTransaction]
+      }
+  }


Each of the validations here is taken from the filters which previously existed in FinancialData. There should be only one behaviour change, called out below.

dc-smith · 2025-10-16T10:33:00Z

app/services/FinancialDataService.scala

+        .flatMap(_.dueDate)
+        .minOption
+        .toValidNec(IgnoredEtmpTransaction.RequiredValueMissing("dueDate"))
+        .map(earliestDueDate => OutstandingCharge.FinancialItems(earliestDueDate, responseItems.map(responseItemToDomain)))


We previously made the assumption that the first item in the sequence always has a due date in OutstandingPaymentsController. This:

Makes that assumption explicit

Slightly tweaks the behaviour to track the earliest due date separately. If they really do always have the same due date, this shouldn't be a change. And if they are different, earliest is likely what we want.

dc-smith · 2025-10-16T10:35:05Z

test/controllers/payments/OutstandingPaymentsControllerSpec.scala

        status(result) mustEqual OK
        contentAsString(result) mustEqual
-          view(Seq.empty, pillar2Id, BigDecimal(0), hasOverdueReturnPayment = false)(
+          view(overdueFinancialSummary, pillar2Id, BigDecimal(1000.00), hasOverdueReturnPayment = true)(


This test's scenario no longer matched the name, and the state it was producing (transaction passed in with an outstanding amount of zero) is no longer representable using the domain model.

dc-smith · 2025-10-16T13:09:18Z

app/models/FinancialData.scala

+object TaxPeriod {
+  implicit val ordering: Ordering[TaxPeriod] = Ordering.by(_.from)
 }


This is equivalent to the ordering previously used in OutstandingPaymentsController where a tuple of (from, to) was sorted (I dug around in the scala stdlib for the implicit Tuple2 Ordering).

gianick · 2025-10-16T10:52:48Z

app/connectors/FinancialDataConnector.scala


 }
+
+object FinancialDataConnector {


I think it'd be better if these were placed under models

gianick · 2025-10-16T10:58:17Z

app/controllers/payments/OutstandingPaymentsController.scala

  private def toOutstandingPaymentsSummaries(financialData: FinancialData): Seq[FinancialSummary] = {
    val outstandingCharges = financialData.outstandingCharges

    outstandingCharges


Since you are refactoring things, the val outstandingCharges = financialData.outstandingCharges assignment is not really needed. We can directly use financialData.outstandingCharges here.

Suggested change

outstandingCharges

financialData.outstandingCharges

gianick · 2025-10-16T13:45:59Z

app/controllers/payments/OutstandingPaymentsController.scala

            .toSeq

-        FinancialSummary(AccountingPeriod(periodFrom, periodTo), transactionSummaries.sortBy(_.dueDate).reverse)
+        FinancialSummary(AccountingPeriod(taxPeriod.from, taxPeriod.to), transactionSummaries.sortBy(_.dueDate).reverse)


Moved the sorting from the FinancialSummary to the end of transactionSummaries:

Suggested change

.toSeq

FinancialSummary(AccountingPeriod(periodFrom, periodTo), transactionSummaries.sortBy(_.dueDate).reverse)

FinancialSummary(AccountingPeriod(taxPeriod.from, taxPeriod.to), transactionSummaries.sortBy(_.dueDate).reverse)

.toSeq

.sortBy(_.dueDate)

.reverse

FinancialSummary(AccountingPeriod(taxPeriod.from, taxPeriod.to), transactionSummaries)

gianick · 2025-10-16T13:54:42Z

app/controllers/DashboardController.scala

    if (financialData.financialTransactions.isEmpty) {
      None
    } else {
-      val totalOutstandingAmount = financialData.getTotalOutstandingAmount
+      val totalOutstandingAmount = financialData.totalOutstandingAmount
      val hasOutstandingPayment  = financialData.hasOverdueOutstandingPayments(currentDate)
-      val hasRecentPayment       = financialData.hasRecentPayment(currentDate = currentDate)
+      val hasRecentPayment       = financialDataService.hasRecentPayment(financialData)

      (hasOutstandingPayment, hasRecentPayment) match {
        case (true, _)                                    => Some(Outstanding(totalOutstandingAmount))
        case (false, true) if totalOutstandingAmount == 0 => Some(Paid)
        case _                                            => None
      }
    }


Since we are refactoring a few things, maybe this looks "cleaner"?
Used Option.when and moved the if totalOutstandingAmount == 0 guard to the match:

Suggested change

Option

.when(financialData.financialTransactions.nonEmpty) {

val totalOutstandingAmount: BigDecimal = financialData.totalOutstandingAmount

val hasOutstandingPayment: Boolean = financialData.hasOverdueOutstandingPayments(currentDate)

val hasRecentPayment: Boolean = financialDataService.hasRecentPayment(financialData)

(hasOutstandingPayment, hasRecentPayment, totalOutstandingAmount == 0) match {

case (true, _, _) => Some(Outstanding(financialData.totalOutstandingAmount))

case (false, true, true) => Some(Paid)

case _ => None

}

}

.flatten

I'm going to hold off on this for now, if that's OK. The next PR introduces a PaymentState model which is then used to determine both the notification banner and the payment tag, so this method is significantly tidied up there anyway.

gianick · 2025-10-16T14:00:14Z

app/models/subscription/AccountStatus.scala

+  case object ActiveAccount extends AccountStatus
+  case object InactiveAccount extends AccountStatus
+
+  val values = findValues


Suggested change

val values = findValues

val values: IndexedSeq[AccountStatus] = findValues

Adding type annotation

gianick · 2025-10-16T14:19:24Z

app/models/TransactionRefs.scala

+      with UnderTaxedProfitsRule
+      with LatePaymentInterest
+
+  val values = findValues


Suggested change

val values = findValues

val values: IndexedSeq[EtmpSubtransactionRef] = findValues

gianick · 2025-10-16T14:22:17Z

app/services/FinancialDataService.scala

+      .retrieveFinancialData(pillar2Id, fromDate, toDate)
+      .map(parseFinancialDataResponse)

+  /** Checks if there has been a recent payment */


Suggested change

/** Checks if there has been a recent payment */

As mentioned above, we should try to not be using "any" comments unless they explain the 'why', not the 'what'. :)

gianick · 2025-10-16T14:23:38Z

app/services/FinancialDataService.scala

+    case class RequiredValueMissing(field: String) extends IgnoredEtmpTransaction(s"$field was missing")
+    case class UnrelatedValue(field: String, value: String) extends IgnoredEtmpTransaction(s"$field has invalid value $value")
+    case class DidNotPassFilter[A](field: String, value: A, reason: String)
+        extends IgnoredEtmpTransaction(s"$field's value $value did not meet critera $reason")


Suggested change

extends IgnoredEtmpTransaction(s"$field's value $value did not meet critera $reason")

extends IgnoredEtmpTransaction(s"$field's value $value did not meet criteria $reason")

Minor typo

gianick · 2025-10-16T14:28:48Z

app/services/FinancialDataService.scala

+    financialData.payments
+      .flatMap(_.paymentItems.latestClearingDate)
+      .exists { latestClearing =>
+        val daysAgoLatestPaymentCleared = ChronoUnit.DAYS.between(latestClearing, LocalDate.now(clock))


Suggested change

val daysAgoLatestPaymentCleared = ChronoUnit.DAYS.between(latestClearing, LocalDate.now(clock))

val daysAgoLatestPaymentCleared: Long = ChronoUnit.DAYS.between(latestClearing, LocalDate.now(clock))

gianick · 2025-10-16T14:39:17Z

test/services/FinancialDataServiceSpec.scala

+    subTransaction = Some("6233"),
+    taxPeriodFrom = Some(today.minusMonths(1)),
+    taxPeriodTo = Some(today),
+    outstandingAmount = Some(BigDecimal(100)), // scalastyle:ignore magic.number


Instead of adding the scalastyle comment, can you modify the scalastyle-config.xml file?

<check level="warning" class="org.scalastyle.scalariform.MagicNumberChecker" enabled="true"> <parameters> <parameter name="ignore"><![CDATA[-1,0,1,2,3,4,5,6,7,8,9,10,100,1000]]></parameter> </parameters> </check>

Ideally, I'd like to ignore the magic number rule only for tests; I think it's a sensible rule for production code but it's a bit of a pain when adding examples. I haven't used scalastyle before, but I'll look into it.

dc-smith · 2025-10-16T14:50:14Z

app/controllers/DashboardController.scala

    if (financialData.financialTransactions.isEmpty) {
      None
    } else {
-      val totalOutstandingAmount = financialData.getTotalOutstandingAmount
+      val totalOutstandingAmount = financialData.totalOutstandingAmount
      val hasOutstandingPayment  = financialData.hasOverdueOutstandingPayments(currentDate)
-      val hasRecentPayment       = financialData.hasRecentPayment(currentDate = currentDate)
+      val hasRecentPayment       = financialDataService.hasRecentPayment(financialData)

      (hasOutstandingPayment, hasRecentPayment) match {
        case (true, _)                                    => Some(Outstanding(totalOutstandingAmount))
        case (false, true) if totalOutstandingAmount == 0 => Some(Paid)
        case _                                            => None
      }
    }


I'm going to hold off on this for now, if that's OK. The next PR introduces a PaymentState model which is then used to determine both the notification banner and the payment tag, so this method is significantly tidied up there anyway.

dc-smith · 2025-10-16T14:51:13Z

app/models/FinancialData.scala

+  /** Calculates the total outstanding amount from financial data */
+  def totalOutstandingAmount: BigDecimal = outstandingCharges.map(_.outstandingAmount).sum


Agreed, I'll revisit these method names and get rid of the comments where they seem redundant.

dc-smith · 2025-10-16T14:53:46Z

app/models/FinancialData.scala

-  outstandingAmount: Option[BigDecimal],
-  items:             Seq[FinancialItem]
-)
+case class TaxPeriod(from: LocalDate, to: LocalDate)


Good idea, I'll get something into the backlog to review the model, reduce any redundancy, etc. I'll highlight this but keep the scope fairly wide.

dc-smith · 2025-10-16T14:55:24Z

app/models/TransactionRefs.scala

+
+package models
+
+import enumeratum.values.{StringEnum, StringEnumEntry}


Good point. These are a bit unwieldy, and there's a clear dividing line to split along, so I'll get that done.

dc-smith · 2025-10-16T14:56:31Z

app/models/TransactionRefs.scala

+object EtmpSubtransactionRef extends StringEnum[EtmpSubtransactionRef] {
+  case object Dtt extends EtmpSubtransactionRef("6233") with DomesticTopupTax
+  case object Mtt extends EtmpSubtransactionRef("6234") with MultinationalTopupTax
+  case object UktrIirIrrMttUtprDiscDet


I couldn't find a source on all of the acronyms I pulled from the previous file 😅 That's why there isn't a trait for IRR, and I'm not sure what DiscDet is so I kept the names identical.

dc-smith · 2025-10-16T15:01:52Z

test/services/FinancialDataServiceSpec.scala

+    subTransaction = Some("6233"),
+    taxPeriodFrom = Some(today.minusMonths(1)),
+    taxPeriodTo = Some(today),
+    outstandingAmount = Some(BigDecimal(100)), // scalastyle:ignore magic.number


Ideally, I'd like to ignore the magic number rule only for tests; I think it's a sensible rule for production code but it's a bit of a pain when adding examples. I haven't used scalastyle before, but I'll look into it.

dc-smith added 3 commits October 15, 2025 14:03

PIL-2449 refactor AccountStatus to ADT

e0df483

PIL-2449 introduce domain model for reading financial data

02204b0

PIL-2449 remove payment time constant as it's now configurable

502cbba

dc-smith force-pushed the PIL-2449 branch from 0905f53 to 502cbba Compare October 15, 2025 13:03

PIL-2449 re-add payment threshold config after botched rebase

5822c96

dc-smith changed the title ~~PIL-2449 New notification area states & content~~ PIL-2449 Introduce domain model for financial data Oct 16, 2025

dc-smith commented Oct 16, 2025

View reviewed changes

gianick reviewed Oct 16, 2025

View reviewed changes

dc-smith commented Oct 16, 2025

View reviewed changes

+    Option
+      .when(financialData.financialTransactions.nonEmpty) {
+        val totalOutstandingAmount: BigDecimal = financialData.totalOutstandingAmount
+        val hasOutstandingPayment:  Boolean    = financialData.hasOverdueOutstandingPayments(currentDate)
+        val hasRecentPayment:       Boolean    = financialDataService.hasRecentPayment(financialData)
+        (hasOutstandingPayment, hasRecentPayment, totalOutstandingAmount == 0) match {
+          case (true, _, _)        => Some(Outstanding(financialData.totalOutstandingAmount))
+          case (false, true, true) => Some(Paid)
+          case _                   => None
+        }
+      }
+      .flatten

	val values = findValues
	val values: IndexedSeq[AccountStatus] = findValues

	val values = findValues
	val values: IndexedSeq[EtmpSubtransactionRef] = findValues

	extends IgnoredEtmpTransaction(s"$field's value $value did not meet critera $reason")
	extends IgnoredEtmpTransaction(s"$field's value $value did not meet criteria $reason")

	val daysAgoLatestPaymentCleared = ChronoUnit.DAYS.between(latestClearing, LocalDate.now(clock))
	val daysAgoLatestPaymentCleared: Long = ChronoUnit.DAYS.between(latestClearing, LocalDate.now(clock))

		/** Calculates the total outstanding amount from financial data */
		def totalOutstandingAmount: BigDecimal = outstandingCharges.map(_.outstandingAmount).sum


		package models

		import enumeratum.values.{StringEnum, StringEnumEntry}

PIL-2449 Introduce domain model for financial data #563

Are you sure you want to change the base?

PIL-2449 Introduce domain model for financial data #563

Uh oh!

Conversation

dc-smith commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

platops-pr-bot commented Oct 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dc-smith commented Oct 15, 2025 •

edited

Loading