From 5a27618aa21611b2787e3586e89fc3ba47e48e83 Mon Sep 17 00:00:00 2001 From: Hiroki Terashima Date: Wed, 27 Nov 2024 10:30:35 -0800 Subject: [PATCH] refactor(StudentDataService, AnnotationService, TeacherDataService): simplify state retrieval logic using Array.findLast() (#2008) --- src/app/services/studentDataService.spec.ts | 15 +---- .../OneWorkgroupPerRowDataExportStrategy.ts | 13 ++-- .../wise5/services/annotationService.ts | 32 ++++------ .../wise5/services/studentDataService.ts | 55 +++++++--------- .../wise5/services/teacherDataService.ts | 64 +++++-------------- 5 files changed, 57 insertions(+), 122 deletions(-) diff --git a/src/app/services/studentDataService.spec.ts b/src/app/services/studentDataService.spec.ts index 41cb62bea37..5e747fa8b66 100644 --- a/src/app/services/studentDataService.spec.ts +++ b/src/app/services/studentDataService.spec.ts @@ -4,23 +4,20 @@ import { StudentDataService } from '../../assets/wise5/services/studentDataServi import { ConfigService } from '../../assets/wise5/services/configService'; import { AnnotationService } from '../../assets/wise5/services/annotationService'; import { ProjectService } from '../../assets/wise5/services/projectService'; -import { MatDialogModule } from '@angular/material/dialog'; import { StudentTeacherCommonServicesModule } from '../student-teacher-common-services.module'; import { provideHttpClient, withInterceptorsFromDi } from '@angular/common/http'; -let $rootScope; let http: HttpTestingController; let service: StudentDataService; let configService: ConfigService; let annotationService: AnnotationService; let projectService: ProjectService; - describe('StudentDataService', () => { beforeEach(() => { TestBed.configureTestingModule({ - imports: [MatDialogModule, StudentTeacherCommonServicesModule], - providers: [provideHttpClient(withInterceptorsFromDi()), provideHttpClientTesting()] -}); + imports: [StudentTeacherCommonServicesModule], + providers: [provideHttpClient(withInterceptorsFromDi()), provideHttpClientTesting()] + }); http = TestBed.inject(HttpTestingController); service = TestBed.inject(StudentDataService); configService = TestBed.inject(ConfigService); @@ -135,7 +132,6 @@ function shouldHandleSaveStudentWorkToServerSuccess() { ] }; spyOn(configService, 'getMode').and.returnValue('preview'); - spyOn($rootScope, '$broadcast'); spyOn(service, 'updateNodeStatuses').and.callFake(() => {}); service.saveToServerRequestCount = 1; service.handleSaveToServerSuccess(savedStudentDataResponse); @@ -148,10 +144,6 @@ function shouldHandleSaveStudentWorkToServerSuccess() { expect(service.studentData.componentStates[2].serverSaveTime).toBeDefined(); expect(service.studentData.componentStates[2].requestToken).toEqual(null); expect(service.studentData.componentStates[2].id).toEqual(3); - expect($rootScope.$broadcast).toHaveBeenCalledWith( - 'studentWorkSavedToServer', - jasmine.any(Object) - ); expect(service.saveToServerRequestCount).toEqual(0); expect(service.updateNodeStatuses).toHaveBeenCalled(); }); @@ -191,7 +183,6 @@ function shouldHandleSaveEventsToServerSuccess() { createEvent(3, 'node1', 'component1', 'nodeEntered', 'c', 3000) ] }; - spyOn($rootScope, '$broadcast'); spyOn(service, 'updateNodeStatuses').and.callFake(() => {}); service.saveToServerRequestCount = 1; service.handleSaveToServerSuccess(savedStudentDataResponse); diff --git a/src/assets/wise5/classroomMonitor/dataExport/strategies/OneWorkgroupPerRowDataExportStrategy.ts b/src/assets/wise5/classroomMonitor/dataExport/strategies/OneWorkgroupPerRowDataExportStrategy.ts index 05723f87c9d..1a3c6269ce9 100644 --- a/src/assets/wise5/classroomMonitor/dataExport/strategies/OneWorkgroupPerRowDataExportStrategy.ts +++ b/src/assets/wise5/classroomMonitor/dataExport/strategies/OneWorkgroupPerRowDataExportStrategy.ts @@ -246,14 +246,11 @@ export class OneWorkgroupPerRowDataExportStrategy extends AbstractDataExportStra } private getLatestBranchPathTakenEvent(workgroupId: number, nodeId: string): any { - const events = this.teacherDataService.getEventsByWorkgroupId(workgroupId); - for (let i = events.length - 1; i >= 0; i--) { - const event = events[i]; - if (event.nodeId === nodeId && event.event === 'branchPathTaken') { - return event; - } - } - return null; + return ( + this.teacherDataService + .getEventsByWorkgroupId(workgroupId) + .findLast((event) => event.nodeId === nodeId && event.event === 'branchPathTaken') ?? null + ); } /** diff --git a/src/assets/wise5/services/annotationService.ts b/src/assets/wise5/services/annotationService.ts index 9ab866799fe..8f9998c0e34 100644 --- a/src/assets/wise5/services/annotationService.ts +++ b/src/assets/wise5/services/annotationService.ts @@ -42,27 +42,17 @@ export class AnnotationService { * @returns the latest annotation that matches the params */ getLatestAnnotation(params): any { - for (let a = this.annotations.length - 1; a >= 0; a--) { - const annotation = this.annotations[a]; - let match = true; - if (annotation.nodeId !== params.nodeId) { - match = false; - } - if (match && annotation.componentId !== params.componentId) { - match = false; - } - if (match) { - if (params.type.constructor === Array) { - match = params.type.every((thisType) => annotation.type === thisType); - } else { - match = annotation.type === params.type; - } - if (match) { - return annotation; - } - } - } - return null; + return ( + this.annotations.findLast((annotation) => { + return ( + annotation.nodeId == params.nodeId && + annotation.componentId == params.componentId && + ((params.type.constructor === Array && + params.type.every((thisType) => annotation.type === thisType)) || + annotation.type === params.type) + ); + }) ?? null + ); } /** diff --git a/src/assets/wise5/services/studentDataService.ts b/src/assets/wise5/services/studentDataService.ts index fa9edd29e4a..6732fa1dab6 100644 --- a/src/assets/wise5/services/studentDataService.ts +++ b/src/assets/wise5/services/studentDataService.ts @@ -524,31 +524,24 @@ export class StudentDataService extends DataService { * are found */ getLatestComponentStateByNodeIdAndComponentId(nodeId: string, componentId: string = null): any { - const componentStates = this.studentData.componentStates; - for (let c = componentStates.length - 1; c >= 0; c--) { - const componentState = componentStates[c]; - if (componentState.nodeId === nodeId) { - if (componentId == null || componentState.componentId === componentId) { - return componentState; - } - } - } - return null; + return ( + this.studentData.componentStates.findLast( + (componentState) => + componentState.nodeId === nodeId && + (componentId == null || componentState.componentId === componentId) + ) ?? null + ); } - getLatestSubmitComponentState(nodeId, componentId) { - const componentStates = this.studentData.componentStates; - for (let c = componentStates.length - 1; c >= 0; c--) { - const componentState = componentStates[c]; - if ( - componentState.nodeId === nodeId && - componentState.componentId === componentId && - componentState.isSubmit - ) { - return componentState; - } - } - return null; + getLatestSubmitComponentState(nodeId: string, componentId: string): any { + return ( + this.studentData.componentStates.findLast( + (componentState) => + componentState.nodeId === nodeId && + componentState.componentId === componentId && + componentState.isSubmit + ) ?? null + ); } getComponentStates(): any[] { @@ -585,18 +578,14 @@ export class StudentDataService extends DataService { * @return the node id of the latest node entered event for an active node * that exists in the project */ - getLatestNodeEnteredEventNodeIdWithExistingNode() { - const events = this.studentData.events; - for (let e = events.length - 1; e >= 0; e--) { - const event = events[e]; - if (event.event == 'nodeEntered' && this.isNodeExistAndActive(event.nodeId)) { - return event.nodeId; - } - } - return null; + getLatestNodeEnteredEventNodeIdWithExistingNode(): string { + const event = this.studentData.events.findLast( + (event) => event.event === 'nodeEntered' && this.isNodeExistAndActive(event.nodeId) + ); + return event?.nodeId ?? null; } - isNodeExistAndActive(nodeId) { + private isNodeExistAndActive(nodeId: string): boolean { return this.ProjectService.getNodeById(nodeId) != null && this.ProjectService.isActive(nodeId); } diff --git a/src/assets/wise5/services/teacherDataService.ts b/src/assets/wise5/services/teacherDataService.ts index a29de7f1dd2..a2e6842d09b 100644 --- a/src/assets/wise5/services/teacherDataService.ts +++ b/src/assets/wise5/services/teacherDataService.ts @@ -1,5 +1,3 @@ -'use strict'; - import { HttpClient, HttpParams } from '@angular/common/http'; import { AnnotationService } from './annotationService'; import { ConfigService } from './configService'; @@ -10,7 +8,6 @@ import { Observable, Subject, tap } from 'rxjs'; import { DataService } from '../../../app/services/data.service'; import { Node } from '../common/Node'; import { compressToEncodedURIComponent } from 'lz-string'; -import { isMatchingPeriods } from '../common/period/period'; import { getIntersectOfArrays } from '../common/array/array'; import { serverSaveTimeComparator } from '../common/object/object'; import { Annotation } from '../common/Annotation'; @@ -411,33 +408,26 @@ export class TeacherDataService extends DataService { } getLatestComponentStateByWorkgroupIdNodeIdAndComponentId(workgroupId, nodeId, componentId) { - const componentStates = this.getComponentStatesByWorkgroupIdAndNodeId(workgroupId, nodeId); - for (let c = componentStates.length - 1; c >= 0; c--) { - const componentState = componentStates[c]; - if (this.isComponentStateMatchingNodeIdComponentId(componentState, nodeId, componentId)) { - return componentState; - } - } - return null; - } - - isComponentStateMatchingNodeIdComponentId(componentState, nodeId, componentId) { - return componentState.nodeId === nodeId && componentState.componentId === componentId; + return ( + this.getComponentStatesByWorkgroupIdAndNodeId(workgroupId, nodeId).findLast( + (componentState) => + componentState.nodeId === nodeId && componentState.componentId === componentId + ) ?? null + ); } - getLatestComponentStateByWorkgroupIdNodeId(workgroupId, nodeId) { - const componentStates = this.getComponentStatesByWorkgroupIdAndNodeId(workgroupId, nodeId); - for (let c = componentStates.length - 1; c >= 0; c--) { - const componentState = componentStates[c]; - if (this.isComponentStateMatchingNodeId(componentState, nodeId)) { - return componentState; - } - } - return null; + getLatestComponentStateByWorkgroupIdNodeId(workgroupId: number, nodeId: string): any { + return ( + this.getComponentStatesByWorkgroupIdAndNodeId(workgroupId, nodeId).findLast( + (componentState) => componentState.nodeId === nodeId + ) ?? null + ); } - isComponentStateMatchingNodeId(componentState, nodeId) { - return componentState.nodeId === nodeId; + private getComponentStatesByWorkgroupIdAndNodeId(workgroupId: number, nodeId: string): any[] { + const componentStatesByWorkgroupId = this.getComponentStatesByWorkgroupId(workgroupId); + const componentStatesByNodeId = this.getComponentStatesByNodeId(nodeId); + return getIntersectOfArrays(componentStatesByWorkgroupId, componentStatesByNodeId); } /** @@ -478,12 +468,6 @@ export class TeacherDataService extends DataService { return componentState.nodeId + '-' + componentState.componentId; } - getComponentStatesByWorkgroupIdAndNodeId(workgroupId, nodeId) { - const componentStatesByWorkgroupId = this.getComponentStatesByWorkgroupId(workgroupId); - const componentStatesByNodeId = this.getComponentStatesByNodeId(nodeId); - return getIntersectOfArrays(componentStatesByWorkgroupId, componentStatesByNodeId); - } - getComponentStatesByWorkgroupIdAndComponentId(workgroupId, componentId) { const componentStatesByWorkgroupId = this.getComponentStatesByWorkgroupId(workgroupId); const componentStatesByComponentId = this.getComponentStatesByComponentId(componentId); @@ -517,22 +501,6 @@ export class TeacherDataService extends DataService { return this.studentData.annotationsByNodeId[nodeId] || []; } - getAnnotationsByNodeIdAndComponentId(nodeId: string, componentId: string): any[] { - const annotationsByNodeId = this.getAnnotationsByNodeId(nodeId); - return annotationsByNodeId.filter((annotation: any) => annotation.componentId === componentId); - } - - getAnnotationsByNodeIdAndPeriodId(nodeId, periodId) { - const annotationsByNodeId = this.studentData.annotationsByNodeId[nodeId]; - if (annotationsByNodeId != null) { - return annotationsByNodeId.filter((annotation) => { - return isMatchingPeriods(annotation.periodId, periodId); - }); - } else { - return []; - } - } - setCurrentPeriod(period) { const previousPeriod = this.currentPeriod; this.currentPeriod = period;