fpdcc / ccfp-asset-dashboard

CCFP Asset Dashboard
0 stars 1 forks source link

Fixes bug that crashed application upon deleting a phase with GIS assets #228

Closed smcalilly closed 1 year ago

smcalilly commented 1 year ago

Overview

Includes:

Notes

This was happening because of a Django signal's dependency on the Phase, LocalAsset, and PhaseZoneDistribution models. The model PhaseZoneDistribution is subscribed to a post_delete signal that runs after the Phase was deleted (since the phase deletes the LocalAssets, which is the signal the PhaseZoneDistribution is subscribed too — whew!). This caused this integrity error:

django.db.utils.IntegrityError: insert or update on table "asset_dashboard_phasezonedistribution" violates foreign key constraint "asset_dashboard_phas_phase_id_535d6e36_fk_asset_das"
DETAIL:  Key (phase_id)=(126) is not present in table "asset_dashboard_phase".

This only happened for a phase with GIS assets. It was happening deep in the Django ORM code (here's a gist with debugger logs if you're curious), so there wasn't a clean way to catch the exception and apply the logic we need.

A story

This fix should cover all of the logical requirements in this hypothetical story:

User creates a Phase. Then user adds some LocalAsset to the phase. Our backend logic should calculate the PhaseZoneDistributions based on the LocalAsset's geographical distribution. This distribution helps with scoring the project.

Then user realizes that they selected the wrong assets. So they delete the assets that are associated with the phase. Whenever the assets are deleted, our backend logic should recalculate the PhaseZoneDistributions (without the deleted asset's geographical area included in the calculation).

Then the user adds more assets and our logic should recalculate the distributions. Then they delete one single asset and our logic should recalculate the distributions. Etc.

Then, for some reason, user needs to delete the entire phase. Since the assets and distributions are associated with the phase, all of those instances should be deleted too.

on_delete=models.CASCADE

Shouldn't this work? This should be the way to clean up the PhaseZoneDistributions.

Yes, but it doesn't work for every case. It works for Phase.objects.delete(), but not when all of the LocalAssets are deleted separately from the phase deletion. Because of this, I'm forcefully deleting the zone distributions whenever all of the local assets are deleted. There's not a good way to set the relationship between LocalAsset and PhaseZoneDistribution in order to take advantage of the cascading deletion.

Testing Instructions