diff --git a/modules/api/functional_test/live_tests/batch/create_batch_change_test.py b/modules/api/functional_test/live_tests/batch/create_batch_change_test.py index 142c81029..be5924987 100644 --- a/modules/api/functional_test/live_tests/batch/create_batch_change_test.py +++ b/modules/api/functional_test/live_tests/batch/create_batch_change_test.py @@ -276,7 +276,7 @@ def test_create_batch_change_with_scheduled_time_and_owner_group_succeeds(shared Test successfully creating a batch change with scheduled time and owner group set """ client = shared_zone_test_context.ok_vinyldns_client - dt = datetime.datetime.now().strftime('%Y-%m-%dT%H:%M:%SZ') + dt = (datetime.datetime.now() + datetime.timedelta(days=1)).strftime('%Y-%m-%dT%H:%M:%SZ') batch_change_input = { "comments": "this is optional", @@ -298,7 +298,7 @@ def test_create_scheduled_batch_change_with_zone_discovery_error_without_owner_g Test creating a scheduled batch without owner group ID fails if there is a zone discovery error """ client = shared_zone_test_context.ok_vinyldns_client - dt = datetime.datetime.now().strftime('%Y-%m-%dT%H:%M:%SZ') + dt = (datetime.datetime.now() + datetime.timedelta(days=1)).strftime('%Y-%m-%dT%H:%M:%SZ') batch_change_input = { "comments": "this is optional", @@ -313,6 +313,28 @@ def test_create_scheduled_batch_change_with_zone_discovery_error_without_owner_g assert_that(errors, is_("Batch change requires owner group for manual review/scheduled changes.")) +@pytest.mark.manual_batch_review +def test_create_scheduled_batch_change_with_scheduled_time_in_the_past_fails(shared_zone_test_context): + """ + Test creating a scheduled batch with a scheduled time in the past + """ + client = shared_zone_test_context.ok_vinyldns_client + yesterday = (datetime.datetime.now() - datetime.timedelta(days=1)).strftime('%Y-%m-%dT%H:%M:%SZ') + + batch_change_input = { + "comments": "this is optional", + "changes": [ + get_change_A_AAAA_json("no-owner-group.scheduled.parent.com.", address="4.5.6.7"), + ], + "ownerGroupId": shared_zone_test_context.ok_group['id'], + "scheduledTime": yesterday + } + + errors = client.create_batch_change(batch_change_input, status=400) + + assert_that(errors, is_("Scheduled time must be in the future.")) + + def test_create_batch_change_without_scheduled_time_succeeds(shared_zone_test_context): """ Test successfully creating a batch change without scheduled time set 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 ef9f8793e..050e08325 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 @@ -62,6 +62,10 @@ case object ScheduledChangesDisabled extends BatchChangeErrorResponse { "Cannot create a scheduled change, as it is currently disabled on this VinylDNS instance." } +case object ScheduledTimeMustBeInFuture extends BatchChangeErrorResponse { + val message: String = "Scheduled time must be in the future." +} + final case class ScheduledChangeNotDue(scheduledTime: DateTime) extends BatchChangeErrorResponse { val message: String = s"Cannot process scheduled change as it is not past the scheduled date of $scheduledTime" diff --git a/modules/api/src/main/scala/vinyldns/api/domain/batch/BatchChangeValidations.scala b/modules/api/src/main/scala/vinyldns/api/domain/batch/BatchChangeValidations.scala index f6d793bb7..aed35d31c 100644 --- a/modules/api/src/main/scala/vinyldns/api/domain/batch/BatchChangeValidations.scala +++ b/modules/api/src/main/scala/vinyldns/api/domain/batch/BatchChangeValidations.scala @@ -507,6 +507,10 @@ class BatchChangeValidations( def validateScheduledChange( input: BatchChangeInput, scheduledChangesEnabled: Boolean): Either[BatchChangeErrorResponse, Unit] = - if (scheduledChangesEnabled || input.scheduledTime.isEmpty) Right(()) - else Left(ScheduledChangesDisabled) + (scheduledChangesEnabled, input.scheduledTime) match { + case (_, None) => Right(()) + case (true, Some(scheduledTime)) if scheduledTime.isAfterNow => Right(()) + case (true, _) => Left(ScheduledTimeMustBeInFuture) + case (false, _) => Left(ScheduledChangesDisabled) + } } 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 fd0bde3d4..1255cf45f 100644 --- a/modules/api/src/main/scala/vinyldns/api/route/BatchChangeRouting.scala +++ b/modules/api/src/main/scala/vinyldns/api/route/BatchChangeRouting.scala @@ -44,6 +44,8 @@ class BatchChangeRoute( complete(StatusCodes.BadRequest, ManualReviewRequiresOwnerGroup.message) case ScheduledChangesDisabled => complete(StatusCodes.BadRequest, ScheduledChangesDisabled.message) + case ScheduledTimeMustBeInFuture => + complete(StatusCodes.BadRequest, ScheduledTimeMustBeInFuture.message) case scnpd: ScheduledChangeNotDue => complete(StatusCodes.Forbidden, scnpd.message) } diff --git a/modules/api/src/test/scala/vinyldns/api/domain/batch/BatchChangeValidationsSpec.scala b/modules/api/src/test/scala/vinyldns/api/domain/batch/BatchChangeValidationsSpec.scala index 4b0f28208..52561fcf7 100644 --- a/modules/api/src/test/scala/vinyldns/api/domain/batch/BatchChangeValidationsSpec.scala +++ b/modules/api/src/test/scala/vinyldns/api/domain/batch/BatchChangeValidationsSpec.scala @@ -184,7 +184,7 @@ class BatchChangeValidationsSpec property( "validateScheduledChange: should succeed if batch is scheduled and scheduled change enabled") { - val input = BatchChangeInput(None, List(), scheduledTime = Some(DateTime.now)) + val input = BatchChangeInput(None, List(), scheduledTime = Some(DateTime.now.plusHours(1))) validateScheduledChange(input, scheduledChangesEnabled = true) should beRight(()) } @@ -284,6 +284,21 @@ class BatchChangeValidationsSpec ScheduledChangesDisabled) } + property( + "validateBatchChangeInput: should fail if scheduled changes is enabled but scheduled time is in the past") { + val input = BatchChangeInput( + None, + List(AddChangeInput("private-create", RecordType.A, ttl, AData("1.1.1.1"))), + scheduledTime = Some(DateTime.now.minusHours(1))) + val bcv = new BatchChangeValidations( + maxChanges, + AccessValidations, + multiRecordEnabled = true, + scheduledChangesEnabled = true) + bcv.validateBatchChangeInput(input, None, okAuth).value.unsafeRunSync() shouldBe Left( + ScheduledTimeMustBeInFuture) + } + property("validateBatchChangePendingReview: should succeed if batch change is PendingReview") { validateBatchChangePendingReview(validPendingBatchChange) should be(right) } diff --git a/modules/core/src/main/scala/vinyldns/core/domain/DomainValidationErrors.scala b/modules/core/src/main/scala/vinyldns/core/domain/DomainValidationErrors.scala index c53c69b67..f00da25f2 100644 --- a/modules/core/src/main/scala/vinyldns/core/domain/DomainValidationErrors.scala +++ b/modules/core/src/main/scala/vinyldns/core/domain/DomainValidationErrors.scala @@ -28,13 +28,16 @@ sealed abstract class DomainValidationError(val isFatal: Boolean = true) { final case class ChangeLimitExceeded(limit: Int) extends DomainValidationError { def message: String = s"Cannot request more than $limit changes in a single batch change request" } + final case class BatchChangeIsEmpty(limit: Int) extends DomainValidationError { def message: String = s"Batch change contained no changes. Batch change must have at least one change, up to a maximum of $limit changes." } + final case class GroupDoesNotExist(id: String) extends DomainValidationError { def message: String = s"""Group with ID "$id" was not found""" } + final case class NotAMemberOfOwnerGroup(ownerGroupId: String, userName: String) extends DomainValidationError { def message: String =