[Issue #3901] Cleanup remainder of historical transformation code (#3914

)

## Summary
Fixes #3901

### Time to review: 10 mins

## Changes proposed
Remove special logic for the revision_number
Remove logic around processing action_type (a historical only field)
Remove revision_number from all of the get_log_extra_x methods
Remove _is_orphaned_historical and _handle_orphaned_historical
Remove fetch_with_opportunity_summary param to check whether the query
is for historical records
Remove some related type aliases
Remove metric TOTAL_HISTORICAL_ORPHANS_SKIPPED

## Context for reviewers
After #3849 disabled the historical transformation code, we have a few
unused bits of code remaining that we can clean up.

## Additional information
Check with @chouinar if the tests removed in this PR can actually
be/should have been previously removed, as stated the code changes
should not have broken any tests.

api/src/data_migration/transformation/subtask/abstract_transform_subtask.py

@@ -98,26 +98,6 @@ def _handle_delete(
         self.increment(transform_constants.Metrics.TOTAL_RECORDS_DELETED, prefix=record_type)
         self.db_session.delete(target)
 
-    def _is_orphaned_historical(
-        self,
-        parent_record: Opportunity | OpportunitySummary | None,
-        source_record: transform_constants.SourceAny,
-    ) -> bool:
-        return parent_record is None and source_record.is_historical_table
-
-    def _handle_orphaned_historical(
-        self, source_record: transform_constants.SourceAny, record_type: str, extra: dict
-    ) -> None:
-        logger.warning(
-            "Historical %s does not have a corresponding parent record - cannot import, but will mark as processed",
-            record_type,
-            extra=extra,
-        )
-        self.increment(
-            transform_constants.Metrics.TOTAL_HISTORICAL_ORPHANS_SKIPPED, prefix=record_type
-        )
-        source_record.transformation_notes = transform_constants.ORPHANED_HISTORICAL_RECORD
-
     def fetch(
         self,
         source_model: Type[transform_constants.S],
@@ -175,7 +155,6 @@ def fetch_with_opportunity_summary(
         destination_model: Type[transform_constants.D],
         join_clause: Sequence,
         is_forecast: bool,
-        is_historical_table: bool,
         relationship_load_value: Any,
     ) -> list[
         Tuple[transform_constants.S, transform_constants.D | None, OpportunitySummary | None]
@@ -187,12 +166,7 @@ def fetch_with_opportunity_summary(
             OpportunitySummary.is_forecast.is_(is_forecast),
         ]
 
-        if is_historical_table:
-            opportunity_summary_join_clause.append(
-                source_model.revision_number == OpportunitySummary.revision_number  # type: ignore[attr-defined]
-            )
-        else:
-            opportunity_summary_join_clause.append(OpportunitySummary.revision_number.is_(None))
+        opportunity_summary_join_clause.append(OpportunitySummary.revision_number.is_(None))
 
         return cast(
             list[

api/src/data_migration/transformation/subtask/transform_applicant_type.py

@@ -29,7 +29,6 @@ def transform_records(self) -> None:
                 == LinkOpportunitySummaryApplicantType.opportunity_summary_id,
             ],
             is_forecast=True,
-            is_historical_table=False,
             relationship_load_value=relationship_load_value,
         )
         self.process_link_applicant_types_group(forecast_applicant_type_records)
@@ -45,7 +44,6 @@ def transform_records(self) -> None:
                 == LinkOpportunitySummaryApplicantType.opportunity_summary_id,
             ],
             is_forecast=False,
-            is_historical_table=False,
             relationship_load_value=relationship_load_value,
         )
         self.process_link_applicant_types_group(synopsis_applicant_type_records)
@@ -96,16 +94,6 @@ def process_link_applicant_type(
                 extra,
             )
 
-        # Historical records are linked to other historical records, however
-        # we don't import historical opportunity records, so if the opportunity
-        # was deleted, we won't have created the opportunity summary. Whenever we do
-        # support historical opportunities, we'll have these all marked with a
-        # flag that we can use to reprocess these.
-        elif self._is_orphaned_historical(opportunity_summary, source_applicant_type):
-            self._handle_orphaned_historical(
-                source_applicant_type, transform_constants.APPLICANT_TYPE, extra
-            )
-
         elif opportunity_summary is None:
             # This shouldn't be possible as the incoming data has foreign keys, but as a safety net
             # we'll make sure the opportunity actually exists

api/src/data_migration/transformation/subtask/transform_funding_category.py

@@ -32,7 +32,6 @@ def transform_records(self) -> None:
                 == LinkOpportunitySummaryFundingCategory.opportunity_summary_id,
             ],
             is_forecast=True,
-            is_historical_table=False,
             relationship_load_value=relationship_load_value,
         )
         self.process_link_funding_categories_group(forecast_funding_category_records)
@@ -48,7 +47,6 @@ def transform_records(self) -> None:
                 == LinkOpportunitySummaryFundingCategory.opportunity_summary_id,
             ],
             is_forecast=False,
-            is_historical_table=False,
             relationship_load_value=relationship_load_value,
         )
         self.process_link_funding_categories_group(synopsis_funding_category_records)
@@ -99,16 +97,6 @@ def process_link_funding_category(
                 extra,
             )
 
-        # Historical records are linked to other historical records, however
-        # we don't import historical opportunity records, so if the opportunity
-        # was deleted, we won't have created the opportunity summary. Whenever we do
-        # support historical opportunities, we'll have these all marked with a
-        # flag that we can use to reprocess these.
-        elif self._is_orphaned_historical(opportunity_summary, source_funding_category):
-            self._handle_orphaned_historical(
-                source_funding_category, transform_constants.FUNDING_CATEGORY, extra
-            )
-
         elif opportunity_summary is None:
             # This shouldn't be possible as the incoming data has foreign keys, but as a safety net
             # we'll make sure the opportunity actually exists

api/src/data_migration/transformation/subtask/transform_funding_instrument.py

@@ -32,7 +32,6 @@ def transform_records(self) -> None:
                 == LinkOpportunitySummaryFundingInstrument.opportunity_summary_id,
             ],
             is_forecast=True,
-            is_historical_table=False,
             relationship_load_value=relationship_load_value,
         )
         self.process_link_funding_instruments_group(forecast_funding_instrument_records)
@@ -48,7 +47,6 @@ def transform_records(self) -> None:
                 == LinkOpportunitySummaryFundingInstrument.opportunity_summary_id,
             ],
             is_forecast=False,
-            is_historical_table=False,
             relationship_load_value=relationship_load_value,
         )
         self.process_link_funding_instruments_group(synopsis_funding_instrument_records)
@@ -101,16 +99,6 @@ def process_link_funding_instrument(
                 extra,
             )
 
-        # Historical records are linked to other historical records, however
-        # we don't import historical opportunity records, so if the opportunity
-        # was deleted, we won't have created the opportunity summary. Whenever we do
-        # support historical opportunities, we'll have these all marked with a
-        # flag that we can use to reprocess these.
-        elif self._is_orphaned_historical(opportunity_summary, source_funding_instrument):
-            self._handle_orphaned_historical(
-                source_funding_instrument, transform_constants.FUNDING_INSTRUMENT, extra
-            )
-
         elif opportunity_summary is None:
             # This shouldn't be possible as the incoming data has foreign keys, but as a safety net
             # we'll make sure the opportunity actually exists

api/src/data_migration/transformation/subtask/transform_opportunity_summary.py

@@ -77,16 +77,6 @@ def process_opportunity_summary(
                 source_summary, target_summary, transform_constants.OPPORTUNITY_SUMMARY, extra
             )
 
-        # Historical records are linked to other historical records, however
-        # we don't import historical opportunity records, so if the opportunity
-        # was deleted, we don't have anything to link these to. Whenever we do
-        # support historical opportunities, we'll have these all marked with a
-        # flag that we can use to reprocess these.
-        elif self._is_orphaned_historical(opportunity, source_summary):
-            self._handle_orphaned_historical(
-                source_summary, transform_constants.OPPORTUNITY_SUMMARY, extra
-            )
-
         elif opportunity is None:
             # This shouldn't be possible as the incoming data has foreign keys, but as a safety net
             # we'll make sure the opportunity actually exists

api/src/data_migration/transformation/transform_constants.py

@@ -6,7 +6,6 @@
     TapplicanttypesForecast,
     TapplicanttypesForecastHist,
     Tforecast,
-    TforecastHist,
     TfundactcatForecast,
     TfundactcatForecastHist,
     TfundinstrForecast,
@@ -21,7 +20,6 @@
     TfundinstrSynopsis,
     TfundinstrSynopsisHist,
     Tsynopsis,
-    TsynopsisHist,
 )
 
 ORPHANED_CFDA = "orphaned_cfda"
@@ -46,7 +44,6 @@ class Metrics(StrEnum):
     TOTAL_RECORDS_SKIPPED = "total_records_skipped"
     TOTAL_RECORDS_ORPHANED = "total_records_orphaned"
     TOTAL_DUPLICATE_RECORDS_SKIPPED = "total_duplicate_records_skipped"
-    TOTAL_HISTORICAL_ORPHANS_SKIPPED = "total_historical_orphans_skipped"
     TOTAL_DELETE_ORPHANS_SKIPPED = "total_delete_orphans_skipped"
 
     TOTAL_ERROR_COUNT = "total_error_count"
@@ -55,8 +52,7 @@ class Metrics(StrEnum):
 S = TypeVar("S", bound=StagingParamMixin)
 D = TypeVar("D", bound=ApiSchemaTable)
 
-
-SourceSummary: TypeAlias = Tforecast | Tsynopsis | TforecastHist | TsynopsisHist
+SourceSummary: TypeAlias = Tforecast | Tsynopsis
 
 SourceApplicantType: TypeAlias = (
     TapplicanttypesForecast
@@ -73,6 +69,4 @@ class Metrics(StrEnum):
     TfundinstrForecastHist | TfundinstrForecast | TfundinstrSynopsisHist | TfundinstrSynopsis
 )
 
-SourceAny: TypeAlias = (
-    SourceSummary | SourceApplicantType | SourceFundingCategory | SourceFundingInstrument
-)
+SourceAny: TypeAlias = SourceApplicantType | SourceFundingCategory | SourceFundingInstrument

api/src/data_migration/transformation/transform_util.py

@@ -209,9 +209,6 @@ def transform_opportunity_summary(
         target_summary = OpportunitySummary(
             opportunity_id=source_summary.opportunity_id,
             is_forecast=source_summary.is_forecast,
-            # Revision number is only found in the historical table, use getattr
-            # to avoid type checking
-            revision_number=getattr(source_summary, "revision_number", None),
         )
     else:
         # We create a new summary object and merge it outside this function
@@ -270,11 +267,6 @@ def transform_opportunity_summary(
     )
     target_summary.fiscal_year = getattr(source_summary, "fiscal_year", None)
 
-    # Set whether it is deleted based on action_type, which only appears on the historical records
-    target_summary.is_deleted = convert_action_type_to_is_deleted(
-        getattr(source_summary, "action_type", None)
-    )
-
     transform_update_create_timestamp(source_summary, target_summary, log_extra=log_extra)
 
     return target_summary
@@ -465,23 +457,6 @@ def convert_null_like_to_none(value: str | None) -> str | None:
     return value
 
 
-def convert_action_type_to_is_deleted(value: str | None) -> bool:
-    # Action type can be U (update) or D (delete)
-    # however many older records seem to not have this set at all
-    # The legacy system looks like it treats anything that isn't D
-    # the same, so we'll go with that assumption as well.
-    if is_empty_str(value):
-        return False
-
-    if value == "D":  # D = Delete
-        return True
-
-    if value == "U":  # U = Update
-        return False
-
-    raise ValueError("Unexpected action type value: %s" % value)
-
-
 def convert_numeric_str_to_int(value: str | None) -> int | None:
     if is_empty_str(value):
         return None
@@ -501,9 +476,6 @@ def get_log_extra_summary(source_summary: SourceSummary) -> dict:
     return {
         "opportunity_id": source_summary.opportunity_id,
         "is_forecast": source_summary.is_forecast,
-        # This value only exists on non-historical records
-        # use getattr instead of an isinstance if/else for simplicity
-        "revision_number": getattr(source_summary, "revision_number", None),
         "table_name": source_summary.__tablename__,
     }
 
@@ -513,7 +485,6 @@ def get_log_extra_applicant_type(source_applicant_type: SourceApplicantType) ->
         "opportunity_id": source_applicant_type.opportunity_id,
         "at_frcst_id": getattr(source_applicant_type, "at_frcst_id", None),
         "at_syn_id": getattr(source_applicant_type, "at_syn_id", None),
-        "revision_number": getattr(source_applicant_type, "revision_number", None),
         "table_name": source_applicant_type.__tablename__,
     }
 
@@ -523,7 +494,6 @@ def get_log_extra_funding_category(source_funding_category: SourceFundingCategor
         "opportunity_id": source_funding_category.opportunity_id,
         "fac_frcst_id": getattr(source_funding_category, "fac_frcst_id", None),
         "fac_syn_id": getattr(source_funding_category, "fac_syn_id", None),
-        "revision_number": getattr(source_funding_category, "revision_number", None),
         "table_name": source_funding_category.__tablename__,
     }
 
@@ -533,7 +503,6 @@ def get_log_extra_funding_instrument(source_funding_instrument: SourceFundingIns
         "opportunity_id": source_funding_instrument.opportunity_id,
         "fi_frcst_id": getattr(source_funding_instrument, "fi_frcst_id", None),
         "fi_syn_id": getattr(source_funding_instrument, "fi_syn_id", None),
-        "revision_number": getattr(source_funding_instrument, "revision_number", None),
         "table_name": source_funding_instrument.__tablename__,
     }
 
@@ -542,5 +511,4 @@ def get_log_extra_opportunity_attachment(source_attachment: TsynopsisAttachment)
     return {
         "opportunity_id": source_attachment.opportunity_id,
         "syn_att_id": source_attachment.syn_att_id,
-        "att_revision_number": source_attachment.att_revision_number,
     }

api/tests/src/data_migration/transformation/subtask/test_transform_funding_category.py

@@ -262,18 +262,3 @@ def test_process_funding_category_but_no_opportunity_summary_non_hist(
             match="Funding category record cannot be processed as the opportunity summary for it does not exist",
         ):
             transform_funding_category.process_link_funding_category(source_record, None, None)
-
-    @pytest.mark.parametrize(
-        "factory_cls",
-        [f.StagingTfundactcatForecastHistFactory, f.StagingTfundactcatSynopsisHistFactory],
-    )
-    def test_process_funding_category_but_no_opportunity_summary_hist(
-        self,
-        db_session,
-        transform_funding_category,
-        factory_cls,
-    ):
-        source_record = factory_cls.create(orphaned_record=True, revision_number=12)
-        transform_funding_category.process_link_funding_category(source_record, None, None)
-        assert source_record.transformed_at is not None
-        assert source_record.transformation_notes == "orphaned_historical_record"

api/tests/src/data_migration/transformation/subtask/test_transform_funding_instrument.py

@@ -192,18 +192,3 @@ def test_process_funding_instrument_but_no_opportunity_summary_non_hist(
             match="Funding instrument record cannot be processed as the opportunity summary for it does not exist",
         ):
             transform_funding_instrument.process_link_funding_instrument(source_record, None, None)
-
-    @pytest.mark.parametrize(
-        "factory_cls",
-        [f.StagingTfundinstrForecastHistFactory, f.StagingTfundinstrSynopsisHistFactory],
-    )
-    def test_process_funding_instrument_but_no_opportunity_summary_hist(
-        self,
-        db_session,
-        transform_funding_instrument,
-        factory_cls,
-    ):
-        source_record = factory_cls.create(orphaned_record=True, revision_number=12)
-        transform_funding_instrument.process_link_funding_instrument(source_record, None, None)
-        assert source_record.transformed_at is not None
-        assert source_record.transformation_notes == "orphaned_historical_record"

api/tests/src/data_migration/transformation/subtask/test_transform_opportunity_summary.py

@@ -191,25 +191,3 @@ def test_process_opportunity_summary_but_no_opportunity_non_hist(
             match="Opportunity summary cannot be processed as the opportunity for it does not exist",
         ):
             transform_opportunity_summary.process_opportunity_summary(source_record, None, None)
-
-    @pytest.mark.parametrize("is_forecast,revision_number", [(True, 10), (False, 9)])
-    def test_process_opportunity_summary_but_no_opportunity_hist(
-        self,
-        db_session,
-        transform_opportunity_summary,
-        is_forecast,
-        revision_number,
-    ):
-        source_record = setup_synopsis_forecast(
-            is_forecast=is_forecast,
-            revision_number=revision_number,
-            create_existing=False,
-            opportunity=None,
-            source_values={"opportunity_id": 12121212},
-        )
-
-        transform_opportunity_summary.process_opportunity_summary(source_record, None, None)
-
-        validate_opportunity_summary(db_session, source_record, expect_in_db=False)
-        assert source_record.transformed_at is not None
-        assert source_record.transformation_notes == "orphaned_historical_record"

api/tests/src/data_migration/transformation/test_transform_util.py

@@ -105,19 +105,6 @@ def test_convert_yn_boolean_unexpected_value(value):
         transform_util.convert_yn_bool(value)
 
 
-@pytest.mark.parametrize(
-    "value,expected_value", [("D", True), ("U", False), ("", False), (" ", False), (None, False)]
-)
-def test_convert_action_type_to_is_deleted(value, expected_value):
-    assert transform_util.convert_action_type_to_is_deleted(value) == expected_value
-
-
-@pytest.mark.parametrize("value", ["A", "B", "d", "u"])
-def test_convert_action_type_to_is_deleted_unexpected_value(value):
-    with pytest.raises(ValueError, match="Unexpected action type value"):
-        transform_util.convert_action_type_to_is_deleted(value)
-
-
 @pytest.mark.parametrize(
     "value,expected_value",
     [