From 5f49cb8d4882512c58ec35b2404426fa5502f6bd Mon Sep 17 00:00:00 2001 From: Philip Molares Date: Sat, 20 Feb 2021 16:50:11 +0100 Subject: [PATCH] NotesService: Replace noteByIdOrAlias with note as parameter As the NotesController has the note already, because it checked with it if the user has the permission to perform the action, it's not necessary to get the note from the DB again, instead we should just provide the note to the functions directly. Signed-off-by: Philip Molares --- src/api/public/notes/notes.controller.ts | 16 ++-- src/notes/notes.service.spec.ts | 104 +++++++---------------- src/notes/notes.service.ts | 25 ++---- src/revisions/revisions.service.ts | 10 +-- test/public-api/notes.e2e-spec.ts | 2 +- 5 files changed, 50 insertions(+), 107 deletions(-) diff --git a/src/api/public/notes/notes.controller.ts b/src/api/public/notes/notes.controller.ts index b32538d9d..42d218c13 100644 --- a/src/api/public/notes/notes.controller.ts +++ b/src/api/public/notes/notes.controller.ts @@ -129,7 +129,7 @@ export class NotesController { throw new UnauthorizedException('Deleting note denied!'); } this.logger.debug('Deleting note: ' + noteIdOrAlias, 'deleteNote'); - await this.noteService.deleteNoteByIdOrAlias(noteIdOrAlias); + await this.noteService.deleteNote(note); this.logger.debug('Successfully deleted ' + noteIdOrAlias, 'deleteNote'); return; } catch (e) { @@ -154,7 +154,7 @@ export class NotesController { } this.logger.debug('Got raw markdown:\n' + text, 'updateNote'); return this.noteService.toNoteDto( - await this.noteService.updateNoteByIdOrAlias(noteIdOrAlias, text), + await this.noteService.updateNote(note, text), ); } catch (e) { if (e instanceof NotInDBError) { @@ -196,9 +196,7 @@ export class NotesController { if (!this.permissionsService.mayRead(req.user, note)) { throw new UnauthorizedException('Reading note denied!'); } - return this.noteService.toNoteMetadataDto( - await this.noteService.getNoteByIdOrAlias(noteIdOrAlias), - ); + return this.noteService.toNoteMetadataDto(note); } catch (e) { if (e instanceof NotInDBError) { throw new NotFoundException(e.message); @@ -223,7 +221,7 @@ export class NotesController { throw new UnauthorizedException('Updating note denied!'); } return this.noteService.toNotePermissionsDto( - await this.noteService.updateNotePermissions(noteIdOrAlias, updateDto), + await this.noteService.updateNotePermissions(note, updateDto), ); } catch (e) { if (e instanceof NotInDBError) { @@ -244,9 +242,7 @@ export class NotesController { if (!this.permissionsService.mayRead(req.user, note)) { throw new UnauthorizedException('Reading note denied!'); } - const revisions = await this.revisionsService.getAllRevisions( - noteIdOrAlias, - ); + const revisions = await this.revisionsService.getAllRevisions(note); return Promise.all( revisions.map((revision) => this.revisionsService.toRevisionMetadataDto(revision), @@ -273,7 +269,7 @@ export class NotesController { throw new UnauthorizedException('Reading note denied!'); } return this.revisionsService.toRevisionDto( - await this.revisionsService.getRevision(noteIdOrAlias, revisionId), + await this.revisionsService.getRevision(note, revisionId), ); } catch (e) { if (e instanceof NotInDBError) { diff --git a/src/notes/notes.service.spec.ts b/src/notes/notes.service.spec.ts index ecdcf9aae..9bc4c7ad8 100644 --- a/src/notes/notes.service.spec.ts +++ b/src/notes/notes.service.spec.ts @@ -245,36 +245,31 @@ describe('NotesService', () => { }); }); - describe('deleteNoteByIdOrAlias', () => { + describe('deleteNote', () => { it('works', async () => { const user = User.create('hardcoded', 'Testy') as User; const note = Note.create(user); - jest.spyOn(noteRepo, 'findOne').mockResolvedValueOnce(note); jest .spyOn(noteRepo, 'remove') .mockImplementationOnce(async (entry, _) => { expect(entry).toEqual(note); return entry; }); - await service.deleteNoteByIdOrAlias('noteThatExists'); + await service.deleteNote(note); }); }); - describe('updateNoteByIdOrAlias', () => { + describe('updateNote', () => { it('works', async () => { const user = User.create('hardcoded', 'Testy') as User; const note = Note.create(user); const revisionLength = (await note.revisions).length; - jest.spyOn(noteRepo, 'findOne').mockResolvedValueOnce(note); jest .spyOn(noteRepo, 'save') .mockImplementationOnce(async (entry: Note) => { return entry; }); - const updatedNote = await service.updateNoteByIdOrAlias( - 'noteThatExists', - 'newContent', - ); + const updatedNote = await service.updateNote(note, 'newContent'); expect(await updatedNote.revisions).toHaveLength(revisionLength + 1); }); }); @@ -294,37 +289,29 @@ describe('NotesService', () => { const note = Note.create(user); describe('works', () => { it('with empty GroupPermissions and with empty UserPermissions', async () => { - jest.spyOn(noteRepo, 'findOne').mockResolvedValueOnce(note); jest .spyOn(noteRepo, 'save') .mockImplementationOnce(async (entry: Note) => { return entry; }); - const savedNote = await service.updateNotePermissions( - 'noteThatExists', - { - sharedToUsers: [], - sharedToGroups: [], - }, - ); + const savedNote = await service.updateNotePermissions(note, { + sharedToUsers: [], + sharedToGroups: [], + }); expect(savedNote.userPermissions).toHaveLength(0); expect(savedNote.groupPermissions).toHaveLength(0); }); it('with empty GroupPermissions and with new UserPermissions', async () => { - jest.spyOn(noteRepo, 'findOne').mockResolvedValueOnce(note); jest .spyOn(noteRepo, 'save') .mockImplementationOnce(async (entry: Note) => { return entry; }); jest.spyOn(userRepo, 'findOne').mockResolvedValueOnce(user); - const savedNote = await service.updateNotePermissions( - 'noteThatExists', - { - sharedToUsers: [userPermissionUpdate], - sharedToGroups: [], - }, - ); + const savedNote = await service.updateNotePermissions(note, { + sharedToUsers: [userPermissionUpdate], + sharedToGroups: [], + }); expect(savedNote.userPermissions).toHaveLength(1); expect(savedNote.userPermissions[0].user.userName).toEqual( userPermissionUpdate.username, @@ -343,22 +330,16 @@ describe('NotesService', () => { canEdit: !userPermissionUpdate.canEdit, }, ]; - jest - .spyOn(noteRepo, 'findOne') - .mockResolvedValueOnce(noteWithPreexistingPermissions); jest .spyOn(noteRepo, 'save') .mockImplementationOnce(async (entry: Note) => { return entry; }); jest.spyOn(userRepo, 'findOne').mockResolvedValueOnce(user); - const savedNote = await service.updateNotePermissions( - 'noteThatExists', - { - sharedToUsers: [userPermissionUpdate], - sharedToGroups: [], - }, - ); + const savedNote = await service.updateNotePermissions(note, { + sharedToUsers: [userPermissionUpdate], + sharedToGroups: [], + }); expect(savedNote.userPermissions).toHaveLength(1); expect(savedNote.userPermissions[0].user.userName).toEqual( userPermissionUpdate.username, @@ -369,20 +350,16 @@ describe('NotesService', () => { expect(savedNote.groupPermissions).toHaveLength(0); }); it('with new GroupPermissions and with empty UserPermissions', async () => { - jest.spyOn(noteRepo, 'findOne').mockResolvedValueOnce(note); jest .spyOn(noteRepo, 'save') .mockImplementationOnce(async (entry: Note) => { return entry; }); jest.spyOn(groupRepo, 'findOne').mockResolvedValueOnce(group); - const savedNote = await service.updateNotePermissions( - 'noteThatExists', - { - sharedToUsers: [], - sharedToGroups: [groupPermissionUpate], - }, - ); + const savedNote = await service.updateNotePermissions(note, { + sharedToUsers: [], + sharedToGroups: [groupPermissionUpate], + }); expect(savedNote.userPermissions).toHaveLength(0); expect(savedNote.groupPermissions[0].group.name).toEqual( groupPermissionUpate.groupname, @@ -392,7 +369,6 @@ describe('NotesService', () => { ); }); it('with new GroupPermissions and with new UserPermissions', async () => { - jest.spyOn(noteRepo, 'findOne').mockResolvedValueOnce(note); jest .spyOn(noteRepo, 'save') .mockImplementationOnce(async (entry: Note) => { @@ -400,13 +376,10 @@ describe('NotesService', () => { }); jest.spyOn(userRepo, 'findOne').mockResolvedValueOnce(user); jest.spyOn(groupRepo, 'findOne').mockResolvedValueOnce(group); - const savedNote = await service.updateNotePermissions( - 'noteThatExists', - { - sharedToUsers: [userPermissionUpdate], - sharedToGroups: [groupPermissionUpate], - }, - ); + const savedNote = await service.updateNotePermissions(note, { + sharedToUsers: [userPermissionUpdate], + sharedToGroups: [groupPermissionUpate], + }); expect(savedNote.userPermissions[0].user.userName).toEqual( userPermissionUpdate.username, ); @@ -429,9 +402,6 @@ describe('NotesService', () => { canEdit: !userPermissionUpdate.canEdit, }, ]; - jest - .spyOn(noteRepo, 'findOne') - .mockResolvedValueOnce(noteWithUserPermission); jest .spyOn(noteRepo, 'save') .mockImplementationOnce(async (entry: Note) => { @@ -440,7 +410,7 @@ describe('NotesService', () => { jest.spyOn(userRepo, 'findOne').mockResolvedValueOnce(user); jest.spyOn(groupRepo, 'findOne').mockResolvedValueOnce(group); const savedNote = await service.updateNotePermissions( - 'noteThatExists', + noteWithUserPermission, { sharedToUsers: [userPermissionUpdate], sharedToGroups: [groupPermissionUpate], @@ -468,9 +438,6 @@ describe('NotesService', () => { canEdit: !groupPermissionUpate.canEdit, }, ]; - jest - .spyOn(noteRepo, 'findOne') - .mockResolvedValueOnce(noteWithPreexistingPermissions); jest.spyOn(groupRepo, 'findOne').mockResolvedValueOnce(group); jest .spyOn(noteRepo, 'save') @@ -478,7 +445,7 @@ describe('NotesService', () => { return entry; }); const savedNote = await service.updateNotePermissions( - 'noteThatExists', + noteWithPreexistingPermissions, { sharedToUsers: [], sharedToGroups: [groupPermissionUpate], @@ -501,9 +468,6 @@ describe('NotesService', () => { canEdit: !groupPermissionUpate.canEdit, }, ]; - jest - .spyOn(noteRepo, 'findOne') - .mockResolvedValueOnce(noteWithPreexistingPermissions); jest .spyOn(noteRepo, 'save') .mockImplementationOnce(async (entry: Note) => { @@ -512,7 +476,7 @@ describe('NotesService', () => { jest.spyOn(userRepo, 'findOne').mockResolvedValueOnce(user); jest.spyOn(groupRepo, 'findOne').mockResolvedValueOnce(group); const savedNote = await service.updateNotePermissions( - 'noteThatExists', + noteWithPreexistingPermissions, { sharedToUsers: [userPermissionUpdate], sharedToGroups: [groupPermissionUpate], @@ -547,9 +511,6 @@ describe('NotesService', () => { canEdit: !userPermissionUpdate.canEdit, }, ]; - jest - .spyOn(noteRepo, 'findOne') - .mockResolvedValueOnce(noteWithPreexistingPermissions); jest .spyOn(noteRepo, 'save') .mockImplementationOnce(async (entry: Note) => { @@ -558,7 +519,7 @@ describe('NotesService', () => { jest.spyOn(userRepo, 'findOne').mockResolvedValueOnce(user); jest.spyOn(groupRepo, 'findOne').mockResolvedValueOnce(group); const savedNote = await service.updateNotePermissions( - 'noteThatExists', + noteWithPreexistingPermissions, { sharedToUsers: [userPermissionUpdate], sharedToGroups: [groupPermissionUpate], @@ -580,9 +541,8 @@ describe('NotesService', () => { }); describe('fails:', () => { it('userPermissions has duplicate entries', async () => { - jest.spyOn(noteRepo, 'findOne').mockResolvedValueOnce(note); try { - await service.updateNotePermissions('noteThatExists', { + await service.updateNotePermissions(note, { sharedToUsers: [userPermissionUpdate, userPermissionUpdate], sharedToGroups: [], }); @@ -592,9 +552,8 @@ describe('NotesService', () => { }); it('groupPermissions has duplicate entries', async () => { - jest.spyOn(noteRepo, 'findOne').mockResolvedValueOnce(note); try { - await service.updateNotePermissions('noteThatExists', { + await service.updateNotePermissions(note, { sharedToUsers: [], sharedToGroups: [groupPermissionUpate, groupPermissionUpate], }); @@ -604,9 +563,8 @@ describe('NotesService', () => { }); it('userPermissions and groupPermissions have duplicate entries', async () => { - jest.spyOn(noteRepo, 'findOne').mockResolvedValueOnce(note); try { - await service.updateNotePermissions('noteThatExists', { + await service.updateNotePermissions(note, { sharedToUsers: [userPermissionUpdate, userPermissionUpdate], sharedToGroups: [groupPermissionUpate, groupPermissionUpate], }); diff --git a/src/notes/notes.service.ts b/src/notes/notes.service.ts index df264c259..8c705b36b 100644 --- a/src/notes/notes.service.ts +++ b/src/notes/notes.service.ts @@ -180,29 +180,24 @@ export class NotesService { /** * @async - * Delete a note by either their id or alias. - * @param {string} noteIdOrAlias - the notes id or alias + * Delete a note + * @param {Note} note - the note to delete * @return {Note} the note, that was deleted * @throws {NotInDBError} there is no note with this id or alias */ - async deleteNoteByIdOrAlias(noteIdOrAlias: string): Promise { - const note = await this.getNoteByIdOrAlias(noteIdOrAlias); + async deleteNote(note: Note): Promise { return await this.noteRepository.remove(note); } /** * @async - * Update a notes content. The note is specified by either their id or alias. - * @param {string} noteIdOrAlias - the notes id or alias + * Update a notes content. + * @param {Note} note - the note * @param {string} noteContent - the new content * @return {Note} the note with a new revision and new content * @throws {NotInDBError} there is no note with this id or alias */ - async updateNoteByIdOrAlias( - noteIdOrAlias: string, - noteContent: string, - ): Promise { - const note = await this.getNoteByIdOrAlias(noteIdOrAlias); + async updateNote(note: Note, noteContent: string): Promise { const revisions = await note.revisions; //TODO: Calculate patch revisions.push(Revision.create(noteContent, noteContent)); @@ -212,19 +207,17 @@ export class NotesService { /** * @async - * Update a notes permissions. The note is specified by either their id or alias. - * @param {string} noteIdOrAlias - the notes id or alias + * Update a notes permissions. + * @param {Note} note - the note * @param {NotePermissionsUpdateDto} newPermissions - the permissions the not should be set to * @return {Note} the note with the new permissions * @throws {NotInDBError} there is no note with this id or alias * @throws {PermissionsUpdateInconsistentError} the new permissions specify a user or group twice. */ async updateNotePermissions( - noteIdOrAlias: string, + note: Note, newPermissions: NotePermissionsUpdateDto, ): Promise { - const note = await this.getNoteByIdOrAlias(noteIdOrAlias); - const users = newPermissions.sharedToUsers.map( (userPermission) => userPermission.username, ); diff --git a/src/revisions/revisions.service.ts b/src/revisions/revisions.service.ts index a3735337d..b9bced063 100644 --- a/src/revisions/revisions.service.ts +++ b/src/revisions/revisions.service.ts @@ -12,6 +12,7 @@ import { NotesService } from '../notes/notes.service'; import { RevisionMetadataDto } from './revision-metadata.dto'; import { RevisionDto } from './revision.dto'; import { Revision } from './revision.entity'; +import { Note } from '../notes/note.entity'; @Injectable() export class RevisionsService { @@ -24,8 +25,7 @@ export class RevisionsService { this.logger.setContext(RevisionsService.name); } - async getAllRevisions(noteIdOrAlias: string): Promise { - const note = await this.notesService.getNoteByIdOrAlias(noteIdOrAlias); + async getAllRevisions(note: Note): Promise { return await this.revisionRepository.find({ where: { note: note, @@ -33,11 +33,7 @@ export class RevisionsService { }); } - async getRevision( - noteIdOrAlias: string, - revisionId: number, - ): Promise { - const note = await this.notesService.getNoteByIdOrAlias(noteIdOrAlias); + async getRevision(note: Note, revisionId: number): Promise { return await this.revisionRepository.findOne({ where: { id: revisionId, diff --git a/test/public-api/notes.e2e-spec.ts b/test/public-api/notes.e2e-spec.ts index 3492ff6d3..97aa31705 100644 --- a/test/public-api/notes.e2e-spec.ts +++ b/test/public-api/notes.e2e-spec.ts @@ -206,7 +206,7 @@ describe('Notes', () => { // wait one second await new Promise((r) => setTimeout(r, 1000)); // update the note - await notesService.updateNoteByIdOrAlias('test5a', 'More test content'); + await notesService.updateNote(note, 'More test content'); const metadata = await request(app.getHttpServer()) .get('/notes/test5a/metadata') .expect(200);