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
Africa's talking integration #2903
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay more data sources! I've added a few suggestions.
I think the only changes that are a must are:
- Fixing the secret used to verify messages coming back from AT
- Adding some unit tests
|
||
public function getOptions() | ||
{ | ||
return array( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible, add links to where to find API keys/secrets in the AT interface
{ | ||
// Check we have the required config | ||
if (!isset($this->config['api_key']) || !isset($this->config['username'])) { | ||
app('log')->warning('Could not send message with Africa\'s Talking, incomplete config'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just use \Log::warning
now. We finally have facades enabled for lumen!
]); | ||
// Successfully executed the request | ||
if ($response->getStatusCode() === 200) { | ||
return array(MessageStatus::SENT, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does AT return a message ID or anything that we could save? (thats the 2nd parameter here)
|
||
public function verifySecret($secret) | ||
{ | ||
if (isset($this->config['secret']) and $secret === $this->config['secret']) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't have secret
in your getOptions
above
public function registerRoutes(\Laravel\Lumen\Routing\Router $router) | ||
{ | ||
$router->post('sms/africastalking', 'Ushahidi\App\DataSource\AfricasTalking\AfricasTalkingController@handleRequest'); | ||
$router->post('africastalking', 'Ushahidi\App\DataSource\AfricasTalking\AfricasTalkingController@handleRequest'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are AT callbacks always POST? Or should we allow GET too?
Since this is a new provider I'd suggest only adding 1 url, probably sms/africastalking
.
@willdoran not sure if you saw my comments. Needs some revision |
Keeping in our notion list for reference so we can bring it up to the current code soon, closing since we don't have a plan to go live with it YET https://www.notion.so/ushahidiorg/2019-12-09-Release-replica-setup-0d35a8a7154c4de5bc69bed183c74898?p=99331d4902914a24b965c97e9d1d9528&showMoveTo=true |
This pull request makes the following changes:
Test checklist:
[ ]
I certify that I ran my checklist
Fixes ushahidi/platform# .
Ping @ushahidi/platform