Verified Commit d1fed156 authored by aguestuser's avatar aguestuser

[215] cleanup: refactor shared db destroy logic in recycle/destroy

* the `common.destroy` function is odd
  * accepts a nullable channel (instead of just not being called)
  * has a funny return signature
  * the fact that it is a method on a channel rather than a function
    call that accepts a channel (or phone number) forces some tightly
    coupled testing patterns (that could be cleaner with pure functions
    on the db) without saving any db queries by doing so
* replace it with `channelRepository.destroy` and also create
  `phoneNumberRepository.destroy`: leaves cleaner test seams and less
  convoluted branching logcis
* clean up tests for destroy that (1) break b/c of this, (2) were
  tightly coupled and a bit hard to follow/change anyway
parent fdf51725
......@@ -32,15 +32,13 @@ const update = (phoneNumber, attrs) =>
.update({ ...attrs }, { where: { phoneNumber }, returning: true })
.then(([, [pNumInstance]]) => pNumInstance)
// (ChannelInstance | null, Transaction) -> Promise<boolean>
const destroy = async (channel, tx) => {
if (channel == null) return false
try {
await channel.destroy({ transaction: tx })
return true
} catch (error) {
return Promise.reject('Failed to destroy channel')
}
// (string, Transaction | null) => Promise<boolean>
const destroy = async (phoneNumber, transaction) => {
const numDestroyed = await app.db.channel.destroy({
where: { phoneNumber },
...(transaction ? { transaction } : {}),
})
return numDestroyed > 0
}
const findAll = () => app.db.channel.findAll()
......
......@@ -10,6 +10,15 @@ const filters = {
const create = ({ phoneNumber, twilioSid, status }) =>
app.db.phoneNumber.create({ phoneNumber, twilioSid, status })
// (string, Transaction | null) => Promise<boolean>
const destroy = async (phoneNumber, transaction) => {
const numDestroyed = await app.db.phoneNumber.destroy({
where: { phoneNumber },
...(transaction ? { transaction } : {}),
})
return numDestroyed > 0
}
const find = phoneNumber => app.db.phoneNumber.findOne({ where: { phoneNumber } })
const findAll = () => app.db.phoneNumber.findAll()
......@@ -35,4 +44,4 @@ const update = (phoneNumber, attrs) =>
.update({ ...attrs }, { where: { phoneNumber }, returning: true })
.then(([, [pNumInstance]]) => pNumInstance)
module.exports = { filters, create, find, findAll, findAllPurchased, list, update }
module.exports = { filters, create, destroy, find, findAll, findAllPurchased, list, update }
......@@ -4,15 +4,6 @@ const inviteRepository = require('./db/repositories/invite')
const smsSenderRepository = require('./db/repositories/smsSender')
const hotlineMessageRepository = require('./db/repositories/hotlineMessage')
const diagnostics = require('./diagnostics')
const util = require('./util')
const recycleRequestRepository = require('./db/repositories/recycleRequest')
const channelRepository = require('./db/repositories/channel')
const notifier = require('./notifier')
const { notificationKeys } = require('./notifier')
const { map } = require('lodash')
const {
job: { recycleInterval },
} = require('./config')
const run = async () => {
logger.log('--- Running startup jobs...')
......@@ -36,11 +27,7 @@ const run = async () => {
logger.log('----- Launching invite scrubbing job...')
inviteRepository.launchInviteDeletionJob()
logger.log('----- Launched invite scrubbing job.')
logger.log('---- Launching recycle request processing job...')
util.repeatEvery(recycleInterval, processRecycleRequests)
logger.log('---- Launched healthcheck job...')
logger.log('---- Launching healthcheck job...')
diagnostics.launchHealthcheckJob()
logger.log('---- Launched healthcheck job...')
......
const app = require('../../index')
const phoneNumberRepository = require('../../db/repositories/phoneNumber')
const { defaultLanguage } = require('../../config')
const { notifyMembersExcept, destroyChannel } = require('./common')
const common = require('./common')
const notifier = require('../../notifier')
const signal = require('../../signal')
const { messagesIn } = require('../../dispatcher/strings/messages')
const channelRepository = require('../../db/repositories/channel')
......@@ -14,33 +14,33 @@ const {
signal: { keystorePath },
} = require('../../config')
// ({Database, Socket, string}) -> SignalboostStatus
// ({phoneNumber: string, sender?: string }) -> SignalboostStatus
const destroy = async ({ phoneNumber, sender }) => {
let tx = await app.db.sequelize.transaction()
try {
const channelInstance = await channelRepository.findDeep(phoneNumber)
const phoneNumberInstance = await phoneNumberRepository.find(phoneNumber)
const channel = await channelRepository.findDeep(phoneNumber)
const phoneNumberRecord = await phoneNumberRepository.find(phoneNumber)
if (!channelInstance && !phoneNumberInstance)
if (!channel && !phoneNumberRecord)
return { status: 'ERROR', message: `No records found for ${phoneNumber}` }
if (phoneNumberInstance) {
await destroyPhoneNumber(phoneNumberInstance, tx)
await releasePhoneNumber(phoneNumberInstance)
if (phoneNumberRecord) {
await phoneNumberRepository.destroy(phoneNumber, tx)
await releasePhoneNumber(phoneNumber)
}
if (channelInstance) {
await destroyChannel(channelInstance, tx)
if (channel) {
await channelRepository.destroy(channel.phoneNumber, tx)
await eventRepository.log(eventTypes.CHANNEL_DESTROYED, phoneNumber, tx)
await notifyMembersExcept(
channelInstance,
await notifier.notifyMembersExcept(
channel,
messagesIn(defaultLanguage).notifications.channelDestroyed,
sender,
)
}
await signal.unsubscribe(phoneNumber)
await destroySignalEntry(phoneNumber)
await deleteSignalKeystore(phoneNumber)
await tx.commit()
return { status: 'SUCCESS', msg: 'All records of phone number have been destroyed.' }
......@@ -53,7 +53,7 @@ const destroy = async ({ phoneNumber, sender }) => {
// HELPERS
// (DB, string) -> Promise<void>
const destroySignalEntry = async phoneNumber => {
const deleteSignalKeystore = async phoneNumber => {
try {
await fs.remove(`${keystorePath}/${phoneNumber}`)
await fs.remove(`${keystorePath}/${phoneNumber}.d`)
......@@ -63,10 +63,10 @@ const destroySignalEntry = async phoneNumber => {
}
// (PhoneNumberInstance) -> Promise<void>
const releasePhoneNumber = async phoneNumberInstance => {
const releasePhoneNumber = async phoneNumber => {
try {
const twilioClient = common.getTwilioClient()
await twilioClient.incomingPhoneNumbers(phoneNumberInstance.twilioSid).remove()
await twilioClient.incomingPhoneNumbers(phoneNumber.twilioSid).remove()
} catch (error) {
return error.status === 404
? Promise.resolve()
......@@ -74,24 +74,13 @@ const releasePhoneNumber = async phoneNumberInstance => {
}
}
// Channel -> Promise<void>
const destroyPhoneNumber = async (phoneNumberInstance, tx) => {
if (phoneNumberInstance === null) return
try {
await phoneNumberInstance.destroy({ transaction: tx })
} catch (error) {
await Promise.reject('Failed to destroy phoneNumber in db')
}
}
// (String, DB, Socket, String) -> SignalboostStatus
const handleDestroyFailure = async (err, phoneNumber) => {
logger.log(`Error destroying channel: ${phoneNumber}:`)
logger.error(err)
await common.notifyMaintainers(
await notifier.notifyMaintainers(
messagesIn(defaultLanguage).notifications.channelDestructionFailed(phoneNumber),
)
return {
status: 'ERROR',
message: `Failed to destroy channel for ${phoneNumber}. Error: ${err}`,
......
......@@ -75,7 +75,7 @@ const recycle = async phoneNumber => {
try {
await notifier.notifyMembers(channel, notificationKeys.CHANNEL_RECYCLED)
await channelRepository.destroy(channel)
await channelRepository.destroy(channel.phoneNumber)
await eventRepository.log(eventTypes.CHANNEL_DESTROYED, phoneNumber)
await phoneNumberRepository.update(phoneNumber, { status: common.statuses.VERIFIED })
return {
......
import chai, { expect } from 'chai'
import { expect } from 'chai'
import { describe, it, before, beforeEach, after, afterEach } from 'mocha'
import chaiAsPromised from 'chai-as-promised'
import { channelFactory, deepChannelFactory } from '../../../support/factories/channel'
import { genPhoneNumber } from '../../../support/factories/phoneNumber'
import { omit, keys, times, map } from 'lodash'
......@@ -13,8 +12,6 @@ const {
} = require('../../../../app/config')
describe('channel repository', () => {
chai.use(chaiAsPromised)
const channelPhoneNumber = genPhoneNumber()
const adminPhoneNumbers = [genPhoneNumber(), genPhoneNumber()]
let db, channel
......@@ -125,28 +122,20 @@ describe('channel repository', () => {
describe('#destroy', () => {
let channel, channelCount
describe('when given a channel instance', () => {
describe('when given the phone number for an existing channel', () => {
beforeEach(async () => (channel = await db.channel.create(channelFactory())))
it('deletes the instance', async () => {
channelCount = await db.channel.count()
expect(await channelRepository.destroy(channel)).to.eql(true)
expect(await channelRepository.destroy(channel.phoneNumber)).to.eql(true)
expect(await db.channel.count()).to.eql(channelCount - 1)
})
})
describe('when given null', () => {
describe('when given the phone number for a non-existent channel', () => {
it('does nothing', async () => {
channelCount = await db.channel.count()
expect(await channelRepository.destroy(channel)).to.eql(false)
expect(await db.channel.count()).to.eql(channelCount)
})
})
describe('when given a channel but db error occurs', () => {
it('rejects with an error', async () => {
channelCount = await db.channel.count()
expect(await channelRepository.destroy('foobar').catch(e => e)).to.include('Failed')
expect(await channelRepository.destroy(genPhoneNumber())).to.eql(false)
expect(await db.channel.count()).to.eql(channelCount)
})
})
......@@ -242,7 +231,7 @@ describe('channel repository', () => {
})
it('retrieves all channels with given phone numbers', async () => {
const results = await channelRepository.findManyDeep(includedPhoneNumbers)
expect(map(results, 'phoneNumber')).to.eql(includedPhoneNumbers)
expect(map(results, 'phoneNumber')).to.have.members(includedPhoneNumbers)
expect(results[0].memberships).not.to.be.empty
})
})
......
import { expect } from 'chai'
import { after, afterEach, before, beforeEach, describe, it } from 'mocha'
import commonService from '../../../../app/registrar/phoneNumber/common'
import { destroy } from '../../../../app/registrar/phoneNumber/destroy'
import sinon from 'sinon'
import fs from 'fs-extra'
import { map } from 'lodash'
import phoneNumberRepository from '../../../../app/db/repositories/phoneNumber'
import channelRepository from '../../../../app/db/repositories/channel'
import eventRepository from '../../../../app/db/repositories/event'
import signal from '../../../../app/signal'
import app from '../../../../app'
import testApp from '../../../support/testApp'
import { destroy } from '../../../../app/registrar/phoneNumber/destroy'
import { deepChannelFactory } from '../../../support/factories/channel'
import { eventFactory } from '../../../support/factories/event'
import { eventTypes } from '../../../../app/db/models/event'
const fs = require('fs-extra')
import { genPhoneNumber, phoneNumberFactory } from '../../../support/factories/phoneNumber'
describe('phone number services -- destroy module', () => {
// SETUP
const phoneNumber = '+11111111111'
const phoneNumber = genPhoneNumber()
const phoneNumberRecord = phoneNumberFactory({ phoneNumber })
const channel = deepChannelFactory({ phoneNumber })
const sender = channel.memberships[0].memberPhoneNumber
let findChannelStub,
findPhoneNumberStub,
broadcastMessageStub,
releasePhoneNumberStub,
destroyChannelSpy,
destroyPhoneNumberSpy,
destroyChannelStub,
destroyPhoneNumberStub,
deleteDirStub,
twilioRemoveSpy,
twilioRemoveStub,
signaldUnsubscribeStub,
logEventStub
const destroyChannelSucceeds = () =>
findChannelStub.callsFake((_, phoneNumber) =>
Promise.resolve({
destroy: destroyChannelSpy,
phoneNumber,
memberships: [{ memberPhoneNumber: '+12223334444' }, { memberPhoneNumber: '+15556667777' }],
}),
)
const channelDoesNotExist = () => findChannelStub.callsFake(() => Promise.resolve(null))
const destroyChannelFails = () =>
findChannelStub.callsFake((_, phoneNumber) =>
Promise.resolve({
destroy: () => {
throw 'Failed to destroy channel'
},
phoneNumber,
memberships: [],
}),
)
const destroyChannelNotCalled = () => findChannelStub.returns(deepChannelFactory({ phoneNumber }))
const destroyPhoneNumberSucceeds = () =>
findPhoneNumberStub.callsFake((_, phoneNumber) =>
Promise.resolve({ destroy: destroyPhoneNumberSpy, phoneNumber, twilioSid: 'PN123' }),
)
const destroyPhoneNumberFails = () =>
findPhoneNumberStub.callsFake((_, phoneNumber) =>
Promise.resolve({
destroy: () => {
throw 'Failed to destroy phone number'
},
phoneNumber,
twilioSid: 'PN123',
}),
)
const destroySignalEntrySucceeds = () => {}
const destroySignalEntryFails = async () => {
deleteDirStub.throws()
}
const releasePhoneNumberSucceeds = () => {
releasePhoneNumberStub.callsFake(() => ({
incomingPhoneNumbers: () => ({ remove: () => twilioRemoveSpy() }),
}))
}
const releasePhoneNumberFails = () => {
releasePhoneNumberStub.throws({ message: 'oh noes!' })
}
const broadcastMessageSucceeds = () => broadcastMessageStub.returns(Promise.resolve())
const signaldUnsubscribeFails = () =>
signaldUnsubscribeStub.throws(() => {
return new Error('Signald failed to unsubscribe')
})
logEventStub,
commitStub,
rollbackStub
before(async () => {
await app.run(testApp)
})
beforeEach(() => {
commitStub = sinon.stub()
rollbackStub = sinon.stub()
sinon.stub(app.db.sequelize, 'transaction').returns({
commit: commitStub,
rollback: rollbackStub,
})
findChannelStub = sinon.stub(channelRepository, 'findDeep')
findPhoneNumberStub = sinon.stub(phoneNumberRepository, 'find')
broadcastMessageStub = sinon.stub(signal, 'broadcastMessage')
destroyChannelSpy = sinon.spy()
destroyPhoneNumberSpy = sinon.spy()
twilioRemoveSpy = sinon.spy()
releasePhoneNumberStub = sinon.stub(commonService, 'getTwilioClient')
destroyChannelStub = sinon.stub(channelRepository, 'destroy')
destroyPhoneNumberStub = sinon.stub(phoneNumberRepository, 'destroy')
twilioRemoveStub = sinon.stub()
sinon.stub(commonService, 'getTwilioClient').callsFake(() => ({
incomingPhoneNumbers: () => ({ remove: twilioRemoveStub }),
}))
deleteDirStub = sinon.stub(fs, 'remove').returns(['/var/lib'])
signaldUnsubscribeStub = sinon.stub(signal, 'unsubscribe')
logEventStub = sinon
......@@ -129,29 +80,33 @@ describe('phone number services -- destroy module', () => {
it('returns an error status', async () => {
const response = await destroy({ phoneNumber })
expect(response).to.eql({
message: 'No records found for +11111111111',
message: `No records found for ${phoneNumber}`,
status: 'ERROR',
})
})
})
describe('when no channel exists with with given phone number', () => {
describe('when phone number exists but no channel uses it', () => {
beforeEach(async () => {
channelDoesNotExist()
broadcastMessageSucceeds()
destroySignalEntrySucceeds()
releasePhoneNumberSucceeds()
destroyPhoneNumberSucceeds()
// no channel
findChannelStub.returns(Promise.resolve(null))
// yes phone number
findPhoneNumberStub.returns(Promise.resolve(phoneNumberRecord))
// all helpers succeed
broadcastMessageStub.returns(Promise.resolve())
deleteDirStub.returns(Promise.resolve())
twilioRemoveStub.returns(Promise.resolve())
destroyPhoneNumberStub.returns(Promise.resolve())
})
it('destroys the phone number', async () => {
await destroy({ phoneNumber })
expect(destroyPhoneNumberSpy.callCount).to.eql(1)
expect(destroyPhoneNumberStub.callCount).to.eql(1)
})
it('releases the phone number back to twilio', async () => {
await destroy({ phoneNumber })
expect(twilioRemoveSpy.callCount).to.eql(1)
expect(twilioRemoveStub.callCount).to.eql(1)
})
it('returns SUCCESS', async () => {
......@@ -166,7 +121,7 @@ describe('phone number services -- destroy module', () => {
it('does not attempt to destroy a channel', async () => {
await destroy({ phoneNumber })
expect(destroyChannelSpy.callCount).to.eql(0)
expect(destroyChannelStub.callCount).to.eql(0)
})
})
})
......@@ -178,30 +133,35 @@ describe('phone number services -- destroy module', () => {
describe('all tasks succeed', () => {
beforeEach(async () => {
broadcastMessageSucceeds()
destroyChannelSucceeds()
destroySignalEntrySucceeds()
releasePhoneNumberSucceeds()
destroyPhoneNumberSucceeds()
findChannelStub.returns(Promise.resolve(channel))
findPhoneNumberStub.returns(Promise.resolve(phoneNumberRecord))
broadcastMessageStub.returns(Promise.resolve())
deleteDirStub.returns(Promise.resolve())
twilioRemoveStub.returns(Promise.resolve())
destroyPhoneNumberStub.returns(Promise.resolve())
})
describe('destroy command called from maintainer', () => {
it('notifies all the members of the channel of destruction', async () => {
await destroy({ phoneNumber })
expect(broadcastMessageStub.getCall(0).args[0]).to.eql(['+12223334444', '+15556667777'])
expect(broadcastMessageStub.getCall(0).args[0]).to.eql(
map(channel.memberships, 'memberPhoneNumber'),
)
})
})
describe('destroy command called from admin of channel', () => {
it('notifies all members of the channel except for the sender', async () => {
await destroy({ phoneNumber, sender: '+15556667777' })
expect(broadcastMessageStub.getCall(0).args[0]).to.eql(['+12223334444'])
await destroy({ phoneNumber, sender })
expect(broadcastMessageStub.getCall(0).args[0]).to.eql(
map(channel.memberships, 'memberPhoneNumber').slice(1),
)
})
})
it('destroys the channel in the db', async () => {
await destroy({ phoneNumber })
expect(destroyChannelSpy.callCount).to.eql(1)
expect(destroyChannelStub.callCount).to.eql(1)
})
it('logs a CHANNEL_DESTROYED event', async () => {
......@@ -212,19 +172,22 @@ describe('phone number services -- destroy module', () => {
])
})
it('deletes the associated signal data dir', async () => {
it("deletes the channel's signal keystore", async () => {
await destroy({ phoneNumber })
expect(deleteDirStub.getCall(0).args[0]).to.eql('/var/lib/signald/data/+11111111111')
expect(map(deleteDirStub.getCalls(), 'args')).to.eql([
[`/var/lib/signald/data/${phoneNumber}`],
[`/var/lib/signald/data/${phoneNumber}.d`],
])
})
it('releases the phone number to twilio', async () => {
await destroy({ phoneNumber })
expect(twilioRemoveSpy.callCount).to.eql(1)
expect(twilioRemoveStub.callCount).to.eql(1)
})
it('destroys the phoneNumber in the db', async () => {
await destroy({ phoneNumber })
expect(destroyPhoneNumberSpy.callCount).to.eql(1)
expect(destroyPhoneNumberStub.callCount).to.eql(1)
})
it('unsubscribes the phoneNumber from signald', async () => {
......@@ -232,6 +195,11 @@ describe('phone number services -- destroy module', () => {
expect(signaldUnsubscribeStub.callCount).to.eql(1)
})
it('commits the db transaction', async () => {
await destroy({ phoneNumber })
expect(commitStub.callCount).to.eql(1)
})
it('returns a success status', async () => {
const response = await destroy({ phoneNumber })
expect(response).to.eql({
......@@ -243,121 +211,166 @@ describe('phone number services -- destroy module', () => {
describe('when notifying members fails', () => {
beforeEach(async () => {
destroyPhoneNumberSucceeds()
destroySignalEntrySucceeds()
releasePhoneNumberSucceeds()
findChannelStub.callsFake((_, phoneNumber) =>
Promise.resolve({
destroy: destroyChannelSpy,
phoneNumber,
memberships: [
{ memberPhoneNumber: '+12223334444', type: 'ADMIN' },
{ memberPhoneNumber: '+15556667777', type: 'ADMIN' },
],
}),
)
broadcastMessageStub.onFirstCall().returns(Promise.reject('Failed to broadcast message'))
broadcastMessageStub.onSecondCall().returns(Promise.resolve())
// business logic succeeds
findChannelStub.returns(Promise.resolve(channel))
findPhoneNumberStub.returns(Promise.resolve(phoneNumberRecord))
destroyChannelStub.returns(Promise.resolve(true))
destroyPhoneNumberStub.returns(Promise.resolve(true))
twilioRemoveStub.returns(Promise.resolve())
broadcastMessageStub.returns(Promise.resolve())
deleteDirStub.returns(Promise.resolve())
// notifying members fails
broadcastMessageStub
.onCall(0)
.callsFake(() => Promise.reject('Failed to broadcast message'))
// notifying maintainers of error succeeds
broadcastMessageStub.onCall(1).returns(Promise.resolve())
})
it('returns an error status', async () => {
const response = await destroy({ phoneNumber })
expect(response).to.eql({
message:
'Failed to destroy channel for +11111111111. Error: Failed to broadcast message',
message: `Failed to destroy channel for ${phoneNumber}. Error: Failed to broadcast message`,
status: 'ERROR',
})
})
it('rolls back the db transaction', async () => {
await destroy({ phoneNumber })
expect(rollbackStub.callCount).to.eql(1)
expect(commitStub.callCount).to.eql(0)
})
})
describe('when destroying the channel in the db fails', () => {
describe('when deleting the phone number from the db fails', () => {
beforeEach(async () => {
broadcastMessageSucceeds()
destroyChannelFails()
findChannelStub.returns(Promise.resolve(channel))
findPhoneNumberStub.returns(Promise.resolve(phoneNumberRecord))
destroyPhoneNumberStub.callsFake(() => Promise.reject('Gnarly db error!'))
})
it('returns an error status', async () => {
const response = await destroy({ phoneNumber })
expect(response).to.eql({
message: 'Failed to destroy channel for +11111111111. Error: Failed to destroy channel',
message: `Failed to destroy channel for ${phoneNumber}. Error: Gnarly db error!`,
status: 'ERROR',
})
})