From 52bd76e6ee4ee220697f6e4997a11d00364e639b Mon Sep 17 00:00:00 2001 From: richard-dds Date: Tue, 7 Aug 2018 15:16:38 -0400 Subject: [PATCH 1/8] Use proper status names for requests --- atst/domain/requests.py | 22 ++++++++++++---------- tests/domain/test_requests.py | 11 ++++++++--- tests/models/__init__.py | 0 tests/models/test_requests.py | 5 +++++ 4 files changed, 25 insertions(+), 13 deletions(-) create mode 100644 tests/models/__init__.py create mode 100644 tests/models/test_requests.py diff --git a/atst/domain/requests.py b/atst/domain/requests.py index 5c04d3c7..f9e83fe3 100644 --- a/atst/domain/requests.py +++ b/atst/domain/requests.py @@ -26,6 +26,12 @@ 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 Requests(object): AUTO_APPROVE_THRESHOLD = 1000000 @@ -33,8 +39,7 @@ class Requests(object): def create(cls, creator_id, body): request = Request(creator=creator_id, body=body) - status_event = RequestStatusEvent(new_status="incomplete") - request.status_events.append(status_event) + request.status_events.append(RequestStatuses.new("started")) db.session.add(request) db.session.commit() @@ -74,10 +79,12 @@ class Requests(object): @classmethod def submit(cls, request): - request.status_events.append(RequestStatusEvent(new_status="submitted")) - if Requests.should_auto_approve(request): - request.status_events.append(RequestStatusEvent(new_status="approved")) + request.status_events.append( + RequestStatuses.new("pending_financial_verification") + ) + else: + request.status_events.append(RequestStatuses.new("pending_ccpo_approval")) db.session.add(request) db.session.commit() @@ -100,11 +107,6 @@ class Requests(object): request.body = deep_merge(request_delta, request.body) - if Requests.should_allow_submission(request): - request.status_events.append( - RequestStatusEvent(new_status="pending_submission") - ) - # Without this, sqlalchemy won't notice the change to request.body, # since it doesn't track dictionary mutations by default. flag_modified(request, "body") diff --git a/tests/domain/test_requests.py b/tests/domain/test_requests.py index 0597714a..14c4127e 100644 --- a/tests/domain/test_requests.py +++ b/tests/domain/test_requests.py @@ -23,22 +23,27 @@ def test_nonexistent_request_raises(): Requests.get(uuid4()) +def test_new_request_has_started_status(): + request = Requests.create(uuid4(), {}) + assert request.status == "started" + + def test_auto_approve_less_than_1m(new_request): new_request.body = {"details_of_use": {"dollar_value": 999999}} request = Requests.submit(new_request) - assert request.status == 'approved' + assert request.status == "pending_financial_verification" def test_dont_auto_approve_if_dollar_value_is_1m_or_above(new_request): new_request.body = {"details_of_use": {"dollar_value": 1000000}} request = Requests.submit(new_request) - assert request.status == 'submitted' + assert request.status == "pending_ccpo_approval" def test_dont_auto_approve_if_no_dollar_value_specified(new_request): new_request.body = {"details_of_use": {}} request = Requests.submit(new_request) - assert request.status == 'submitted' + assert request.status == "pending_ccpo_approval" diff --git a/tests/models/__init__.py b/tests/models/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/models/test_requests.py b/tests/models/test_requests.py new file mode 100644 index 00000000..245c587e --- /dev/null +++ b/tests/models/test_requests.py @@ -0,0 +1,5 @@ +from tests.factories import RequestFactory + + +def test_started_request_requires_mo_action(): + request = RequestFactory.create() From 0c9005aaf6b681d0997f7e445045e45fa8fd3305 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Tue, 7 Aug 2018 16:22:24 -0400 Subject: [PATCH 2/8] 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" From 49b9fca79399c2a5fb82dd94de5d7b52516a2062 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Tue, 7 Aug 2018 16:41:47 -0400 Subject: [PATCH 3/8] Use Enum for request statuses --- atst/domain/requests.py | 13 +++---------- atst/models/request.py | 1 + atst/models/request_status_event.py | 14 +++++++++++--- tests/domain/test_requests.py | 9 +++++---- tests/factories.py | 14 +++++--------- 5 files changed, 25 insertions(+), 26 deletions(-) diff --git a/atst/domain/requests.py b/atst/domain/requests.py index 17ceb9b0..4c1d7bef 100644 --- a/atst/domain/requests.py +++ b/atst/domain/requests.py @@ -1,9 +1,9 @@ -from enum import Enum from sqlalchemy import exists, and_ from sqlalchemy.orm.exc import NoResultFound from sqlalchemy.orm.attributes import flag_modified -from atst.models import Request, RequestStatusEvent +from atst.models.request import Request +from atst.models.request_status_event import RequestStatusEvent, RequestStatus from atst.database import db from .exceptions import NotFoundError @@ -27,13 +27,6 @@ def deep_merge(source, destination: dict): return _deep_merge(source, dict(destination)) -class RequestStatus(Enum): - STARTED = "started" - PENDING_FINANCIAL_VERIFICATION = "pending_financial_verification" - PENDING_CCPO_APPROVAL = "pending_ccpo_approval" - APPROVED = "approved" - - class Requests(object): AUTO_APPROVE_THRESHOLD = 1000000 @@ -118,7 +111,7 @@ class Requests(object): @classmethod def set_status(cls, request: Request, status: RequestStatus): - status_event = RequestStatusEvent(new_status=status.value) + status_event = RequestStatusEvent(new_status=status) request.status_events.append(status_event) return request diff --git a/atst/models/request.py b/atst/models/request.py index 36b9b77c..147d28f1 100644 --- a/atst/models/request.py +++ b/atst/models/request.py @@ -24,3 +24,4 @@ class Request(Base): def set_status(self, status): self.status_events.append(status) + diff --git a/atst/models/request_status_event.py b/atst/models/request_status_event.py index 4f65a2ec..7eec2716 100644 --- a/atst/models/request_status_event.py +++ b/atst/models/request_status_event.py @@ -1,5 +1,6 @@ -from sqlalchemy import Column, func, ForeignKey -from sqlalchemy.types import DateTime, String, BigInteger +from enum import Enum, auto +from sqlalchemy import Column, func, ForeignKey, Enum as SQLAEnum +from sqlalchemy.types import DateTime, BigInteger from sqlalchemy.schema import Sequence from sqlalchemy.dialects.postgresql import UUID @@ -7,11 +8,18 @@ from atst.models import Base from atst.models.types import Id +class RequestStatus(Enum): + STARTED = auto() + PENDING_FINANCIAL_VERIFICATION = auto() + PENDING_CCPO_APPROVAL = auto() + APPROVED = auto() + + class RequestStatusEvent(Base): __tablename__ = "request_status_events" id = Id() - new_status = Column(String()) + new_status = Column(SQLAEnum(RequestStatus)) time_created = Column(DateTime(timezone=True), server_default=func.now()) request_id = Column( UUID(as_uuid=True), ForeignKey("requests.id", ondelete="CASCADE") diff --git a/tests/domain/test_requests.py b/tests/domain/test_requests.py index 14c4127e..1d696097 100644 --- a/tests/domain/test_requests.py +++ b/tests/domain/test_requests.py @@ -3,6 +3,7 @@ from uuid import uuid4 from atst.domain.exceptions import NotFoundError from atst.domain.requests import Requests +from atst.models.request_status_event import RequestStatus from tests.factories import RequestFactory @@ -25,25 +26,25 @@ def test_nonexistent_request_raises(): def test_new_request_has_started_status(): request = Requests.create(uuid4(), {}) - assert request.status == "started" + assert request.status == RequestStatus.STARTED def test_auto_approve_less_than_1m(new_request): new_request.body = {"details_of_use": {"dollar_value": 999999}} request = Requests.submit(new_request) - assert request.status == "pending_financial_verification" + assert request.status == RequestStatus.PENDING_FINANCIAL_VERIFICATION def test_dont_auto_approve_if_dollar_value_is_1m_or_above(new_request): new_request.body = {"details_of_use": {"dollar_value": 1000000}} request = Requests.submit(new_request) - assert request.status == "pending_ccpo_approval" + assert request.status == RequestStatus.PENDING_CCPO_APPROVAL def test_dont_auto_approve_if_no_dollar_value_specified(new_request): new_request.body = {"details_of_use": {}} request = Requests.submit(new_request) - assert request.status == "pending_ccpo_approval" + assert request.status == RequestStatus.PENDING_CCPO_APPROVAL diff --git a/tests/factories.py b/tests/factories.py index 65395479..dc68eb5c 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -1,7 +1,8 @@ import factory from uuid import uuid4 -from atst.models import Request, RequestStatusEvent +from atst.models.request import Request +from atst.models.request_status_event import RequestStatusEvent, RequestStatus from atst.models.pe_number import PENumber from atst.models.task_order import TaskOrder from atst.models.user import User @@ -9,34 +10,31 @@ 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") + status_events = factory.RelatedFactory( + RequestStatusFactory, "request", new_status=RequestStatus.STARTED + ) class PENumberFactory(factory.alchemy.SQLAlchemyModelFactory): - class Meta: model = PENumber class TaskOrderFactory(factory.alchemy.SQLAlchemyModelFactory): - class Meta: model = TaskOrder class RoleFactory(factory.alchemy.SQLAlchemyModelFactory): - class Meta: model = Role @@ -44,7 +42,6 @@ class RoleFactory(factory.alchemy.SQLAlchemyModelFactory): class UserFactory(factory.alchemy.SQLAlchemyModelFactory): - class Meta: model = User @@ -53,4 +50,3 @@ class UserFactory(factory.alchemy.SQLAlchemyModelFactory): first_name = "Fake" last_name = "User" atat_role = factory.SubFactory(RoleFactory) - From 5381e68a8d245b7487389ea8e6d6e8e43e8295ab Mon Sep 17 00:00:00 2001 From: richard-dds Date: Tue, 7 Aug 2018 17:01:13 -0400 Subject: [PATCH 4/8] Add a migration to convert request statuses --- .../77b065750596_new_request_statuses.py | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 alembic/versions/77b065750596_new_request_statuses.py diff --git a/alembic/versions/77b065750596_new_request_statuses.py b/alembic/versions/77b065750596_new_request_statuses.py new file mode 100644 index 00000000..28cce335 --- /dev/null +++ b/alembic/versions/77b065750596_new_request_statuses.py @@ -0,0 +1,49 @@ +"""new request statuses + +Revision ID: 77b065750596 +Revises: 1f57f784ed5b +Create Date: 2018-08-07 16:42:11.502361 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.orm.session import sessionmaker + +from atst.models.request_status_event import RequestStatus + + +# revision identifiers, used by Alembic. +revision = '77b065750596' +down_revision = '1f57f784ed5b' +branch_labels = None +depends_on = None + + +def upgrade(): + """ + Update all existing request statuses so that the state of the + table reflects the statuses listed in RequestStatus. + + This involves fixing the casing on existing statuses, and + deleting statuses that have no match. + """ + + db = op.get_bind() + + status_events = db.execute("SELECT * FROM request_status_events").fetchall() + for status_event in status_events: + try: + status = RequestStatus[status_event["new_status"].upper()] + query = sa.text(""" + UPDATE request_status_events + SET new_status = :status + WHERE id = :id""" + ) + db.execute(query, id=status_event["id"], status=status.name) + except ValueError: + query = sa.text("DELETE FROM request_status_events WHERE id = :id") + db.execute(query, id=status_event["id"]) + + +def downgrade(): + pass From e437b0d6baf0ebcefc71ccdf88efbd6541025027 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Tue, 7 Aug 2018 17:07:25 -0400 Subject: [PATCH 5/8] str -> RequestStatus coercion is no longer necessary --- atst/domain/requests.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/atst/domain/requests.py b/atst/domain/requests.py index 4c1d7bef..e7b64e5d 100644 --- a/atst/domain/requests.py +++ b/atst/domain/requests.py @@ -117,16 +117,11 @@ class Requests(object): @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) + }.get(request.status) @classmethod def should_auto_approve(cls, request): From fc7ef59e9bf8fc0589eb8cf626dcff648e75495e Mon Sep 17 00:00:00 2001 From: richard-dds Date: Wed, 8 Aug 2018 11:18:18 -0400 Subject: [PATCH 6/8] Remove unused Request.set_status --- atst/models/request.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/atst/models/request.py b/atst/models/request.py index 147d28f1..66e5bf1e 100644 --- a/atst/models/request.py +++ b/atst/models/request.py @@ -21,7 +21,3 @@ class Request(Base): @property def status(self): return self.status_events[-1].new_status - - def set_status(self, status): - self.status_events.append(status) - From 4d3889c14428b6e07b03007ade5bfd7df32fe428 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Wed, 8 Aug 2018 13:04:40 -0400 Subject: [PATCH 7/8] Hardcode enum values instead of using auto() --- atst/models/request_status_event.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/atst/models/request_status_event.py b/atst/models/request_status_event.py index 7eec2716..757d033d 100644 --- a/atst/models/request_status_event.py +++ b/atst/models/request_status_event.py @@ -1,4 +1,4 @@ -from enum import Enum, auto +from enum import Enum from sqlalchemy import Column, func, ForeignKey, Enum as SQLAEnum from sqlalchemy.types import DateTime, BigInteger from sqlalchemy.schema import Sequence @@ -9,10 +9,10 @@ from atst.models.types import Id class RequestStatus(Enum): - STARTED = auto() - PENDING_FINANCIAL_VERIFICATION = auto() - PENDING_CCPO_APPROVAL = auto() - APPROVED = auto() + STARTED = "started" + PENDING_FINANCIAL_VERIFICATION = "pending_financial_verification" + PENDING_CCPO_APPROVAL = "pending_ccpo_approval" + APPROVED = "approved" class RequestStatusEvent(Base): From 1f41b717bfcfade3971ab3391e254251634ba042 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Wed, 8 Aug 2018 13:05:03 -0400 Subject: [PATCH 8/8] Add expired and deleted statuses --- atst/models/request_status_event.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/atst/models/request_status_event.py b/atst/models/request_status_event.py index 757d033d..2e492ac4 100644 --- a/atst/models/request_status_event.py +++ b/atst/models/request_status_event.py @@ -13,6 +13,8 @@ class RequestStatus(Enum): PENDING_FINANCIAL_VERIFICATION = "pending_financial_verification" PENDING_CCPO_APPROVAL = "pending_ccpo_approval" APPROVED = "approved" + EXPIRED = "expired" + DELETED = "deleted" class RequestStatusEvent(Base):