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

Add OneSignal delivery method #524

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Conversation

Spone
Copy link
Contributor

@Spone Spone commented Jan 31, 2025

Pull Request

Summary:
Add a OneSignal delivery method

Co-Authored with @lovethebomb

Related Issue:
#473

Description:
We used the Twilio delivery method as a starting point to implement OneSignal.

Testing:
We added a tests based on Twilio delivery method tests, but I think more thorough testing would be needed.

Checklist:

  • Code follows the project's coding standards
  • Tests have been added or updated to cover the changes
  • Documentation has been updated (if applicable)
  • All existing tests pass
  • Conforms to the contributing guidelines

Additional Notes:

@excid3
Copy link
Owner

excid3 commented Jan 31, 2025

I'm not familiar with OneSignal API but looks like they also support email, SMS, so should this be named more specifically for the type or is their API generic enough to support all of their notification types?

@Spone
Copy link
Contributor Author

Spone commented Jan 31, 2025

I'm not familiar with OneSignal API but looks like they also support email, SMS, so should this be named more specifically for the type or is their API generic enough to support all of their notification types?

I'll look into it. We're only using push in our current project. At first sight, it seems you just need to alter the target_channel param.

image

@Spone

This comment was marked as resolved.

@excid3
Copy link
Owner

excid3 commented Jan 31, 2025

Standard is set to use Ruby 3.0 syntax but not newer (for compatibility) so looks like you're maybe using some newer features?

We could move to 3.1 since 3.0 is EOL but unless it's a really useful new feature, I don't mind keeping some older compatibility.

@Spone
Copy link
Contributor Author

Spone commented Jan 31, 2025

Got it! Not sure what's going on there: https://github.com/excid3/noticed/actions/runs/13077089941/job/36491828610?pr=524

@Spone
Copy link
Contributor Author

Spone commented Feb 3, 2025

@excid3 any clue about the CI fail?

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.

2 participants