Improve phone handling #1433

akjanik · 2017-12-06T11:42:04Z

I want to merge this change because...

Pull Request Checklist

Privileged views and APIs are guarded by proper permission checks.
All visible strings are translated with proper context.
All data-formatting is locale-aware (dates, numbers, and so on).
Database queries are optimized and the number of queries is constant.
The changes are tested.
The code is documented (docstrings, project documentation).
Python code quality checks pass: pycodestyle, pydocstyle, pylint.
JavaScript code quality checks pass: eslint.

akjanik · 2017-12-06T11:47:04Z

saleor/userprofile/forms.py

@@ -15,13 +16,18 @@ def get_address_form(data, country_code, initial=None, instance=None, **kwargs):
        country_code = country_form.cleaned_data['country']
        preview = country_form.cleaned_data['preview']

+    if data:


What I am struggling right now is to transfer user input to the form, so it's handled properly (by 'properly' I mean to glue phoneprefix and phonenumber and doing a reverse - after providing address data, user can go back and select another address - proper fields should be repopulated. It could be done with a lot of ifs (checking if instance exists, if it has phone attribute, etc), but it seems to be very non-elegant and messy solution.

You'll find it rather challenging to handle this as two separate fields. I think it would be much easier if it was a single field (as it's going to be stored as such) with a MultiWidget.

In fact there is already one provided by the library you're using.

Such a great tip, thank you, solved many issues I had!

akjanik · 2017-12-06T11:49:19Z

saleor/userprofile/i18n.py

+        exclude = ['phone']
+
+    phoneprefix = ChoiceField(choices=phone_prefixes)
+    phone = forms.CharField()


I have tried use this one:
https://github.com/stefanfoulis/django-phonenumber-field/blob/master/phonenumber_field/formfields.py
but with not much success, as it wants "correct" number (with phone country code included), while here we want to separate things.

No, we don't necessarily want to separate them as fields, we want separate widgets.

akjanik · 2017-12-06T11:51:46Z

saleor/userprofile/i18n.py

        super(AddressForm, self).__init__(*args, **kwargs)
+        if self.instance.phone:


Not sure why, but form does not see phone and phoneprefix provided in the way form(instance=instance, dtaa, phone=phone...), without those lines.

This looks like a workaround and not a solution.

akjanik · 2017-12-06T15:18:02Z

saleor/userprofile/i18n.py

@@ -37,6 +48,12 @@
    'zip': pgettext_lazy('Address field', 'ZIP code')}


+class PhonePrefixWidget(PhoneNumberPrefixWidget):


I decided to overwrite this one as there are issues with default widget value:
stefanfoulis/django-phonenumber-field#82
Quick example: choosing "United Kingdom +44 -> continue -> select new address makes field being populated by Guernsey as it has the same phone code, but it's alphabetically before UK.

codecov-io · 2017-12-11T00:51:42Z

Codecov Report

❗ No coverage uploaded for pull request base (master@acdd238). Click here to learn what that means.
The diff coverage is 97.22%.

@@            Coverage Diff            @@
##             master    #1433   +/-   ##
=========================================
  Coverage          ?   83.27%           
=========================================
  Files             ?      130           
  Lines             ?     5882           
  Branches          ?      678           
=========================================
  Hits              ?     4898           
  Misses            ?      821           
  Partials          ?      163

Impacted Files	Coverage Δ
saleor/userprofile/i18n.py	`96.74% <100%> (ø)`
saleor/userprofile/validators.py	`100% <100%> (ø)`
saleor/userprofile/widgets.py	`100% <100%> (ø)`
saleor/userprofile/models.py	`88.31% <100%> (ø)`
saleor/userprofile/forms.py	`65.62% <80%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update acdd238...caba37c. Read the comment docs.

maarcingebala · 2017-12-12T12:18:08Z

@dominik-zeglen Address form requires a few UI improvements:

prefix and phone number should be placed next to each other
prefix and country selection have rounded corners - maybe it's a general problem with select boxes
prefix and phone number fields should be rendered as the last fields in the form

maarcingebala · 2017-12-12T13:00:00Z

saleor/userprofile/i18n.py

+
+
+PhoneNumberField.default_validators = [validate_possible_number]
+PhoneNumberField.to_python = lambda self, value: value


Instead of this kind of patching, we should rather extend the original class and override the methods or fields that we want to customize. This would be also a good place to include relevant docstrings, explaining how the customization is different from the original implementation.

maarcingebala · 2017-12-12T13:08:51Z

saleor/userprofile/i18n.py

@@ -37,6 +50,22 @@
    'zip': pgettext_lazy('Address field', 'ZIP code')}


+class CustomPhonePrefix(PhonePrefixSelect):


It's a good practice to include the widget type in the class name so in this case, CustomPhonePrefixSelect would be a better name. Also, the word "custom" doesn't say much. What makes this widget custom? It would be great if we could reflect the purpose of this class in the name.

maarcingebala · 2017-12-12T13:09:45Z

saleor/userprofile/i18n.py

+class CustomPhonePrefix(PhonePrefixSelect):
+
+    def __init__(self, initial=None):
+        choices = phone_prefixes


I think there's no need to assign phone_prefixes to a new variable here.

maarcingebala · 2017-12-12T13:10:50Z

saleor/userprofile/i18n.py

+        choices = phone_prefixes
+        if initial:
+            self.initial = phone_prefixes[initial]
+


Empty blank lines in simple functions aren't necessary, it doesn't add much to readability.

maarcingebala · 2017-12-12T13:11:19Z

saleor/userprofile/i18n.py

+        super(PhonePrefixSelect, self).__init__(choices=choices)
+
+
+class PhonePrefixWidget(PhoneNumberPrefixWidget):


Missing docstring.

akjanik added the in progress label Dec 6, 2017

akjanik self-assigned this Dec 6, 2017

akjanik requested review from patrys and maarcingebala December 6, 2017 11:42

akjanik commented Dec 6, 2017

View reviewed changes

akjanik added 12 commits December 12, 2017 12:27

WIP: ad phoneprefix and try make it work

b245afe

WIP: mess, but sometimes it works

6577af8

WIP: hacky solution

333cada

WIP: messy version with commented content

54090e1

WIP: some clean up

e129c03

Use PhoneNumber widget for handling split input

cea2516

WIP: add function to relate country code to phone prefix

921dc51

Set default country prefix based on country code

989821d

Fix conditional

73bddd1

Fix setting default phone prefix

2e0b382

Fix PEP8

b2e2ccb

Add test for validate_possible_number

0623a62

akjanik force-pushed the improve_phone_handling branch from a722964 to 0623a62 Compare December 12, 2017 11:27

dominik-zeglen self-assigned this Dec 12, 2017

maarcingebala reviewed Dec 12, 2017

View reviewed changes

akjanik and others added 5 commits December 12, 2017 15:09

Refactor code, add docstrings

334cf8f

Change select input border-radius

dbd1cd7

Add PhonePrefixWidget template

ce57b8e

Stylize PhonePrefixWidget

77cca38

Move PhonePrefixWidget to a form end

81af621

akjanik and others added 5 commits December 13, 2017 11:46

Sort phone prefixes

0187d67

Move custom PhoneNumber fields and widget to separate files

1692ec8

Update docs

549e8d6

Move modelfield and formfield to separate modules

fcc7964

Add migration file

caba37c

maarcingebala approved these changes Dec 13, 2017

View reviewed changes

maarcingebala merged commit d678d39 into master Dec 13, 2017

maarcingebala deleted the improve_phone_handling branch December 13, 2017 13:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve phone handling #1433

Improve phone handling #1433

akjanik commented Dec 6, 2017 •

edited by maarcingebala

akjanik Dec 6, 2017

patrys Dec 6, 2017

akjanik Dec 6, 2017

akjanik Dec 6, 2017

patrys Dec 6, 2017

akjanik Dec 6, 2017

patrys Dec 6, 2017

akjanik Dec 6, 2017

codecov-io commented Dec 11, 2017 •

edited

maarcingebala commented Dec 12, 2017

maarcingebala Dec 12, 2017

maarcingebala Dec 12, 2017

maarcingebala Dec 12, 2017

maarcingebala Dec 12, 2017

maarcingebala Dec 12, 2017

		super(AddressForm, self).__init__(args, *kwargs)
		if self.instance.phone:

		@@ -37,6 +48,12 @@
		'zip': pgettext_lazy('Address field', 'ZIP code')}


		class PhonePrefixWidget(PhoneNumberPrefixWidget):



		PhoneNumberField.default_validators = [validate_possible_number]
		PhoneNumberField.to_python = lambda self, value: value

		@@ -37,6 +50,22 @@
		'zip': pgettext_lazy('Address field', 'ZIP code')}


		class CustomPhonePrefix(PhonePrefixSelect):

		super(PhonePrefixSelect, self).__init__(choices=choices)


		class PhonePrefixWidget(PhoneNumberPrefixWidget):

Improve phone handling #1433

Improve phone handling #1433

Conversation

akjanik commented Dec 6, 2017 • edited by maarcingebala

Pull Request Checklist

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

codecov-io commented Dec 11, 2017 • edited

Codecov Report

maarcingebala commented Dec 12, 2017

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

akjanik commented Dec 6, 2017 •

edited by maarcingebala

codecov-io commented Dec 11, 2017 •

edited