Phone number inventory management #1525
Conversation
schuyler1d
added
the
S-work in progress (WIP)
matteosb
changed the title
src/server/api/lib/twilio.js
Outdated
| @@ -10,6 +10,8 @@ import { | |||
| import { log } from "../../../lib"; | |||
| import { saveNewIncomingMessage } from "./message-sending"; | |||
| import { getConfig } from "./config"; | |||
| import _ from "lodash"; | |||
| import urlJoin from "url-join"; | |||
There was a problem hiding this comment.
doesn't look like you're using this method or library -- can we delete it after all?
src/server/api/lib/fakeservice.js
Outdated
| const rows = _.times(limit, () => ({ | ||
| organization_id: organization.id, | ||
| area_code: areaCode, | ||
| phone_number: `+1${areaCode}555FAKE`, |
There was a problem hiding this comment.
doesn't this need to be different for each row? -- I do like the TEXT in the number to make it 'really broken' -- maybe 555FK{random two digits}?
Similar to this code:
https://github.com/MoveOnOrg/Spoke/blob/main/src/integrations/contact-loaders/test-fakedata/index.js#L128-L130
Nit: Aesthetically, we try to avoid using lodash, when a native JS pattern is ok. _.chunk seems like a decent exception. I'm on the edge here -- I think I'd favor a boring for loop, so people don't need to lookup what _.times does.
| @@ -91,6 +91,13 @@ export default function renderIndex(html, css, assetMap, store) { | |||
| window.EXPERIMENTAL_TAGS=${process.env.EXPERIMENTAL_TAGS || false} | |||
| window.EXPERIMENTAL_TEXTERUI="${process.env.EXPERIMENTAL_TEXTERUI || ""}" | |||
| window.TWILIO_MULTI_ORG=${process.env.TWILIO_MULTI_ORG || false} | |||
| window.EXPERIMENTAL_PHONE_INVENTORY=${getConfig( | |||
There was a problem hiding this comment.
one downside of using global window variables for these things is that it can't be per-org, then, because this globally loaded -- it might be better to have an organization graphql property that can return getConfig('EXPERIMENTAL_PHONE_INVENTORY', organization) and then condition on that (after adding to a (apparently not yet present) organization query in the components/AdminDashboard.jsx component
Right now this writes fake data rather than talking to twilio
|
@schuyler1d This is ready for a full review now. Screenshots:
|
matteosb
removed
the
S-work in progress (WIP)
|
Added functionality to add purchased numbers to organization messaging services from the UI
cc @jeffm2001, this should work well with TWILIO_MULTI_ORG |
src/server/api/lib/twilio.js
Outdated
| */ | ||
| async function searchForAvailableNumbers(twilioInstance, areaCode, limit) { | ||
| const count = Math.min(limit, 30); // Twilio limit | ||
| return twilioInstance.availablePhoneNumbers("US").local.list({ |
There was a problem hiding this comment.
let's make this an environment variable getConfig(COUNTRY_CODE, organization) || "US"
There was a problem hiding this comment.
Can I use the PHONE_NUMBER_COUNTRY env var, which appears to be used for formatting, or is there a reason to introduce a new one?
src/server/api/schema.js
Outdated
| @@ -1298,6 +1299,52 @@ const rootMutations = { | |||
| .where("id", id) | |||
| .update({ is_deleted: true }); | |||
| return { id }; | |||
| }, | |||
| buyPhoneNumbers: async ( | |||
There was a problem hiding this comment.
@lperson has started what might be a good trend in creating these as mutations/ files rather than continue making the large schema.js file even bigger.
Optional, but if you move it to one, I think that would be good. If you do so, please move the whole function, so you can import and then just include buyPhoneNumbers as a unitary key-value in the list.
There was a problem hiding this comment.
nice, i definitely prefer this pattern
| @@ -61,6 +61,7 @@ const tableList = [ | |||
| "question_response", | |||
| "tag", | |||
| "tag_campaign_contact", | |||
| "owned_phone_number", | |||
There was a problem hiding this comment.
I know it's silly, but can you create a models/owned-phone-number.js file in the style of the old ones. even if it's dead code, I'd like us to keep that directory documenting the current schema for all the models (rather than needing to search through changes in the migrations).
There was a problem hiding this comment.
Done. As we plan the final phase of the migration away from thinky we should consider adding a pg schema dump as a reference
src/workers/jobs.js
Outdated
| if (!areaCode || !limit) { | ||
| throw new Error("areaCode and limit are required"); | ||
| } | ||
| const organization = await Organization.get(job.organization_id); |
There was a problem hiding this comment.
please use loaders here, so cache can be used.
There was a problem hiding this comment.
cacheableData, not loaders, since we are outside of the request scope right?
__test__/cypress/plugins/index.js
Outdated
| @@ -5,6 +5,20 @@ require("dotenv").load(); | |||
| require("babel-register"); | |||
| require("babel-polyfill"); | |||
|
|
|||
| if (process.env.DEFAULT_SERVICE !== "fakeservice") { | |||
| throw Error( | |||
There was a problem hiding this comment.
Even so, can we do conditionals on DEFAULT_SERVICE == 'fakeservice' for the number buying parts within the cypress tasks themselves?
|
@schuyler1 review comments addressed, mind taking another look? |
schuyler1d
added
the
S-ready for stage-main (qa)
First part of #1518.
This adds the phone number inventory table and basic admin functionality to list and buy numbers. It also allows numbers to be added directly to organization-scoped Twilio Messaging Services.
Note: allowing individual campaigns to create messaging services with numbers from the inventory will be added in a later release.