Add phone to User Profile #1698

valaparthvi · 2017-08-03T18:30:28Z

With this PR, changes have been introduced to Edit Profile Page. Once user changes their phone number, a token will be sent to the user, and user will be redirected to Resend Token Page. Once the new phone has been verified, user account will be updated with the new phone.

Proposed changes in this pull request

Changes to ProfileForm and add tests.
Changes to UserSerializer and add tests.
Add language and measurement to UserFactory.
Make changes to 'accounts/views/default.pyandaccounts/views/api.py` and add tests.
Make changes to templates/accounts/profile.html.

When should this PR be merged

[Please describe any preconditions that need to be addressed before we
can merge this pull request.]

Once all tests and changes are approved and reviewers are satisfied with it

Risks

None

Follow-up actions

None

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

The code looks mostly ok, there are some tests missing (I think) and the code can be improved here and there.

The functionality is ok; when you're finished with all your other tasks, we should verify that all of the emails and texts are send as expected when users change their info or passwords I believe some are missing, but I haven't looked in detail. Let's not bother with this now and make this a follow-up task.

oliverroick · 2017-08-09T09:10:55Z

cadasta/templates/core/identifier.html

@@ -0,0 +1 @@
+<span class="site-name">local</span>


This file is listed in .gitignore and shouldn't be committed.

I don't know for some reason this file keeps deleting itself, and then I have to re-add it every time.

Run vagrant up --provision when it's gone. But this file must not be committed.

This file is still committed. Please remove it from your branch

oliverroick · 2017-08-09T09:35:37Z

cadasta/accounts/forms.py

@@ -143,25 +158,56 @@ def clean_password(self):
            raise forms.ValidationError(
                _("Please provide the correct password for your account."))

+    def clean_phone(self):
+        phone = self.data.get('phone')
+        if self.instance.phone != phone:


I think this condition is not needed. self.instance.phone != phone always becomes True unless instance.phone is already None. So there's no harm in always setting phone = None if phone is empty.

Got it, I was trying to write code similar to what was defined in clean_email. But as you said, it is not required to check both the phones, although it would've been required if unique was not set True for phone in the User model. I think it's safe to remove self.instance.email != email as well.

Yes, I agree; self.instance.email != email can be removed as well

oliverroick · 2017-08-09T09:36:25Z

cadasta/accounts/forms.py

    def clean_email(self):
        email = self.data.get('email')
        if self.instance.email != email:
-            if User.objects.filter(email=email).exists():


Why is this removed?

I removed that because, we have set unique=True for email in User model, and if a user with that email address already exists, that error will be automatically raised by Django.

oliverroick · 2017-08-09T09:57:58Z

cadasta/accounts/tests/test_forms.py

@@ -673,6 +696,338 @@ def test_sanitize(self):
        assert form.is_valid() is False
        assert SANITIZE_ERROR in form.errors.get('username')

+    def test_update_phone_only(self):


Two test cases are missing here, which are defined in the specification:

User changes phone number and removes verified email address

User changes email address and removes verified phone number

Ah, I did write those test cases, but I remember asking you if we should allow a user to change email or phone even if they have unverified phone or email linked to their account, you said it was okay, and so I deleted those test cases. I'll add them again, and I think it's better to write more generalized test cases,

User changes phone number and removes email address

User changes email address and removes phone number

What I meant is when a user changes their phone and removes the email address (both are verified, not unverified). Then you should send a text to the new phone number

I think it's better if I cover those test cases once I've implemented Twilio. Currently, no message is sent when user changes or removes phone/email.

oliverroick · 2017-08-09T10:31:55Z

cadasta/accounts/tests/test_forms.py

+        assert '[email protected]' in mail.outbox[1].to
+
+        with pytest.raises(EmailAddress.DoesNotExist):
+            EmailAddress.objects.get(email="[email protected]")


write assert EmailAddress.objects.filter(email="[email protected]").exists() is False

oliverroick · 2017-08-09T10:33:55Z

cadasta/accounts/tests/test_forms.py

+        }
+        form = forms.ProfileForm(data=data, instance=user)
+        assert form.is_valid() is False
+        assert (phone_format in form.errors.get('phone'))


Can you add an assertion here that the correct error message is in form.errors.get('phone')?

Did I not do that already? Um, I don't understand what you're trying to suggest. Should I assert against the actual message instead of just writing phone_format?

Ok yes, I missed that

oliverroick · 2017-08-09T11:02:37Z

cadasta/accounts/serializers.py

+                    _("You cannot leave both phone and email empty."))
+        return data
+
+    def validate_email(self, email):


I think, you write this method way shorter; something along the lines of:

if (instance and email != instance.email and self.context['request'].user != instance): raise serializers.ValidationError('Cannot update email') if instance and instance.email == email: return email if email: email = email.casefold() if EmailAddress.objects.filter(email=email): raise serializers.ValidationError( _("User with this Email address already exists.")) else: email = None return email

Awesome! This is the code I was talking about when I said that I need to make the code I write more efficient and DRY.

oliverroick · 2017-08-09T11:03:24Z

cadasta/accounts/serializers.py

+                email = None
+        return email
+
+    def validate_phone(self, phone):


This can be shortened as well, similar to validate_email

oliverroick · 2017-08-09T11:04:40Z

cadasta/accounts/serializers.py

+            if (instance.email != email):
+                if email:
+                    email = email.casefold()
+                    if EmailAddress.objects.filter(email=email):


Here a comment would help to point out we're checking that no unverified emails with that address exist.

oliverroick · 2017-08-09T11:20:11Z

cadasta/accounts/tests/test_views_api.py

+        assert self.user.phone == '+12345678990'
+        assert self.user.phone_verified is True
+
+    def test_keep_phone_number(self):


I don't fully understand the purpose of this test. especially, why you are creating a VerificationDevice instance and why you check that it still exists after the update.

Um my bad, I must've added VerificationDevice instance to make sure the code I wrote worked fine, I was not very confident about it. I'll remove that.
And this test makes sure that a verification token is not sent to the user if they've not changed their phone number. It's similar to test_keep_email_address.

And this test makes sure that a verification token is not sent to the user if they've not changed their phone number.

But there's no assertion to verify this. I assume you need to check that no VerificationDevice instance exists.

If user is not redirected to the verification page, it would mean that the token was not sent. assert response.status_code ==200 makes sure of that. I don't think checking whether VerificationDevice instance exists would be of any help. I'm not sure if I've confirmed this with you, but should I delete VerificationDevice instance once the phone number has been verified?

How does a HTTP status code verify that you haven't sent the token? If you update the user profile and the user keeps their email address you want to verify that no token is sent. Checking for the status code does verify that how would it?

Also deleting the VerificationDevice instance when the number is confirmed is unrelated. Just add a line to check if that there's no VerificationDevice instance.

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

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 Phone registration page, and authentication backend

changes corresponding to setting user as OneToOneField in VerificationDevice add phone_format, instead of message Remove unnecessary lines of code and add required tests Check if new email/phone updated by user exists in EmailAddress/VerificationDevice, make sure user is redirected to AccountVerification Page only when they update phone, add tests for the same. delete identifier.html change to tests

oliverroick

This looks ok, go ahead an merge it.

* 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 * 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 Phone registration page, and authentication backend * Edit Profile Page changes corresponding to setting user as OneToOneField in VerificationDevice add phone_format, instead of message Remove unnecessary lines of code and add required tests Check if new email/phone updated by user exists in EmailAddress/VerificationDevice, make sure user is redirected to AccountVerification Page only when they update phone, add tests for the same. delete identifier.html change to tests

* 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 August 3, 2017 18:30

valaparthvi force-pushed the additional-login-options branch from 21001c8 to 309b69c Compare August 4, 2017 10:39

valaparthvi force-pushed the phone/EditProfile branch from eb7c8f9 to ac2bd86 Compare August 4, 2017 10:41

valaparthvi added the PR: needs review label Aug 8, 2017

oliverroick requested changes Aug 9, 2017

View reviewed changes

valaparthvi force-pushed the additional-login-options branch from 309b69c to bbf4843 Compare August 10, 2017 15:50

valaparthvi added 3 commits August 14, 2017 18:43

Phone registration page, and authentication backend

f140375

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

Phone registration page, and authentication backend

32e92f4

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 Phone registration page, and authentication backend

valaparthvi force-pushed the phone/EditProfile branch from 14bac19 to 064153b Compare August 14, 2017 13:15

oliverroick approved these changes Aug 15, 2017

View reviewed changes

valaparthvi merged commit 9f327fe into additional-login-options Aug 15, 2017

valaparthvi removed the PR: needs review label Aug 15, 2017

valaparthvi deleted the phone/EditProfile branch September 21, 2017 05:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add phone to User Profile #1698

Add phone to User Profile #1698

valaparthvi commented Aug 3, 2017

oliverroick left a comment

oliverroick Aug 9, 2017

valaparthvi Aug 9, 2017

oliverroick Aug 14, 2017

oliverroick Aug 14, 2017

oliverroick Aug 9, 2017

valaparthvi Aug 9, 2017 •

edited

Loading

oliverroick Aug 14, 2017

oliverroick Aug 9, 2017

valaparthvi Aug 9, 2017

oliverroick Aug 14, 2017

oliverroick Aug 9, 2017

valaparthvi Aug 9, 2017 •

edited

Loading

oliverroick Aug 14, 2017

valaparthvi Aug 14, 2017

oliverroick Aug 9, 2017

oliverroick Aug 9, 2017

valaparthvi Aug 9, 2017

oliverroick Aug 14, 2017

oliverroick Aug 9, 2017

valaparthvi Aug 9, 2017

oliverroick Aug 9, 2017

oliverroick Aug 9, 2017

oliverroick Aug 9, 2017

valaparthvi Aug 9, 2017 •

edited

Loading

oliverroick Aug 14, 2017

valaparthvi Aug 14, 2017 •

edited

Loading

oliverroick Aug 14, 2017

oliverroick left a comment

Add phone to User Profile #1698

Add phone to User Profile #1698

Conversation

valaparthvi commented Aug 3, 2017

Proposed changes in this pull request

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valaparthvi Aug 9, 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

Choose a reason for hiding this comment

valaparthvi Aug 9, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valaparthvi Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valaparthvi Aug 14, 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

valaparthvi Aug 9, 2017 •

edited

Loading

valaparthvi Aug 9, 2017 •

edited

Loading

valaparthvi Aug 9, 2017 •

edited

Loading

valaparthvi Aug 14, 2017 •

edited

Loading