Create Cohortmember added validation if Admin role is there donot che…#746
Conversation
…ck Application endDate
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Review limit reached
More reviews will be available in 22 minutes and 3 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughAdds admin-role checking to cohort-member creation so application end-date validation is skipped for admins, and refactors referral slug utilities to add a user-provided-slug validator plus corresponding service updates to validate trimmed explicit slugs and enforce uniqueness. ChangesAdmin bypass for application end-date validation
Referral slug validation and normalization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (1)
src/adapters/postgres/cohortMembers-adapter.ts (1)
783-786: ⚡ Quick winAvoid querying admin roles when end-date validation is disabled.
isUserAdmin()runs on every create request even whenAPPLICATION_END_FIELD_IDis unset, so this adds an unnecessary DB hit on the hot path and makes the request depend on role tables for a branch that should be skipped.♻️ Proposed change
// Validate Application End Date before creating cohort member (only if configured and not admin) const applicationEndDateFieldId = process.env.APPLICATION_END_FIELD_ID; - const callerIsAdmin = await this.isUserAdmin(loginUser, tenantId); - - if (applicationEndDateFieldId && !callerIsAdmin) { + if (applicationEndDateFieldId) { + const callerIsAdmin = await this.isUserAdmin(loginUser, tenantId); + if (callerIsAdmin) { + cohortMembers.createdBy = loginUser; + cohortMembers.updatedBy = loginUser; + cohortMembers.cohortAcademicYearId = cohortacAdemicyearId; + const savedCohortMember = await this.cohortMembersRepository.save( + cohortMembers + ); + // existing Elasticsearch update + response flow + } + + if (!callerIsAdmin) { const cohortId = cohortMembers.cohortId; // Query FieldValues table for the Application End Date field const applicationEndDateField = await this.fieldValuesRepository.findOne({ where: { @@ } } } + }As per coding guidelines "Ensure that: - The code adheres to best practices recommended for performance."
🤖 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 `@src/adapters/postgres/cohortMembers-adapter.ts` around lines 783 - 786, The code eagerly calls isUserAdmin(loginUser, tenantId) even when APPLICATION_END_FIELD_ID is unset; change the flow so applicationEndDateFieldId is checked first and only then call await this.isUserAdmin(loginUser, tenantId). Concretely, remove the top-level callerIsAdmin assignment, evaluate if (applicationEndDateFieldId) { const callerIsAdmin = await this.isUserAdmin(loginUser, tenantId); if (!callerIsAdmin) { ... } } and ensure any later use of callerIsAdmin is scoped or defaulted accordingly.
🤖 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.
Nitpick comments:
In `@src/adapters/postgres/cohortMembers-adapter.ts`:
- Around line 783-786: The code eagerly calls isUserAdmin(loginUser, tenantId)
even when APPLICATION_END_FIELD_ID is unset; change the flow so
applicationEndDateFieldId is checked first and only then call await
this.isUserAdmin(loginUser, tenantId). Concretely, remove the top-level
callerIsAdmin assignment, evaluate if (applicationEndDateFieldId) { const
callerIsAdmin = await this.isUserAdmin(loginUser, tenantId); if (!callerIsAdmin)
{ ... } } and ensure any later use of callerIsAdmin is scoped or defaulted
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 82916d9a-9c61-4aa7-857b-6e4e91bfba90
📒 Files selected for processing (1)
src/adapters/postgres/cohortMembers-adapter.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/referrals/referrals.service.ts (1)
68-77: 💤 Low valueConsider extracting duplicate validation error message.
The same error message appears in both
createReferralEntity(line 71) andupdateSlug(line 252). Extract to a constant for consistency and easier maintenance.Proposed refactor
// At top of file or in a constants file const INVALID_SLUG_ERROR = "Slug may only contain letters, digits, hyphens (-), underscores (_), dots (.) and tildes (~)";Then use
throw new BadRequestException(INVALID_SLUG_ERROR);in both locations.Also applies to: 249-257
🤖 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 `@src/referrals/referrals.service.ts` around lines 68 - 77, Extract the duplicated validation message into a single constant (e.g., INVALID_SLUG_ERROR) and replace the literal string usages in createReferralEntity and updateSlug with that constant; define the constant near the top of the file or in a shared constants module, then change both throw new BadRequestException('...') occurrences to throw new BadRequestException(INVALID_SLUG_ERROR) to ensure consistency and easier maintenance (also update any import/exports if you move the constant to a shared file).
🤖 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 `@src/referrals/utils/referral-slug.util.ts`:
- Around line 43-46: The comment is misleading because standardizeSlugInput now
only trims instead of performing the previous normalization; either restore the
original normalization (lowercase + strip special chars) inside
standardizeSlugInput or rename it to trimSlugInput and update all callers
(notably resolveSlug) to use the correct normalization helper; ensure
resolveSlug still lowercases/strips when performing lookups or call a new
normalizeSlug function, and update tests/usage sites to reflect the
renamed/changed behavior so existing slug lookups continue to work.
- Around line 25-30: The DTO promises slugs are normalized to lowercase a-z0-9_,
so update validation and storage to match: change isValidUserProvidedSlug to
validate only lowercase a-z, digits and underscore (e.g., /^[a-z0-9_]+$/) and,
in the service that currently stores trimmedSlug, call trimmedSlug.toLowerCase()
before uniqueness checks and persistence so slugs are normalized and
case-insensitive; alternatively, if you prefer preserving case, update the DTO
docs in CreateReferralEntityDto and UpdateReferralSlugDto to list the actual
allowed characters and behavior instead.
---
Nitpick comments:
In `@src/referrals/referrals.service.ts`:
- Around line 68-77: Extract the duplicated validation message into a single
constant (e.g., INVALID_SLUG_ERROR) and replace the literal string usages in
createReferralEntity and updateSlug with that constant; define the constant near
the top of the file or in a shared constants module, then change both throw new
BadRequestException('...') occurrences to throw new
BadRequestException(INVALID_SLUG_ERROR) to ensure consistency and easier
maintenance (also update any import/exports if you move the constant to a shared
file).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aae20a31-cc97-4adb-8a9b-60bdf1421fc3
📒 Files selected for processing (2)
src/referrals/referrals.service.tssrc/referrals/utils/referral-slug.util.ts
|
2 participants
…ck Application endDate
Summary by CodeRabbit