From 054f3439f1417a45150090f0a60a20c30dd9aae5 Mon Sep 17 00:00:00 2001 From: Hiroki Terashima Date: Wed, 27 Nov 2024 18:05:40 -0800 Subject: [PATCH] refactor(TeacherDataService, DiscussionService, TeacherDiscussionService): simplify component state retrieval --- src/app/services/discussionService.spec.ts | 57 ++++++++----------- .../services/teacherDiscussionService.spec.ts | 44 ++++---------- .../discussion-show-work.component.ts | 29 +++++----- .../discussion/discussionService.ts | 17 +++--- .../discussion/teacherDiscussionService.ts | 25 ++++---- .../wise5/services/teacherDataService.ts | 21 ------- src/messages.xlf | 2 +- 7 files changed, 76 insertions(+), 119 deletions(-) diff --git a/src/app/services/discussionService.spec.ts b/src/app/services/discussionService.spec.ts index a34062e4746..f52844012cb 100644 --- a/src/app/services/discussionService.spec.ts +++ b/src/app/services/discussionService.spec.ts @@ -15,11 +15,7 @@ import { NotificationService } from '../../assets/wise5/services/notificationSer import { ClassroomStatusService } from '../../assets/wise5/services/classroomStatusService'; import { provideHttpClient, withInterceptorsFromDi } from '@angular/common/http'; -class MockTeacherDataService { - getComponentStatesByComponentIds() { - return []; - } -} +class MockTeacherDataService {} let service: DiscussionService; let http: HttpTestingController; @@ -31,8 +27,8 @@ const runId = 1; describe('DiscussionService', () => { beforeEach(() => { TestBed.configureTestingModule({ - imports: [], - providers: [ + imports: [], + providers: [ AchievementService, AnnotationService, ClassroomStatusService, @@ -48,8 +44,8 @@ describe('DiscussionService', () => { TeacherWebSocketService, provideHttpClient(withInterceptorsFromDi()), provideHttpClientTesting() - ] -}); + ] + }); http = TestBed.inject(HttpTestingController); service = TestBed.inject(DiscussionService); @@ -184,29 +180,26 @@ function getClassmateResponsesFromComponents() { } function getClassmateResponses() { - it( - 'should get classmate responses', - waitForAsync(() => { - const componentState1 = { studentData: { response: 'Hello World' } }; - const annotation1 = { data: { action: 'Delete' } }; - service - .getClassmateResponses(runId, periodId, nodeId, componentId) - .subscribe((response: any) => { - expect(response.studentWork).toEqual([componentState1]); - expect(response.annotations).toEqual([annotation1]); - }); - http - .expectOne( - `/api/classmate/discussion/student-work/${runId}/${periodId}/${nodeId}/${componentId}` - ) - .flush([componentState1]); - http - .expectOne( - `/api/classmate/discussion/annotations/${runId}/${periodId}/${nodeId}/${componentId}` - ) - .flush([annotation1]); - }) - ); + it('should get classmate responses', waitForAsync(() => { + const componentState1 = { studentData: { response: 'Hello World' } }; + const annotation1 = { data: { action: 'Delete' } }; + service + .getClassmateResponses(runId, periodId, nodeId, componentId) + .subscribe((response: any) => { + expect(response.studentWork).toEqual([componentState1]); + expect(response.annotations).toEqual([annotation1]); + }); + http + .expectOne( + `/api/classmate/discussion/student-work/${runId}/${periodId}/${nodeId}/${componentId}` + ) + .flush([componentState1]); + http + .expectOne( + `/api/classmate/discussion/annotations/${runId}/${periodId}/${nodeId}/${componentId}` + ) + .flush([annotation1]); + })); } function isTopLevelPost() { diff --git a/src/app/services/teacherDiscussionService.spec.ts b/src/app/services/teacherDiscussionService.spec.ts index 0807fd15734..d8b30f2583a 100644 --- a/src/app/services/teacherDiscussionService.spec.ts +++ b/src/app/services/teacherDiscussionService.spec.ts @@ -1,25 +1,16 @@ import { DiscussionService } from '../../assets/wise5/components/discussion/discussionService'; import { TestBed } from '@angular/core/testing'; -import { provideHttpClientTesting } from '@angular/common/http/testing'; import { AnnotationService } from '../../assets/wise5/services/annotationService'; import { ConfigService } from '../../assets/wise5/services/configService'; import { ProjectService } from '../../assets/wise5/services/projectService'; import { StudentAssetService } from '../../assets/wise5/services/studentAssetService'; -import { TagService } from '../../assets/wise5/services/tagService'; -import { SessionService } from '../../assets/wise5/services/sessionService'; import { TeacherDataService } from '../../assets/wise5/services/teacherDataService'; import { TeacherProjectService } from '../../assets/wise5/services/teacherProjectService'; -import { TeacherWebSocketService } from '../../assets/wise5/services/teacherWebSocketService'; -import { AchievementService } from '../../assets/wise5/services/achievementService'; -import { NotificationService } from '../../assets/wise5/services/notificationService'; -import { ClassroomStatusService } from '../../assets/wise5/services/classroomStatusService'; import { TeacherDiscussionService } from '../../assets/wise5/components/discussion/teacherDiscussionService'; import { provideHttpClient, withInterceptorsFromDi } from '@angular/common/http'; class MockTeacherDataService { - getComponentStatesByComponentIds() { - return []; - } + getComponentStatesByComponentId() {} } class ComponentState { @@ -46,45 +37,30 @@ const nodeId = 'node1'; describe('TeacherDiscussionService', () => { beforeEach(() => { TestBed.configureTestingModule({ - imports: [], - providers: [ - AchievementService, + providers: [ AnnotationService, - ClassroomStatusService, ConfigService, DiscussionService, - NotificationService, ProjectService, - SessionService, StudentAssetService, - TagService, { provide: TeacherDataService, useClass: MockTeacherDataService }, TeacherDiscussionService, TeacherProjectService, - TeacherWebSocketService, - provideHttpClient(withInterceptorsFromDi()), - provideHttpClientTesting() - ] -}); - + provideHttpClient(withInterceptorsFromDi()) + ] + }); service = TestBed.inject(TeacherDiscussionService); }); - getPostAndAllRepliesWithComponentIdAndComponentStateId(); }); function getPostAndAllRepliesWithComponentIdAndComponentStateId() { it('should get post and all replies with component id and component state id', () => { - spyOn(TestBed.inject(TeacherDataService), 'getComponentStatesByComponentIds').and.callFake( - () => { - const componentStates = [ - new ComponentState(1, nodeId, componentId, null, 'Hello', []), - new ComponentState(2, nodeId, componentId, 1, 'World', []), - new ComponentState(3, nodeId, componentId, null, 'OK', []) - ]; - return componentStates; - } - ); + spyOn(TestBed.inject(TeacherDataService), 'getComponentStatesByComponentId').and.returnValue([ + new ComponentState(1, nodeId, componentId, null, 'Hello', []), + new ComponentState(2, nodeId, componentId, 1, 'World', []), + new ComponentState(3, nodeId, componentId, null, 'OK', []) + ]); const postAndAllReplies = service.getPostAndAllRepliesByComponentIds([componentId], 1); expect(postAndAllReplies.length).toEqual(2); }); diff --git a/src/assets/wise5/components/discussion/discussion-show-work/discussion-show-work.component.ts b/src/assets/wise5/components/discussion/discussion-show-work/discussion-show-work.component.ts index d311c5f3e87..6a3a2960380 100644 --- a/src/assets/wise5/components/discussion/discussion-show-work/discussion-show-work.component.ts +++ b/src/assets/wise5/components/discussion/discussion-show-work/discussion-show-work.component.ts @@ -36,12 +36,13 @@ export class DiscussionShowWorkComponent extends ComponentShowWorkDirective { this.setStudentWork(); } - setStudentWork(): void { + private setStudentWork(): void { const componentIds = this.getGradingComponentIds(); - const componentStates = this.TeacherDiscussionService.getPostsAssociatedWithComponentIdsAndWorkgroupId( - componentIds, - this.workgroupId - ); + const componentStates = + this.TeacherDiscussionService.getPostsAssociatedWithComponentIdsAndWorkgroupId( + componentIds, + this.workgroupId + ); const annotations = this.getInappropriateFlagAnnotationsByComponentStates(componentStates); this.setClassResponses(componentStates, annotations); } @@ -54,10 +55,11 @@ export class DiscussionShowWorkComponent extends ComponentShowWorkDirective { getInappropriateFlagAnnotationsByComponentStates(componentStates = []) { const annotations = []; for (const componentState of componentStates) { - const latestInappropriateFlagAnnotation = this.AnnotationService.getLatestAnnotationByStudentWorkIdAndType( - componentState.id, - 'inappropriateFlag' - ); + const latestInappropriateFlagAnnotation = + this.AnnotationService.getLatestAnnotationByStudentWorkIdAndType( + componentState.id, + 'inappropriateFlag' + ); if (latestInappropriateFlagAnnotation != null) { annotations.push(latestInappropriateFlagAnnotation); } @@ -145,10 +147,11 @@ export class DiscussionShowWorkComponent extends ComponentShowWorkDirective { data ); this.AnnotationService.saveAnnotation(annotation).then(() => { - const componentStates = this.TeacherDiscussionService.getPostsAssociatedWithComponentIdsAndWorkgroupId( - this.getGradingComponentIds(), - this.workgroupId - ); + const componentStates = + this.TeacherDiscussionService.getPostsAssociatedWithComponentIdsAndWorkgroupId( + this.getGradingComponentIds(), + this.workgroupId + ); const annotations = this.getInappropriateFlagAnnotationsByComponentStates(componentStates); this.setClassResponses(componentStates, annotations); }); diff --git a/src/assets/wise5/components/discussion/discussionService.ts b/src/assets/wise5/components/discussion/discussionService.ts index 2e8c93a8f0d..726ffc9f103 100644 --- a/src/assets/wise5/components/discussion/discussionService.ts +++ b/src/assets/wise5/components/discussion/discussionService.ts @@ -8,7 +8,10 @@ import { serverSaveTimeComparator } from '../../common/object/object'; @Injectable() export class DiscussionService extends ComponentService { - constructor(protected http: HttpClient, protected ConfigService: ConfigService) { + constructor( + protected configService: ConfigService, + protected http: HttpClient + ) { super(); } @@ -216,10 +219,8 @@ export class DiscussionService extends ComponentService { if (componentState.studentData.isSubmit) { componentState.replies = []; this.setUsernames(componentState); - const latestInappropriateFlagAnnotation = this.getLatestInappropriateFlagAnnotationByStudentWorkId( - annotations, - componentState.id - ); + const latestInappropriateFlagAnnotation = + this.getLatestInappropriateFlagAnnotationByStudentWorkId(annotations, componentState.id); if (isStudentMode) { if ( latestInappropriateFlagAnnotation == null || @@ -244,9 +245,9 @@ export class DiscussionService extends ComponentService { setUsernames(componentState: any): void { const workgroupId = componentState.workgroupId; - const usernames = this.ConfigService.getUsernamesByWorkgroupId(workgroupId); + const usernames = this.configService.getUsernamesByWorkgroupId(workgroupId); if (usernames.length > 0) { - componentState.usernames = this.ConfigService.getUsernamesStringByWorkgroupId(workgroupId); + componentState.usernames = this.configService.getUsernamesStringByWorkgroupId(workgroupId); } else if (componentState.usernamesArray != null) { componentState.usernames = componentState.usernamesArray .map(function (obj) { @@ -254,7 +255,7 @@ export class DiscussionService extends ComponentService { }) .join(', '); } else { - componentState.usernames = this.ConfigService.getUserIdsStringByWorkgroupId(workgroupId); + componentState.usernames = this.configService.getUserIdsStringByWorkgroupId(workgroupId); } } diff --git a/src/assets/wise5/components/discussion/teacherDiscussionService.ts b/src/assets/wise5/components/discussion/teacherDiscussionService.ts index bacebfac757..1005076dcc6 100644 --- a/src/assets/wise5/components/discussion/teacherDiscussionService.ts +++ b/src/assets/wise5/components/discussion/teacherDiscussionService.ts @@ -3,25 +3,22 @@ import { Injectable } from '@angular/core'; import { ConfigService } from '../../services/configService'; import { TeacherDataService } from '../../services/teacherDataService'; import { DiscussionService } from './discussionService'; +import { getIntersectOfArrays } from '../../common/array/array'; @Injectable() export class TeacherDiscussionService extends DiscussionService { constructor( protected http: HttpClient, protected configService: ConfigService, - protected teacherDataService: TeacherDataService + protected dataService: TeacherDataService ) { - super(http, configService); + super(configService, http); } getPostsAssociatedWithComponentIdsAndWorkgroupId(componentIds: string[], workgroupId: number) { let allPosts = []; const topLevelComponentStateIdsFound = []; - const componentStates = this.teacherDataService.getComponentStatesByWorkgroupIdAndComponentIds( - workgroupId, - componentIds - ); - for (const componentState of componentStates) { + for (const componentState of this.getComponentStates(workgroupId, componentIds)) { const componentStateIdReplyingTo = componentState.studentData.componentStateIdReplyingTo; if (this.isTopLevelPost(componentState)) { if ( @@ -49,12 +46,20 @@ export class TeacherDiscussionService extends DiscussionService { return allPosts; } + private getComponentStates(workgroupId: number, componentIds: string[]): any[] { + const workgroupComponentStates = this.dataService.getComponentStatesByWorkgroupId(workgroupId); + const componentStates = componentIds.flatMap((componentId) => + this.dataService.getComponentStatesByComponentId(componentId) + ); + return getIntersectOfArrays(workgroupComponentStates, componentStates); + } + getPostAndAllRepliesByComponentIds(componentIds: string[], componentStateId: number) { const postAndAllReplies = []; - const componentStatesForComponentIds = this.teacherDataService.getComponentStatesByComponentIds( - componentIds + const componentStates = componentIds.flatMap((componentId) => + this.dataService.getComponentStatesByComponentId(componentId) ); - for (const componentState of componentStatesForComponentIds) { + for (const componentState of componentStates) { if (componentState.id === componentStateId) { postAndAllReplies.push(componentState); } else { diff --git a/src/assets/wise5/services/teacherDataService.ts b/src/assets/wise5/services/teacherDataService.ts index c6489d3015f..6b235d30fa5 100644 --- a/src/assets/wise5/services/teacherDataService.ts +++ b/src/assets/wise5/services/teacherDataService.ts @@ -397,16 +397,6 @@ export class TeacherDataService extends DataService { return this.studentData.componentStatesByComponentId[componentId] || []; } - getComponentStatesByComponentIds(componentIds) { - let componentStatesByComponentId = []; - for (const componentId of componentIds) { - componentStatesByComponentId = componentStatesByComponentId.concat( - this.studentData.componentStatesByComponentId[componentId] - ); - } - return componentStatesByComponentId; - } - getLatestComponentStateByWorkgroupIdNodeIdAndComponentId(workgroupId, nodeId, componentId) { return ( this.getComponentStatesByWorkgroupIdAndNodeId(workgroupId, nodeId).findLast( @@ -436,17 +426,6 @@ export class TeacherDataService extends DataService { return getIntersectOfArrays(componentStatesByWorkgroupId, componentStatesByComponentId); } - getComponentStatesByWorkgroupIdAndComponentIds(workgroupId, componentIds) { - const componentStatesByWorkgroupId = this.getComponentStatesByWorkgroupId(workgroupId); - let componentStatesByComponentId = []; - for (const componentId of componentIds) { - componentStatesByComponentId = componentStatesByComponentId.concat( - this.getComponentStatesByComponentId(componentId) - ); - } - return getIntersectOfArrays(componentStatesByWorkgroupId, componentStatesByComponentId); - } - getEventsByWorkgroupId(workgroupId) { return this.studentData.eventsByWorkgroupId[workgroupId] || []; } diff --git a/src/messages.xlf b/src/messages.xlf index 5d2fd0662a3..a90c4c1bcd6 100644 --- a/src/messages.xlf +++ b/src/messages.xlf @@ -17717,7 +17717,7 @@ Category Name: src/assets/wise5/components/discussion/discussionService.ts - 16 + 19