Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

crutan
Copy link
Contributor

@crutan crutan commented Oct 16, 2024

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

…etion so as to not try to remove org before the org is empty
@crutan crutan added the Bug label Oct 16, 2024
@crutan crutan requested a review from kflemin October 17, 2024 13:39
@haneslinger
Copy link
Contributor

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

@crutan
Copy link
Contributor Author

crutan commented Oct 22, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants