Skip to content

Fyst 293/implement sms notification delivery #5064

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.

Sign up for GitHub

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

Jump to bottom
Merged
powersurge360 merged 4 commits into from
Dec 3, 2024
Merged

Fyst 293/implement sms notification delivery #5064

powersurge360 merged 4 commits into from
Dec 3, 2024

Conversation

@powersurge360
Copy link
Contributor

@powersurge360 powersurge360 commented Dec 2, 2024

Link to pivotal/JIRA issue

Is PM acceptance required? (delete one)

  • Yes - don't merge until JIRA issue is accepted!

Reminder: merge main into this branch and get green tests before merging to main

What was done?

  • Hooks up send sms in the messaging service. Any message that uses the service should automatically pick up for SMS
  • It also adds a test harness for smoke testing notifications for both email and SMS. It inspects the automated message namespace to automatically register new messages. It creates a temporary intake and accepts build args which can be used to populate content. For example:
image

How to test?

  • Specs provided
  • I'm a little unsure where all the triggers are. If acceptance has trouble I'll need a little guidance in testing in the application
  • Smoke testing of SMS can be done with the new harness.

Screenshots (for visual changes)

  • Before
  • After

@github-actions
Copy link

github-actions bot commented Dec 2, 2024

Heroku app: https://gyr-review-app-5064-d2461821117c.herokuapp.com/
View logs: heroku logs --app gyr-review-app-5064 (optionally add --tail)

jnf
jnf approved these changes Dec 3, 2024
View reviewed changes
Copy link
Contributor

@jnf jnf left a comment

Choose a reason for hiding this comment

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

love it

app/services/state_file/messaging_service.rb Outdated
end


# def send_sms
Copy link
Contributor

@jnf jnf Dec 3, 2024

Choose a reason for hiding this comment

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

[pebble] let's clean this up

mrotondo
mrotondo approved these changes Dec 3, 2024
View reviewed changes
Copy link
Contributor

@mrotondo mrotondo left a comment

Choose a reason for hiding this comment

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

lgtm also :)

@powersurge360 powersurge360 merged commit 4d28cc0 into main Dec 3, 2024
4 checks passed
@powersurge360 powersurge360 deleted the FYST-293/implement-sms-notification-delivery branch December 3, 2024 17:16
@jnf jnf mentioned this pull request Dec 5, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jnf jnf jnf approved these changes

@mrotondo mrotondo mrotondo approved these changes

Assignees

No one assigned

Labels

in-acceptance

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@powersurge360 @jnf @mrotondo