UN-3494 [GATED-FEAT] Email group members on share and membership change - #2224
Draft
kirtimanmishrazipstack wants to merge 2 commits into
Draft
UN-3494 [GATED-FEAT] Email group members on share and membership change#2224kirtimanmishrazipstack wants to merge 2 commits into
kirtimanmishrazipstack wants to merge 2 commits into
Conversation
…ship changes Sharing a resource with a group gave its members access silently, and adding or removing someone from a group told nobody. Both now send email. - share_notifications.py holds the feature flag, the two task names and the two enqueue hooks. Dispatch uses the same resolve_transport branch the execution path uses: the PG queue where pg_queue_enabled is on for the org, Celery otherwise. - One hook in ResourceShareManagementMixin.share covers all 7 resource types plus cloud agentic, including service-account shares — every group share funnels through it and shared_groups has no PATCH path. No on_commit needed: _commit's transaction has closed by the time the view resumes, so the diff reads committed state. - Group membership hooks on the add and remove actions. The add serializer already subtracts existing members, so nobody is mailed twice. - Internal endpoints under /internal/v1/group-notification/ do the work the worker cannot: group expansion, OrganizationMember re-validation (this is where the offboarding race closes), resource lookup via ShareableResource, and the kind -> ResourceType mapping, which is not 1:1 — pipelines split on pipeline_type and adapters four ways on adapter_type. - Two worker tasks that only POST to that endpoint, since workers/ has no Django. They raise on failure, unlike _mark_buffer_outcome which has a reaper behind it, and retry transient 5xx in-task because a raise is terminal on the Celery transport. - The whole feature is gated on Flipt group_sharing_notifications_enabled and fails closed: a blind Flipt, a missing org, or any dispatch error means no notification, never a broken share. - worker-pg-notification compose service so the PG arm is not a black hole. Membership changes with no actor (the org-removal cascade, Django admin, group deletion) do not notify — SharingNotificationService requires an actor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
UN-2977 moved sharing from PATCH to POST /{id}/share/, but the mixin's
share action only diffed the groups axis. The per-viewset
_notify_shared_users hooks stayed on partial_update, which nothing calls
anymore, so sharing a resource with a user sent no email.
Snapshot every declared axis and invoke the hook after the commit; declare
it on the mixin as a no-op for hosts without a direct-share email.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
kirtimanmishrazipstack
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
/internal/v1/group-notification/plus twoworkers/notificationtasks that call them.POST /{id}/share/.Why
_notify_shared_userswas still wired topartial_update, which no client has called since UN-2977, so single-user share emails stopped going out entirely.How
ResourceShareManagementMixin.sharecovers all 7 shareable resources plus cloud agentic; dispatch reusesresolve_transport(PG queue wherepg_queue_enabledis on for the org, Celery otherwise).OrganizationMemberre-validation and the kind →ResourceTypemapping live in the internal views, sinceworkers/has no Django.group_sharing_notifications_enabled— a blind Flipt, a missing org or any dispatch error means no email, never a broken share.Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
Database Migrations
Env Config
group_sharing_notifications_enabled— new, gates the whole feature. Off or unreachable means no emails.pg_queue_enabled— existing, reused to pick PG queue vs Celery transport.worker-pg-notificationcompose service (pg-queueprofile), with optional overridesPG_NOTIFICATION_CONCURRENCY,PG_NOTIFICATION_VT_SECONDS,PG_NOTIFICATION_HEALTH_STALE_SECONDS.Relevant Docs
Related Issues or PRs
Dependencies Versions
Notes on Testing
Screenshots
Checklist
I have read and understood the Contribution Guidelines.