SMS notifier. Closes #30.#74
SMS notifier. Closes #30.#74ghost wants to merge 6 commits intoGizra:8.x-1.xfrom portlandwebworks:sms_notifier
Conversation
jhedstrom
left a comment
jhedstrom
left a comment
There was a problem hiding this comment.
This is looking great! Thanks for working on this.
The main feedback below is to split this off into a sub-module (message_notify_sms) rather than require all of the sms stuff for all users of this module.
It would also be great to have a kernel test in addition to the unit test I think to make sure the end-to-end functionality is working.
|
Thanks @fskreuz working on this! 😄 |
| // SmsMessage.php#L326 is calling uuid service directly. Need to setup | ||
| // fake container for this test. | ||
| $container = new ContainerBuilder(); | ||
| $container->set('uuid', $this->getMockUuidService()); |
There was a problem hiding this comment.
You could subclass \Drupal\sms\Message\SmsMessage in your test file and override the uuid generator method
There was a problem hiding this comment.
This one I couldn't wrap my head around. I saw one test extending SmsMessage and was planning to do that. But not sure how that would work when the usage of SmsMessage is in the plugin, not the test.
I'm not yet that familiar with Drupal's injection system 😓 Any pointers would help.
|
I've added #76 to fix the tests. |
|
Hey everyone! I think I've added all that's needed (including kernel tests) and I think this is ready to go. Let me know if I missed anything. Thanks again for everyone's help. 😁 |
ghost
closed this
ghost
deleted the
|
Closing this PR for now while I do cleanup. Will create a separate PR (now here #77). |
4 participants
Implements an SMS notifier (connects
messagemodule to thesmsmodule).