feat(event): restrict event join to piscine/42cursus audience - #706
feat(event): restrict event join to piscine/42cursus audience#7064n4k1n wants to merge 5 commits into
Conversation
|
@copilot resolve the merge conflicts in this pull request |
|
Warning Review limit reached
Next review available in: 49 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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughAdds Piscine/cursus enrollment flags from 42 OAuth profiles, introduces audience-scoped events, enforces audience eligibility during registration, and updates event creation and team-access UI flows. ChangesEvent audience eligibility
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant EventController
participant EventService
participant SocialAccountService
User->>EventController: Request to join event
EventController->>EventService: Check event audience
EventService->>SocialAccountService: Read FortyTwo enrollment flags
SocialAccountService-->>EventService: Piscine/cursus eligibility
EventService-->>EventController: Accept or reject registration
EventController-->>User: Join result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
api/src/event/entities/event.entity.ts (1)
107-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using
type: "enum"for consistency with other entities.In
SocialAccountEntity, enums are mapped using@Column({ type: "enum", enum: ... }). Usingtype: "text"works, but explicitly using theenumtype provides stricter database-level validation and consistency across your entities.♻️ Proposed fix to map as an enum
- `@Column`({ type: "text", default: EventAudience.BOTH }) + `@Column`({ type: "enum", enum: EventAudience, default: EventAudience.BOTH }) audience: EventAudience;🤖 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 `@api/src/event/entities/event.entity.ts` around lines 107 - 109, Update the audience column in EventEntity to use the enum column type, supplying EventAudience through the enum option. Preserve EventAudience.BOTH as the default value and match the mapping pattern used by SocialAccountEntity.api/src/event/event.controller.ts (1)
178-204: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOptimize social account fetching to avoid duplicate queries.
this.userService.getUserWithSocialAccounts(userId)already retrieves the user's social accounts, including their 42 profile. Callingthis.eventService.isUserEligibleForEventAudience(event.audience, userId)immediately afterwards triggers a second database query for the exact same social account.Consider updating
isUserEligibleForEventAudienceto accept an optionalfortyTwoAccountargument (or evaluating the boolean flags directly in the controller) to eliminate the redundant database hit.🤖 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 `@api/src/event/event.controller.ts` around lines 178 - 204, Avoid the duplicate social-account query in the event join flow: update eventService.isUserEligibleForEventAudience to accept and reuse the already-resolved fortyTwoAccount from getUserWithSocialAccounts, or evaluate the audience eligibility directly in the controller. Preserve the existing audience validation behavior while ensuring no second lookup occurs.
🤖 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 `@frontend/src/routes/events/`$id/my-team.tsx:
- Around line 194-215: Update the unregistered-user branch in the event page to
use the social account’s isPiscineStudent and isCursusStudent eligibility flags,
rather than merely checking whether fortyTwoAccountQuery.data exists. Show the
audience-mismatch message only when the user lacks the required flag for the
event audience; otherwise retain the message directing the eligible user to join
from the Info tab.
- Around line 171-175: Update the loading condition in the event team route to
include fortyTwoAccountQuery.isLoading alongside the existing pending checks, so
the account-linking UI is not rendered while the enabled query is fetching for
the first time.
---
Nitpick comments:
In `@api/src/event/entities/event.entity.ts`:
- Around line 107-109: Update the audience column in EventEntity to use the enum
column type, supplying EventAudience through the enum option. Preserve
EventAudience.BOTH as the default value and match the mapping pattern used by
SocialAccountEntity.
In `@api/src/event/event.controller.ts`:
- Around line 178-204: Avoid the duplicate social-account query in the event
join flow: update eventService.isUserEligibleForEventAudience to accept and
reuse the already-resolved fortyTwoAccount from getUserWithSocialAccounts, or
evaluate the audience eligibility directly in the controller. Preserve the
existing audience validation behavior while ensuring no second lookup occurs.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5d686e1-6d62-41a5-805b-6cf1868c8ae0
📒 Files selected for processing (17)
api/db/migrations/1784375696042-socialAccountCursusFlags.tsapi/db/migrations/1784375696043-eventAudience.tsapi/src/auth/auth.controller.tsapi/src/auth/fortytwo.strategy.tsapi/src/event/dtos/createEventDto.tsapi/src/event/dtos/updateEventSettingsDto.tsapi/src/event/entities/event.entity.tsapi/src/event/event.controller.tsapi/src/event/event.service.tsapi/src/user/entities/social-account.entity.tsapi/src/user/social-account.service.tsfrontend/src/app/actions/event.tsfrontend/src/app/actions/social-accounts.tsfrontend/src/components/event-navbar.tsxfrontend/src/lib/constants/event-audience.tsfrontend/src/routes/events/$id/my-team.tsxfrontend/src/routes/events/create.tsx
| if (!isRegisteredQuery.data) { | ||
| const audience = eventQuery.data.audience | ||
| const audienceLabel = | ||
| audience === EventAudience.PISCINE ? 'piscine' : '42cursus' | ||
|
|
||
| const description = | ||
| audience !== EventAudience.BOTH | ||
| ? fortyTwoAccountQuery.data | ||
| ? `This event is only open to ${audienceLabel} students, and your linked 42 intra account doesn't meet that requirement.` | ||
| : `This event is only open to ${audienceLabel} students. Link your 42 intra account in your profile settings to check your eligibility.` | ||
| : "You haven't joined this event yet. Head to the Info tab to join." | ||
|
|
||
| return ( | ||
| <main className="container mx-auto max-w-4xl px-4 py-8"> | ||
| <Alert> | ||
| <AlertTitle>Team unavailable</AlertTitle> | ||
| <AlertDescription>{description}</AlertDescription> | ||
| </Alert> | ||
| </main> | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix eligibility logic for unregistered users on private events.
For private events, isRegisteredQuery.data being false can simply mean the user hasn't joined the event yet, even if they are perfectly eligible. The current logic incorrectly assumes that if they aren't registered and fortyTwoAccountQuery.data exists, they must be ineligible, resulting in an erroneous "doesn't meet that requirement" message.
Verify the actual eligibility flags (isPiscineStudent / isCursusStudent) on the user's social account to distinguish between a truly ineligible user and an eligible user who just needs to click join.
🐛 Proposed fix
- const description =
- audience !== EventAudience.BOTH
- ? fortyTwoAccountQuery.data
- ? `This event is only open to ${audienceLabel} students, and your linked 42 intra account doesn't meet that requirement.`
- : `This event is only open to ${audienceLabel} students. Link your 42 intra account in your profile settings to check your eligibility.`
- : "You haven't joined this event yet. Head to the Info tab to join."
+ let description = "You haven't joined this event yet. Head to the Info tab to join."
+
+ if (audience !== EventAudience.BOTH) {
+ if (!fortyTwoAccountQuery.data) {
+ description = `This event is only open to ${audienceLabel} students. Link your 42 intra account in your profile settings to check your eligibility.`
+ } else {
+ const isEligible =
+ audience === EventAudience.PISCINE
+ ? fortyTwoAccountQuery.data.isPiscineStudent
+ : fortyTwoAccountQuery.data.isCursusStudent
+
+ if (!isEligible) {
+ description = `This event is only open to ${audienceLabel} students, and your linked 42 intra account doesn't meet that requirement.`
+ }
+ }
+ }📝 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 (!isRegisteredQuery.data) { | |
| const audience = eventQuery.data.audience | |
| const audienceLabel = | |
| audience === EventAudience.PISCINE ? 'piscine' : '42cursus' | |
| const description = | |
| audience !== EventAudience.BOTH | |
| ? fortyTwoAccountQuery.data | |
| ? `This event is only open to ${audienceLabel} students, and your linked 42 intra account doesn't meet that requirement.` | |
| : `This event is only open to ${audienceLabel} students. Link your 42 intra account in your profile settings to check your eligibility.` | |
| : "You haven't joined this event yet. Head to the Info tab to join." | |
| return ( | |
| <main className="container mx-auto max-w-4xl px-4 py-8"> | |
| <Alert> | |
| <AlertTitle>Team unavailable</AlertTitle> | |
| <AlertDescription>{description}</AlertDescription> | |
| </Alert> | |
| </main> | |
| ) | |
| } | |
| if (!isRegisteredQuery.data) { | |
| const audience = eventQuery.data.audience | |
| const audienceLabel = | |
| audience === EventAudience.PISCINE ? 'piscine' : '42cursus' | |
| let description = "You haven't joined this event yet. Head to the Info tab to join." | |
| if (audience !== EventAudience.BOTH) { | |
| if (!fortyTwoAccountQuery.data) { | |
| description = `This event is only open to ${audienceLabel} students. Link your 42 intra account in your profile settings to check your eligibility.` | |
| } else { | |
| const isEligible = | |
| audience === EventAudience.PISCINE | |
| ? fortyTwoAccountQuery.data.isPiscineStudent | |
| : fortyTwoAccountQuery.data.isCursusStudent | |
| if (!isEligible) { | |
| description = `This event is only open to ${audienceLabel} students, and your linked 42 intra account doesn't meet that requirement.` | |
| } | |
| } | |
| } | |
| return ( | |
| <main className="container mx-auto max-w-4xl px-4 py-8"> | |
| <Alert> | |
| <AlertTitle>Team unavailable</AlertTitle> | |
| <AlertDescription>{description}</AlertDescription> | |
| </Alert> | |
| </main> | |
| ) | |
| } |
🤖 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 `@frontend/src/routes/events/`$id/my-team.tsx around lines 194 - 215, Update
the unregistered-user branch in the event page to use the social account’s
isPiscineStudent and isCursusStudent eligibility flags, rather than merely
checking whether fortyTwoAccountQuery.data exists. Show the audience-mismatch
message only when the user lacks the required flag for the event audience;
otherwise retain the message directing the eligible user to join from the Info
tab.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
PaulicStudios
left a comment
PaulicStudios
left a comment
There was a problem hiding this comment.
also add the option to set an audience in the dashboard while the event is running
| { name: 'My Team', path: `/events/${eventId}/my-team` }, | ||
| ] | ||
|
|
||
| if (isUserRegistered) { | ||
| items.push({ name: 'My Team', path: `/events/${eventId}/my-team` }) | ||
|
|
There was a problem hiding this comment.
please revert, the isUserRegistered should be fixed instead so it returns true if an event is public && matches the audience
| export enum EventAudience { | ||
| PISCINE = 'PISCINE', | ||
| CURSUS = 'CURSUS', | ||
| BOTH = 'BOTH', |
There was a problem hiding this comment.
call it all in case there will be more options
|
Also update the Datenschutz page to include that we save if you are an active student or a pisciner |
| ? "This event is only open to piscine students." | ||
| : "This event is only open to 42cursus students.", |
There was a problem hiding this comment.
add here that if you think this is wrong please relink your account in case your status updated. Or as there are already students linked they have to link again to get their correct status
Summary by CodeRabbit
New Features
Bug Fixes