From 5191b411a485f8dd60e85933b5c3a201f2444a78 Mon Sep 17 00:00:00 2001 From: Hiroki Terashima Date: Fri, 13 Oct 2023 09:35:07 -0700 Subject: [PATCH 1/2] refactor(AnnotationService): getLatestScoreAnnotation() --- .../component-new-work-badge.component.ts | 2 +- src/app/services/annotationService.spec.ts | 77 +++++++++++++----- .../services/studentPeerGroupService.spec.ts | 3 +- .../ScoreConstraintStrategy.spec.ts | 3 +- .../wise5/services/annotationService.ts | 80 ++++++------------- 5 files changed, 90 insertions(+), 75 deletions(-) diff --git a/src/app/classroom-monitor/component-new-work-badge/component-new-work-badge.component.ts b/src/app/classroom-monitor/component-new-work-badge/component-new-work-badge.component.ts index a8ae55e937f..fc54a01a67f 100644 --- a/src/app/classroom-monitor/component-new-work-badge/component-new-work-badge.component.ts +++ b/src/app/classroom-monitor/component-new-work-badge/component-new-work-badge.component.ts @@ -62,7 +62,7 @@ export class ComponentNewWorkBadgeComponent { } let latestTeacherScore = null; if (latestAnnotations && latestAnnotations.score) { - if (latestAnnotations.score !== 'autoScore') { + if (latestAnnotations.score.type !== 'autoScore') { latestTeacherScore = latestAnnotations.score; } } diff --git a/src/app/services/annotationService.spec.ts b/src/app/services/annotationService.spec.ts index 4d8c3783de9..4ced47474f6 100644 --- a/src/app/services/annotationService.spec.ts +++ b/src/app/services/annotationService.spec.ts @@ -5,27 +5,41 @@ import { AnnotationService } from '../../assets/wise5/services/annotationService import { ProjectService } from '../../assets/wise5/services/projectService'; import { StudentTeacherCommonServicesModule } from '../student-teacher-common-services.module'; import demoProjectJSON_import from './sampleData/curriculum/Demo.project.json'; +import { Annotation } from '../../assets/wise5/common/Annotation'; let service: AnnotationService; let projectService: ProjectService; let demoProjectJSON: any; -const annotations = [ - { - toWorkgroupId: 1, - type: 'score', - nodeId: 'node2', - componentId: '7edwu1p29b', - data: { value: 1 } - }, - { - toWorkgroupId: 1, - type: 'autoScore', - nodeId: 'node3', - componentId: '0sef5ya2wj', - data: { value: 2 } - } -]; +const annotation1 = { + toWorkgroupId: 1, + type: 'score', + nodeId: 'node2', + componentId: '7edwu1p29b', + data: { value: 1 } +}; +const annotation2 = { + toWorkgroupId: 1, + type: 'comment', + nodeId: 'node2', + componentId: '7edwu1p29b', + data: { value: 'Nice job!' } +}; +const annotation3 = { + toWorkgroupId: 1, + type: 'autoScore', + nodeId: 'node3', + componentId: '0sef5ya2wj', + data: { value: 2 } +}; +const annotation4 = { + toWorkgroupId: 1, + type: 'autoScore', + nodeId: 'node2', + componentId: '7edwu1p29b', + data: { value: 5 } +}; +const annotations = [annotation1, annotation2, annotation3, annotation4]; describe('AnnotationService', () => { beforeEach(() => { @@ -33,13 +47,40 @@ describe('AnnotationService', () => { imports: [HttpClientTestingModule, StudentTeacherCommonServicesModule] }); service = TestBed.inject(AnnotationService); + service.annotations = annotations as Annotation[]; projectService = TestBed.inject(ProjectService); demoProjectJSON = copy(demoProjectJSON_import); projectService.setProject(demoProjectJSON); }); + getLatestScoreAnnotation(); getTotalScore(); }); +function getLatestScoreAnnotation() { + describe('getLatestScoreAnnotation()', () => { + getLatestScoreAnnotation_NoMatch_ReturnNull(); + getLatestScoreAnnotation_MultipleMatches_ReturnLatestAnnotation(); + }); +} + +function getLatestScoreAnnotation_NoMatch_ReturnNull() { + describe('no matching annotation is found', () => { + it('returns null', () => { + expect(service.getLatestScoreAnnotation('nodeX', 'componentX', 10, 'any')).toBeUndefined(); + }); + }); +} + +function getLatestScoreAnnotation_MultipleMatches_ReturnLatestAnnotation() { + describe('multiple annotations are found', () => { + it('returns latest annotation', () => { + expect(service.getLatestScoreAnnotation('node2', '7edwu1p29b', 1, 'any')).toEqual( + annotation4 as Annotation + ); + }); + }); +} + function getTotalScore() { describe('getTotalScore()', () => { getTotalScore_noAnnotationForWorkgroup_return0(); @@ -57,7 +98,7 @@ function getTotalScore_noAnnotationForWorkgroup_return0() { function getTotalScore_returnSumScoresAutoScoresForWorkgroup() { it('should return sum of annotation scores and autoscores for workgroup', () => { - expect(service.getTotalScore(annotations)).toEqual(3); + expect(service.getTotalScore(annotations)).toEqual(7); }); } @@ -73,6 +114,6 @@ function getTotalScore_omitInActiveNodes() { function getTotalScore_omitExcludFromTotalScoreNodes() { it('should omit scores for nodes marked as excludeFromTotalScore', () => { projectService.getComponent('node3', '0sef5ya2wj').excludeFromTotalScore = true; - expect(service.getTotalScore(annotations)).toEqual(1); + expect(service.getTotalScore(annotations)).toEqual(5); }); } diff --git a/src/app/services/studentPeerGroupService.spec.ts b/src/app/services/studentPeerGroupService.spec.ts index 523fe55b0cf..7a83cb32b1b 100644 --- a/src/app/services/studentPeerGroupService.spec.ts +++ b/src/app/services/studentPeerGroupService.spec.ts @@ -11,11 +11,12 @@ import { ProjectService } from '../../assets/wise5/services/projectService'; import { StudentDataService } from '../../assets/wise5/services/studentDataService'; import { StudentPeerGroupService } from '../../assets/wise5/services/studentPeerGroupService'; import { StudentTeacherCommonServicesModule } from '../student-teacher-common-services.module'; +import { Annotation } from '../../assets/wise5/common/Annotation'; let annotationService: AnnotationService; const componentId1 = 'component1'; let configService: ConfigService; -const dummyAnnotation = { id: 200, data: {} }; +const dummyAnnotation = { id: 200, data: {} } as Annotation; const dummyStudentData = { id: 100, studentData: {} }; let http: HttpClient; const nodeId1 = 'node1'; diff --git a/src/assets/wise5/common/constraint/strategies/ScoreConstraintStrategy.spec.ts b/src/assets/wise5/common/constraint/strategies/ScoreConstraintStrategy.spec.ts index eaaa713c7b5..6c78ca0b4b4 100644 --- a/src/assets/wise5/common/constraint/strategies/ScoreConstraintStrategy.spec.ts +++ b/src/assets/wise5/common/constraint/strategies/ScoreConstraintStrategy.spec.ts @@ -5,6 +5,7 @@ import { AnnotationService } from '../../../services/annotationService'; import { ConfigService } from '../../../services/configService'; import { StudentDataService } from '../../../services/studentDataService'; import { ScoreConstraintStrategy } from './ScoreConstraintStrategy'; +import { Annotation } from '../../Annotation'; let annotationService: AnnotationService; let configService: ConfigService; @@ -47,7 +48,7 @@ function evaluate() { function expectEvaluate(criteria: any, score: number, expected: boolean): void { spyOn(configService, 'getWorkgroupId').and.returnValue(1); - spyOn(annotationService, 'getLatestScoreAnnotation').and.returnValue({}); + spyOn(annotationService, 'getLatestScoreAnnotation').and.returnValue({} as Annotation); spyOn(annotationService, 'getScoreValueFromScoreAnnotation').and.returnValue(score); expect(strategy.evaluate(criteria)).toEqual(expected); } diff --git a/src/assets/wise5/services/annotationService.ts b/src/assets/wise5/services/annotationService.ts index 95a860ee0eb..6030ee93536 100644 --- a/src/assets/wise5/services/annotationService.ts +++ b/src/assets/wise5/services/annotationService.ts @@ -443,7 +443,7 @@ export class AnnotationService { nodeId, componentId, workgroupId, - scoreType = null, + scoreType: 'score' | 'autoScore' | 'any' = 'any', commentType = null ) { let latestScoreAnnotation = this.getLatestScoreAnnotation( @@ -530,59 +530,31 @@ export class AnnotationService { return null; } - /** - * Get the latest score annotation - * @param nodeId the node id - * @param componentId the component id - * @param workgroupId the workgroup id - * @param scoreType (optional) the type of score - * e.g. - * 'autoScore' for auto graded score - * 'score' for teacher graded score - * 'any' for auto graded score or teacher graded score - * @returns the latest score annotation - */ - getLatestScoreAnnotation(nodeId, componentId, workgroupId, scoreType = null) { - let annotation = null; - const annotations = this.getAnnotations(); - - if (scoreType == null) { - scoreType = 'any'; - } - - for (let a = annotations.length - 1; a >= 0; a--) { - const tempAnnotation = annotations[a]; - if (tempAnnotation != null) { - let acceptAnnotation = false; - const tempNodeId = tempAnnotation.nodeId; - const tempComponentId = tempAnnotation.componentId; - const tempToWorkgroupId = tempAnnotation.toWorkgroupId; - const tempAnnotationType = tempAnnotation.type; - - if ( - nodeId == tempNodeId && - componentId == tempComponentId && - workgroupId == tempToWorkgroupId - ) { - if ( - scoreType === 'any' && - (tempAnnotationType === 'autoScore' || tempAnnotationType === 'score') - ) { - acceptAnnotation = true; - } else if (scoreType === 'autoScore' && tempAnnotationType === 'autoScore') { - acceptAnnotation = true; - } else if (scoreType === 'score' && tempAnnotationType === 'score') { - acceptAnnotation = true; - } - - if (acceptAnnotation) { - annotation = tempAnnotation; - break; - } - } - } - } - return annotation; + getLatestScoreAnnotation( + nodeId: string, + componentId: string, + workgroupId: number, + scoreType: 'score' | 'autoScore' | 'any' = 'any' + ): Annotation { + return this.getAnnotations() + .filter( + (annotation) => + annotation.nodeId == nodeId && + annotation.componentId == componentId && + annotation.toWorkgroupId == workgroupId && + this.matchesScoreType(annotation, scoreType) + ) + .at(-1); + } + + private matchesScoreType( + annotation: Annotation, + scoreType: 'score' | 'autoScore' | 'any' + ): boolean { + return ( + ['autoScore', 'score'].includes(annotation.type) && + (scoreType === 'any' || annotation.type === scoreType) + ); } isThereAnyScoreAnnotation(nodeId, componentId, periodId) { From 63332bd5e7a68cdf08c8f45700e0828d929de3d0 Mon Sep 17 00:00:00 2001 From: Hiroki Terashima Date: Mon, 16 Oct 2023 10:02:09 -0700 Subject: [PATCH 2/2] getLatestAnnotations() return null instead of undefined Refactor matchesScoreType() --- src/app/services/annotationService.spec.ts | 2 +- .../wise5/services/annotationService.ts | 24 ++++++++++--------- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/src/app/services/annotationService.spec.ts b/src/app/services/annotationService.spec.ts index 4ced47474f6..791ae80c8e0 100644 --- a/src/app/services/annotationService.spec.ts +++ b/src/app/services/annotationService.spec.ts @@ -66,7 +66,7 @@ function getLatestScoreAnnotation() { function getLatestScoreAnnotation_NoMatch_ReturnNull() { describe('no matching annotation is found', () => { it('returns null', () => { - expect(service.getLatestScoreAnnotation('nodeX', 'componentX', 10, 'any')).toBeUndefined(); + expect(service.getLatestScoreAnnotation('nodeX', 'componentX', 10, 'any')).toBeNull(); }); }); } diff --git a/src/assets/wise5/services/annotationService.ts b/src/assets/wise5/services/annotationService.ts index 6030ee93536..d14691f6b0b 100644 --- a/src/assets/wise5/services/annotationService.ts +++ b/src/assets/wise5/services/annotationService.ts @@ -536,15 +536,17 @@ export class AnnotationService { workgroupId: number, scoreType: 'score' | 'autoScore' | 'any' = 'any' ): Annotation { - return this.getAnnotations() - .filter( - (annotation) => - annotation.nodeId == nodeId && - annotation.componentId == componentId && - annotation.toWorkgroupId == workgroupId && - this.matchesScoreType(annotation, scoreType) - ) - .at(-1); + return ( + this.getAnnotations() + .filter( + (annotation) => + annotation.nodeId == nodeId && + annotation.componentId == componentId && + annotation.toWorkgroupId == workgroupId && + this.matchesScoreType(annotation, scoreType) + ) + .at(-1) || null + ); } private matchesScoreType( @@ -552,8 +554,8 @@ export class AnnotationService { scoreType: 'score' | 'autoScore' | 'any' ): boolean { return ( - ['autoScore', 'score'].includes(annotation.type) && - (scoreType === 'any' || annotation.type === scoreType) + (scoreType === 'any' && ['autoScore', 'score'].includes(annotation.type)) || + annotation.type === scoreType ); }