feat(routes): get all notes by parent note id #282
Conversation
Tushar504
commented
Nov 5, 2024
•
Tushar504
commented
- This PR adds the api endpoint and tests to get all notes by parent note as described in Issue Closes (First task): add GET all notes by parentNote endpoint #279 .
- Added getNoteListByParentNote() method for NoteService
- Added getNoteListByParentNote() method for NoteRepository
- Added getNoteListByParentNote() method for NoteStorage
- Updated getTeamMembersWithUserInfoByNoteId() method for teamStorage (to include UserId)
- Added tests for the route
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| policy: [ | ||
| 'authRequired', | ||
| 'notePublicOrUserInTeam', | ||
| ], |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
i have updated the notResolver to consider parentId as well
There was a problem hiding this comment.
i think that this is not a case for noteResolver usage, because in some case user can pass noteId and noteParentId in one request
There was a problem hiding this comment.
let me know, which policies required here
There was a problem hiding this comment.
i think that we should check it without policy and send reply.forbidden if user is not in team of the parent note
There was a problem hiding this comment.
request you to review new changes
| @@ -144,3 +144,130 @@ describe('GET /notes?page', () => { | |||
| } | |||
| }); | |||
| }); | |||
|
|
|||
| describe('GET /notes/:parentNoteId?page', () => { | |||
There was a problem hiding this comment.
no tests for userInTeam policy
| noteStorage.createAssociationWithNoteRelationsModel(noteRelationshipStorage.model); | ||
| noteRelationshipStorage.createAssociationWithNoteModel(noteStorage.model); |
There was a problem hiding this comment.
we need that association to use include clause(join table)
There was a problem hiding this comment.
as i can see, we already linked noteRelationshipStorage with noteStorage
There was a problem hiding this comment.
should i implement this getNoteListByParentNote() method in noteRelationshipStorage
There was a problem hiding this comment.
no, but do you actually need to make another assotiation in addition to the existing?
There was a problem hiding this comment.
it is required, i refered other relationships implemenation(noteVisits, noteSetting)
| const childNotes = await this.relationsModel.findAll({ | ||
| where: { parentId }, | ||
| attributes: ['noteId'], | ||
| }); | ||
|
|
||
| const noteIds = childNotes.map(relation => relation.noteId); | ||
|
|
||
| const reply = await this.model.findAll({ | ||
| where: { | ||
| id: { | ||
| [Op.in]: noteIds, | ||
| }, | ||
| }, | ||
| include: [{ | ||
| model: this.settingsModel, | ||
| as: 'noteSettings', | ||
| attributes: ['cover'], | ||
| duplicating: false, | ||
| }], | ||
| offset, | ||
| limit, | ||
| }); |
There was a problem hiding this comment.
it's better to do it via one request
Tushar504
changed the title
