Do not specify migrations which did not change process in process mig…

…ration comment
TouK · Nov 30, 2022 · b5372e5 · b5372e5
1 parent 6eb2f53
commit b5372e5
Show file tree

Hide file tree

Showing 9 changed files with 26 additions and 24 deletions.
diff --git a/designer/server/src/main/scala/pl/touk/nussknacker/ui/db/entity/CommentEntityFactory.scala b/designer/server/src/main/scala/pl/touk/nussknacker/ui/db/entity/CommentEntityFactory.scala
@@ -55,22 +55,21 @@ trait CommentActions {
     Sequence[Long]("process_comments_id_sequence").next.result
   }
 
-  def newCommentAction(processId: ProcessId, processVersionId: VersionId, comment: Comment)
+  def newCommentAction(processId: ProcessId, processVersionId: VersionId, comment: Option[Comment])
                       (implicit ec: ExecutionContext, loggedUser: LoggedUser): DB[Option[Long]] = {
-    if (comment.value.nonEmpty) {
-      for {
+    comment match {
+      case Some(c) if c.value.nonEmpty => for {
         newId <- nextIdAction
         _ <- commentsTable += CommentEntityData(
           id = newId,
           processId = processId,
           processVersionId = processVersionId,
-          content = comment.value,
+          content = c.value,
           user = loggedUser.username,
           createDate = Timestamp.from(Instant.now())
         )
       } yield Some(newId)
-    } else {
-      DBIO.successful(None)
+      case None => DBIO.successful(None)
     }
   }
 

diff --git a/designer/server/src/main/scala/pl/touk/nussknacker/ui/process/ProcessService.scala b/designer/server/src/main/scala/pl/touk/nussknacker/ui/process/ProcessService.scala
@@ -243,7 +243,7 @@ class DBProcessService(managerActor: ActorRef,
         }
         processUpdated <- EitherT(repositoryManager
           .runInTransaction(processRepository
-            .updateProcess(UpdateProcessAction(processIdWithName.id, substituted, action.comment, increaseVersionWhenJsonNotChanged = false))
+            .updateProcess(UpdateProcessAction(processIdWithName.id, substituted, Option(action.comment), increaseVersionWhenJsonNotChanged = false))
           ))
       } yield UpdateProcessResponse(
         processUpdated

diff --git a/...r/server/src/main/scala/pl/touk/nussknacker/ui/process/migrate/ProcessModelMigrator.scala b/...r/server/src/main/scala/pl/touk/nussknacker/ui/process/migrate/ProcessModelMigrator.scala
@@ -16,7 +16,7 @@ case class MigrationResult(process: CanonicalProcess, migrationsApplied: List[Pr
   def toUpdateAction(processId: ProcessId): UpdateProcessAction = UpdateProcessAction(
     id = processId,
     canonicalProcess = process,
-    comment = MigrationComment(migrationsApplied),
+    comment = Option(migrationsApplied).filter(_.nonEmpty).map(MigrationComment),
     increaseVersionWhenJsonNotChanged = true
   )
 
@@ -39,10 +39,13 @@ class ProcessModelMigrator(migrations: ProcessingTypeDataProvider[ProcessMigrati
   }
 
   private def migrateWithMigrations(process: CanonicalProcess, migrationsToApply: List[ProcessMigration]): MigrationResult = {
-    val resultProcess = migrationsToApply.foldLeft(process) {
-      case (processToConvert, migration) => migration.migrateProcess(processToConvert)
+    val (resultProcess, migrationsApplied) = migrationsToApply.foldLeft((process, Nil: List[ProcessMigration])) {
+      case ((processToConvert, migrationsAppliedAcc), migration) =>
+        val migrated = migration.migrateProcess(processToConvert)
+        val migrationsApplied = if (migrated != processToConvert) migration :: migrationsAppliedAcc else migrationsAppliedAcc
+        (migrated, migrationsApplied)
     }
-    MigrationResult(resultProcess, migrationsToApply)
+    MigrationResult(resultProcess, migrationsApplied.reverse)
   }
 
 }

diff --git a/...rc/main/scala/pl/touk/nussknacker/ui/process/repository/DbProcessActivityRepository.scala b/...rc/main/scala/pl/touk/nussknacker/ui/process/repository/DbProcessActivityRepository.scala
@@ -30,7 +30,7 @@ case class DbProcessActivityRepository(dbConfig: DbConfig)
 
   override def addComment(processId: ProcessId, processVersionId: VersionId, comment: CommentValue)
                 (implicit ec: ExecutionContext, loggedUser: LoggedUser): Future[Unit] = {
-    run(newCommentAction(processId, processVersionId, comment)).map(_ => ())
+    run(newCommentAction(processId, processVersionId, Option(comment))).map(_ => ())
   }
 
   override def deleteComment(commentId: Long)(implicit ec: ExecutionContext): Future[Unit] = {

diff --git a/...er/src/main/scala/pl/touk/nussknacker/ui/process/repository/ProcessActionRepository.scala b/...er/src/main/scala/pl/touk/nussknacker/ui/process/repository/ProcessActionRepository.scala
@@ -56,7 +56,7 @@ extends BasicRepository with EspTables with CommentActions with ProcessActionRep
   //FIXME: Use ProcessVersionId instead of Long at processVersion
   private def action(processId: ProcessId, processVersion: VersionId, comment: Option[Comment], action: ProcessActionType, buildInfo: Option[String])(implicit user: LoggedUser) =
     for {
-      commentId <- withComment(processId, processVersion, comment)
+      commentId <- newCommentAction(processId, processVersion, comment)
       processActionData = ProcessActionEntityData(
         processId = processId,
         processVersionId = processVersion,
@@ -68,9 +68,4 @@ extends BasicRepository with EspTables with CommentActions with ProcessActionRep
       )
       _ <- processActionsTable += processActionData
     } yield processActionData
-
-  private def withComment(processId: ProcessId, processVersion: VersionId, comment: Option[Comment])(implicit ec: ExecutionContext, user: LoggedUser): DB[Option[Long]] = comment match {
-    case None => DBIOAction.successful(None)
-    case Some(comm) => newCommentAction(processId, processVersion, comm)
-  }
 }
diff --git a/...r/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/ProcessRepository.scala b/...r/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/ProcessRepository.scala
@@ -31,7 +31,7 @@ object ProcessRepository {
   def create(dbConfig: DbConfig, modelData: ProcessingTypeDataProvider[ModelData]): DBProcessRepository =
     new DBProcessRepository(dbConfig, modelData.mapValues(_.migrations.version))
 
-  case class UpdateProcessAction(id: ProcessId, canonicalProcess: CanonicalProcess, comment: Comment, increaseVersionWhenJsonNotChanged: Boolean)
+  case class UpdateProcessAction(id: ProcessId, canonicalProcess: CanonicalProcess, comment: Option[Comment], increaseVersionWhenJsonNotChanged: Boolean)
 
   case class CreateProcessAction(processName: ProcessName, category: String, canonicalProcess: CanonicalProcess, processingType: ProcessingType, isSubprocess: Boolean)
 
@@ -191,7 +191,7 @@ class DBProcessRepository(val dbConfig: DbConfig, val modelVersion: ProcessingTy
       .filter(_.processId === process.id)
       .sortBy(_.id.desc)
       .result.headOption.flatMap {
-      case Some(version) => newCommentAction(process.id, version.id, UpdateProcessComment(s"Rename: [${process.name.value}] -> [$newName]"))
+      case Some(version) => newCommentAction(process.id, version.id, Some(UpdateProcessComment(s"Rename: [${process.name.value}] -> [$newName]")))
       case None =>  DBIO.successful(())
     }
 

diff --git a/...rver/src/test/scala/pl/touk/nussknacker/ui/process/migrate/ProcessModelMigratorSpec.scala b/...rver/src/test/scala/pl/touk/nussknacker/ui/process/migrate/ProcessModelMigratorSpec.scala
@@ -24,6 +24,13 @@ class ProcessModelMigratorSpec extends AnyFlatSpec with BeforeAndAfterEach with
 
   private implicit val user = TestFactory.adminUser("test1")
 
+  it should "return migrations that actually changed process in migrationsApplied in MigrationResult" in {
+
+    val migrationResult: MigrationResult = migrateByVersions(None, 1, 2, 3, 4, 6, 7, 8, 9)
+
+    migrationResult.migrationsApplied.map(_.description) should contain theSameElementsAs List("testMigration1", "testMigration2", "testMigration8")
+  }
+
   it should "migrate processes to new versions when not migrated" in {
 
     val migrationResult: MigrationResult = migrateByVersions(None, 1, 2)

diff --git a/...est/scala/pl/touk/nussknacker/ui/process/repository/DBFetchingProcessRepositorySpec.scala b/...est/scala/pl/touk/nussknacker/ui/process/repository/DBFetchingProcessRepositorySpec.scala
@@ -214,7 +214,7 @@ class DBFetchingProcessRepositorySpec
     fetching.fetchProcessId(processName).futureValue.nonEmpty
 
   private def updateProcess(processId: ProcessId, canonicalProcess: CanonicalProcess, increaseVersionWhenJsonNotChanged: Boolean): ProcessUpdated = {
-    val action = UpdateProcessAction(processId, canonicalProcess, UpdateProcessComment(""), increaseVersionWhenJsonNotChanged)
+    val action = UpdateProcessAction(processId, canonicalProcess, None, increaseVersionWhenJsonNotChanged)
 
     val processUpdated = repositoryManager.runInTransaction(writingRepo.updateProcess(action)).futureValue
     processUpdated shouldBe 'right

diff --git a/docs/Changelog.md b/docs/Changelog.md
@@ -1,6 +1,3 @@
-
-# Changelog
-
 1.7.0 (Not released yet)
 ------------------------
 * [#3560](https://github.com/TouK/nussknacker/pull/3560), [#3560](https://github.com/TouK/nussknacker/pull/3560), [#3595](https://github.com/TouK/nussknacker/pull/3595) Migrate from Flink Scala API to Java API
@@ -46,6 +43,7 @@
 * [#3682](https://github.com/TouK/nussknacker/pull/3682) Extract generic `BaseSharedKafkaProducer`, rename `SharedKafkaProducerHolder` to `DefaultSharedKafkaProducerHolder`. 
 * [#3701](https://github.com/TouK/nussknacker/pull/3701) `TypedMap` allows access to non-existing keys in SpEL (returning `null`)
 * [#3733](https://github.com/TouK/nussknacker/pull/3733) Fix for: some validation (e.g. Flink scenario name validation) were causing error message blinking in scenario properties.
+* [#3752](https://github.com/TouK/nussknacker/pull/3752) Do not specify migrations which did not change process in process migration comment. If no migrations, do not add comment.
 
 1.6.1 (8 Nov 2022)
 ------------------------