Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(routes): get all notes by parent note id #282

Closed
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/domain/service/note.ts
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,20 @@ export default class NoteService {
};
}

/**
* Returns note list by parent note id
* @param parentNoteId - id of the parent note
* @param page - number of current page
* @returns list of the notes ordered by time of last visit
*/
public async getNoteListByParentNote(parentNoteId: NoteInternalId, page: number): Promise<NoteList> {
const offset = (page - 1) * this.noteListPortionSize;

return {
items: await this.noteRepository.getNoteListByParentNote(parentNoteId, offset, this.noteListPortionSize),
};
}

/**
* Create note relation
* @param noteId - id of the current note
Expand Down
1 change: 1 addition & 0 deletions src/presentation/http/http-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ export default class HttpApi implements Api {
await this.server?.register(NoteListRouter, {
prefix: '/notes',
noteService: domainServices.noteService,
noteSettingsService: domainServices.noteSettingsService,
});

await this.server?.register(JoinRouter, {
Expand Down
6 changes: 5 additions & 1 deletion src/presentation/http/middlewares/note/useNoteResolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { notEmpty } from '@infrastructure/utils/empty.js';
import { StatusCodes } from 'http-status-codes';
import hasProperty from '@infrastructure/utils/hasProperty.js';
import { getLogger } from '@infrastructure/logging/index.js';
import type { Note, NotePublicId } from '@domain/entities/note.js';
import type { Note, NotePublicId, NoteInternalId } from '@domain/entities/note.js';

/**
* Add middleware for resolve Note by public id and add it to request
Expand Down Expand Up @@ -35,6 +35,10 @@ export default function useNoteResolver(noteService: NoteService): {
const publicId = requestData.notePublicId as NotePublicId;

return await noteService.getNoteByPublicId(publicId);
} else if (hasProperty(requestData, 'parentNoteId') && notEmpty(requestData.parentNoteId)) {
const noteId = requestData.parentNoteId as NoteInternalId;

return await noteService.getNoteById(noteId);
}
}

Expand Down
127 changes: 127 additions & 0 deletions src/presentation/http/router/noteList.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -144,3 +144,130 @@ describe('GET /notes?page', () => {
}
});
});

describe('GET /notes/:parentNoteId?page', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no tests for userInTeam policy

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added tests, review it

test.each([
/**
* Returns noteList with specified length
* User is authorized
*/
{
isAuthorized: true,
expectedStatusCode: 200,
expectedMessage: null,
expectedLength: 30,
pageNumber: 1,
},
/**
* Returns noteList with specified length (for last page)
* User is authorized
*/
{
isAuthorized: true,
expectedStatusCode: 200,
expectedMessage: null,
expectedLength: 19,
pageNumber: 2,
},
/**
* Returns noteList with no items if there are no notes for certain parentNote
* User is authorized
*/
{
isAuthorized: true,
expectedStatusCode: 200,
expectedMessage: null,
expectedLength: 0,
pageNumber: 3,
},
/**
* Returns 'querystring/page must be >= 1' message when page < 0
*/
{
isAuthorized: true,
expectedStatusCode: 400,
expectedMessage: 'querystring/page must be >= 1',
expectedLength: 0,
pageNumber: -1,
},
/**
* Returns 'querystring/page must be <= 30' message when page is too large (maximum page numbrer is 30 by default)
*/
{
isAuthorized: true,
expectedStatusCode: 400,
expectedMessage: 'querystring/page must be <= 30',
expectedLength: 0,
pageNumber: 31,
},
/**
* Returns 'unauthorized' message when user is not authorized
*/
{
isAuthorized: false,
expectedStatusCode: 401,
expectedMessage: 'You must be authenticated to access this resource',
expectedLength: 0,
pageNumber: 1,
},
])('Get note list', async ({ isAuthorized, expectedStatusCode, expectedMessage, expectedLength, pageNumber }) => {
const portionSize = 49;
let accessToken;

/** Insert creator and randomGuy */
const creator = await global.db.insertUser();

const randomGuy = await global.db.insertUser();

if (isAuthorized) {
accessToken = global.auth(randomGuy.id);
}

const parentNote = await global.db.insertNote({
creatorId: creator.id,
});

await global.db.insertNoteSetting({
noteId: parentNote.id,
cover: 'DZnvqi63.png',
isPublic: true,
});

for (let i = 0; i < portionSize; i++) {
const note = await global.db.insertNote({
creatorId: creator.id,
});

await global.db.insertNoteSetting({
noteId: note.id,
cover: 'DZnvqi63.png',
isPublic: true,
});

await global.db.insertNoteRelation({
parentId: parentNote.id,
noteId: note.id,
});
}

const response = await global.api?.fakeRequest({
method: 'GET',
headers: {
authorization: `Bearer ${accessToken}`,
},
url: `/notes/${parentNote.id}?page=${pageNumber}`,
});

const body = response?.json();

if (expectedMessage !== null) {
expect(response?.statusCode).toBe(expectedStatusCode);

expect(body.message).toBe(expectedMessage);
} else {
expect(response?.statusCode).toBe(expectedStatusCode);

expect(body.items).toHaveLength(expectedLength);
}
});
});
96 changes: 96 additions & 0 deletions src/presentation/http/router/noteList.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
import type { FastifyPluginCallback } from 'fastify';
import type NoteService from '@domain/service/note.js';
import useNoteResolver from '../middlewares/note/useNoteResolver.js';
import useNoteSettingsResolver from '../middlewares/noteSettings/useNoteSettingsResolver.js';
import useMemberRoleResolver from '../middlewares/noteSettings/useMemberRoleResolver.js';
import type NoteSettingsService from '@domain/service/noteSettings.js';
import { definePublicNote, type NotePublic } from '@domain/entities/notePublic.js';
import type { NoteListPublic } from '@domain/entities/noteList.js';
import type { NoteInternalId } from '@domain/entities/note.js';

/**
* Interface for the noteList router.
Expand All @@ -12,6 +17,11 @@ interface NoteListRouterOptions {
*/
noteService: NoteService;

/**
* Note Settings service instance
*/
noteSettingsService: NoteSettingsService;

}

/**
Expand All @@ -22,6 +32,25 @@ interface NoteListRouterOptions {
*/
const NoteListRouter: FastifyPluginCallback<NoteListRouterOptions> = (fastify, opts, done) => {
const noteService = opts.noteService;
const noteSettingsService = opts.noteSettingsService;

/**
* Prepare note id resolver middleware
* It should be used in routes that accepts note public id
*/
const { noteResolver } = useNoteResolver(noteService);

/**
* Prepare note settings resolver middleware
* It should be used to use note settings in middlewares
*/
const { noteSettingsResolver } = useNoteSettingsResolver(noteSettingsService);

/**
* Prepare user role resolver middleware
* It should be used to use user role in middlewares
*/
const { memberRoleResolver } = useMemberRoleResolver(noteSettingsService);

/**
* Get note list ordered by time of last visit
Expand Down Expand Up @@ -77,6 +106,73 @@ const NoteListRouter: FastifyPluginCallback<NoteListRouterOptions> = (fastify, o
return reply.send(noteListPublic);
});

/**
* Get note list by parent note
*/
fastify.get<{
Params: {
parentNoteId: NoteInternalId;
};
Querystring: {
page: number;
};
}>('/:parentNoteId', {
config: {
policy: [
'authRequired',
'notePublicOrUserInTeam',
],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

notePublicOrUserInTeam checks permission, for the note which is resolved by noteResolver. noteResolver resolves note by publicId, that means, that it checks for notePublicId field, your request would not have notePublicId so policy would return notAcceptable when note is not public

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have updated the notResolver to consider parentId as well

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think that this is not a case for noteResolver usage, because in some case user can pass noteId and noteParentId in one request

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me know, which policies required here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think that we should check it without policy and send reply.forbidden if user is not in team of the parent note

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

request you to review new changes

},
schema: {
params: {
notePublicId: {
$ref: 'NoteSchema#/properties/id',
},
},
querystring: {
page: {
type: 'number',
minimum: 1,
maximum: 30,
},
},
response: {
'2xx': {
description: 'Query notelist',
properties: {
items: {
id: { type: 'string' },
content: { type: 'string' },
createdAt: { type: 'string' },
creatorId: { type: 'string' },
updatedAt: { type: 'string' },
},
},
Tushar504 marked this conversation as resolved.
Show resolved Hide resolved
},
},
},
preHandler: [
noteResolver,
noteSettingsResolver,
memberRoleResolver,
],
}, async (request, reply) => {
const { parentNoteId } = request.params;
const { page } = request.query;

const noteList = await noteService.getNoteListByParentNote(parentNoteId, page);
/**
* Wrapping Notelist for public use
*/
const noteListItemsPublic: NotePublic[] = noteList.items.map(definePublicNote);

const noteListPublic: NoteListPublic = {
items: noteListItemsPublic,
};

return reply.send(noteListPublic);
});

done();
};

Expand Down
1 change: 1 addition & 0 deletions src/repository/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ export async function init(orm: Orm, s3Config: S3StorageConfig): Promise<Reposit
/**
* Create associations between note and relations table
*/
noteStorage.createAssociationWithNoteRelationsModel(noteRelationshipStorage.model);
noteRelationshipStorage.createAssociationWithNoteModel(noteStorage.model);
Comment on lines +142 to 143
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this line needed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need that association to use include clause(join table)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as i can see, we already linked noteRelationshipStorage with noteStorage

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should i implement this getNoteListByParentNote() method in noteRelationshipStorage

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, but do you actually need to make another assotiation in addition to the existing?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is required, i refered other relationships implemenation(noteVisits, noteSetting)


/**
Expand Down
10 changes: 10 additions & 0 deletions src/repository/note.repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,4 +90,14 @@ export default class NoteRepository {
public async getNotesByIds(noteIds: NoteInternalId[]): Promise<Note[]> {
return await this.storage.getNotesByIds(noteIds);
}

/**
* Gets note list by parent note id
* @param parentNoteId - parent note id
* @param offset - number of skipped notes
* @param limit - number of notes to get
*/
public async getNoteListByParentNote(parentNoteId: number, offset: number, limit: number): Promise<Note[]> {
return await this.storage.getNoteListByParentNote(parentNoteId, offset, limit);
}
}
Loading
Loading