Fix/org owners edit own username#24378
Conversation
…rnames - Fix UserListTable to show edit option for org owners/admins on themselves - Fix MemberList to show edit option for org owners/admins on themselves - Resolves issue where org owners/admins couldn't see triple dot menu to edit their own usernames - Maintains existing security by only allowing edit for users with proper permissions Fixes calcom#24377
|
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: |
|
@antcybersec is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
bot
added
organizations
WalkthroughThis change updates Google Calendar integration to support per-event custom reminder minutes during create and update, modifies guestsCanSeeOtherGuests based on seatsPerTimeSlot, and adjusts an error type guard. It adds optional customReminderMinutes to CalendarEvent and CalendarServiceEvent types. In the teams and users UI, edit permissions now allow org admins/owners to edit their own entries; minor refactors include local variable renames and added braces in switch cases. MemberList’s Props gains facetedTeamValues and permissions fields; internal hook variable is renamed. No exported function signatures are changed beyond the added optional type fields. Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 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.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/features/ee/teams/components/MemberList.tsx (1)
66-70: Remove the duplicate Props interface definition.This Props interface is shadowed by the second definition at lines 150-166, making it dead code. The second definition includes additional required fields (
facetedTeamValuesandpermissions) that are actually used in the component.Apply this diff to remove the duplicate:
-interface Props { - team: NonNullable<RouterOutputs["viewer"]["teams"]["get"]>; - isOrgAdminOrOwner: boolean | undefined; - setShowMemberInvitationModal: Dispatch<SetStateAction<boolean>>; -} -
🧹 Nitpick comments (3)
packages/types/Calendar.d.ts (1)
267-268: Note: Redundant field declaration.Since
CalendarServiceEventextendsCalendarEvent(line 266), thecustomReminderMinutesfield is inherited. While explicit redeclaration can aid clarity, it introduces maintenance risk if the types diverge.Consider removing this redundant declaration to avoid duplication:
export interface CalendarServiceEvent extends CalendarEvent { - // Optional per-calendar reminder override in minutes for integrations that support it - customReminderMinutes?: number; calendarDescription: string; }packages/app-store/googlecalendar/lib/CalendarService.ts (1)
219-219: Confirm guest visibility logic
Change aligns with the Office365 implementation (when seatsPerTimeSlot is set, guestsCanSeeOtherGuests ⇄ seatsShowAttendees). Consider adding a test case for a truthy seatsPerTimeSlot with seatsShowAttendees=false to assert guestsCanSeeOtherGuests=false.packages/features/ee/teams/components/MemberList.tsx (1)
690-695: Consider clarifying the intentionally unused return value.The underscore prefix appropriately indicates this value is intentionally unused. However, if the hook's return value is truly unnecessary, consider using
voidor adding a comment to make the intent explicit.Alternative approaches:
- const _fetchMoreOnBottomReached = useFetchMoreOnBottomReached({ + // Hook sets up scroll listener for infinite pagination; return value unused + useFetchMoreOnBottomReached({ tableContainerRef, hasNextPage, fetchNextPage, isFetching, });or
- const _fetchMoreOnBottomReached = useFetchMoreOnBottomReached({ + void useFetchMoreOnBottomReached({ tableContainerRef, hasNextPage, fetchNextPage, isFetching, });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/app-store/googlecalendar/lib/CalendarService.ts(5 hunks)packages/features/ee/teams/components/MemberList.tsx(3 hunks)packages/features/users/components/UserTable/UserListTable.tsx(4 hunks)packages/types/Calendar.d.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/types/Calendar.d.tspackages/app-store/googlecalendar/lib/CalendarService.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/types/Calendar.d.tspackages/app-store/googlecalendar/lib/CalendarService.tspackages/features/users/components/UserTable/UserListTable.tsxpackages/features/ee/teams/components/MemberList.tsx
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/types/Calendar.d.tspackages/app-store/googlecalendar/lib/CalendarService.tspackages/features/users/components/UserTable/UserListTable.tsxpackages/features/ee/teams/components/MemberList.tsx
**/*Service.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Service files must include
Servicesuffix, use PascalCase matching exported class, and avoid generic names (e.g.,MembershipService.ts)
Files:
packages/app-store/googlecalendar/lib/CalendarService.ts
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/features/users/components/UserTable/UserListTable.tsxpackages/features/ee/teams/components/MemberList.tsx
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (7)
packages/features/users/components/UserTable/UserListTable.tsx (1)
451-461: Self-edit now correctly unlocked for admins/owners.The updated predicate lets accepted admins/owners reach their own edit actions while keeping regular members restricted. Matches the PR goal without loosening other checks.
packages/types/Calendar.d.ts (1)
214-215: LGTM: Well-documented optional field addition.The optional
customReminderMinutesfield is clearly documented and appropriately typed. The comment explains its purpose for integrations that support custom reminders.packages/app-store/googlecalendar/lib/CalendarService.ts (4)
45-46: LGTM: Safer property check.Using
Object.prototype.hasOwnProperty.callinstead of directhasOwnPropertyis a best practice that avoids potential issues with objects that don't inherit from Object.prototype or have overridden the method.
205-218: LGTM: Correct reminder override logic.The conditional logic correctly applies custom reminders when
customReminderMinutesis defined and finite, with appropriate safeguards:
Number.isFinite()check prevents NaN and InfinityMath.max(0, ...)clamps negative values to zeroMath.floor()ensures integer minutes
383-396: LGTM: Consistent reminder override logic.The reminder logic in
updateEventcorrectly mirrors the implementation increateEvent, maintaining consistency across event creation and updates.
397-397: Verify consistency: guestsCanSeeOtherGuests behavioral change.This change mirrors the one in
createEvent(line 219), ensuring consistent behavior between event creation and updates. The conditional logic based onseatsPerTimeSlotis correct.Confirm this behavioral change is documented and tested, as it affects how guests see other attendees in seated events.
packages/features/ee/teams/components/MemberList.tsx (1)
436-437: VerifyisOrgAdminOrOwnerisn’t undefined
props.isOrgAdminOrOwneris typed asboolean | undefined; if it’sundefinedwhenisSelfis true,editModewill befalseand org-admins/owners couldn’t edit themselves. Ensure this prop is always a boolean at runtime or coalesce it (e.g.!!props.isOrgAdminOrOwner).
| // Determine optional custom reminder minutes from event payload or destination calendar metadata | ||
| // We intentionally access via `any` to avoid widening Calendar types and to keep this change isolated. | ||
| const customReminderMinutes: number | undefined = | ||
| (calEvent as any)?.customReminderMinutes ?? | ||
| (calEvent.destinationCalendar?.find((cal) => cal.credentialId === credentialId) | ||
| ? (calEvent.destinationCalendar.find((cal) => cal.credentialId === credentialId) as any) | ||
| ?.customReminderMinutes | ||
| : undefined); |
There was a problem hiding this comment.
Remove unnecessary any-casts for type safety.
The type definitions in packages/types/Calendar.d.ts now include customReminderMinutes?: number on both CalendarEvent and CalendarServiceEvent. Since calEvent is typed as CalendarServiceEvent, you can access this field directly without casting to any.
The comment claims this avoids "widening Calendar types," but the types have already been widened in the type definition file. These casts bypass type safety without benefit.
Apply this diff to access the field safely:
- // Determine optional custom reminder minutes from event payload or destination calendar metadata
- // We intentionally access via `any` to avoid widening Calendar types and to keep this change isolated.
- const customReminderMinutes: number | undefined =
- (calEvent as any)?.customReminderMinutes ??
- (calEvent.destinationCalendar?.find((cal) => cal.credentialId === credentialId)
- ? (calEvent.destinationCalendar.find((cal) => cal.credentialId === credentialId) as any)
- ?.customReminderMinutes
- : undefined);
+ // Determine optional custom reminder minutes from event payload or destination calendar metadata
+ const customReminderMinutes: number | undefined =
+ calEvent.customReminderMinutes ??
+ calEvent.destinationCalendar?.find((cal) => cal.credentialId === credentialId)?.customReminderMinutes;📝 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.
| // Determine optional custom reminder minutes from event payload or destination calendar metadata | |
| // We intentionally access via `any` to avoid widening Calendar types and to keep this change isolated. | |
| const customReminderMinutes: number | undefined = | |
| (calEvent as any)?.customReminderMinutes ?? | |
| (calEvent.destinationCalendar?.find((cal) => cal.credentialId === credentialId) | |
| ? (calEvent.destinationCalendar.find((cal) => cal.credentialId === credentialId) as any) | |
| ?.customReminderMinutes | |
| : undefined); | |
| // Determine optional custom reminder minutes from event payload or destination calendar metadata | |
| const customReminderMinutes: number | undefined = | |
| calEvent.customReminderMinutes ?? | |
| calEvent.destinationCalendar?.find((cal) => cal.credentialId === credentialId)?.customReminderMinutes; |
🤖 Prompt for AI Agents
In packages/app-store/googlecalendar/lib/CalendarService.ts around lines 184 to
191, remove the unnecessary any casts and access customReminderMinutes directly
from the typed CalendarServiceEvent and its destinationCalendar entries; replace
the double any-cast/find with a single typed find result (e.g., assign
destinationCalendar.find(...) to a const typed as Calendar | undefined) and use
optional chaining (event.customReminderMinutes ?? found?.customReminderMinutes)
so the code relies on the declared types in packages/types/Calendar.d.ts and
preserves type safety.
| // Determine optional custom reminder minutes from event payload or destination calendar metadata | ||
| const updateCustomReminderMinutes: number | undefined = | ||
| (event as any)?.customReminderMinutes ?? | ||
| (event.destinationCalendar && externalCalendarId | ||
| ? ((event.destinationCalendar.find((cal) => cal.externalId === externalCalendarId) as any) | ||
| ?.customReminderMinutes as number | undefined) | ||
| : undefined); |
There was a problem hiding this comment.
Remove unnecessary any-casts for type safety (updateEvent).
Same issue as in createEvent: these any-casts are unnecessary since the types now include customReminderMinutes. Additionally, this code has a redundant find() operation - it searches destinationCalendar array twice.
Apply this diff:
- // Determine optional custom reminder minutes from event payload or destination calendar metadata
- const updateCustomReminderMinutes: number | undefined =
- (event as any)?.customReminderMinutes ??
- (event.destinationCalendar && externalCalendarId
- ? ((event.destinationCalendar.find((cal) => cal.externalId === externalCalendarId) as any)
- ?.customReminderMinutes as number | undefined)
- : undefined);
+ // Determine optional custom reminder minutes from event payload or destination calendar metadata
+ const updateCustomReminderMinutes: number | undefined =
+ event.customReminderMinutes ??
+ event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.customReminderMinutes;📝 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.
| // Determine optional custom reminder minutes from event payload or destination calendar metadata | |
| const updateCustomReminderMinutes: number | undefined = | |
| (event as any)?.customReminderMinutes ?? | |
| (event.destinationCalendar && externalCalendarId | |
| ? ((event.destinationCalendar.find((cal) => cal.externalId === externalCalendarId) as any) | |
| ?.customReminderMinutes as number | undefined) | |
| : undefined); | |
| // Determine optional custom reminder minutes from event payload or destination calendar metadata | |
| const updateCustomReminderMinutes: number | undefined = | |
| event.customReminderMinutes ?? | |
| event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.customReminderMinutes; |
🤖 Prompt for AI Agents
In packages/app-store/googlecalendar/lib/CalendarService.ts around lines 363 to
369, the code uses unnecessary any-casts and repeats a
destinationCalendar.find() call when resolving customReminderMinutes for
updateEvent; remove the any casts and perform a single find into a typed
variable (e.g. const destCal = event.destinationCalendar?.find(c => c.externalId
=== externalCalendarId)) then set updateCustomReminderMinutes =
event.customReminderMinutes ?? destCal?.customReminderMinutes, ensuring proper
typing (number | undefined) without using any.
1 participant
Fix: Allow org owners and admins to edit their own usernames
Problem: Org owners and admins couldn't see the triple dot menu to edit their own usernames in the organization member list.
Solution: Modified permission logic to allow org owners/admins to edit themselves while maintaining security for regular members.
Changes:
UserListTable.tsx: Allow edit for self if user is admin/owner
MemberList.tsx: Allow edit for self if user is org admin/owner
Security: Only users with proper admin/owner permissions can edit themselves; regular members still cannot.
Fixes #24377