diff --git a/src/web/app/pages-instructor/instructor-search-page/__snapshots__/instructor-search-page.component.spec.ts.snap b/src/web/app/pages-instructor/instructor-search-page/__snapshots__/instructor-search-page.component.spec.ts.snap index ebcb5be28b0..73bb1180294 100644 --- a/src/web/app/pages-instructor/instructor-search-page/__snapshots__/instructor-search-page.component.spec.ts.snap +++ b/src/web/app/pages-instructor/instructor-search-page/__snapshots__/instructor-search-page.component.spec.ts.snap @@ -5,6 +5,7 @@ exports[`InstructorSearchPageComponent > should snap with a search key 1`] = ` courseService={[Function Object]} instructorService={[Function Object]} isSearching="false" + privilegeCache={[Function Map]} searchParams={[Function Object]} searchService={[Function _SearchService]} searchString="" @@ -61,6 +62,7 @@ exports[`InstructorSearchPageComponent > should snap with a student table 1`] = courseService={[Function Object]} instructorService={[Function Object]} isSearching="false" + privilegeCache={[Function Map]} searchParams={[Function Object]} searchService={[Function _SearchService]} searchString="" @@ -520,6 +522,7 @@ exports[`InstructorSearchPageComponent > should snap with default fields 1`] = ` courseService={[Function Object]} instructorService={[Function Object]} isSearching="false" + privilegeCache={[Function Map]} searchParams={[Function Object]} searchService={[Function _SearchService]} searchString="" diff --git a/src/web/app/pages-instructor/instructor-search-page/instructor-search-page.component.spec.ts b/src/web/app/pages-instructor/instructor-search-page/instructor-search-page.component.spec.ts index 658ac2fcc71..24f9aa35608 100644 --- a/src/web/app/pages-instructor/instructor-search-page/instructor-search-page.component.spec.ts +++ b/src/web/app/pages-instructor/instructor-search-page/instructor-search-page.component.spec.ts @@ -278,12 +278,23 @@ describe('InstructorSearchPageComponent', () => { ).toEqual(students.filter((s: Student) => s.sectionName === students[0].sectionName).length); }); - it('should execute GET when fetching privileges', () => { - component.getPrivileges(coursesWithStudents); + it('should call loadInstructorPrivilege once per unique courseId', () => { + component.getPrivileges(coursesWithStudents).subscribe(() => { + // 2 unique courses: CS3281 and CS3282 + expect(mockInstructorService.loadInstructorPrivilege).toHaveBeenCalledTimes(2); + expect(mockInstructorService.loadInstructorPrivilege).toHaveBeenCalledWith({ courseId: 'CS3281' }); + expect(mockInstructorService.loadInstructorPrivilege).toHaveBeenCalledWith({ courseId: 'CS3282' }); + }); + }); + + it('should use cached privileges on subsequent calls', () => { + component.getPrivileges(coursesWithStudents).subscribe(() => { + mockInstructorService.loadInstructorPrivilege.mockClear(); - for (const course of coursesWithStudents) { - expect(mockInstructorService.loadInstructorPrivilege).toHaveBeenCalledWith({ courseId: course.courseId }); - } + component.getPrivileges(coursesWithStudents).subscribe(() => { + expect(mockInstructorService.loadInstructorPrivilege).not.toHaveBeenCalled(); + }); + }); }); it('should combine privileges and course data correctly', () => { @@ -295,59 +306,39 @@ describe('InstructorSearchPageComponent', () => { canViewSession: true, canSubmitSession: true, }; - const mockPrivilegesArray: InstructorPrivilege[] = [ - { - privileges: { - courseLevel: basePrivilege, - sectionLevel: {}, - sessionLevel: {}, - }, - }, - { - privileges: { - courseLevel: { - ...basePrivilege, - canModifyStudent: true, + const privilegeMap: Map = new Map([ + [ + 'CS3281', + { + privileges: { + courseLevel: basePrivilege, + sectionLevel: {}, + sessionLevel: {}, }, - sectionLevel: {}, - sessionLevel: {}, }, - }, - { - privileges: { - courseLevel: { - ...basePrivilege, - canModifyStudent: false, - }, - sectionLevel: {}, - sessionLevel: {}, - }, - }, - { - privileges: { - courseLevel: { - ...basePrivilege, - canModifyStudent: false, + ], + [ + 'CS3282', + { + privileges: { + courseLevel: { + ...basePrivilege, + canModifyStudent: false, + }, + sectionLevel: {}, + sessionLevel: {}, }, - sectionLevel: {}, - sessionLevel: {}, }, - }, - ]; - component.combinePrivileges([coursesWithStudents, mockPrivilegesArray]); - - const course1Student1: StudentListRowModel = coursesWithStudents[0].students[0]; - expect(course1Student1.isAllowedToModifyStudent).toEqual(true); - - const course1Student2: StudentListRowModel = coursesWithStudents[0].students[1]; - expect(course1Student2.isAllowedToModifyStudent).toEqual(true); - - const course1Student3: StudentListRowModel = coursesWithStudents[0].students[2]; - expect(course1Student3.isAllowedToModifyStudent).toEqual(false); + ], + ]); + component.combinePrivileges(coursesWithStudents, privilegeMap); - const course2Student1: StudentListRowModel = coursesWithStudents[1].students[0]; - expect(course2Student1.isAllowedToModifyStudent).toEqual(false); + // CS3281 has courseLevel canModifyStudent: true + expect(coursesWithStudents[0].students[0].isAllowedToModifyStudent).toEqual(true); + expect(coursesWithStudents[0].students[1].isAllowedToModifyStudent).toEqual(true); + expect(coursesWithStudents[0].students[2].isAllowedToModifyStudent).toEqual(true); - expect(mockPrivilegesArray.length).toEqual(0); + // CS3282 has courseLevel canModifyStudent: false + expect(coursesWithStudents[1].students[0].isAllowedToModifyStudent).toEqual(false); }); }); diff --git a/src/web/app/pages-instructor/instructor-search-page/instructor-search-page.component.ts b/src/web/app/pages-instructor/instructor-search-page/instructor-search-page.component.ts index 7edbbd8e8d7..1e5585d265a 100644 --- a/src/web/app/pages-instructor/instructor-search-page/instructor-search-page.component.ts +++ b/src/web/app/pages-instructor/instructor-search-page/instructor-search-page.component.ts @@ -32,6 +32,7 @@ export class InstructorSearchPageComponent implements OnInit { private courseService = inject(CourseService); private studentService = inject(StudentService); private visibleCourseIds$: Observable = of([]); + private privilegeCache: Map = new Map(); searchParams: SearchParams = { searchKey: '', @@ -58,16 +59,18 @@ export class InstructorSearchPageComponent implements OnInit { mergeMap((courseIds: string[]) => this.searchService.searchInstructor(this.searchParams.searchKey, courseIds)), map((res: InstructorSearchResult) => this.getCoursesWithStudents(res.students)), mergeMap((coursesWithStudents: SearchStudentsListRowTable[]) => - forkJoin([of(coursesWithStudents), this.getPrivileges(coursesWithStudents)]), + this.getPrivileges(coursesWithStudents).pipe( + map((privilegeMap: Map) => + this.combinePrivileges(coursesWithStudents, privilegeMap), + ), + ), ), - map((res: [SearchStudentsListRowTable[], InstructorPrivilege[]]) => this.combinePrivileges(res)), finalize(() => { this.isSearching = false; }), ) .subscribe({ - next: (resp: TransformedInstructorSearchResult) => { - const searchStudentsTable: SearchStudentsListRowTable[] = resp.searchStudentTables; + next: (searchStudentsTable: SearchStudentsListRowTable[]) => { const hasStudents = !!searchStudentsTable?.length; if (hasStudents) { @@ -105,51 +108,48 @@ export class InstructorSearchPageComponent implements OnInit { return coursesWithStudents; } - getPrivileges(coursesWithStudents: SearchStudentsListRowTable[]): Observable { - if (coursesWithStudents.length === 0) { - return of([]); + getPrivileges(coursesWithStudents: SearchStudentsListRowTable[]): Observable> { + const courseIds: string[] = Array.from( + new Set(coursesWithStudents.map((c: SearchStudentsListRowTable) => c.courseId)), + ); + const uncachedIds: string[] = courseIds.filter((id: string) => !this.privilegeCache.has(id)); + + if (uncachedIds.length === 0) { + return of(new Map(this.privilegeCache)); } - const privileges: Observable[] = []; - coursesWithStudents.forEach((course: SearchStudentsListRowTable) => { - const sectionToPrivileges: Record> = {}; - Array.from( - new Set(course.students.map((studentModel: StudentListRowModel) => studentModel.student.sectionName)), - ).forEach((section: string) => { - sectionToPrivileges[section] = this.instructorService.loadInstructorPrivilege({ courseId: course.courseId }); - }); - course.students.forEach((studentModel: StudentListRowModel) => - privileges.push(sectionToPrivileges[studentModel.student.sectionName]), - ); + + const requests: Record> = {}; + uncachedIds.forEach((id: string) => { + requests[id] = this.instructorService.loadInstructorPrivilege({ courseId: id }); }); - return forkJoin(privileges); + + return forkJoin(requests).pipe( + map((results: Record) => { + Object.entries(results).forEach(([courseId, privilege]: [string, InstructorPrivilege]) => { + this.privilegeCache.set(courseId, privilege); + }); + return new Map(this.privilegeCache); + }), + ); } - combinePrivileges([coursesWithStudents, privileges]: [ - SearchStudentsListRowTable[], - InstructorPrivilege[], - ]): TransformedInstructorSearchResult { - /** - * Pop the privilege objects one at a time and attach them to the results. This is possible - * because `forkJoin` guarantees that the `InstructorPrivilege` results are returned in the - * same order the requests were made. - */ + combinePrivileges( + coursesWithStudents: SearchStudentsListRowTable[], + privilegeMap: Map, + ): SearchStudentsListRowTable[] { for (const course of coursesWithStudents) { + const privilege: InstructorPrivilege | undefined = privilegeMap.get(course.courseId); + if (!privilege) { + continue; + } + const courseLevel: InstructorPermissionSet = privilege.privileges.courseLevel; for (const studentModel of course.students) { - const privilege: InstructorPrivilege | undefined = privileges.shift(); - if (!privilege) { - continue; - } const sectionId: string = studentModel.student.sectionId; - const courseLevel: InstructorPermissionSet = privilege.privileges.courseLevel; const sectionLevel: InstructorPermissionSet = privilege.privileges.sectionLevel[sectionId] || courseLevel; - studentModel.isAllowedToModifyStudent = sectionLevel.canModifyStudent; } } - - return { - searchStudentTables: coursesWithStudents, - }; + return coursesWithStudents; } /** @@ -185,7 +185,3 @@ export class InstructorSearchPageComponent implements OnInit { .pipe(map((courses) => Array.from(new Set(courses.courses.map((course) => course.courseId))))); } } - -interface TransformedInstructorSearchResult { - searchStudentTables: SearchStudentsListRowTable[]; -}