Add WhatsApp message sending capability to adapter and create task to call such code #287
Conversation
…ing test functions
|
for the purpose of consistency, this function should be path of This also makes sense because after thinking about the data model for the notifications feature, It makes sense that notifications model be tied to the creators model through a one to one field (just like PhoneNumber and Settings models) (see #284 (comment)). If this ends up being the approach that we take, then moving this function into the adapters.py file seems like the best thing to do. |
…create send-notification path for testing, test get method in views
…ing test functions
…create send-notification path for testing, test get method in views
AndrewLester
force-pushed
the
al-acm/whatsapp-twilio
branch
from
866c2fb
to
4ec3ce5
Compare
AndrewLester
changed the title
AndrewLester
requested review from
NdibeRaymond,
tuxology and
ajain1921
as code owners
| @@ -29,5 +29,8 @@ | |||
| path('<uuid:pk>/toggle_follow/', | |||
| ToggleFollowAPIView.as_view(), name="toggle_follow"), | |||
| path('<uuid:pk>/remove_member/', | |||
| RemoveGroupMemberAPIView.as_view(), name="remove_member") | |||
| RemoveGroupMemberAPIView.as_view(), name="remove_member"), | |||
| path('<str:phone>/send_notification', | |||
There was a problem hiding this comment.
Wondering why we have a send_notification url path. Notifications are supposed to be processed on the backend right? If you can explain the flow better so anyone not actively writing the program can understand the intent of the code better.
There was a problem hiding this comment.
The endpoint is just there for testing the Whatsapp message sending. It won't actually be called by the frontend for sending notifications and that will be handled by the backend.
There was a problem hiding this comment.
ok. maybe when we are done testing it, we can remove it so we don't push it to production by mistake? it's a potential security risk. Anyone who can find out that such endpoints exist can send a raw api request to fetch all notifications in our database. Of course it is difficult imagining who might want to do such, but still it makes sense to be mindful of things like this.
|
code is properly structured and follows the existing convention. Tomorrow I will pull it down into the machine and play around with it a bit more. One request, is it possible to squash your local commits into one before pushing? one or two is fine but multiple commits hurt the eye. |
@NdibeRaymond |
No, squashing the commit themselves before pushing. It will require a force push since you've already pushed to github. what is does is give us a single commit here, instead of the multiple commits as they are right now. |
|
@NdibeRaymond, I believe Github has a builtin feature that allows you to do this (which Aditya was referencing) so we don't have to manually do a rebase. Next to "merge pull request," click the dropdown arrow, then click "squash and merge." You can also make this the default merge strategy in the repository settings. While squash and merge won't alter commits to this branch, it will create only a single commit in master. This branch can then be deleted. We could certainly do the rebase on this branch, but I don't want to mess up the commit history yet incase there are any last minute changes. |
Yes, precisely 👍 The general idea is that if there are multiple commits in the same branch-PR, they all show up here. Even if there are multiple commits, I believe reviews can be effectively done using the The reasoning behind squashing is that each PR is a single state in the application's history that "works" And that state is linked to a single commit. In case something catastrophically fails in the PR, we can revert a single commit even from CLI and save the day. Else we may be spending most of time drinking ☕ and git bisecting 😉 |
Sounds good. If I'm not misunderstanding, there's no need for us to manually squash our commits with a rebase right? Since you'll take care of this squashing in the actual merge step. Then, this branch can be deleted and, along with it, the commits we made to the branch. |
| @@ -514,3 +515,10 @@ def create(self, request, *args, **kwargs): | |||
| {"text": text, "profile_username": result.username, "creator": request.user.username}) | |||
|
|
|||
| return Response(CreatorSerializer(result).data, status=status.HTTP_201_CREATED) | |||
|
|
|||
|
|
|||
| class SendNotificationAPIView(APIView): | |||
There was a problem hiding this comment.
I forgot to mention but since notifications are computed on the backend, I don't think there is a need for this view and it's associated endpoint. You can store the code snippet somewhere on your local machine to be used when we start integrating the notification stuff fully, or you might still leave it here for now for testing purposes, but ultimately I think it will probably be removed by the time the notification feature is up and running
Summary
Closes #285
Add a send_whatsapp method to the adapter class to send whatsapp messages to a certain phone number. The method uses a from phone number determined by a new environment variable. For testing, this means using a testing whatsapp number provided by Twilio.
Changes
Requests / Responses
Not included because the only endpoint is for alpha testing and will be removed shortly.