Skip to content

Commit 33ec2ea

Browse files
PhuocHoanPhuocHoan
committed
fix: resolve some hidden bugs
1 parent 1076d84 commit 33ec2ea

20 files changed

+488
-172
lines changed

‎apps/api/src/courses/courses.controller.ts‎

Lines changed: 24 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,8 @@ export class CoursesController {
6363
* Get enrolled courses for the current user (My Learning)
6464
*/
6565
@Get('enrolled')
66-
@UseGuards(JwtAuthGuard)
66+
@UseGuards(JwtAuthGuard, RolesGuard)
67+
@Roles(UserRole.STUDENT, UserRole.INSTRUCTOR)
6768
getEnrolledCourses(
6869
@CurrentUser() user: User,
6970
@Query('archived') archived?: string,
@@ -77,7 +78,7 @@ export class CoursesController {
7778

7879
@Get('instructor/my-courses')
7980
@UseGuards(JwtAuthGuard, RolesGuard)
80-
@Roles(UserRole.INSTRUCTOR, UserRole.ADMIN)
81+
@Roles(UserRole.INSTRUCTOR)
8182
getMyCourses(@CurrentUser() user: User): Promise<Course[]> {
8283
return this.coursesService.findInstructorCourses(user.id);
8384
}
@@ -121,7 +122,8 @@ export class CoursesController {
121122
}
122123

123124
@Post(':id/enroll')
124-
@UseGuards(JwtAuthGuard)
125+
@UseGuards(JwtAuthGuard, RolesGuard)
126+
@Roles(UserRole.STUDENT, UserRole.INSTRUCTOR)
125127
async enroll(
126128
@Param('id') id: string,
127129
@CurrentUser() user: User,
@@ -138,13 +140,15 @@ export class CoursesController {
138140
isEnrolled: boolean;
139141
isInstructor: boolean;
140142
isAdmin: boolean;
143+
hasAccess: boolean;
141144
progress: Enrollment | null;
142145
}> {
143146
const accessInfo = await this.coursesService.checkCourseAccess(user, id);
144147
return {
145148
isEnrolled: accessInfo.isEnrolled,
146149
isInstructor: accessInfo.isInstructor,
147150
isAdmin: accessInfo.isAdmin,
151+
hasAccess: accessInfo.hasAccess,
148152
progress: accessInfo.enrollment,
149153
};
150154
}
@@ -168,7 +172,8 @@ export class CoursesController {
168172
}
169173

170174
@Post(':id/lessons/:lessonId/complete')
171-
@UseGuards(JwtAuthGuard)
175+
@UseGuards(JwtAuthGuard, RolesGuard)
176+
@Roles(UserRole.STUDENT, UserRole.INSTRUCTOR)
172177
async completeLesson(
173178
@Param('id') courseId: string,
174179
@Param('lessonId') lessonId: string,
@@ -181,7 +186,8 @@ export class CoursesController {
181186
* Archive a course (hide from main list, preserve progress)
182187
*/
183188
@Patch(':id/archive')
184-
@UseGuards(JwtAuthGuard)
189+
@UseGuards(JwtAuthGuard, RolesGuard)
190+
@Roles(UserRole.STUDENT, UserRole.INSTRUCTOR)
185191
async archiveCourse(
186192
@Param('id') id: string,
187193
@CurrentUser() user: User,
@@ -194,7 +200,8 @@ export class CoursesController {
194200
* Unarchive a course (restore to main list)
195201
*/
196202
@Patch(':id/unarchive')
197-
@UseGuards(JwtAuthGuard)
203+
@UseGuards(JwtAuthGuard, RolesGuard)
204+
@Roles(UserRole.STUDENT, UserRole.INSTRUCTOR)
198205
async unarchiveCourse(
199206
@Param('id') id: string,
200207
@CurrentUser() user: User,
@@ -205,7 +212,7 @@ export class CoursesController {
205212

206213
@Post()
207214
@UseGuards(JwtAuthGuard, RolesGuard)
208-
@Roles(UserRole.INSTRUCTOR, UserRole.ADMIN)
215+
@Roles(UserRole.INSTRUCTOR)
209216
create(
210217
@Body() createCourseDto: CreateCourseDto,
211218
@CurrentUser() user: User,
@@ -215,7 +222,7 @@ export class CoursesController {
215222

216223
@Patch(':id')
217224
@UseGuards(JwtAuthGuard, RolesGuard)
218-
@Roles(UserRole.INSTRUCTOR, UserRole.ADMIN)
225+
@Roles(UserRole.INSTRUCTOR)
219226
update(
220227
@Param('id') id: string,
221228
@Body() updateCourseDto: UpdateCourseDto,
@@ -226,7 +233,7 @@ export class CoursesController {
226233

227234
@Delete(':id')
228235
@UseGuards(JwtAuthGuard, RolesGuard)
229-
@Roles(UserRole.INSTRUCTOR, UserRole.ADMIN)
236+
@Roles(UserRole.INSTRUCTOR)
230237
remove(@Param('id') id: string, @CurrentUser() user: User): Promise<void> {
231238
return this.coursesService.remove(id, user.id);
232239
}
@@ -235,7 +242,7 @@ export class CoursesController {
235242

236243
@Post(':id/sections')
237244
@UseGuards(JwtAuthGuard, RolesGuard)
238-
@Roles(UserRole.INSTRUCTOR, UserRole.ADMIN)
245+
@Roles(UserRole.INSTRUCTOR)
239246
createSection(
240247
@Param('id') courseId: string,
241248
@Body() dto: CreateSectionDto,
@@ -246,7 +253,7 @@ export class CoursesController {
246253

247254
@Delete('sections/:sectionId')
248255
@UseGuards(JwtAuthGuard, RolesGuard)
249-
@Roles(UserRole.INSTRUCTOR, UserRole.ADMIN)
256+
@Roles(UserRole.INSTRUCTOR)
250257
deleteSection(
251258
@Param('sectionId') sectionId: string,
252259
@CurrentUser() user: User,
@@ -256,7 +263,7 @@ export class CoursesController {
256263

257264
@Post('sections/:sectionId/lessons')
258265
@UseGuards(JwtAuthGuard, RolesGuard)
259-
@Roles(UserRole.INSTRUCTOR, UserRole.ADMIN)
266+
@Roles(UserRole.INSTRUCTOR)
260267
createLesson(
261268
@Param('sectionId') sectionId: string,
262269
@Body() dto: CreateLessonDto,
@@ -267,7 +274,7 @@ export class CoursesController {
267274

268275
@Patch('lessons/:lessonId')
269276
@UseGuards(JwtAuthGuard, RolesGuard)
270-
@Roles(UserRole.INSTRUCTOR, UserRole.ADMIN)
277+
@Roles(UserRole.INSTRUCTOR)
271278
updateLesson(
272279
@Param('lessonId') lessonId: string,
273280
@Body() dto: UpdateLessonDto,
@@ -278,7 +285,7 @@ export class CoursesController {
278285

279286
@Delete('lessons/:lessonId')
280287
@UseGuards(JwtAuthGuard, RolesGuard)
281-
@Roles(UserRole.INSTRUCTOR, UserRole.ADMIN)
288+
@Roles(UserRole.INSTRUCTOR)
282289
deleteLesson(
283290
@Param('lessonId') lessonId: string,
284291
@CurrentUser() user: User,
@@ -288,7 +295,7 @@ export class CoursesController {
288295

289296
@Post(':id/sections/reorder')
290297
@UseGuards(JwtAuthGuard, RolesGuard)
291-
@Roles(UserRole.INSTRUCTOR, UserRole.ADMIN)
298+
@Roles(UserRole.INSTRUCTOR)
292299
reorderSections(
293300
@Param('id') courseId: string,
294301
@Body('sectionIds') sectionIds: string[],
@@ -299,7 +306,7 @@ export class CoursesController {
299306

300307
@Post('sections/:sectionId/lessons/reorder')
301308
@UseGuards(JwtAuthGuard, RolesGuard)
302-
@Roles(UserRole.INSTRUCTOR, UserRole.ADMIN)
309+
@Roles(UserRole.INSTRUCTOR)
303310
reorderLessons(
304311
@Param('sectionId') sectionId: string,
305312
@Body('lessonIds') lessonIds: string[],
@@ -323,7 +330,7 @@ export class CoursesController {
323330

324331
@Post(':id/sections/:sectionId/generate-quiz-preview')
325332
@UseGuards(JwtAuthGuard, RolesGuard)
326-
@Roles(UserRole.INSTRUCTOR, UserRole.ADMIN)
333+
@Roles(UserRole.INSTRUCTOR)
327334
async generateQuiz(
328335
@Body() dto: GenerateQuizPreviewDto,
329336
): Promise<{ title: string; questions: GeneratedQuestion[] }> {

‎apps/api/src/courses/courses.service.spec.ts‎

Lines changed: 98 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -558,6 +558,7 @@ describe('CoursesService', () => {
558558
courseRepository.findOne.mockResolvedValue({
559559
id: 'course-1',
560560
instructorId: 'instructor-1',
561+
status: CourseStatus.PUBLISHED,
561562
sections: [],
562563
});
563564

@@ -568,14 +569,16 @@ describe('CoursesService', () => {
568569

569570
expect(result.isEnrolled).toBe(true);
570571
expect(result.isInstructor).toBe(false);
572+
expect(result.hasAccess).toBe(true);
571573
expect(result.enrollment).toEqual(enrollment);
572574
});
573575

574-
it('should return isEnrolled=true when user is the instructor', async () => {
576+
it('should return hasAccess=true and isEnrolled=false when user is the instructor', async () => {
575577
mockEnrollmentRepositoryValue.findOne.mockResolvedValue(null);
576578
courseRepository.findOne.mockResolvedValue({
577579
id: 'course-1',
578580
instructorId: 'instructor-1',
581+
status: CourseStatus.PUBLISHED,
579582
sections: [],
580583
});
581584

@@ -584,16 +587,18 @@ describe('CoursesService', () => {
584587
'course-1',
585588
);
586589

587-
expect(result.isEnrolled).toBe(true);
590+
expect(result.isEnrolled).toBe(false);
588591
expect(result.isInstructor).toBe(true);
592+
expect(result.hasAccess).toBe(true);
589593
expect(result.enrollment).toBeNull();
590594
});
591595

592-
it('should return isEnrolled=false when user is neither enrolled nor instructor', async () => {
596+
it('should return hasAccess=false when user is neither enrolled nor instructor', async () => {
593597
mockEnrollmentRepositoryValue.findOne.mockResolvedValue(null);
594598
courseRepository.findOne.mockResolvedValue({
595599
id: 'course-1',
596600
instructorId: 'instructor-1',
601+
status: CourseStatus.PUBLISHED,
597602
sections: [],
598603
});
599604

@@ -604,8 +609,48 @@ describe('CoursesService', () => {
604609

605610
expect(result.isEnrolled).toBe(false);
606611
expect(result.isInstructor).toBe(false);
612+
expect(result.hasAccess).toBe(false);
613+
expect(result.enrollment).toBeNull();
614+
});
615+
616+
it('should return hasAccess=true for admin on published course', async () => {
617+
mockEnrollmentRepositoryValue.findOne.mockResolvedValue(null);
618+
courseRepository.findOne.mockResolvedValue({
619+
id: 'course-1',
620+
instructorId: 'instructor-1',
621+
status: CourseStatus.PUBLISHED,
622+
sections: [],
623+
});
624+
625+
const result = await service.checkCourseAccess(
626+
{ id: 'admin-1', role: UserRole.ADMIN } as User,
627+
'course-1',
628+
);
629+
630+
expect(result.isEnrolled).toBe(false);
631+
expect(result.isInstructor).toBe(false);
632+
expect(result.isAdmin).toBe(true);
633+
expect(result.hasAccess).toBe(true);
607634
expect(result.enrollment).toBeNull();
608635
});
636+
637+
it('should return hasAccess=true for admin on draft course', async () => {
638+
mockEnrollmentRepositoryValue.findOne.mockResolvedValue(null);
639+
courseRepository.findOne.mockResolvedValue({
640+
id: 'course-1',
641+
instructorId: 'instructor-1',
642+
status: CourseStatus.DRAFT,
643+
sections: [],
644+
});
645+
646+
const result = await service.checkCourseAccess(
647+
{ id: 'admin-1', role: UserRole.ADMIN } as User,
648+
'course-1',
649+
);
650+
651+
expect(result.isAdmin).toBe(true);
652+
expect(result.hasAccess).toBe(true);
653+
});
609654
});
610655

611656
describe('completeLesson', () => {
@@ -809,6 +854,56 @@ describe('CoursesService', () => {
809854
expect(result.hasAccess).toBe(true);
810855
});
811856

857+
it('should return lesson for admin on published course', async () => {
858+
const lesson = {
859+
id: 'lesson-1',
860+
isFreePreview: false,
861+
section: {
862+
course: {
863+
id: 'course-1',
864+
instructorId: 'instructor-1',
865+
status: CourseStatus.PUBLISHED,
866+
},
867+
},
868+
};
869+
mockLessonRepositoryValue.findOne.mockResolvedValue(lesson);
870+
mockEnrollmentRepositoryValue.findOne.mockResolvedValue(null);
871+
872+
const result = await service.getLessonWithAccessControl(
873+
{ id: 'admin-1', role: UserRole.ADMIN } as User,
874+
'course-1',
875+
'lesson-1',
876+
);
877+
878+
expect(result.lesson).toEqual(lesson);
879+
expect(result.hasAccess).toBe(true);
880+
});
881+
882+
it('should return lesson for admin on draft course', async () => {
883+
const lesson = {
884+
id: 'lesson-1',
885+
isFreePreview: false,
886+
section: {
887+
course: {
888+
id: 'course-1',
889+
instructorId: 'instructor-1',
890+
status: CourseStatus.DRAFT,
891+
},
892+
},
893+
};
894+
mockLessonRepositoryValue.findOne.mockResolvedValue(lesson);
895+
mockEnrollmentRepositoryValue.findOne.mockResolvedValue(null);
896+
897+
const result = await service.getLessonWithAccessControl(
898+
{ id: 'admin-1', role: UserRole.ADMIN } as User,
899+
'course-1',
900+
'lesson-1',
901+
);
902+
903+
expect(result.lesson).toEqual(lesson);
904+
expect(result.hasAccess).toBe(true);
905+
});
906+
812907
it('should throw NotFoundException if lesson does not belong to course', async () => {
813908
const lesson = {
814909
id: 'lesson-1',

‎apps/api/src/courses/courses.service.ts‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,7 @@ export class CoursesService {
249249
isEnrolled: boolean;
250250
isInstructor: boolean;
251251
isAdmin: boolean;
252+
hasAccess: boolean;
252253
enrollment: Enrollment | null;
253254
}> {
254255
const userId = user.id;
@@ -264,9 +265,11 @@ export class CoursesService {
264265
const isInstructor = course?.instructorId === userId;
265266
const isAdmin = user.role === UserRole.ADMIN;
266267
const isPending = course?.status === CourseStatus.PENDING;
268+
const isPublished = course?.status === CourseStatus.PUBLISHED;
269+
const isDraft = course?.status === CourseStatus.DRAFT;
267270

268-
// Admin has access to all courses, especially during review (PENDING)
269-
const hasAdminAccess = isAdmin && isPending;
271+
// Admin has read-only audit access during DRAFT/PENDING and after approval (PUBLISHED)
272+
const hasAdminAccess = isAdmin && (isDraft || isPending || isPublished);
270273

271274
// Sanitize enrollment data to only include existing lessons
272275
if (enrollment) {
@@ -285,9 +288,10 @@ export class CoursesService {
285288
}
286289

287290
return {
288-
isEnrolled: Boolean(enrollment) || isInstructor || hasAdminAccess,
291+
isEnrolled: Boolean(enrollment),
289292
isInstructor,
290293
isAdmin,
294+
hasAccess: Boolean(enrollment) || isInstructor || hasAdminAccess,
291295
enrollment,
292296
};
293297
}
@@ -369,12 +373,14 @@ export class CoursesService {
369373
// Check access: enrolled OR free preview OR is instructor OR is admin reviewing
370374
const isInstructor = lesson.section.course.instructorId === userId;
371375
const isAdmin = user.role === UserRole.ADMIN;
376+
const isDraft = lesson.section.course.status === CourseStatus.DRAFT;
372377
const isPending = lesson.section.course.status === CourseStatus.PENDING;
378+
const isPublished = lesson.section.course.status === CourseStatus.PUBLISHED;
373379
const hasAccess =
374380
isEnrolled ||
375381
lesson.isFreePreview ||
376382
isInstructor ||
377-
(isAdmin && isPending);
383+
(isAdmin && (isDraft || isPending || isPublished));
378384

379385
if (!hasAccess) {
380386
throw new ForbiddenException(

0 commit comments

Comments
 (0)