From 0c9005aaf6b681d0997f7e445045e45fa8fd3305 Mon Sep 17 00:00:00 2001
From: richard-dds <richard-ctr@friends.dds.mil>
Date: Tue, 7 Aug 2018 16:22:24 -0400
Subject: [PATCH] Fix and refactor action_required_by

---
 atst/domain/requests.py       | 41 ++++++++++++++++++++++++++---------
 atst/models/request.py        | 10 ++-------
 tests/factories.py            |  9 +++++++-
 tests/models/test_requests.py | 16 ++++++++++++++
 4 files changed, 57 insertions(+), 19 deletions(-)

diff --git a/atst/domain/requests.py b/atst/domain/requests.py
index f9e83fe3..17ceb9b0 100644
--- a/atst/domain/requests.py
+++ b/atst/domain/requests.py
@@ -1,3 +1,4 @@
+from enum import Enum
 from sqlalchemy import exists, and_
 from sqlalchemy.orm.exc import NoResultFound
 from sqlalchemy.orm.attributes import flag_modified
@@ -26,10 +27,11 @@ def deep_merge(source, destination: dict):
     return _deep_merge(source, dict(destination))
 
 
-class RequestStatuses(object):
-    @classmethod
-    def new(cls, status_name):
-        return RequestStatusEvent(new_status=status_name)
+class RequestStatus(Enum):
+    STARTED = "started"
+    PENDING_FINANCIAL_VERIFICATION = "pending_financial_verification"
+    PENDING_CCPO_APPROVAL = "pending_ccpo_approval"
+    APPROVED = "approved"
 
 
 class Requests(object):
@@ -38,8 +40,7 @@ class Requests(object):
     @classmethod
     def create(cls, creator_id, body):
         request = Request(creator=creator_id, body=body)
-
-        request.status_events.append(RequestStatuses.new("started"))
+        request = Requests.set_status(request, RequestStatus.STARTED)
 
         db.session.add(request)
         db.session.commit()
@@ -79,12 +80,13 @@ class Requests(object):
 
     @classmethod
     def submit(cls, request):
+        new_status = None
         if Requests.should_auto_approve(request):
-            request.status_events.append(
-                RequestStatuses.new("pending_financial_verification")
-            )
+            new_status = RequestStatus.PENDING_FINANCIAL_VERIFICATION
         else:
-            request.status_events.append(RequestStatuses.new("pending_ccpo_approval"))
+            new_status = RequestStatus.PENDING_CCPO_APPROVAL
+
+        request = Requests.set_status(request, new_status)
 
         db.session.add(request)
         db.session.commit()
@@ -114,6 +116,25 @@ class Requests(object):
         db.session.add(request)
         db.session.commit()
 
+    @classmethod
+    def set_status(cls, request: Request, status: RequestStatus):
+        status_event = RequestStatusEvent(new_status=status.value)
+        request.status_events.append(status_event)
+        return request
+
+    @classmethod
+    def action_required_by(cls, request):
+        try:
+            status = RequestStatus(request.status)
+        except ValueError:
+            return None
+
+        return {
+            RequestStatus.STARTED: "mission_owner",
+            RequestStatus.PENDING_FINANCIAL_VERIFICATION: "mission_owner",
+            RequestStatus.PENDING_CCPO_APPROVAL: "ccpo"
+        }.get(status)
+
     @classmethod
     def should_auto_approve(cls, request):
         try:
diff --git a/atst/models/request.py b/atst/models/request.py
index e8690332..36b9b77c 100644
--- a/atst/models/request.py
+++ b/atst/models/request.py
@@ -22,11 +22,5 @@ class Request(Base):
     def status(self):
         return self.status_events[-1].new_status
 
-    @property
-    def action_required_by(self):
-        return {
-            "incomplete": "mission_owner",
-            "pending_submission": "mission_owner",
-            "submitted": "ccpo",
-            "approved": "mission_owner",
-        }.get(self.status)
+    def set_status(self, status):
+        self.status_events.append(status)
diff --git a/tests/factories.py b/tests/factories.py
index 5cefc1eb..65395479 100644
--- a/tests/factories.py
+++ b/tests/factories.py
@@ -1,19 +1,26 @@
 import factory
 from uuid import uuid4
 
-from atst.models import Request
+from atst.models import Request, RequestStatusEvent
 from atst.models.pe_number import PENumber
 from atst.models.task_order import TaskOrder
 from atst.models.user import User
 from atst.models.role import Role
 
 
+class RequestStatusFactory(factory.alchemy.SQLAlchemyModelFactory):
+
+    class Meta:
+        model = RequestStatusEvent
+
+
 class RequestFactory(factory.alchemy.SQLAlchemyModelFactory):
 
     class Meta:
         model = Request
 
     id = factory.Sequence(lambda x: uuid4())
+    status_events = factory.RelatedFactory(RequestStatusFactory, "request", new_status="started")
 
 
 class PENumberFactory(factory.alchemy.SQLAlchemyModelFactory):
diff --git a/tests/models/test_requests.py b/tests/models/test_requests.py
index 245c587e..a1990a35 100644
--- a/tests/models/test_requests.py
+++ b/tests/models/test_requests.py
@@ -1,5 +1,21 @@
 from tests.factories import RequestFactory
+from atst.domain.requests import Requests, RequestStatus
 
 
 def test_started_request_requires_mo_action():
     request = RequestFactory.create()
+    assert Requests.action_required_by(request) == "mission_owner"
+
+
+def test_pending_financial_requires_mo_action():
+    request = RequestFactory.create()
+    request = Requests.set_status(request, RequestStatus.PENDING_FINANCIAL_VERIFICATION)
+
+    assert Requests.action_required_by(request) == "mission_owner"
+
+
+def test_pending_ccpo_approval_requires_ccpo():
+    request = RequestFactory.create()
+    request = Requests.set_status(request, RequestStatus.PENDING_CCPO_APPROVAL)
+
+    assert Requests.action_required_by(request) == "ccpo"