Verified Commit 1b7d24ed authored by aguestuser's avatar aguestuser
Browse files

[91] refactor: use notifications type when dispatching REMOVE



context:
* long-term goal: all command responses return notifications,
  messenger is dumb: it just sees notificatoins in the CR, and
  gets 'em out the door!
* but we want to move incrementally and move to that signature one
  command at a time!
* this commit uses the refactor pattern that we want for *all*
  commands but only for the REMOVE comand

implementation:
* `execute.processCommand` (-> `removeAdmin`):
  * generates a notification for each person who is affected by the command
  * returns an array of notifications in the commandResult
* `messenger.handleNotifications` sees the notifications and sends
  them (doing a bit of datamunging that is necessary to keep the
  contract with `notify` but which we will eventually get rid of)
* TESTS!!! (both for `execute.removeAdmin` and for `messenger.handleNotification`)

co-authored-by: Mari's avatarMari <m@marrri.com>
parent c892b7de
......@@ -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
......
......@@ -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,
......
......@@ -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']),
......
......@@ -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),
})
})
})
......
......@@ -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}`),
])
})
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment