feat: make redirect on booking a teams-only feature with grandfathering#28076
feat: make redirect on booking a teams-only feature with grandfathering#28076
Conversation
Co-Authored-By: peer@cal.com <peer@cal.com>
🤖 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:
|
Co-Authored-By: peer@cal.com <peer@cal.com>
There was a problem hiding this comment.
2 issues found across 8 files (changes from recent commits).
Prompt for AI agents (all 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="packages/features/eventtypes/lib/successRedirectUrlAllowed.ts">
<violation number="1" location="packages/features/eventtypes/lib/successRedirectUrlAllowed.ts:33">
P2: Use a membership lookup that selects only the needed field (e.g., `id`) or a boolean-only helper to avoid fetching the full membership row when checking team plan eligibility.</violation>
</file>
<file name="packages/trpc/server/routers/viewer/eventTypes/heavy/update.handler.ts">
<violation number="1" location="packages/trpc/server/routers/viewer/eventTypes/heavy/update.handler.ts:847">
P1: The redirect URL plan check runs after the event type update, so the database already contains the new redirect URL. Since `checkSuccessRedirectUrlAllowed` allows updates when a redirect URL exists, this gate will always pass and free users can still set `successRedirectUrl`. Move the check before the update so unauthorized changes are blocked before persisting.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
packages/trpc/server/routers/viewer/eventTypes/heavy/update.handler.ts
Outdated
Show resolved
Hide resolved
Devin AI is addressing Cubic AI's review feedbackA Devin session has been created to address the issues identified by Cubic AI. |
|
Reviewed the Cubic AI feedback on this PR. Both issues have confidence scores below the 9/10 threshold for automated fixes:
No changes made as neither issue meets the 9/10 confidence threshold for automated fixes. These can be reviewed manually if needed. |
sean-brydon
left a comment
There was a problem hiding this comment.
Overall this looks pretty solid - Is this something we want to allow on team/org trials?
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
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="packages/features/ee/teams/lib/checkUserHasActivePaidTeamPlan.ts">
<violation number="1" location="packages/features/ee/teams/lib/checkUserHasActivePaidTeamPlan.ts:49">
P2: Select only the plan field from platformBilling to avoid fetching unnecessary columns and potential sensitive data exposure.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
What does this PR do?
Gates the "Redirect on booking" (
successRedirectUrl) feature behind a team plan, both in the UI and the API v2 backend.UI (EventAdvancedTab)
UpgradeTeamsBadge(same pattern as CalVideoSettings)isRedirectUrlGrandfathered:trueifeventType.successRedirectUrlis already set (from DB)isRedirectUrlDisabled:trueonly if NOT grandfathered AND user has no team planAPI v2 Backend Enforcement
createUserEventType): IfsuccessRedirectUrlis provided, checks the user has an accepted published team membership. Throws403 Forbiddenif not.updateEventType): IfsuccessRedirectUrlis being changed, first checks whether the event type already has a redirect URL (grandfathered). Only enforces the team plan check for event types with no existing redirect.MembershipsRepository.hasAcceptedPublishedTeamMembership()— checks for an accepted membership in a team with a non-null slugEventTypesRepository_2024_06_14.getSuccessRedirectUrl()— fetches only thesuccessRedirectUrlfield for grandfathering checksRequested by: @PeerRich
Link to Devin run: https://app.devin.ai/sessions/b33de57ca82548cf9b5a8a34317b1606
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
UI
API v2
POST /v2/event-typesas a free user withsuccessRedirectUrlset → should return403POST /v2/event-typesas a team user withsuccessRedirectUrlset → should succeedPATCH /v2/event-types/:idas a free user on an event type with no existing redirect, settingsuccessRedirectUrl→ should return403PATCH /v2/event-types/:idas a free user on an event type that already has a redirect URL → should succeed (grandfathered)Human Review Checklist
hasAcceptedPublishedTeamMembershipvsuseHasTeamPlanalignment: The API v2 check queries for an accepted membership in a team with a non-null slug. Verify this produces equivalent results to the UI'suseHasTeamPlan()hook (which callshasTeamPlanHandlervia tRPC).successRedirectUrl(sets tonull) and then tries to set it again later, they'd lose grandfathering. Confirm this is the intended behavior.successRedirectUrlis gated — the "Redirect on no routing form response" toggle is NOT gated. Confirm this is the intended scope.event-types.service.ts— this is auto-formatter noise from biome, not intentional changes.