From c79f9523f8c0d911896422b2a398a95e4153e5c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20Jedlicska?= Date: Thu, 28 Oct 2021 11:28:34 +0200 Subject: [PATCH 1/3] feat((server) branche ordering): query branches without cursor orders by createdAt fixes #119 --- .../server/modules/core/services/branches.js | 12 ++-- .../modules/core/tests/branches.spec.js | 58 ++++++++++++------- 2 files changed, 44 insertions(+), 26 deletions(-) diff --git a/packages/server/modules/core/services/branches.js b/packages/server/modules/core/services/branches.js index 34faf6048..1d2602d05 100644 --- a/packages/server/modules/core/services/branches.js +++ b/packages/server/modules/core/services/branches.js @@ -18,7 +18,7 @@ module.exports = { branch.name = name.toLowerCase( ) branch.description = description - if(name) module.exports.validateBranchName( { name } ) + if ( name ) module.exports.validateBranchName( { name } ) let [ id ] = await Branches( ).returning( 'id' ).insert( branch ) @@ -45,10 +45,12 @@ module.exports = { limit = limit || 25 let query = Branches( ).select( '*' ).where( { streamId: streamId } ) - if ( cursor ) - query.andWhere( 'updatedAt', '<', cursor ) - - query.orderBy( 'updatedAt', 'desc' ).limit( limit ) + if ( cursor ) { + query.andWhere( 'updatedAt', '<', cursor ).orderBy( 'updatedAt', 'desc' ) + } else { + query.orderBy( 'createdAt' ) + } + query.limit( limit ) let totalCount = await module.exports.getBranchesByStreamIdTotalCount( { streamId } ) let rows = await query diff --git a/packages/server/modules/core/tests/branches.spec.js b/packages/server/modules/core/tests/branches.spec.js index 743c6469d..891e13327 100644 --- a/packages/server/modules/core/tests/branches.spec.js +++ b/packages/server/modules/core/tests/branches.spec.js @@ -22,8 +22,9 @@ const { getBranchByNameAndStreamId, deleteBranchById } = require( '../services/branches' ) +const { async } = require( 'crypto-random-string' ) -describe( 'Branches @core-branches', ( ) => { +describe( 'Branches @core-branches', () => { let user = { name: 'Dimitrie Stefanescu', email: 'didimitrie4342@gmail.com', @@ -40,9 +41,9 @@ describe( 'Branches @core-branches', ( ) => { baz: 'qux' } - before( async ( ) => { - await knex.migrate.rollback( ) - await knex.migrate.latest( ) + before( async () => { + await knex.migrate.rollback() + await knex.migrate.latest() await init() @@ -51,19 +52,19 @@ describe( 'Branches @core-branches', ( ) => { testObject.id = await createObject( stream.id, testObject ) } ) - after( async ( ) => { - await knex.migrate.rollback( ) + after( async () => { + await knex.migrate.rollback() } ) let branch = { name: 'dim/dev' } - it( 'Should create a branch', async ( ) => { + it( 'Should create a branch', async () => { branch.id = await createBranch( { ...branch, streamId: stream.id, authorId: user.id } ) expect( branch.id ).to.be.not.null expect( branch.id ).to.be.a.string } ) - it( 'Should not allow duplicate branch names', async ( ) => { + it( 'Should not allow duplicate branch names', async () => { try { await createBranch( { name: 'main', streamId: stream.id, authorId: user.id } ) assert.fail( 'Duplicate branches should not be allowed.' ) @@ -72,7 +73,7 @@ describe( 'Branches @core-branches', ( ) => { } } ) - it( 'Should not allow branch names starting with # or /', async ( ) => { + it( 'Should not allow branch names starting with # or /', async () => { try { await createBranch( { name: '/pasta', streamId: stream.id, authorId: user.id } ) assert.fail( 'Illegal branch name passed through.' ) @@ -102,36 +103,36 @@ describe( 'Branches @core-branches', ( ) => { } } ) - it( 'Branch names should be case insensitive (always lowercase)', async ( ) => { + it( 'Branch names should be case insensitive (always lowercase)', async () => { let id = await createBranch( { name: 'CaseSensitive', streamId: stream.id, authorId: user.id } ) - - let b = await getBranchByNameAndStreamId( { streamId: stream.id, name:'casesensitive' } ) + + let b = await getBranchByNameAndStreamId( { streamId: stream.id, name: 'casesensitive' } ) expect( b.name ).to.equal( 'casesensitive' ) - let bb = await getBranchByNameAndStreamId( { streamId: stream.id, name:'CaseSensitive' } ) + let bb = await getBranchByNameAndStreamId( { streamId: stream.id, name: 'CaseSensitive' } ) expect( bb.name ).to.equal( 'casesensitive' ) - let bbb = await getBranchByNameAndStreamId( { streamId: stream.id, name:'CASESENSITIVE' } ) + let bbb = await getBranchByNameAndStreamId( { streamId: stream.id, name: 'CASESENSITIVE' } ) expect( bbb.name ).to.equal( 'casesensitive' ) // cleanup await deleteBranchById( { id, streamId: stream.id } ) } ) - it( 'Should get a branch', async ( ) => { + it( 'Should get a branch', async () => { let myBranch = await getBranchById( { id: branch.id } ) expect( myBranch.authorId ).to.equal( user.id ) expect( myBranch.streamId ).to.equal( stream.id ) } ) - it( 'Should update a branch', async ( ) => { + it( 'Should update a branch', async () => { await updateBranch( { id: branch.id, description: 'lorem ipsum' } ) let b1 = await getBranchById( { id: branch.id } ) expect( b1.description ).to.equal( 'lorem ipsum' ) } ) - it( 'Should get all stream branches', async ( ) => { + it( 'Should get all stream branches', async () => { await createBranch( { name: 'main-faster', streamId: stream.id, authorId: user.id } ) await createBranch( { name: 'main-blaster', streamId: stream.id, authorId: user.id } ) await createBranch( { name: 'blaster-farter', streamId: stream.id, authorId: user.id } ) @@ -142,19 +143,34 @@ describe( 'Branches @core-branches', ( ) => { expect( totalCount ).to.exist } ) - it( 'Should delete a branch', async ( ) => { + it( 'Should delete a branch', async () => { await deleteBranchById( { id: branch.id, streamId: stream.id } ) let { items } = await getBranchesByStreamId( { streamId: stream.id } ) expect( items ).to.have.lengthOf( 4 ) } ) - it( 'Should NOT delete the main branch', async ( ) => { + it( 'Should NOT delete the main branch', async () => { let b = await getBranchByNameAndStreamId( { streamId: stream.id, name: 'main' } ) try { await deleteBranchById( { id: b.id, streamId: stream.id } ) assert.fail() - } catch ( e ){ + } catch ( e ) { // pass } } ) -} ) + + it( 'Should return branches in updatedAt order when using cursor', async () => { + let { items } = await getBranchesByStreamId( { streamId: stream.id } ) + let branch = items[3] + await updateBranch( { id: branch.id, description: 'lorem ipsum' } ) + + let cursor = new Date().toISOString() + let got = await getBranchesByStreamId( { streamId: stream.id, cursor: cursor } ) + expect( got.items[0].name ).to.equal( branch.name ) + } ) + + it( 'Should return branches in time createdAt order, MAIN first', async () => { + let { items } = await getBranchesByStreamId( { streamId: stream.id } ) + expect( items[0].name ).to.equal( 'main' ) + } ) +} ) \ No newline at end of file From 77363aeb98d5f11540c4c07258669d3c082f501c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20Jedlicska?= Date: Thu, 28 Oct 2021 17:56:53 +0200 Subject: [PATCH 2/3] feat(server branch query): always order by createdAt --- .../server/modules/core/services/branches.js | 8 ++------ .../server/modules/core/tests/branches.spec.js | 18 ++++++++---------- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/packages/server/modules/core/services/branches.js b/packages/server/modules/core/services/branches.js index 1d2602d05..afc0ef5f9 100644 --- a/packages/server/modules/core/services/branches.js +++ b/packages/server/modules/core/services/branches.js @@ -45,12 +45,8 @@ module.exports = { limit = limit || 25 let query = Branches( ).select( '*' ).where( { streamId: streamId } ) - if ( cursor ) { - query.andWhere( 'updatedAt', '<', cursor ).orderBy( 'updatedAt', 'desc' ) - } else { - query.orderBy( 'createdAt' ) - } - query.limit( limit ) + if ( cursor ) query.andWhere( 'createdAt', '<', cursor ) + query.orderBy( 'createdAt' ).limit( limit ) let totalCount = await module.exports.getBranchesByStreamIdTotalCount( { streamId } ) let rows = await query diff --git a/packages/server/modules/core/tests/branches.spec.js b/packages/server/modules/core/tests/branches.spec.js index 891e13327..ad6d844e9 100644 --- a/packages/server/modules/core/tests/branches.spec.js +++ b/packages/server/modules/core/tests/branches.spec.js @@ -159,18 +159,16 @@ describe( 'Branches @core-branches', () => { } } ) - it( 'Should return branches in updatedAt order when using cursor', async () => { - let { items } = await getBranchesByStreamId( { streamId: stream.id } ) - let branch = items[3] - await updateBranch( { id: branch.id, description: 'lorem ipsum' } ) - - let cursor = new Date().toISOString() - let got = await getBranchesByStreamId( { streamId: stream.id, cursor: cursor } ) - expect( got.items[0].name ).to.equal( branch.name ) - } ) - it( 'Should return branches in time createdAt order, MAIN first', async () => { let { items } = await getBranchesByStreamId( { streamId: stream.id } ) expect( items[0].name ).to.equal( 'main' ) + + let branch = items[3] + await updateBranch( { id: branch.id, description: 'lorem ipsum' } ) + let cursor = new Date().toISOString() + let got = await getBranchesByStreamId( { streamId: stream.id, cursor: cursor } ) + + expect( got.items[3].name ).to.equal( branch.name ) + expect( got.items[0].name ).to.equal( 'main' ) } ) } ) \ No newline at end of file From de9dcb5258bd2c663ea5567cca34de758d86cc5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20Jedlicska?= Date: Thu, 28 Oct 2021 19:11:17 +0200 Subject: [PATCH 3/3] fix(server branches.spec): remove random crypto-random --- packages/server/modules/core/tests/branches.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/server/modules/core/tests/branches.spec.js b/packages/server/modules/core/tests/branches.spec.js index ad6d844e9..dc08bf50a 100644 --- a/packages/server/modules/core/tests/branches.spec.js +++ b/packages/server/modules/core/tests/branches.spec.js @@ -22,7 +22,7 @@ const { getBranchByNameAndStreamId, deleteBranchById } = require( '../services/branches' ) -const { async } = require( 'crypto-random-string' ) + describe( 'Branches @core-branches', () => { let user = {