Skip to content

Commit

Permalink
[ACS-6190] prefer-promise-reject-errors rule and fixes (#9021)
Browse files Browse the repository at this point in the history 
* fix promise rejections

* fix promise errors

* promise error fixes

* fix promise rejections

* [ci:force] fix formatting

* test fixes

* [ci:force] fix tests

* [ci:force] fix tests

* fix incorrect return types

* meaningful errors

* remove useless pipe

* fix accessing private members in the test
  • Loading branch information
@DenysVuika
DenysVuika authored Oct 25, 2023
1 parent 352e0e4 commit 8bd24db
Show file tree
Hide file tree
Showing 18 changed files with 155 additions and 189 deletions.
2 changes: 1 addition & 1 deletion .eslintrc.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ module.exports = {
],
'@typescript-eslint/member-ordering': 'off',
'prefer-arrow/prefer-arrow-functions': 'off',

'prefer-promise-reject-errors': 'error',
'brace-style': 'off',
'@typescript-eslint/brace-style': 'error',
'comma-dangle': 'error',
Expand Down
17 changes: 5 additions & 12 deletions lib/content-services/src/lib/common/services/people-content.service.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,14 @@ import {
AlfrescoApiService,
AlfrescoApiServiceMock,
AuthenticationService,
CoreTestingModule,
LogService
CoreTestingModule
} from '@alfresco/adf-core';
import { PeopleContentQueryRequestModel, PeopleContentService } from './people-content.service';
import { TranslateModule } from '@ngx-translate/core';
import { TestBed } from '@angular/core/testing';

describe('PeopleContentService', () => {

let peopleContentService: PeopleContentService;
let logService: LogService;
let authenticationService: AuthenticationService;

beforeEach(() => {
Expand All @@ -52,7 +49,6 @@ describe('PeopleContentService', () => {

authenticationService = TestBed.inject(AuthenticationService);
peopleContentService = TestBed.inject(PeopleContentService);
logService = TestBed.inject(LogService);
});

it('should be able to fetch person details based on id', async () => {
Expand Down Expand Up @@ -110,14 +106,11 @@ describe('PeopleContentService', () => {
});

it('should be able to throw an error if createPerson api failed', (done) => {
spyOn(peopleContentService.peopleApi, 'createPerson').and.returnValue(Promise.reject('failed to create new person'));
const logErrorSpy = spyOn(logService, 'error');
spyOn(peopleContentService.peopleApi, 'createPerson').and.returnValue(Promise.reject(new Error('failed to create new person')));
peopleContentService.createPerson(createNewPersonMock).subscribe(
() => {
},
(error) => {
expect(logErrorSpy).toHaveBeenCalledWith('failed to create new person');
expect(error).toEqual('failed to create new person');
() => {},
(error: Error) => {
expect(error.message).toEqual('failed to create new person');
done();
}
);
Expand Down
25 changes: 8 additions & 17 deletions lib/content-services/src/lib/common/services/people-content.service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@
*/

import { Injectable } from '@angular/core';
import { from, Observable, of, throwError } from 'rxjs';
import { AlfrescoApiService, AuthenticationService, LogService } from '@alfresco/adf-core';
import { catchError, map, tap } from 'rxjs/operators';
import { from, Observable, of } from 'rxjs';
import { AlfrescoApiService, AuthenticationService } from '@alfresco/adf-core';
import { map, tap } from 'rxjs/operators';
import { Pagination, PeopleApi, PersonBodyCreate, PersonBodyUpdate } from '@alfresco/js-api';
import { EcmUserModel } from '../models/ecm-user.model';
import { ContentService } from './content.service';
Expand Down Expand Up @@ -59,7 +59,6 @@ export class PeopleContentService {
constructor(
private apiService: AlfrescoApiService,
authenticationService: AuthenticationService,
private logService: LogService,
private contentService: ContentService
) {
authenticationService.onLogout.subscribe(() => {
Expand All @@ -76,8 +75,8 @@ export class PeopleContentService {
getPerson(personId: string): Observable<EcmUserModel> {
return from(this.peopleApi.getPerson(personId))
.pipe(
map((personEntry) => new EcmUserModel(personEntry.entry)),
catchError((error) => this.handleError(error)));
map((personEntry) => new EcmUserModel(personEntry.entry))
);
}

getCurrentPerson(): Observable<EcmUserModel> {
Expand Down Expand Up @@ -130,8 +129,7 @@ export class PeopleContentService {
map(response => ({
pagination: response.list.pagination,
entries: response.list.entries.map((person) => person.entry as EcmUserModel)
})),
catchError((err) => this.handleError(err))
}))
);
}

Expand All @@ -144,8 +142,7 @@ export class PeopleContentService {
*/
createPerson(newPerson: PersonBodyCreate, opts?: any): Observable<EcmUserModel> {
return from(this.peopleApi.createPerson(newPerson, opts)).pipe(
map((res) => res?.entry as EcmUserModel),
catchError((error) => this.handleError(error))
map((res) => res?.entry as EcmUserModel)
);
}

Expand All @@ -159,8 +156,7 @@ export class PeopleContentService {
*/
updatePerson(personId: string, details: PersonBodyUpdate, opts?: any): Observable<EcmUserModel> {
return from(this.peopleApi.updatePerson(personId, details, opts)).pipe(
map((res) => res?.entry as EcmUserModel),
catchError((error) => this.handleError(error))
map((res) => res?.entry as EcmUserModel)
);
}

Expand All @@ -177,9 +173,4 @@ export class PeopleContentService {
private buildOrderArray(sorting: PeopleContentSortingModel): string[] {
return sorting?.orderBy && sorting?.direction ? [`${sorting.orderBy} ${sorting.direction.toUpperCase()}`] : [];
}

private handleError(error: any) {
this.logService.error(error);
return throwError(error || 'Server error');
}
}
7 changes: 4 additions & 3 deletions lib/content-services/src/lib/common/services/rendition.service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ export class RenditionService {
clearInterval(intervalId);
return resolve(this.handleNodeRendition(nodeId, rendition.entry.content.mimeType, versionId));
}
}, () => reject());
}, () => reject(new Error('Error geting version rendition')));
} else {
this.renditionsApi.getRendition(nodeId, renditionId).then((rendition: RenditionEntry) => {
const status: string = rendition.entry.status.toString();
Expand All @@ -210,11 +210,11 @@ export class RenditionService {
clearInterval(intervalId);
return resolve(this.handleNodeRendition(nodeId, renditionId, versionId));
}
}, () => reject());
}, () => reject(new Error('Error getting rendition')));
}
} else {
clearInterval(intervalId);
return reject();
return reject(new Error('Error getting rendition'));
}
}, this.TRY_TIMEOUT);
});
Expand Down Expand Up @@ -287,6 +287,7 @@ export class RenditionService {
* These are: images, PDF files, or PDF rendition of files.
* We also force PDF rendition for TEXT type objects, otherwise the default URL is to download.
* TODO there are different TEXT type objects, (HTML, plaintext, xml, etc. we should determine how these are handled)
*
* @param objectId object it
* @param mimeType mime type
*/
Expand Down
30 changes: 14 additions & 16 deletions lib/content-services/src/lib/common/services/rendition.services.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,22 +20,20 @@ import { AlfrescoApiService, LogService, TranslationService, ViewUtilService } f
import { Rendition, RenditionEntry, RenditionPaging, RenditionsApi } from '@alfresco/js-api';
import { RenditionService } from '@alfresco/adf-content-services';

const getRenditionEntry = (status: Rendition.StatusEnum): RenditionEntry => {
return {
entry: {
id: 'pdf',
status: status,
content: { mimeType: 'application/pdf', mimeTypeName: 'application/pdf', sizeInBytes: 10 }
}
};
};
const getRenditionPaging = (status: Rendition.StatusEnum): RenditionPaging => {
return {
list: {
entries: [getRenditionEntry(status)]
}
};
};
const getRenditionEntry = (status: Rendition.StatusEnum): RenditionEntry => ({
entry: {
id: 'pdf',
status,
content: { mimeType: 'application/pdf', mimeTypeName: 'application/pdf', sizeInBytes: 10 }
}
});

const getRenditionPaging = (status: Rendition.StatusEnum): RenditionPaging => ({
list: {
entries: [getRenditionEntry(status)]
}
});

describe('RenditionService', () => {
let renditionService: RenditionService;
let renditionsApi: any;
Expand Down
2 changes: 1 addition & 1 deletion lib/content-services/src/lib/dialogs/node-lock.dialog.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ describe('NodeLockDialogComponent', () => {
}));

it('should call onError if submit fails', fakeAsync(() => {
spyOn(component.nodesApi, 'lockNode').and.returnValue(Promise.reject('error'));
spyOn(component.nodesApi, 'lockNode').and.returnValue(Promise.reject(new Error('error')));
spyOn(component.data, 'onError');

component.submit();
Expand Down
24 changes: 12 additions & 12 deletions lib/content-services/src/lib/directives/library-favorite.directive.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,39 +53,39 @@ describe('LibraryFavoriteDirective', () => {
});

it('should not check for favorite if no selection exists', () => {
spyOn(component.directive['favoritesApi'], 'getFavoriteSite');
spyOn(component.directive.favoritesApi, 'getFavoriteSite');
fixture.detectChanges();

expect(component.directive['favoritesApi'].getFavoriteSite).not.toHaveBeenCalled();
expect(component.directive.favoritesApi.getFavoriteSite).not.toHaveBeenCalled();
});

it('should mark selection as favorite', async () => {
spyOn(component.directive['favoritesApi'], 'getFavoriteSite').and.returnValue(Promise.resolve(null));
spyOn(component.directive.favoritesApi, 'getFavoriteSite').and.returnValue(Promise.resolve(null));

delete selection.isFavorite;

fixture.detectChanges();
await fixture.whenStable();

expect(component.directive['favoritesApi'].getFavoriteSite).toHaveBeenCalled();
expect(component.directive.favoritesApi.getFavoriteSite).toHaveBeenCalled();
expect(component.directive.isFavorite()).toBe(true);
});

it('should mark selection not favorite', async () => {
spyOn(component.directive['favoritesApi'], 'getFavoriteSite').and.returnValue(Promise.reject());
spyOn(component.directive.favoritesApi, 'getFavoriteSite').and.returnValue(Promise.reject(new Error('error')));

delete selection.isFavorite;

fixture.detectChanges();
await fixture.whenStable();

expect(component.directive['favoritesApi'].getFavoriteSite).toHaveBeenCalled();
expect(component.directive.favoritesApi.getFavoriteSite).toHaveBeenCalled();
expect(component.directive.isFavorite()).toBe(false);
});

it('should call addFavorite() on click event when selection is not a favorite', async () => {
spyOn(component.directive['favoritesApi'], 'getFavoriteSite').and.returnValue(Promise.reject());
spyOn(component.directive['favoritesApi'], 'createFavorite').and.returnValue(Promise.resolve(null));
spyOn(component.directive.favoritesApi, 'getFavoriteSite').and.returnValue(Promise.reject(new Error('error')));
spyOn(component.directive.favoritesApi, 'createFavorite').and.returnValue(Promise.resolve(null));

fixture.detectChanges();
await fixture.whenStable();
Expand All @@ -94,12 +94,12 @@ describe('LibraryFavoriteDirective', () => {

fixture.nativeElement.querySelector('button').dispatchEvent(new MouseEvent('click'));
fixture.detectChanges();
expect(component.directive['favoritesApi'].createFavorite).toHaveBeenCalled();
expect(component.directive.favoritesApi.createFavorite).toHaveBeenCalled();
});

it('should call removeFavoriteSite() on click event when selection is favorite', async () => {
spyOn(component.directive['favoritesApi'], 'getFavoriteSite').and.returnValue(Promise.resolve(null));
spyOn(component.directive['favoritesApi'], 'deleteFavorite').and.returnValue(Promise.resolve());
spyOn(component.directive.favoritesApi, 'getFavoriteSite').and.returnValue(Promise.resolve(null));
spyOn(component.directive.favoritesApi, 'deleteFavorite').and.returnValue(Promise.resolve());

selection.isFavorite = true;

Expand All @@ -113,6 +113,6 @@ describe('LibraryFavoriteDirective', () => {
fixture.detectChanges();
await fixture.whenStable();

expect(component.directive['favoritesApi'].deleteFavorite).toHaveBeenCalled();
expect(component.directive.favoritesApi.deleteFavorite).toHaveBeenCalled();
});
});
10 changes: 5 additions & 5 deletions lib/content-services/src/lib/directives/library-membership.directive.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ describe('LibraryMembershipDirective', () => {

describe('markMembershipRequest', () => {
beforeEach(() => {
getMembershipSpy = spyOn(directive['sitesApi'], 'getSiteMembershipRequestForPerson').and.returnValue(Promise.resolve({ entry: requestedMembershipResponse }));
getMembershipSpy = spyOn(directive.sitesApi, 'getSiteMembershipRequestForPerson').and.returnValue(Promise.resolve({ entry: requestedMembershipResponse }));
});

it('should not check membership requests if no entry is selected', fakeAsync(() => {
Expand Down Expand Up @@ -98,7 +98,7 @@ describe('LibraryMembershipDirective', () => {
}));

it('should remember when a membership request is not found for selected library', fakeAsync(() => {
getMembershipSpy.and.returnValue(Promise.reject());
getMembershipSpy.and.returnValue(Promise.reject(new Error('error')));

const selection = { entry: testSiteEntry };
const change = new SimpleChange(null, selection, true);
Expand All @@ -111,9 +111,9 @@ describe('LibraryMembershipDirective', () => {
describe('toggleMembershipRequest', () => {
beforeEach(() => {
mockSupportedVersion = false;
getMembershipSpy = spyOn(directive['sitesApi'], 'getSiteMembershipRequestForPerson').and.returnValue(Promise.resolve({ entry: requestedMembershipResponse }));
addMembershipSpy = spyOn(directive['sitesApi'], 'createSiteMembershipRequestForPerson').and.returnValue(Promise.resolve({ entry: requestedMembershipResponse }));
deleteMembershipSpy = spyOn(directive['sitesApi'], 'deleteSiteMembershipRequestForPerson').and.returnValue(Promise.resolve());
getMembershipSpy = spyOn(directive.sitesApi, 'getSiteMembershipRequestForPerson').and.returnValue(Promise.resolve({ entry: requestedMembershipResponse }));
addMembershipSpy = spyOn(directive.sitesApi, 'createSiteMembershipRequestForPerson').and.returnValue(Promise.resolve({ entry: requestedMembershipResponse }));
deleteMembershipSpy = spyOn(directive.sitesApi, 'deleteSiteMembershipRequestForPerson').and.returnValue(Promise.resolve());
});

it('should do nothing if there is no selected library ', fakeAsync(() => {
Expand Down
14 changes: 7 additions & 7 deletions lib/content-services/src/lib/directives/node-delete.directive.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,10 +111,10 @@ describe('NodeDeleteDirective', () => {
element = fixture.debugElement.query(By.directive(NodeDeleteDirective));
elementWithPermanentDelete = fixtureWithPermanentComponent.debugElement.query(By.directive(NodeDeleteDirective));

deleteNodeSpy = spyOn(component.deleteDirective['nodesApi'], 'deleteNode').and.returnValue(Promise.resolve());
deleteNodeSpy = spyOn(component.deleteDirective.nodesApi, 'deleteNode').and.returnValue(Promise.resolve());

deleteNodePermanentSpy = spyOn(componentWithPermanentDelete.deleteDirective['nodesApi'], 'deleteNode').and.returnValue(Promise.resolve());
purgeDeletedNodePermanentSpy = spyOn(componentWithPermanentDelete.deleteDirective['trashcanApi'], 'deleteDeletedNode').and.returnValue(Promise.resolve());
deleteNodePermanentSpy = spyOn(componentWithPermanentDelete.deleteDirective.nodesApi, 'deleteNode').and.returnValue(Promise.resolve());
purgeDeletedNodePermanentSpy = spyOn(componentWithPermanentDelete.deleteDirective.trashcanApi, 'deleteDeletedNode').and.returnValue(Promise.resolve());

});

Expand Down Expand Up @@ -152,7 +152,7 @@ describe('NodeDeleteDirective', () => {
});

it('should notify failed node deletion', async () => {
deleteNodeSpy.and.returnValue(Promise.reject('error'));
deleteNodeSpy.and.returnValue(Promise.reject(new Error('error')));

component.selection = [{ entry: { id: '1', name: 'name1' } }];
fixture.detectChanges();
Expand Down Expand Up @@ -187,7 +187,7 @@ describe('NodeDeleteDirective', () => {
});

it('should notify failed nodes deletion', async () => {
deleteNodeSpy.and.returnValue(Promise.reject('error'));
deleteNodeSpy.and.returnValue(Promise.reject(new Error('error')));

component.selection = [
{ entry: { id: '1', name: 'name1' } },
Expand All @@ -209,7 +209,7 @@ describe('NodeDeleteDirective', () => {
it('should notify partial deletion when only one node is successful', async () => {
deleteNodeSpy.and.callFake((id) => {
if (id === '1') {
return Promise.reject('error');
return Promise.reject(new Error('error'));
} else {
return Promise.resolve();
}
Expand All @@ -235,7 +235,7 @@ describe('NodeDeleteDirective', () => {
it('should notify partial deletion when some nodes are successful', async () => {
deleteNodeSpy.and.callFake((id) => {
if (id === '1') {
return Promise.reject(null);
return Promise.reject(new Error('error'));
}

return Promise.resolve();
Expand Down
Loading

0 comments on commit 8bd24db

Please sign in to comment.