Fix/deduplicate username validation#29789
Conversation
- Replaces flawed containment logic with strict interval intersection math - Centralizes Prisma where-clause generation into prismaIntervalOverlap.ts - Migrates getBusyTimes, BookingRepository, and HolidayRepository to new utility - Adds comprehensive behavioral and unit test coverage for overlapping scenarios
fix: resolve busy time overlap logic and centralize interval queries
|
Welcome to Cal.diy, @Ashid332! Thanks for opening this pull request. A few things to keep in mind:
A maintainer will review your PR soon. Thanks for contributing! |
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a shared Prisma interval-overlap helper with strict and inclusive boundary modes, validation, and unit tests. Booking, busy-time, and holiday queries now use the helper for overlap filtering, with behavioral coverage for booking-limit intervals. Signup username checking now reuses the existing username check result and adjusts availability and suggestions for existing users who are not organization members. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/lib/server/username.ts (1)
234-261: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEnsure correct availability for org members claiming their own username and apply early returns.
By moving the base availability check to
usernameCheck(usernameRaw),check.availablewill initially befalseif the user's username is already set in the database (becauseusernameCheckwill find them in the global namespace).For a regular user (
!userIsAMemberOfAnOrg), the code correctly overrides this totruewhen they claim their own username. However, ifuserIsAMemberOfAnOrgis true, theif (!userIsAMemberOfAnOrg)block is bypassed entirely, leavingcheck.availableasfalse. This breaks the scenario where an org user claims their already-set username (which the test snippet expects to betrue).To resolve this regression, the
isClaimingAlreadySetUsernamecheck should be extracted so it applies to org members as well. Additionally, as per coding guidelines, we should use an early return (if (!user) return check;) to reduce nesting, and specify aselectclause in the PrismafindFirstquery for better performance.🐛 Proposed fix and refactor
- if (user) { - // TODO: When supporting multiple profiles of a user, we would need to check if the user has a membership with the correct organization - const userIsAMemberOfAnOrg = await prisma.membership.findFirst({ - where: { - userId: user.id, - team: { - isOrganization: true, - }, - }, - }); - - // When we invite an email, that doesn't match the orgAutoAcceptEmail, we create a user with organizationId=null. - // The only way to differentiate b/w 'a new email that was invited to an Org' and 'a user that was created using regular signup' is to check if the user is a member of an org. - // If username is in global namespace - if (!userIsAMemberOfAnOrg) { - const isClaimingAlreadySetUsername = user.username === username; - const isClaimingUnsetUsername = !user.username; - - if (!isClaimingUnsetUsername && !isClaimingAlreadySetUsername) { - check.available = false; - } else if (isClaimingAlreadySetUsername) { - check.available = true; - check.suggestedUsername = ""; - } - } - } - - return check; + if (!user) { + return check; + } + + // TODO: When supporting multiple profiles of a user, we would need to check if the user has a membership with the correct organization + const userIsAMemberOfAnOrg = await prisma.membership.findFirst({ + where: { + userId: user.id, + team: { + isOrganization: true, + }, + }, + select: { + id: true, + }, + }); + + // When we invite an email, that doesn't match the orgAutoAcceptEmail, we create a user with organizationId=null. + // The only way to differentiate b/w 'a new email that was invited to an Org' and 'a user that was created using regular signup' is to check if the user is a member of an org. + const isClaimingAlreadySetUsername = user.username === username; + const isClaimingUnsetUsername = !user.username; + + if (isClaimingAlreadySetUsername) { + check.available = true; + check.suggestedUsername = ""; + } else if (!userIsAMemberOfAnOrg && !isClaimingUnsetUsername) { + check.available = false; + } + + return check;🤖 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 `@packages/lib/server/username.ts` around lines 234 - 261, Refactor the logic around usernameCheck so it returns immediately when no user exists, and add a minimal select clause to the membership.findFirst query. Compute isClaimingAlreadySetUsername for every existing user, allowing check.available and suggestedUsername to reflect ownership regardless of organization membership; keep the unset-username and non-member restrictions within the appropriate conditional.Source: Coding guidelines
🧹 Nitpick comments (1)
packages/features/busyTimes/services/getBusyTimes.test.ts (1)
402-422: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace Day.js with native
Date.As per coding guidelines, use native
Dateinstead of Day.js when timezone awareness isn't needed. The mock emulation and test parameters can be written simply with native Date methods, eliminating the overhead of Day.js object instantiation.
packages/features/busyTimes/services/getBusyTimes.test.ts#L402-L422: Usenew Date(...)and native.getTime()comparisons instead of.isBefore()/.isAfter().packages/features/busyTimes/services/getBusyTimes.test.ts#L461-L477: Usenew Date(...)instead ofdayjs(...).toDate(), and.toISOString()instead of.format().🤖 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 `@packages/features/busyTimes/services/getBusyTimes.test.ts` around lines 402 - 422, Replace Day.js with native Date throughout the affected test setup in packages/features/busyTimes/services/getBusyTimes.test.ts lines 402-422 and 461-477: use Date values for requestedStart/requestedEnd and booking conversions, compare timestamps with getTime() in setupPrismaMock, and use toISOString() where the existing code formats dates. No other behavior should change.Source: Coding guidelines
🤖 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 `@packages/lib/server/username.ts`:
- Around line 234-261: Refactor the logic around usernameCheck so it returns
immediately when no user exists, and add a minimal select clause to the
membership.findFirst query. Compute isClaimingAlreadySetUsername for every
existing user, allowing check.available and suggestedUsername to reflect
ownership regardless of organization membership; keep the unset-username and
non-member restrictions within the appropriate conditional.
---
Nitpick comments:
In `@packages/features/busyTimes/services/getBusyTimes.test.ts`:
- Around line 402-422: Replace Day.js with native Date throughout the affected
test setup in packages/features/busyTimes/services/getBusyTimes.test.ts lines
402-422 and 461-477: use Date values for requestedStart/requestedEnd and booking
conversions, compare timestamps with getTime() in setupPrismaMock, and use
toISOString() where the existing code formats dates. No other behavior should
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 62e277a5-15ee-496f-9454-c6dd114b04d2
📒 Files selected for processing (7)
packages/features/bookings/repositories/BookingRepository.tspackages/features/busyTimes/services/getBusyTimes.test.tspackages/features/busyTimes/services/getBusyTimes.tspackages/features/holidays/repositories/HolidayRepository.tspackages/lib/prismaIntervalOverlap.test.tspackages/lib/prismaIntervalOverlap.tspackages/lib/server/username.ts
1 participant
What does this PR do?
Visual Demo (For contributors especially)
A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).
Video Demo (if applicable):
Image Demo (if applicable):
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist