diff --git a/modules/api/functional_test/live_tests/batch/approve_batch_change_test.py b/modules/api/functional_test/live_tests/batch/approve_batch_change_test.py index 9817c1eeb..0676a87fe 100644 --- a/modules/api/functional_test/live_tests/batch/approve_batch_change_test.py +++ b/modules/api/functional_test/live_tests/batch/approve_batch_change_test.py @@ -59,9 +59,55 @@ def test_approve_pending_batch_change_success(shared_zone_test_context): assert_that(get_batch, not(has_key('cancelledTimestamp'))) finally: clear_zoneid_rsid_tuple_list(to_delete, client) - if to_disconnect is not None: + if to_disconnect: approver.abandon_zones(to_disconnect['id'], status=202) +@pytest.mark.manual_batch_review +def test_approve_pending_batch_change_fails_if_there_are_still_errors(shared_zone_test_context): + """ + Test approving a batch change fails if there are still errors + """ + client = shared_zone_test_context.ok_vinyldns_client + approver = shared_zone_test_context.support_user_client + batch_change_input = { + "changes": [ + get_change_A_AAAA_json("needs-review.nonexistent.", address="4.3.2.1"), + get_change_A_AAAA_json("zone.does.not.exist.") + ], + "ownerGroupId": shared_zone_test_context.ok_group['id'] + } + complete_rs = None + + try: + result = client.create_batch_change(batch_change_input, status=202) + get_batch = client.get_batch_change(result['id']) + assert_that(get_batch['status'], is_('PendingReview')) + assert_that(get_batch['approvalStatus'], is_('PendingReview')) + assert_that(get_batch['changes'][0]['status'], is_('NeedsReview')) + assert_that(get_batch['changes'][0]['validationErrors'][0]['errorType'], is_('RecordRequiresManualReview')) + assert_that(get_batch['changes'][1]['status'], is_('NeedsReview')) + assert_that(get_batch['changes'][1]['validationErrors'][0]['errorType'], is_('ZoneDiscoveryError')) + + approval_response = approver.approve_batch_change(result['id'], status=400) + assert_that((approval_response[0]['errors'][0]), contains_string('Zone Discovery Failed')) + assert_that((approval_response[1]['errors'][0]), contains_string('Zone Discovery Failed')) + + updated_batch = client.get_batch_change(result['id'], status=200) + assert_that(updated_batch['status'], is_('PendingReview')) + assert_that(updated_batch['approvalStatus'], is_('PendingReview')) + assert_that(updated_batch, not(has_key('reviewerId'))) + assert_that(updated_batch, not(has_key('reviewerUserName'))) + assert_that(updated_batch, not(has_key('reviewTimestamp'))) + assert_that(updated_batch, not(has_key('cancelledTimestamp'))) + assert_that(updated_batch['changes'][0]['status'], is_('NeedsReview')) + assert_that(updated_batch['changes'][0]['validationErrors'][0]['errorType'], is_('ZoneDiscoveryError')) + assert_that(updated_batch['changes'][1]['status'], is_('NeedsReview')) + assert_that(updated_batch['changes'][1]['validationErrors'][0]['errorType'], is_('ZoneDiscoveryError')) + finally: + if complete_rs: + delete_result = client.delete_recordset(complete_rs['zoneId'], complete_rs['id'], status=202) + client.wait_until_recordset_change_status(delete_result, 'Complete') + @pytest.mark.manual_batch_review def test_approve_batch_change_with_invalid_batch_change_id_fails(shared_zone_test_context): """ diff --git a/modules/api/src/main/scala/vinyldns/api/domain/batch/BatchChangeErrors.scala b/modules/api/src/main/scala/vinyldns/api/domain/batch/BatchChangeErrors.scala index 840e1fe3a..c79fba1cd 100644 --- a/modules/api/src/main/scala/vinyldns/api/domain/batch/BatchChangeErrors.scala +++ b/modules/api/src/main/scala/vinyldns/api/domain/batch/BatchChangeErrors.scala @@ -20,7 +20,7 @@ import org.joda.time.DateTime import vinyldns.api.domain.batch.BatchChangeInterfaces.ValidatedBatch import vinyldns.api.domain.batch.BatchTransformations.ChangeForValidation import vinyldns.core.domain.DomainValidationError -import vinyldns.core.domain.batch.SingleChange +import vinyldns.core.domain.batch.{BatchChange, SingleChange} /* Error response options */ sealed trait BatchChangeErrorResponse @@ -33,6 +33,9 @@ final case class InvalidBatchChangeResponses( changeRequestResponses: ValidatedBatch[ChangeForValidation]) extends BatchChangeErrorResponse +final case class BatchChangeFailedApproval(batchChange: BatchChange) + extends BatchChangeErrorResponse + final case class BatchChangeNotFound(id: String) extends BatchChangeErrorResponse { def message: String = s"Batch change with id $id cannot be found" } diff --git a/modules/api/src/main/scala/vinyldns/api/domain/batch/BatchChangeService.scala b/modules/api/src/main/scala/vinyldns/api/domain/batch/BatchChangeService.scala index d2e5781c7..c329e2aec 100644 --- a/modules/api/src/main/scala/vinyldns/api/domain/batch/BatchChangeService.scala +++ b/modules/api/src/main/scala/vinyldns/api/domain/batch/BatchChangeService.scala @@ -32,7 +32,7 @@ import vinyldns.core.domain.auth.AuthPrincipal import vinyldns.core.domain.batch.BatchChangeApprovalStatus.BatchChangeApprovalStatus import vinyldns.core.domain.batch._ import vinyldns.core.domain.batch.BatchChangeApprovalStatus._ -import vinyldns.core.domain.{CnameAtZoneApexError, ZoneDiscoveryError} +import vinyldns.core.domain.{CnameAtZoneApexError, SingleChangeError, ZoneDiscoveryError} import vinyldns.core.domain.membership.{ Group, GroupRepository, @@ -159,17 +159,17 @@ class BatchChangeService( authPrincipal.userId, approveBatchChangeInput.reviewComment) validationOutput <- applyBatchChangeValidationFlow(asInput, requesterAuth, isApproved = true) - changeForConversion <- buildResponseForApprover( + changeForConversion = rebuildBatchChangeForUpdate( batchChange, - asInput, validationOutput.validatedChanges, - reviewInfo).toBatchResult + reviewInfo) serviceCompleteBatch <- convertOrSave( changeForConversion, validationOutput.existingZones, validationOutput.groupedChanges.existingRecordSets, batchChange.ownerGroupId) - } yield serviceCompleteBatch + response <- buildResponseForApprover(serviceCompleteBatch).toBatchResult + } yield response def cancelBatchChange( batchChangeId: String, @@ -440,33 +440,33 @@ class BatchChangeService( } } - def buildResponseForApprover( + def rebuildBatchChangeForUpdate( existingBatchChange: BatchChange, - batchChangeInput: BatchChangeInput, transformed: ValidatedBatch[ChangeForValidation], - reviewInfo: BatchChangeReviewInfo): Either[BatchChangeErrorResponse, BatchChange] = - if (transformed.forall(_.isValid)) { - val changes = transformed.getValid.zip(existingBatchChange.changes).map { - case (newValidation, existing) => newValidation.asStoredChange(Some(existing.id)) - } - - BatchChange( - existingBatchChange.userId, - existingBatchChange.userName, - existingBatchChange.comments, - existingBatchChange.createdTimestamp, - changes, - existingBatchChange.ownerGroupId, - BatchChangeApprovalStatus.ManuallyApproved, - Some(reviewInfo.reviewerId), - reviewInfo.reviewComment, - Some(reviewInfo.reviewTimestamp), - id = existingBatchChange.id - ).asRight - } else { - InvalidBatchChangeResponses(batchChangeInput.changes, transformed).asLeft + reviewInfo: BatchChangeReviewInfo): BatchChange = { + val changes = transformed.zip(existingBatchChange.changes).map { + case (validated, existing) => + validated match { + case Valid(v) => v.asStoredChange(Some(existing.id)) + case Invalid(errors) => + existing.updateValidationErrors(errors.map(e => SingleChangeError(e)).toList) + } } + if (transformed.forall(_.isValid)) { + existingBatchChange + .copy( + changes = changes, + approvalStatus = BatchChangeApprovalStatus.ManuallyApproved, + reviewerId = Some(reviewInfo.reviewerId), + reviewComment = reviewInfo.reviewComment, + reviewTimestamp = Some(reviewInfo.reviewTimestamp) + ) + } else { + existingBatchChange.copy(changes = changes) + } + } + def convertOrSave( batchChange: BatchChange, existingZones: ExistingZones, @@ -485,6 +485,7 @@ class BatchChangeService( case PendingReview if manualReviewEnabled => // save the change, will need to return to it later on approval batchChangeRepo.save(batchChange).toBatchResult + // TODO: handle PendingReview if manualReviewEnabled is false case _ => // this should not be called with a rejected change (or if manual review is off)! logger.error( @@ -493,6 +494,13 @@ class BatchChangeService( UnknownConversionError("Cannot convert a rejected batch change").toLeftBatchResult } + def buildResponseForApprover( + batchChange: BatchChange): Either[BatchChangeErrorResponse, BatchChange] = + batchChange.approvalStatus match { + case ManuallyApproved => batchChange.asRight + case _ => BatchChangeFailedApproval(batchChange).asLeft + } + def addOwnerGroupNamesToSummaries( summaries: List[BatchChangeSummary], groups: Set[Group]): List[BatchChangeSummary] = diff --git a/modules/api/src/main/scala/vinyldns/api/route/BatchChangeJsonProtocol.scala b/modules/api/src/main/scala/vinyldns/api/route/BatchChangeJsonProtocol.scala index 4464d6e18..0e4a8401d 100644 --- a/modules/api/src/main/scala/vinyldns/api/route/BatchChangeJsonProtocol.scala +++ b/modules/api/src/main/scala/vinyldns/api/route/BatchChangeJsonProtocol.scala @@ -47,6 +47,7 @@ trait BatchChangeJsonProtocol extends JsonValidation { SingleDeleteRecordChangeSerializer, BatchChangeSerializer, BatchChangeErrorListSerializer, + BatchChangeRevalidationErrorListSerializer, BatchChangeErrorSerializer, RejectBatchChangeInputSerializer, ApproveBatchChangeInputSerializer @@ -223,6 +224,20 @@ trait BatchChangeJsonProtocol extends JsonValidation { } } + case object BatchChangeRevalidationErrorListSerializer + extends ValidationSerializer[BatchChangeFailedApproval] { + override def toJson(bcfa: BatchChangeFailedApproval): JValue = { + val asInput = BatchChangeInput(bcfa.batchChange) + bcfa.batchChange.changes.zip(asInput.changes).map { + case (sc, ci) if sc.validationErrors.isEmpty => Extraction.decompose(ci) + case (sc, ci) => + val change = Extraction.decompose(ci) + val errors: JValue = "errors" -> Extraction.decompose(sc.validationErrors.map(_.message)) + change.merge(errors) + } + } + } + case object BatchChangeErrorSerializer extends ValidationSerializer[DomainValidationError] { override def toJson(dve: DomainValidationError): JValue = dve.message } diff --git a/modules/api/src/main/scala/vinyldns/api/route/BatchChangeRouting.scala b/modules/api/src/main/scala/vinyldns/api/route/BatchChangeRouting.scala index 15518b2f1..07edb3ad0 100644 --- a/modules/api/src/main/scala/vinyldns/api/route/BatchChangeRouting.scala +++ b/modules/api/src/main/scala/vinyldns/api/route/BatchChangeRouting.scala @@ -33,6 +33,7 @@ class BatchChangeRoute( def handleErrors(e: BatchChangeErrorResponse): PartialFunction[BatchChangeErrorResponse, Route] = { case ibci: InvalidBatchChangeInput => complete(StatusCodes.BadRequest, ibci) case crl: InvalidBatchChangeResponses => complete(StatusCodes.BadRequest, crl) + case bcfa: BatchChangeFailedApproval => complete(StatusCodes.BadRequest, bcfa) case cnf: BatchChangeNotFound => complete(StatusCodes.NotFound, cnf.message) case una: UserNotAuthorizedError => complete(StatusCodes.Forbidden, una.message) case uct: BatchConversionError => complete(StatusCodes.BadRequest, uct) diff --git a/modules/api/src/test/scala/vinyldns/api/domain/batch/BatchChangeServiceSpec.scala b/modules/api/src/test/scala/vinyldns/api/domain/batch/BatchChangeServiceSpec.scala index 62be42712..369064bba 100644 --- a/modules/api/src/test/scala/vinyldns/api/domain/batch/BatchChangeServiceSpec.scala +++ b/modules/api/src/test/scala/vinyldns/api/domain/batch/BatchChangeServiceSpec.scala @@ -64,6 +64,7 @@ class BatchChangeServiceSpec implicit val contextShift: ContextShift[IO] = IO.contextShift(ExecutionContext.global) private val nonFatalError = ZoneDiscoveryError("test") + private val fatalError = RecordAlreadyExists("test") private val validations = new BatchChangeValidations(10, new AccessValidations()) private val ttl = Some(200L) @@ -1800,7 +1801,7 @@ class BatchChangeServiceSpec } } - "buildResponseForApprover" should { + "rebuildBatchChangeForUpdate" should { val batchChangeNeedsApproval = BatchChange( auth.userId, auth.signedInUser.userName, @@ -1817,9 +1818,8 @@ class BatchChangeServiceSpec val reviewInfo = BatchChangeReviewInfo(supportUser.id, Some("some approval comment")) "return a BatchChange if all data inputs are valid" in { val result = underTestManualEnabled - .buildResponseForApprover( + .rebuildBatchChangeForUpdate( batchChangeNeedsApproval, - asInput, List( AddChangeForValidation( baseZone, @@ -1829,31 +1829,27 @@ class BatchChangeServiceSpec ), reviewInfo ) - .toOption - .get result shouldBe a[BatchChange] result.changes.head shouldBe singleChangeGood result.changes(1) shouldBe singleChangeNRPostReview + result.approvalStatus shouldBe BatchChangeApprovalStatus.ManuallyApproved } - "return an error even with valid/soft failures and manual review enabled" in { + "return a BatchChange with current failures if any data is invalid" in { val result = underTestManualEnabled - .buildResponseForApprover( + .rebuildBatchChangeForUpdate( batchChangeNeedsApproval, - asInput, List( AddChangeForValidation( baseZone, singleChangeGood.inputName.split('.').head, asAdds.head).validNel, - nonFatalError.invalidNel + fatalError.invalidNel ), reviewInfo ) - .left - .value - result shouldBe an[InvalidBatchChangeResponses] + result shouldBe a[BatchChange] } } "listBatchChangeSummaries" should { @@ -1880,7 +1876,7 @@ class BatchChangeServiceSpec result.ignoreAccess shouldBe false result.batchChanges.length shouldBe 1 - result.batchChanges(0).createdTimestamp shouldBe batchChange.createdTimestamp + result.batchChanges.head.createdTimestamp shouldBe batchChange.createdTimestamp result.batchChanges(0).ownerGroupId shouldBe None result.batchChanges(0).approvalStatus shouldBe BatchChangeApprovalStatus.ManuallyApproved result.batchChanges(0).reviewerName shouldBe Some(superUser.userName) @@ -2264,4 +2260,55 @@ class BatchChangeServiceSpec } } + "buildResponseForApprover" should { + "return batch change with ManuallyApproved approval status" in { + val updatedBatchChange = BatchChange( + auth.userId, + auth.signedInUser.userName, + Some("comments in"), + DateTime.now.minusDays(1), + List(singleChangeGood, singleChangeNR), + Some(authGrp.id), + BatchChangeApprovalStatus.ManuallyApproved, + Some("reviewer_id"), + Some("approved"), + Some(DateTime.now) + ) + + val result = underTest.buildResponseForApprover(updatedBatchChange).right.value + + result shouldBe a[BatchChange] + } + "return BatchChangeFailedApproval error if batch change has PendingReview approval status" in { + val batchChange = + BatchChange( + auth.userId, + auth.signedInUser.userName, + Some("check approval status"), + DateTime.now, + List(singleChangeGood, singleChangeNR), + approvalStatus = BatchChangeApprovalStatus.PendingReview + ) + + val result = underTest.buildResponseForApprover(batchChange).left.value + + result shouldBe a[BatchChangeFailedApproval] + } + "return BatchChangeFailedApproval if batch change has an approval status other than" + + "ManuallyApproved or PendingReview" in { + val batchChange = + BatchChange( + auth.userId, + auth.signedInUser.userName, + Some("check approval status"), + DateTime.now, + List(singleChangeGood, singleChangeNR), + approvalStatus = BatchChangeApprovalStatus.AutoApproved + ) + + val result = underTest.buildResponseForApprover(batchChange).left.value + + result shouldBe a[BatchChangeFailedApproval] + } + } } diff --git a/modules/api/src/test/scala/vinyldns/api/route/BatchChangeJsonProtocolSpec.scala b/modules/api/src/test/scala/vinyldns/api/route/BatchChangeJsonProtocolSpec.scala index d293e3039..5ee7a2f6c 100644 --- a/modules/api/src/test/scala/vinyldns/api/route/BatchChangeJsonProtocolSpec.scala +++ b/modules/api/src/test/scala/vinyldns/api/route/BatchChangeJsonProtocolSpec.scala @@ -29,7 +29,13 @@ import vinyldns.api.domain.batch.BatchTransformations.{AddChangeForValidation, C import vinyldns.api.domain.batch.ChangeInputType._ import vinyldns.api.domain.batch._ import vinyldns.core.TestZoneData.okZone -import vinyldns.core.domain.{InvalidIpv4Address, InvalidTTL, SingleChangeError, ZoneDiscoveryError} +import vinyldns.core.domain.{ + DomainValidationErrorType, + InvalidIpv4Address, + InvalidTTL, + SingleChangeError, + ZoneDiscoveryError +} import vinyldns.core.domain.batch.SingleChangeStatus._ import vinyldns.core.domain.batch._ import vinyldns.core.domain.record.RecordType._ @@ -524,6 +530,65 @@ class BatchChangeJsonProtocolSpec } } + "Serializing BatchChangeRevalidationErrorList" should { + "serialize a mix of valid and invalid inputs" in { + val errorMessage = "Zone Discovery Failed: zone for \"foo.\" does not exist in VinylDNS. " + + "If zone exists, then it must be connected to in VinylDNS." + val delete = SingleDeleteRRSetChange( + Some("zoneId"), + Some("zoneName"), + Some("recordName"), + "foo", + A, + Pending, + None, + None, + None, + id = "id") + val add = SingleAddChange( + Some("zoneId"), + Some("zoneName"), + Some("recordName"), + "foo", + A, + 3600, + AData("1.1.1.1"), + Pending, + None, + None, + None, + List(SingleChangeError(DomainValidationErrorType.ZoneDiscoveryError, errorMessage)), + id = "id" + ) + + val time = DateTime.now + val batchChange = BatchChange( + "someUserId", + "someUserName", + Some("these be comments!"), + time, + List(add, delete), + None, + BatchChangeApprovalStatus.PendingReview, + None, + None, + None, + "someId") + val result = + BatchChangeRevalidationErrorListSerializer.toJson(BatchChangeFailedApproval(batchChange)) + + val expected = decompose( + List( + decompose(addAChangeInput) + .asInstanceOf[JObject] ~ ("errors" -> List(fooDiscoveryError.message)), + decompose(deleteAChangeInput) + ) + ) + + result shouldBe expected + } + } + "Serializing BatchChangeError" should { "return the message of the BatchChange" in { val error = ZoneDiscoveryError("name") diff --git a/modules/core/src/main/scala/vinyldns/core/domain/batch/SingleChange.scala b/modules/core/src/main/scala/vinyldns/core/domain/batch/SingleChange.scala index d2b4628d3..2a432dc9b 100644 --- a/modules/core/src/main/scala/vinyldns/core/domain/batch/SingleChange.scala +++ b/modules/core/src/main/scala/vinyldns/core/domain/batch/SingleChange.scala @@ -98,6 +98,13 @@ sealed trait SingleChange { // TODO: Implement cancel once DeleteRecord is connected in batch change process flow case _ => throw new RuntimeException("Not yet implemented") } + + def updateValidationErrors(errors: List[SingleChangeError]): SingleChange = + this match { + case sad: SingleAddChange => sad.copy(validationErrors = errors) + case sdc: SingleDeleteRRSetChange => sdc.copy(validationErrors = errors) + case sdrc: SingleDeleteRecordChange => sdrc.copy(validationErrors = errors) + } } final case class SingleAddChange( diff --git a/modules/portal/public/lib/batch-change/batch-change-detail.controller.js b/modules/portal/public/lib/batch-change/batch-change-detail.controller.js index f22b3ed15..14ed7dbda 100644 --- a/modules/portal/public/lib/batch-change/batch-change-detail.controller.js +++ b/modules/portal/public/lib/batch-change/batch-change-detail.controller.js @@ -79,7 +79,7 @@ .then(success) .catch(function (error) { if (typeof error.data == "object") { - for(var i = 0; i < error.data.length; i++) { + for (var i = 0; i < error.data.length; i++) { if (error.data[i].errors) { $scope.batch.changes[i].validationErrors = error.data[i].errors $scope.batch.changes[i].outstandingErrors = true