From a8c6534347d4961acad4328914d371cf2599165a Mon Sep 17 00:00:00 2001 From: Ulf Gebhardt Date: Fri, 3 Feb 2023 01:17:15 +0100 Subject: [PATCH 1/7] fix logger on contributionResolver --- .../resolver/ContributionResolver.test.ts | 124 +++++++++------- .../graphql/resolver/ContributionResolver.ts | 135 +++++++----------- 2 files changed, 123 insertions(+), 136 deletions(-) diff --git a/backend/src/graphql/resolver/ContributionResolver.test.ts b/backend/src/graphql/resolver/ContributionResolver.test.ts index d48502c69..0078ecafd 100644 --- a/backend/src/graphql/resolver/ContributionResolver.test.ts +++ b/backend/src/graphql/resolver/ContributionResolver.test.ts @@ -132,13 +132,13 @@ describe('ContributionResolver', () => { }), ).resolves.toEqual( expect.objectContaining({ - errors: [new GraphQLError('memo text is too short (5 characters minimum)')], + errors: [new GraphQLError('Memo text is too short')], }), ) }) it('logs the error found', () => { - expect(logger.error).toBeCalledWith(`memo text is too short: memo.length=4 < 5`) + expect(logger.error).toBeCalledWith('Memo text is too short', 4) }) it('throws error when memo length greater than 255 chars', async () => { @@ -155,13 +155,13 @@ describe('ContributionResolver', () => { }), ).resolves.toEqual( expect.objectContaining({ - errors: [new GraphQLError('memo text is too long (255 characters maximum)')], + errors: [new GraphQLError('Memo text is too long')], }), ) }) it('logs the error found', () => { - expect(logger.error).toBeCalledWith(`memo text is too long: memo.length=259 > 255`) + expect(logger.error).toBeCalledWith('Memo text is too long', 259) }) it('throws error when creationDate not-valid', async () => { @@ -437,13 +437,13 @@ describe('ContributionResolver', () => { }), ).resolves.toEqual( expect.objectContaining({ - errors: [new GraphQLError('No contribution found to given id.')], + errors: [new GraphQLError('Contribution not found')], }), ) }) it('logs the error found', () => { - expect(logger.error).toBeCalledWith('No contribution found to given id') + expect(logger.error).toBeCalledWith('Contribution not found', -1) }) }) @@ -463,13 +463,13 @@ describe('ContributionResolver', () => { }), ).resolves.toEqual( expect.objectContaining({ - errors: [new GraphQLError('memo text is too short (5 characters minimum)')], + errors: [new GraphQLError('Memo text is too short')], }), ) }) it('logs the error found', () => { - expect(logger.error).toBeCalledWith('memo text is too short: memo.length=4 < 5') + expect(logger.error).toBeCalledWith('Memo text is too short', 4) }) }) @@ -489,13 +489,13 @@ describe('ContributionResolver', () => { }), ).resolves.toEqual( expect.objectContaining({ - errors: [new GraphQLError('memo text is too long (255 characters maximum)')], + errors: [new GraphQLError('Memo text is too long')], }), ) }) it('logs the error found', () => { - expect(logger.error).toBeCalledWith('memo text is too long: memo.length=259 > 255') + expect(logger.error).toBeCalledWith('Memo text is too long', 259) }) }) @@ -521,18 +521,16 @@ describe('ContributionResolver', () => { }), ).resolves.toEqual( expect.objectContaining({ - errors: [ - new GraphQLError( - 'user of the pending contribution and send user does not correspond', - ), - ], + errors: [new GraphQLError('Can not update contribution of another user')], }), ) }) it('logs the error found', () => { expect(logger.error).toBeCalledWith( - 'user of the pending contribution and send user does not correspond', + 'Can not update contribution of another user', + expect.any(Object), + expect.any(Number), ) }) }) @@ -553,12 +551,16 @@ describe('ContributionResolver', () => { }), ).resolves.toEqual( expect.objectContaining({ - errors: [new GraphQLError('An admin is not allowed to update a user contribution.')], + errors: [new GraphQLError('An admin is not allowed to update an user contribution')], }), ) }) - // TODO check that the error is logged (need to modify AdminResolver, avoid conflicts) + it('logs the error found', () => { + expect(logger.error).toBeCalledWith( + 'An admin is not allowed to update an user contribution', + ) + }) }) describe('update too much so that the limit is exceeded', () => { @@ -615,16 +617,13 @@ describe('ContributionResolver', () => { }), ).resolves.toEqual( expect.objectContaining({ - errors: [new GraphQLError('Currently the month of the contribution cannot change.')], + errors: [new GraphQLError('Month of contribution can not be changed')], }), ) }) - it.skip('logs the error found', () => { - expect(logger.error).toBeCalledWith( - 'No information for available creations with the given creationDate=', - 'Invalid Date', - ) + it('logs the error found', () => { + expect(logger.error).toBeCalledWith('Month of contribution can not be changed') }) }) @@ -1158,6 +1157,7 @@ describe('ContributionResolver', () => { describe('wrong contribution id', () => { it('returns an error', async () => { + jest.clearAllMocks() await expect( mutate({ mutation: deleteContribution, @@ -1167,18 +1167,19 @@ describe('ContributionResolver', () => { }), ).resolves.toEqual( expect.objectContaining({ - errors: [new GraphQLError('Contribution not found for given id.')], + errors: [new GraphQLError('Contribution not found')], }), ) }) it('logs the error found', () => { - expect(logger.error).toBeCalledWith('Contribution not found for given id') + expect(logger.error).toBeCalledWith('Contribution not found', -1) }) }) describe('other user sends a deleteContribution', () => { it('returns an error', async () => { + jest.clearAllMocks() await mutate({ mutation: login, variables: { email: 'peter@lustig.de', password: 'Aa12345_' }, @@ -1198,7 +1199,11 @@ describe('ContributionResolver', () => { }) it('logs the error found', () => { - expect(logger.error).toBeCalledWith('Can not delete contribution of another user') + expect(logger.error).toBeCalledWith( + 'Can not delete contribution of another user', + expect.any(Object), + expect.any(Number), + ) }) }) @@ -1274,7 +1279,10 @@ describe('ContributionResolver', () => { }) it('logs the error found', () => { - expect(logger.error).toBeCalledWith('A confirmed contribution can not be deleted') + expect(logger.error).toBeCalledWith( + 'A confirmed contribution can not be deleted', + expect.objectContaining({ contributionStatus: 'CONFIRMED' }), + ) }) }) }) @@ -1540,15 +1548,13 @@ describe('ContributionResolver', () => { mutate({ mutation: adminCreateContribution, variables }), ).resolves.toEqual( expect.objectContaining({ - errors: [new GraphQLError('Could not find user with email: bibi@bloxberg.de')], + errors: [new GraphQLError('Could not find user')], }), ) }) it('logs the error thrown', () => { - expect(logger.error).toBeCalledWith( - 'Could not find user with email: bibi@bloxberg.de', - ) + expect(logger.error).toBeCalledWith('Could not find user', 'bibi@bloxberg.de') }) }) @@ -1568,7 +1574,7 @@ describe('ContributionResolver', () => { ).resolves.toEqual( expect.objectContaining({ errors: [ - new GraphQLError('This user was deleted. Cannot create a contribution.'), + new GraphQLError('Cannot create contribution since the user was deleted'), ], }), ) @@ -1576,7 +1582,8 @@ describe('ContributionResolver', () => { it('logs the error thrown', () => { expect(logger.error).toBeCalledWith( - 'This user was deleted. Cannot create a contribution.', + 'Cannot create contribution since the user was deleted', + expect.objectContaining({ deletedAt: new Date('2018-03-14T09:17:52.000Z') }), ) }) }) @@ -1597,7 +1604,9 @@ describe('ContributionResolver', () => { ).resolves.toEqual( expect.objectContaining({ errors: [ - new GraphQLError('Contribution could not be saved, Email is not activated'), + new GraphQLError( + 'Cannot create contribution since the users email is not activated', + ), ], }), ) @@ -1605,7 +1614,8 @@ describe('ContributionResolver', () => { it('logs the error thrown', () => { expect(logger.error).toBeCalledWith( - 'Contribution could not be saved, Email is not activated', + 'Cannot create contribution since the users email is not activated', + expect.objectContaining({ emailChecked: false }), ) }) }) @@ -1624,13 +1634,13 @@ describe('ContributionResolver', () => { mutate({ mutation: adminCreateContribution, variables }), ).resolves.toEqual( expect.objectContaining({ - errors: [new GraphQLError(`invalid Date for creationDate=invalid-date`)], + errors: [new GraphQLError('CreationDate is invalid')], }), ) }) it('logs the error thrown', () => { - expect(logger.error).toBeCalledWith(`invalid Date for creationDate=invalid-date`) + expect(logger.error).toBeCalledWith('CreationDate is invalid', 'invalid-date') }) }) @@ -1826,17 +1836,13 @@ describe('ContributionResolver', () => { }), ).resolves.toEqual( expect.objectContaining({ - errors: [ - new GraphQLError('Could not find UserContact with email: bob@baumeister.de'), - ], + errors: [new GraphQLError('Could not find UserContact')], }), ) }) it('logs the error thrown', () => { - expect(logger.error).toBeCalledWith( - 'Could not find UserContact with email: bob@baumeister.de', - ) + expect(logger.error).toBeCalledWith('Could not find UserContact', 'bob@baumeister.de') }) }) @@ -1856,13 +1862,13 @@ describe('ContributionResolver', () => { }), ).resolves.toEqual( expect.objectContaining({ - errors: [new GraphQLError('User was deleted (stephen@hawking.uk)')], + errors: [new GraphQLError('User was deleted')], }), ) }) it('logs the error thrown', () => { - expect(logger.error).toBeCalledWith('User was deleted (stephen@hawking.uk)') + expect(logger.error).toBeCalledWith('User was deleted', 'stephen@hawking.uk') }) }) @@ -1882,13 +1888,13 @@ describe('ContributionResolver', () => { }), ).resolves.toEqual( expect.objectContaining({ - errors: [new GraphQLError('No contribution found to given id.')], + errors: [new GraphQLError('Contribution not found')], }), ) }) it('logs the error thrown', () => { - expect(logger.error).toBeCalledWith('No contribution found to given id.') + expect(logger.error).toBeCalledWith('Contribution not found', -1) }) }) @@ -1912,7 +1918,7 @@ describe('ContributionResolver', () => { expect.objectContaining({ errors: [ new GraphQLError( - 'user of the pending contribution and send user does not correspond', + 'User of the pending contribution and send user does not correspond', ), ], }), @@ -1921,7 +1927,7 @@ describe('ContributionResolver', () => { it('logs the error thrown', () => { expect(logger.error).toBeCalledWith( - 'user of the pending contribution and send user does not correspond', + 'User of the pending contribution and send user does not correspond', ) }) }) @@ -2116,13 +2122,13 @@ describe('ContributionResolver', () => { }), ).resolves.toEqual( expect.objectContaining({ - errors: [new GraphQLError('Contribution not found for given id.')], + errors: [new GraphQLError('Contribution not found')], }), ) }) it('logs the error thrown', () => { - expect(logger.error).toBeCalledWith('Contribution not found for given id: -1') + expect(logger.error).toBeCalledWith('Contribution not found', -1) }) }) @@ -2242,13 +2248,13 @@ describe('ContributionResolver', () => { }), ).resolves.toEqual( expect.objectContaining({ - errors: [new GraphQLError('Contribution not found to given id.')], + errors: [new GraphQLError('Contribution not found')], }), ) }) it('logs the error thrown', () => { - expect(logger.error).toBeCalledWith('Contribution not found for given id: -1') + expect(logger.error).toBeCalledWith('Contribution not found', -1) }) }) @@ -2359,6 +2365,7 @@ describe('ContributionResolver', () => { describe('confirm same contribution again', () => { it('throws an error', async () => { + jest.clearAllMocks() await expect( mutate({ mutation: confirmContribution, @@ -2368,11 +2375,18 @@ describe('ContributionResolver', () => { }), ).resolves.toEqual( expect.objectContaining({ - errors: [new GraphQLError('Contribution already confirmd.')], + errors: [new GraphQLError('Contribution already confirmed')], }), ) }) }) + + it('logs the error thrown', () => { + expect(logger.error).toBeCalledWith( + 'Contribution already confirmed', + expect.any(Number), + ) + }) }) describe('confirm two creations one after the other quickly', () => { diff --git a/backend/src/graphql/resolver/ContributionResolver.ts b/backend/src/graphql/resolver/ContributionResolver.ts index c7946d2c8..8765d29a8 100644 --- a/backend/src/graphql/resolver/ContributionResolver.ts +++ b/backend/src/graphql/resolver/ContributionResolver.ts @@ -53,6 +53,7 @@ import { sendContributionDeniedEmail, } from '@/emails/sendEmailVariants' import { TRANSACTIONS_LOCK } from '@/util/TRANSACTIONS_LOCK' +import LogError from '@/server/LogError' @Resolver() export class ContributionResolver { @@ -63,14 +64,11 @@ export class ContributionResolver { @Ctx() context: Context, ): Promise { const clientTimezoneOffset = getClientTimezoneOffset(context) - if (memo.length > MEMO_MAX_CHARS) { - logger.error(`memo text is too long: memo.length=${memo.length} > ${MEMO_MAX_CHARS}`) - throw new Error(`memo text is too long (${MEMO_MAX_CHARS} characters maximum)`) - } - if (memo.length < MEMO_MIN_CHARS) { - logger.error(`memo text is too short: memo.length=${memo.length} < ${MEMO_MIN_CHARS}`) - throw new Error(`memo text is too short (${MEMO_MIN_CHARS} characters minimum)`) + throw new LogError('Memo text is too short', memo.length) + } + if (memo.length > MEMO_MAX_CHARS) { + throw new LogError('Memo text is too long', memo.length) } const event = new Event() @@ -112,16 +110,13 @@ export class ContributionResolver { const user = getUser(context) const contribution = await DbContribution.findOne(id) if (!contribution) { - logger.error('Contribution not found for given id') - throw new Error('Contribution not found for given id.') + throw new LogError('Contribution not found', id) } if (contribution.userId !== user.id) { - logger.error('Can not delete contribution of another user') - throw new Error('Can not delete contribution of another user') + throw new LogError('Can not delete contribution of another user', contribution, user.id) } if (contribution.confirmedAt) { - logger.error('A confirmed contribution can not be deleted') - throw new Error('A confirmed contribution can not be deleted') + throw new LogError('A confirmed contribution can not be deleted', contribution) } contribution.contributionStatus = ContributionStatus.DELETED @@ -215,14 +210,11 @@ export class ContributionResolver { @Ctx() context: Context, ): Promise { const clientTimezoneOffset = getClientTimezoneOffset(context) - if (memo.length > MEMO_MAX_CHARS) { - logger.error(`memo text is too long: memo.length=${memo.length} > ${MEMO_MAX_CHARS}`) - throw new Error(`memo text is too long (${MEMO_MAX_CHARS} characters maximum)`) - } - if (memo.length < MEMO_MIN_CHARS) { - logger.error(`memo text is too short: memo.length=${memo.length} < ${MEMO_MIN_CHARS}`) - throw new Error(`memo text is too short (${MEMO_MIN_CHARS} characters minimum)`) + throw new LogError('Memo text is too short', memo.length) + } + if (memo.length > MEMO_MAX_CHARS) { + throw new LogError('Memo text is too long', memo.length) } const user = getUser(context) @@ -231,22 +223,22 @@ export class ContributionResolver { where: { id: contributionId, confirmedAt: IsNull(), deniedAt: IsNull() }, }) if (!contributionToUpdate) { - logger.error('No contribution found to given id') - throw new Error('No contribution found to given id.') + throw new LogError('Contribution not found', contributionId) } if (contributionToUpdate.userId !== user.id) { - logger.error('user of the pending contribution and send user does not correspond') - throw new Error('user of the pending contribution and send user does not correspond') + throw new LogError( + 'Can not update contribution of another user', + contributionToUpdate, + user.id, + ) } if ( contributionToUpdate.contributionStatus !== ContributionStatus.IN_PROGRESS && contributionToUpdate.contributionStatus !== ContributionStatus.PENDING ) { - logger.error( - `Contribution can not be updated since the state is ${contributionToUpdate.contributionStatus}`, - ) - throw new Error( - `Contribution can not be updated since the state is ${contributionToUpdate.contributionStatus}`, + throw new LogError( + 'Contribution can not be updated due to status', + contributionToUpdate.contributionStatus, ) } const creationDateObj = new Date(creationDate) @@ -254,8 +246,7 @@ export class ContributionResolver { if (contributionToUpdate.contributionDate.getMonth() === creationDateObj.getMonth()) { creations = updateCreations(creations, contributionToUpdate, clientTimezoneOffset) } else { - logger.error('Currently the month of the contribution cannot change.') - throw new Error('Currently the month of the contribution cannot change.') + throw new LogError('Month of contribution can not be changed') } // all possible cases not to be true are thrown in this function @@ -306,8 +297,7 @@ export class ContributionResolver { ) const clientTimezoneOffset = getClientTimezoneOffset(context) if (!isValidDateString(creationDate)) { - logger.error(`invalid Date for creationDate=${creationDate}`) - throw new Error(`invalid Date for creationDate=${creationDate}`) + throw new LogError('CreationDate is invalid', creationDate) } const emailContact = await UserContact.findOne({ where: { email }, @@ -315,20 +305,22 @@ export class ContributionResolver { relations: ['user'], }) if (!emailContact) { - logger.error(`Could not find user with email: ${email}`) - throw new Error(`Could not find user with email: ${email}`) + throw new LogError('Could not find user', email) } if (emailContact.deletedAt) { - logger.error('This emailContact was deleted. Cannot create a contribution.') - throw new Error('This emailContact was deleted. Cannot create a contribution.') + throw new LogError( + 'Cannot create contribution since the emailContact was deleted', + emailContact, + ) } if (emailContact.user.deletedAt) { - logger.error('This user was deleted. Cannot create a contribution.') - throw new Error('This user was deleted. Cannot create a contribution.') + throw new LogError('Cannot create contribution since the user was deleted', emailContact.user) } if (!emailContact.emailChecked) { - logger.error('Contribution could not be saved, Email is not activated') - throw new Error('Contribution could not be saved, Email is not activated') + throw new LogError( + 'Cannot create contribution since the users email is not activated', + emailContact, + ) } const event = new Event() @@ -404,17 +396,14 @@ export class ContributionResolver { relations: ['user'], }) if (!emailContact) { - logger.error(`Could not find UserContact with email: ${email}`) - throw new Error(`Could not find UserContact with email: ${email}`) + throw new LogError('Could not find UserContact', email) } const user = emailContact.user if (!user) { - logger.error(`Could not find User to emailContact: ${email}`) - throw new Error(`Could not find User to emailContact: ${email}`) + throw new LogError('Could not find User', email) } if (user.deletedAt) { - logger.error(`User was deleted (${email})`) - throw new Error(`User was deleted (${email})`) + throw new LogError('User was deleted', email) } const moderator = getUser(context) @@ -423,18 +412,15 @@ export class ContributionResolver { where: { id, confirmedAt: IsNull(), deniedAt: IsNull() }, }) if (!contributionToUpdate) { - logger.error('No contribution found to given id.') - throw new Error('No contribution found to given id.') + throw new LogError('Contribution not found', id) } if (contributionToUpdate.userId !== user.id) { - logger.error('user of the pending contribution and send user does not correspond') - throw new Error('user of the pending contribution and send user does not correspond') + throw new LogError('User of the pending contribution and send user does not correspond') } if (contributionToUpdate.moderatorId === null) { - logger.error('An admin is not allowed to update a user contribution.') - throw new Error('An admin is not allowed to update a user contribution.') + throw new LogError('An admin is not allowed to update an user contribution') } const creationDateObj = new Date(creationDate) @@ -443,8 +429,7 @@ export class ContributionResolver { if (contributionToUpdate.contributionDate.getMonth() === creationDateObj.getMonth()) { creations = updateCreations(creations, contributionToUpdate, clientTimezoneOffset) } else { - logger.error('Currently the month of the contribution cannot change.') - throw new Error('Currently the month of the contribution cannot change.') + throw new LogError('Month of contribution can not be changed') } // all possible cases not to be true are thrown in this function @@ -521,19 +506,17 @@ export class ContributionResolver { ): Promise { const contribution = await DbContribution.findOne(id) if (!contribution) { - logger.error(`Contribution not found for given id: ${id}`) - throw new Error('Contribution not found for given id.') + throw new LogError('Contribution not found', id) } if (contribution.confirmedAt) { - logger.error('A confirmed contribution can not be deleted') - throw new Error('A confirmed contribution can not be deleted') + throw new LogError('A confirmed contribution can not be deleted') } const moderator = getUser(context) if ( contribution.contributionType === ContributionType.USER && contribution.userId === moderator.id ) { - throw new Error('Own contribution can not be deleted as admin') + throw new LogError('Own contribution can not be deleted as admin') } const user = await DbUser.findOneOrFail( { id: contribution.userId }, @@ -577,29 +560,24 @@ export class ContributionResolver { const clientTimezoneOffset = getClientTimezoneOffset(context) const contribution = await DbContribution.findOne(id) if (!contribution) { - logger.error(`Contribution not found for given id: ${id}`) - throw new Error('Contribution not found to given id.') + throw new LogError('Contribution not found', id) } if (contribution.confirmedAt) { - logger.error(`Contribution already confirmd: ${id}`) - throw new Error('Contribution already confirmd.') + throw new LogError('Contribution already confirmed', id) } if (contribution.contributionStatus === 'DENIED') { - logger.error(`Contribution already denied: ${id}`) - throw new Error('Contribution already denied.') + throw new LogError('Contribution already denied', id) } const moderatorUser = getUser(context) if (moderatorUser.id === contribution.userId) { - logger.error('Moderator can not confirm own contribution') - throw new Error('Moderator can not confirm own contribution') + throw new LogError('Moderator can not confirm own contribution') } const user = await DbUser.findOneOrFail( { id: contribution.userId }, { withDeleted: true, relations: ['emailContact'] }, ) if (user.deletedAt) { - logger.error('This user was deleted. Cannot confirm a contribution.') - throw new Error('This user was deleted. Cannot confirm a contribution.') + throw new LogError('Can not confirm contribution since the user was deleted') } const creations = await getUserCreation(contribution.userId, clientTimezoneOffset, false) validateContribution( @@ -668,8 +646,7 @@ export class ContributionResolver { }) } catch (e) { await queryRunner.rollbackTransaction() - logger.error('Creation was not successful', e) - throw new Error('Creation was not successful.') + throw new LogError('Creation was not successful', e) } finally { await queryRunner.release() } @@ -744,17 +721,16 @@ export class ContributionResolver { deniedBy: IsNull(), }) if (!contributionToUpdate) { - logger.error(`Contribution not found for given id: ${id}`) - throw new Error(`Contribution not found for given id.`) + throw new LogError('Contribution not found', id) } if ( contributionToUpdate.contributionStatus !== ContributionStatus.IN_PROGRESS && contributionToUpdate.contributionStatus !== ContributionStatus.PENDING ) { - logger.error( - `Contribution state (${contributionToUpdate.contributionStatus}) is not allowed.`, + throw new LogError( + 'Status of the contribution is not allowed', + contributionToUpdate.contributionStatus, ) - throw new Error(`State of the contribution is not allowed.`) } const moderator = getUser(context) const user = await DbUser.findOne( @@ -762,10 +738,7 @@ export class ContributionResolver { { relations: ['emailContact'] }, ) if (!user) { - logger.error( - `Could not find User for the Contribution (userId: ${contributionToUpdate.userId}).`, - ) - throw new Error('Could not find User for the Contribution.') + throw new LogError('Could not find User of the Contribution', contributionToUpdate.userId) } contributionToUpdate.contributionStatus = ContributionStatus.DENIED From 3724295b6f2a959ce7615ae56191f6b9643fad7a Mon Sep 17 00:00:00 2001 From: Ulf Gebhardt Date: Mon, 6 Feb 2023 20:37:51 +0100 Subject: [PATCH 2/7] test another error case --- .../resolver/ContributionResolver.test.ts | 100 +++++++++++++----- 1 file changed, 75 insertions(+), 25 deletions(-) diff --git a/backend/src/graphql/resolver/ContributionResolver.test.ts b/backend/src/graphql/resolver/ContributionResolver.test.ts index 0078ecafd..dcbf0d52e 100644 --- a/backend/src/graphql/resolver/ContributionResolver.test.ts +++ b/backend/src/graphql/resolver/ContributionResolver.test.ts @@ -46,6 +46,7 @@ import { User } from '@entity/User' import { EventProtocolType } from '@/event/EventProtocolType' import { logger, i18n as localization } from '@test/testSetup' import { UserInputError } from 'apollo-server-express' +import { ContributionStatus } from '../enum/ContributionStatus' // mock account activation email to avoid console spam // mock account activation email to avoid console spam @@ -422,31 +423,6 @@ describe('ContributionResolver', () => { resetToken() }) - describe('wrong contribution id', () => { - it('throws an error', async () => { - jest.clearAllMocks() - await expect( - mutate({ - mutation: updateContribution, - variables: { - contributionId: -1, - amount: 100.0, - memo: 'Test env contribution', - creationDate: new Date().toString(), - }, - }), - ).resolves.toEqual( - expect.objectContaining({ - errors: [new GraphQLError('Contribution not found')], - }), - ) - }) - - it('logs the error found', () => { - expect(logger.error).toBeCalledWith('Contribution not found', -1) - }) - }) - describe('Memo length smaller than 5 chars', () => { it('throws error', async () => { jest.clearAllMocks() @@ -499,6 +475,31 @@ describe('ContributionResolver', () => { }) }) + describe('wrong contribution id', () => { + it('throws an error', async () => { + jest.clearAllMocks() + await expect( + mutate({ + mutation: updateContribution, + variables: { + contributionId: -1, + amount: 100.0, + memo: 'Test env contribution', + creationDate: new Date().toString(), + }, + }), + ).resolves.toEqual( + expect.objectContaining({ + errors: [new GraphQLError('Contribution not found')], + }), + ) + }) + + it('logs the error found', () => { + expect(logger.error).toBeCalledWith('Contribution not found', -1) + }) + }) + describe('wrong user tries to update the contribution', () => { beforeAll(async () => { await mutate({ @@ -535,6 +536,7 @@ describe('ContributionResolver', () => { }) }) + // TODO: why is this here - this is a different call `adminUpdateContribution` not `updateContribution` describe('admin tries to update a user contribution', () => { it('throws an error', async () => { jest.clearAllMocks() @@ -563,6 +565,54 @@ describe('ContributionResolver', () => { }) }) + describe('contribution has wrong status', () => { + beforeAll(async () => { + const contribution = await Contribution.findOneOrFail({ + id: result.data.createContribution.id, + }) + contribution.contributionStatus = ContributionStatus.DELETED + contribution.save() + await mutate({ + mutation: login, + variables: { email: 'bibi@bloxberg.de', password: 'Aa12345_' }, + }) + }) + + afterAll(async () => { + const contribution = await Contribution.findOneOrFail({ + id: result.data.createContribution.id, + }) + contribution.contributionStatus = ContributionStatus.PENDING + contribution.save() + }) + + it('throws an error', async () => { + jest.clearAllMocks() + await expect( + mutate({ + mutation: updateContribution, + variables: { + contributionId: result.data.createContribution.id, + amount: 10.0, + memo: 'Test env contribution', + creationDate: new Date().toString(), + }, + }), + ).resolves.toEqual( + expect.objectContaining({ + errors: [new GraphQLError('Contribution can not be updated due to status')], + }), + ) + }) + + it('logs the error found', () => { + expect(logger.error).toBeCalledWith( + 'Contribution can not be updated due to status', + ContributionStatus.DELETED, + ) + }) + }) + describe('update too much so that the limit is exceeded', () => { beforeAll(async () => { await mutate({ From 225652ecc77e4d221f1de3d3b19ca65589d3d065 Mon Sep 17 00:00:00 2001 From: Ulf Gebhardt Date: Mon, 6 Feb 2023 22:30:23 +0100 Subject: [PATCH 3/7] refactor adminCreateContribution to check for deleted users --- .../src/graphql/resolver/ContributionResolver.test.ts | 6 +++++- backend/src/graphql/resolver/ContributionResolver.ts | 10 ++-------- backend/src/seeds/users/stephen-hawking.ts | 1 + 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/backend/src/graphql/resolver/ContributionResolver.test.ts b/backend/src/graphql/resolver/ContributionResolver.test.ts index dcbf0d52e..c3df9bd1d 100644 --- a/backend/src/graphql/resolver/ContributionResolver.test.ts +++ b/backend/src/graphql/resolver/ContributionResolver.test.ts @@ -1633,7 +1633,11 @@ describe('ContributionResolver', () => { it('logs the error thrown', () => { expect(logger.error).toBeCalledWith( 'Cannot create contribution since the user was deleted', - expect.objectContaining({ deletedAt: new Date('2018-03-14T09:17:52.000Z') }), + expect.objectContaining({ + user: expect.objectContaining({ + deletedAt: new Date('2018-03-14T09:17:52.000Z'), + }), + }), ) }) }) diff --git a/backend/src/graphql/resolver/ContributionResolver.ts b/backend/src/graphql/resolver/ContributionResolver.ts index 8765d29a8..340d99524 100644 --- a/backend/src/graphql/resolver/ContributionResolver.ts +++ b/backend/src/graphql/resolver/ContributionResolver.ts @@ -307,14 +307,8 @@ export class ContributionResolver { if (!emailContact) { throw new LogError('Could not find user', email) } - if (emailContact.deletedAt) { - throw new LogError( - 'Cannot create contribution since the emailContact was deleted', - emailContact, - ) - } - if (emailContact.user.deletedAt) { - throw new LogError('Cannot create contribution since the user was deleted', emailContact.user) + if (emailContact.deletedAt || emailContact.user.deletedAt) { + throw new LogError('Cannot create contribution since the user was deleted', emailContact) } if (!emailContact.emailChecked) { throw new LogError( diff --git a/backend/src/seeds/users/stephen-hawking.ts b/backend/src/seeds/users/stephen-hawking.ts index a683b7579..6c4f34d10 100644 --- a/backend/src/seeds/users/stephen-hawking.ts +++ b/backend/src/seeds/users/stephen-hawking.ts @@ -1,5 +1,6 @@ import { UserInterface } from './UserInterface' +// TODO: the generated email_contact is not deleted export const stephenHawking: UserInterface = { email: 'stephen@hawking.uk', firstName: 'Stephen', From bf24d15d96628615bcb6d3bb230c0cd722fc92e2 Mon Sep 17 00:00:00 2001 From: Ulf Gebhardt Date: Mon, 6 Feb 2023 22:37:38 +0100 Subject: [PATCH 4/7] unify checks on adminCreateContribution and adminUpdateContribution --- .../resolver/ContributionResolver.test.ts | 4 ++-- .../graphql/resolver/ContributionResolver.ts | 18 +++++++----------- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/backend/src/graphql/resolver/ContributionResolver.test.ts b/backend/src/graphql/resolver/ContributionResolver.test.ts index c3df9bd1d..1e0930d91 100644 --- a/backend/src/graphql/resolver/ContributionResolver.test.ts +++ b/backend/src/graphql/resolver/ContributionResolver.test.ts @@ -1890,13 +1890,13 @@ describe('ContributionResolver', () => { }), ).resolves.toEqual( expect.objectContaining({ - errors: [new GraphQLError('Could not find UserContact')], + errors: [new GraphQLError('Could not find User')], }), ) }) it('logs the error thrown', () => { - expect(logger.error).toBeCalledWith('Could not find UserContact', 'bob@baumeister.de') + expect(logger.error).toBeCalledWith('Could not find User', 'bob@baumeister.de') }) }) diff --git a/backend/src/graphql/resolver/ContributionResolver.ts b/backend/src/graphql/resolver/ContributionResolver.ts index 340d99524..a845af022 100644 --- a/backend/src/graphql/resolver/ContributionResolver.ts +++ b/backend/src/graphql/resolver/ContributionResolver.ts @@ -304,7 +304,7 @@ export class ContributionResolver { withDeleted: true, relations: ['user'], }) - if (!emailContact) { + if (!emailContact || !emailContact.user) { throw new LogError('Could not find user', email) } if (emailContact.deletedAt || emailContact.user.deletedAt) { @@ -389,14 +389,10 @@ export class ContributionResolver { withDeleted: true, relations: ['user'], }) - if (!emailContact) { - throw new LogError('Could not find UserContact', email) - } - const user = emailContact.user - if (!user) { + if (!emailContact || !emailContact.user) { throw new LogError('Could not find User', email) } - if (user.deletedAt) { + if (emailContact.deletedAt || emailContact.user.deletedAt) { throw new LogError('User was deleted', email) } @@ -409,7 +405,7 @@ export class ContributionResolver { throw new LogError('Contribution not found', id) } - if (contributionToUpdate.userId !== user.id) { + if (contributionToUpdate.userId !== emailContact.user.id) { throw new LogError('User of the pending contribution and send user does not correspond') } @@ -418,7 +414,7 @@ export class ContributionResolver { } const creationDateObj = new Date(creationDate) - let creations = await getUserCreation(user.id, clientTimezoneOffset) + let creations = await getUserCreation(emailContact.user.id, clientTimezoneOffset) if (contributionToUpdate.contributionDate.getMonth() === creationDateObj.getMonth()) { creations = updateCreations(creations, contributionToUpdate, clientTimezoneOffset) @@ -441,11 +437,11 @@ export class ContributionResolver { result.memo = contributionToUpdate.memo result.date = contributionToUpdate.contributionDate - result.creation = await getUserCreation(user.id, clientTimezoneOffset) + result.creation = await getUserCreation(emailContact.user.id, clientTimezoneOffset) const event = new Event() const eventAdminContributionUpdate = new EventAdminContributionUpdate() - eventAdminContributionUpdate.userId = user.id + eventAdminContributionUpdate.userId = emailContact.user.id eventAdminContributionUpdate.amount = amount eventAdminContributionUpdate.contributionId = contributionToUpdate.id await eventProtocol.writeEvent( From 741d0c2b9717d1b1994bb251d33d10a0a66a42f6 Mon Sep 17 00:00:00 2001 From: Ulf Gebhardt Date: Mon, 6 Feb 2023 22:38:45 +0100 Subject: [PATCH 5/7] choose todo over coverage --- backend/src/graphql/resolver/ContributionResolver.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/backend/src/graphql/resolver/ContributionResolver.ts b/backend/src/graphql/resolver/ContributionResolver.ts index a845af022..c68c34f99 100644 --- a/backend/src/graphql/resolver/ContributionResolver.ts +++ b/backend/src/graphql/resolver/ContributionResolver.ts @@ -416,6 +416,7 @@ export class ContributionResolver { const creationDateObj = new Date(creationDate) let creations = await getUserCreation(emailContact.user.id, clientTimezoneOffset) + // TODO: remove this restriction if (contributionToUpdate.contributionDate.getMonth() === creationDateObj.getMonth()) { creations = updateCreations(creations, contributionToUpdate, clientTimezoneOffset) } else { From 2773e270b73b1e54c00f65be9ad7ef65976622a3 Mon Sep 17 00:00:00 2001 From: Ulf Gebhardt Date: Thu, 9 Feb 2023 14:12:44 +0100 Subject: [PATCH 6/7] Update backend/src/seeds/users/stephen-hawking.ts --- backend/src/seeds/users/stephen-hawking.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/backend/src/seeds/users/stephen-hawking.ts b/backend/src/seeds/users/stephen-hawking.ts index 6c4f34d10..a683b7579 100644 --- a/backend/src/seeds/users/stephen-hawking.ts +++ b/backend/src/seeds/users/stephen-hawking.ts @@ -1,6 +1,5 @@ import { UserInterface } from './UserInterface' -// TODO: the generated email_contact is not deleted export const stephenHawking: UserInterface = { email: 'stephen@hawking.uk', firstName: 'Stephen', From 0caff31e0729dda4c15ac581ae6f4032fc811378 Mon Sep 17 00:00:00 2001 From: Ulf Gebhardt Date: Thu, 9 Feb 2023 14:16:49 +0100 Subject: [PATCH 7/7] Update backend/src/graphql/resolver/ContributionResolver.test.ts --- backend/src/graphql/resolver/ContributionResolver.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/backend/src/graphql/resolver/ContributionResolver.test.ts b/backend/src/graphql/resolver/ContributionResolver.test.ts index 1e0930d91..c75f71f13 100644 --- a/backend/src/graphql/resolver/ContributionResolver.test.ts +++ b/backend/src/graphql/resolver/ContributionResolver.test.ts @@ -536,7 +536,6 @@ describe('ContributionResolver', () => { }) }) - // TODO: why is this here - this is a different call `adminUpdateContribution` not `updateContribution` describe('admin tries to update a user contribution', () => { it('throws an error', async () => { jest.clearAllMocks()