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

feat(notification): support for multiple phone numbers #738

Merged
merged 2 commits into from
Nov 10, 2020

Conversation

benw25
Copy link
Contributor

@benw25 benw25 commented Nov 10, 2020

Description

Fixes #273.
Update to stale pull request #446

Multiple carriers & phone numbers are now supported in the .env file. Number of carriers must strictly match the number of phone numbers (documented in readme)

Error handling was changed a bit in sms.ts:

  • if number of carriers does not match number of phones (even if one is 0), logger will warn
  • if there is a difference in carrier.length or number.length, sendSms will say:

(carrier) is not associated with a number

or

(number) is not associated with a carrier

  • phone.availableCarriers.has(carrier) has been moved to trigger a logging error inside of sendSms, and this logging function has been removed from generateAddress
  • any of these above errors will continue and skip the rest of sendSms
  • generateAddress now takes in a number and a carrier as arguments
  • sendMail error callback now has the specific number & carrier to provide more info as to what happened

Misc changes were added to fix linting errors:

  • max-params: 0 (unlimited) to account for xo linting showing an error in maxPrice in logger.ts. 5 is more than the apparent default of 4.
  • because templating was added, I removed the backslash in the error logging for

couldn't send sms

Testing

  • Tested multiple phone numbers & carriers (0 & 0, 1 & 1, 2 & 2), and mismatched combinations of the two (0 & 1, 1 & 0, ...etc.).
  • Tested with carriers that do not match the list of availableCarriers (e.g. "goog")
  • Tested with spaces in the envs, seems like this is already covered in the .trim in envOrArray
  • All the above was run with npm run test:notification
  • Ran the app with multiple numbers/carriers with no issues

@benw25 benw25 requested a review from jef as a code owner November 10, 2020 01:53
@benw25 benw25 changed the title support for multiple phone numbers, improved error handling for sendSms feat: support for multiple phone numbers, improved error handling for sendSms Nov 10, 2020
@jef
Copy link
Owner

jef commented Nov 10, 2020

  • max-params: 0 (unlimited) to account for xo linting showing an error in maxPrice in logger.ts. 5 is more than the apparent default of 4.

Maybe this could be something more reasonable like 10 for now? I'll change that in a later PR. Thanks for updating :)

Copy link
Owner

@jef jef left a comment

Choose a reason for hiding this comment

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

Great! I appreciate your work on fixing this. I really appreciate the help!

Comment on lines +22 to +28
if (!currentNumber) {
logger.error(`✖ ${currentCarrier} is not associated with a number`);
continue;
} else if (!currentCarrier) {
logger.error(`✖ ${currentNumber} is not associated with a carrier`);
continue;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Since we are doing the check on sizing in sms.ts:13-15, I'm not sure if this is needed.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, I see now. It's because it still allows the user to have misconfigured phone config.

@jef jef changed the title feat: support for multiple phone numbers, improved error handling for sendSms feat(notification): support for multiple phone numbers Nov 10, 2020
@jef jef merged commit 9f28fe5 into jef:main Nov 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible to add function for two phon numbers?
2 participants