fix(notification): sms subject output #298
Conversation
|
As an FYI, tmobile is blocking SMS messages with bestbuy.com links in them. |
|
Ahhh, I was wondering why my T-mobile SMS messages were failing. Good catch. |
| @@ -27,7 +27,7 @@ export function sendSMS(link: Link, store: Store) { | |||
| } | |||
| ] : undefined, | |||
| from: email.username, | |||
| subject: Print.inStock(link, store), | |||
| subject: Print.inStock(link, store, false, true), | |||
There was a problem hiding this comment.
Looks like I forgot to do the false here. My bad. I thought I tested that 😢
That being said, do you need to scrape the emojis as well? I don't really care, but does it not send otherwise?
There was a problem hiding this comment.
I think this is the only required change really. Just set this to
| subject: Print.inStock(link, store, false, true), | |
| subject: Print.inStock(link, store, false), |
There was a problem hiding this comment.
@jef look at the signature of the inStock print function:
inStock(link: Link, store: Store, color?: boolean, sms?: boolean)
It's always had a 3rd parameter to enable color scheme for console output, which is default to undefined (false) in most places that have nothing to do with console output. It's an optional parameter. I added a 4th optional parameter to control removing the emoji's from non-console output. Only SMS subject line is currently affected by them, everywhere else can take emojis but no color (which is the default of both optional parameters). SMS subject line specifies false for color and true for removing emojis. Everywhere else does not
There was a problem hiding this comment.
SMS subject line specifies false for color and true for removing emojis. Everywhere else does not
Right, I understand the change you're making. But I'm interested if we really need to scrape everything and add another parameter as opposed to updating the call and setting the color param to false.
There was a problem hiding this comment.
Obviously color is going to screw up the SMS. That's why I introduced this logic in the first place, but I had mistakenly forgot to set the color to false for this statement.
That being said, do emojis not work?
Oddly enough, my SMS doesn't work as is, but can definitely see this being a problem.
There was a problem hiding this comment.
I guess not for SMS.
Alright, I'll merge this one in. Thank you for the contribution!
I'm pretty sure this is illegal. I'd be hard pressed to believe this to be the case. |
| @@ -27,7 +27,7 @@ export function sendSMS(link: Link, store: Store) { | |||
| } | |||
| ] : undefined, | |||
| from: email.username, | |||
| subject: Print.inStock(link, store), | |||
| subject: Print.inStock(link, store, false, true), | |||
There was a problem hiding this comment.
I think this is the only required change really. Just set this to
| subject: Print.inStock(link, store, false, true), | |
| subject: Print.inStock(link, store, false), |
jef
changed the title
Description
Best Buy test card is out of stock and wouldn't trigger tests, replaced with an in-stock card.
SMS messages were getting jumbled mess because of emojis in the subject line:
Removing them returns the subject line back to normal
I can only confirm this was affecting TMobile sms messages, but in lieu of adding lots of conditional logic to check for carriers individually the emojis should just go for compatibility.