Merge pull request #460 from dod-ccpo/finver-bug-#162270400

be more generous in finding PDF attachment file name when rendering f…

atst/domain/workspace_roles.py

@@ -2,7 +2,9 @@ from sqlalchemy.orm.exc import NoResultFound
 
 from atst.database import db
 from atst.models.workspace_role import (
-    WorkspaceRole, Status as WorkspaceRoleStatus, MEMBER_STATUSES
+    WorkspaceRole,
+    Status as WorkspaceRoleStatus,
+    MEMBER_STATUSES,
 )
 from atst.models.user import User
 
@@ -12,8 +14,7 @@ from .exceptions import NotFoundError
 
 
 MEMBER_STATUS_CHOICES = [
-    dict(name=key, display_name=value)
-    for key, value in MEMBER_STATUSES.items()
+    dict(name=key, display_name=value) for key, value in MEMBER_STATUSES.items()
 ]
 
 

atst/forms/financial.py

@@ -219,7 +219,7 @@ class FinancialVerificationForm(CacheableForm):
         return self.task_order.number
 
     @property
-    def has_task_order_pdf(self):
+    def has_pdf_upload(self):
         return isinstance(self.task_order.pdf.data, FileStorage)
 
     @property

atst/models/workspace_role.py

@@ -82,21 +82,21 @@ class WorkspaceRole(Base, mixins.TimestampsMixin, mixins.AuditableMixin):
     @property
     def display_status(self):
         if self.status == Status.ACTIVE:
-            return MEMBER_STATUSES['active']
+            return MEMBER_STATUSES["active"]
         elif self.latest_invitation:
             if self.latest_invitation.is_revoked:
-                return MEMBER_STATUSES['revoked']
+                return MEMBER_STATUSES["revoked"]
             elif self.latest_invitation.is_rejected_wrong_user:
-                return MEMBER_STATUSES['error']
+                return MEMBER_STATUSES["error"]
             elif (
                 self.latest_invitation.is_rejected_expired
                 or self.latest_invitation.is_expired
             ):
-                return MEMBER_STATUSES['expired']
+                return MEMBER_STATUSES["expired"]
             else:
-                return MEMBER_STATUSES['pending']
+                return MEMBER_STATUSES["pending"]
         else:
-            return MEMBER_STATUSES['unknown']
+            return MEMBER_STATUSES["unknown"]
 
     @property
     def has_dod_id_error(self):

atst/routes/requests/financial_verification.py

@@ -31,12 +31,17 @@ class FinancialVerificationBase(object):
         fv = FinancialVerification(request)
         form = FinancialVerificationForm(obj=fv, formdata=_formdata)
 
-        if not form.has_task_order_pdf:
-            try:
-                attachment = Attachment.get_for_resource("task_order", self.request.id)
-                form.task_order.pdf.data = attachment.filename
-            except NotFoundError:
-                pass
+        if not form.has_pdf_upload:
+            if isinstance(form.task_order.pdf.data, Attachment):
+                form.task_order.pdf.data = form.task_order.pdf.data.filename
+            else:
+                try:
+                    attachment = Attachment.get_for_resource(
+                        "task_order", self.request.id
+                    )
+                    form.task_order.pdf.data = attachment.filename
+                except NotFoundError:
+                    pass
 
         return form
 

tests/routes/requests/test_financial_verification.py

@@ -11,13 +11,14 @@ from atst.routes.requests.financial_verification import (
 )
 
 from tests.mocks import MOCK_VALID_PE_ID
-from tests.factories import RequestFactory, UserFactory
+from tests.factories import RequestFactory, UserFactory, TaskOrderFactory
 from atst.forms.exceptions import FormValidationError
 from atst.domain.requests.financial_verification import (
     PENumberValidator,
     TaskOrderNumberValidator,
 )
 from atst.models.request_status_event import RequestStatus
+from atst.models.attachment import Attachment
 from atst.domain.requests.query import RequestsQuery
 
 
@@ -509,3 +510,30 @@ def test_pdf_clearing(fv_data, e_fv_data, pdf_upload, pdf_upload2):
 
     form = GetFinancialVerificationForm(user, request, is_extended=True).execute()
     assert form.task_order.pdf.data == pdf_upload2.filename
+
+
+# TODO: This test manages an edge case for our current non-unique handling of
+# task orders. Because two requests can reference the same task order but we
+# only record one request ID on the PDF attachment, multiple task
+# orders/requests reference the same task order but only one of them is noted
+# in the related attachment entity. I have changed the handling in
+# FinancialVerificationBase#_get_form to be more generous in how it finds the
+# PDF filename and prepopulates the form data with that name.
+def test_always_derives_pdf_filename(fv_data, e_fv_data, pdf_upload):
+    user = UserFactory.create()
+    request_one = RequestFactory.create(creator=user)
+    attachment = Attachment.attach(
+        pdf_upload, resource="task_order", resource_id=request_one.id
+    )
+    task_order = TaskOrderFactory.create(pdf=attachment)
+    request_two = RequestFactory.create(creator=user, task_order=task_order)
+
+    form_one = GetFinancialVerificationForm(
+        user, request_one, is_extended=True
+    ).execute()
+    form_two = GetFinancialVerificationForm(
+        user, request_two, is_extended=True
+    ).execute()
+
+    assert form_one.task_order.pdf.data == attachment.filename
+    assert form_two.task_order.pdf.data == attachment.filename