diff --git a/packages/server/modules/comments/graph/resolvers/comments.js b/packages/server/modules/comments/graph/resolvers/comments.js index 78c79f7bc..123980291 100644 --- a/packages/server/modules/comments/graph/resolvers/comments.js +++ b/packages/server/modules/comments/graph/resolvers/comments.js @@ -69,8 +69,8 @@ module.exports = { await authorizeResolver( context.userId, args.input.streamId, 'stream:reviewer' ) // the reply also has to be linked to the stream, for the recursive reply lookup to work let input = { ...args.input, resources: [ - { id: args.input.parentComment, type: 'comment' }, - { id: args.input.streamId, type: 'stream' } + { resourceId: args.input.parentComment, resourceType: 'comment' }, + { resourceId: args.input.streamId, resourceType: 'stream' } ] } let id = await createComment( { userId: context.userId, input } ) await pubsub.publish( 'COMMENT_REPLY_CREATED', { diff --git a/packages/server/modules/comments/migrations/20220222173000_comments.js b/packages/server/modules/comments/migrations/20220222173000_comments.js index 5dfd63047..2dae939de 100644 --- a/packages/server/modules/comments/migrations/20220222173000_comments.js +++ b/packages/server/modules/comments/migrations/20220222173000_comments.js @@ -3,8 +3,9 @@ exports.up = async ( knex ) => { await knex.schema.createTable( 'comments', table => { table.string( 'id', 10 ).primary( ) table.string( 'authorId', 10 ).references( 'id' ).inTable( 'users' ).notNullable().index( ) - table.timestamp( 'createdAt' ).defaultTo( knex.fn.now( ) ) - table.timestamp( 'updatedAt' ).defaultTo( knex.fn.now( ) ) + // table.timestamp( 'createdAt' ).defaultTo( knex.fn.now( ) ) + table.specificType( 'createdAt', 'TIMESTAMPTZ(3)' ).defaultTo( knex.fn.now( ) ) + table.specificType( 'updatedAt', 'TIMESTAMPTZ(3)' ).defaultTo( knex.fn.now( ) ) table.string( 'text' ) table.text( 'screenshot' ) table.jsonb( 'data' ) diff --git a/packages/server/modules/comments/services/index.js b/packages/server/modules/comments/services/index.js index 97fa72c63..7edfe9683 100644 --- a/packages/server/modules/comments/services/index.js +++ b/packages/server/modules/comments/services/index.js @@ -50,9 +50,21 @@ const getCommentLinksForResources = async ( streamId, resources ) => { // since all other types are directly linked to a stream commentLinks = commentLinks.filter( link => link.resourceType === 'object' ? streamObjectIds.includes( link.resourceId ) : true ) } - // let commentGroups = {} - // for (const link of commentLinks) - return commentLinks + + // group comment links by comment ids, so that the resources can be filtered below + let commentGroups = {} + for ( const link of commentLinks ) { + if ( !( link.commentId in commentGroups ) ) commentGroups[link.commentId] = [] + commentGroups[link.commentId].push( link.resourceId ) + } + + const relevantCommentIds = Object + .keys( commentGroups ) + .filter( + // make sure, that the given comment targets exactly the same set of resources, as the input requested + commentId => commentGroups[commentId].length === resourceIds.length && resourceIds.every( resId => commentGroups[commentId].includes( resId ) ) + ) + return commentLinks.filter( l => relevantCommentIds.includes( l.commentId ) ) } module.exports = { @@ -97,8 +109,10 @@ module.exports = { const commentLinks = await getCommentLinksForResources( streamId, resources ) const relevantComments = [ ...new Set( commentLinks.map( l => l.commentId ) ) ] let query = Comments().whereIn( 'id', relevantComments ).orderBy( 'createdAt' ) - if ( cursor ) query = query.where( 'createdAt', '>', cursor ) - let items = await query.limit( limit ) + if ( cursor ) query = query.where( 'createdAt', '>', cursor.toISOString() ) + + const defaultLimit = 100 + let items = await query.limit( limit ?? defaultLimit ) if ( items.length ) { cursor = items[items.length - 1].createdAt } else { diff --git a/packages/server/modules/comments/tests/comments.spec.js b/packages/server/modules/comments/tests/comments.spec.js index 4eca10240..77499ca53 100644 --- a/packages/server/modules/comments/tests/comments.spec.js +++ b/packages/server/modules/comments/tests/comments.spec.js @@ -59,8 +59,12 @@ describe( 'Comments @comments', () => { .then( () => { throw new Error( 'This should have been rejected' ) } ) .catch( error => expect( error.message ).to.be.equal( 'Must specify atleast one resource as the comment target' ) ) } ) - it( 'Should not be allowed to comment with mismatching streamId and stream resourceId', async ( ) => { - return await createComment( { + it( 'Should not be able to comment resources that do not belong to the input streamId', async ( ) => { + // need to check streamId - commit link + // need to check streamId - object link + // need to check streamId - stream match + // need to check comment reply recursively? that sounds too much of an effort + await createComment( { userId: user.id, input: { streamId: stream.id, @@ -75,6 +79,9 @@ describe( 'Comments @comments', () => { } ) .then( () => { throw new Error( 'This should have been rejected' ) } ) .catch( error => expect( error.message ).to.be.equal( 'Input streamId doesn\'t match the stream resource.resourceId' ) ) + + //add the checks from above + expect( 1 ).to.equal( 2 ) } ) it( 'Should not be allowed to comment targeting multiple streams as a resource', async ( ) => { return await createComment( { @@ -154,12 +161,6 @@ describe( 'Comments @comments', () => { .catch( error => expect( error.message ).to.equal( 'resource type flux capacitor is not supported as a comment target' ) ) } ) - it( 'Should not be able to comment resources that do not belong to the input streamId', async () => { - // need to check streamId - commit link - // need to check streamId - object link - // need to check streamId - stream match - // need to check comment reply recursively? that sounds too much of an effort - } ) it( 'Should be able to comment on valid resources in any permutation', async () => { //comment on branches too!!! const resourceCombinations = [ @@ -220,7 +221,7 @@ describe( 'Comments @comments', () => { const localObjectId = await createObject( stream.id, { testObject: 1 } ) const commentCount = 3 - for ( let i = 0; i <= commentCount; i++ ) { + for ( let i = 0; i < commentCount; i++ ) { await createComment( { userId: user.id, input: { @@ -242,12 +243,59 @@ describe( 'Comments @comments', () => { ] } ) expect( comments.items ).to.have.lengthOf( commentCount ) } ) - it( 'Should handle cursor and limit for queries' ) + it( 'Should handle cursor and limit for queries', async ( ) => { + const localObjectId = await createObject( stream.id, { testObject: 'something completely different' } ) + + let createdComments = [] + const commentCount = 10 + for ( let i = 0; i < commentCount; i++ ) { + createdComments.push( await createComment( { + userId: user.id, + input: { + streamId: stream.id, + resources: [ + { resourceId: stream.id, resourceType: 'stream' }, + { resourceId: commitId1, resourceType: 'commit' }, + { resourceId: localObjectId, resourceType: 'object' } + ], + text: crs( { length: 10 } ), + data: { justSome: crs( { length: 10 } ) } + } + } ) ) + await new Promise( resolve => setTimeout( resolve, 500 ) ) + } + + let comments = await getComments( { + streamId: stream.id, + resources: [ + { resourceId: commitId1, resourceType: 'commit' }, + { resourceId: localObjectId, resourceType: 'object' } + ], + limit : 2 + } ) + expect( comments.items ).to.have.lengthOf( 2 ) + expect( createdComments.slice( 0, 2 ) ).deep.to.equal( comments.items.map( c => c.id ) ) + + const cursor = comments.items[1].createdAt + comments = await getComments( { + streamId: stream.id, + resources: [ + { resourceId: commitId1, resourceType: 'commit' }, + { resourceId: localObjectId, resourceType: 'object' } + ], + limit : 2, + cursor + } ) + expect( comments.items ).to.have.lengthOf( 2 ) + expect( createdComments.slice( 2, 4 ) ).deep.to.equal( comments.items.map( c => c.id ) ) + } ) it( 'Should handle very and limit for queries' ) + it( 'Should properly return replies for a comment' ) it( 'Should return all the referenced resources for a comment' ) it( 'Should be able to edit a comment text and its context???' ) it( 'Should not be allowed to edit a not existing comment' ) it( 'Should be able to archive a comment' ) + it( 'Replies to archived comment should be archived, when a parent is archived' ) it( 'Should not be allowed to archive a not existing comment' ) it( 'Should not return archived comments in plain queries' ) it( 'Should return archived comments if explicitly asked for them' )