Registration with Phone number #1662

valaparthvi · 2017-07-19T18:03:24Z

Proposed changes in this pull request

With this PR, the user can now register with a Phone number. Once a user has registered, an email with verification link or a text with a verification token will be sent to user's registered email or phone. Once a user registers, they will be redirected to an Account Verification Page.

Registration Page

Account Verification Page

Changes

Make changes to Registration form and tests
Add AccountRegister view to accounts/views/default.py and tests
Make changes to RegistraitionSerializer and tests
Makes changes to AccountRegister view in accounts/views/api.pi and tests
Add PhoneAuthenticationBackend ,tests and corresponding settings.
Make changes to existing AuthenticationBackend to only obtain user's who are active
Add PhoneVerificationForm and tests
Add ConfirmPhone view to accounts/views/default.py and tests
Add urls and tests.
Add template for AccountVerificationPage.
Make changes to signup page by adding phone field.

When should this PR be merged

2 tests from accounts/tests/test_views_default.py currently fail and are waiting for an update from django-skivvy. Once django-skivvy is updated and all the tests pass.
Once all tests and changes are approved and reviewers are satisfied with it

Risks

None

Follow-up actions

-Install phonenumbers on staging

Checklist (for reviewing)

General

Is this PR explained thoroughly? All code changes must be accounted for in the PR description.
- Review 1
- Review 2
Is the PR labeled correctly? It should have the migration label if a new migration is added.
- Review 1
- Review 2
Is the risk level assessment sufficient? The risks section should contain all risks that might be introduced with the PR and which actions we need to take to mitigate these risks. Possible risks are database migrations, new libraries that need to be installed or changes to deployment scripts.
- Review 1
- Review 2

Functionality

Are all requirements met? Compare implemented functionality with the requirements specification.
- Review 1
- Review 2
Does the UI work as expected? There should be no Javascript errors in the console; all resources should load. There should be no unexpected errors. Deliberately try to break the feature to find out if there are corner cases that are not handled.
- Review 1
- Review 2

Code

Do you fully understand the introduced changes to the code? If not ask for clarification, it might uncover ways to solve a problem in a more elegant and efficient way.
- Review 1
- Review 2
Does the PR introduce any inefficient database requests? Use the debug server to check for duplicate requests.
- Review 1
- Review 2
Are all necessary strings marked for translation? All strings that are exposed to users via the UI must be marked for translation.
- Review 1
- Review 2
Is the code documented sufficiently? Large and complex classes, functions or methods must be annotated with comments following our code-style guidelines.
- Review 1
- Review 2
Has the scalability of this change been evaluated?
- Review 1
- Review 2
Is there a maintenance plan in place?
- Review 1
- Review 2

Tests

Are there sufficient test cases? Ensure that all components are tested individually; models, forms, and serializers should be tested in isolation even if a test for a view covers these components.
- Review 1
- Review 2
If this is a bug fix, are tests for the issue in place? There must be a test case for the bug to ensure the issue won’t regress. Make sure that the tests break without the new code to fix the issue.
- Review 1
- Review 2
If this is a new feature or a significant change to an existing feature? has the manual testing spreadsheet been updated with instructions for manual testing?
- Review 1
- Review 2

Security

Confirm this PR doesn't commit any keys, passwords, tokens, usernames, or other secrets.
- Review 1
- Review 2
Are all UI and API inputs run through forms or serializers?
- Review 1
- Review 2
Are all external inputs validated and sanitized appropriately?
- Review 1
- Review 2
Does all branching logic have a default case?
- Review 1
- Review 2
Does this solution handle outliers and edge cases gracefully?
- Review 1
- Review 2
Are all external communications secured and restricted to SSL?
- Review 1
- Review 2

Documentation

Are changes to the UI documented in the platform docs? If this PR introduces new platform site functionality or changes existing ones, the changes must be documented in the Cadasta Platform Documentation.
- Review 1
- Review 2
Are changes to the API documented in the API docs? If this PR introduces new API functionality or changes existing ones, the changes must be documented in the API docs.
- Review 1
- Review 2
Are reusable components documented? If this PR introduces components that are relevant to other developers (for instance a mixin for a view or a generic form) they should be documented in the Wiki.
- Review 1
- Review 2

oliverroick

This is already quite solid work. I left a fair bit of comments in the code, but most of these are smaller things, so don't let that discourage you. I like that you wrote so many tests for forms and serializers, that's a really good basis for further work.

oliverroick · 2017-07-20T15:37:06Z

cadasta/accounts/views/default.py

@@ -46,6 +79,7 @@ def get_context_data(self, *args, **kwargs):
        emails_to_verify = EmailAddress.objects.filter(
            user=self.object, verified=False).exists()
        context['emails_to_verify'] = emails_to_verify
+        print(context)


Stray print statement. Can you remove that?

oliverroick · 2017-07-20T15:41:58Z

cadasta/accounts/backends.py



 class AuthenticationBackend(Backend):
+    def _authenticate_by_username(self, **credentials):


I don't fully understand the use of the different authentication backends here. You extend AuthenticationBackend with _authenticate_by_username and add PhoneAuthenticationBackend. Could you explain why they are needed and what they do?

According to the django docs,

An inactive user is one that has its is_active field set to False. The ModelBackend and RemoteUserBackend authentication backends prohibits these users from authenticating. If a custom user model doesn’t have an is_active field, all users will be allowed to authenticate.

Previously authenticate() returned a user, even if their is_active status was False. I added a method self.user_can_authenticate(user) to the condition(check line 12-13 & 29-30) , which makes sure that authenticate() only returns user whose is_active is True.

PhoneAuthenticationBackend allows authenticating a user with their phone. So authenticate(username='+12345678990',password='password@1234') or authenticate(phone='+12345678990',password='password@1234') will return an authenticated user with that phone.

Previously authenticate() returned a user, even if their is_active status was False. I added a method self.user_can_authenticate(user) to the condition(check line 12-13 & 29-30) , which makes sure that authenticate() only returns user whose is_active is True.

I was reading django-allauth LoginForm and LoginView, and I don't think that changing this was a good idea. Every user is authenticated before they're allowed to login. If the user account has been set inactive, user will not be authenticated, and every time a user with inactive account tries to login, they will be shown an error The login and/or password you specified are not correct. So I think changing that back would be a better idea. Let me know your thoughts on this @oliverroick .

Yes, revert the changes.

oliverroick · 2017-07-20T15:48:38Z

cadasta/accounts/forms.py

-    email = forms.EmailField(required=True)
+    email = forms.EmailField(required=False)
+
+    message = _("Phone must have format: +9999999999. Upto 15 digits allowed."


I think, "Phone numbers must be provided in the format +99999..." sounds better. Also "Upto" should be "Up to"

oliverroick · 2017-07-20T15:49:25Z

cadasta/accounts/serializers.py

-            message=_("Another user is already registered with this "
-                      "email address")
-        )]
+            message=_("User with this Email address already exists.")


Why have you changed the error message?

Now that we have set unique to True for email and phone inside the User model, I removed the lines of code which checked uniqueness of an identifier(phone/email) from RegistrationForm, so Django now provides a default message, 'User with this {identifier_label} already exists.`, if the identifier is not unique. So I changed this as well, to show the same message everywhere.

oliverroick · 2017-07-20T15:51:33Z

cadasta/accounts/serializers.py

+        allow_null=True,
+        required=False
+    )
+    message = _("Phone must have format: +9999999999. Upto 15 digits allowed."


You're using the same message here and in the form. I would suggest moving the message into a separate file called messages.py and import the message from there. That way, if you want to change the message later, you only have to do that once.

oliverroick · 2017-07-20T16:27:32Z

cadasta/templates/accounts/account_verification.html

+      <button type="submit" name="Verify" class="btn btn-primary btn-lg btn-block text-uppercase">{% trans "Verify Token" %}</button>
+  </form>
+  {% endif %}
+  <p>{% blocktrans %} Click <a href="#">here</a> if you do not receive any email or text.{% endblocktrans %}</p>


It should be "did not receive any email ... "

oliverroick · 2017-07-20T16:41:23Z

cadasta/accounts/tests/test_forms.py

+
+        assert User.objects.count() == 0
+
+    def test_signup_with_phone_only(self):


This case replicates test_password_contains_blank_email, doesn't it?

It does replicate. But both test test_password_contains_blank_email and test_signup_with_phone_only, represented different test cases:- 1) user signs up with phone only and 2) password does not throw error when blank email is provided.
Same thing goes for test_signup_with_email_only and test_password_contains_blank_phone.
So I kept them separate. Should I write single test instead?

Ok, it's not 100% obvious from the test itself. Maybe remove assert User.objects.count() == 0 from test_password_contains_blank_email because it's a bit misleading; you never attempt to create the user so of course there are no User instances.

oliverroick · 2017-07-20T16:41:34Z

cadasta/accounts/tests/test_forms.py

+        assert user.email is None
+        assert user.check_password('221B@bakerstreet') is True
+
+    def test_signup_with_email_only(self):


This case replicates test_password_contains_blank_phone, doesn't it?

oliverroick · 2017-07-20T17:04:46Z

cadasta/accounts/views/default.py

+
+    def get_user(self):
+        phone = self.request.session["unverified_phone"]
+        device = VerificationDevice.objects.get(unverified_phone=phone)


This breaks when I sign up with an email address only and I land on the page for the first time. get_user should only be called when a phone number is provided or at least you need to wrap this with try..except.

oliverroick · 2017-07-20T17:08:11Z

cadasta/accounts/views/default.py

+
+    def post(self, *args, **kwargs):
+        form = self.form_class(self.request.POST)
+        user = self.get_user()


The database calls are a bit inefficient here. In get_user you query the VerificationDevice to get the user and then you query the database again to get VerificationDevice for the user. So you end up querying the same information from the database twice.

You could make this more efficient by changing get_user to get_device, which returns a verification device instance, and the access the user through device.user where ever you need it.

clash99 · 2017-07-20T18:28:27Z

I would recommend simplifying the ui to make it more obvious that the user can sign up with either an email OR a phone number. It seems the default should still be email so that should always be shown and perhaps a link with something like "Register with my phone number" would hide email address and would show phone number field.

Something like this:

I'm sure you have other ideas as well so let me know if you want to talk ui. Looks good!

oliverroick · 2017-07-24T14:10:14Z

cadasta/accounts/views/default.py

@@ -140,30 +138,23 @@ def get_context_data(self, **kwargs):
        context['phone'] = user.phone
        return context

-    def post(self, *args, **kwargs):
-        form = self.form_class(self.request.POST)
+    def form_valid(self, form):
        user = self.get_user()
        device = user.verificationdevice_set.get()


Are we always certain that there is only one VerificationDevice instance per user. I believe this will break if there is more than one.

oliverroick · 2017-07-24T14:12:01Z

cadasta/accounts/models.py

@@ -180,10 +180,10 @@ def generate_challenge(self):

        return token

-    def verify_token(self, token):
+    def verify_token(self, token, tolerance=0):


What does tolerance represent? Are we ever going to use anything other than 0? If not this should be a constant in line 186.

tolerance checks a token against previously generated tokens.
Consider this,

tolerance = 1, token at time t0 = 123456, and token at time t1 = 234567

Consider a user entered token at time t1, token entered was 123456, so an error will be generated which says that "Token has expired and you need a new token."

If tolerance=2, token will be verified against previously generated 2 tokens.
I hope that makes sense.

oliverroick · 2017-07-24T14:25:35Z

cadasta/accounts/tests/test_forms.py

+
+        assert User.objects.count() == 0
+
+    def test_signup_with_phone_only(self):


Ok, it's not 100% obvious from the test itself. Maybe remove assert User.objects.count() == 0 from test_password_contains_blank_email because it's a bit misleading; you never attempt to create the user so of course there are no User instances.

oliverroick

Functionality looks good now, just a few smaller still...

oliverroick · 2017-07-24T14:36:37Z

cadasta/templates/accounts/account_verification.html

 </div>
-{% endblock %}
+{% endblock %}


Empty line missing at the end of the file.

oliverroick

Almost there, there are still two small problems you haven't addressed yet.

oliverroick · 2017-07-26T13:28:06Z

cadasta/accounts/views/default.py

+    def form_valid(self, form):
+        token = form.cleaned_data.get('token')
+        user = self.get_user()
+        device = user.verificationdevice_set.get()


This line will throw an exception when there is more the one VerificationDevice instance for the user.

A user will always have only one VerificationDevice instance, so it will not throw the error. I have tried to make VerificationDevice function in a similar way to EmailAddress.

If that is always the case, then you should make user of VerificationDevice a OneToOneField instead of ForeignKey to enforce that assumption on the database level.

Currently, it is possible to add another VerificationDevice instance to the set. You can never know how VerificatioDevice instance will be created, so there might be a possibility that you end up with more the one VerificationDevice instance at some point.

I had implemented OneToOneField before, but I don't know for some reason I did not go ahead with that. But if a user will only have one VerificationDevice object linked to it, then setting the OneToOneField sounds logical and safe way to go. I'll make the required changes there.

oliverroick · 2017-07-26T13:29:13Z

cadasta/accounts/tests/test_forms.py

@@ -693,3 +911,20 @@ def test_email_sent_reset(self):

        form = forms.ResetPasswordForm(data)
        assert form.is_valid() is True
+
+
+class PhoneVerificationFormTest(UserTestCase, TestCase):


Should there be a test for when an expired token is provided?

I have added that test in test_views_default.py, here.

What I was asking for is an isolated test for the form. If the view test fails it could be anywhere, the form, the model, you name it. If the test for the form fails, we know the form is broken.

Solve migration merge conflicts, and create a new migration file for phone and verification device Pass all tests after rebasing Make all tests pass

* Phone registration page, and authentication backend * Upgrade django-skivvy to 0.1.8 * 100% test coverage * Minor change to phone validator * Changes to default.py and tests as addressed by Oliver * Make all addressed changes * allow user to authenticate even if the account status is inactive * add changes addressed to PR * Minor changes to the code * Make VerificationDevice OneToOneField and make corresponding changes Solve migration merge conflicts, and create a new migration file for phone and verification device Pass all tests after rebasing Make all tests pass * change logger from info to debug

* VerificationDevice model and Removal of 48hr email verification period (#1606) * Registration with Phone number (#1662) * Add phone to User Profile (#1698) * Add Ansible provisioning for Twilio * Allow user to login with phone and add Resend Token Page (#1708) * Reset Password with Phone (#1717) * Twilio Integration and More update notification (#1719) * Add API endpoint for phone verification(2) (#1748)

valaparthvi requested a review from oliverroick July 19, 2017 18:03

amplifi added PR: migration PR: needs review PR: needs testing and removed PR: migration labels Jul 19, 2017

valaparthvi force-pushed the additional-login-options branch from 5bf21df to 12640f5 Compare July 20, 2017 10:27

valaparthvi force-pushed the phone/Registration branch from eebd2b0 to acc664b Compare July 20, 2017 11:26

oliverroick requested changes Jul 20, 2017

View reviewed changes

oliverroick reviewed Jul 24, 2017

View reviewed changes

oliverroick requested changes Jul 24, 2017

View reviewed changes

valaparthvi force-pushed the additional-login-options branch from 12640f5 to 9a51a27 Compare July 24, 2017 18:00

valaparthvi force-pushed the phone/Registration branch 2 times, most recently from 014952e to 888a5d4 Compare July 25, 2017 12:22

oliverroick requested changes Jul 26, 2017

View reviewed changes

valaparthvi force-pushed the additional-login-options branch from 9a51a27 to 96095e5 Compare July 27, 2017 09:52

valaparthvi added 9 commits July 27, 2017 16:13

Phone registration page, and authentication backend

05e070d

Upgrade django-skivvy to 0.1.8

5788739

100% test coverage

d27c3c2

Minor change to phone validator

9bc0d2c

Changes to default.py and tests as addressed by Oliver

5f68e9e

Make all addressed changes

54ea66f

allow user to authenticate even if the account status is inactive

36f0bbc

add changes addressed to PR

ab769ec

Minor changes to the code

d176946

valaparthvi force-pushed the phone/Registration branch from 888a5d4 to 03855c6 Compare July 27, 2017 11:01

Make VerificationDevice OneToOneField and make corresponding changes

2302184

Solve migration merge conflicts, and create a new migration file for phone and verification device Pass all tests after rebasing Make all tests pass

valaparthvi force-pushed the phone/Registration branch from 03855c6 to 2302184 Compare July 27, 2017 11:20

change logger from info to debug

c764c46

oliverroick approved these changes Jul 28, 2017

View reviewed changes

valaparthvi deleted the phone/Registration branch September 21, 2017 05:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Registration with Phone number #1662

Registration with Phone number #1662

valaparthvi commented Jul 19, 2017 •

edited

Loading

oliverroick left a comment

oliverroick Jul 20, 2017

oliverroick Jul 20, 2017

valaparthvi Jul 21, 2017 •

edited

Loading

valaparthvi Jul 23, 2017 •

edited

Loading

oliverroick Jul 24, 2017

oliverroick Jul 20, 2017

oliverroick Jul 20, 2017

valaparthvi Jul 21, 2017 •

edited

Loading

oliverroick Jul 20, 2017

oliverroick Jul 20, 2017

oliverroick Jul 20, 2017

valaparthvi Jul 21, 2017 •

edited

Loading

oliverroick Jul 24, 2017

oliverroick Jul 20, 2017

oliverroick Jul 20, 2017

oliverroick Jul 20, 2017

clash99 commented Jul 20, 2017 •

edited

Loading

oliverroick Jul 24, 2017

oliverroick Jul 24, 2017

valaparthvi Jul 24, 2017 •

edited

Loading

oliverroick Jul 24, 2017

oliverroick left a comment

oliverroick Jul 24, 2017

oliverroick left a comment

oliverroick Jul 26, 2017

valaparthvi Jul 26, 2017 •

edited

Loading

oliverroick Jul 26, 2017

valaparthvi Jul 26, 2017 •

edited

Loading

oliverroick Jul 26, 2017

valaparthvi Jul 26, 2017

oliverroick Jul 26, 2017



		class AuthenticationBackend(Backend):
		def _authenticate_by_username(self, **credentials):


		assert User.objects.count() == 0

		def test_signup_with_phone_only(self):

Registration with Phone number #1662

Registration with Phone number #1662

Conversation

valaparthvi commented Jul 19, 2017 • edited Loading

Proposed changes in this pull request

Changes

When should this PR be merged

Risks

Follow-up actions

Checklist (for reviewing)

General

Functionality

Code

Tests

Security

Documentation

oliverroick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valaparthvi Jul 21, 2017 • edited Loading

Choose a reason for hiding this comment

valaparthvi Jul 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valaparthvi Jul 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valaparthvi Jul 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clash99 commented Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valaparthvi Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliverroick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliverroick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valaparthvi Jul 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valaparthvi Jul 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valaparthvi commented Jul 19, 2017 •

edited

Loading

valaparthvi Jul 21, 2017 •

edited

Loading

valaparthvi Jul 23, 2017 •

edited

Loading

valaparthvi Jul 21, 2017 •

edited

Loading

valaparthvi Jul 21, 2017 •

edited

Loading

clash99 commented Jul 20, 2017 •

edited

Loading

valaparthvi Jul 24, 2017 •

edited

Loading

valaparthvi Jul 26, 2017 •

edited

Loading

valaparthvi Jul 26, 2017 •

edited

Loading