-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
ref(hc): Updates org deletion code to queue an org mapping outbox update #51004
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
ref(hc): Updates org deletion code to queue an org mapping outbox update #51004
Conversation
org: Organization = Organization.objects.get(id=org_id) | ||
if org.status != OrganizationStatus.ACTIVE: | ||
return None | ||
|
||
org.update(status=OrganizationStatus.PENDING_DELETION) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The semantics of a read, then update are slightly different than how the code behaved before where it was an update with condition on the status being active. We have a remote chance of emitting multiple outbox messages and scheduling deletions multiple times, however I don't think it will end up mattering as both those systems handle duplicates well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh because it's an update on the QuerySet before any accesses are made right? I can go ahead and replicate the previous behavior instead to make this atomic.
).update(status=OrganizationStatus.PENDING_DELETION) | ||
if updated: | ||
organization.status = OrganizationStatus.PENDING_DELETION | ||
updated_organization = mark_organization_as_pending_deletion_with_outbox_message( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should use these functions as the start of a domain layer for organization operations? Several operations for organizations are collecting more behavior that needs to be used as a unit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally think we should. I was very tempted to move the scheduling creation here as well since the only 2 references I found essentially tried to maintain their own idempotency so centralizing that here may be a good idea.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #51004 +/- ##
==========================================
+ Coverage 81.17% 81.22% +0.04%
==========================================
Files 4877 4886 +9
Lines 204602 204912 +310
Branches 11128 11128
==========================================
+ Hits 166081 166430 +349
+ Misses 38275 38236 -39
Partials 246 246
|
Adds a mark for deletion helper for organizations that queues an update outbox message. Updates the organization deletion API to use this new helper to ensure consistency between our org mappings and organizations