feat(sms): Add localized body text to SMS #18312
Conversation
vpomerleau
force-pushed
the
FXA-11007
branch
4 times, most recently
from
255780d to
c82a71e
Compare
| @@ -2181,7 +2181,7 @@ const convictConf = convict({ | |||
| format: Array, | |||
| }, | |||
| maxMessageLength: { | |||
| default: 60, | |||
| default: 160, | |||
There was a problem hiding this comment.
@dschom I saw that 160 was the limit for a deprecated Twilio endpoint, other active endpoints seem to have a limit of 1600 (which is way too much IMO for our needs). Considering that the localized body text in English is roughly 60, setting the limit here to 160 seems fair to account for expansion in other locales.
Is there a reason for the limit being set to 60 here vs 160 in sms.manager.spec.ts?
There was a problem hiding this comment.
I think 60 was just seemed like a 'short' message. If 160 is needed, that's no problem.
There was a problem hiding this comment.
I've shortened the English message to be under 60 characters, but languages that use a different encoding like Russian and Japanese might require a higher number of characters. Hopefully comfortably under the 160 character limit.
There was a problem hiding this comment.
I read up a bit more on the encoding for SMS, and it's trickier to calculate than I thought. For now I've reverted to full length message from Figma which is about 100 characters long - this is fine for GSM encoding - and added a comment in the FTL string to check the messaging segment calculator when localizing. I've also filed a follow-up (FXA-11072) to review handling for formatted messages that exceed the max length set here.
vpomerleau
force-pushed
the
FXA-11007
branch
2 times, most recently
from
87debca to
14416c8
Compare
libs/accounts/recovery-phone/src/lib/recovery-phone.manager.in.spec.ts
Outdated
Show resolved
Hide resolved
libs/accounts/recovery-phone/src/lib/recovery-phone.manager.in.spec.ts
Outdated
Show resolved
Hide resolved
| public async setupPhoneNumber( | ||
| uid: string, | ||
| phoneNumber: string, | ||
| localizedMessageBody?: { part1: string; part2: string } |
There was a problem hiding this comment.
Instead of doing part1 and part 2, another approach is a template function, e.g.
messageBody = (code:string) =>${code}`
This is a bit cleaner, because it abstracts away any assumptions about the message format.
There was a problem hiding this comment.
Yes, I see what you mean! I'll have to modify our localization handling for auth-server a bit because it wasn't set up to take in variables, but should be a simple enough change to make this work.
libs/accounts/recovery-phone/src/lib/recovery-phone.service.spec.ts
Outdated
Show resolved
Hide resolved
| } catch (error) { | ||
| // log email send error but don't throw | ||
| // user should be allowed to proceed | ||
| this.log.trace('account.recoveryPhone.phoneRemovedNotification.error', { |
There was a problem hiding this comment.
I feel like statsd metrics are more useful than logs for monitoring things.
There was a problem hiding this comment.
Will look into using that instead, but might file a follow up
vpomerleau
force-pushed
the
FXA-11007
branch
2 times, most recently
from
f713205 to
fdbdd52
Compare
Because: * We want to include a localized message in addition to the code This commit: * Add ftl strings * Pass strings to sms manager for inclusion in sms * Add missing import in integration test * Add tags to accounts libs projects for inclusion in CI integration tests Closes #FXA-11007
Because
This pull request
Issue that this pull request solves
Closes: #FXA-11007
Checklist
Put an
xin the boxes that applyScreenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
Noticed that the recovery-phone libs tests weren't running on CI and added them + updated instructions in readme to run the tests locally.