Merge pull request #5656 from HHS/OPS-4523/service-req-type

feat(agreements): require service_requirement_type for Contract and AA agreements

Author: johndeange

backend/ops_api/ops/resources/agreements.py

@@ -407,7 +407,7 @@ def _update(id: int, message_prefix: str, meta: OpsEventHandler, partial: bool =
 
     data["agreement_cls"] = AGREEMENT_TYPE_TO_CLASS_MAPPING.get(old_agreement.agreement_type)
 
-    agreement, status_code = service.update(old_agreement.id, data)
+    agreement, status_code = service.update(old_agreement.id, data, partial=partial)
 
     response_schema = AGREEMENT_ITEM_TYPE_TO_RESPONSE_MAPPING.get(agreement.agreement_type)()
     agreement_dict = response_schema.dump(agreement)

backend/ops_api/ops/services/agreements.py

@@ -49,6 +49,8 @@
 from ops_api.ops.utils.events import OpsEventHandler
 from ops_api.ops.validation.agreement_validator import AgreementValidator
 from ops_api.ops.validation.awarded_agreement_validator import AwardedAgreementValidator
+from ops_api.ops.validation.context import ValidationContext
+from ops_api.ops.validation.rules.agreement import ServiceRequirementTypeRule
 
 
 @dataclass
@@ -145,6 +147,16 @@ def create(self, create_request: dict[str, Any]) -> tuple[Agreement, dict[str, A
             ValidationError: If validation fails (e.g., invalid services_component_ref)
             ResourceNotFoundError: If referenced entities don't exist (e.g., invalid can_id)
         """
+        # Validate service_requirement_type for Contract and AA agreements
+        ServiceRequirementTypeRule().validate(
+            None,
+            ValidationContext(
+                user=get_current_user(),
+                updated_fields=create_request,
+                db_session=self.db_session,
+            ),
+        )
+
         # STEP 0: Extract nested entity data from request
         budget_line_items_data = create_request.pop("budget_line_items", [])
         services_components_data = create_request.pop("services_components", [])
@@ -316,7 +328,7 @@ def _create_budget_line_items(
 
         return bli_count
 
-    def update(self, id: int, updated_fields: dict[str, Any]) -> tuple[Agreement, int]:
+    def update(self, id: int, updated_fields: dict[str, Any], partial: bool = True) -> tuple[Agreement, int]:
         """
         Update an existing agreement
         """
@@ -329,7 +341,7 @@ def update(self, id: int, updated_fields: dict[str, Any]) -> tuple[Agreement, in
         else:
             validator = AgreementValidator()
 
-        validator.validate(agreement, user, updated_fields, self.db_session)
+        validator.validate(agreement, user, updated_fields, self.db_session, metadata={"full_update": not partial})
 
         agreement_cls = updated_fields.get("agreement_cls")
         del updated_fields["agreement_cls"]

backend/ops_api/ops/validation/agreement_validator.py

@@ -39,6 +39,7 @@ def _get_default_validators(self) -> List[ValidationRule]:
             ProcurementShopChangeRule,
             ResearchMetadataRule,
             ResourceExistsRule,
+            ServiceRequirementTypeRule,
         )
 
         return [
@@ -47,9 +48,17 @@ def _get_default_validators(self) -> List[ValidationRule]:
             AgreementTypeImmutableRule(),
             ProcurementShopChangeRule(),
             ResearchMetadataRule(),
+            ServiceRequirementTypeRule(),
         ]
 
-    def validate(self, agreement: Agreement, user: User, updated_fields: Dict[str, Any], db_session: Session) -> None:
+    def validate(
+        self,
+        agreement: Agreement,
+        user: User,
+        updated_fields: Dict[str, Any],
+        db_session: Session,
+        metadata: Dict[str, Any] = None,
+    ) -> None:
         """
         Execute all validation rules in sequence.
 
@@ -58,14 +67,17 @@ def validate(self, agreement: Agreement, user: User, updated_fields: Dict[str, A
             user: The user making the request
             updated_fields: Dictionary of fields being updated
             db_session: Database session for queries
+            metadata: Optional metadata for validation context
 
         Raises:
             ValidationError: If any validation fails
             ResourceNotFoundError: If resource not found
             AuthorizationError: If authorization fails
         """
         # Create validation context
-        context = ValidationContext(user=user, updated_fields=updated_fields, db_session=db_session)
+        context = ValidationContext(
+            user=user, updated_fields=updated_fields, db_session=db_session, metadata=metadata or {}
+        )
 
         # Execute all validators
         for validator in self.validators:

backend/ops_api/ops/validation/rules/agreement.py

@@ -1,6 +1,6 @@
 """Concrete validation rules for agreement updates."""
 
-from models import Agreement, BudgetLineItemStatus, ResearchMethodology, SpecialTopic
+from models import Agreement, AgreementType, BudgetLineItemStatus, ResearchMethodology, SpecialTopic
 from ops_api.ops.services.ops_service import (
     AuthorizationError,
     ResourceNotFoundError,
@@ -125,3 +125,37 @@ def validate(self, agreement: Agreement, context: ValidationContext) -> None:
 
             if invalid_ids:
                 raise ValidationError({"special_topics": [f"Special Topic IDs do not exist: {invalid_ids}"]})
+
+
+class ServiceRequirementTypeRule(ValidationRule):
+    """Validates that Contract and AA agreements have service_requirement_type set."""
+
+    @property
+    def name(self) -> str:
+        return "Service Requirement Type Required"
+
+    def validate(self, agreement: Agreement, context: ValidationContext) -> None:
+        updated_fields = context.updated_fields
+
+        agreement_type = updated_fields.get("agreement_type") or (agreement.agreement_type if agreement else None)
+
+        if agreement_type not in (AgreementType.CONTRACT, AgreementType.AA):
+            return
+
+        if "service_requirement_type" in updated_fields:
+            if updated_fields["service_requirement_type"] is None:
+                raise ValidationError(
+                    {"service_requirement_type": "Service Requirement Type is required for Contract and AA agreements."}
+                )
+            return
+
+        if agreement is None:
+            raise ValidationError(
+                {"service_requirement_type": "Service Requirement Type is required for Contract and AA agreements."}
+            )
+
+        if agreement.service_requirement_type is None:
+            if context.metadata.get("full_update"):
+                raise ValidationError(
+                    {"service_requirement_type": "Service Requirement Type is required for Contract and AA agreements."}
+                )

backend/ops_api/tests/ops/agreement/test_agreement.py

@@ -1387,12 +1387,25 @@ def test_post_iaa_agreement(auth_client, loaded_db, app_ctx):
     assert response.status_code == 200
 
 
+def test_agreements_post_contract_without_service_requirement_type_returns_400(auth_client, loaded_db, app_ctx):
+    response = auth_client.post(
+        url_for("api.agreements-group"),
+        json={
+            "agreement_type": AgreementType.CONTRACT.name,
+            "name": "Test Contract (missing SRT)",
+        },
+    )
+    assert response.status_code == 400
+    assert "service_requirement_type" in response.json["errors"]
+
+
 def test_agreements_post(auth_client, loaded_db, app_ctx):
     response = auth_client.post(
         url_for("api.agreements-group"),
         json={
             "agreement_type": AgreementType.CONTRACT.name,
             "name": "Test Contract (for post)",
+            "service_requirement_type": "SEVERABLE",
         },
     )
     assert response.status_code == 201
@@ -1471,6 +1484,7 @@ def test_agreements_post_contract_with_vendor(
             "project_id": test_project.id,
             "awarding_entity_id": 2,
             "contract_type": "FIRM_FIXED_PRICE",
+            "service_requirement_type": "SEVERABLE",
             "research_methodologies": [
                 {
                     "id": 1,

backend/ops_api/tests/ops/budget_line_items/test_budget_line_item.py

@@ -815,6 +815,7 @@ def test_budget_line_item_validation_create_invalid(auth_client, app, test_can,
         "agreement_type": "CONTRACT",
         "agreement_reason": "NEW_REQ",
         "name": "TEST: Agreement for BLI Validation",
+        "service_requirement_type": "SEVERABLE",
         "team_members": [
             {
                 "id": 520,
@@ -862,6 +863,7 @@ def test_budget_line_item_validation_patch_to_invalid(auth_client, app, test_can
         "agreement_type": "CONTRACT",
         "agreement_reason": "NEW_REQ",
         "name": "TEST: Agreement for BLI Validation",
+        "service_requirement_type": "SEVERABLE",
         "team_members": [
             {
                 "id": 520,
@@ -919,6 +921,7 @@ def test_budget_line_item_validation_patch_to_zero_or_negative_amount(
         "agreement_type": "CONTRACT",
         "agreement_reason": "NEW_REQ",
         "name": "TEST: Agreement for BLI Validation",
+        "service_requirement_type": "SEVERABLE",
         "description": "Description",
         "awarding_entity_id": 2,
         "product_service_code_id": 1,
@@ -973,6 +976,7 @@ def test_budget_line_item_validation_patch_to_invalid_date(auth_client, app, tes
         "agreement_type": "CONTRACT",
         "agreement_reason": "NEW_REQ",
         "name": "TEST: Agreement for BLI Validation",
+        "service_requirement_type": "SEVERABLE",
         "description": "Description",
         "awarding_entity_id": 2,
         "product_service_code_id": 1,

backend/ops_api/tests/ops/features/test_edit_agreement_metadata.py

@@ -41,6 +41,7 @@ def test_contract(loaded_db, test_project, test_admin_user, test_vendor):
         name="Feature Test Contract",
         contract_number="CT0999",
         contract_type=ContractType.FIRM_FIXED_PRICE,
+        service_requirement_type=ServiceRequirementType.SEVERABLE,
         agreement_type=AgreementType.CONTRACT,
         project_id=test_project.id,
         project_officer_id=test_admin_user.id,
@@ -243,6 +244,7 @@ def test_contract_with_division_director(loaded_db, test_project):
         contract_number="CT0111",
         contract_type=ContractType.FIRM_FIXED_PRICE,
         agreement_type=AgreementType.CONTRACT,
+        service_requirement_type=ServiceRequirementType.SEVERABLE,
         project_id=test_project.id,
     )
     loaded_db.add(contract_agreement)

backend/ops_api/tests/ops/history/test_history.py

@@ -135,6 +135,7 @@ def test_history_expanded_with_web_client(auth_client, loaded_db, test_user, tes
         "name": "Contract123",
         "description": "History Test Description",
         "product_service_code_id": 1,
+        "service_requirement_type": "SEVERABLE",
         "project_officer_id": test_user.id,
         "team_members": [
             {

backend/ops_api/tests/ops/services/test_agreements.py

@@ -79,7 +79,10 @@ def test_immediate_change_with_all_draft_and_update_fees(service):
     assert result is None
 
 
-def test_create_with_bad_research_methodologies(service):
+@patch("ops_api.ops.services.agreements.get_current_user")
+def test_create_with_bad_research_methodologies(mock_get_user, service):
+    mock_get_user.return_value = MagicMock(id=1)
+
     create_request = {
         "agreement_cls": ContractAgreement,
         "name": "Test Agreement",
@@ -608,6 +611,10 @@ def test_no_award_type_filter_returns_all(self, loaded_db, app_ctx):
 class TestAgreementsAtomicCreation:
     """Test suite for atomic agreement creation with nested entities"""
 
+    @pytest.fixture(autouse=True)
+    def _mock_current_user(self, monkeypatch):
+        monkeypatch.setattr("ops_api.ops.services.agreements.get_current_user", lambda: MagicMock(id=1))
+
     def test_create_agreement_with_budget_line_items_without_services_component_ref(self, loaded_db, app_ctx):
         """Test that budget line items can be created without services_component_ref (backward compatibility)"""
         service = AgreementsService(loaded_db)
@@ -761,6 +768,10 @@ def test_create_agreement_with_default_numeric_sc_refs(self, loaded_db, app_ctx)
 class TestAgreementsAtomicCreationRollback:
     """Test suite for transaction rollback behavior in atomic agreement creation"""
 
+    @pytest.fixture(autouse=True)
+    def _mock_current_user(self, monkeypatch):
+        monkeypatch.setattr("ops_api.ops.services.agreements.get_current_user", lambda: MagicMock(id=1))
+
     def test_invalid_can_id_causes_complete_rollback(self, loaded_db, app_ctx):
         """Test that invalid CAN ID causes complete rollback - no agreement or BLIs created"""
         from ops_api.ops.services.ops_service import ResourceNotFoundError
@@ -1097,6 +1108,10 @@ def test_first_bli_failure_prevents_subsequent_bli_creation(self, loaded_db, app
 class TestAgreementsDuplicateNameHandling:
     """Test suite for duplicate agreement name validation"""
 
+    @pytest.fixture(autouse=True)
+    def _mock_current_user(self, monkeypatch):
+        monkeypatch.setattr("ops_api.ops.services.agreements.get_current_user", lambda: MagicMock(id=1))
+
     def test_create_agreement_with_duplicate_name_raises_validation_error(self, loaded_db, app_ctx):
         """Test that creating an agreement with a duplicate name (same type) raises ValidationError"""
         service = AgreementsService(loaded_db)