Achivement endpoint update #47
Conversation
WalkthroughAdds an optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant AchievementService as Service
participant DB
Client->>Service: getAchievements(params)
Note right of Service #E8F8F5: Build query with includes:\n- members (id,name,email,profilePhoto)\n- createdBy (id,name)\n- updatedBy (id,name)
Service->>DB: SELECT ... WITH related includes
DB-->>Service: achievements + included relations
Service-->>Client: achievements[] (with members, createdBy, updatedBy)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (1)
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
prisma/migrations/20251016191132_birth_date_added_to_member/migration.sql(1 hunks)prisma/schema.prisma(1 hunks)src/services/achievement.service.ts(1 hunks)
🔇 Additional comments (1)
src/services/achievement.service.ts (1)
5-32: Good optimization with eager loading.The addition of
includeto eagerly load related entities (members,createdBy,updatedBy) is a good optimization that prevents N+1 query issues and provides richer data for the API response. The nested structure for members is correct sinceMemberAchievementis a join table.Note: The newly added
birth_datefield from the Member model is not included in the select clause. If this field should be exposed through the achievement endpoint, add it to the member select at line 14:member: { select: { id: true, name: true, email: true, profilePhoto: true, + birth_date: true, }, },The same change would need to be applied to
getAchievementByIdat line 75 for consistency.
| -- AlterTable | ||
| ALTER TABLE "Member" ADD COLUMN "birth_date" TEXT; |
There was a problem hiding this comment.
Use a proper date type instead of TEXT.
The migration adds birth_date as TEXT, which matches the String type in the Prisma schema but is not ideal for storing dates. After updating the schema to use DateTime (as suggested in prisma/schema.prisma), regenerate this migration to use a proper date type.
The updated migration should use either DATE or TIMESTAMP:
- ALTER TABLE "Member" ADD COLUMN "birth_date" TEXT;
+ ALTER TABLE "Member" ADD COLUMN "birth_date" DATE;Use DATE if you only need to store the date (YYYY-MM-DD) without time information, or TIMESTAMP if you need date and time.
📝 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.
| -- AlterTable | |
| ALTER TABLE "Member" ADD COLUMN "birth_date" TEXT; | |
| -- AlterTable | |
| ALTER TABLE "Member" ADD COLUMN "birth_date" DATE; |
🤖 Prompt for AI Agents
In prisma/migrations/20251016191132_birth_date_added_to_member/migration.sql
lines 1–2, the new "birth_date" column is created as TEXT; update the migration
to use an appropriate date/time type (e.g., ALTER TABLE "Member" ADD COLUMN
"birth_date" DATE; or ADD COLUMN "birth_date" TIMESTAMP; depending on whether
you need time) that matches the Prisma schema DateTime, then regenerate or
replace the migration SQL so it uses DATE or TIMESTAMP for your database dialect
(Postgres/MySQL) and re-run prisma migrate so the schema and migration are
consistent.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Achievement.test.ts (1)
42-62: Replaceanytest doubles with typed helpers to satisfy no-explicit-anyEvery
const req/res: any = …(same pattern repeats throughout the file) violates@typescript-eslint/no-explicit-any, blocking lint. Please introduce lightweight typed builders—e.g., importtype Request, Responsefromexpressand cast viaas unknown as Request/Response, or wrap them in helper functions returning those types—so tests stay type-safe without relying onany.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/Achievement.test.ts(15 hunks)tests/Member.test.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/Member.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Achievement.test.ts (4)
src/utils/imageUtils.ts (2)
uploadImage(23-70)deleteImage(72-85)src/services/achievement.service.ts (5)
createAchievement(40-60)getAchievementById(63-95)updateAchievementById(98-116)deleteAchievementById(132-136)removeMemberFromAchievement(139-148)src/controllers/achievement.controller.ts (5)
createAchievement(37-71)getAchievementById(18-35)updateAchievementById(74-134)deleteAchievementById(137-159)removeMemberFromAchievement(162-180)src/utils/apiError.ts (1)
ApiError(6-17)
🪛 ESLint
tests/Achievement.test.ts
[error] 29-29: A require() style import is forbidden.
(@typescript-eslint/no-require-imports)
[error] 42-42: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 94-94: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 112-112: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 128-128: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 134-134: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 150-150: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 219-219: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 279-279: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 291-291: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 297-297: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 338-338: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 372-372: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 401-401: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 424-424: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 452-452: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 465-465: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 481-481: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 501-501: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 518-518: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 535-535: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 588-588: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 601-601: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 630-630: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 637-637: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 645-645: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 652-652: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
| return { | ||
| __esModule: true, | ||
| default: jest.fn(() => require('express').Router()), | ||
| default: jest.fn(() => require("express").Router()), | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Fix ESLint no-require-imports violation in achievements route mock
require("express") trips the project’s @typescript-eslint/no-require-imports rule, so the suite fails linting. Import Router (or express) once at the top and use it in the mock factory instead of require to keep the mock functional without breaking the rule.
🧰 Tools
🪛 ESLint
[error] 29-29: A require() style import is forbidden.
(@typescript-eslint/no-require-imports)
🤖 Prompt for AI Agents
In tests/Achievement.test.ts around lines 27 to 31, the mock uses
require("express") which violates @typescript-eslint/no-require-imports; fix by
importing Router (or express) at the top of the test file (e.g., import { Router
} from "express" or import express) and replace the require call in the
jest.mock factory with the imported Router/express reference so the mock no
longer uses require.
Summary by CodeRabbit
New Features
Chores
Tests