Apb 11788 - Special Case to allow setting previously Failed applications to be Success by geoffreywatson · Pull Request #143 · hmrc/agent-registration-risking

geoffreywatson · 2026-07-06T12:34:01Z

Applications that have been approved following an appeals process may be added to a new config array (the id's) so the results can be overturned and the Failed state is instead set to Success.
Unsets the previous Failed responses and replaces with Success, allowing the applications to be subscribed to ASA, with all side effects (new emails and audit events).

…ations

platops-pr-bot · 2026-07-06T12:37:23Z

Routes defined in conf/testOnlyDoNotUseInAppConf.routes should contain /test-only/ prefix

* APB-11588 feature flag sending notifications for failed fixable Signed-off-by: pav <173694350+paweldigital@users.noreply.github.com> * APB-11588 PR Signed-off-by: pav <173694350+paweldigital@users.noreply.github.com> --------- Signed-off-by: pav <173694350+paweldigital@users.noreply.github.com>

…ations # Conflicts: # conf/application.conf

…g into APB-11788 Signed-off-by: geoffreywatson <9043337+geoffreywatson@users.noreply.github.com>

gitwojciech · 2026-07-06T14:07:44Z

+            to be Approved instead (to allow for an ASA to be created). For example, where an applicant has successfully
+            appealed a Failed Non-Fixable.
+   */
+  def specialCaseApprovePreviouslyFailedApplications()(using request: RequestHeader): Future[Unit] =


I do not see apart from testing where that function is called

ah, oh dear...I think Pav moved changed the functionality so all that logic is run from a scheduler instead....I manually tested this before rebasing so I'll need to add this method call to that scheduled process.

gitwojciech · 2026-07-06T16:52:36Z

+      )
+    ).toFuture().map(_ => ())
+  }
+


In this project we decided not to do field level updates. We update whole records

Few flags that needs to reset too isEmailSent=false, correctiveActionExpiryDate=None, emailSentAt=None , isSubscribed=false. Please note emailSentAt is new in my PR

gitwojciech · 2026-07-06T17:29:39Z

+  def specialCaseApprovePreviouslyFailedApplications()(using request: RequestHeader): Future[Unit] =
+    if (appConfig.enableUnsetRiskingResponses) {
+      logger.info("[SpecialCaseApprovePreviouslyFailedApplications] feature ENABLED.")
+      Future.sequence(


Future sequence has some drow back like failed fast that leave orphan futures etc. would be better to use ProcessInSequence.processInSequence

gitwojciech · 2026-07-07T07:12:52Z

+    )
+      .toFuture().map(_ => ())
+  }
+


Small suggestion — the Repo base class already provides upsert(...) that does the same whole-document replaceOne under the hood. It's used everywhere else in the codebase (e.g. BackendNotificationService, EmailServiceForApprovedApplications, EmailServiceForFailedNonFixable). Would you mind switching this to upsert(updated) for consistency?

gitwojciech · 2026-07-07T07:21:29Z

+    if (appConfig.enableUnsetRiskingResponses) {
+      logger.info("[SpecialCaseApprovePreviouslyFailedApplications] feature ENABLED.")
+      ProcessInSequence.processInSequence[ApplicationReference, Unit](
+        appConfig.applicationIdsForUnsettingRiskingResponses.map(ApplicationReference(_))


Small suggestion — switch processInSequence to processAllInSequence so a single problematic config entry doesn't fail I think. The other batch services (BackendNotificationService, EmailServiceForFailedNonFixable, RiskingArchivalService) use this pattern to get per-item error

paweldigital · 2026-07-07T07:45:38Z

+  private def processFeatureFlagged(applicationWithIndividuals: ApplicationWithIndividuals)(using RequestHeader): Future[Unit] =
+    val applicationForRisking: ApplicationForRisking = applicationWithIndividuals.application
+    applicationForRisking.overallStatus.riskingOutcome.getOrThrowExpectedDataMissing("risking outcome") match
+      case RiskingOutcome.FailedFixable =>
+        if appConfig.Features.fixableFailures
+        then process(applicationWithIndividuals)
+        else
+          logger.info(s"Not notifying backend for application ${applicationForRisking.applicationReference} with failed fixable outcome because of feature flag: ${appConfig.Features.fixableFailures}")
+          Future.unit
+      case _ => process(applicationWithIndividuals)
+


This code is already on the main branch. Could you please check that your merge from main was completed correctly?

paweldigital · 2026-07-07T07:45:43Z

+  Email
+  CorrectiveAction
+  Base64
+  Features


This code is already on the main branch. Could you please check that your merge from main was completed correctly?

paweldigital · 2026-07-07T07:54:27Z

+  def findAlreadyRiskedApplication(applicationReference: ApplicationReference): Future[Option[ApplicationWithIndividuals]] = findApplicationWithIndividuals(
+    applicationFilter = Filters.and(
+      Filters.eq(FieldNames.applicationReference, applicationReference.value),
+      Filters.exists(FieldNames.entityRiskingResult),


riskingOutcome can only exist if entityRiskingResult is already present, so we don't really need that condition in the filter.

That said, I'm happy to leave it as is since this is one-off migration code that will be removed afterwards.

paweldigital · 2026-07-07T07:54:43Z

+      Filters.exists(FieldNames.entityRiskingResult),
+      Filters.exists(FieldNames.overallStatus.riskingOutcome, true)
+    ),
+    individualForAllFilter = Filters.exists(FieldNames.individualRiskingResult)


paweldigital · 2026-07-07T07:56:34Z

    individualForAllFilter = Filters.exists(FieldNames.individualRiskingResult)
  )

+  def findAlreadyRiskedApplication(applicationReference: ApplicationReference): Future[Option[ApplicationWithIndividuals]] = findApplicationWithIndividuals(


I don't think this needs to fetch the application together with its individuals. It should be simpler and sufficient to fetch just the application.

paweldigital · 2026-07-07T07:56:55Z

+
+  val riskingOutcomeIndividualDetailsFix: RiskingOutcomeIndividual.FailedFixable = RiskingOutcomeIndividual.FailedFixable(
+    fixes = Seq(
+      IndividualFix._10.IndividualDetailsFix(
+        dateOfBirth = Some(IndividualDateOfBirth.Provided(dateOfBirth)),
+        nino = Some(IndividualNino.Provided(nino)),
+        saUtr = Some(IndividualSaUtr.Provided(saUtr)),
+        isConfirmed = None
+      )
+    )
+  )


This code is already on the main branch. Could you please check that your merge from main was completed correctly?

paweldigital · 2026-07-07T08:02:19Z

+
+import java.time.ZoneOffset
+
+class BackendNotificationServiceFeatureFlagSpec


This is already in the main branch

paweldigital · 2026-07-07T08:02:34Z

 class BackendNotificationServiceSpec
 extends ISpec:

+  override protected def configOverrides: Map[String, Any] = Map(


already on main branch

paweldigital · 2026-07-07T08:28:37Z

+
+      riskingResultsService.specialCaseApprovePreviouslyFailedApplications().futureValue
+
+      val modified = applicationForRiskingRepo.findById(ApplicationReference(failedNonFixableAppRef)).futureValue


Could you please name the value and add the type annotation, as we usually do in this codebase?

paweldigital · 2026-07-07T08:30:50Z

+          )
+        )
+
+      modified.flatMap(_.overallStatus.riskingOutcome) shouldBe Some(RiskingOutcome.Approved)


Could you please assert the entire object, to ensure that only the expected fields are changed?

Also, please use OptionValues from ScalaTest (as we do in other places) when asserting on Option values.

paweldigital · 2026-07-07T08:37:32Z

    individualForAllFilter = Filters.exists(FieldNames.individualRiskingResult)
  )

+  def findAlreadyRiskedApplication(applicationReference: ApplicationReference): Future[Option[ApplicationWithIndividuals]] = findApplicationWithIndividuals(


I don't think this function is needed. The goal is to find the application and its individuals by applicationReference, so it would be simpler to reuse the existing functions that are already dedicated to this purpose instead of introducing new ones.

Also, please note that in this project all custom find functions need to be covered by unit tests. This is another reason to prefer reusing the existing functionality rather than adding a new method that would require additional testing.

Please use the existing methods:

ApplicationForRiskingRepo.findById( applicationRef: ApplicationReference ): Future[Option[ApplicationForRisking]]

to retrieve the application, and:

IndividualForRiskingRepo.findByApplicationReference( applicationReference: ApplicationReference ): Future[Seq[IndividualForRisking]]

to retrieve the corresponding individuals.

paweldigital · 2026-07-07T08:39:54Z

+            to be Approved instead (to allow for an ASA to be created). For example, where an applicant has successfully
+            appealed a Failed Non-Fixable.
+   */
+  def specialCaseApprovePreviouslyFailedApplications()(using request: RequestHeader): Future[Unit] =


Could you rename the function so that its name better reflects what it does? It would make the code easier to understand and read.

Something like:

def updateApplicationOutcomeToApproved

or a similar name that clearly describes the action being performed.

As a general convention, we'd like function names to contain a verb so that it is immediately clear what the function does.

paweldigital · 2026-07-07T08:43:22Z

+  [APB-11788] special case / temporary solution for allowing applications that have already been determined as Failed
+            to be Approved instead (to allow for an ASA to be created). For example, where an applicant has successfully
+            appealed a Failed Non-Fixable.
+   */


Sorry for bothering, but could you convert this into a proper ScalaDoc comment instead of a multiline comment?

paweldigital · 2026-07-07T09:17:31Z

+    if (appConfig.enableUnsetRiskingResponses) {
+      logger.info("[SpecialCaseApprovePreviouslyFailedApplications] feature ENABLED.")
+      ProcessInSequence.processAllInSequence[ApplicationReference, Unit](
+        appConfig.applicationIdsForUnsettingRiskingResponses.map(ApplicationReference(_))
+      )(appRef =>
+        logger.info(s"[SpecialCaseApprovePreviouslyFailedApplications] Trying from config appRef: ${appRef.value}")
+        applicationForRiskingRepo.findAlreadyRiskedApplication(appRef).flatMap {
+          case Some(appWithIndividuals) =>
+            appWithIndividuals.application.overallStatus.riskingOutcome match {
+              case Some(RiskingOutcome.FailedNonFixable | RiskingOutcome.FailedFixable) =>
+                for {
+                  _ <- updateRiskingResults(RiskingResult.ForEntity(
+                    applicationReference = appWithIndividuals.application.applicationReference,


Could you consider a simpler implementation, something along these lines?

def updateApplicationOutcomeToApprovedIfNeeded(): Future[Unit] = if appConfig.enableUnsetRiskingResponses then updateApplicationOutcomeToApproved() else Future.unit private def requiresUpdate(application: ApplicationForRisking): Boolean = ... private def requiresUpdate(individual: IndividualForRisking): Boolean = ... private def updateApplicationOutcomeToApproved(): Future[Unit] = ProcessInSequence.processAllInSequence(appConfig.applicationIdsForUnsettingRiskingResponses): applicationReference => for application <- applicationForRiskingRepo.findById(applicationReference) individuals <- individualForRiskingRepo.findByApplicationReference(applicationReference) updatedIndividuals = individuals.map(_.copy(...)) _ <- if requiresUpdate(application) then applicationForRiskingRepo.upsert(application.copy(...)) else Future.unit _ <- ProcessInSequence.processAllInSequence(individuals)(individual => if requiresUpdate(individual) then individualForRiskingRepo.upsert(updatedIndividual)) else Future.unit yield logger.info("all done")

//please include missing logger statements, I skipeed them for simplicity
//probably better if the last two setps could be implemented as private helper functions.

paweldigital · 2026-07-07T09:20:14Z

  val hipBaseUrl: String = servicesConfig.baseUrl("hip")
  val hipAuthToken: HipAuthToken = HipAuthToken(config.get[String]("microservice.services.hip.authorization-token"))
+  val enableUnsetRiskingResponses: Boolean = servicesConfig.getBoolean("features.enable-unset-risking-responses")
+  val applicationIdsForUnsettingRiskingResponses: Seq[String] = config.get[Seq[String]]("applicationIdsForUnsettingRiskingResponses")


Could you please change the type signature to Seq[ApplicationReference]?

Also, since these are application references, could you rename this value and the corresponding config property to:

val applicationReferencesToBeApproved: Seq[ApplicationReference]

and in appconfig:

applicationReferencesToBeApproved = []

geoffreywatson added 2 commits July 6, 2026 13:26

[APB-11788] special case to Approve selected previously failed applic…

ce03e68

…ations

cleanup

7ee54a9

nubz and others added 8 commits July 6, 2026 13:39

APB-11555 details page for fixable individual journey (sync from BE) (#…

7dcd4c2

…142)

[APB-11788] special case to Approve selected previously failed applic…

46d8633

…ations # Conflicts: # conf/application.conf

cleanup

a965173

resolve conflict

1715aca

Merge branch 'APB-11788' of github.com:hmrc/agent-registration-riskin…

d498c0f

…g into APB-11788 Signed-off-by: geoffreywatson <9043337+geoffreywatson@users.noreply.github.com>

missing imports

de02993

remove duplicate config

1b0125a

geoffreywatson changed the title ~~Apb 11788~~ Apb 11788 - Special Case to allow setting previously Failed applications to be Success Jul 6, 2026

gitwojciech reviewed Jul 6, 2026

View reviewed changes

add trigger for special case into the risking runner follwing refactor

e406c4f

gitwojciech reviewed Jul 6, 2026

View reviewed changes

Wojciech's suggestions

2b8e12d

gitwojciech reviewed Jul 7, 2026

View reviewed changes

further suggestions

b0fb94a

paweldigital reviewed Jul 7, 2026

View reviewed changes

clean up imports

e71beb1

paweldigital reviewed Jul 7, 2026

View reviewed changes

paweldigital mentioned this pull request Jul 7, 2026

[APB-11788][GW] special case allow previously failed applications to … #145

Open


		import java.time.ZoneOffset

		class BackendNotificationServiceFeatureFlagSpec


		riskingResultsService.specialCaseApprovePreviouslyFailedApplications().futureValue

		val modified = applicationForRiskingRepo.findById(ApplicationReference(failedNonFixableAppRef)).futureValue

Conversation

geoffreywatson commented Jul 6, 2026

Uh oh!

platops-pr-bot commented Jul 6, 2026

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!

paweldigital Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

paweldigital Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

5 participants

paweldigital Jul 7, 2026 •

edited

Loading

paweldigital Jul 7, 2026 •

edited

Loading