github/hedgedoc
1
0
Fork 0
mirror of https://github.com/hedgedoc/hedgedoc.git synced 2024-10-19 13:50:15 -04:00
Code Issues Projects Releases Packages Wiki Activity

NotesService: Replace noteByIdOrAlias with note as parameter

Browse source 
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 <philip.molares@udo.edu>
This commit is contained in:
Philip Molares 2021-02-20 16:50:11 +01:00
parent c9b05b3c44
commit 5f49cb8d48
5 changed files with 50 additions and 107 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

16
src/api/public/notes/notes.controller.ts
View file

@ -129,7 +129,7 @@ export class NotesController {
throw new UnauthorizedException('Deleting note denied!'); throw new UnauthorizedException('Deleting note denied!');
} }
this.logger.debug('Deleting note: ' + noteIdOrAlias, 'deleteNote'); this.logger.debug('Deleting note: ' + noteIdOrAlias, 'deleteNote');
await this.noteService.deleteNoteByIdOrAlias(noteIdOrAlias); await this.noteService.deleteNote(note);
this.logger.debug('Successfully deleted ' + noteIdOrAlias, 'deleteNote'); this.logger.debug('Successfully deleted ' + noteIdOrAlias, 'deleteNote');
return; return;
} catch (e) { } catch (e) {
@ -154,7 +154,7 @@ export class NotesController {
} }
this.logger.debug('Got raw markdown:\n' + text, 'updateNote'); this.logger.debug('Got raw markdown:\n' + text, 'updateNote');
return this.noteService.toNoteDto( return this.noteService.toNoteDto(
await this.noteService.updateNoteByIdOrAlias(noteIdOrAlias, text), await this.noteService.updateNote(note, text),
); );
} catch (e) { } catch (e) {
if (e instanceof NotInDBError) { if (e instanceof NotInDBError) {
@ -196,9 +196,7 @@ export class NotesController {
if (!this.permissionsService.mayRead(req.user, note)) { if (!this.permissionsService.mayRead(req.user, note)) {
throw new UnauthorizedException('Reading note denied!'); throw new UnauthorizedException('Reading note denied!');
} }
return this.noteService.toNoteMetadataDto( return this.noteService.toNoteMetadataDto(note);
await this.noteService.getNoteByIdOrAlias(noteIdOrAlias),
);
} catch (e) { } catch (e) {
if (e instanceof NotInDBError) { if (e instanceof NotInDBError) {
throw new NotFoundException(e.message); throw new NotFoundException(e.message);
@ -223,7 +221,7 @@ export class NotesController {
throw new UnauthorizedException('Updating note denied!'); throw new UnauthorizedException('Updating note denied!');
} }
return this.noteService.toNotePermissionsDto( return this.noteService.toNotePermissionsDto(
await this.noteService.updateNotePermissions(noteIdOrAlias, updateDto), await this.noteService.updateNotePermissions(note, updateDto),
); );
} catch (e) { } catch (e) {
if (e instanceof NotInDBError) { if (e instanceof NotInDBError) {
@ -244,9 +242,7 @@ export class NotesController {
if (!this.permissionsService.mayRead(req.user, note)) { if (!this.permissionsService.mayRead(req.user, note)) {
throw new UnauthorizedException('Reading note denied!'); throw new UnauthorizedException('Reading note denied!');
} }
const revisions = await this.revisionsService.getAllRevisions( const revisions = await this.revisionsService.getAllRevisions(note);
noteIdOrAlias,
);
return Promise.all( return Promise.all(
revisions.map((revision) => revisions.map((revision) =>
this.revisionsService.toRevisionMetadataDto(revision), this.revisionsService.toRevisionMetadataDto(revision),
@ -273,7 +269,7 @@ export class NotesController {
throw new UnauthorizedException('Reading note denied!'); throw new UnauthorizedException('Reading note denied!');
} }
return this.revisionsService.toRevisionDto( return this.revisionsService.toRevisionDto(
await this.revisionsService.getRevision(noteIdOrAlias, revisionId), await this.revisionsService.getRevision(note, revisionId),
); );
} catch (e) { } catch (e) {
if (e instanceof NotInDBError) { if (e instanceof NotInDBError) {

84
src/notes/notes.service.spec.ts
View file

@ -245,36 +245,31 @@ describe('NotesService', () => {
}); });
}); });
describe('deleteNoteByIdOrAlias', () => { describe('deleteNote', () => {
it('works', async () => { it('works', async () => {
const user = User.create('hardcoded', 'Testy') as User; const user = User.create('hardcoded', 'Testy') as User;
const note = Note.create(user); const note = Note.create(user);
jest.spyOn(noteRepo, 'findOne').mockResolvedValueOnce(note);
jest jest
.spyOn(noteRepo, 'remove') .spyOn(noteRepo, 'remove')
.mockImplementationOnce(async (entry, _) => { .mockImplementationOnce(async (entry, _) => {
expect(entry).toEqual(note); expect(entry).toEqual(note);
return entry; return entry;
}); });
await service.deleteNoteByIdOrAlias('noteThatExists'); await service.deleteNote(note);
}); });
}); });
describe('updateNoteByIdOrAlias', () => { describe('updateNote', () => {
it('works', async () => { it('works', async () => {
const user = User.create('hardcoded', 'Testy') as User; const user = User.create('hardcoded', 'Testy') as User;
const note = Note.create(user); const note = Note.create(user);
const revisionLength = (await note.revisions).length; const revisionLength = (await note.revisions).length;
jest.spyOn(noteRepo, 'findOne').mockResolvedValueOnce(note);
jest jest
.spyOn(noteRepo, 'save') .spyOn(noteRepo, 'save')
.mockImplementationOnce(async (entry: Note) => { .mockImplementationOnce(async (entry: Note) => {
return entry; return entry;
}); });
const updatedNote = await service.updateNoteByIdOrAlias( const updatedNote = await service.updateNote(note, 'newContent');
'noteThatExists',
'newContent',
);
expect(await updatedNote.revisions).toHaveLength(revisionLength + 1); expect(await updatedNote.revisions).toHaveLength(revisionLength + 1);
}); });
}); });
@ -294,37 +289,29 @@ describe('NotesService', () => {
const note = Note.create(user); const note = Note.create(user);
describe('works', () => { describe('works', () => {
it('with empty GroupPermissions and with empty UserPermissions', async () => { it('with empty GroupPermissions and with empty UserPermissions', async () => {
jest.spyOn(noteRepo, 'findOne').mockResolvedValueOnce(note);
jest jest
.spyOn(noteRepo, 'save') .spyOn(noteRepo, 'save')
.mockImplementationOnce(async (entry: Note) => { .mockImplementationOnce(async (entry: Note) => {
return entry; return entry;
}); });
const savedNote = await service.updateNotePermissions( const savedNote = await service.updateNotePermissions(note, {
'noteThatExists',
{
sharedToUsers: [], sharedToUsers: [],
sharedToGroups: [], sharedToGroups: [],
}, });
);
expect(savedNote.userPermissions).toHaveLength(0); expect(savedNote.userPermissions).toHaveLength(0);
expect(savedNote.groupPermissions).toHaveLength(0); expect(savedNote.groupPermissions).toHaveLength(0);
}); });
it('with empty GroupPermissions and with new UserPermissions', async () => { it('with empty GroupPermissions and with new UserPermissions', async () => {
jest.spyOn(noteRepo, 'findOne').mockResolvedValueOnce(note);
jest jest
.spyOn(noteRepo, 'save') .spyOn(noteRepo, 'save')
.mockImplementationOnce(async (entry: Note) => { .mockImplementationOnce(async (entry: Note) => {
return entry; return entry;
}); });
jest.spyOn(userRepo, 'findOne').mockResolvedValueOnce(user); jest.spyOn(userRepo, 'findOne').mockResolvedValueOnce(user);
const savedNote = await service.updateNotePermissions( const savedNote = await service.updateNotePermissions(note, {
'noteThatExists',
{
sharedToUsers: [userPermissionUpdate], sharedToUsers: [userPermissionUpdate],
sharedToGroups: [], sharedToGroups: [],
}, });
);
expect(savedNote.userPermissions).toHaveLength(1); expect(savedNote.userPermissions).toHaveLength(1);
expect(savedNote.userPermissions[0].user.userName).toEqual( expect(savedNote.userPermissions[0].user.userName).toEqual(
userPermissionUpdate.username, userPermissionUpdate.username,
@ -343,22 +330,16 @@ describe('NotesService', () => {
canEdit: !userPermissionUpdate.canEdit, canEdit: !userPermissionUpdate.canEdit,
}, },
]; ];
jest
.spyOn(noteRepo, 'findOne')
.mockResolvedValueOnce(noteWithPreexistingPermissions);
jest jest
.spyOn(noteRepo, 'save') .spyOn(noteRepo, 'save')
.mockImplementationOnce(async (entry: Note) => { .mockImplementationOnce(async (entry: Note) => {
return entry; return entry;
}); });
jest.spyOn(userRepo, 'findOne').mockResolvedValueOnce(user); jest.spyOn(userRepo, 'findOne').mockResolvedValueOnce(user);
const savedNote = await service.updateNotePermissions( const savedNote = await service.updateNotePermissions(note, {
'noteThatExists',
{
sharedToUsers: [userPermissionUpdate], sharedToUsers: [userPermissionUpdate],
sharedToGroups: [], sharedToGroups: [],
}, });
);
expect(savedNote.userPermissions).toHaveLength(1); expect(savedNote.userPermissions).toHaveLength(1);
expect(savedNote.userPermissions[0].user.userName).toEqual( expect(savedNote.userPermissions[0].user.userName).toEqual(
userPermissionUpdate.username, userPermissionUpdate.username,
@ -369,20 +350,16 @@ describe('NotesService', () => {
expect(savedNote.groupPermissions).toHaveLength(0); expect(savedNote.groupPermissions).toHaveLength(0);
}); });
it('with new GroupPermissions and with empty UserPermissions', async () => { it('with new GroupPermissions and with empty UserPermissions', async () => {
jest.spyOn(noteRepo, 'findOne').mockResolvedValueOnce(note);
jest jest
.spyOn(noteRepo, 'save') .spyOn(noteRepo, 'save')
.mockImplementationOnce(async (entry: Note) => { .mockImplementationOnce(async (entry: Note) => {
return entry; return entry;
}); });
jest.spyOn(groupRepo, 'findOne').mockResolvedValueOnce(group); jest.spyOn(groupRepo, 'findOne').mockResolvedValueOnce(group);
const savedNote = await service.updateNotePermissions( const savedNote = await service.updateNotePermissions(note, {
'noteThatExists',
{
sharedToUsers: [], sharedToUsers: [],
sharedToGroups: [groupPermissionUpate], sharedToGroups: [groupPermissionUpate],
}, });
);
expect(savedNote.userPermissions).toHaveLength(0); expect(savedNote.userPermissions).toHaveLength(0);
expect(savedNote.groupPermissions[0].group.name).toEqual( expect(savedNote.groupPermissions[0].group.name).toEqual(
groupPermissionUpate.groupname, groupPermissionUpate.groupname,
@ -392,7 +369,6 @@ describe('NotesService', () => {
); );
}); });
it('with new GroupPermissions and with new UserPermissions', async () => { it('with new GroupPermissions and with new UserPermissions', async () => {
jest.spyOn(noteRepo, 'findOne').mockResolvedValueOnce(note);
jest jest
.spyOn(noteRepo, 'save') .spyOn(noteRepo, 'save')
.mockImplementationOnce(async (entry: Note) => { .mockImplementationOnce(async (entry: Note) => {
@ -400,13 +376,10 @@ describe('NotesService', () => {
}); });
jest.spyOn(userRepo, 'findOne').mockResolvedValueOnce(user); jest.spyOn(userRepo, 'findOne').mockResolvedValueOnce(user);
jest.spyOn(groupRepo, 'findOne').mockResolvedValueOnce(group); jest.spyOn(groupRepo, 'findOne').mockResolvedValueOnce(group);
const savedNote = await service.updateNotePermissions( const savedNote = await service.updateNotePermissions(note, {
'noteThatExists',
{
sharedToUsers: [userPermissionUpdate], sharedToUsers: [userPermissionUpdate],
sharedToGroups: [groupPermissionUpate], sharedToGroups: [groupPermissionUpate],
}, });
);
expect(savedNote.userPermissions[0].user.userName).toEqual( expect(savedNote.userPermissions[0].user.userName).toEqual(
userPermissionUpdate.username, userPermissionUpdate.username,
); );
@ -429,9 +402,6 @@ describe('NotesService', () => {
canEdit: !userPermissionUpdate.canEdit, canEdit: !userPermissionUpdate.canEdit,
}, },
]; ];
jest
.spyOn(noteRepo, 'findOne')
.mockResolvedValueOnce(noteWithUserPermission);
jest jest
.spyOn(noteRepo, 'save') .spyOn(noteRepo, 'save')
.mockImplementationOnce(async (entry: Note) => { .mockImplementationOnce(async (entry: Note) => {
@ -440,7 +410,7 @@ describe('NotesService', () => {
jest.spyOn(userRepo, 'findOne').mockResolvedValueOnce(user); jest.spyOn(userRepo, 'findOne').mockResolvedValueOnce(user);
jest.spyOn(groupRepo, 'findOne').mockResolvedValueOnce(group); jest.spyOn(groupRepo, 'findOne').mockResolvedValueOnce(group);
const savedNote = await service.updateNotePermissions( const savedNote = await service.updateNotePermissions(
'noteThatExists', noteWithUserPermission,
{ {
sharedToUsers: [userPermissionUpdate], sharedToUsers: [userPermissionUpdate],
sharedToGroups: [groupPermissionUpate], sharedToGroups: [groupPermissionUpate],
@ -468,9 +438,6 @@ describe('NotesService', () => {
canEdit: !groupPermissionUpate.canEdit, canEdit: !groupPermissionUpate.canEdit,
}, },
]; ];
jest
.spyOn(noteRepo, 'findOne')
.mockResolvedValueOnce(noteWithPreexistingPermissions);
jest.spyOn(groupRepo, 'findOne').mockResolvedValueOnce(group); jest.spyOn(groupRepo, 'findOne').mockResolvedValueOnce(group);
jest jest
.spyOn(noteRepo, 'save') .spyOn(noteRepo, 'save')
@ -478,7 +445,7 @@ describe('NotesService', () => {
return entry; return entry;
}); });
const savedNote = await service.updateNotePermissions( const savedNote = await service.updateNotePermissions(
'noteThatExists', noteWithPreexistingPermissions,
{ {
sharedToUsers: [], sharedToUsers: [],
sharedToGroups: [groupPermissionUpate], sharedToGroups: [groupPermissionUpate],
@ -501,9 +468,6 @@ describe('NotesService', () => {
canEdit: !groupPermissionUpate.canEdit, canEdit: !groupPermissionUpate.canEdit,
}, },
]; ];
jest
.spyOn(noteRepo, 'findOne')
.mockResolvedValueOnce(noteWithPreexistingPermissions);
jest jest
.spyOn(noteRepo, 'save') .spyOn(noteRepo, 'save')
.mockImplementationOnce(async (entry: Note) => { .mockImplementationOnce(async (entry: Note) => {
@ -512,7 +476,7 @@ describe('NotesService', () => {
jest.spyOn(userRepo, 'findOne').mockResolvedValueOnce(user); jest.spyOn(userRepo, 'findOne').mockResolvedValueOnce(user);
jest.spyOn(groupRepo, 'findOne').mockResolvedValueOnce(group); jest.spyOn(groupRepo, 'findOne').mockResolvedValueOnce(group);
const savedNote = await service.updateNotePermissions( const savedNote = await service.updateNotePermissions(
'noteThatExists', noteWithPreexistingPermissions,
{ {
sharedToUsers: [userPermissionUpdate], sharedToUsers: [userPermissionUpdate],
sharedToGroups: [groupPermissionUpate], sharedToGroups: [groupPermissionUpate],
@ -547,9 +511,6 @@ describe('NotesService', () => {
canEdit: !userPermissionUpdate.canEdit, canEdit: !userPermissionUpdate.canEdit,
}, },
]; ];
jest
.spyOn(noteRepo, 'findOne')
.mockResolvedValueOnce(noteWithPreexistingPermissions);
jest jest
.spyOn(noteRepo, 'save') .spyOn(noteRepo, 'save')
.mockImplementationOnce(async (entry: Note) => { .mockImplementationOnce(async (entry: Note) => {
@ -558,7 +519,7 @@ describe('NotesService', () => {
jest.spyOn(userRepo, 'findOne').mockResolvedValueOnce(user); jest.spyOn(userRepo, 'findOne').mockResolvedValueOnce(user);
jest.spyOn(groupRepo, 'findOne').mockResolvedValueOnce(group); jest.spyOn(groupRepo, 'findOne').mockResolvedValueOnce(group);
const savedNote = await service.updateNotePermissions( const savedNote = await service.updateNotePermissions(
'noteThatExists', noteWithPreexistingPermissions,
{ {
sharedToUsers: [userPermissionUpdate], sharedToUsers: [userPermissionUpdate],
sharedToGroups: [groupPermissionUpate], sharedToGroups: [groupPermissionUpate],
@ -580,9 +541,8 @@ describe('NotesService', () => {
}); });
describe('fails:', () => { describe('fails:', () => {
it('userPermissions has duplicate entries', async () => { it('userPermissions has duplicate entries', async () => {
jest.spyOn(noteRepo, 'findOne').mockResolvedValueOnce(note);
try { try {
await service.updateNotePermissions('noteThatExists', { await service.updateNotePermissions(note, {
sharedToUsers: [userPermissionUpdate, userPermissionUpdate], sharedToUsers: [userPermissionUpdate, userPermissionUpdate],
sharedToGroups: [], sharedToGroups: [],
}); });
@ -592,9 +552,8 @@ describe('NotesService', () => {
}); });
it('groupPermissions has duplicate entries', async () => { it('groupPermissions has duplicate entries', async () => {
jest.spyOn(noteRepo, 'findOne').mockResolvedValueOnce(note);
try { try {
await service.updateNotePermissions('noteThatExists', { await service.updateNotePermissions(note, {
sharedToUsers: [], sharedToUsers: [],
sharedToGroups: [groupPermissionUpate, groupPermissionUpate], sharedToGroups: [groupPermissionUpate, groupPermissionUpate],
}); });
@ -604,9 +563,8 @@ describe('NotesService', () => {
}); });
it('userPermissions and groupPermissions have duplicate entries', async () => { it('userPermissions and groupPermissions have duplicate entries', async () => {
jest.spyOn(noteRepo, 'findOne').mockResolvedValueOnce(note);
try { try {
await service.updateNotePermissions('noteThatExists', { await service.updateNotePermissions(note, {
sharedToUsers: [userPermissionUpdate, userPermissionUpdate], sharedToUsers: [userPermissionUpdate, userPermissionUpdate],
sharedToGroups: [groupPermissionUpate, groupPermissionUpate], sharedToGroups: [groupPermissionUpate, groupPermissionUpate],
}); });

25
src/notes/notes.service.ts
View file

@ -180,29 +180,24 @@ export class NotesService {
/** /**
* @async * @async
* Delete a note by either their id or alias. * Delete a note
* @param {string} noteIdOrAlias - the notes id or alias * @param {Note} note - the note to delete
* @return {Note} the note, that was deleted * @return {Note} the note, that was deleted
* @throws {NotInDBError} there is no note with this id or alias * @throws {NotInDBError} there is no note with this id or alias
*/ */
async deleteNoteByIdOrAlias(noteIdOrAlias: string): Promise<Note> { async deleteNote(note: Note): Promise<Note> {
const note = await this.getNoteByIdOrAlias(noteIdOrAlias);
return await this.noteRepository.remove(note); return await this.noteRepository.remove(note);
} }
/** /**
* @async * @async
* Update a notes content. The note is specified by either their id or alias. * Update a notes content.
* @param {string} noteIdOrAlias - the notes id or alias * @param {Note} note - the note
* @param {string} noteContent - the new content * @param {string} noteContent - the new content
* @return {Note} the note with a new revision and new content * @return {Note} the note with a new revision and new content
* @throws {NotInDBError} there is no note with this id or alias * @throws {NotInDBError} there is no note with this id or alias
*/ */
async updateNoteByIdOrAlias( async updateNote(note: Note, noteContent: string): Promise<Note> {
noteIdOrAlias: string,
noteContent: string,
): Promise<Note> {
const note = await this.getNoteByIdOrAlias(noteIdOrAlias);
const revisions = await note.revisions; const revisions = await note.revisions;
//TODO: Calculate patch //TODO: Calculate patch
revisions.push(Revision.create(noteContent, noteContent)); revisions.push(Revision.create(noteContent, noteContent));
@ -212,19 +207,17 @@ export class NotesService {
/** /**
* @async * @async
* Update a notes permissions. The note is specified by either their id or alias. * Update a notes permissions.
* @param {string} noteIdOrAlias - the notes id or alias * @param {Note} note - the note
* @param {NotePermissionsUpdateDto} newPermissions - the permissions the not should be set to * @param {NotePermissionsUpdateDto} newPermissions - the permissions the not should be set to
* @return {Note} the note with the new permissions * @return {Note} the note with the new permissions
* @throws {NotInDBError} there is no note with this id or alias * @throws {NotInDBError} there is no note with this id or alias
* @throws {PermissionsUpdateInconsistentError} the new permissions specify a user or group twice. * @throws {PermissionsUpdateInconsistentError} the new permissions specify a user or group twice.
*/ */
async updateNotePermissions( async updateNotePermissions(
noteIdOrAlias: string, note: Note,
newPermissions: NotePermissionsUpdateDto, newPermissions: NotePermissionsUpdateDto,
): Promise<Note> { ): Promise<Note> {
const note = await this.getNoteByIdOrAlias(noteIdOrAlias);
const users = newPermissions.sharedToUsers.map( const users = newPermissions.sharedToUsers.map(
(userPermission) => userPermission.username, (userPermission) => userPermission.username,
); );

10
src/revisions/revisions.service.ts
View file

@ -12,6 +12,7 @@ import { NotesService } from '../notes/notes.service';
import { RevisionMetadataDto } from './revision-metadata.dto'; import { RevisionMetadataDto } from './revision-metadata.dto';
import { RevisionDto } from './revision.dto'; import { RevisionDto } from './revision.dto';
import { Revision } from './revision.entity'; import { Revision } from './revision.entity';
import { Note } from '../notes/note.entity';
@Injectable() @Injectable()
export class RevisionsService { export class RevisionsService {
@ -24,8 +25,7 @@ export class RevisionsService {
this.logger.setContext(RevisionsService.name); this.logger.setContext(RevisionsService.name);
} }
async getAllRevisions(noteIdOrAlias: string): Promise<Revision[]> { async getAllRevisions(note: Note): Promise<Revision[]> {
const note = await this.notesService.getNoteByIdOrAlias(noteIdOrAlias);
return await this.revisionRepository.find({ return await this.revisionRepository.find({
where: { where: {
note: note, note: note,
@ -33,11 +33,7 @@ export class RevisionsService {
}); });
} }
async getRevision( async getRevision(note: Note, revisionId: number): Promise<Revision> {
noteIdOrAlias: string,
revisionId: number,
): Promise<Revision> {
const note = await this.notesService.getNoteByIdOrAlias(noteIdOrAlias);
return await this.revisionRepository.findOne({ return await this.revisionRepository.findOne({
where: { where: {
id: revisionId, id: revisionId,

2
test/public-api/notes.e2e-spec.ts
View file

@ -206,7 +206,7 @@ describe('Notes', () => {
// wait one second // wait one second
await new Promise((r) => setTimeout(r, 1000)); await new Promise((r) => setTimeout(r, 1000));
// update the note // update the note
await notesService.updateNoteByIdOrAlias('test5a', 'More test content'); await notesService.updateNote(note, 'More test content');
const metadata = await request(app.getHttpServer()) const metadata = await request(app.getHttpServer())
.get('/notes/test5a/metadata') .get('/notes/test5a/metadata')
.expect(200); .expect(200);