diff --git a/app/services/dispatcher/commands/execute.js b/app/services/dispatcher/commands/execute.js
index 1339cd3d9523939afdf13c63c20cbda7ac59f084..e51dcfcff975fc03a233fb33e45a7f31d3193654 100644
--- a/app/services/dispatcher/commands/execute.js
+++ b/app/services/dispatcher/commands/execute.js
@@ -3,6 +3,7 @@ const channelRepository = require('../../../db/repositories/channel')
const membershipRepository = require('../../../db/repositories/membership')
const validator = require('../../../db/validations/phoneNumber')
const logger = require('../logger')
+const { getAdminMemberships } = require('../../../db/repositories/channel')
const { messagesIn } = require('../strings/messages')
const { memberTypes } = require('../../../db/repositories/membership')
const { ADMIN, NONE } = memberTypes
@@ -21,6 +22,7 @@ const {
* command: string,
* status: string,
* message: string,
+ * notifications: Array<{ recipient: Array<string>, message: string }>
* }
*
* */
@@ -134,31 +136,53 @@ const removeSender = (db, channel, sender, cr) => {
// REMOVE
-const maybeRemoveAdmin = async (db, channel, sender, adminNumber) => {
+const maybeRemoveAdmin = async (db, channel, sender, adminPhoneNumber) => {
const cr = messagesIn(sender.language).commandResponses.remove
- const { isValid, phoneNumber: validNumber } = validator.parseValidPhoneNumber(adminNumber)
+ const { isValid, phoneNumber: validAdminNumber } = validator.parseValidPhoneNumber(
+ adminPhoneNumber,
+ )
if (!(sender.type === ADMIN)) {
return { status: statuses.UNAUTHORIZED, message: cr.unauthorized }
}
if (!isValid) {
- return { status: statuses.ERROR, message: cr.invalidNumber(adminNumber) }
+ return { status: statuses.ERROR, message: cr.invalidNumber(adminPhoneNumber) }
}
- if (!(await membershipRepository.isAdmin(db, channel.phoneNumber, validNumber)))
- return { status: statuses.ERROR, message: cr.targetNotAdmin(validNumber) }
+ if (!(await membershipRepository.isAdmin(db, channel.phoneNumber, validAdminNumber)))
+ return { status: statuses.ERROR, message: cr.targetNotAdmin(validAdminNumber) }
- return removeAdmin(db, channel, validNumber, cr)
+ return removeAdmin(db, channel, validAdminNumber, sender, cr)
}
-const removeAdmin = async (db, channel, adminNumber, cr) =>
- membershipRepository
- .removeAdmin(db, channel.phoneNumber, adminNumber)
+const removeAdmin = async (db, channel, adminPhoneNumber, sender, cr) => {
+ const notifications = removalNotificationsOf(channel, adminPhoneNumber, sender)
+ return membershipRepository
+ .removeAdmin(db, channel.phoneNumber, adminPhoneNumber)
.then(() => ({
status: statuses.SUCCESS,
- message: cr.success(adminNumber),
- payload: adminNumber,
+ message: cr.success(adminPhoneNumber),
+ notifications: removalNotificationsOf(channel, adminPhoneNumber, sender),
}))
- .catch(() => ({ status: statuses.ERROR, message: cr.dbError(adminNumber) }))
+ .catch(() => ({ status: statuses.ERROR, message: cr.dbError(adminPhoneNumber) }))
+}
+
+const removalNotificationsOf = (channel, adminPhoneNumber, sender) => {
+ const allMemberships = getAdminMemberships(channel)
+ const removedMembership = allMemberships.find(m => m.memberPhoneNumber === adminPhoneNumber)
+ const bystanderMemberships = allMemberships.filter(
+ m => m.memberPhoneNumber !== sender.phoneNumber && m.memberPhoneNumber !== adminPhoneNumber,
+ )
+ return [
+ {
+ recipient: adminPhoneNumber,
+ message: `${messagesIn(removedMembership.language).notifications.toRemovedAdmin}`,
+ },
+ ...bystanderMemberships.map(membership => ({
+ recipient: membership.memberPhoneNumber,
+ message: messagesIn(membership.language).notifications.adminRemoved,
+ })),
+ ]
+}
// RENAME
diff --git a/app/services/dispatcher/messenger.js b/app/services/dispatcher/messenger.js
index 9ccf63d2f14c27a17d82d0223461d5ef4ae4cbf8..fd0ede80d62afdc1d5153b38458a9cdd0d6c3043 100644
--- a/app/services/dispatcher/messenger.js
+++ b/app/services/dispatcher/messenger.js
@@ -107,6 +107,22 @@ const handleNotifications = async ({ commandResult, dispatchable }) => {
const { command, message, status, payload } = commandResult
const { db, sock, channel, sender } = dispatchable
const notifyBase = { db, sock, channel }
+ // TODO(aguestuser|2019-12-08):
+ // once if/else branch logic has all been moved into new format
+ // - return this Promise.all
+ // - update the signature of `notify` to
+ // - take one recipient, *not* many recipients
+ // - call signal.sendMessage *not* broadcastMessage
+ // - don't call `format` (to add msg header) in `notify` anymore (?)
+ await Promise.all(
+ commandResult.notifications.map(notification =>
+ notify({
+ ...notifyBase,
+ notification: notification.message,
+ recipients: [notification.recipient],
+ }),
+ ),
+ )
if (command === commands.ADD && status === statuses.SUCCESS) {
// welcome new admin
await notify({
@@ -126,23 +142,6 @@ const handleNotifications = async ({ commandResult, dispatchable }) => {
})
}
- if (command === commands.REMOVE && status === statuses.SUCCESS) {
- // notify removed admin they've been removed
- await notify({
- ...notifyBase,
- notification: messagesIn(sender.language).notifications.toRemovedAdmin,
- recipients: [payload],
- })
- // notify everyone else that an admin has been removed
- return notify({
- ...notifyBase,
- notification: messagesIn(sender.language).notifications.adminRemoved,
- recipients: getAdminPhoneNumbers(channel).filter(
- pNum => pNum !== sender.phoneNumber && pNum !== payload,
- ),
- })
- }
-
if (command === commands.RENAME && status === statuses.SUCCESS) {
return notify({
...notifyBase,
diff --git a/test/support/factories/phoneNumber.js b/test/support/factories/phoneNumber.js
index 559328c399e61a674817d380bf8ccaf0139b8a8d..31c250e0cf97f9692b3e49417637425a757d5176 100644
--- a/test/support/factories/phoneNumber.js
+++ b/test/support/factories/phoneNumber.js
@@ -2,6 +2,9 @@ import { times, random, sample } from 'lodash'
import { statuses } from '../../../app/db/models/phoneNumber'
export const genPhoneNumber = () => '+1' + times(10, () => random(0, 9).toString()).join('')
+export const parenthesize = pn =>
+ pn.slice(0,2) + '(' + pn.slice(2,5) + ') ' + pn.slice(5)
+
export const genSid = () =>
times(34, () =>
sample(['0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'a', 'b', 'c', 'd', 'e', 'f']),
diff --git a/test/unit/services/dispatcher/commands/execute.spec.js b/test/unit/services/dispatcher/commands/execute.spec.js
index 98edf8d0151eb272ed671bdfa58588198a9b3724..205a142e49675c8bc4fabcd1489ddb8c5dea4ebf 100644
--- a/test/unit/services/dispatcher/commands/execute.spec.js
+++ b/test/unit/services/dispatcher/commands/execute.spec.js
@@ -10,33 +10,20 @@ import channelRepository from '../../../../../app/db/repositories/channel'
import membershipRepository from '../../../../../app/db/repositories/membership'
import validator from '../../../../../app/db/validations/phoneNumber'
import { subscriptionFactory } from '../../../../support/factories/subscription'
-import { genPhoneNumber } from '../../../../support/factories/phoneNumber'
+import { genPhoneNumber, parenthesize } from '../../../../support/factories/phoneNumber'
import { memberTypes } from '../../../../../app/db/repositories/membership'
import { sdMessageOf } from '../../../../../app/services/signal'
import {
adminMembershipFactory,
subscriberMembershipFactory,
} from '../../../../support/factories/membership'
+import { messagesIn } from '../../../../../app/services/dispatcher/strings/messages'
const {
signal: { signupPhoneNumber },
} = require('../../../../../app/config')
describe('executing commands', () => {
const db = {}
- const channel = {
- name: 'foobar',
- phoneNumber: '+13333333333',
- memberships: [
- ...times(2, () => adminMembershipFactory({ channelPhoneNumber: '+13333333333' })),
- ...times(2, () => subscriberMembershipFactory({ channelPhoneNumber: '+13333333333' })),
- ],
- messageCount: { broadcastIn: 42 },
- }
- const signupChannel = {
- name: 'SB_SIGNUP',
- phoneNumber: signupPhoneNumber,
- publications: channel.publications,
- }
const admin = {
phoneNumber: '+11111111111',
type: memberTypes.ADMIN,
@@ -52,6 +39,24 @@ describe('executing commands', () => {
type: memberTypes.NONE,
language: languages.EN,
}
+ const channel = {
+ name: 'foobar',
+ phoneNumber: '+13333333333',
+ memberships: [
+ adminMembershipFactory({
+ memberPhoneNumber: admin.phoneNumber,
+ channelPhoneNumber: '+13333333333',
+ }),
+ ...times(3, () => adminMembershipFactory({ channelPhoneNumber: '+13333333333' })),
+ ...times(2, () => subscriberMembershipFactory({ channelPhoneNumber: '+13333333333' })),
+ ],
+ messageCount: { broadcastIn: 42 },
+ }
+ const signupChannel = {
+ name: 'SB_SIGNUP',
+ phoneNumber: signupPhoneNumber,
+ publications: channel.publications,
+ }
describe('ADD command', () => {
let addAdminStub
@@ -420,9 +425,9 @@ describe('executing commands', () => {
beforeEach(() => removeAdminStub.returns(Promise.resolve()))
describe('when payload is a valid phone number', () => {
- const payload = '+1 (555) 555-5555' // to ensure we catch errors
- const adminPhoneNumber = '+15555555555'
- const sdMessage = sdMessageOf(channel, `REMOVE ${payload}`)
+ const removalTargetNumber = channel.memberships[1].memberPhoneNumber
+ const rawRemovalTargetNumber = parenthesize(removalTargetNumber)
+ const sdMessage = sdMessageOf(channel, `REMOVE ${rawRemovalTargetNumber}`)
const dispatchable = { db, channel, sender, sdMessage }
beforeEach(() => validateStub.returns(true))
@@ -434,7 +439,7 @@ describe('executing commands', () => {
expect(removeAdminStub.getCall(0).args).to.eql([
db,
channel.phoneNumber,
- adminPhoneNumber,
+ removalTargetNumber,
])
})
@@ -445,8 +450,23 @@ describe('executing commands', () => {
expect(await processCommand(dispatchable)).to.eql({
command: commands.REMOVE,
status: statuses.SUCCESS,
- payload: adminPhoneNumber,
- message: CR.remove.success(adminPhoneNumber),
+ message: CR.remove.success(removalTargetNumber),
+ notifications: [
+ // removed
+ {
+ recipient: removalTargetNumber,
+ message: messagesIn(languages.EN).notifications.toRemovedAdmin,
+ },
+ // bystanders
+ {
+ recipient: channel.memberships[2].memberPhoneNumber,
+ message: messagesIn(languages.EN).notifications.adminRemoved,
+ },
+ {
+ recipient: channel.memberships[3].memberPhoneNumber,
+ message: messagesIn(languages.EN).notifications.adminRemoved,
+ },
+ ],
})
})
})
@@ -458,7 +478,7 @@ describe('executing commands', () => {
expect(await processCommand(dispatchable)).to.eql({
command: commands.REMOVE,
status: statuses.ERROR,
- message: CR.remove.dbError(adminPhoneNumber),
+ message: CR.remove.dbError(removalTargetNumber),
})
})
})
@@ -475,7 +495,7 @@ describe('executing commands', () => {
expect(await processCommand(dispatchable)).to.eql({
command: commands.REMOVE,
status: statuses.ERROR,
- message: CR.remove.targetNotAdmin(adminPhoneNumber),
+ message: CR.remove.targetNotAdmin(removalTargetNumber),
})
})
})
diff --git a/test/unit/services/dispatcher/messenger.spec.js b/test/unit/services/dispatcher/messenger.spec.js
index aa515209bd890618c37ba14d33e6d9cea0686cee..37782391f3f27d7de83807564d8f585d3228b14f 100644
--- a/test/unit/services/dispatcher/messenger.spec.js
+++ b/test/unit/services/dispatcher/messenger.spec.js
@@ -22,23 +22,25 @@ describe('messenger service', () => {
const notifications = messagesIn(defaultLanguage).notifications
const [db, sock] = [{}, { write: () => {} }]
const channelPhoneNumber = genPhoneNumber()
- const subscriberNumbers = times(2, genPhoneNumber)
- const adminNumbers = [genPhoneNumber(), genPhoneNumber()]
+ const subscriberPhoneNumbers = times(2, genPhoneNumber)
+ const adminPhoneNumbers = times(4, genPhoneNumber)
const channel = {
name: 'foobar',
phoneNumber: channelPhoneNumber,
memberships: [
- { type: memberTypes.ADMIN, channelPhoneNumber, memberPhoneNumber: adminNumbers[0] },
- { type: memberTypes.ADMIN, channelPhoneNumber, memberPhoneNumber: adminNumbers[1] },
+ { type: memberTypes.ADMIN, channelPhoneNumber, memberPhoneNumber: adminPhoneNumbers[0] },
+ { type: memberTypes.ADMIN, channelPhoneNumber, memberPhoneNumber: adminPhoneNumbers[1] },
+ { type: memberTypes.ADMIN, channelPhoneNumber, memberPhoneNumber: adminPhoneNumbers[2] },
+ { type: memberTypes.ADMIN, channelPhoneNumber, memberPhoneNumber: adminPhoneNumbers[3] },
{
type: memberTypes.SUBSCRIBER,
channelPhoneNumber,
- memberPhoneNumber: subscriberNumbers[0],
+ memberPhoneNumber: subscriberPhoneNumbers[0],
},
{
type: memberTypes.SUBSCRIBER,
channelPhoneNumber,
- memberPhoneNumber: subscriberNumbers[1],
+ memberPhoneNumber: subscriberPhoneNumbers[1],
},
],
messageCount: { broadcastIn: 42 },
@@ -58,12 +60,12 @@ describe('messenger service', () => {
attachments,
}
const adminSender = {
- phoneNumber: adminNumbers[0],
+ phoneNumber: adminPhoneNumbers[0],
type: memberTypes.ADMIN,
language: languages.EN,
}
const subscriberSender = {
- phoneNumber: subscriberNumbers[0],
+ phoneNumber: subscriberPhoneNumbers[0],
type: memberTypes.SUBSCRIBER,
language: languages.EN,
}
@@ -149,7 +151,7 @@ describe('messenger service', () => {
it('broadcasts the message to all channel subscribers and admins', () => {
expect(broadcastMessageStub.getCall(0).args).to.eql([
sock,
- [...adminNumbers, ...subscriberNumbers],
+ [...adminPhoneNumbers, ...subscriberPhoneNumbers],
{ ...sdMessage, messageBody: '[foobar]\nplease help!' },
])
})
@@ -196,7 +198,7 @@ describe('messenger service', () => {
it('forwards the message to channel admins', () => {
expect(broadcastMessageStub.getCall(0).args).to.eql([
sock,
- adminNumbers,
+ adminPhoneNumbers,
{ ...sdMessage, messageBody: `[SUBSCRIBER RESPONSE]\n${sdMessage.messageBody}` },
])
})
@@ -228,7 +230,7 @@ describe('messenger service', () => {
it('forwards the message to channel admins', () => {
expect(broadcastMessageStub.getCall(0).args).to.eql([
sock,
- adminNumbers,
+ adminPhoneNumbers,
{ ...sdMessage, messageBody: `[SUBSCRIBER RESPONSE]\n${sdMessage.messageBody}` },
])
})
@@ -346,18 +348,21 @@ describe('messenger service', () => {
it('sends an alert to the other channel admins', () => {
expect(broadcastMessageStub.getCall(1).args).to.eql([
sock,
- adminNumbers,
+ adminPhoneNumbers,
sdMessageOf(channel, `[${channel.name}]\n${alert}`),
])
})
})
describe('for a REMOVE command', () => {
- const removedAdminPhoneNumber = genPhoneNumber()
- const adminsWhoDidntRemoveOrGetRemoved = getAdminPhoneNumbers(channel).filter(
- pNum => pNum !== adminSender.phoneNumber && pNum !== removedAdminPhoneNumber,
- )
- const sdMessage = `${commands.REMOVE} ${removedAdminPhoneNumber}`
+ const remover = {
+ type: memberTypes.ADMIN,
+ language: languages.EN,
+ phoneNumber: adminPhoneNumbers[0],
+ }
+ const removedPhoneNumber = adminPhoneNumbers[1]
+ const bystanderPhoneNumbers = [adminPhoneNumbers[2], adminPhoneNumbers[3]]
+ const sdMessage = `${commands.REMOVE} ${removedPhoneNumber}`
beforeEach(async () => {
await messenger.dispatch({
@@ -365,13 +370,26 @@ describe('messenger service', () => {
db,
sock,
channel,
- sender: adminSender,
+ sender: remover,
sdMessage,
},
commandResult: {
command: commands.REMOVE,
status: statuses.SUCCESS,
- payload: removedAdminPhoneNumber,
+ notifications: [
+ {
+ recipient: removedPhoneNumber,
+ message: `${messages.notifications.toRemovedAdmin}`,
+ },
+ {
+ recipient: bystanderPhoneNumbers[0],
+ message: messages.notifications.adminRemoved,
+ },
+ {
+ recipient: bystanderPhoneNumbers[1],
+ message: messages.notifications.adminRemoved,
+ },
+ ],
},
})
})
@@ -379,7 +397,7 @@ describe('messenger service', () => {
it('notifies the removed admin', () => {
expect(broadcastMessageStub.getCall(0).args).to.eql([
sock,
- [removedAdminPhoneNumber],
+ [removedPhoneNumber],
sdMessageOf(channel, `[${channel.name}]\n${messages.notifications.toRemovedAdmin}`),
])
})
@@ -387,7 +405,12 @@ describe('messenger service', () => {
it('notifies all other admins', () => {
expect(broadcastMessageStub.getCall(1).args).to.eql([
sock,
- adminsWhoDidntRemoveOrGetRemoved,
+ [bystanderPhoneNumbers[0]],
+ sdMessageOf(channel, `[${channel.name}]\n${messages.notifications.adminRemoved}`),
+ ])
+ expect(broadcastMessageStub.getCall(2).args).to.eql([
+ sock,
+ [bystanderPhoneNumbers[1]],
sdMessageOf(channel, `[${channel.name}]\n${messages.notifications.adminRemoved}`),
])
})