Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete phone numbers from inventory and Twilio #1881

Merged
merged 5 commits into from
Jan 27, 2021

Conversation

jeffm2001
Copy link
Collaborator

Description

Adds delete buttons to the phone number inventory page, which lets you bulk remove unallocated numbers for an area code from both the Spoke inventory and you Twilio account.

2020-11-06 at 9 03 AM
2020-11-06 at 9 03 AM

Checklist:

  • I have manually tested my changes on desktop and mobile
  • The test suite passes locally with my changes
  • If my change is a UI change, I have attached a screenshot to the description section of this pull request
  • My change is 300 lines of code or less, or has a documented reason in the description why it’s longer
  • I have made any necessary changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • My PR is labeled [WIP] if it is in progress

agreenspan24
agreenspan24 previously approved these changes Nov 6, 2020
Copy link
Collaborator

@agreenspan24 agreenspan24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe not a task for this first iteration, but would be nice to be able to control how many numbers are being deleted - add some sort of limit there.

Otherwise looks mostly good!

style: inlineStyles.column,
render: (columnKey, row) =>
this.props.params.ownerPerms ? (
<FlatButton
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not an IconButton?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially tried IconButton but it didn't look right to me. No hover state and the tool tip was getting cut off by the row, which I'm sure is fixable but FlatButton seemed fine.

@@ -264,6 +309,29 @@ class AdminPhoneNumberInventory extends React.Component {
>
{this.renderBuyNumbersForm()}
</Dialog>
<Dialog
title="Delete Numbers"
modal={false}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe false is the default here.

src/workers/jobs.js Outdated Show resolved Hide resolved
matteosb
matteosb previously approved these changes Nov 8, 2020
Copy link
Collaborator

@matteosb matteosb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌

*/
async function deleteNumber(twilioInstance, phoneSid, phoneNumber) {
const response = await twilioInstance.incomingPhoneNumbers(phoneSid).remove();
if (response.error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my one request here is that instead of throwing if deleting a number fails, we should just count/track the errors here. A use case that came up for us and at least one other campaign was deleting numbers in Spoke when at least some of them were already deleted from the Twilio console.

Other than that, looks good.

@schuyler1d schuyler1d added the S-waiting on author Status: PR is awaiting some action (such as code changes) from the PR author label Nov 10, 2020
@jeffm2001 jeffm2001 removed the S-waiting on author Status: PR is awaiting some action (such as code changes) from the PR author label Nov 16, 2020
@schuyler1d schuyler1d mentioned this pull request Jan 15, 2021
@schuyler1d schuyler1d merged commit 23a8bea into StateVoicesNational:main Jan 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants