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(parent structure): add method that return note parent structure #276

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
df44b32
feat(parent structure): add method that return note parent structure
dependentmadani Aug 29, 2024
0551b9f
update: start doing the search in team storage
dependentmadani Sep 1, 2024
a5adcc2
update: first part of code review is done
dependentmadani Sep 4, 2024
bccf7b1
fix: finish the review modifications
dependentmadani Sep 4, 2024
42cb421
update: first part of modification after review
dependentmadani Sep 6, 2024
bb6caff
fix: build problem fixed
dependentmadani Sep 6, 2024
a11d1f8
update(note request): update the content send in get request of note …
dependentmadani Sep 7, 2024
0e3e8f0
update(tests): add different tests for the note parent structure
dependentmadani Sep 8, 2024
3adff28
update(tests): tests are now functional
dependentmadani Sep 9, 2024
d6c9ded
update (parents note): modification based on the reviews, still tests…
dependentmadani Sep 14, 2024
6bdc56a
update: fix lint
dependentmadani Sep 14, 2024
6cf3d4c
update (note parents): few modification in the return value type, add…
dependentmadani Sep 15, 2024
1809ca1
update: add a test
dependentmadani Sep 15, 2024
ca7a9a2
update (note parents): modifications based on reviews, use test.each …
dependentmadani Sep 18, 2024
e14d948
update (parent note test): modification based on previous reviews
dependentmadani Sep 19, 2024
5c6cfc0
update (parent notes): few modification done, still work to be done
dependentmadani Sep 23, 2024
5bb6be5
update (note parents): fix the issue of tests, working good
dependentmadani Sep 24, 2024
fee9bbf
update (note parents): modification based on previous review, chore m…
dependentmadani Sep 26, 2024
d65c589
update (note parents tests): the issue of test failure is fixed
dependentmadani Sep 26, 2024
14234a1
udpate (note parents): based on last review
dependentmadani Sep 27, 2024
c09ad3a
Merge branch 'feat/return-note' of github.com:codex-team/notes.api in…
dependentmadani Sep 27, 2024
9872d5e
update (test cases): the issue is fixed due to missing an id
dependentmadani Sep 28, 2024
5a9fad2
update (test case): change the naming of a test case
dependentmadani Sep 29, 2024
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
3 changes: 2 additions & 1 deletion docker-compose.yml
Copy link
Member

Choose a reason for hiding this comment

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

unexpected change

Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
version: "3.2"
version: '3.2'

services:
api:
build:
Expand Down
17 changes: 17 additions & 0 deletions src/domain/service/note.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import type User from '@domain/entities/user.js';
import type { NoteList } from '@domain/entities/noteList.js';
import type NoteHistoryRepository from '@repository/noteHistory.repository.js';
import type { NoteHistoryMeta, NoteHistoryRecord, NoteHistoryPublic } from '@domain/entities/noteHistory.js';
import { definePublicNote, type NotePublic } from '@domain/entities/notePublic.js';

/**
* Note service
Expand Down Expand Up @@ -441,4 +442,20 @@ export default class NoteService {

return noteHistoryPublic;
}

/**
* Get note parent structure recursively by note id and user id
* and check if user has access to the parent note.
* @param noteId - id of the note to get parent structure
* @returns - array of notes that are parent structure of the note
*/
public async getNoteParents(noteId: NoteInternalId): Promise<NotePublic[]> {
Copy link
Member

Choose a reason for hiding this comment

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

Domain level should return Note[]. The Presentation level is responsible for converting it to NotePublic[]

const noteIds: NoteInternalId[] = await this.noteRelationsRepository.getNoteParentsIds(noteId);
const noteParents = await this.noteRepository.getNotesByIds(noteIds);
const noteParentsPublic: NotePublic[] = noteParents.map((note) => {
return definePublicNote(note);
});

return noteParentsPublic;
}
}
215 changes: 215 additions & 0 deletions src/presentation/http/router/note.test.ts
neSpecc marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -518,6 +518,221 @@ describe('Note API', () => {

expect(response?.json().message).toStrictEqual(expectedMessage);
});

e11sy marked this conversation as resolved.
Show resolved Hide resolved
test('Returns one parents note in case when note has one parents', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
test('Returns one parents note in case when note has one parents', async () => {
test('Returns one parents note in case when note has one parent', async () => {

/** Create test user */
const user = await global.db.insertUser();

/** Create acces token for the user */
const accessToken = global.auth(user.id);

/** Create test note - a parent note */
const parentNote = await global.db.insertNote({
creatorId: user.id,
});

/** Create test note - a child note */
const childNote = await global.db.insertNote({
creatorId: user.id,
});

/** Create test note settings */
await global.db.insertNoteSetting({
noteId: childNote.id,
isPublic: true,
});

/** Create test note relation */
await global.db.insertNoteRelation({
parentId: parentNote.id,
noteId: childNote.id,
});

const response = await global.api?.fakeRequest({
method: 'GET',
headers: {
authorization: `Bearer ${accessToken}`,
},
url: `/note/${childNote.publicId}`,
});

expect(response?.statusCode).toBe(200);

expect(response?.json()).toMatchObject({
parents: [
{
id: parentNote.publicId,
content: parentNote.content,
},
{
id: childNote.publicId,
content: childNote.content,
},
Comment on lines +563 to +570
Copy link
Member

Choose a reason for hiding this comment

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

Returns one parents note in case when note has one parents

so why there are two notes?

Copy link
Author

Choose a reason for hiding this comment

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

childNote is the main note, from where the request is made. And one parent note is the parenteNote. As you can see in the request made:
/note/${childNote.publicId

],
});
});

test('Returns two parents note in case when note has two parents', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
test('Returns two parents note in case when note has two parents', async () => {
test('Returns two note parents in case when note has two parents', async () => {

Copy link
Member

Choose a reason for hiding this comment

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

this test repeats the previous one? or its name is not clear. What the exact case you are testing?

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 almost similar, just adding more than one note.

/** Create test user */
const user = await global.db.insertUser();

/** Create acces token for the user */
const accessToken = global.auth(user.id);

/** Create test note - a grand parent note */
const grandParentNote = await global.db.insertNote({
creatorId: user.id,
});

/** Create test note - a parent note */
const parentNote = await global.db.insertNote({
creatorId: user.id,
});

/** Create test note - a child note */
const childNote = await global.db.insertNote({
creatorId: user.id,
});

/** Create test note settings */
await global.db.insertNoteSetting({
noteId: childNote.id,
isPublic: true,
});

/** Create note relation between parent and grandParentNote */
await global.db.insertNoteRelation({
parentId: grandParentNote.id,
noteId: parentNote.id,
});

/** Create test note relation */
await global.db.insertNoteRelation({
parentId: parentNote.id,
noteId: childNote.id,
});

const response = await global.api?.fakeRequest({
method: 'GET',
headers: {
authorization: `Bearer ${accessToken}`,
},
url: `/note/${childNote.publicId}`,
});

expect(response?.statusCode).toBe(200);

expect(response?.json()).toMatchObject({
parents: [
{
id: grandParentNote.publicId,
content: grandParentNote.content,
},
{
id: parentNote.publicId,
content: parentNote.content,
},
{
id: childNote.publicId,
content: childNote.content,
},
],
});
});

test('Returns one parent\'s note when the user is not in the parent note\'s team but shares a relation', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
test('Returns one parent\'s note when the user is not in the parent note\'s team but shares a relation', async () => {
test('Returns one parent note when the user is not in the parent note\'s team but shares a relation', async () => {

Copy link
Contributor

Choose a reason for hiding this comment

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

also if note has parent, that already means that relation exists, so but shares a relation is unneded

Copy link
Contributor

Choose a reason for hiding this comment

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

also, i did not get the case, if one parent note is returned, then overall two notes would be returned? (actual note and parent) or only just actual note?

Copy link
Author

Choose a reason for hiding this comment

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

In this case, the note is not created by the same user, at the same time the main user does not exist in the team of the parent note. Just to prove that the notes are returned based on the note relation, not on team members of the note.

Copy link
Member

Choose a reason for hiding this comment

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

shares a relation

unclear statement

/** Create test user */
const user = await global.db.insertUser();

/** Create another user */
const anotherUser = await global.db.insertUser();

/** Create acces token for the user */
const accessToken = global.auth(user.id);

/** Create test note - a parent note */
const parentNote = await global.db.insertNote({
creatorId: anotherUser.id,
});

/** Create test note - a child note */
const childNote = await global.db.insertNote({
creatorId: user.id,
});

/** Create test note settings */
await global.db.insertNoteSetting({
noteId: childNote.id,
isPublic: true,
});

/** Create test note relation */
await global.db.insertNoteRelation({
parentId: parentNote.id,
noteId: childNote.id,
});

const response = await global.api?.fakeRequest({
method: 'GET',
headers: {
authorization: `Bearer ${accessToken}`,
},
url: `/note/${childNote.publicId}`,
});

expect(response?.statusCode).toBe(200);

expect(response?.json()).toMatchObject({
parents: [
{
id: parentNote.publicId,
content: parentNote.content,
},
{
id: childNote.publicId,
content: childNote.content,
},
],
});
});

test('Returns one note in case where there is no relation exist for the note with status 200', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

case is also unclear for me. If note has no relation, why do you return a some note?

/** Create test user */
const user = await global.db.insertUser();

/** Create acces token for the user */
const accessToken = global.auth(user.id);

/** Create test note - a child note */
const note = await global.db.insertNote({
creatorId: user.id,
});

/** Create test note settings */
await global.db.insertNoteSetting({
noteId: note.id,
isPublic: true,
});

const response = await global.api?.fakeRequest({
method: 'GET',
headers: {
authorization: `Bearer ${accessToken}`,
},
url: `/note/${note.publicId}`,
});

expect(response?.statusCode).toBe(200);

expect(response?.json()).toMatchObject({
parents: [
{
id: note.publicId,
content: note.content,
},
],
});
});
});

describe('PATCH note/:notePublicId ', () => {
Expand Down
10 changes: 10 additions & 0 deletions src/presentation/http/router/note.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ const NoteRouter: FastifyPluginCallback<NoteRouterOptions> = (fastify, opts, don
canEdit: boolean;
};
tools: EditorTool[];
parents: NotePublic[];
} | ErrorResponse;
}>('/:notePublicId', {
config: {
Expand Down Expand Up @@ -123,6 +124,12 @@ const NoteRouter: FastifyPluginCallback<NoteRouterOptions> = (fastify, opts, don
$ref: 'EditorToolSchema',
},
},
parents: {
type: 'array',
items: {
$ref: 'NoteSchema',
},
},
},
},
},
Expand Down Expand Up @@ -172,11 +179,14 @@ const NoteRouter: FastifyPluginCallback<NoteRouterOptions> = (fastify, opts, don
*/
const canEdit = memberRole === MemberRole.Write;

const noteParentStructure = await noteService.getNoteParents(noteId);

return reply.send({
note: notePublic,
parentNote: parentNote,
accessRights: { canEdit: canEdit },
tools: noteTools,
parents: noteParentStructure,
});
});

Expand Down
9 changes: 9 additions & 0 deletions src/repository/note.repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,4 +81,13 @@ export default class NoteRepository {
public async getNoteListByUserId(id: number, offset: number, limit: number): Promise<Note[]> {
return await this.storage.getNoteListByUserId(id, offset, limit);
}

/**
* Get all notes based on their ids
* @param noteIds : list of note ids
* @returns an array of notes
*/
public async getNotesByIds(noteIds: NoteInternalId[]): Promise<Note[]> {
neSpecc marked this conversation as resolved.
Show resolved Hide resolved
return await this.storage.getNotesByIds(noteIds);
}
}
9 changes: 9 additions & 0 deletions src/repository/noteRelations.repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,4 +67,13 @@ export default class NoteRelationsRepository {
public async hasRelation(noteId: NoteInternalId): Promise<boolean> {
return await this.storage.hasRelation(noteId);
}

/**
* Get all note parents based on note id
* @param noteId : note id to get all its parents
* @returns an array of note parents ids
*/
public async getNoteParentsIds(noteId: NoteInternalId): Promise<NoteInternalId[]> {
return await this.storage.getAllNoteParentsIds(noteId);
Copy link
Member

Choose a reason for hiding this comment

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

what is the meaning of "All" here? is it something different between regular getNoteParentsIds which is used in other places?

}
}
28 changes: 24 additions & 4 deletions src/repository/storage/postgres/orm/sequelize/note.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ import type { Note, NoteCreationAttributes, NoteInternalId, NotePublicId } from
import { UserModel } from '@repository/storage/postgres/orm/sequelize/user.js';
import type { NoteSettingsModel } from './noteSettings.js';
import type { NoteVisitsModel } from './noteVisits.js';
import { DomainError } from '@domain/entities/DomainError.js';
import type { NoteHistoryModel } from './noteHistory.js';
import { notEmpty } from '@infrastructure/utils/empty.js';

/* eslint-disable @typescript-eslint/naming-convention */

Expand Down Expand Up @@ -233,11 +233,11 @@ export default class NoteSequelizeStorage {
*/
public async getNoteListByUserId(userId: number, offset: number, limit: number): Promise<Note[]> {
if (this.visitsModel === null) {
throw new DomainError('NoteVisit model should be defined');
throw new Error('NoteStorage: NoteVisit model should be defined');
}

if (!this.settingsModel) {
throw new Error('Note settings model not initialized');
throw new Error('NoteStorage: Note settings model not initialized');
}

const reply = await this.model.findAll({
Expand Down Expand Up @@ -293,7 +293,7 @@ export default class NoteSequelizeStorage {
*/
public async getNoteByHostname(hostname: string): Promise<Note | null> {
if (!this.settingsModel) {
throw new Error('Note settings model not initialized');
throw new Error('NoteStorage: Note settings model not initialized');
}

/**
Expand Down Expand Up @@ -324,4 +324,24 @@ export default class NoteSequelizeStorage {
},
});
};

/**
* Get all notes based on their ids
* @param noteIds - list of note ids
*/
public async getNotesByIds(noteIds: NoteInternalId[]): Promise<Note[]> {
const notes: Note[] = [];

for (const noteId of noteIds) {
const note: Note | null = await this.model.findOne({
where: { id: noteId },
});

if (notEmpty(note)) {
notes.push(note);
}
}
Comment on lines +335 to +343
Copy link
Member

Choose a reason for hiding this comment

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

its better to use the single sql query instead of looping over them


return notes;
}
}
Loading
Loading