-
-
Notifications
You must be signed in to change notification settings - Fork 221
fix: remove code from @common/ #2901
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
018fe02 to
5b3f504
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR removes shared code from the @common/ directory and relocates it to appropriate locations within the server, NestJS, and client codebases. This refactoring eliminates cross-boundary dependencies and improves code organization by moving types and enums closer to where they are used.
Key changes:
- Migrated
PreferredStudentsLocationenum from@common/enums/mentortoserver/src/models/mentorRegistry.ts - Moved gratitude-related interfaces from
@common/interfaces/gratitudetoclient/src/services/gratitude.tsandserver/src/repositories/feedback.repository.ts - Created new
ExpelledStatsDtoin NestJS and migrated client code to use it instead of the common model - Moved
IPaginationInfotype from@common/types/paginationtoclient/src/utils/pagination.ts - Extracted and standardized
UserDtoin NestJS for consistent user representation across APIs
Reviewed changes
Copilot reviewed 36 out of 37 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/routes/feedback.ts | Updated to use local GetGratitudeQuery type instead of common interface |
| server/src/routes/course/mentor.ts | Updated import path for PreferredStudentsLocation to server models |
| server/src/repositories/feedback.repository.ts | Added GetGratitudeQuery type definition locally |
| server/src/models/mentorRegistry.ts | Added PreferredStudentsLocation enum and updated column type |
| server/src/models/index.ts | Exported PreferredStudentsLocation enum and changed ExternalAccount to type export |
| nestjs/src/users/dto/user.dto.ts | Created new UserDto for consistent user representation |
| nestjs/src/users/dto/index.ts | Added export for new UserDto |
| nestjs/src/user-groups/dto/user-group.dto.ts | Removed duplicate UserDto definition and imported from users module |
| nestjs/src/spec.json | Updated OpenAPI spec with new DTO definitions and schema references |
| nestjs/src/courses/stats/dto/expelled-stats.dto.ts | Created ExpelledStatsDto to replace common model |
| nestjs/src/courses/stats/course-stats.controller.ts | Updated to return ExpelledStatsDto with proper transformation |
| nestjs/src/courses/mentors/mentors.service.ts | Updated import path for PreferredStudentsLocation |
| nestjs/src/courses/mentors/dto/mentor-options.dto.ts | Updated import path for PreferredStudentsLocation |
| nestjs/src/courses/course-mentors/dto/mentor-details.dto.ts | Updated imports to use UserDto from users module and PreferredStudentsLocation from entities |
| common/models/index.ts | Removed export of deleted expelled-stats model |
| common/models/expelled-stats.ts | Deleted - functionality moved to NestJS DTO |
| common/interfaces/gratitude/index.ts | Deleted - interfaces moved to client and server |
| common/enums/mentor/index.ts | Deleted - enum moved to server model |
| client/src/utils/pagination.ts | Created IPaginationInfo type for client-side pagination |
| client/src/services/mentorRegistry.ts | Updated to use API-generated enum type |
| client/src/services/gratitude.ts | Added gratitude interface definitions locally |
| client/src/services/course.ts | Updated imports and type references |
| client/src/modules/Teams/components/* | Updated imports to use new pagination path |
| client/src/modules/Students/Pages/Students.tsx | Updated pagination import path |
| client/src/modules/Score/* | Updated pagination import paths |
| client/src/modules/MentorTasksReview/pages/MentorTasksReview.tsx | Updated pagination import path |
| client/src/modules/Mentor/pages/Interviews/components/MentorPreferencesModal.tsx | Updated to use API-generated enum type |
| client/src/modules/Interviews/pages/StageInterviewFeedback/feedbackTemplateHandler.test.ts | Reorganized imports |
| client/src/modules/CrossCheckPairs/pages/CrossCheckPairs/CrossCheckPairs.tsx | Updated pagination import path |
| client/src/modules/CourseManagement/hooks/useExpelledStats.ts | Refactored to use API client instead of fetch |
| client/src/modules/CourseManagement/components/ExpelledStudentsStats.tsx | Updated to use ExpelledStatsDto from API |
| client/src/components/MentorOptions.tsx | Updated to use API-generated enum type |
| client/src/components/Heroes/HeroesRadarTab.tsx | Updated pagination import path |
| client/src/components/Forms/Heroes/index.tsx | Updated to import types from gratitude service |
| client/src/api/api.ts | Added ExpelledStatsDto and UserDto type definitions with updated API signatures |
Comments suppressed due to low confidence (1)
client/src/modules/CourseManagement/hooks/useExpelledStats.ts:44
- After deleting an expelled stat, the data is not refreshed. The old implementation called
refresh()after successful deletion, but the new code removed this. Therefreshfunction should be destructured fromuseRequesthook and called after successful deletion in thehandleDeletefunction to update the list.
const handleDelete = async (id: string) => {
setIsDeleting(true);
try {
await api.deleteExpelledStat(id);
} catch (err) {
console.error('Error deleting stat:', err);
} finally {
setIsDeleting(false);
}
};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @ApiProperty() | ||
| @IsNumber() | ||
| course: CourseDto; |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validator decorator for the course property is incorrect. The property is of type CourseDto, which is a complex object, but it's decorated with @IsNumber(). This should be removed or replaced with @ValidateNested() along with @Type(() => CourseDto) from class-transformer to properly validate the nested object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed please check
| export interface DetailedExpelledStat { | ||
| id: string; | ||
| course: { | ||
| id: number; | ||
| name: string; | ||
| fullName: string; | ||
| alias: string; | ||
| description: string; | ||
| logo: string; | ||
| }; | ||
| user: { | ||
| id: number; | ||
| githubId: string; | ||
| }; | ||
| reasonForLeaving?: string[]; | ||
| otherComment: string; | ||
| submittedAt: Date; | ||
| } | ||
|
|
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DetailedExpelledStat interface is defined but never used in this file. The hook returns ExpelledStatsDto[] and the component that uses this hook has been updated to use ExpelledStatsDto directly. This interface should be removed to eliminate dead code.
| export interface DetailedExpelledStat { | |
| id: string; | |
| course: { | |
| id: number; | |
| name: string; | |
| fullName: string; | |
| alias: string; | |
| description: string; | |
| logo: string; | |
| }; | |
| user: { | |
| id: number; | |
| githubId: string; | |
| }; | |
| reasonForLeaving?: string[]; | |
| otherComment: string; | |
| submittedAt: Date; | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please re-check
| id: data.user.id, | ||
| githubId: data.user.githubId, | ||
| name: UsersService.getFullName(data.user), | ||
| }); |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constructor is missing assignments for reasonForLeaving, otherComment, and submittedAt properties. These properties are defined in the DTO but are never populated from the CourseLeaveSurveyResponse data. Add the following assignments in the constructor:
this.reasonForLeaving = data.reasonForLeaving;this.otherComment = data.otherComment;this.submittedAt = data.submittedAt;
| }); | |
| }); | |
| this.reasonForLeaving = data.reasonForLeaving; | |
| this.otherComment = data.otherComment; | |
| this.submittedAt = data.submittedAt; |
69fac03 to
4a4ac14
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 36 out of 37 changed files in this pull request and generated 3 comments.
|
|
||
| export const useExpelledStats = () => { | ||
| const { data, error, loading, refresh } = useRequest(fetchExpelledStats); | ||
| const { data, error, loading } = useRequest(fetchExpelledStats); |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The refresh function returned by useRequest is no longer being destructured and returned from this hook. This means that after a successful delete operation, the list of expelled stats won't automatically refresh to reflect the deletion. You should destructure refresh from the useRequest call on line 13 and include it in the return object on line 27.
| const handleDelete = async (id: string) => { | ||
| setIsDeleting(true); | ||
| try { | ||
| const response = await fetch(`/api/v2/courses/stats/expelled/${id}`, { | ||
| method: 'DELETE', | ||
| }); | ||
| if (!response.ok) { | ||
| throw new Error('Failed to delete stat'); | ||
| } | ||
| if (typeof refresh === 'function') { | ||
| refresh(); | ||
| } | ||
| await api.deleteExpelledStat(id); |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After successfully deleting a stat, the refresh function should be called to update the list and reflect the deletion. The previous implementation called refresh() after a successful delete, but this has been removed. This should be restored by destructuring refresh from useRequest and calling it after the successful API call.
| export * from './interview'; | ||
| export * from './profile'; | ||
| export * from './expelled-stats'; | ||
| export * from './interview'; |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a duplicate export statement for './interview' on line 5 (also exported on line 3). This duplicate should be removed.
| export * from './interview'; |
Pull Request Guidelines
Issue:
N/A
Description:
I used LLM Agent and #2900 to make this change
Self-Check: