fix: harden seed script org settings upsert and P2002 error handling#28527
Draft
fix: harden seed script org settings upsert and P2002 error handling#28527
Conversation
Co-Authored-By: romitgabani1 <romitgabani1.work@gmail.com>
Contributor
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Contributor
There was a problem hiding this comment.
3 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/seed.ts">
<violation number="1" location="scripts/seed.ts:333">
P2: The new org-settings upsert uses `existingTeam.id` without verifying `existingTeam.isOrganization`, so it can update the wrong team type on slug/root collisions.</violation>
<violation number="2" location="scripts/seed.ts:410">
P1: Custom agent: **Avoid Logging Sensitive Information**
Do not log usernames directly; this added log line exposes PII in seed/CI logs.</violation>
<violation number="3" location="scripts/seed.ts:411">
P1: Skipping P2002 members (`return null`) drops existing users from org membership/profile creation, so retries can still produce incomplete organization seeding.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
Devin AI is addressing Cubic AI's review feedbackA Devin session has been created to address the issues identified by Cubic AI. |
…eturning null - Replace username interpolation in log message with generic text (Cubic violation #2, confidence 9/10) - On P2002, fetch the existing user from DB and return it with membership data instead of returning null, which was dropping users from org setup on retries (Cubic violation #3, confidence 9/10) Co-Authored-By: bot_apk <apk@cognition.ai>
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
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 does this PR do?
Fixes flaky org-admin integration tests (
isAdmin,retrieveScopedAccessibleUsers,_get,_patch) that fail intermittently when the CI database is cached.Root cause: The seed script in
createOrganizationAndAddMembersAndTeamshad two bugs:Stale org settings on re-seed: When an organization already existed (cached DB), the function returned immediately without ensuring
organizationSettings.isAdminAPIEnabledwas set. This causedisAdminGuardto returnfalsefor org admins.P2002 error abandoned entire org creation: If any single org member hit a unique constraint violation, the batch-level
catchabandoned the entire function — meaning the org team, memberships, profiles, and settings were never created, even though some member users already existed.Changes
scripts/seed.ts:organizationSettingswhen the org already exists, instead of silently returningprisma.user.upsertforusersOutsideOrginstead ofprisma.user.createto avoid P2002 there tooTest files (
isAdmin.integration-test.ts,retrieveScopedAccessibleUsers.integration-test.ts):beforeAllhooks that were workarounds for the seed script bug — the proper fix is in the seed script itselfUpdates since last revision
Addressed Cubic AI review feedback (confidence ≥ 9/10):
Member ${member.memberData.username} already seeded, skippingwith a generic"Organization member already seeded, skipping"messagereturn nullon P2002 (which dropped the user from org membership/profile creation), the code now fetches the existing user viaprisma.user.findUniqueand returns it with the membership data so downstream setup proceedsexistingTeamlookup is scoped byslug+parentId: nullusing org-specific slugs, so a collision with a non-org team is not a realistic concern in seedingMandatory Tasks (DO NOT REMOVE)
How should this be tested?
The integration test suite should pass consistently on both fresh and cached CI databases:
To reproduce the flaky state: run the seed twice (simulating a cached DB) and verify the tests still pass on the second run.
Human Review Checklist
email_usernamecompound unique constraint exists in the Prisma schema (used in both theusersOutsideOrgupsert and the P2002 recoveryfindUnique)orgData.organizationSettingsshape is compatible withprisma.organizationSettings.upsertfor bothupdateandcreatefieldsbeforeAllworkarounds from test files is safe — integration tests should always run against a seeded DBLink to Devin session: https://app.devin.ai/sessions/6be6fd80dedc4fc8891cb523d7d4ddbe