Merge pull request #63 from hmrc/model-refactor

Model refactor

app/controllers/CheckYourAnswersController.scala

@@ -63,7 +63,7 @@ class CheckYourAnswersController @Inject()(
       val (maybeErrors, maybeModel) = TraderProfile.build(request.userAnswers).pad
 
       val errors = maybeErrors.map { errors =>
-        errors.toChain.toList.map(_.path).mkString(", ")
+        errors.toChain.toList.map(_.message).mkString(", ")
       }.getOrElse("")
 
       maybeModel.map { model =>

app/models/TraderProfile.scala

@@ -16,11 +16,10 @@
 
 package models
 
-import cats.data.{Ior, IorNec}
+import cats.data.{Ior, IorNec, NonEmptyChain}
 import cats.implicits._
 import pages.{HasNiphlPage, HasNirmsPage, NiphlNumberPage, NirmsNumberPage, UkimsNumberPage}
 import play.api.libs.json.{Json, OFormat}
-import queries.Query
 
 final case class TraderProfile(
                                 ukimsNumber: String,
@@ -32,22 +31,26 @@ object TraderProfile {
 
   implicit lazy val format: OFormat[TraderProfile] = Json.format
 
-  def build(answers: UserAnswers): IorNec[Query, TraderProfile] =
+  def build(answers: UserAnswers): IorNec[ValidationError, TraderProfile] =
     (
       answers.getIor(UkimsNumberPage),
       getNirms(answers),
       getNiphl(answers)
     ).parMapN(TraderProfile.apply)
 
-  private def getNirms(answers: UserAnswers): IorNec[Query, Option[String]] =
+  private def getNirms(answers: UserAnswers): IorNec[ValidationError, Option[String]] =
     answers.getIor(HasNirmsPage).flatMap {
-      case true  => answers.getIor(NirmsNumberPage).map(Some(_))
-      case false => Ior.Right(None)
+      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[Query, Option[String]] =
+  private def getNiphl(answers: UserAnswers): IorNec[ValidationError, Option[String]] =
     answers.getIor(HasNiphlPage).flatMap {
-      case true  => answers.getIor(NiphlNumberPage).map(Some(_))
-      case false => Ior.Right(None)
+      case true  =>
+        answers.getIor(NiphlNumberPage).map(Some(_))
+      case false =>
+        if (answers.isDefined(NiphlNumberPage)) Ior.Left(NonEmptyChain.one(UnexpectedPage(NiphlNumberPage))) else Ior.Right(None)
     }
 }

app/models/UserAnswers.scala

@@ -34,8 +34,8 @@ 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[Query, A] =
-    get(page).toRightIor(NonEmptyChain.one(page))
+  def getIor[A](page: Gettable[A])(implicit rds: Reads[A]): IorNec[ValidationError, A] =
+    get(page).toRightIor(NonEmptyChain.one(PageMissing(page)))
 
   def set[A](page: Settable[A], value: A)(implicit writes: Writes[A]): Try[UserAnswers] = {
 

app/models/ValidationError.scala

@@ -0,0 +1,35 @@
+/*
+ * Copyright 2024 HM Revenue & Customs
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package models
+
+import queries.Query
+
+sealed trait ValidationError {
+
+  val query: Query
+  val message: String
+}
+
+final case class PageMissing(query: Query) extends ValidationError {
+
+  val message: String = s"Page missing: ${query.path}"
+}
+
+final case class UnexpectedPage(query: Query) extends ValidationError {
+
+  val message: String = s"Unexpected page: ${query.path}"
+}

test/models/TraderProfileSpec.scala

@@ -68,9 +68,9 @@ class TraderProfileSpec extends AnyFreeSpec with Matchers with TryValues with Op
 
         data must not be defined
         errors.value.toChain.toList must contain theSameElementsAs Seq(
-          UkimsNumberPage,
-          HasNirmsPage,
-          HasNiphlPage
+          PageMissing(UkimsNumberPage),
+          PageMissing(HasNirmsPage),
+          PageMissing(HasNiphlPage)
         )
       }
 
@@ -85,7 +85,7 @@ class TraderProfileSpec extends AnyFreeSpec with Matchers with TryValues with Op
         val (errors, data) = TraderProfile.build(answers).pad
 
         data must not be defined
-        errors.value.toChain.toList must contain only NirmsNumberPage
+        errors.value.toChain.toList must contain only PageMissing(NirmsNumberPage)
       }
 
       "when the user said they have a Niphl number but it is missing" in {
@@ -99,7 +99,26 @@ class TraderProfileSpec extends AnyFreeSpec with Matchers with TryValues with Op
         val (errors, data) = TraderProfile.build(answers).pad
 
         data must not be defined
-        errors.value.toChain.toList must contain only NiphlNumberPage
+        errors.value.toChain.toList must contain only PageMissing(NiphlNumberPage)
+      }
+
+      "when the user said they don't have optional data but it is present" in {
+
+        val answers =
+          UserAnswers("id")
+            .set(UkimsNumberPage, "1").success.value
+            .set(HasNirmsPage, false).success.value
+            .set(NirmsNumberPage, "2").success.value
+            .set(HasNiphlPage, false).success.value
+            .set(NiphlNumberPage, "3").success.value
+
+        val (errors, data) = TraderProfile.build(answers).pad
+
+        data must not be defined
+        errors.value.toChain.toList must contain theSameElementsAs Seq(
+          UnexpectedPage(NirmsNumberPage),
+          UnexpectedPage(NiphlNumberPage)
+        )
       }
     }
   }