Bug Fix: UpdateFilingStatus now only updates the fields it should

Previously, UpdateFilingStatus would fully replace a Filing in the
filing State with a new one. This meant it was possible to update filing
period or institutionID, which should not be updated.

Author: schbetsy

persistence/src/main/scala/hmda/persistence/institutions/FilingPersistence.scala

@@ -12,7 +12,7 @@ object FilingPersistence {
   val name = "filings"
 
   case class CreateFiling(filing: Filing) extends Command
-  case class UpdateFilingStatus(filing: Filing) extends Command
+  case class UpdateFilingStatus(period: String, status: FilingStatus) extends Command
   case class GetFilingByPeriod(period: String) extends Command
 
   def props(institutionId: String): Props = Props(new FilingPersistence(institutionId))
@@ -59,27 +59,20 @@ class FilingPersistence(institutionId: String) extends HmdaPersistentActor {
         log.warning(s"Could not create Filing. Filing period ${f.period} already exists for institution $institutionId.")
       }
 
-    case UpdateFilingStatus(modified) =>
-      if (state.filings.map(x => x.period).contains(modified.period)) {
-        val start = if (modified.status == InProgress) {
-          System.currentTimeMillis
-        } else {
-          modified.start
-        }
-        val end = if (modified.status == Completed) {
-          System.currentTimeMillis
-        } else {
-          modified.end
-        }
-        val updatedFiling = modified.copy(start = start, end = end)
-        persist(FilingStatusUpdated(updatedFiling)) { e =>
-          log.debug(s"persisted: $updatedFiling")
-          updateState(e)
-          sender() ! Some(updatedFiling)
-        }
-      } else {
-        sender() ! None
-        log.warning(s"Period does not exist. Could not update $modified")
+    case UpdateFilingStatus(period, newStatus) =>
+      state.filings.find(f => f.period == period) match {
+        case Some(filing) =>
+          val startTime = if (newStatus == InProgress) System.currentTimeMillis else filing.start
+          val endTime = if (newStatus == Completed) System.currentTimeMillis else filing.end
+          val updatedFiling = filing.copy(status = newStatus, start = startTime, end = endTime)
+          persist(FilingStatusUpdated(updatedFiling)) { e =>
+            log.debug(s"persisted: $updatedFiling")
+            updateState(e)
+            sender() ! Some(updatedFiling)
+          }
+        case None =>
+          sender() ! None
+          log.warning(s"Could not update filing status. Institution $institutionId, filing period $period")
       }
 
     case GetFilingByPeriod(period) =>

persistence/src/main/scala/hmda/persistence/processing/SubmissionManager.scala

@@ -166,9 +166,8 @@ class SubmissionManager(submissionId: SubmissionId) extends HmdaActor {
   private def updateFilingStatus(filingStatus: FilingStatus) = {
     for {
       p <- filingPersistence
-      f <- (p ? GetFilingByPeriod(period)).mapTo[Filing]
     } yield {
-      p ? UpdateFilingStatus(f.copy(status = filingStatus))
+      p ? UpdateFilingStatus(period, filingStatus)
     }
   }
 

persistence/src/test/scala/hmda/persistence/institutions/FilingPersistenceSpec.scala

@@ -3,7 +3,6 @@ package hmda.persistence.institutions
 import akka.testkit.TestProbe
 import hmda.model.fi._
 import hmda.persistence.messages.CommonMessages.GetState
-import hmda.persistence.demo.DemoData
 import hmda.persistence.institutions.FilingPersistence._
 import hmda.persistence.model.ActorSpec
 
@@ -26,15 +25,6 @@ class FilingPersistenceSpec extends ActorSpec {
       probe.send(filingsActor, GetState)
       probe.expectMsg(filings.reverse)
     }
-    "be able to change their status" in {
-      val filing = DemoData.testFilings.head
-      val modified = filing.copy(status = Cancelled)
-      probe.send(filingsActor, UpdateFilingStatus(modified))
-      probe.expectMsg(Some(modified))
-      probe.send(filingsActor, GetFilingByPeriod(filing.period))
-      probe.expectMsg(modified)
-    }
-
     "return None when persisting a filing period that already exists" in {
       val f1 = Filing("2018", "12345", Completed, true, 1483287071000L, 1514736671000L)
       probe.send(filingsActor, CreateFiling(f1))
@@ -44,12 +34,34 @@ class FilingPersistenceSpec extends ActorSpec {
       probe.send(filingsActor, CreateFiling(f2))
       probe.expectMsg(None)
     }
+  }
 
-    "return None for nonexistent period" in {
-      val f = Filing("2006", "12345", Cancelled, false, 1483287071000L, 1514736671000L)
-      probe.send(filingsActor, UpdateFilingStatus(f))
+  "UpdateFilingStatus" must {
+    "update status of filing for given period" in {
+      val expected = sample1.copy(status = Cancelled)
+      probe.send(filingsActor, UpdateFilingStatus("2016", Cancelled))
+      probe.expectMsg(Some(expected))
+      probe.send(filingsActor, GetFilingByPeriod("2016"))
+      probe.expectMsg(expected)
+    }
+    "return None for nonexistent filing period" in {
+      probe.send(filingsActor, UpdateFilingStatus("2006", InProgress))
       probe.expectMsg(None)
     }
+    "update start and end timestamp, if necessary" in {
+      probe.send(filingsActor, UpdateFilingStatus("2017", InProgress))
+      val inProg = probe.expectMsgType[Option[Filing]].get
+      inProg.status mustBe InProgress
+      val startTime = inProg.start
+      startTime must be > 0L
+      inProg.end mustBe 0L
+
+      probe.send(filingsActor, UpdateFilingStatus("2017", Completed))
+      val comp = probe.expectMsgType[Option[Filing]].get
+      comp.status mustBe Completed
+      comp.start mustBe startTime
+      comp.end must be > 0L
+    }
   }
 
 }