Feat: 410 Report Verification - LFO #2714

shon-button · 2025-01-21T00:21:40Z

Addresses 410

🚀 Impact:

Model Splitting: The ReportVerification model was split to move visit-related fields (visit_name, visit_type, other_facility_name, other_facility_coordinates) into a new model, ReportVerificationVisit
New Relationship: ForeignKey relationship from ReportVerificationVisit to ReportVerification
New Constraint: CheckConstraint in ReportVerificationVisit to enforce that visit_coordinates must be provided when is_other_visit=True
Database schemas: updated to introduce the new verification_visit table

🔬 Local Testing:

From terminal command, start the api server:

cd bc_obps
make reset_db
make run

From terminal command, start the app development server:

cd bciers && yarn dev-all

Tests WIP

Navigate to http://localhost:3000
Click button "Log in with Business BCeID"
Navigate to Bugle SFO - Registered: http://localhost:3000/reporting/reports/1/verification
Navigate to Banana LFO - Registered: http://localhost:3000/reporting/reports/2/verification

chore: update verification service chore: cleanup chore: cleanup test: verification api get chore: add verification operation type chore: ad shared verification schema properties chore: add verification dependacncies chore: shared schemas chore: refactor SFO updateReportVerificationVisits chore: cleanup

pbastia

Nice work!! I've left a few comments, mostly small stuff.

I think there's some RJSF features that we can use to cut down on the schema/data manipulation, ideally we're writing code where only the form drives the data, and the components only needs to (hopefully not too much) transform it before submitting it to the API

pbastia · 2025-01-24T21:38:07Z

bc_obps/reporting/models/report_verification_visit.py

+        constraints = [
+            models.CheckConstraint(
+                name="other_facility_must_have_coordinates",
+                check=~Q(is_other_visit=True) | Q(visit_coordinates__isnull=False),


I think we want to exclude the case "is_other_visit=True and visit_coordinates__isnull=True"

check=~(Q(is_other_visit=True) & Q(visit_coordinates__isnull=True))

pbastia · 2025-01-24T21:39:07Z

bc_obps/reporting/schema/report_verification.py

+    verification_body_name: str = Field(...)
+    accredited_by: str = Field(...)
+    scope_of_verification: str = Field(...)
+    threats_to_independence: bool = Field(...)
+    verification_conclusion: str = Field(...)


these are redundant with the fields = ... in the Meta class, we can just delete these lines

pbastia · 2025-01-24T21:41:22Z

bc_obps/reporting/schema/report_verification.py

+    visit_name: str = Field(...)
+    visit_type: str = Field(choices=ReportVerificationVisit.VisitType.choices)
+    is_other_visit: bool = Field(...)
+    visit_coordinates: str = Field(required=False)


Same as above, I think the choices and required options will be inferred from the model if we just remove these lines

pbastia · 2025-01-24T21:43:12Z

bc_obps/reporting/schema/report_verification.py

+    report_verification_visits: List[ReportVerificationVisitSchema] = Field(default_factory=list) 
+
+    class Meta(BaseReportVerification.Meta):
+        fields = BaseReportVerification.Meta.fields + ['report_version']


For all these schemas we could actually generate them dynamically, since they're just a subset of the fields on the model: https://django-ninja.dev/guides/response/django-pydantic-create-schema/

pbastia · 2025-01-24T21:58:05Z

bc_obps/reporting/service/report_verification_service.py

@@ -13,17 +15,19 @@ class ReportVerificationService:
    @staticmethod
    def get_report_verification_by_version_id(
        report_version_id: int,
-    ) -> ReportVerification:
+    ) -> ReportVerificationOut:


We should consider decoupling the service layer from the API layer who owns these data structures. The service layer can declare similar types, or just return the Model types

pbastia · 2025-01-24T22:02:13Z

bc_obps/reporting/service/report_verification_service.py

+        # Fetch ReportVerificationVisit instances corresponding to the IDs
+        visit_instances_to_keep = ReportVerificationVisit.objects.filter(id__in=visit_ids_to_keep)
+
+        # Update report_verification_visits
+        report_verification.report_verification_visits.set(visit_instances_to_keep)
+


I don't think this is necessary, all the ReportVerificationVisit models have been created with the right report_verification field set, and this is not an m2m relationship.
We can remove these few lines

pbastia · 2025-01-24T22:03:22Z

bc_obps/reporting/tests/api/test_report_verification_api.py

+        # Create related ReportVerificationVisit instances and link them
+        report_verification_visits = baker.make_recipe(
+            "reporting.tests.utils.report_verification_visit",
+            _quantity=2,
+        )
+
+        # Attach the visits to the report_verification instance
+        self.report_verification.report_verification_visits.set(report_verification_visits)


Suggested change

# Create related ReportVerificationVisit instances and link them

report_verification_visits = baker.make_recipe(

"reporting.tests.utils.report_verification_visit",

_quantity=2,

)

# Attach the visits to the report_verification instance

self.report_verification.report_verification_visits.set(report_verification_visits)

# Create related ReportVerificationVisit instances and link them

report_verification_visits = baker.make_recipe(

"reporting.tests.utils.report_verification_visit",

report_verification=report_verification,

_quantity=2,

)

pbastia · 2025-01-24T22:10:35Z

bciers/apps/reporting/src/app/components/verification/VerificationForm.tsx

+      is_other_visit: true,
+      visit_coordinates: formData.visit_others.visit_coordinates || "",
+    });
+  }


I'm pretty sure there is a way to bake this in to the rjsf structure instead, with if then else or oneOf / anyOf. Happy to try to pair on that if you want!

pbastia · 2025-01-24T22:12:09Z

bciers/apps/reporting/src/app/components/verification/VerificationForm.tsx

+      }
+    }
+
+    // 🔄 Update the form data state with the modified data


This is a LOT of logic to run on every single key stroke on the form. I think it would be very helpful to try to build this logic into the schema itself instead

pbastia · 2025-01-24T22:17:50Z

bciers/apps/reporting/src/app/components/verification/VerificationPage.tsx

+      initialData.visit_types = undefined;
+      initialData.visit_others = [{}];
+    }
+  }


This feels like work that could be done on the server by the API layer (maybe in django ninja schema?): preparing the data for the form and the client page

shon-button force-pushed the feat/410-verification-lfo branch 6 times, most recently from 38b4e0b to ed75993 Compare January 23, 2025 16:46

shon-button added 2 commits January 24, 2025 10:27

feat: report verification for LFO

fb74805

chore: update verification RJSF schemas

9f055e5

shon-button force-pushed the feat/410-verification-lfo branch from 94022f2 to 828d63c Compare January 24, 2025 15:28

shon-button force-pushed the feat/410-verification-lfo branch from 1c5ce43 to 26f888f Compare January 24, 2025 20:45

pbastia reviewed Jan 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: 410 Report Verification - LFO #2714

Feat: 410 Report Verification - LFO #2714

shon-button commented Jan 21, 2025 •

edited

Loading

pbastia left a comment

pbastia Jan 24, 2025

pbastia Jan 24, 2025

pbastia Jan 24, 2025

pbastia Jan 24, 2025

pbastia Jan 24, 2025

pbastia Jan 24, 2025

pbastia Jan 24, 2025

pbastia Jan 24, 2025

pbastia Jan 24, 2025

pbastia Jan 24, 2025

Feat: 410 Report Verification - LFO #2714

Are you sure you want to change the base?

Feat: 410 Report Verification - LFO #2714

Conversation

shon-button commented Jan 21, 2025 • edited Loading

🚀 Impact:

🔬 Local Testing:

Tests WIP

pbastia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shon-button commented Jan 21, 2025 •

edited

Loading