From 1ea9623efcae7cfad7ab311f5ae2102156bd204e Mon Sep 17 00:00:00 2001 From: Kristof De Langhe Date: Fri, 13 Mar 2020 13:18:20 +0100 Subject: [PATCH] 68346: Review - TypeDocs and Test cases --- .../bitstream-page-routing.module.ts | 3 + .../+bitstream-page/bitstream-page.module.ts | 3 + .../upload/upload-bitstream.component.ts | 8 ++ .../item-edit-bitstream-bundle.component.ts | 2 + ...em-edit-bitstream-drag-handle.component.ts | 5 + .../item-edit-bitstream.component.ts | 2 + ...array-move-change-analyzer.service.spec.ts | 107 ++++++++++++++++++ src/app/core/data/data.service.spec.ts | 4 +- .../object-updates.service.spec.ts | 105 ++++++++++++++++- src/app/core/shared/item-bitstreams-utils.ts | 16 --- .../custom-switch.component.spec.ts | 99 ++++++++++++++++ .../custom-switch/custom-switch.component.ts | 35 ++++++ .../custom-switch/custom-switch.model.ts | 4 + 13 files changed, 374 insertions(+), 19 deletions(-) create mode 100644 src/app/core/data/array-move-change-analyzer.service.spec.ts delete mode 100644 src/app/core/shared/item-bitstreams-utils.ts create mode 100644 src/app/shared/form/builder/ds-dynamic-form-ui/models/custom-switch/custom-switch.component.spec.ts diff --git a/src/app/+bitstream-page/bitstream-page-routing.module.ts b/src/app/+bitstream-page/bitstream-page-routing.module.ts index 790530abee..14d688064c 100644 --- a/src/app/+bitstream-page/bitstream-page-routing.module.ts +++ b/src/app/+bitstream-page/bitstream-page-routing.module.ts @@ -6,6 +6,9 @@ import { BitstreamPageResolver } from './bitstream-page.resolver'; const EDIT_BITSTREAM_PATH = ':id/edit'; +/** + * Routing module to help navigate Bitstream pages + */ @NgModule({ imports: [ RouterModule.forChild([ diff --git a/src/app/+bitstream-page/bitstream-page.module.ts b/src/app/+bitstream-page/bitstream-page.module.ts index 63656752c9..24b4cd512f 100644 --- a/src/app/+bitstream-page/bitstream-page.module.ts +++ b/src/app/+bitstream-page/bitstream-page.module.ts @@ -4,6 +4,9 @@ import { SharedModule } from '../shared/shared.module'; import { EditBitstreamPageComponent } from './edit-bitstream-page/edit-bitstream-page.component'; import { BitstreamPageRoutingModule } from './bitstream-page-routing.module'; +/** + * This module handles all components that are necessary for Bitstream related pages + */ @NgModule({ imports: [ CommonModule, diff --git a/src/app/+item-page/bitstreams/upload/upload-bitstream.component.ts b/src/app/+item-page/bitstreams/upload/upload-bitstream.component.ts index fb0ca79d4d..8b25384bd0 100644 --- a/src/app/+item-page/bitstreams/upload/upload-bitstream.component.ts +++ b/src/app/+item-page/bitstreams/upload/upload-bitstream.component.ts @@ -69,6 +69,14 @@ export class UploadBitstreamComponent implements OnInit, OnDestroy { protected translate: TranslateService) { } + /** + * Initialize component properties: + * itemRD$ Fetched from the current route data (populated by BitstreamPageResolver) + * bundlesRD$ List of bundles on the item + * selectedBundleId Starts off by checking if the route's queryParams contain a "bundle" parameter. If none is found, + * the ID of the first bundle in the list is selected. + * Calls setUploadUrl after setting the selected bundle + */ ngOnInit(): void { this.itemRD$ = this.route.data.pipe(map((data) => data.item)); this.bundlesRD$ = this.itemRD$.pipe( diff --git a/src/app/+item-page/edit-item-page/item-bitstreams/item-edit-bitstream-bundle/item-edit-bitstream-bundle.component.ts b/src/app/+item-page/edit-item-page/item-bitstreams/item-edit-bitstream-bundle/item-edit-bitstream-bundle.component.ts index 1f50594685..115e326241 100644 --- a/src/app/+item-page/edit-item-page/item-bitstreams/item-edit-bitstream-bundle/item-edit-bitstream-bundle.component.ts +++ b/src/app/+item-page/edit-item-page/item-bitstreams/item-edit-bitstream-bundle/item-edit-bitstream-bundle.component.ts @@ -11,6 +11,8 @@ import { ResponsiveTableSizes } from '../../../../shared/responsive-table-sizes/ }) /** * Component that displays a single bundle of an item on the item bitstreams edit page + * Creates an embedded view of the contents. This is to ensure the table structure won't break. + * (which means it'll be added to the parents html without a wrapping ds-item-edit-bitstream-bundle element) */ export class ItemEditBitstreamBundleComponent implements OnInit { diff --git a/src/app/+item-page/edit-item-page/item-bitstreams/item-edit-bitstream-drag-handle/item-edit-bitstream-drag-handle.component.ts b/src/app/+item-page/edit-item-page/item-bitstreams/item-edit-bitstream-drag-handle/item-edit-bitstream-drag-handle.component.ts index 3610fe4603..e6d72cbd57 100644 --- a/src/app/+item-page/edit-item-page/item-bitstreams/item-edit-bitstream-drag-handle/item-edit-bitstream-drag-handle.component.ts +++ b/src/app/+item-page/edit-item-page/item-bitstreams/item-edit-bitstream-drag-handle/item-edit-bitstream-drag-handle.component.ts @@ -5,6 +5,11 @@ import { Component, OnInit, TemplateRef, ViewChild, ViewContainerRef } from '@an styleUrls: ['../item-bitstreams.component.scss'], templateUrl: './item-edit-bitstream-drag-handle.component.html', }) +/** + * Component displaying a drag handle for the item-edit-bitstream page + * Creates an embedded view of the contents + * (which means it'll be added to the parents html without a wrapping ds-item-edit-bitstream-drag-handle element) + */ export class ItemEditBitstreamDragHandleComponent implements OnInit { /** * The view on the drag-handle diff --git a/src/app/+item-page/edit-item-page/item-bitstreams/item-edit-bitstream/item-edit-bitstream.component.ts b/src/app/+item-page/edit-item-page/item-bitstreams/item-edit-bitstream/item-edit-bitstream.component.ts index 87aab109a0..5a02b9cac4 100644 --- a/src/app/+item-page/edit-item-page/item-bitstreams/item-edit-bitstream/item-edit-bitstream.component.ts +++ b/src/app/+item-page/edit-item-page/item-bitstreams/item-edit-bitstream/item-edit-bitstream.component.ts @@ -17,6 +17,8 @@ import { DSONameService } from '../../../../core/breadcrumbs/dso-name.service'; }) /** * Component that displays a single bitstream of an item on the edit page + * Creates an embedded view of the contents + * (which means it'll be added to the parents html without a wrapping ds-item-edit-bitstream element) */ export class ItemEditBitstreamComponent implements OnChanges, OnInit { diff --git a/src/app/core/data/array-move-change-analyzer.service.spec.ts b/src/app/core/data/array-move-change-analyzer.service.spec.ts new file mode 100644 index 0000000000..5f5388d935 --- /dev/null +++ b/src/app/core/data/array-move-change-analyzer.service.spec.ts @@ -0,0 +1,107 @@ +import { ArrayMoveChangeAnalyzer } from './array-move-change-analyzer.service'; +import { moveItemInArray } from '@angular/cdk/drag-drop'; +import { Operation } from 'fast-json-patch'; + +/** + * Helper class for creating move tests + * Define a "from" and "to" index to move objects within the array before comparing + */ +class MoveTest { + from: number; + to: number; + + constructor(from: number, to: number) { + this.from = from; + this.to = to; + } +} + +describe('ArrayMoveChangeAnalyzer', () => { + const comparator = new ArrayMoveChangeAnalyzer(); + + let originalArray = []; + + describe('when all values are defined', () => { + beforeEach(() => { + originalArray = [ + '98700118-d65d-4636-b1d0-dba83fc932e1', + '4d7d0798-a8fa-45b8-b4fc-deb2819606c8', + 'e56eb99e-2f7c-4bee-9b3f-d3dcc83386b1', + '0f608168-cdfc-46b0-92ce-889f7d3ac684', + '546f9f5c-15dc-4eec-86fe-648007ac9e1c' + ]; + }); + + testMove([ + { op: 'move', from: '/2', path: '/4' }, + ], new MoveTest(2, 4)); + + testMove([ + { op: 'move', from: '/0', path: '/3' }, + ], new MoveTest(0, 3)); + + testMove([ + { op: 'move', from: '/0', path: '/3' }, + { op: 'move', from: '/2', path: '/1' } + ], new MoveTest(0, 3), new MoveTest(1, 2)); + + testMove([ + { op: 'move', from: '/0', path: '/1' }, + { op: 'move', from: '/3', path: '/4' } + ], new MoveTest(0, 1), new MoveTest(3, 4)); + + testMove([], new MoveTest(0, 4), new MoveTest(4, 0)); + + testMove([ + { op: 'move', from: '/0', path: '/3' }, + { op: 'move', from: '/2', path: '/1' } + ], new MoveTest(0, 4), new MoveTest(1, 3), new MoveTest(2, 4)); + }); + + describe('when some values are undefined (index 2 and 3)', () => { + beforeEach(() => { + originalArray = [ + '98700118-d65d-4636-b1d0-dba83fc932e1', + '4d7d0798-a8fa-45b8-b4fc-deb2819606c8', + undefined, + undefined, + '546f9f5c-15dc-4eec-86fe-648007ac9e1c' + ]; + }); + + // It can't create a move operation for undefined values, so it should create move operations for the defined values instead + testMove([ + { op: 'move', from: '/4', path: '/3' }, + ], new MoveTest(2, 4)); + + // Moving a defined value should result in the same operations + testMove([ + { op: 'move', from: '/0', path: '/3' }, + ], new MoveTest(0, 3)); + }); + + /** + * Helper function for creating a move test + * + * @param expectedOperations An array of expected operations after comparing the original array with the array + * created using the provided MoveTests + * @param moves An array of MoveTest objects telling the test where to move objects before comparing + */ + function testMove(expectedOperations: Operation[], ...moves: MoveTest[]) { + describe(`move ${moves.map((move) => `${move.from} to ${move.to}`).join(' and ')}`, () => { + let result; + + beforeEach(() => { + const movedArray = [...originalArray]; + moves.forEach((move) => { + moveItemInArray(movedArray, move.from, move.to); + }); + result = comparator.diff(originalArray, movedArray); + }); + + it('should create the expected move operations', () => { + expect(result).toEqual(expectedOperations); + }); + }); + } +}); diff --git a/src/app/core/data/data.service.spec.ts b/src/app/core/data/data.service.spec.ts index 626af323cc..6dcb7df837 100644 --- a/src/app/core/data/data.service.spec.ts +++ b/src/app/core/data/data.service.spec.ts @@ -302,7 +302,7 @@ describe('DataService', () => { }); }); - describe('immediatePatch', () => { + describe('patch', () => { const dso = { uuid: 'dso-uuid' }; @@ -315,7 +315,7 @@ describe('DataService', () => { ]; beforeEach(() => { - service.immediatePatch(dso, operations); + service.patch(dso, operations); }); it('should configure a PatchRequest', () => { diff --git a/src/app/core/data/object-updates/object-updates.service.spec.ts b/src/app/core/data/object-updates/object-updates.service.spec.ts index 9de966268d..780a402a84 100644 --- a/src/app/core/data/object-updates/object-updates.service.spec.ts +++ b/src/app/core/data/object-updates/object-updates.service.spec.ts @@ -13,6 +13,8 @@ import { Notification } from '../../../shared/notifications/models/notification. import { NotificationType } from '../../../shared/notifications/models/notification-type'; import { OBJECT_UPDATES_TRASH_PATH } from './object-updates.reducer'; import {Relationship} from '../../shared/item-relationships/relationship.model'; +import { MoveOperation } from 'fast-json-patch/lib/core'; +import { ArrayMoveChangeAnalyzer } from '../array-move-change-analyzer.service'; describe('ObjectUpdatesService', () => { let service: ObjectUpdatesService; @@ -45,7 +47,7 @@ describe('ObjectUpdatesService', () => { }; store = new Store(undefined, undefined, undefined); spyOn(store, 'dispatch'); - service = new ObjectUpdatesService(store, undefined); + service = new ObjectUpdatesService(store, new ArrayMoveChangeAnalyzer()); spyOn(service as any, 'getObjectEntry').and.returnValue(observableOf(objectEntry)); spyOn(service as any, 'getFieldState').and.callFake((uuid) => { @@ -97,6 +99,66 @@ describe('ObjectUpdatesService', () => { }); }); + describe('getFieldUpdatesExclusive', () => { + it('should return the list of all fields, including their update if there is one, excluding updates that aren\'t part of the initial values provided', (done) => { + const result$ = service.getFieldUpdatesExclusive(url, identifiables); + expect((service as any).getObjectEntry).toHaveBeenCalledWith(url); + + const expectedResult = { + [identifiable1.uuid]: { field: identifiable1Updated, changeType: FieldChangeType.UPDATE }, + [identifiable2.uuid]: { field: identifiable2, changeType: undefined } + }; + + result$.subscribe((result) => { + expect(result).toEqual(expectedResult); + done(); + }); + }); + }); + + describe('getFieldUpdatesByCustomOrder', () => { + beforeEach(() => { + const fieldStates = { + [identifiable1.uuid]: { editable: false, isNew: false, isValid: true }, + [identifiable2.uuid]: { editable: true, isNew: false, isValid: false }, + [identifiable3.uuid]: { editable: true, isNew: true, isValid: true }, + }; + + const customOrder = { + initialOrderPages: [{ + order: [identifiable1.uuid, identifiable2.uuid, identifiable3.uuid] + }], + newOrderPages: [{ + order: [identifiable2.uuid, identifiable3.uuid, identifiable1.uuid] + }], + pageSize: 20, + changed: true + }; + + const objectEntry = { + fieldStates, fieldUpdates, lastModified: modDate, virtualMetadataSources: {}, customOrder + }; + + (service as any).getObjectEntry.and.returnValue(observableOf(objectEntry)) + }); + + it('should return the list of all fields, including their update if there is one, ordered by their custom order', (done) => { + const result$ = service.getFieldUpdatesByCustomOrder(url, identifiables); + expect((service as any).getObjectEntry).toHaveBeenCalledWith(url); + + const expectedResult = { + [identifiable2.uuid]: { field: identifiable2, changeType: undefined }, + [identifiable3.uuid]: { field: identifiable3, changeType: FieldChangeType.ADD }, + [identifiable1.uuid]: { field: identifiable1Updated, changeType: FieldChangeType.UPDATE } + }; + + result$.subscribe((result) => { + expect(result).toEqual(expectedResult); + done(); + }); + }); + }); + describe('isEditable', () => { it('should return false if this identifiable is currently not editable in the store', () => { const result$ = service.isEditable(url, identifiable1.uuid); @@ -283,4 +345,45 @@ describe('ObjectUpdatesService', () => { expect(store.dispatch).toHaveBeenCalledWith(new SelectVirtualMetadataAction(url, relationship.uuid, identifiable1.uuid, true)); }); }); + + describe('getMoveOperations', () => { + beforeEach(() => { + const fieldStates = { + [identifiable1.uuid]: { editable: false, isNew: false, isValid: true }, + [identifiable2.uuid]: { editable: true, isNew: false, isValid: false }, + [identifiable3.uuid]: { editable: true, isNew: true, isValid: true }, + }; + + const customOrder = { + initialOrderPages: [{ + order: [identifiable1.uuid, identifiable2.uuid, identifiable3.uuid] + }], + newOrderPages: [{ + order: [identifiable2.uuid, identifiable3.uuid, identifiable1.uuid] + }], + pageSize: 20, + changed: true + }; + + const objectEntry = { + fieldStates, fieldUpdates, lastModified: modDate, virtualMetadataSources: {}, customOrder + }; + + (service as any).getObjectEntry.and.returnValue(observableOf(objectEntry)) + }); + + it('should return the expected move operations', (done) => { + const result$ = service.getMoveOperations(url); + + const expectedResult = [ + { op: 'move', from: '/0', path: '/2' } + ] as MoveOperation[]; + + result$.subscribe((result) => { + expect(result).toEqual(expectedResult); + done(); + }); + }); + }); + }); diff --git a/src/app/core/shared/item-bitstreams-utils.ts b/src/app/core/shared/item-bitstreams-utils.ts deleted file mode 100644 index 2d825f1208..0000000000 --- a/src/app/core/shared/item-bitstreams-utils.ts +++ /dev/null @@ -1,16 +0,0 @@ -import { Observable } from 'rxjs/internal/Observable'; -import { RemoteData } from '../data/remote-data'; -import { PaginatedList } from '../data/paginated-list'; -import { Bitstream } from './bitstream.model'; -import { map } from 'rxjs/operators'; -import { hasValue, hasValueOperator } from '../../shared/empty.util'; - -/** - * Operator for turning the current page of bitstreams into an array - */ -export const toBitstreamsArray = () => - (source: Observable>>): Observable => - source.pipe( - hasValueOperator(), - map((bitstreamRD: RemoteData>) => bitstreamRD.payload.page.filter((bitstream: Bitstream) => hasValue(bitstream))) - ); diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/models/custom-switch/custom-switch.component.spec.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/models/custom-switch/custom-switch.component.spec.ts new file mode 100644 index 0000000000..6c2502a92b --- /dev/null +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/models/custom-switch/custom-switch.component.spec.ts @@ -0,0 +1,99 @@ +import { DynamicFormsCoreModule, DynamicFormService } from '@ng-dynamic-forms/core'; +import { FormGroup, ReactiveFormsModule } from '@angular/forms'; +import { async, ComponentFixture, inject, TestBed } from '@angular/core/testing'; +import { DebugElement } from '@angular/core'; +import { NoopAnimationsModule } from '@angular/platform-browser/animations'; +import { TextMaskModule } from 'angular2-text-mask'; +import { By } from '@angular/platform-browser'; +import { DynamicCustomSwitchModel } from './custom-switch.model'; +import { CustomSwitchComponent } from './custom-switch.component'; + +describe('CustomSwitchComponent', () => { + + const testModel = new DynamicCustomSwitchModel({id: 'switch'}); + const formModel = [testModel]; + let formGroup: FormGroup; + let fixture: ComponentFixture; + let component: CustomSwitchComponent; + let debugElement: DebugElement; + let testElement: DebugElement; + + beforeEach(async(() => { + TestBed.configureTestingModule({ + imports: [ + ReactiveFormsModule, + NoopAnimationsModule, + TextMaskModule, + DynamicFormsCoreModule.forRoot() + ], + declarations: [CustomSwitchComponent] + + }).compileComponents().then(() => { + fixture = TestBed.createComponent(CustomSwitchComponent); + + component = fixture.componentInstance; + debugElement = fixture.debugElement; + }); + })); + + beforeEach(inject([DynamicFormService], (service: DynamicFormService) => { + formGroup = service.createFormGroup(formModel); + + component.group = formGroup; + component.model = testModel; + + fixture.detectChanges(); + + testElement = debugElement.query(By.css(`input[id='${testModel.id}']`)); + })); + + it('should initialize correctly', () => { + expect(component.bindId).toBe(true); + expect(component.group instanceof FormGroup).toBe(true); + expect(component.model instanceof DynamicCustomSwitchModel).toBe(true); + + expect(component.blur).toBeDefined(); + expect(component.change).toBeDefined(); + expect(component.focus).toBeDefined(); + + expect(component.onBlur).toBeDefined(); + expect(component.onChange).toBeDefined(); + expect(component.onFocus).toBeDefined(); + + expect(component.hasFocus).toBe(false); + expect(component.isValid).toBe(true); + expect(component.isInvalid).toBe(false); + }); + + it('should have an input element', () => { + expect(testElement instanceof DebugElement).toBe(true); + }); + + it('should have an input element of type checkbox', () => { + expect(testElement.nativeElement.getAttribute('type')).toEqual('checkbox'); + }); + + it('should emit blur event', () => { + spyOn(component.blur, 'emit'); + + component.onBlur(null); + + expect(component.blur.emit).toHaveBeenCalled(); + }); + + it('should emit change event', () => { + spyOn(component.change, 'emit'); + + component.onChange(null); + + expect(component.change.emit).toHaveBeenCalled(); + }); + + it('should emit focus event', () => { + spyOn(component.focus, 'emit'); + + component.onFocus(null); + + expect(component.focus.emit).toHaveBeenCalled(); + }); +}); diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/models/custom-switch/custom-switch.component.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/models/custom-switch/custom-switch.component.ts index 5bab244587..ab02fc159d 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/models/custom-switch/custom-switch.component.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/models/custom-switch/custom-switch.component.ts @@ -8,13 +8,48 @@ import { DynamicCustomSwitchModel } from './custom-switch.model'; styleUrls: ['./custom-switch.component.scss'], templateUrl: './custom-switch.component.html', }) +/** + * Component displaying a custom switch usable in dynamic forms + * Extends from bootstrap's checkbox component but displays a switch instead + */ export class CustomSwitchComponent extends DynamicNGBootstrapCheckboxComponent { + /** + * Use the model's ID for the input element + */ @Input() bindId = true; + + /** + * The formgroup containing this component + */ @Input() group: FormGroup; + + /** + * The model used for displaying the switch + */ @Input() model: DynamicCustomSwitchModel; + + /** + * Emit an event when the input is selected + */ @Output() selected = new EventEmitter(); + + /** + * Emit an event when the input value is removed + */ @Output() remove = new EventEmitter(); + + /** + * Emit an event when the input is blurred out + */ @Output() blur = new EventEmitter(); + + /** + * Emit an event when the input value changes + */ @Output() change = new EventEmitter(); + + /** + * Emit an event when the input is focused + */ @Output() focus = new EventEmitter(); } diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/models/custom-switch/custom-switch.model.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/models/custom-switch/custom-switch.model.ts index 73360caa3e..97cf71c4a0 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/models/custom-switch/custom-switch.model.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/models/custom-switch/custom-switch.model.ts @@ -7,6 +7,10 @@ import { export const DYNAMIC_FORM_CONTROL_TYPE_CUSTOM_SWITCH = 'CUSTOM_SWITCH'; +/** + * Model class for displaying a custom switch input in a form + * Functions like a checkbox, but displays a switch instead + */ export class DynamicCustomSwitchModel extends DynamicCheckboxModel { @serializable() readonly type: string = DYNAMIC_FORM_CONTROL_TYPE_CUSTOM_SWITCH;