Skip to content

LG-11317 account ref string for sms#9474

Merged
JackRyan1989 merged 12 commits intomainfrom
jryan/lg-11317-account-ref-string-for-sms
Nov 3, 2023
Merged

LG-11317 account ref string for sms#9474
JackRyan1989 merged 12 commits intomainfrom
jryan/lg-11317-account-ref-string-for-sms

Conversation

@JackRyan1989
Copy link
Contributor

@JackRyan1989 JackRyan1989 commented Oct 27, 2023

🎫 Ticket

🛠 Summary of changes

Added reference number to SMS message & translations. This number is simply the enrollment code.

📜 Testing Plan

  1. Push branch to dev sandbox and create an enrollment there.
  2. Have USPS pass the enrollment to trigger an SMS response.
  3. Capture image of SMS message on phone.
  4. Supply screen shots for design review.
  5. Confirm translations.

@JackRyan1989 JackRyan1989 self-assigned this Oct 27, 2023
@gina-yamada
Copy link
Contributor

gina-yamada commented Oct 30, 2023

@JackRyan1989 Your French translation uses reference_number. Your other translation files use reference_string. Your test will pass when you change it

@JackRyan1989 JackRyan1989 requested review from a team, rutvigupta-design and svalexander and removed request for a team October 30, 2023 20:49
@JackRyan1989 JackRyan1989 marked this pull request as ready for review October 30, 2023 20:49
@gina-yamada
Copy link
Contributor

gina-yamada commented Oct 31, 2023

Your code changes look good to me. Maybe overkill but what do you think about adding to tests for the English and Spanish version of the outgoing SMS (just because it is going out to our users- just to be really sure it reads as we'd expect)? If the variable for Spanish or English got mistyped it may have not been caught as it was a sneaky typo. I think lucky we tested in French. Ultimately up to you, I'd still approve if you disagree.

Is it possible to see screenshots/testing in joy before approving or does it need to happen in dev? (If dev, if you merge in right after a deployment on Thursday it might buy you some more time to test so that this does not make its way up to production as fast.) Also, when testing, an enrollment for each language would be nice.

@JackRyan1989
Copy link
Contributor Author

Your code changes look good to me. Maybe overkill but what do you think about adding to tests for the English and Spanish version of the outgoing SMS (just because it is going out to our users- just to be really sure it reads as we'd expect)? If the variable for Spanish or English got mistyped it may have not been caught as it was a sneaky typo. I think lucky we tested in French. Ultimately up to you, I'd still approve if you disagree.

Is it possible to see screenshots/testing in joy before approving or does it need to happen in dev? Also, when testing, an enrollment for each language would be nice.

Totally. I'll add the english and spanish tests as well.

Copy link
Contributor

@gina-yamada gina-yamada left a comment

Choose a reason for hiding this comment

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

Code changes look good. You are adding tests for Spanish and English soon.

)

job.perform(passed_enrollment.id)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

😍

@carmenrosalop carmenrosalop requested review from carmenrosalop and removed request for rutvigupta-design November 3, 2023 19:58
Copy link

@carmenrosalop carmenrosalop left a comment

Choose a reason for hiding this comment

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

LGTM!

@JackRyan1989 JackRyan1989 merged commit 89963a7 into main Nov 3, 2023
@JackRyan1989 JackRyan1989 deleted the jryan/lg-11317-account-ref-string-for-sms branch November 3, 2023 20:09
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.

4 participants