-
Notifications
You must be signed in to change notification settings - Fork 118
feat: add Modica Group SMS provider support #673
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
Conversation
I just need to work out the lint errors I should fix, and those I should leave alone, Lint from prior to my commit:
Lint after my commit:
|
@dbarrosop I've added an additional commit that resolves lint / removes goconst lint supressions in favour of extracting repeating strings into consts, but it increases scope as touches a bunch of files unrelated to my change. Let me know if that approach is too heavy and I'll dial it back.
|
@superstructor yeah, please, remove the linter fixes that are unrelated to this wo. The issue is that there are a couple of linters that are not very deterministic under some circumstances and I was thinking in disabling those linters in those circumstances instead so don't worry about them, I will fix them myself and push to your branch os ask you to rebase afterwards. Thanks! |
what the... I rebased your branch because I merged a couple of other PRs that caused several conflicts and git closed your PR... let me see if I can figure it out... |
Ok, looks like the issue might be related to the fact your PR came from your main branch... Anyway, could you try to open this PR: https://github.com/nhost/hasura-auth/compare/superstructor/modica?expand=1 I don't know if you will be able, otherwise feel free to copy the contents, or cherry-pick or whatever you think it's easier. Just want to make sure the git history credits you for your work... |
no worries @dbarrosop , I also had some bizzare issues with my fork, probably user error on my part 🤦🏻 I'll fix and open a fresh PR incorporating all feedback to date today. |
Yeah, I don't know if it's a git or a github issue but rebasing/committing as a maintainer on PR branches when the PR is coming from the default branch is always a mess. It is fine if the PR came from a different branch on the fork so if you need to open a new PR, please, try to do it from a branch instead of main as that usually just works if I need to do some small push/change myself before merging. Thanks again. |
Summary
This is a re-creation of #667 incorporating the feedback from @dbarrosop, sorry I broke my fork repo.
Configuration
New environment variables:
AUTH_SMS_MODICA_USERNAME
- Modica API usernameAUTH_SMS_MODICA_PASSWORD
- Modica API passwordProvider selection:
AUTH_SMS_PROVIDER=modica
to use Modica Grouptwilio
for backward compatibilityTechnical Details
+
prefix required)Test Coverage
Before submitting this PR:
Checklist
Breaking changes
No breaking changes. The implementation maintains full backward compatibility with existing Twilio configurations and defaults to Twilio when no
provider is specified.
Tests
go test -v ./...
Documentation
AUTH_SMS_PROVIDER
documentation to show bothtwilio
andmodica
optionsAUTH_SMS_MODICA_USERNAME
andAUTH_SMS_MODICA_PASSWORD
Test Plan
go test -v ./go/notifications/sms/
Warning
Despite extensive tests I havn't actually done e2e testing in the nhost stack (e..g cloud) with this setup and would appreciate your help with doing so, I can provide test Modica group credentials on DM if needed.