Merge pull request #56 from hmrc/CYA-validation

Cya validation

app/controllers/CheckYourAnswersController.scala

@@ -16,13 +16,15 @@
 
 package controllers
 
+import cats.data
 import com.google.inject.Inject
 import connectors.RouterConnector
 import controllers.actions.{DataRequiredAction, DataRetrievalAction, IdentifierAction}
 import logging.Logging
-import models.TraderProfile
+import models.{TraderProfile, ValidationError}
 import play.api.i18n.{I18nSupport, MessagesApi}
-import play.api.mvc.{Action, AnyContent, MessagesControllerComponents}
+import play.api.mvc.{Action, AnyContent, MessagesControllerComponents, Result}
+import uk.gov.hmrc.play.bootstrap.binders.RedirectUrl
 import uk.gov.hmrc.play.bootstrap.frontend.controller.FrontendBaseController
 import viewmodels.checkAnswers.{HasNiphlSummary, HasNirmsSummary, NiphlNumberSummary, NirmsNumberSummary, UkimsNumberSummary}
 import viewmodels.govuk.summarylist._
@@ -43,36 +45,40 @@ class CheckYourAnswersController @Inject()(
 
   def onPageLoad(): Action[AnyContent] = (identify andThen getData andThen requireData) {
     implicit request =>
-
-      val list = SummaryListViewModel(
-        rows = Seq(
-          UkimsNumberSummary.row(request.userAnswers),
-          HasNirmsSummary.row(request.userAnswers),
-          NirmsNumberSummary.row(request.userAnswers),
-          HasNiphlSummary.row(request.userAnswers),
-          NiphlNumberSummary.row(request.userAnswers)
-        ).flatten
-      )
-
-      Ok(view(list))
+      TraderProfile.build(request.userAnswers) match {
+        case Right(_) =>
+        val list = SummaryListViewModel(
+          rows = Seq(
+            UkimsNumberSummary.row(request.userAnswers),
+            HasNirmsSummary.row(request.userAnswers),
+            NirmsNumberSummary.row(request.userAnswers),
+            HasNiphlSummary.row(request.userAnswers),
+            NiphlNumberSummary.row(request.userAnswers)
+          ).flatten
+        )
+        Ok(view(list))
+        case Left(errors) => logErrorsAndContinue(errors)
+      }
   }
 
   def onSubmit(): Action[AnyContent] = (identify andThen getData andThen requireData).async {
     implicit request =>
 
-      val (maybeErrors, maybeModel) = TraderProfile.build(request.userAnswers).pad
-
-      val errors = maybeErrors.map { errors =>
-        errors.toChain.toList.map(_.message).mkString(", ")
-      }.getOrElse("")
-
-      maybeModel.map { model =>
+      TraderProfile.build(request.userAnswers) match {
+        case Right(model) =>
         routerConnector.submitTraderProfile(model).map { _ =>
           Redirect(routes.HomePageController.onPageLoad())
         }
-      }.getOrElse {
-        logger.warn(s"Unable to create Trader profile.  Missing pages: $errors")
-        Future.successful(Redirect(routes.JourneyRecoveryController.onPageLoad()))
+        case Left(errors) => Future.successful(logErrorsAndContinue(errors))
       }
   }
+
+  def logErrorsAndContinue(errors: data.NonEmptyChain[ValidationError]): Result = {
+    val errorMessages = errors.toChain.toList.map(_.message).mkString(", ")
+
+    val continueUrl = RedirectUrl(routes.ProfileSetupController.onPageLoad().url)
+
+    logger.warn(s"Unable to create Trader profile.  Missing pages: $errorMessages")
+    Redirect(routes.JourneyRecoveryController.onPageLoad(Some(continueUrl)))
+  }
 }

app/models/TraderProfile.scala

@@ -16,9 +16,9 @@
 
 package models
 
-import cats.data.{Ior, IorNec, NonEmptyChain}
+import cats.data.{EitherNec, NonEmptyChain}
 import cats.implicits._
-import pages.{HasNiphlPage, HasNirmsPage, NiphlNumberPage, NirmsNumberPage, UkimsNumberPage}
+import pages.{HasNiphlPage, HasNirmsPage, NiphlNumberPage, NirmsNumberPage, QuestionPage, UkimsNumberPage}
 import play.api.libs.json.{Json, OFormat}
 
 final case class TraderProfile(
@@ -31,26 +31,27 @@ object TraderProfile {
 
   implicit lazy val format: OFormat[TraderProfile] = Json.format
 
-  def build(answers: UserAnswers): IorNec[ValidationError, TraderProfile] =
+  def build(answers: UserAnswers): EitherNec[ValidationError, TraderProfile] =
     (
-      answers.getIor(UkimsNumberPage),
+      answers.getPageValue(UkimsNumberPage),
       getNirms(answers),
       getNiphl(answers)
     ).parMapN(TraderProfile.apply)
 
-  private def getNirms(answers: UserAnswers): IorNec[ValidationError, Option[String]] =
-    answers.getIor(HasNirmsPage).flatMap {
-      case true  =>
-        answers.getIor(NirmsNumberPage).map(Some(_))
-      case false =>
-        if (answers.isDefined(NirmsNumberPage)) Ior.Left(NonEmptyChain.one(UnexpectedPage(NirmsNumberPage))) else Ior.Right(None)
-    }
-
-  private def getNiphl(answers: UserAnswers): IorNec[ValidationError, Option[String]] =
-    answers.getIor(HasNiphlPage).flatMap {
-      case true  =>
-        answers.getIor(NiphlNumberPage).map(Some(_))
-      case false =>
-        if (answers.isDefined(NiphlNumberPage)) Ior.Left(NonEmptyChain.one(UnexpectedPage(NiphlNumberPage))) else Ior.Right(None)
+  private def getNirms(answers: UserAnswers): EitherNec[ValidationError, Option[String]] =
+    getNumber(answers, HasNirmsPage, NirmsNumberPage)
+
+  private def getNiphl(answers: UserAnswers): EitherNec[ValidationError, Option[String]] =
+    getNumber(answers, HasNiphlPage, NiphlNumberPage)
+
+  private def getNumber(
+                         answers: UserAnswers,
+                         questionPage: QuestionPage[Boolean],
+                         numberPage: QuestionPage[String]
+                       ): EitherNec[ValidationError, Option[String]] =
+    answers.getPageValue(questionPage) match {
+      case Right(true)  => answers.getPageValue(numberPage).map(Some(_))
+      case Right(false)  => answers.unexpectedValueDefined(answers, numberPage)
+      case Left(errors) =>  Left(errors)
     }
 }

app/models/UserAnswers.scala

@@ -16,8 +16,9 @@
 
 package models
 
-import cats.data.{IorNec, NonEmptyChain}
+import cats.data.{EitherNec, IorNec, NonEmptyChain}
 import cats.implicits._
+import pages.{Page, QuestionPage}
 import play.api.libs.json._
 import queries.{Gettable, Query, Settable}
 import uk.gov.hmrc.mongo.play.json.formats.MongoJavatimeFormats
@@ -34,8 +35,11 @@ final case class UserAnswers(
   def get[A](page: Gettable[A])(implicit rds: Reads[A]): Option[A] =
     Reads.optionNoError(Reads.at(page.path)).reads(data).getOrElse(None)
 
-  def getIor[A](page: Gettable[A])(implicit rds: Reads[A]): IorNec[ValidationError, A] =
-    get(page).toRightIor(NonEmptyChain.one(PageMissing(page)))
+  def getPageValue[A](page: Gettable[A])(implicit rds: Reads[A]): EitherNec[ValidationError, A] =
+    get(page).map(Right(_)).getOrElse(Left(NonEmptyChain.one(PageMissing(page))))
+
+  def unexpectedValueDefined(answers: UserAnswers, page: Gettable[_]): EitherNec[ValidationError, Option[Nothing]] =
+    if (answers.isDefined(page)) Left(NonEmptyChain.one(UnexpectedPage(page))) else Right(None)
 
   def set[A](page: Settable[A], value: A)(implicit writes: Writes[A]): Try[UserAnswers] = {
 

app/views/CheckYourAnswersView.scala.html

@@ -16,7 +16,9 @@
 
 @this(
     layout: templates.Layout,
-    govukSummaryList: GovukSummaryList
+    govukSummaryList: GovukSummaryList,
+    formHelper: FormWithCSRF,
+    govukButton: GovukButton
 )
 
 @(list: SummaryList)(implicit request: Request[_], messages: Messages)
@@ -33,4 +35,8 @@ <h2 class="govuk-heading-m">@messages("checkYourAnswers.h2")</h2>
             @messages("checkYourAnswers.warningText")
         </strong>
     </div>
+
+    @formHelper(action = routes.CheckYourAnswersController.onSubmit) {
+        @govukButton(ButtonViewModel(messages("site.continue")))
+    }
 }

conf/messages.en

@@ -124,8 +124,8 @@ ukimsNumber.hint = For example, XIUKIM47699357400020231115081800
 ukimsNumber.linkText = I am not UKIMS registered
 ukimsNumber.error.required = Enter your UKIMS number
 ukimsNumber.error.invalidFormat = Enter your UKIMS number in the correct format
-ukimsNumber.checkYourAnswersLabel = UKIMS Number
-ukimsNumber.change.hidden = UKIMS Number
+ukimsNumber.checkYourAnswersLabel = UKIMS number
+ukimsNumber.change.hidden = UKIMS number
 
 categoryGuidance.title = Categorisation
 categoryGuidance.heading = Categorisation

test/controllers/CheckYourAnswersControllerSpec.scala

@@ -23,10 +23,14 @@ import org.apache.pekko.Done
 import org.mockito.ArgumentMatchers.{any, eq => eqTo}
 import org.mockito.Mockito.{never, times, verify, when}
 import org.scalatestplus.mockito.MockitoSugar
-import pages.{HasNiphlPage, HasNirmsPage, UkimsNumberPage}
+import pages.{HasNiphlPage, HasNirmsPage, NiphlNumberPage, NirmsNumberPage, UkimsNumberPage}
+import play.api.Application
 import play.api.test.FakeRequest
 import play.api.inject.bind
 import play.api.test.Helpers._
+import uk.gov.hmrc.govukfrontend.views.Aliases.SummaryList
+import uk.gov.hmrc.play.bootstrap.binders.RedirectUrl
+import viewmodels.checkAnswers.{HasNiphlSummary, HasNirmsSummary, NiphlNumberSummary, NirmsNumberSummary, UkimsNumberSummary}
 import viewmodels.govuk.SummaryListFluency
 import views.html.CheckYourAnswersView
 
@@ -36,40 +40,95 @@ class CheckYourAnswersControllerSpec extends SpecBase with SummaryListFluency wi
 
   "Check Your Answers Controller" - {
 
+    def createChangeList(userAnswers: UserAnswers, app: Application): SummaryList = SummaryListViewModel(
+      rows = Seq(
+        UkimsNumberSummary.row(userAnswers)(messages(app)),
+        HasNirmsSummary.row(userAnswers)(messages(app)),
+        NirmsNumberSummary.row(userAnswers)(messages(app)),
+        HasNiphlSummary.row(userAnswers)(messages(app)),
+        NiphlNumberSummary.row(userAnswers)(messages(app))
+      ).flatten
+    )
+
     "for a GET" - {
 
-      "must return OK and the correct view" in {
+      "must return OK and the correct view with valid mandatory data" in {
+
+        val userAnswers = UserAnswers(userAnswersId)
+          .set(UkimsNumberPage, "1").success.value
+          .set(HasNirmsPage, false).success.value
+          .set(HasNiphlPage, false).success.value
 
-        val application = applicationBuilder(userAnswers = Some(emptyUserAnswers)).build()
+        val application = applicationBuilder(userAnswers = Some(userAnswers)).build()
 
         running(application) {
           val request = FakeRequest(GET, routes.CheckYourAnswersController.onPageLoad.url)
 
           val result = route(application, request).value
 
           val view = application.injector.instanceOf[CheckYourAnswersView]
-          val list = SummaryListViewModel(Seq.empty)
+          val list = createChangeList(userAnswers, application)
 
           status(result) mustEqual OK
           contentAsString(result) mustEqual view(list)(request, messages(application)).toString
         }
       }
 
-      "must redirect to Journey Recovery if no existing data is found" in {
+      "must return OK and the correct view with all data (including optional)" in {
 
-        val application = applicationBuilder(userAnswers = None).build()
+        val userAnswers = UserAnswers(userAnswersId)
+          .set(UkimsNumberPage, "1").success.value
+          .set(HasNirmsPage, true).success.value
+          .set(NirmsNumberPage, "2").success.value
+          .set(HasNiphlPage, true).success.value
+          .set(NiphlNumberPage, "3").success.value
+
+        val application = applicationBuilder(userAnswers = Some(userAnswers)).build()
 
         running(application) {
           val request = FakeRequest(GET, routes.CheckYourAnswersController.onPageLoad.url)
 
           val result = route(application, request).value
 
-          status(result) mustEqual SEE_OTHER
-          redirectLocation(result).value mustEqual routes.JourneyRecoveryController.onPageLoad().url
+          val view = application.injector.instanceOf[CheckYourAnswersView]
+          val list = createChangeList(userAnswers, application)
+
+          status(result) mustEqual OK
+          contentAsString(result) mustEqual view(list)(request, messages(application)).toString
         }
       }
+
+      "must redirect to Journey Recovery if no answers are found" in {
+
+        val application = applicationBuilder(Some(emptyUserAnswers)).build()
+        val continueUrl = RedirectUrl(routes.ProfileSetupController.onPageLoad().url)
+
+        running(application) {
+          val request = FakeRequest(GET, routes.CheckYourAnswersController.onPageLoad.url)
+
+          val result = route(application, request).value
+
+          status(result) mustEqual SEE_OTHER
+          redirectLocation(result).value mustEqual routes.JourneyRecoveryController.onPageLoad(Some(continueUrl)).url
+
+      }
     }
 
+    "must redirect to Journey Recovery if no existing data is found" in {
+
+      val application = applicationBuilder(userAnswers = None).build()
+
+      running(application) {
+        val request = FakeRequest(GET, routes.CheckYourAnswersController.onPageLoad.url)
+
+        val result = route(application, request).value
+
+        status(result) mustEqual SEE_OTHER
+        redirectLocation(result).value mustEqual routes.JourneyRecoveryController.onPageLoad().url
+      }
+    }
+  }
+
     "for a POST" - {
 
       "when user answers can create a valid trader profile" - {
@@ -109,6 +168,7 @@ class CheckYourAnswersControllerSpec extends SpecBase with SummaryListFluency wi
         "must not submit anything, and redirect to Journey Recovery" in {
 
           val mockConnector = mock[RouterConnector]
+          val continueUrl = RedirectUrl(routes.ProfileSetupController.onPageLoad().url)
 
           val application =
             applicationBuilder(userAnswers = Some(UserAnswers("")))
@@ -121,7 +181,7 @@ class CheckYourAnswersControllerSpec extends SpecBase with SummaryListFluency wi
             val result = route(application, request).value
 
             status(result) mustEqual SEE_OTHER
-            redirectLocation(result).value mustEqual routes.JourneyRecoveryController.onPageLoad().url
+            redirectLocation(result).value mustEqual routes.JourneyRecoveryController.onPageLoad(Some(continueUrl)).url
             verify(mockConnector, never()).submitTraderProfile(any())(any())
           }
         }

test/controllers/CommodityCodeControllerSpec.scala

@@ -35,12 +35,12 @@ import scala.concurrent.Future
 
 class CommodityCodeControllerSpec extends SpecBase with MockitoSugar {
 
-  def onwardRoute = Call("GET", "/foo")
+  private def onwardRoute = Call("GET", "/foo")
 
   val formProvider = new CommodityCodeFormProvider()
-  val form = formProvider()
+  private val form = formProvider()
 
-  lazy val commodityCodeRoute = routes.CommodityCodeController.onPageLoad(NormalMode).url
+  private lazy val commodityCodeRoute = routes.CommodityCodeController.onPageLoad(NormalMode).url
 
   "CommodityCode Controller" - {
 

test/controllers/UkimsNumberControllerSpec.scala

@@ -35,12 +35,12 @@ import scala.concurrent.Future
 
 class UkimsNumberControllerSpec extends SpecBase with MockitoSugar {
 
-  def onwardRoute = Call("GET", "/foo")
+  private def onwardRoute = Call("GET", "/foo")
 
   val formProvider = new UkimsNumberFormProvider()
-  val form = formProvider()
+  private val form = formProvider()
 
-  lazy val ukimsNumberRoute = routes.UkimsNumberController.onPageLoad(NormalMode).url
+  private lazy val ukimsNumberRoute = routes.UkimsNumberController.onPageLoad(NormalMode).url
 
   "UkimsNumber Controller" - {
 

test/controllers/actions/DataRetrievalActionSpec.scala

@@ -57,7 +57,7 @@ class DataRetrievalActionSpec extends SpecBase with MockitoSugar {
         when(sessionRepository.get("id")) thenReturn Future(Some(UserAnswers("id")))
         val action = new Harness(sessionRepository)
 
-        val result = action.callTransform(new IdentifierRequest(FakeRequest(), "id", "eori")).futureValue
+        val result = action.callTransform(IdentifierRequest(FakeRequest(), "id", "eori")).futureValue
 
         result.userAnswers mustBe defined
       }