diff --git a/app/controllers/CyaUpdateRecordController.scala b/app/controllers/CyaUpdateRecordController.scala index fe06175e..8915e5c6 100644 --- a/app/controllers/CyaUpdateRecordController.scala +++ b/app/controllers/CyaUpdateRecordController.scala @@ -22,15 +22,12 @@ import com.google.inject.Inject import config.FrontendAppConfig import connectors.{GoodsRecordConnector, OttConnector} import controllers.actions.{DataRequiredAction, DataRetrievalAction, IdentifierAction} -import models.requests.DataRequest -import models.router.requests.PutRecordRequest -import models.router.responses.GetGoodsRecordResponse -import models.{CheckMode, Commodity, Country, NormalMode, UpdateGoodsRecord, UserAnswers, ValidationError} +import models.{CheckMode, Country, NormalMode, UpdateGoodsRecord, UserAnswers, ValidationError} import navigation.Navigator import org.apache.pekko.Done import pages._ import play.api.i18n.MessagesApi -import play.api.mvc.{Action, AnyContent, MessagesControllerComponents, Request, Result} +import play.api.mvc.{Action, AnyContent, MessagesControllerComponents, Request} import queries.CountriesQuery import repositories.SessionRepository import services.AuditService @@ -68,9 +65,8 @@ class CyaUpdateRecordController @Inject() ( .getRecord(request.eori, recordId) .flatMap { recordResponse => UpdateGoodsRecord - .buildCountryOfOrigin( + .validateCountryOfOrigin( request.userAnswers, - request.eori, recordId, recordResponse.category.isDefined ) match { @@ -221,9 +217,9 @@ class CyaUpdateRecordController @Inject() ( override def getMessage: String = s"$errorMessage Missing pages: $errorsAsString" } - private def updateTraderReferenceIfValueChanged[T]( - newValue: T, - oldValue: T, + private def updateGoodsRecordIfValueChanged( + newValue: String, + oldValue: String, newUpdateGoodsRecord: UpdateGoodsRecord )(implicit hc: HeaderCarrier): Future[Done] = if (newValue != oldValue) { @@ -243,93 +239,17 @@ class CyaUpdateRecordController @Inject() ( Future.successful(Done) } - private def updateCommodityCodeAndSession(recordId: String, commodity: Commodity, oldRecord: GetGoodsRecordResponse)( - implicit request: DataRequest[AnyContent] - ): Future[Result] = - for { - // TODO: remove this flag when EIS has implemented the PATCH method - TGP-2417 and keep the call to putGoodsRecord as default - _ <- if (config.useEisPatchMethod) { - goodsRecordConnector.putGoodsRecord( - PutRecordRequest( - actorId = request.eori, - traderRef = oldRecord.traderRef, - comcode = commodity.commodityCode, - goodsDescription = oldRecord.goodsDescription, - countryOfOrigin = oldRecord.countryOfOrigin, - category = None, - assessments = oldRecord.assessments, - supplementaryUnit = oldRecord.supplementaryUnit, - measurementUnit = oldRecord.measurementUnit, - comcodeEffectiveFromDate = oldRecord.comcodeEffectiveFromDate, - comcodeEffectiveToDate = oldRecord.comcodeEffectiveToDate - ), - recordId - ) - } else { - goodsRecordConnector.updateGoodsRecord( - UpdateGoodsRecord(request.eori, recordId, commodityCode = Some(commodity)) - ) - } - - updatedAnswersWithChange <- - Future.fromTry(request.userAnswers.remove(HasCommodityCodeChangePage(recordId))) - updatedAnswers <- Future.fromTry(updatedAnswersWithChange.remove(CommodityCodeUpdatePage(recordId))) - _ <- sessionRepository.set(updatedAnswers) - } yield Redirect(navigator.nextPage(CyaUpdateRecordPage(recordId), NormalMode, updatedAnswers)) - - private def updateCountryOfOriginAndSession( - recordId: String, - updateGoodsRecord: UpdateGoodsRecord, - oldRecord: GetGoodsRecordResponse - )(implicit - request: DataRequest[AnyContent] - ): Future[Result] = - for { - // TODO: remove this flag when EIS has implemented the PATCH method - TGP-2417 and keep the call to putGoodsRecord as default - _ <- if (config.useEisPatchMethod) { - goodsRecordConnector.putGoodsRecord( - PutRecordRequest( - actorId = request.eori, - traderRef = oldRecord.traderRef, - comcode = oldRecord.comcode, - goodsDescription = oldRecord.goodsDescription, - countryOfOrigin = updateGoodsRecord.countryOfOrigin.get, - category = None, - assessments = oldRecord.assessments, - supplementaryUnit = oldRecord.supplementaryUnit, - measurementUnit = oldRecord.measurementUnit, - comcodeEffectiveFromDate = oldRecord.comcodeEffectiveFromDate, - comcodeEffectiveToDate = oldRecord.comcodeEffectiveToDate - ), - recordId - ) - } else { - goodsRecordConnector.updateGoodsRecord(updateGoodsRecord) - } - - updatedAnswersWithChange <- - Future.fromTry(request.userAnswers.remove(HasCountryOfOriginChangePage(recordId))) - updatedAnswers <- Future.fromTry(updatedAnswersWithChange.remove(CountryOfOriginUpdatePage(recordId))) - _ <- sessionRepository.set(updatedAnswers) - } yield Redirect(navigator.nextPage(CyaUpdateRecordPage(recordId), NormalMode, updatedAnswers)) - def onSubmitTraderReference(recordId: String): Action[AnyContent] = (identify andThen getData andThen requireData).async { implicit request => (for { - traderReference <- handleValidateError(UpdateGoodsRecord.validateTraderReference(request.userAnswers, recordId)) - _ = auditService.auditFinishUpdateGoodsRecord( - recordId, - request.affinityGroup, - UpdateGoodsRecord(request.eori, recordId, traderReference = Some(traderReference)) - ) - oldRecord <- goodsRecordConnector.getRecord(request.eori, recordId) - _ <- updateTraderReferenceIfValueChanged( - traderReference, - oldRecord.traderRef, - UpdateGoodsRecord(request.eori, recordId, traderReference = Some(traderReference)) - ) - updatedAnswers <- Future.fromTry(request.userAnswers.remove(TraderReferenceUpdatePage(recordId))) - _ <- sessionRepository.set(updatedAnswers) + traderReference <- handleValidateError(UpdateGoodsRecord.validateTraderReference(request.userAnswers, recordId)) + updateGoodsRecord <- + Future.successful(UpdateGoodsRecord(request.eori, recordId, traderReference = Some(traderReference))) + _ = auditService.auditFinishUpdateGoodsRecord(recordId, request.affinityGroup, updateGoodsRecord) + oldRecord <- goodsRecordConnector.getRecord(request.eori, recordId) + _ <- updateGoodsRecordIfValueChanged(traderReference, oldRecord.traderRef, updateGoodsRecord) + updatedAnswers <- Future.fromTry(request.userAnswers.remove(TraderReferenceUpdatePage(recordId))) + _ <- sessionRepository.set(updatedAnswers) } yield Redirect(navigator.nextPage(CyaUpdateRecordPage(recordId), NormalMode, updatedAnswers))).recover { case e: GoodsRecordBuildFailure => logErrorsAndContinue(e.getMessage, routes.SingleRecordController.onPageLoad(recordId)) @@ -338,108 +258,72 @@ class CyaUpdateRecordController @Inject() ( def onSubmitCountryOfOrigin(recordId: String): Action[AnyContent] = (identify andThen getData andThen requireData).async { implicit request => - goodsRecordConnector - .getRecord(request.eori, recordId) - .flatMap { recordResponse => - UpdateGoodsRecord - .buildCountryOfOrigin( - request.userAnswers, - request.eori, - recordId, - recordResponse.category.isDefined - ) match { - case Right(model) => - auditService.auditFinishUpdateGoodsRecord(recordId, request.affinityGroup, model) - updateCountryOfOriginAndSession(recordId, model, recordResponse) - case Left(errors) => - Future.successful( - logErrorsAndContinue( - errorMessage, - routes.SingleRecordController.onPageLoad(recordId), - errors - ) - ) - } - } - .recoverWith { case e: Exception => - logger.error(s"Unable to fetch record $recordId: ${e.getMessage}") - Future.successful( - navigator.journeyRecovery(Some(RedirectUrl(routes.SingleRecordController.onPageLoad(recordId).url))) + (for { + oldRecord <- goodsRecordConnector.getRecord(request.eori, recordId) + countryOfOrigin <- + handleValidateError( + UpdateGoodsRecord.validateCountryOfOrigin(request.userAnswers, recordId, oldRecord.category.isDefined) ) - } + updateGoodsRecord <- + Future.successful(UpdateGoodsRecord(request.eori, recordId, countryOfOrigin = Some(countryOfOrigin))) + _ = auditService.auditFinishUpdateGoodsRecord(recordId, request.affinityGroup, updateGoodsRecord) + _ <- updateGoodsRecordIfValueChanged(countryOfOrigin, oldRecord.countryOfOrigin, updateGoodsRecord) + updatedAnswersWithChange <- Future.fromTry(request.userAnswers.remove(HasCountryOfOriginChangePage(recordId))) + updatedAnswers <- Future.fromTry(updatedAnswersWithChange.remove(CountryOfOriginUpdatePage(recordId))) + _ <- sessionRepository.set(updatedAnswers) + } yield Redirect(navigator.nextPage(CyaUpdateRecordPage(recordId), NormalMode, updatedAnswers))).recover { + case e: GoodsRecordBuildFailure => + logErrorsAndContinue(e.getMessage, routes.SingleRecordController.onPageLoad(recordId)) + } } def onSubmitGoodsDescription(recordId: String): Action[AnyContent] = (identify andThen getData andThen requireData).async { implicit request => - UpdateGoodsRecord.validateGoodsDescription(request.userAnswers, recordId) match { - case Right(goodsDescription) => - auditService.auditFinishUpdateGoodsRecord( - recordId, - request.affinityGroup, - UpdateGoodsRecord(request.eori, recordId, goodsDescription = Some(goodsDescription)) - ) - for { - // TODO: remove this flag when EIS has implemented the PATCH method - TGP-2417 and keep the call to patchGoodsRecord as default - _ <- if (config.useEisPatchMethod) { - goodsRecordConnector.patchGoodsRecord( - UpdateGoodsRecord(request.eori, recordId, goodsDescription = Some(goodsDescription)) - ) - } else { - goodsRecordConnector.updateGoodsRecord( - UpdateGoodsRecord(request.eori, recordId, goodsDescription = Some(goodsDescription)) - ) - } - - updatedAnswers <- Future.fromTry(request.userAnswers.remove(GoodsDescriptionUpdatePage(recordId))) - _ <- sessionRepository.set(updatedAnswers) - } yield Redirect(navigator.nextPage(CyaUpdateRecordPage(recordId), NormalMode, updatedAnswers)) - case Left(errors) => - Future.successful( - logErrorsAndContinue( - errorMessage, - routes.SingleRecordController.onPageLoad(recordId), - errors - ) - ) + (for { + goodsDescription <- + handleValidateError(UpdateGoodsRecord.validateGoodsDescription(request.userAnswers, recordId)) + updateGoodsRecord <- + Future.successful(UpdateGoodsRecord(request.eori, recordId, goodsDescription = Some(goodsDescription))) + _ = auditService.auditFinishUpdateGoodsRecord(recordId, request.affinityGroup, updateGoodsRecord) + oldRecord <- goodsRecordConnector.getRecord(request.eori, recordId) + _ <- updateGoodsRecordIfValueChanged(goodsDescription, oldRecord.goodsDescription, updateGoodsRecord) + updatedAnswers <- Future.fromTry(request.userAnswers.remove(GoodsDescriptionUpdatePage(recordId))) + _ <- sessionRepository.set(updatedAnswers) + } yield Redirect(navigator.nextPage(CyaUpdateRecordPage(recordId), NormalMode, updatedAnswers))).recover { + case e: GoodsRecordBuildFailure => + logErrorsAndContinue(e.getMessage, routes.SingleRecordController.onPageLoad(recordId)) } } def onSubmitCommodityCode(recordId: String): Action[AnyContent] = (identify andThen getData andThen requireData).async { implicit request => - goodsRecordConnector - .getRecord(request.eori, recordId) - .flatMap { recordResponse => - val isCommCodeExpired = request.session.get(fromExpiredCommodityCodePage).contains("true") - UpdateGoodsRecord - .validateCommodityCode( - request.userAnswers, - recordId, - recordResponse.category.isDefined, - isCommCodeExpired - ) match { - case Right(commodity) => - auditService.auditFinishUpdateGoodsRecord( + (for { + oldRecord <- goodsRecordConnector.getRecord(request.eori, recordId) + commodity <- + handleValidateError( + UpdateGoodsRecord + .validateCommodityCode( + request.userAnswers, recordId, - request.affinityGroup, - UpdateGoodsRecord(request.eori, recordId, commodityCode = Some(commodity)) + oldRecord.category.isDefined, + request.session.get(fromExpiredCommodityCodePage).contains("true") ) - updateCommodityCodeAndSession(recordId, commodity, recordResponse) - case Left(errors) => - Future.successful( - logErrorsAndContinue( - errorMessage, - routes.SingleRecordController.onPageLoad(recordId), - errors - ) - ) - } - } - .recoverWith { case e: Exception => - logger.error(s"Unable to fetch record $recordId: ${e.getMessage}") - Future.successful( - navigator.journeyRecovery(Some(RedirectUrl(routes.SingleRecordController.onPageLoad(recordId).url))) ) - } + updateGoodsRecord <- + Future.successful(UpdateGoodsRecord(request.eori, recordId, commodityCode = Some(commodity))) + _ = auditService.auditFinishUpdateGoodsRecord(recordId, request.affinityGroup, updateGoodsRecord) + _ <- updateGoodsRecordIfValueChanged( + commodity.commodityCode, + oldRecord.comcode, + updateGoodsRecord + ) + updatedAnswersWithChange <- + Future.fromTry(request.userAnswers.remove(HasCommodityCodeChangePage(recordId))) + updatedAnswers <- Future.fromTry(updatedAnswersWithChange.remove(CommodityCodeUpdatePage(recordId))) + _ <- sessionRepository.set(updatedAnswers) + } yield Redirect(navigator.nextPage(CyaUpdateRecordPage(recordId), NormalMode, updatedAnswers))).recover { + case e: GoodsRecordBuildFailure => + logErrorsAndContinue(e.getMessage, routes.SingleRecordController.onPageLoad(recordId)) + } } - } diff --git a/app/models/UpdateGoodsRecord.scala b/app/models/UpdateGoodsRecord.scala index d9ded072..4b631c7e 100644 --- a/app/models/UpdateGoodsRecord.scala +++ b/app/models/UpdateGoodsRecord.scala @@ -17,7 +17,7 @@ package models import cats.data.{EitherNec, NonEmptyChain} -import cats.implicits.{catsSyntaxTuple2Parallel, catsSyntaxTuple3Parallel} +import cats.implicits.catsSyntaxTuple2Parallel import pages._ import play.api.libs.json.{Json, OFormat} import queries.CommodityUpdateQuery @@ -40,23 +40,15 @@ object UpdateGoodsRecord { implicit lazy val format: OFormat[UpdateGoodsRecord] = Json.format - def buildCountryOfOrigin( + def validateCountryOfOrigin( answers: UserAnswers, - eori: String, recordId: String, isCategorised: Boolean - ): EitherNec[ValidationError, UpdateGoodsRecord] = + ): EitherNec[ValidationError, String] = ( - Right(eori), Right(recordId), getCountryOfOrigin(answers, recordId, isCategorised) - ).parMapN((eori, recordId, value) => - UpdateGoodsRecord( - eori, - recordId, - countryOfOrigin = Some(value) - ) - ) + ).parMapN((_, value) => value) def validateGoodsDescription( answers: UserAnswers, diff --git a/test/controllers/CyaUpdateRecordControllerSpec.scala b/test/controllers/CyaUpdateRecordControllerSpec.scala index bbc48771..5a7d1f88 100644 --- a/test/controllers/CyaUpdateRecordControllerSpec.scala +++ b/test/controllers/CyaUpdateRecordControllerSpec.scala @@ -314,7 +314,7 @@ class CyaUpdateRecordControllerSpec extends SpecBase with SummaryListFluency wit } } - "must not submit anything when record is not found, and redirect to Journey Recovery" in { + "must not submit anything when record is not found, and must let the play error handler deal with connector failure" in { val mockGoodsRecordConnector = mock[GoodsRecordConnector] when(mockGoodsRecordConnector.getRecord(any(), any())(any())) thenReturn Future @@ -327,19 +327,14 @@ class CyaUpdateRecordControllerSpec extends SpecBase with SummaryListFluency wit running(application) { val request = FakeRequest(POST, postUrl) - - val result = route(application, request).value - - status(result) mustEqual SEE_OTHER - redirectLocation(result).value mustEqual - routes.JourneyRecoveryController - .onPageLoad(continueUrl = Some(RedirectUrl(journeyRecoveryContinueUrl))) - .url + intercept[RuntimeException] { + await(route(application, request).value) + } } } } - "must let the play error handler deal with connector failure" in { + "must let the play error handler deal with connector failure when updating" in { val userAnswers = emptyUserAnswers .set(page, answer) @@ -369,14 +364,9 @@ class CyaUpdateRecordControllerSpec extends SpecBase with SummaryListFluency wit running(application) { val request = FakeRequest(POST, postUrl) - - val result = route(application, request).value - - status(result) mustEqual SEE_OTHER - redirectLocation(result).value mustEqual - routes.JourneyRecoveryController - .onPageLoad(continueUrl = Some(RedirectUrl(journeyRecoveryContinueUrl))) - .url + intercept[RuntimeException] { + await(route(application, request).value) + } withClue("must call the audit connector with the supplied details") { verify(mockAuditService) @@ -507,6 +497,8 @@ class CyaUpdateRecordControllerSpec extends SpecBase with SummaryListFluency wit when(mockAppConfig.useEisPatchMethod).thenReturn(false) + when(mockConnector.getRecord(any(), any())(any())).thenReturn(Future.successful(record)) + when(mockConnector.updateGoodsRecord(any())(any())).thenReturn(Future.successful(Done)) when(mockAuditService.auditFinishUpdateGoodsRecord(any(), any(), any())(any)) .thenReturn(Future.successful(Done)) @@ -562,7 +554,7 @@ class CyaUpdateRecordControllerSpec extends SpecBase with SummaryListFluency wit } } - "must let the play error handler deal with connector failure" in { + "must let the play error handler deal with connector failure when updating" in { val userAnswers = emptyUserAnswers .set(page, answer) @@ -824,7 +816,7 @@ class CyaUpdateRecordControllerSpec extends SpecBase with SummaryListFluency wit } } - "must let the play error handler deal with connector failure" in { + "must let the play error handler deal with connector failure when updating" in { val userAnswers = emptyUserAnswers .set(page, answer) @@ -1159,7 +1151,7 @@ class CyaUpdateRecordControllerSpec extends SpecBase with SummaryListFluency wit } } - "must not submit anything when record is not found, and redirect to Journey Recovery" in { + "must not submit anything when record is not found, and must let the play error handler deal with connector failure" in { val mockGoodsRecordConnector = mock[GoodsRecordConnector] when(mockGoodsRecordConnector.getRecord(any(), any())(any())) thenReturn Future @@ -1172,19 +1164,14 @@ class CyaUpdateRecordControllerSpec extends SpecBase with SummaryListFluency wit running(application) { val request = FakeRequest(POST, postUrl) - - val result = route(application, request).value - - status(result) mustEqual SEE_OTHER - redirectLocation(result).value mustEqual - routes.JourneyRecoveryController - .onPageLoad(Some(RedirectUrl(journeyRecoveryContinueUrl))) - .url + intercept[RuntimeException] { + await(route(application, request).value) + } } } } - "must let the play error handler deal with connector failure " in { + "must let the play error handler deal with connector failure when updating" in { val userAnswers = emptyUserAnswers .set(page, testCommodity.commodityCode) .success @@ -1219,13 +1206,9 @@ class CyaUpdateRecordControllerSpec extends SpecBase with SummaryListFluency wit running(application) { val request = FakeRequest(POST, postUrl) - val result = route(application, request).value - - status(result) mustEqual SEE_OTHER - redirectLocation(result).value mustEqual - routes.JourneyRecoveryController - .onPageLoad(Some(RedirectUrl(journeyRecoveryContinueUrl))) - .url + intercept[RuntimeException] { + await(route(application, request).value) + } withClue("must call the audit connector with the supplied details") { verify(mockAuditService) diff --git a/test/models/UpdateGoodsRecordSpec.scala b/test/models/UpdateGoodsRecordSpec.scala index f81eee3d..7bdbc8d2 100644 --- a/test/models/UpdateGoodsRecordSpec.scala +++ b/test/models/UpdateGoodsRecordSpec.scala @@ -16,7 +16,7 @@ package models -import base.TestConstants.{testEori, testRecordId, userAnswersId} +import base.TestConstants.{testRecordId, userAnswersId} import org.scalatest.Inside.inside import org.scalatest.freespec.AnyFreeSpec import org.scalatest.matchers.must.Matchers @@ -43,9 +43,9 @@ class UpdateGoodsRecordSpec extends AnyFreeSpec with Matchers with TryValues wit .success .value - val result = UpdateGoodsRecord.buildCountryOfOrigin(answers, testEori, testRecordId, isCategorised = true) + val result = UpdateGoodsRecord.validateCountryOfOrigin(answers, testRecordId, isCategorised = true) - result mustBe Right(UpdateGoodsRecord(testEori, testRecordId, Some("CN"))) + result mustBe Right("CN") } "and all country of origin data is present when record is not categorised" in { @@ -56,9 +56,9 @@ class UpdateGoodsRecordSpec extends AnyFreeSpec with Matchers with TryValues wit .success .value - val result = UpdateGoodsRecord.buildCountryOfOrigin(answers, testEori, testRecordId, isCategorised = false) + val result = UpdateGoodsRecord.validateCountryOfOrigin(answers, testRecordId, isCategorised = false) - result mustBe Right(UpdateGoodsRecord(testEori, testRecordId, Some("CN"))) + result mustBe Right("CN") } "and all goods description data is present" in { @@ -175,7 +175,7 @@ class UpdateGoodsRecordSpec extends AnyFreeSpec with Matchers with TryValues wit .success .value - val result = UpdateGoodsRecord.buildCountryOfOrigin(answers, testEori, testRecordId, isCategorised = true) + val result = UpdateGoodsRecord.validateCountryOfOrigin(answers, testRecordId, isCategorised = true) inside(result) { case Left(errors) => errors.toChain.toList must contain only PageMissing(CountryOfOriginUpdatePage(testRecordId)) @@ -189,7 +189,7 @@ class UpdateGoodsRecordSpec extends AnyFreeSpec with Matchers with TryValues wit .success .value - val result = UpdateGoodsRecord.buildCountryOfOrigin(answers, testEori, testRecordId, isCategorised = true) + val result = UpdateGoodsRecord.validateCountryOfOrigin(answers, testRecordId, isCategorised = true) inside(result) { case Left(errors) => errors.toChain.toList must contain only UnexpectedPage(HasCountryOfOriginChangePage(testRecordId))