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

feat(phone_verify): Add Kavenegar backend #34

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

SepehrHasanabadi
Copy link
Contributor

Kavenegar is an Iranian sending message platform like twilio.
The backend class is created to send single and bulk messages to iranian sim cards.
Twilio does not support Iranian sim cards due to any reason.

@SepehrHasanabadi
Copy link
Contributor Author

@CuriousLearner
Decreasing coverage was inevitable! :(

@SepehrHasanabadi
Copy link
Contributor Author

I've increased test coverage by mocking top layer of send message functions that is clients like: twilio.rest.Client, instead of the functions we had written before.

tests/test_api.py Outdated Show resolved Hide resolved
tests/test_api.py Outdated Show resolved Hide resolved
Copy link
Owner

@CuriousLearner CuriousLearner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work!

I've just a few minor comments and we'll be good to go!

💯

@CuriousLearner CuriousLearner changed the title Kavenegar api feat(phone_verify): Add Kavenegar backend Jan 28, 2020
@CuriousLearner
Copy link
Owner

Hi @SepehrHasanabadi

Once you're done with the changes, can you please let me know how can I test the Kavenegar API locally?

Copy link
Contributor Author

@SepehrHasanabadi SepehrHasanabadi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @SepehrHasanabadi

Once you're done with the changes, can you please let me know how can I test the Kavenegar API locally?

There is its documentation:
https://github.com/kavenegar/kavenegar-python

phone_verify/backends/kavenegar.py Outdated Show resolved Hide resolved
@SepehrHasanabadi
Copy link
Contributor Author

Hi @CuriousLearner
Do you wanna test it on your phone?

Hi @SepehrHasanabadi

Once you're done with the changes, can you please let me know how can I test the Kavenegar API locally?

self.client.sms_send(params)

def send_bulk_sms(self, numbers, message):
params = {'sender': self.sender, 'receptor': numbers, 'message': message, }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API here states that sender should be a list of strings and you're providing a single sender here 🤔

https://github.com/kavenegar/kavenegar-python

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @SepehrHasanabadi

Can you please have a look at this when you get the time?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping @SepehrHasanabadi Can you please have a look at this conversation? Have you tested these changes?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, @SepehrHasanabadi, have a look at this comment as well.

@CuriousLearner
Copy link
Owner

Hi @SepehrHasanabadi Yes, the idea was for me to test it, but I don't have a number for that country. I will trust if you can ensure that when you install it in a Django project, this works and you indeed get an SMS :) So, let me know.

CuriousLearner and others added 5 commits February 19, 2020 00:55
…y into SepehrHasanabadi-kavenegar-api

* 'master' of github.com:CuriousLearner/django-phone-verify:
  chore(deps-dev): Bump django from 2.1.11 to 2.2.10 in /requirements (CuriousLearner#36)
  fix(backends): Emit `post_save` signal on phone number verification (CuriousLearner#29)
…y into SepehrHasanabadi-kavenegar-api

* 'master' of github.com:CuriousLearner/django-phone-verify:
  fix(phone_verify): Return same number of arguments in the overridden method (CuriousLearner#38)
@CuriousLearner
Copy link
Owner

@SepehrHasanabadi Hello,

Is your PR ready to review?

@SepehrHasanabadi
Copy link
Contributor Author

@CuriousLearner
hello, I didn't find any manner to test in your phone. If have any idea It is welcome.

@CuriousLearner
Copy link
Owner

@SepehrHasanabadi I think it won't be possible for me to test. But can you integrate the package with a django project and do a test that this backend works?

* master:
  tests(*): Increase test coverage to 100% 🔥 (CuriousLearner#46)
  fix(backends/nexmo): Define exception class (CuriousLearner#45)
  chore(): Release django-phone-verify 1.1.0
@CuriousLearner
Copy link
Owner

@SepehrHasanabadi I just updated your branch with the latest code that we have. Can you please take a look at the failing tests? Once they pass, please check that you get a message while using this backend.

thanks!

@SepehrHasanabadi
Copy link
Contributor Author

@CuriousLearner send_bulk_sms in KavenegarBackend does not implement by sending multi single send_sms therefore test_send_bulk_sms method needed to be modified.

@gutsytechster
Copy link
Collaborator

gutsytechster commented May 18, 2020

@SepehrHasanabadi will it make sense to put the condition if the backend is Kavenegar, then test in some other way? Most probably, you'll mock the Kavenegar API you are using for sending bulk_sms.


It seems you are doing so, just instead of return statement, you could put Kavenegar specific test within the if condition and put the remaining in the else clause.

Copy link
Collaborator

@gutsytechster gutsytechster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you have not added any Sandbox class for the Kavenegar Backend. Will it be possible for you to do so?

self.client.sms_send(params)

def send_bulk_sms(self, numbers, message):
params = {'sender': self.sender, 'receptor': numbers, 'message': message, }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, @SepehrHasanabadi, have a look at this comment as well.

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.

3 participants