Feature/resource sharing v2 - #1436
Conversation
…Dict) sentry_sdk auto-loads its anthropic integration at settings import; the new anthropic SDK's generated types use TypedDict(extra_items=...), which typing-extensions 4.12.2 rejects, crashing every manage.py command. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds organization groups, resource grants, access requests, access defaults, centralized access evaluation, ownership authorization, and resource-level checks. New models, APIs, migrations, schemas, permissions, and invitation promotion logic support user, group, and resource access management. ChangesAccess controls
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ddpui/api/access_api.py (1)
1-110: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCI:
blackreformatted this file.Same
0_checks (3.10, 6)failure asddpui/core/access/resource_share.py— runblack ddpui/api/access_api.pylocally (this would also collapse the unnecessarily multi-lineadd_resource_grantssignature at lines 60-62) and commit the result.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ddpui/api/access_api.py` around lines 1 - 110, Run Black on the access API module and commit its formatting changes, including collapsing the unnecessarily split add_resource_grants signature while preserving all behavior.Source: Pipeline failures
ddpui/core/access/resource_share.py (1)
1-280: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCI:
blackreformatted this file.The
0_checks (3.10, 6)job failed because theblackpre-commit hook modified this file. Runblack ddpui/core/access/resource_share.pylocally and commit the result.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ddpui/core/access/resource_share.py` around lines 1 - 280, Run Black on the resource-share module, applying its formatting changes to the file containing list_grants, add_grants, and _resolve_pending_email_to_invitation_id, then commit the reformatted result.Source: Pipeline failures
🧹 Nitpick comments (2)
ddpui/schemas/access/resource_share_schema.py (1)
48-56: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse
EmailStrforPendingGrantPayload.email.Plain
straccepts any garbage input;_resolve_pending_email_to_invitation_idonly normalizes with.strip().lower()and doesn't validate format, so a malformed email can create a bogusInvitation/ResourceSharerow. Pydantic'sEmailStrcatches this at the schema boundary.♻️ Proposed fix
-from ninja import Schema +from ninja import Schema +from pydantic import EmailStr class PendingGrantPayload(Schema): """...""" - email: str + email: EmailStr access_level: AccessLevel🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ddpui/schemas/access/resource_share_schema.py` around lines 48 - 56, Update PendingGrantPayload.email from str to Pydantic’s EmailStr type, preserving the existing access_level field and payload behavior while enforcing email-format validation at schema validation.pyproject.toml (1)
261-261: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a bounded specifier over an unbounded
>=.Moving from an exact pin to
>=4.14.0trades reproducibility for an open-ended range.typing_extensions's own docs recommend depending on it as a compatible-release range rather than an unbounded floor.♻️ Proposed fix
- "typing-extensions>=4.14.0", + "typing-extensions~=4.14",Confirm which of your other dependencies actually require
>=4.14.0before deciding on the bound.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyproject.toml` at line 261, Update the typing-extensions dependency specifier in pyproject.toml to use a bounded compatible-release range instead of the unbounded >=4.14.0 constraint. First verify which dependencies require version 4.14.0, then choose an upper bound that preserves those requirements while keeping dependency resolution reproducible.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ddpui/api/access_api.py`:
- Around line 7-19: Remove the unused has_permission import and update the add,
update, and delete grant endpoints in the access router to enforce their
corresponding can_share_* permissions before invoking resource_share operations.
Preserve owner/admin access while rejecting org users who lack the relevant
sharing permission, using the existing is_creator_or_admin and router endpoint
symbols.
In `@ddpui/api/user_org_api.py`:
- Around line 615-634: Move invitation email dispatch out of the
transaction.atomic flow used by add_user_group_members, coordinating with the
shared invitation logic in create_user_group. Ensure database changes complete
successfully before sending invitations, using the existing
transaction-completion mechanism where appropriate, while preserving member and
invitation creation behavior.
- Around line 293-305: Guard the nullable new_role access in the active-user
branch of the OrgUser iteration, matching the existing pending-invitation
handling below: return a safe null/default value for role_slug and role_name
when curr_orguser.new_role is absent, while preserving the current values when a
role exists.
- Around line 524-550: Move invitation email delivery out of the
transaction.atomic scope in create_user_group, coordinating with the shared
_resolve_pending_emails and _add_members_to_group flow so emails are sent only
after the database transaction commits. Apply the same change to the
corresponding add_user_group_members path, preserving group creation and
membership updates within the transaction.
- Around line 460-472: Update the callers create_user_group and
add_user_group_members to retain the newly created active OrgUser IDs returned
by invite_user_v1, merge them with the existing orguser_ids, and pass the
combined IDs to _add_members_to_group. Ensure existing-user invitations, which
do not create Invitation rows, are still linked to the group and represented in
the returned email-to-invitation mapping as appropriate.
In `@ddpui/core/orguserfunctions.py`:
- Around line 317-359: The invitation acceptance flow, including
accept_invitation_v1 and its caller post_organization_user_accept_invite_v1,
must execute atomically. Add the established transaction.atomic protection
around the full acceptance operation so failures roll back group membership and
resource-share promotions along with the invitation deletion.
In `@ddpui/models/org_user.py`:
- Around line 255-273: Add a database-level UniqueConstraint for the (group,
orguser) pair in OrgUserGroupMember.Meta.constraints, alongside the existing
invitation constraint. Preserve the current invitation uniqueness constraint and
use a distinct constraint name for the orguser pairing.
In `@ddpui/models/resource_share.py`:
- Around line 63-70: Update the ResourceShare model’s Meta constraints to add
database-level uniqueness for the active principal/resource grant identity used
by add_grants, covering org, resource_type, resource_id, principal_type, and
principal_id. Preserve the existing lookup indexes and align the constraint with
the app’s idempotency semantics so concurrent creates cannot produce duplicate
active rows.
---
Outside diff comments:
In `@ddpui/api/access_api.py`:
- Around line 1-110: Run Black on the access API module and commit its
formatting changes, including collapsing the unnecessarily split
add_resource_grants signature while preserving all behavior.
In `@ddpui/core/access/resource_share.py`:
- Around line 1-280: Run Black on the resource-share module, applying its
formatting changes to the file containing list_grants, add_grants, and
_resolve_pending_email_to_invitation_id, then commit the reformatted result.
---
Nitpick comments:
In `@ddpui/schemas/access/resource_share_schema.py`:
- Around line 48-56: Update PendingGrantPayload.email from str to Pydantic’s
EmailStr type, preserving the existing access_level field and payload behavior
while enforcing email-format validation at schema validation.
In `@pyproject.toml`:
- Line 261: Update the typing-extensions dependency specifier in pyproject.toml
to use a bounded compatible-release range instead of the unbounded >=4.14.0
constraint. First verify which dependencies require version 4.14.0, then choose
an upper bound that preserves those requirements while keeping dependency
resolution reproducible.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c1762a0-e4c7-46f9-ae5e-a65e0c5ba872
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
ddpui/api/access_api.pyddpui/api/dashboard_native_api.pyddpui/api/org_preferences_api.pyddpui/api/user_org_api.pyddpui/core/access/__init__.pyddpui/core/access/ownership.pyddpui/core/access/resource_share.pyddpui/core/access/shareable_types.pyddpui/core/alerts/alert_service.pyddpui/core/kpi/kpi_service.pyddpui/core/metric/metric_service.pyddpui/core/orguserfunctions.pyddpui/core/reports/report_service.pyddpui/migrations/0170_orgusergroup_orgpreferences_allow_public_sharing_and_more.pyddpui/models/__init__.pyddpui/models/org_preferences.pyddpui/models/org_user.pyddpui/models/resource_share.pyddpui/routes.pyddpui/schemas/access/__init__.pyddpui/schemas/access/resource_share_schema.pyddpui/schemas/org_preferences_schema.pyddpui/services/chart_service.pyddpui/services/dashboard_service.pyddpui/tests/core/test_ownership.pypyproject.tomlseed/002_permissions.jsonseed/003_role_permissions.json
| for curr_orguser in OrgUser.objects.filter(org=org).select_related("user", "new_role"): | ||
| rows.append( | ||
| PeopleRow( | ||
| email=curr_orguser.user.email, | ||
| role_slug=curr_orguser.new_role.slug, | ||
| role_name=curr_orguser.new_role.name, | ||
| status="active", | ||
| created_by_email=None, | ||
| orguser_id=curr_orguser.id, | ||
| invitation_id=None, | ||
| created_at=curr_orguser.user.date_joined, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unguarded new_role access can crash the endpoint.
OrgUser.new_role is nullable (on_delete=models.SET_NULL, null=True), so curr_orguser.new_role.slug/.name will raise AttributeError for any active org user whose role was deleted. Note the pending-invitation branch just below (Lines 313-314) already guards this the same way — the active branch should be consistent.
🐛 Proposed fix
PeopleRow(
email=curr_orguser.user.email,
- role_slug=curr_orguser.new_role.slug,
- role_name=curr_orguser.new_role.name,
+ role_slug=curr_orguser.new_role.slug if curr_orguser.new_role else "",
+ role_name=curr_orguser.new_role.name if curr_orguser.new_role else "",
status="active",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for curr_orguser in OrgUser.objects.filter(org=org).select_related("user", "new_role"): | |
| rows.append( | |
| PeopleRow( | |
| email=curr_orguser.user.email, | |
| role_slug=curr_orguser.new_role.slug, | |
| role_name=curr_orguser.new_role.name, | |
| status="active", | |
| created_by_email=None, | |
| orguser_id=curr_orguser.id, | |
| invitation_id=None, | |
| created_at=curr_orguser.user.date_joined, | |
| ) | |
| ) | |
| for curr_orguser in OrgUser.objects.filter(org=org).select_related("user", "new_role"): | |
| rows.append( | |
| PeopleRow( | |
| email=curr_orguser.user.email, | |
| role_slug=curr_orguser.new_role.slug if curr_orguser.new_role else "", | |
| role_name=curr_orguser.new_role.name if curr_orguser.new_role else "", | |
| status="active", | |
| created_by_email=None, | |
| orguser_id=curr_orguser.id, | |
| invitation_id=None, | |
| created_at=curr_orguser.user.date_joined, | |
| ) | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ddpui/api/user_org_api.py` around lines 293 - 305, Guard the nullable
new_role access in the active-user branch of the OrgUser iteration, matching the
existing pending-invitation handling below: return a safe null/default value for
role_slug and role_name when curr_orguser.new_role is absent, while preserving
the current values when a role exists.
| for email in to_invite: | ||
| payload = NewInvitationSchema( | ||
| invited_email=email, | ||
| invited_role_uuid=invite_role_uuid, | ||
| ) | ||
| _, error = orguserfunctions.invite_user_v1(orguser, payload) | ||
| if error: | ||
| raise HttpError(400, f"failed to invite {email}: {error}") | ||
| invitation = Invitation.objects.filter(invited_by__org=org, invited_email=email).first() | ||
| if invitation is not None: | ||
| existing_invites[email] = invitation.id | ||
|
|
||
| return existing_invites |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Newly-invited emails belonging to existing platform users silently never join the group.
When invite_user_v1 (ddpui/core/orguserfunctions.py:207-278) is called for an email that already has a Django User account, it creates an OrgUser directly and returns without creating an Invitation row. Back here, the subsequent Invitation.objects.filter(...).first() lookup then returns None, so existing_invites[email] is never populated for that email — the address is silently dropped from the returned email_to_invitation_id dict, and the freshly-created OrgUser is never linked via _add_members_to_group. The caller gets a 200 response with no indication that the member wasn't added.
🐛 Proposed fix sketch
if to_invite:
...
+ newly_active_orguser_ids: list[int] = []
for email in to_invite:
payload = NewInvitationSchema(
invited_email=email,
invited_role_uuid=invite_role_uuid,
)
_, error = orguserfunctions.invite_user_v1(orguser, payload)
if error:
raise HttpError(400, f"failed to invite {email}: {error}")
invitation = Invitation.objects.filter(invited_by__org=org, invited_email=email).first()
if invitation is not None:
existing_invites[email] = invitation.id
+ else:
+ ou = OrgUser.objects.filter(org=org, user__email__iexact=email).first()
+ if ou is not None:
+ newly_active_orguser_ids.append(ou.id)
- return existing_invites
+ return existing_invites, newly_active_orguser_idsCallers (create_user_group, add_user_group_members) would need to merge newly_active_orguser_ids into the orguser_ids passed to _add_members_to_group.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ddpui/api/user_org_api.py` around lines 460 - 472, Update the callers
create_user_group and add_user_group_members to retain the newly created active
OrgUser IDs returned by invite_user_v1, merge them with the existing
orguser_ids, and pass the combined IDs to _add_members_to_group. Ensure
existing-user invitations, which do not create Invitation rows, are still linked
to the group and represented in the returned email-to-invitation mapping as
appropriate.
| @user_org_router.post( | ||
| "/v1/organizations/user_groups", | ||
| response=GroupDetailSchema, | ||
| ) | ||
| @has_permission(["can_create_user_group"]) | ||
| @transaction.atomic | ||
| def create_user_group(request, payload: CreateGroupPayload): | ||
| orguser: OrgUser = request.orguser | ||
| if orguser.org is None: | ||
| raise HttpError(400, "no associated org") | ||
|
|
||
| name = payload.name.strip() | ||
| if not name: | ||
| raise HttpError(400, "group name is required") | ||
|
|
||
| if OrgUserGroup.objects.filter(org=orguser.org, name__iexact=name).exists(): | ||
| raise HttpError(400, "a group with this name already exists") | ||
|
|
||
| email_to_invitation_id = _resolve_pending_emails( | ||
| orguser.org, orguser, payload.pending_emails, payload.invite_role_uuid | ||
| ) | ||
|
|
||
| group = OrgUserGroup.objects.create(org=orguser.org, name=name, created_by=orguser) | ||
| _add_members_to_group(group, payload.orguser_ids, email_to_invitation_id) | ||
|
|
||
| return _serialize_group_detail(group) | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Invitation email sent inside transaction.atomic — root cause shared with add_user_group_members.
See consolidated comment below for details and the second affected site.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ddpui/api/user_org_api.py` around lines 524 - 550, Move invitation email
delivery out of the transaction.atomic scope in create_user_group, coordinating
with the shared _resolve_pending_emails and _add_members_to_group flow so emails
are sent only after the database transaction commits. Apply the same change to
the corresponding add_user_group_members path, preserving group creation and
membership updates within the transaction.
| @user_org_router.post( | ||
| "/v1/organizations/user_groups/{group_id}/members", | ||
| response=GroupDetailSchema, | ||
| ) | ||
| @has_permission(["can_edit_user_group"]) | ||
| @transaction.atomic | ||
| def add_user_group_members(request, group_id: int, payload: AddMembersPayload): | ||
| orguser: OrgUser = request.orguser | ||
| if orguser.org is None: | ||
| raise HttpError(400, "no associated org") | ||
| group = OrgUserGroup.objects.filter(id=group_id, org=orguser.org).first() | ||
| if group is None: | ||
| raise HttpError(404, "group not found") | ||
|
|
||
| email_to_invitation_id = _resolve_pending_emails( | ||
| orguser.org, orguser, payload.pending_emails, payload.invite_role_uuid | ||
| ) | ||
| _add_members_to_group(group, payload.orguser_ids, email_to_invitation_id) | ||
| return _serialize_group_detail(group) | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Invitation email sent inside transaction.atomic — root cause shared with create_user_group.
See consolidated comment below for details and the second affected site.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ddpui/api/user_org_api.py` around lines 615 - 634, Move invitation email
dispatch out of the transaction.atomic flow used by add_user_group_members,
coordinating with the shared invitation logic in create_user_group. Ensure
database changes complete successfully before sending invitations, using the
existing transaction-completion mechanism where appropriate, while preserving
member and invitation creation behavior.
|
|
||
| # Preserve any group memberships that were pinned to this invitation: | ||
| # promote each member row to point at the accepting orguser instead. If | ||
| # the orguser is already a direct member of that group, drop the | ||
| # invitation-linked row to avoid duplicates. After this, invitation.delete() | ||
| # nulls out any remaining invitation_id via SET_NULL. | ||
| from ddpui.models.org_user import OrgUserGroupMember # local import to avoid cycles | ||
| from ddpui.models.resource_share import ResourceShare, ResourceSharePrincipalType | ||
|
|
||
| invitation_member_rows = OrgUserGroupMember.objects.filter(invitation=invitation) | ||
| existing_group_ids = set( | ||
| OrgUserGroupMember.objects.filter(orguser=orguser).values_list("group_id", flat=True) | ||
| ) | ||
| for member_row in invitation_member_rows: | ||
| if member_row.group_id in existing_group_ids: | ||
| member_row.delete() | ||
| else: | ||
| member_row.orguser = orguser | ||
| member_row.save(update_fields=["orguser", "updated_at"]) | ||
| existing_group_ids.add(member_row.group_id) | ||
|
|
||
| # Promote any pending resource shares in the same way: point them at the | ||
| # accepting orguser as a direct user grant. If the orguser already has a | ||
| # direct share on the same resource, drop the invitation-linked row. | ||
| invitation_share_rows = ResourceShare.objects.filter(invitation=invitation) | ||
| existing_direct_keys = set( | ||
| ResourceShare.objects.filter( | ||
| org=invitation.invited_by.org, | ||
| principal_type=ResourceSharePrincipalType.USER, | ||
| principal_id=orguser.id, | ||
| ).values_list("resource_type", "resource_id") | ||
| ) | ||
| for share_row in invitation_share_rows: | ||
| key = (share_row.resource_type, share_row.resource_id) | ||
| if key in existing_direct_keys: | ||
| share_row.delete() | ||
| else: | ||
| share_row.principal_type = ResourceSharePrincipalType.USER | ||
| share_row.principal_id = orguser.id | ||
| share_row.save(update_fields=["principal_type", "principal_id"]) | ||
| existing_direct_keys.add(key) | ||
|
|
||
| invitation.delete() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -B5 'def accept_invitation_v1' ddpui/core/orguserfunctions.py
rg -n -B3 -A3 'accept_invitation_v1' ddpui/api/user_org_api.pyRepository: DalgoT4D/DDP_backend
Length of output: 640
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== orguserfunctions imports and accept_invitation_v1 wrapper =="
sed -n '1,80p;240,365p' ddpui/core/orguserfunctions.py
echo
echo "== user_org_api accept endpoint relevant block =="
sed -n '820,860p' ddpui/api/user_org_api.py
echo
echo "== search for atomic decorators/calls around accept_invitation_v1 =="
rg -n 'transaction\.atomic|`@transaction`|accept_invitation_v1|accept_invitation' ddpui -g '*.py'Repository: DalgoT4D/DDP_backend
Length of output: 11480
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== imports and accept_invitation_v1 wrapper =="
sed -n '1,80p' ddpui/core/orguserfunctions.py
sed -n '240,365p' ddpui/core/orguserfunctions.py
echo
echo "== user_org_api accept endpoint relevant block =="
sed -n '820,860p' ddpui/api/user_org_api.py
echo
echo "== atomic/accept_invitation references (concise) =="
rg -n 'transaction\.atomic|`@transaction`|accept_invitation_v1|accept_invitation' ddpui -g '*.py'Repository: DalgoT4D/DDP_backend
Length of output: 11446
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== user_org_api relevant surrounding lines around nearby atoms and accept route =="
sed -n '505,545p' ddpui/api/user_org_api.py
sed -n '608,630p' ddpui/api/user_org_api.py
sed -n '836,860p' ddpui/api/user_org_api.py
echo
echo "== line-numbered context for user_org_api imports/router setup =="
sed -n '1,45p' ddpui/api/user_org_api.py
rg -n -C2 'user_org_router\s*=|`@router`|database_router|connection' ddpui/api/user_org_api.pyRepository: DalgoT4D/DDP_backend
Length of output: 4752
Wrap the invitation-accept path in a transaction.
post_organization_user_accept_invite_v1 calls accept_invitation_v1 without @transaction.atomic, and accept_invitation_v1 performs several DB writes before invitation.delete(). If any step fails partway through, group memberships or resource shares can be promoted while others remain pinned to the still-existing invitation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ddpui/core/orguserfunctions.py` around lines 317 - 359, The invitation
acceptance flow, including accept_invitation_v1 and its caller
post_organization_user_accept_invite_v1, must execute atomically. Add the
established transaction.atomic protection around the full acceptance operation
so failures roll back group membership and resource-share promotions along with
the invitation deletion.
| class OrgUserGroupMember(models.Model): | ||
| """One membership row: a group has this ``OrgUser`` (active) or this | ||
| ``invitation`` (invited, not yet a user) as a member. When the invitation | ||
| is accepted, ``orguser`` is set and ``invitation`` becomes NULL via | ||
| ``on_delete=SET_NULL``.""" | ||
|
|
||
| group = models.ForeignKey(OrgUserGroup, on_delete=models.CASCADE, related_name="members") | ||
| orguser = models.ForeignKey(OrgUser, on_delete=models.CASCADE, null=True) | ||
| invitation = models.ForeignKey(Invitation, on_delete=models.SET_NULL, null=True) | ||
| created_at = models.DateTimeField(auto_created=True, default=timezone.now) | ||
| updated_at = models.DateTimeField(auto_now=True) | ||
|
|
||
| class Meta: | ||
| db_table = "orguser_group_member" | ||
| constraints = [ | ||
| models.UniqueConstraint( | ||
| fields=["group", "invitation"], name="uq_orguser_group_member_invitation" | ||
| ) | ||
| ] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Asymmetric uniqueness: (group, invitation) is constrained, (group, orguser) is not.
OrgUserGroupMember guards duplicate invitation-based membership at the DB level but not duplicate orguser-based membership, even though _add_members_to_group uses the same check-then-create idempotency pattern for both paths. See consolidated comment (shared root cause with ResourceShare in ddpui/models/resource_share.py).
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 269-273: Mutable default value for class attribute
(RUF012)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ddpui/models/org_user.py` around lines 255 - 273, Add a database-level
UniqueConstraint for the (group, orguser) pair in
OrgUserGroupMember.Meta.constraints, alongside the existing invitation
constraint. Preserve the current invitation uniqueness constraint and use a
distinct constraint name for the orguser pairing.
…rs + gating
Grants and org-default floors are now enforced on dashboard endpoints
(previously grants could be created/displayed but nothing consumed them).
- core/access/access_resolver.py: the single per-resource access decision
(effective_level / effective_levels / accessible_q). Precedence:
creator/admin -> explicit user grant -> group grants (max) -> org floor.
- auth.py: with_resource + require_level decorators (next to has_permission).
- dashboard endpoints: edit-path gates move to the area slug can_view_dashboards
+ require_level("edit"), so a granted/floored member can edit; list filters
via accessible_q; detail/list responses carry my_access.
Dashboard-only this pass (charts/reports follow). No-op for orgs on the
default view floors; behavior changes only when a floor is lowered or a
grant is added.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Purely mechanical rename for readability (no behavior change): - module access_resolver.py -> resource_access.py - effective_level -> get_user_access - effective_levels -> get_user_access_map - accessible_q -> accessible_filter - helpers: _floor_level -> _org_floor, _grant_levels -> _grants_map, _visible -> _hide_no_access Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…cement Resource sharing enforcement (dashboards)
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
ddpui/api/user_org_api.py (4)
1340-1341: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard nullable
orguser.orgbefore readinglogo_url.
OrgUser.orgis nullable. Both newhad_logoexpressions execute before thetryblock. A user without an organization therefore receives anAttributeErrorinstead of a client error.Proposed fix
orguser: OrgUser = request.orguser + if orguser.org is None: + raise HttpError(400, "create an organization first") had_logo = bool(orguser.org.logo_url)Apply the same guard in
upload_logo_from_url.Also applies to: 1378-1379
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ddpui/api/user_org_api.py` around lines 1340 - 1341, Guard nullable orguser.org before accessing logo_url in both had_logo expressions, including upload_logo_from_url. Ensure users without an organization receive the existing client-error response rather than an AttributeError, and apply the guard before either expression executes outside the try block.
872-876: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not assign organization-scoped audit events to
.first()membership.
OrgUser.userallows multiple organization memberships. Neither endpoint provides an organization selector..first()can therefore store a login or password-reset event under the wrong organization and omit it from the other organizations. Emit a user-level event, emit one event per membership, or use an explicitly selected organization.Also applies to: 1235-1239
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ddpui/api/user_org_api.py` around lines 872 - 876, Update the audit-log handling around the user lookup and OrgUser membership resolution in both affected endpoints so it does not use OrgUser.objects.filter(user=user).first() to choose an organization. Emit a user-level event, create an event for every matching OrgUser membership, or require and honor an explicit organization selection while preserving the existing event details.
1077-1083: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winEnforce organization scoping for invitation operations.
ddpui/api/user_org_api.py#L1077-L1083: Filter withinvited_by__org=orguser.org. Return 404 when no scoped invitation exists.ddpui/api/user_org_api.py#L1047-L1049andddpui/core/orguserfunctions.py#L407-L425: Pass the current organization toresend_invitationand include it in the lookup. The current ID-only lookup can resend another organization’s invitation and send its invite URL.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ddpui/api/user_org_api.py` around lines 1077 - 1083, Scope invitation operations to the current organization: update the invitation lookup near the deletion flow to filter by both ID and invited_by__org=orguser.org, returning 404 when no scoped invitation exists. Update the resend invitation call near the existing endpoint and the orguserfunctions.py resend_invitation implementation to accept the current organization and include it in its lookup, preventing cross-organization invite URLs from being sent.
1125-1137: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReturn and use the new organization membership as the audit actor.
ensure_orguser_for_orgcreates a newOrgUserbut returns(None, None). The audit call stores the new organization ID with the requestor’s old membership ID. Return the created or updatedOrgUserand pass it tocreate_audit_log.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ddpui/api/user_org_api.py` around lines 1125 - 1137, Update ensure_orguser_for_org to return the created or updated OrgUser instead of (None, None), then capture that membership in the organization-creation flow and pass it as orguser to create_audit_log. Preserve the existing organization creation behavior while ensuring the audit actor is the new organization membership.
🧹 Nitpick comments (1)
ddpui/api/user_org_api.py (1)
828-840: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the original
ValueErrorcontext.This handler raises
HttpErrorwithout chaining the original exception. Addfrom errorto preserve the traceback and satisfy Ruff B904.Proposed fix
- raise HttpError(400, str(error)) + raise HttpError(400, str(error)) from error🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ddpui/api/user_org_api.py` around lines 828 - 840, Update the ValueError handler around create_warehouse and create_audit_log to raise HttpError with explicit exception chaining using the caught error, preserving the original traceback while keeping the existing 400 status and message.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@ddpui/api/user_org_api.py`:
- Around line 1340-1341: Guard nullable orguser.org before accessing logo_url in
both had_logo expressions, including upload_logo_from_url. Ensure users without
an organization receive the existing client-error response rather than an
AttributeError, and apply the guard before either expression executes outside
the try block.
- Around line 872-876: Update the audit-log handling around the user lookup and
OrgUser membership resolution in both affected endpoints so it does not use
OrgUser.objects.filter(user=user).first() to choose an organization. Emit a
user-level event, create an event for every matching OrgUser membership, or
require and honor an explicit organization selection while preserving the
existing event details.
- Around line 1077-1083: Scope invitation operations to the current
organization: update the invitation lookup near the deletion flow to filter by
both ID and invited_by__org=orguser.org, returning 404 when no scoped invitation
exists. Update the resend invitation call near the existing endpoint and the
orguserfunctions.py resend_invitation implementation to accept the current
organization and include it in its lookup, preventing cross-organization invite
URLs from being sent.
- Around line 1125-1137: Update ensure_orguser_for_org to return the created or
updated OrgUser instead of (None, None), then capture that membership in the
organization-creation flow and pass it as orguser to create_audit_log. Preserve
the existing organization creation behavior while ensuring the audit actor is
the new organization membership.
---
Nitpick comments:
In `@ddpui/api/user_org_api.py`:
- Around line 828-840: Update the ValueError handler around create_warehouse and
create_audit_log to raise HttpError with explicit exception chaining using the
caught error, preserving the original traceback while keeping the existing 400
status and message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 58fd57e9-5fa9-4803-b47a-ec8f3c7a28d5
📒 Files selected for processing (9)
ddpui/api/dashboard_native_api.pyddpui/api/user_org_api.pyddpui/core/alerts/alert_service.pyddpui/core/kpi/kpi_service.pyddpui/core/metric/metric_service.pyddpui/core/orguserfunctions.pyddpui/core/reports/report_service.pyddpui/services/chart_service.pyddpui/services/dashboard_service.py
🚧 Files skipped from review as they are similar to previous changes (8)
- ddpui/core/reports/report_service.py
- ddpui/core/alerts/alert_service.py
- ddpui/core/kpi/kpi_service.py
- ddpui/services/chart_service.py
- ddpui/core/metric/metric_service.py
- ddpui/services/dashboard_service.py
- ddpui/core/orguserfunctions.py
- ddpui/api/dashboard_native_api.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ddpui/models/metric.py`:
- Line 120: Register KPI in ResourceType and RTYPES, then update the KPI list,
detail, and data endpoint authorization flows to use access_control with KPI’s
is_private field in addition to can_view_kpis and organization filtering. Ensure
private KPIs are only accessible to authorized users while preserving existing
public KPI behavior.
In `@ddpui/models/resource_share.py`:
- Around line 97-109: Add a conditional UniqueConstraint to the ResourceShare
model’s constraints for org, resource_type, resource_id, and invitation when
invitation is non-null, preventing duplicate invitation-backed grants; add the
matching constraint operation in ddpui/migrations/0175_resource_sharing.py lines
109-116.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 46c29946-b705-449a-a293-ccfaab031bb5
📒 Files selected for processing (8)
ddpui/migrations/0175_resource_sharing.pyddpui/migrations/0176_access_request.pyddpui/models/dashboard.pyddpui/models/metric.pyddpui/models/org_preferences.pyddpui/models/report.pyddpui/models/resource_share.pyddpui/models/visualization.py
| constraints = [ | ||
| # prevent duplicate direct grants for the same principal on the same resource | ||
| models.UniqueConstraint( | ||
| fields=["org", "resource_type", "resource_id", "principal_type", "principal_id"], | ||
| condition=models.Q(parent__isnull=True), | ||
| name="uq_resource_share_direct_grant", | ||
| ), | ||
| # prevent duplicate cascade rows for the same principal+resource from the same parent share | ||
| models.UniqueConstraint( | ||
| fields=["org", "resource_type", "resource_id", "principal_type", "principal_id", "parent"], | ||
| condition=models.Q(parent__isnull=False), | ||
| name="uq_resource_share_cascade_grant", | ||
| ), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Add uniqueness for invitation-backed grants.
add_grants identifies pending grants by (org, resource_type, resource_id, invitation_id). The current constraints only use nullable principal columns. Concurrent pending-email requests can create duplicate ResourceShare rows because NULL values do not collide in a unique constraint.
ddpui/models/resource_share.py#L97-L109: add a conditionalUniqueConstraintfororg,resource_type,resource_id, andinvitationwheninvitation__isnull=False.ddpui/migrations/0175_resource_sharing.py#L109-L116: add the matching migration operation.
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 97-110: Mutable default value for class attribute
(RUF012)
📍 Affects 2 files
ddpui/models/resource_share.py#L97-L109(this comment)ddpui/migrations/0175_resource_sharing.py#L109-L116
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ddpui/models/resource_share.py` around lines 97 - 109, Add a conditional
UniqueConstraint to the ResourceShare model’s constraints for org,
resource_type, resource_id, and invitation when invitation is non-null,
preventing duplicate invitation-backed grants; add the matching constraint
operation in ddpui/migrations/0175_resource_sharing.py lines 109-116.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
ddpui/core/access/resource_share.py (1)
330-348: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winGrant mutation endpoints authorize one resource and then mutate a share on any resource. The root cause is in the service layer:
update_grantandremove_grantfilterResourceSharebyidandorgonly. The API layer authorizes the caller against the{rtype}/{resource_id}path but never passes those values down, so a caller with Edit access on resource A can change or revoke a grant belonging to resource B in the same org.
ddpui/core/access/resource_share.py#L330-L348: addrtypeandresource_idparameters toupdate_grantand includeresource_type=rtype, resource_id=str(resource_id)in the lookup filter; apply the same change toremove_grantat lines 351-356.ddpui/api/access_api.py#L93-L103: passrtypeandresource_idintoresource_share.update_grantso the share lookup is scoped to the authorized resource.ddpui/api/access_api.py#L110-L118: passrtypeandresource_idintoresource_share.remove_grantso the share lookup is scoped to the authorized resource.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ddpui/core/access/resource_share.py` around lines 330 - 348, Scope grant mutations to the authorized resource: in ddpui/core/access/resource_share.py lines 330-348, add rtype and resource_id to update_grant and filter by resource_type and stringified resource_id; apply the same lookup scoping to remove_grant at lines 351-356. In ddpui/api/access_api.py lines 93-103 and 110-118, pass rtype and resource_id into update_grant and remove_grant respectively.ddpui/api/user_org_api.py (4)
351-363: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not mark deactivated users as active.
The People query includes every
OrgUser, but every returned row usesstatus="active". A user withUser.is_active=Falseis therefore reported as active. Filter byuser__is_active=True, or add an explicit inactive status toPeopleRow. (raw.githubusercontent.com)Proposed fix
-OrgUser.objects.filter(org=org) +OrgUser.objects.filter(org=org, user__is_active=True)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ddpui/api/user_org_api.py` around lines 351 - 363, Update the OrgUser queryset in the People-row construction to exclude deactivated users by filtering on user__is_active=True before iterating, so the existing status="active" assignment remains accurate.Source: MCP tools
351-363: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the organization-membership timestamp.
PeopleRow.created_atusescurr_orguser.user.date_joined, while pending rows use the invitation timestamp. A user added to another organization will show the platform account creation time instead of the organization membership time. Usecurr_orguser.created_atif this field represents when the person joined the organization. (raw.githubusercontent.com)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ddpui/api/user_org_api.py` around lines 351 - 363, Update the PeopleRow construction in the organization-user loop to set created_at from curr_orguser.created_at, preserving the organization membership timestamp rather than the platform user's date_joined.Source: MCP tools
582-607: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEnforce case-insensitive group-name uniqueness at the database boundary.
OrgUserGrouphas no database constraint, while both endpoints rely onexists()checks. Concurrent create or rename requests can create duplicate names within one organization. Add a PostgreSQL constraint onorgandLower("name"), add its migration, and map constraint conflicts to HTTP 400 in both endpoints.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ddpui/api/user_org_api.py` around lines 582 - 607, Add a PostgreSQL unique constraint to OrgUserGroup on org and Lower("name"), and create the corresponding migration. Update create_user_group and the group-rename endpoint to catch the resulting IntegrityError and return HTTP 400 using the existing duplicate-name error response, while retaining their current validation checks.
533-561: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftEnforce active membership uniqueness at the database boundary.
OrgUserGroupMemberconstrains only(group, invitation). Active rows useinvitation=NULL, so concurrent calls can create duplicate(group, orguser)rows. Add a conditional databaseUniqueConstraintfor(group, orguser)whereorguser IS NOT NULL, and use conflict-safe inserts for both membership types.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ddpui/api/user_org_api.py` around lines 533 - 561, Add a conditional UniqueConstraint on OrgUserGroupMember for (group, orguser) when orguser is not null, alongside the existing invitation constraint. Update _add_members_to_group to use conflict-safe inserts for both orguser and invitation memberships, preserving idempotent behavior under concurrent calls.
🧹 Nitpick comments (3)
ddpui/models/resource_share.py (1)
31-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the ambiguous loop variable
l.Ruff reports E741 on both lines.
ddpui/core/access/resource_share.pyalready useslvlfor the same concept. Uselvlhere for consistency.♻️ Proposed rename
def max_access_level(*levels: Optional[str]) -> Optional[str]: """Return the highest AccessLevel from args, skipping None. Returns None if all None.""" - valid = [l for l in levels if l is not None] - return max(valid, key=lambda l: LEVEL_RANK[l]) if valid else None + valid = [lvl for lvl in levels if lvl is not None] + return max(valid, key=lambda lvl: LEVEL_RANK[lvl]) if valid else None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ddpui/models/resource_share.py` around lines 31 - 34, Rename the ambiguous loop variable l to lvl throughout max_access_level, including the list comprehension and LEVEL_RANK lookup, matching the established naming in resource-share access logic.Source: Linters/SAST tools
ddpui/schemas/access/resource_share_schema.py (1)
137-141: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNarrow
granted_levelso an approval cannot write a deny grant.
AccessLevelincludesno_access.respond_to_access_requestinddpui/api/access_api.pypassesgranted_levelstraight intoadd_grantsand then sets the status toapproved. A responder can therefore approve a request and store ano_accessgrant, which revokes the floor-based access the requester might otherwise get.AccessRequest.requested_levelalready restricts the model column to view/edit. Align the payload with that restriction.♻️ Proposed narrowing
class RespondToRequestPayload(Schema): """Body for ``POST /api/access/{rtype}/{id}/request-access/{req_id}/respond``.""" decision: Literal["approved", "declined"] - granted_level: Optional[AccessLevel] = None # defaults to requested_level when approved + # only view/edit may be granted — no_access is a revoke, not an approval + granted_level: Optional[Literal["view", "edit"]] = None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ddpui/schemas/access/resource_share_schema.py` around lines 137 - 141, Update RespondToRequestPayload.granted_level to accept only the view/edit access levels, matching AccessRequest.requested_level and preventing approved responses from creating no_access grants; leave the default-to-requested-level behavior unchanged.ddpui/api/user_org_api.py (1)
429-478: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid N+1 queries in group serialization.
_serialize_group_rowcallsgroup.members.count()once per group. The detail serializer selectsinvitationbut notinvitation__invited_new_role, so pending members can trigger one query each. Annotatemember_countwithCount("members")and include the invitation role inselect_related. (raw.githubusercontent.com)Proposed query changes
- "invitation" + "invitation__invited_new_role" - member_count=group.members.count(), + member_count=group.member_count,Add
.annotate(member_count=Count("members"))to the group-list query.Also applies to: 564-579
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ddpui/api/user_org_api.py` around lines 429 - 478, Eliminate per-group and per-pending-member queries in the group serializers: update the group-list queryset feeding _serialize_group_row to annotate member_count with Count("members"), then use that annotation instead of group.members.count(). In _serialize_group_detail, extend select_related to include invitation__invited_new_role so pending member role access remains query-free.Source: MCP tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ddpui/api/access_api.py`:
- Around line 190-206: Update the access-request creation flow around
AccessRequest.objects.create to catch IntegrityError from the
uq_access_request_pending_per_user constraint and raise HttpError 409 with the
existing pending-request message. Preserve the preliminary exists() check while
ensuring concurrent duplicate requests return 409 instead of propagating a
server error.
In `@ddpui/api/dashboard_native_api.py`:
- Around line 100-101: Map get_user_access(...) returning None to each
resource’s established 404 not-found response instead of 403 across
ddpui/api/dashboard_native_api.py:100-101, ddpui/api/charts_api.py:1036-1038 and
1067-1068, ddpui/api/kpi_api.py:140-141 and 262-263, and
ddpui/api/report_api.py:150-151, 163-164, and 190-191. Add tests comparing
nonexistent and existing-but-ungranted resource IDs to verify both return the
same 404 response.
In `@ddpui/api/kpi_api.py`:
- Around line 140-142: Update get_kpi to store the result of get_user_access in
access, continue rejecting None access, and pass access as access_level to
KPIService.kpi_to_response so resolved permissions are preserved in the
response.
In `@ddpui/api/user_org_api.py`:
- Around line 669-674: Wrap the ResourceShare cleanup and group deletion in the
endpoint’s existing database transaction mechanism so both operations in the
deletion flow succeed or roll back together. Update the block containing
ResourceShare.objects.filter(...).delete() and group.delete(), preserving their
order and ensuring any failure in group.delete() restores the removed shares.
In `@ddpui/core/access/resource_share.py`:
- Around line 54-108: Refactor sync_dashboard_cascade to batch-create chart and
KPI child shares with bulk_create(ignore_conflicts=True) instead of
per-component get_or_create calls, and combine stale chart/KPI deletion
predicates with Q. Wrap each source write and its subsequent
sync_dashboard_cascade call in one transaction.atomic() block in
update_dashboard, add_grants, and update_grant; do not rely on a transaction
only inside the sync function.
In `@ddpui/core/kpi/kpi_service.py`:
- Around line 188-195: Update get_kpi_summary to apply
accessible_filter(orguser, ResourceType.KPI) when building its KPI query,
ensuring both KPI values and RAG status are restricted to resources accessible
by the requesting orguser rather than organization membership alone.
In `@ddpui/services/chart_service.py`:
- Around line 92-102: Update every service-layer list call to pass the existing
orguser argument, including test and equivalent callers of KPIService.list_kpis
and ReportService.list_snapshots that currently provide only org. Preserve the
service method signatures and ensure all callers satisfy the required
authorization context.
---
Outside diff comments:
In `@ddpui/api/user_org_api.py`:
- Around line 351-363: Update the OrgUser queryset in the People-row
construction to exclude deactivated users by filtering on user__is_active=True
before iterating, so the existing status="active" assignment remains accurate.
- Around line 351-363: Update the PeopleRow construction in the
organization-user loop to set created_at from curr_orguser.created_at,
preserving the organization membership timestamp rather than the platform user's
date_joined.
- Around line 582-607: Add a PostgreSQL unique constraint to OrgUserGroup on org
and Lower("name"), and create the corresponding migration. Update
create_user_group and the group-rename endpoint to catch the resulting
IntegrityError and return HTTP 400 using the existing duplicate-name error
response, while retaining their current validation checks.
- Around line 533-561: Add a conditional UniqueConstraint on OrgUserGroupMember
for (group, orguser) when orguser is not null, alongside the existing invitation
constraint. Update _add_members_to_group to use conflict-safe inserts for both
orguser and invitation memberships, preserving idempotent behavior under
concurrent calls.
In `@ddpui/core/access/resource_share.py`:
- Around line 330-348: Scope grant mutations to the authorized resource: in
ddpui/core/access/resource_share.py lines 330-348, add rtype and resource_id to
update_grant and filter by resource_type and stringified resource_id; apply the
same lookup scoping to remove_grant at lines 351-356. In ddpui/api/access_api.py
lines 93-103 and 110-118, pass rtype and resource_id into update_grant and
remove_grant respectively.
---
Nitpick comments:
In `@ddpui/api/user_org_api.py`:
- Around line 429-478: Eliminate per-group and per-pending-member queries in the
group serializers: update the group-list queryset feeding _serialize_group_row
to annotate member_count with Count("members"), then use that annotation instead
of group.members.count(). In _serialize_group_detail, extend select_related to
include invitation__invited_new_role so pending member role access remains
query-free.
In `@ddpui/models/resource_share.py`:
- Around line 31-34: Rename the ambiguous loop variable l to lvl throughout
max_access_level, including the list comprehension and LEVEL_RANK lookup,
matching the established naming in resource-share access logic.
In `@ddpui/schemas/access/resource_share_schema.py`:
- Around line 137-141: Update RespondToRequestPayload.granted_level to accept
only the view/edit access levels, matching AccessRequest.requested_level and
preventing approved responses from creating no_access grants; leave the
default-to-requested-level behavior unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dbdc0830-68a2-4636-9fb4-c02653e01f20
📒 Files selected for processing (19)
ddpui/api/access_api.pyddpui/api/charts_api.pyddpui/api/dashboard_native_api.pyddpui/api/kpi_api.pyddpui/api/org_preferences_api.pyddpui/api/report_api.pyddpui/api/user_org_api.pyddpui/core/access/access_control.pyddpui/core/access/ownership.pyddpui/core/access/resource_share.pyddpui/core/access/shareable_types.pyddpui/core/kpi/kpi_service.pyddpui/core/reports/report_service.pyddpui/models/resource_share.pyddpui/schemas/access/resource_share_schema.pyddpui/schemas/chart_schemas/crud.pyddpui/schemas/kpi_schema.pyddpui/schemas/report_schema.pyddpui/services/chart_service.py
🚧 Files skipped from review as they are similar to previous changes (2)
- ddpui/core/access/access_control.py
- ddpui/api/org_preferences_api.py
| if AccessRequest.objects.filter( | ||
| org=orguser.org, | ||
| resource_type=rtype, | ||
| resource_id=str(resource_id), | ||
| requester=orguser, | ||
| status=AccessRequestStatus.PENDING, | ||
| ).exists(): | ||
| raise HttpError(409, "a pending request already exists for this resource") | ||
|
|
||
| req = AccessRequest.objects.create( | ||
| org=orguser.org, | ||
| resource_type=rtype, | ||
| resource_id=str(resource_id), | ||
| requester=orguser, | ||
| requested_level=payload.requested_level, | ||
| note=payload.note, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle the unique-constraint violation on concurrent access requests.
The exists() check and the create() are separate statements. AccessRequest now carries the conditional constraint uq_access_request_pending_per_user. Two concurrent requests from the same requester can both pass the check, and the second create() then raises IntegrityError, which surfaces as HTTP 500 instead of the intended 409.
🛡️ Proposed fix
+from django.db import IntegrityError
+
@@
- req = AccessRequest.objects.create(
- org=orguser.org,
- resource_type=rtype,
- resource_id=str(resource_id),
- requester=orguser,
- requested_level=payload.requested_level,
- note=payload.note,
- )
+ try:
+ req = AccessRequest.objects.create(
+ org=orguser.org,
+ resource_type=rtype,
+ resource_id=str(resource_id),
+ requester=orguser,
+ requested_level=payload.requested_level,
+ note=payload.note,
+ )
+ except IntegrityError as err:
+ raise HttpError(409, "a pending request already exists for this resource") from err📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if AccessRequest.objects.filter( | |
| org=orguser.org, | |
| resource_type=rtype, | |
| resource_id=str(resource_id), | |
| requester=orguser, | |
| status=AccessRequestStatus.PENDING, | |
| ).exists(): | |
| raise HttpError(409, "a pending request already exists for this resource") | |
| req = AccessRequest.objects.create( | |
| org=orguser.org, | |
| resource_type=rtype, | |
| resource_id=str(resource_id), | |
| requester=orguser, | |
| requested_level=payload.requested_level, | |
| note=payload.note, | |
| ) | |
| if AccessRequest.objects.filter( | |
| org=orguser.org, | |
| resource_type=rtype, | |
| resource_id=str(resource_id), | |
| requester=orguser, | |
| status=AccessRequestStatus.PENDING, | |
| ).exists(): | |
| raise HttpError(409, "a pending request already exists for this resource") | |
| try: | |
| req = AccessRequest.objects.create( | |
| org=orguser.org, | |
| resource_type=rtype, | |
| resource_id=str(resource_id), | |
| requester=orguser, | |
| requested_level=payload.requested_level, | |
| note=payload.note, | |
| ) | |
| except IntegrityError as err: | |
| raise HttpError(409, "a pending request already exists for this resource") from err |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ddpui/api/access_api.py` around lines 190 - 206, Update the access-request
creation flow around AccessRequest.objects.create to catch IntegrityError from
the uq_access_request_pending_per_user constraint and raise HttpError 409 with
the existing pending-request message. Preserve the preliminary exists() check
while ensuring concurrent duplicate requests return 409 instead of propagating a
server error.
| ResourceShare.objects.filter( | ||
| org=group.org, | ||
| principal_type=ResourceSharePrincipalType.GROUP, | ||
| principal_id=group.id, | ||
| ).delete() | ||
| group.delete() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make resource-share cleanup and group deletion atomic.
The endpoint deletes ResourceShare rows before group.delete(). If group deletion fails, the group remains but its grants are already gone. Wrap both operations in one transaction.
Proposed fix
- ResourceShare.objects.filter(
- org=group.org,
- principal_type=ResourceSharePrincipalType.GROUP,
- principal_id=group.id,
- ).delete()
- group.delete()
+ with transaction.atomic():
+ ResourceShare.objects.filter(
+ org=group.org,
+ principal_type=ResourceSharePrincipalType.GROUP,
+ principal_id=group.id,
+ ).delete()
+ group.delete()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ResourceShare.objects.filter( | |
| org=group.org, | |
| principal_type=ResourceSharePrincipalType.GROUP, | |
| principal_id=group.id, | |
| ).delete() | |
| group.delete() | |
| with transaction.atomic(): | |
| ResourceShare.objects.filter( | |
| org=group.org, | |
| principal_type=ResourceSharePrincipalType.GROUP, | |
| principal_id=group.id, | |
| ).delete() | |
| group.delete() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ddpui/api/user_org_api.py` around lines 669 - 674, Wrap the ResourceShare
cleanup and group deletion in the endpoint’s existing database transaction
mechanism so both operations in the deletion flow succeed or roll back together.
Update the block containing ResourceShare.objects.filter(...).delete() and
group.delete(), preserving their order and ensuring any failure in
group.delete() restores the removed shares.
| def sync_dashboard_cascade(dashboard: Dashboard) -> None: | ||
| """Full sync of cascade child rows for all shares on a dashboard. | ||
|
|
||
| After this call, cascade children exactly match the dashboard's current | ||
| tabs — correct level, correct set of charts/KPIs, stale rows removed. | ||
| Call after any share change (add/update) or any tabs change. | ||
| Invitation-backed shares are skipped — promoted on acceptance. | ||
| """ | ||
| inner = _parse_inner_ids(dashboard.tabs) | ||
| current_chart_ids = [str(cid) for cid in inner["chart_ids"]] | ||
| current_kpi_ids = [str(kid) for kid in inner["kpi_ids"]] | ||
|
|
||
| dashboard_shares = list( | ||
| ResourceShare.objects.filter( | ||
| org=dashboard.org, | ||
| resource_type=ResourceType.DASHBOARD, | ||
| resource_id=str(dashboard.id), | ||
| parent__isnull=True, | ||
| principal_type__isnull=False, | ||
| ) | ||
| ) | ||
| if not dashboard_shares: | ||
| return | ||
|
|
||
| for share in dashboard_shares: | ||
| ResourceShare.objects.filter(parent=share).update(access_level=share.access_level) | ||
|
|
||
| for chart_id in current_chart_ids: | ||
| ResourceShare.objects.get_or_create( | ||
| org=dashboard.org, | ||
| resource_type=ResourceType.CHART, | ||
| resource_id=chart_id, | ||
| principal_type=share.principal_type, | ||
| principal_id=share.principal_id, | ||
| parent=share, | ||
| defaults={"access_level": share.access_level, "created_by": share.created_by}, | ||
| ) | ||
| for kpi_id in current_kpi_ids: | ||
| ResourceShare.objects.get_or_create( | ||
| org=dashboard.org, | ||
| resource_type=ResourceType.KPI, | ||
| resource_id=kpi_id, | ||
| principal_type=share.principal_type, | ||
| principal_id=share.principal_id, | ||
| parent=share, | ||
| defaults={"access_level": share.access_level, "created_by": share.created_by}, | ||
| ) | ||
|
|
||
| ResourceShare.objects.filter( | ||
| parent=share, resource_type=ResourceType.CHART | ||
| ).exclude(resource_id__in=current_chart_ids).delete() | ||
|
|
||
| ResourceShare.objects.filter( | ||
| parent=share, resource_type=ResourceType.KPI | ||
| ).exclude(resource_id__in=current_kpi_ids).delete() |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | grep -E '(^|/)(resource_share\.py|dashboard_native_api\.py|.*models.*\.py)$' | head -200
printf '%s\n' '--- resource_share.py outline and relevant references ---'
ast-grep outline ddpui/core/access/resource_share.py --lang python || true
rg -n -C 5 'sync_dashboard_cascade|uq_resource_share_cascade_grant|class ResourceShare|UniqueConstraint|bulk_create|get_or_create|transaction' ddpui/core/access ddpui/api ddpui -g '*.py' | head -400
printf '%s\n' '--- dashboard update call sites ---'
rg -n -C 8 'sync_dashboard_cascade|payload\.tabs|tabs is not None' ddpui/api ddpui -g '*.py' | head -300Repository: DalgoT4D/DDP_backend
Length of output: 41229
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- resource share implementation ---'
sed -n '1,125p' ddpui/core/access/resource_share.py
printf '%s\n' '--- ResourceShare model ---'
sed -n '1,260p' ddpui/models/resource_share.py
printf '%s\n' '--- migrations containing cascade constraint ---'
rg -n -C 12 'uq_resource_share_cascade_grant|cascade_grant|UniqueConstraint|unique' ddpui/migrations ddpui/models/resource_share.py -g '*.py' | head -300
printf '%s\n' '--- dashboard update flow ---'
sed -n '135,215p' ddpui/api/dashboard_native_api.py
sed -n '340,410p' ddpui/services/dashboard_service.pyRepository: DalgoT4D/DDP_backend
Length of output: 40484
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- migration inventory and constraint history ---'
git ls-files ddpui/migrations | tail -30
rg -n 'ResourceShare|resource_share|uq_resource_share' ddpui/migrations -g '*.py' | tail -200
printf '%s\n' '--- grant write flows ---'
sed -n '220,355p' ddpui/core/access/resource_share.py
printf '%s\n' '--- repository diff summary ---'
git diff --stat
git status --short
printf '%s\n' '--- behavioral verifier: ORM operation count and constraint fields ---'
python3 - <<'PY'
import ast
from pathlib import Path
path = Path("ddpui/core/access/resource_share.py")
tree = ast.parse(path.read_text())
fn = next(n for n in tree.body if isinstance(n, ast.FunctionDef)
and n.name == "sync_dashboard_cascade")
calls = []
for node in ast.walk(fn):
if isinstance(node, ast.Call):
text = ast.unparse(node.func)
if text.endswith(("filter", "update", "get_or_create", "delete")):
calls.append(text)
print("ORM call sites:", calls)
print("per-share get_or_create call sites:",
sum(x.endswith("get_or_create") for x in calls))
print("per-share update call sites:",
sum(x.endswith("update") for x in calls))
print("per-share delete call sites:",
sum(x.endswith("delete") for x in calls))
model = ast.parse(Path("ddpui/models/resource_share.py").read_text())
for node in ast.walk(model):
if isinstance(node, ast.ClassDef) and node.name == "ResourceShare":
for child in ast.walk(node):
if isinstance(child, ast.Call) and ast.unparse(child.func) == "models.UniqueConstraint":
keywords = {kw.arg: ast.literal_eval(kw.value) for kw in child.keywords
if kw.arg in {"fields", "name"}}
print("constraint:", keywords)
PYRepository: DalgoT4D/DDP_backend
Length of output: 8491
Batch cascade synchronization and make the source update atomic.
sync_dashboard_cascade performs one ORM lookup per component, plus per-share updates and deletes. Use bulk_create(..., ignore_conflicts=True) for missing child rows and combine stale-row deletes with Q.
Wrap the source write and cascade sync in the same transaction.atomic() block. update_dashboard, add_grants, and update_grant save the source row before calling sync_dashboard_cascade; a transaction inside sync_dashboard_cascade alone cannot prevent stale children if the sync fails. The uq_resource_share_cascade_grant constraint already covers the required fields.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ddpui/core/access/resource_share.py` around lines 54 - 108, Refactor
sync_dashboard_cascade to batch-create chart and KPI child shares with
bulk_create(ignore_conflicts=True) instead of per-component get_or_create calls,
and combine stale chart/KPI deletion predicates with Q. Wrap each source write
and its subsequent sync_dashboard_cascade call in one transaction.atomic() block
in update_dashboard, add_grants, and update_grant; do not rely on a transaction
only inside the sync function.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1436 +/- ##
==========================================
+ Coverage 65.75% 66.70% +0.95%
==========================================
Files 170 191 +21
Lines 19661 20751 +1090
==========================================
+ Hits 12928 13842 +914
- Misses 6733 6909 +176 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
siddhant3030
left a comment
siddhant3030
left a comment
There was a problem hiding this comment.
Three structural issues — all variants of the same "write it once" principle from the repo's own rules (layer architecture, empty init.py, service-delegation skill). Details inline.
| return levels | ||
|
|
||
|
|
||
| def get_access_map_for_resource(org, rtype: str, resource_id) -> dict[int, str]: |
There was a problem hiding this comment.
Why can’t this decision logic be a single function? The same rule—owner/admin → edit; private → explicit grant only; otherwise max(grant, floor)—is duplicated three times in this file (get_user_access:93, get_user_access_map:121), and the no_access handling already differs.
Something like effective_level(...) would give us one place to read, change, and unit-test the rule, preventing the implementations from drifting.
def effective_level(is_owner_or_admin, grant, floor, is_private):
if is_owner_or_admin:
return AccessLevel.EDIT
if is_private:
return grant or AccessLevel.NO_ACCESS
return max_access_level(grant, floor) or AccessLevel.NO_ACCESS| 4. org-default floor -> default_analyst_level / default_member_level | ||
|
|
||
| ``no_access`` at step 2 is the explicit deny — the only way to revoke one | ||
| principal below a permissive org floor. This is precedence, not max-merge, |
There was a problem hiding this comment.
This docstring promise isn't what the code does: _grants_map ends with a max-merge of user and group levels, so an explicit USER no_access loses to any group grant (and to a permissive floor in get_user_access). Either make the code precedence-based as documented, or fix the docstring — right now the most-read lines of the file describe an algorithm the file doesn't implement.
| return prefs.default_member_level | ||
|
|
||
|
|
||
| def _grants_map( |
There was a problem hiding this comment.
nit: this map keys by str(resource_id), so every caller writes grants.get(str(...)) and accessible_filter converts back with int(rid). Normalizing to one key type here would remove the juggling at all call sites.
| ``test_trial_shell_is_single_source_of_truth`` enforce the invariant. | ||
| """ | ||
|
|
||
| from ddpui.core.notifications.templates.alert import render_alert_email |
There was a problem hiding this comment.
Repo invariant: core feature __init__.py stays empty (no re-exports, no __all__). This 58-line hub adds no behavior, puts an extra hop in every "where does this live" search, and re-exports two underscore-private names (_render_email_shell) as if they were public API. Suggest emptying it and importing from the concrete modules.
| allowed_ids = [int(rid) for rid, level in grants.items() if level != AccessLevel.NO_ACCESS] | ||
| denied_ids = [int(rid) for rid, level in grants.items() if level == AccessLevel.NO_ACCESS] |
There was a problem hiding this comment.
resource_id is a CharField with no numeric constraint, so one malformed row ("", "12 ", a future writer bug) makes int(rid) raise — and since this filter runs in every list endpoint, that one row 500s every list page for that user (or a whole group, if it's a group grant). Q(id__in=...) accepts strings directly, so the cast can just be dropped — or skip-and-log non-numeric rows.
2 participants
Summary by CodeRabbit