Merge pull request cfpb#1118 from schbetsy/filing-status

Fix for Filing Status Bugs

Author: Nick Grippin

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

@@ -1,7 +1,7 @@
 package hmda.persistence.institutions
 
 import akka.actor.{ ActorRef, ActorSystem, Props }
-import hmda.model.fi.{ Completed, Filing, InProgress, NotStarted }
+import hmda.model.fi._
 import hmda.persistence.messages.CommonMessages._
 import hmda.persistence.institutions.FilingPersistence._
 import hmda.persistence.model.HmdaPersistentActor
@@ -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))
@@ -48,38 +48,26 @@ class FilingPersistence(institutionId: String) extends HmdaPersistentActor {
 
   override def receiveCommand: Receive = super.receiveCommand orElse {
     case CreateFiling(f) =>
-      if (!state.filings.contains(f)) {
-        persist(FilingCreated(f)) { e =>
-          log.debug(s"Persisted: $f")
-          updateState(e)
-          sender() ! Some(f)
-        }
+      if (!state.filings.map(_.period).contains(f.period)) {
+        persistFilingEvent(FilingCreated(f), f)
       } else {
         sender() ! None
-        log.warning(s"Filing already exists. Could not create $f")
+        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) if filing.status == Completed =>
+          sender() ! None
+          log.debug(s"$period Filing for Institution $institutionId is already Completed. Not updating status")
+        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)
+          persistFilingEvent(FilingStatusUpdated(updatedFiling), updatedFiling)
+        case None =>
+          sender() ! None
+          log.warning(s"Could not update filing status. Institution $institutionId, filing period $period")
       }
 
     case GetFilingByPeriod(period) =>
@@ -94,4 +82,12 @@ class FilingPersistence(institutionId: String) extends HmdaPersistentActor {
       sender() ! state.filings
 
   }
+
+  private def persistFilingEvent(event: Event, filing: Filing) = {
+    persist(event) { e =>
+      log.debug(s"persisted: $filing")
+      updateState(e)
+      sender() ! Some(filing)
+    }
+  }
 }

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

@@ -164,12 +164,7 @@ 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))
-    }
+    filingPersistence.map(_ ? UpdateFilingStatus(period, filingStatus))
   }
 
 }

persistence/src/test/resources/application.conf

@@ -14,4 +14,5 @@ akka.test.single-expect-default = 10s
 
 hmda {
   isDemo = false
+  edits.demoMode = false
 }

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

@@ -3,48 +3,68 @@ 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
 
 class FilingPersistenceSpec extends ActorSpec {
 
   val filingsActor = createFilings("12345", system)
 
+  val sample1 = Filing("2016", "12345", Cancelled, filingRequired = true, 1483287071000L, 1514736671000L)
+  val sample2 = Filing("2017", "12345", NotStarted, filingRequired = true, 0L, 0L)
+
   val probe = TestProbe()
 
-  "Filings" must {
-    "be created and read back" in {
-      val filings = DemoData.testFilings
+  "CreateFiling" must {
+    "create a filing" in {
+      val filings = Seq(sample1, sample2)
       for (filing <- filings) {
         probe.send(filingsActor, CreateFiling(filing))
         probe.expectMsg(Some(filing))
       }
       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 that already exists" in {
-      val f1 = Filing("2016", "12345", Completed, true, 1483287071000L, 1514736671000L)
+    "return Some when creating filing for period with no filings" in {
+      val f1 = Filing("2018", "12345", Completed, true, 1483287071000L, 1514736671000L)
       probe.send(filingsActor, CreateFiling(f1))
       probe.expectMsg(Some(f1))
-
-      val f2 = Filing("2016", "12345", Completed, true, 1483287071000L, 1514736671000L)
+    }
+    "return None when persisting a filing period that already has a filing" in {
+      val f2 = Filing("2016", "12345", InProgress, true, 1483287071000L, 0L)
       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 = NotStarted)
+      probe.send(filingsActor, UpdateFilingStatus("2016", NotStarted))
+      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
+    }
+    "not update if filing status is already 'Completed'" in {
+      probe.send(filingsActor, UpdateFilingStatus("2017", InProgress))
       probe.expectMsg(None)
     }
   }

validation/src/test/resources/application.conf

@@ -15,5 +15,6 @@ akka {
 
 hmda {
   isDemo = false
+  edits.demoMode = false
   persistent-actor-timeout = 60
 }