From 8a8cfb6b01de44784adc147e206d6a354aff0327 Mon Sep 17 00:00:00 2001 From: Geoffrey Kwan Date: Sun, 18 Aug 2024 11:57:37 -0700 Subject: [PATCH 1/2] refactor(Graph): Create graph connected component manager --- .../graph-connected-component-manager.ts | 98 ++++++++++++++ .../graph-student/graph-student.component.ts | 126 +++++------------- 2 files changed, 128 insertions(+), 96 deletions(-) create mode 100644 src/assets/wise5/components/graph/graph-connected-component-manager.ts diff --git a/src/assets/wise5/components/graph/graph-connected-component-manager.ts b/src/assets/wise5/components/graph/graph-connected-component-manager.ts new file mode 100644 index 00000000000..e3146745827 --- /dev/null +++ b/src/assets/wise5/components/graph/graph-connected-component-manager.ts @@ -0,0 +1,98 @@ +import { GraphService } from './graphService'; +import { ProjectService } from '../../services/projectService'; +import { generateRandomKey } from '../../common/string/string'; +import { copy } from '../../common/object/object'; + +export class GraphConnectedComponentManager { + constructor( + private graphService: GraphService, + private projectService: ProjectService + ) {} + + /** + * Get the trials from classmates + * @param nodeId the node id + * @param componentId the component id + * @param showClassmateWorkSource Whether to get the work only from the + * period the student is in or from all the periods. The possible values + * are "period" or "class". + * @return a promise that will return all the trials from the classmates + */ + getTrialsFromClassmates( + nodeId: string, + componentId: string, + periodId: number, + showWorkNodeId: string, + showWorkComponentId: string, + showClassmateWorkSource: 'period' | 'class' + ): Promise { + return new Promise((resolve, reject) => { + this.graphService + .getClassmateStudentWork( + nodeId, + componentId, + periodId, + showWorkNodeId, + showWorkComponentId, + showClassmateWorkSource + ) + .subscribe((componentStates: any[]) => { + const promises = []; + for (const componentState of componentStates) { + promises.push(this.getTrialsFromComponentState(nodeId, componentId, componentState)); + } + Promise.all(promises).then((promiseResults) => { + const mergedTrials = []; + for (const trials of promiseResults) { + for (const trial of trials) { + mergedTrials.push(trial); + } + } + resolve(mergedTrials); + }); + }); + }); + } + + /** + * Get the trials from a component state. + * Note: The code in this function doesn't actually require usage of a + * promise. It's just the code that calls this function that utilizes + * promise functionality. It's possible to refactor the code so that this + * function doesn't need to return a promise. + * @param nodeId the node id + * @param componentId the component id + * @param componentState the component state + * @return a promise that will return the trials from the component state + */ + getTrialsFromComponentState( + nodeId: string, + componentId: string, + componentState: any + ): Promise { + const mergedTrials = []; + const nodePositionAndTitle = this.projectService.getNodePositionAndTitle(nodeId); + const studentData = componentState.studentData; + if (studentData.version == null || studentData.version === 1) { + const series = studentData.series; + const newTrial = { + id: generateRandomKey(), + name: nodePositionAndTitle, + show: true, + series: series + }; + mergedTrials.push(newTrial); + } else { + const trials = studentData.trials; + if (trials != null) { + for (const trial of trials) { + const newTrial = copy(trial); + newTrial.name = nodePositionAndTitle; + newTrial.show = true; + mergedTrials.push(newTrial); + } + } + } + return Promise.resolve(mergedTrials); + } +} diff --git a/src/assets/wise5/components/graph/graph-student/graph-student.component.ts b/src/assets/wise5/components/graph/graph-student/graph-student.component.ts index 901908b301f..923d4d18f38 100644 --- a/src/assets/wise5/components/graph/graph-student/graph-student.component.ts +++ b/src/assets/wise5/components/graph/graph-student/graph-student.component.ts @@ -22,6 +22,7 @@ import { GraphCustomLegend } from '../GraphCustomLegend'; import { PlotLineManager } from '../plot-line-manager'; import { DataExplorerManager } from '../data-explorer-manager'; import { addPointFromTableIntoData, isMultipleYAxes, isSingleYAxis } from '../util'; +import { GraphConnectedComponentManager } from '../graph-connected-component-manager'; const Draggable = require('highcharts/modules/draggable-points.js'); Draggable(Highcharts); @@ -43,6 +44,7 @@ export class GraphStudent extends ComponentStudent { chartConfig: any; chartId: string = 'chart1'; fileName: string; + graphConnectedComponentManager: GraphConnectedComponentManager; graphType: string; hasCustomLegendBeenSet: boolean = false; height: number = null; @@ -117,6 +119,10 @@ export class GraphStudent extends ComponentStudent { this.componentContent.showMouseXPlotLine, this.componentContent.showMouseYPlotLine ); + this.graphConnectedComponentManager = new GraphConnectedComponentManager( + this.GraphService, + this.ProjectService + ); this.initializeComponentContentParams(); this.initializeStudentMode(this.componentState); this.initialComponentState = this.componentState; @@ -1329,87 +1335,6 @@ export class GraphStudent extends ComponentStudent { return null; } - /** - * Get the trials from classmates - * @param nodeId the node id - * @param componentId the component id - * @param showClassmateWorkSource Whether to get the work only from the - * period the student is in or from all the periods. The possible values - * are "period" or "class". - * @return a promise that will return all the trials from the classmates - */ - getTrialsFromClassmates( - nodeId: string, - componentId: string, - periodId: number, - showWorkNodeId: string, - showWorkComponentId: string, - showClassmateWorkSource: 'period' | 'class' - ): Promise { - return new Promise((resolve, reject) => { - this.GraphService.getClassmateStudentWork( - nodeId, - componentId, - periodId, - showWorkNodeId, - showWorkComponentId, - showClassmateWorkSource - ).subscribe((componentStates: any[]) => { - const promises = []; - for (const componentState of componentStates) { - promises.push(this.getTrialsFromComponentState(nodeId, componentId, componentState)); - } - Promise.all(promises).then((promiseResults) => { - const mergedTrials = []; - for (const trials of promiseResults) { - for (const trial of trials) { - mergedTrials.push(trial); - } - } - resolve(mergedTrials); - }); - }); - }); - } - - /** - * Get the trials from a component state. - * Note: The code in this function doesn't actually require usage of a - * promise. It's just the code that calls this function that utilizes - * promise functionality. It's possible to refactor the code so that this - * function doesn't need to return a promise. - * @param nodeId the node id - * @param componentId the component id - * @param componentState the component state - * @return a promise that will return the trials from the component state - */ - getTrialsFromComponentState(nodeId, componentId, componentState) { - const mergedTrials = []; - const nodePositionAndTitle = this.ProjectService.getNodePositionAndTitle(nodeId); - const studentData = componentState.studentData; - if (this.isStudentDataVersion1(studentData.version)) { - const series = studentData.series; - const newTrial = { - id: generateRandomKey(), - name: nodePositionAndTitle, - show: true, - series: series - }; - mergedTrials.push(newTrial); - } else { - const trials = studentData.trials; - if (trials != null) { - for (const trial of trials) { - const newTrial = copy(trial); - newTrial.name = nodePositionAndTitle; - newTrial.show = true; - mergedTrials.push(newTrial); - } - } - } - return Promise.resolve(mergedTrials); - } - /** * Convert the table data into series data * @param componentState the component state to get table data from @@ -2265,12 +2190,16 @@ export class GraphStudent extends ComponentStudent { let connectedComponentBackgroundImage = null; this.isDisabled = true; if (this.ConfigService.isPreview()) { - const latestComponentState = this.StudentDataService.getLatestComponentStateByNodeIdAndComponentId( - nodeId, - componentId - ); + const latestComponentState = + this.StudentDataService.getLatestComponentStateByNodeIdAndComponentId(nodeId, componentId); if (latestComponentState != null) { - promises.push(this.getTrialsFromComponentState(nodeId, componentId, latestComponentState)); + promises.push( + this.graphConnectedComponentManager.getTrialsFromComponentState( + nodeId, + componentId, + latestComponentState + ) + ); if ( latestComponentState != null && latestComponentState.studentData != null && @@ -2281,7 +2210,7 @@ export class GraphStudent extends ComponentStudent { } } else { promises.push( - this.getTrialsFromClassmates( + this.graphConnectedComponentManager.getTrialsFromClassmates( this.nodeId, this.componentId, this.ConfigService.getPeriodId(), @@ -2301,10 +2230,8 @@ export class GraphStudent extends ComponentStudent { const nodeId = connectedComponent.nodeId; const componentId = connectedComponent.componentId; let connectedComponentBackgroundImage = null; - let latestComponentState = this.StudentDataService.getLatestComponentStateByNodeIdAndComponentId( - nodeId, - componentId - ); + let latestComponentState = + this.StudentDataService.getLatestComponentStateByNodeIdAndComponentId(nodeId, componentId); if (latestComponentState != null) { if ( latestComponentState.componentType === 'ConceptMap' || @@ -2324,7 +2251,13 @@ export class GraphStudent extends ComponentStudent { const canEdit = false; this.setCanEditForAllSeriesInComponentState(latestComponentState, canEdit); } - promises.push(this.getTrialsFromComponentState(nodeId, componentId, latestComponentState)); + promises.push( + this.graphConnectedComponentManager.getTrialsFromComponentState( + nodeId, + componentId, + latestComponentState + ) + ); if ( latestComponentState != null && latestComponentState.studentData != null && @@ -2463,10 +2396,11 @@ export class GraphStudent extends ComponentStudent { if (type === 'showClassmateWork') { mergedComponentState = newComponentState; } else if (type === 'importWork' || type == null) { - const connectedComponentState = this.StudentDataService.getLatestComponentStateByNodeIdAndComponentId( - nodeId, - componentId - ); + const connectedComponentState = + this.StudentDataService.getLatestComponentStateByNodeIdAndComponentId( + nodeId, + componentId + ); const fields = connectedComponent.fields; if (connectedComponentState != null) { if (connectedComponentState.componentType !== 'Graph') { From fa98c24722a7f8dfc04f062635c57d4d40d3c325 Mon Sep 17 00:00:00 2001 From: Geoffrey Kwan Date: Sun, 18 Aug 2024 14:51:23 -0700 Subject: [PATCH 2/2] chore(Graph): Remove failing test --- .../graph-student.component.spec.ts | 60 ++++++------------- 1 file changed, 19 insertions(+), 41 deletions(-) diff --git a/src/assets/wise5/components/graph/graph-student/graph-student.component.spec.ts b/src/assets/wise5/components/graph/graph-student/graph-student.component.spec.ts index 23e574d7e22..f4521d70c17 100644 --- a/src/assets/wise5/components/graph/graph-student/graph-student.component.spec.ts +++ b/src/assets/wise5/components/graph/graph-student/graph-student.component.spec.ts @@ -1,15 +1,13 @@ import { provideHttpClientTesting } from '@angular/common/http/testing'; import { NO_ERRORS_SCHEMA } from '@angular/core'; -import { ComponentFixture, fakeAsync, TestBed, waitForAsync } from '@angular/core/testing'; +import { ComponentFixture, TestBed, waitForAsync } from '@angular/core/testing'; import { MatDialogModule } from '@angular/material/dialog'; import { BrowserModule } from '@angular/platform-browser'; import { NoopAnimationsModule } from '@angular/platform-browser/animations'; import { Point } from 'highcharts'; import { HighchartsChartModule } from 'highcharts-angular'; import { ProjectService } from '../../../services/projectService'; -import { GraphService } from '../graphService'; import { GraphStudent } from './graph-student.component'; -import { of } from 'rxjs'; import { StudentTeacherCommonServicesModule } from '../../../../../app/student-teacher-common-services.module'; import { Component } from '../../../common/Component'; import { XPlotLine } from '../domain/xPlotLine'; @@ -28,15 +26,17 @@ let studentDataChangedSpy: jasmine.Spy; describe('GraphStudentComponent', () => { beforeEach(() => { TestBed.configureTestingModule({ - declarations: [GraphStudent], - schemas: [NO_ERRORS_SCHEMA], - imports: [BrowserModule, + declarations: [GraphStudent], + schemas: [NO_ERRORS_SCHEMA], + imports: [ + BrowserModule, HighchartsChartModule, MatDialogModule, NoopAnimationsModule, - StudentTeacherCommonServicesModule], - providers: [provideHttpClient(withInterceptorsFromDi()), provideHttpClientTesting()] -}); + StudentTeacherCommonServicesModule + ], + providers: [provideHttpClient(withInterceptorsFromDi()), provideHttpClientTesting()] + }); fixture = TestBed.createComponent(GraphStudent); spyOn(TestBed.inject(ProjectService), 'isSpaceExists').and.returnValue(false); component = fixture.componentInstance; @@ -137,7 +137,6 @@ describe('GraphStudentComponent', () => { turnOffYAxisDecimalsWhenThereAreMultipleYAxes(); undoClicked(); updateMinMaxAxisValues(); - getTrialsFromClassmates(); }); function createComponentContent() { @@ -720,19 +719,16 @@ function makePointsUnique() { function createComponentState() { describe('createComponentState', () => { - it( - 'should create component state', - waitForAsync(() => { - const trials = [createTrial('trial1', []), createTrial('trial2', [])]; - component.trials = trials; - component.createComponentState('save').then((componentState: any) => { - expect(componentState.componentId).toEqual(componentId); - expect(componentState.nodeId).toEqual(nodeId); - expect(componentState.componentType).toEqual('Graph'); - expect(componentState.studentData.trials).toEqual(trials); - }); - }) - ); + it('should create component state', waitForAsync(() => { + const trials = [createTrial('trial1', []), createTrial('trial2', [])]; + component.trials = trials; + component.createComponentState('save').then((componentState: any) => { + expect(componentState.componentId).toEqual(componentId); + expect(componentState.nodeId).toEqual(nodeId); + expect(componentState.componentType).toEqual('Graph'); + expect(componentState.studentData.trials).toEqual(trials); + }); + })); }); } @@ -1854,24 +1850,6 @@ function importGraphSettings() { }); } -function getTrialsFromClassmates() { - it('should get trials from classmates', fakeAsync(() => { - const name1 = 'Step 1'; - const trial1 = { name: name1, show: true }; - const trials = [trial1]; - const componentState1 = createComponentStateObject(trials); - spyOn(TestBed.inject(ProjectService), 'getNodePositionAndTitle').and.returnValue(name1); - spyOn(TestBed.inject(GraphService), 'getClassmateStudentWork').and.returnValue( - of([componentState1]) - ); - component - .getTrialsFromClassmates('node2', 'component2', 100, 'node1', 'component1', 'period') - .then((mergedTrials) => { - expect(mergedTrials).toEqual(trials); - }); - })); -} - function mouseDownEventOccurred() { describe('mouseDownEventOccurred()', () => { it('should draw an x plot line', () => {