feat(parent structure): add method that return note parent structure #276

dependentmadani · 2024-08-29T10:35:48Z

Problem:

First task in issue #275, make a method to return recursively note parent structure.

Solution:

Return an array of note parent structure on the following form:

NoteParents : 
[
  {
    noteId: Note['NotePublicId'],
    content: Note['content']
  }
]

Making sure that the user has access to each note parent before including them in the returned array.

github-actions · 2024-08-29T10:38:46Z

Coverage Report

Status	Category	Percentage	Covered / Total
🟢	Lines	85.94% (🎯 80%) ⬆️ +0.15%	8572 / 9974
🟢	Statements	85.94% (🎯 80%) ⬆️ +0.15%	8572 / 9974
🔴	Functions	79.22% (🎯 80%) ⬆️ +0.31%	267 / 337
🟢	Branches	85.42% (🎯 80%) ⬆️ +0.15%	428 / 501

File Coverage

File	Stmts	% Branch	% Funcs	% Lines	Uncovered Lines
Changed Files
src/domain/service/note.ts	96.09%	85%	100%	96.09%	100-101, 127-128, 139-140, 168-169, 190-191, 278-279, 367-368, 384-385, 426-427
src/presentation/http/router/note.ts	98.06%	80.64%	100%	98.06%	152-153, 622-623, 639-645, 706-707, 763-764
src/repository/note.repository.ts	100%	100%	100%	100%
src/repository/noteRelations.repository.ts	100%	100%	100%	100%
src/repository/storage/postgres/orm/sequelize/note.ts	97.69%	77.77%	100%	97.69%	192-193, 236-237, 240-241, 296-297
src/repository/storage/postgres/orm/sequelize/noteRelations.ts	100%	94.44%	100%	100%
src/repository/storage/postgres/orm/sequelize/teams.ts	99.19%	85.71%	100%	99.19%	195-196

Generated in workflow #849

src/domain/service/note.ts

src/domain/entities/note.ts

src/repository/storage/postgres/orm/sequelize/teams.ts

src/domain/entities/note.ts

src/repository/storage/postgres/orm/sequelize/teams.ts

e11sy · 2024-09-06T11:17:48Z

you can also patch GET /note endpoint and related test in this pr on in the following one

Good job, almost done!

…by adding note parrent structure

dependentmadani · 2024-09-09T20:43:47Z

resolve #275

docker-compose.yml

src/repository/storage/postgres/orm/sequelize/teams.ts

src/presentation/http/router/note.test.ts

src/domain/entities/note.ts

src/domain/service/note.ts

src/presentation/http/router/note.test.ts

src/presentation/http/router/note.ts

src/repository/storage/postgres/orm/sequelize/teams.ts

… to be fixed

… a test, tests work in progress

src/repository/storage/postgres/orm/sequelize/teams.ts

docker-compose.yml

src/presentation/http/router/note.test.ts

neSpecc · 2024-09-16T10:37:35Z

docker-compose.yml

unexpected change

src/domain/service/note.ts

src/repository/note.repository.ts

neSpecc · 2024-09-16T10:46:01Z

src/repository/storage/postgres/orm/sequelize/note.ts

+  public createAssociationWithTeamsModel(model: ModelStatic<TeamsModel>): void {
+    /**
+     * Create association with teams
+     */
+    this.teamModel = model;
+  }


I'm not sure that you need to access team model here. You can return all note parents by getNoteParents.

If you want to skip notes where user has no access, you can do it on a domain level. But I'm not sure that is is a real case. Is it possible that user has an access to a particular note, but has no access to it's parent?

Probably if there was a nested note relation, with different team members, where there could be some missing role access for team member in some specific notes. I might be wrong, that was based on what I understand from the code. 👀

I still think, that such a coupling of two independent modules (note and team) is bad idea. Such domains designed to be decoupled from each other.

My suggestion is to return all note parents. The only check is that user should has and access to the target note — it can be checked on a presentation (http) level via the notePublicOrUserInTeam policy

it seems like creation of a golem, user would have ability to see notes which he has no access to (we return note with full content)
like in default cases
privateNote -> privateNote -> userInThisNoteTeam
all privateNotes should not be returned in this case but in your suggestion user would have ability to see full note contents of them

i think that we can use shared domains and move this logic to domain level as trade-off between speed and architectural purity

I think we can keep it simple for the moment, as @neSpecc proposed, by returning all the note parents, when the user try to access the note, and he does not have access to, the return of content will be blocked by the policy used.

src/repository/storage/postgres/orm/sequelize/note.ts

…in tests instead of individual test cases

src/presentation/http/router/note.test.ts

e11sy · 2024-09-19T16:33:25Z

src/presentation/http/router/note.test.ts

+      /** Returns one parent in case where there is no note relation with 200 status */
+      {
+        numberOfNotes: 1,
+        userNoteCreationDifferent: false,


naming is not clear

src/presentation/http/router/note.test.ts

neSpecc · 2024-09-20T22:52:32Z

src/presentation/http/router/note.test.ts

+      grandChildNote: {} as Note,
+    };
+
+    beforeEach(async () => {


this may affect all existing tests. What is the reason to add it event in tests that does not need it?

neSpecc · 2024-09-20T22:53:17Z

src/presentation/http/router/note.test.ts

+    test.each([
+      /** Returns two parents in case of relation between child and parent notes with 200 status */
+      {
+        testCase: 1,


testCase — unclear variable

neSpecc · 2024-09-20T22:53:27Z

src/presentation/http/router/note.test.ts

+        isPublic: true,
+        expectedStatusCode: 200,
+      },
+    ])('Get note parents in different scenarios', async ({ testCase, numberOfNotes, childNoteCreatedByDifferentUser, isPublic, expectedStatusCode }) => {


unclear test case name

neSpecc · 2024-09-20T22:55:33Z

src/presentation/http/router/note.test.ts

+          url: `/note/${notePublicId}`,
+        });
+
+        switch (testCase) {


its hard to read such a big and complex text. Test.each designed for simple tests. In current implementation it decreases readability

I tried to make it clearer, by providing testCase variable, that indicates which object in test.each. testCase is like an incremental index over the array of objects in test.each.

neSpecc · 2024-09-20T22:56:19Z

src/domain/service/note.ts

+   * @param userId - id of the user that is requesting the parent structure
+   * @returns - array of notes that are parent structure of the note
+   */
+  public async getNoteParentStructure(noteId: NoteInternalId, userId: number): Promise<NotePublic[]> {


Suggested change

public async getNoteParentStructure(noteId: NoteInternalId, userId: number): Promise<NotePublic[]> {

public async getNoteParents(noteId: NoteInternalId, userId: number): Promise<NotePublic[]> {

neSpecc · 2024-09-20T23:03:39Z

src/repository/storage/postgres/orm/sequelize/note.ts

+  public createAssociationWithTeamsModel(model: ModelStatic<TeamsModel>): void {
+    /**
+     * Create association with teams
+     */
+    this.teamModel = model;
+  }


I still think, that such a coupling of two independent modules (note and team) is bad idea. Such domains designed to be decoupled from each other.

My suggestion is to return all note parents. The only check is that user should has and access to the target note — it can be checked on a presentation (http) level via the notePublicOrUserInTeam policy

src/repository/storage/postgres/orm/sequelize/note.ts

neSpecc · 2024-09-25T12:50:12Z

src/repository/note.repository.ts

+   * @param noteId : note id to get all its parents
+   * @returns an array of note parents objects containing public id and content
+   */
+  public async getNoteParents(noteId: NoteInternalId): Promise<Note[]> {


I'd suggest to move this method to the NoteRelationsRepository since it is responsible for working with note relations.

And remove added association from Note Storage.

Repository can return a list of NoteInternalId

Then on Domain level (in a Note Service) you can access Note Repository for getting Notes by ids.

For doing this, you can implement getNotesByIds method in Note Repository and Store.

src/presentation/http/router/note.test.ts

neSpecc · 2024-09-25T12:57:56Z

src/domain/service/note.ts

+  public async getNoteParents(noteId: NoteInternalId): Promise<NotePublic[]> {
+    const noteParents = await this.noteRepository.getNoteParents(noteId);
+    const noteParentsPublic: NotePublic[] = noteParents.map((note) => {
+      return {


consider using definePublicNote() util

…odification of logic

neSpecc · 2024-09-27T13:10:41Z

src/domain/service/note.ts

+   */
+  public async getNoteParents(noteId: NoteInternalId): Promise<NotePublic[]> {
+    const noteIds: NoteInternalId[] = await this.noteRelationsRepository.getNoteParentsIds(noteId);
+    const noteParents = await this.noteRelationsRepository.getNotesByIds(noteIds);


getNoteByIds should belong to NoteRepository

neSpecc · 2024-09-27T13:13:28Z

src/presentation/http/router/note.test.ts

@@ -518,6 +564,221 @@ describe('Note API', () => {

      expect(response?.json().message).toStrictEqual(expectedMessage);
    });
+
+    test('Returns two parents note in case of relation between child and parent note with status 200', async () => {


Suggested change

test('Returns two parents note in case of relation between child and parent note with status 200', async () => {

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

neSpecc · 2024-09-27T13:13:47Z

src/presentation/http/router/note.test.ts

+      });
+    });
+
+    test('Returns three parents note in case of relation between all notes with status 200', async () => {


unclear case name, especially this:

in case of relation between all notes

neSpecc · 2024-09-27T13:17:00Z

src/presentation/http/router/note.test.ts

+      });
+    });
+
+    test('Returns two parents note in case where the note is not created by the same user but share relation with status 200', async () => {


why do we need this test? is't it repeat the first one?

it is not totally the same as the first one, as here there is one note created by user, and parent note created by anotherUser, but there is a note relation between both notes.

neSpecc · 2024-09-27T13:17:23Z

src/presentation/http/router/note.test.ts

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


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

neSpecc · 2024-09-27T13:18:01Z

src/repository/index.ts

@@ -139,6 +139,7 @@ export async function init(orm: Orm, s3Config: S3StorageConfig): Promise<Reposit
  /**
   * Create associations between note and relations table
   */
+  noteStorage.createAssociationWithNoteRelationModel(noteRelationshipStorage.model);


redundant association

neSpecc · 2024-09-27T13:18:28Z

src/repository/storage/postgres/orm/sequelize/noteRelations.ts

+      const note: Note | null = await this.noteModel.findOne({
+        where: { id: currentNoteId },
+      });
+
+      if (notEmpty(note)) {
+        parentNotes.push(note.id);
+      }


seems redundant

neSpecc · 2024-09-27T13:19:06Z

src/repository/storage/postgres/orm/sequelize/noteRelations.ts

+      const noteRelation: NoteRelationsModel | null = await this.model.findOne({
+        where: { noteId: currentNoteId },
+      });


I think, you can try to get all note ids via a singe sql query instead of many

…to feat/return-note

e11sy · 2024-09-28T19:23:11Z

src/presentation/http/router/note.test.ts

+      });
+    });
+
+    test('Returns one parents note in case where the note is not created by the same user but share relation', async () => {


explain that user is not in parent note team, that would be more clear

src/repository/note.repository.ts

e11sy · 2024-10-03T16:23:09Z

src/presentation/http/router/note.test.ts

@@ -518,6 +518,221 @@ describe('Note API', () => {

      expect(response?.json().message).toStrictEqual(expectedMessage);
    });
+
+    test('Returns one parents note in case when note has one parents', async () => {


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 () => {

e11sy · 2024-10-03T16:23:34Z

src/presentation/http/router/note.test.ts

+      });
+    });
+
+    test('Returns two parents note in case when note has two parents', async () => {


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 () => {

e11sy · 2024-10-03T16:23:55Z

src/presentation/http/router/note.test.ts

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


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 () => {

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

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?

neSpecc · 2024-10-03T16:57:19Z

src/domain/service/note.ts

+   * @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[]> {


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

neSpecc · 2024-10-03T16:58:35Z

src/presentation/http/router/note.test.ts

+          {
+            id: parentNote.publicId,
+            content: parentNote.content,
+          },
+          {
+            id: childNote.publicId,
+            content: childNote.content,
+          },


Returns one parents note in case when note has one parents

so why there are two notes?

neSpecc · 2024-10-03T16:59:41Z

src/presentation/http/router/note.test.ts

+      });
+    });
+
+    test('Returns two parents note in case when note has two parents', async () => {


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

neSpecc · 2024-10-03T17:00:30Z

src/presentation/http/router/note.test.ts

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


shares a relation

unclear statement

neSpecc · 2024-10-03T17:01:59Z

src/repository/noteRelations.repository.ts

+   * @returns an array of note parents ids
+   */
+  public async getNoteParentsIds(noteId: NoteInternalId): Promise<NoteInternalId[]> {
+    return await this.storage.getAllNoteParentsIds(noteId);


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

neSpecc · 2024-10-03T17:02:54Z

src/repository/storage/postgres/orm/sequelize/note.ts

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


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

neSpecc · 2024-10-03T17:03:13Z

src/repository/storage/postgres/orm/sequelize/noteRelations.ts

+    let currentNoteId: number | null = noteId;
+
+    // get all note ids via a singe sql query instead of many
+    while (currentNoteId !== null) {


have you tried to resolve it by a single sql query?

neSpecc · 2024-10-03T17:03:48Z

src/repository/storage/postgres/orm/sequelize/teams.ts

+  /**
+   * Note relation content
+   */
+  public declare notes?: NoteModel | null;


seems unused

feat(parent structure): add method that return note parent structure

df44b32

dependentmadani requested review from neSpecc and e11sy August 29, 2024 10:37

e11sy requested changes Aug 29, 2024

View reviewed changes

src/domain/service/note.ts Outdated Show resolved Hide resolved

src/domain/service/note.ts Outdated Show resolved Hide resolved

src/domain/service/note.ts Outdated Show resolved Hide resolved

update: start doing the search in team storage

0551b9f

dependentmadani requested a review from e11sy September 1, 2024 10:59

e11sy requested changes Sep 3, 2024

View reviewed changes

dependentmadani added 2 commits September 4, 2024 01:16

update: first part of code review is done

a5adcc2

fix: finish the review modifications

bccf7b1

dependentmadani requested a review from e11sy September 4, 2024 21:34

e11sy reviewed Sep 6, 2024

View reviewed changes

dependentmadani added 5 commits September 6, 2024 18:37

update: first part of modification after review

42cb421

fix: build problem fixed

bb6caff

update(note request): update the content send in get request of note …

a11d1f8

…by adding note parrent structure

update(tests): add different tests for the note parent structure

0e3e8f0

update(tests): tests are now functional

3adff28

dependentmadani requested a review from e11sy September 9, 2024 17:30

e11sy reviewed Sep 10, 2024

View reviewed changes

docker-compose.yml Outdated Show resolved Hide resolved

src/repository/storage/postgres/orm/sequelize/teams.ts Outdated Show resolved Hide resolved

src/presentation/http/router/note.test.ts Show resolved Hide resolved

neSpecc requested changes Sep 13, 2024

View reviewed changes

dependentmadani added 4 commits September 15, 2024 00:17

update (parents note): modification based on the reviews, still tests…

d6c9ded

… to be fixed

update: fix lint

6bdc56a

update (note parents): few modification in the return value type, add…

6cf3d4c

… a test, tests work in progress

update: add a test

1809ca1

e11sy reviewed Sep 15, 2024

View reviewed changes

src/repository/storage/postgres/orm/sequelize/teams.ts Show resolved Hide resolved

docker-compose.yml Show resolved Hide resolved

src/presentation/http/router/note.test.ts Outdated Show resolved Hide resolved

neSpecc reviewed Sep 16, 2024

View reviewed changes

update (note parents): modifications based on reviews, use test.each …

ca7a9a2

…in tests instead of individual test cases

dependentmadani requested a review from neSpecc September 18, 2024 22:03

dependentmadani requested a review from e11sy September 18, 2024 22:03

e11sy reviewed Sep 19, 2024

View reviewed changes

update (parent note test): modification based on previous reviews

e14d948

neSpecc requested changes Sep 20, 2024

View reviewed changes

dependentmadani added 2 commits September 23, 2024 20:44

update (parent notes): few modification done, still work to be done

5c6cfc0

update (note parents): fix the issue of tests, working good

5bb6be5

dependentmadani requested review from e11sy and neSpecc September 24, 2024 19:13

neSpecc requested changes Sep 25, 2024

View reviewed changes

dependentmadani added 2 commits September 26, 2024 13:17

update (note parents): modification based on previous review, chore m…

fee9bbf

…odification of logic

update (note parents tests): the issue of test failure is fixed

d65c589

neSpecc reviewed Sep 27, 2024

View reviewed changes

dependentmadani added 3 commits September 27, 2024 19:18

udpate (note parents): based on last review

14234a1

Merge branch 'feat/return-note' of github.com:codex-team/notes.api in…

c09ad3a

…to feat/return-note

update (test cases): the issue is fixed due to missing an id

9872d5e

e11sy reviewed Sep 28, 2024

View reviewed changes

update (test case): change the naming of a test case

5a9fad2

dependentmadani requested review from neSpecc and e11sy October 1, 2024 11:27

e11sy reviewed Oct 3, 2024

View reviewed changes

neSpecc requested changes Oct 3, 2024

View reviewed changes

	public async getNoteParentStructure(noteId: NoteInternalId, userId: number): Promise<NotePublic[]> {
	public async getNoteParents(noteId: NoteInternalId, userId: number): Promise<NotePublic[]> {

	test('Returns two parents note in case of relation between child and parent note with status 200', async () => {
	test('Returns two parents note in case when note has two parents', async () => {

	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 () => {

	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 () => {

	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 () => {

feat(parent structure): add method that return note parent structure #276

Are you sure you want to change the base?

feat(parent structure): add method that return note parent structure #276

Conversation

dependentmadani commented Aug 29, 2024

Problem:

Solution:

github-actions bot commented Aug 29, 2024 • edited Loading

Coverage Report

e11sy commented Sep 6, 2024

dependentmadani commented Sep 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dependentmadani Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 29, 2024 •

edited

Loading

dependentmadani Sep 27, 2024 •

edited

Loading