search

SEED-platform / seed

Standard Energy Efficiency Data (SEED) Platform™ is a web-based application that helps organizations easily manage data on the energy performance of large groups of buildings.
Other
111 stars 55 forks source link

Fix organization delete - ensure there is a chance to confirm and that it works in one step. #4851

Open crutan opened 1 month ago

crutan commented 1 month ago

What's this PR do?

Reworks the organization deletion job in celery to ensure the property/taxlot deletions happen before the organization deletion is attempted.

Adds a modal confirmation for organization deletion, migrates the inventory progress bar to that modal.

How should this be manually tested?

Create an org, load some inventory, then delete the organization.

What are the relevant tickets?

4814 #4836

Screenshots (if appropriate)

image

haneslinger commented 1 month ago

this is all so much cleaner. Can you explain where the bug was?

crutan commented 1 month ago

this is all so much cleaner. Can you explain where the bug was?

I can try. This is based on a lot of experimentation and reading of the celery docs - and I may well be misunderstanding them.

In short, I think the chain() method, while techincally working correctly, is not aware of the underlying tasks generated in the task it fires off - _delete_organization_inventory doesn't do any deletion itself, but instead schedules a slew of tasks to delete chunks of inventory. It appears that the chain only waits until that _delete_organization_inventory task finishes before movign on, and when it does there is usually still inventory in the process of being deleted so the Organization.get().delete() call fails.

So I effectively single-threaded it. This will mean that orgs with boatloads of properties/taxlots will take longer to delete - but I'm not entirely sure how core of a use-case that is. This is doing deletes in chunks of 100. We could certainly increase the chunk size there - postgres is fast and could handle a bigger set of IDs without any issue.