Update phone formatting on AJAX response

[pjsier]

Fixes #187 by updating the row with the JSON response data, including the re-formatted phone number.

This doesn't fix delivering the correct error message though. It looks like the issue is in the exception handling for the phone number input field. It catches any NumberParseException (which it sounds like we want), changes that to the raw value, and then passes it to the generic PhoneNumberField which gives the standard error message.

Do we want to throw the exception from phonenumbers if a phone number fails the parsing rather than passing it to the PhoneNumberField? And if that makes sense, should we use their existing error messages? They seem comprehensive, but it might be worth condensing some of them (i.e. there are two versions of phone numbers being too short that could just return "Phone number is too short").

Let me know what makes the most sense and I can either add it to this PR or make a separate one

[pjsier]

The phone number error message issue was different than I originally thought, but updated this PR to address it. PhoneNumberField handles any exceptions from NumberParseException, but there's an additional is_valid check in the field that was failing in the modified PhoneNumber class, and it looks like that's where the real reasons for numbers failing are coming from.

The more detailed reasons for failing the validity check are in ValidationResult of the phonenumbers library, so I added those to the field's error messages and call them based on the is_possible_number_with_reason method that returns the reason instead of a boolean.

Setting the PHONENUMBER_DEFAULT_REGION and PHONENUMBER_DEFAULT_FORMAT seem to cover adding the + to the numbers while defaulting to US, so other than the error messages this looks more like the original to_python method of PhoneNumberField. Had to go down the rabbit hole a bit on this one, so let me know if any of that is unclear!

[cecilia-donnelly]

Thanks @pjsier! I'm excited that you went beyond the immediate thing to fixing the bigger issue with the app returning incorrect errors.

When I just tried to run the app from your branch, I got an error:

Unhandled exception in thread started by <function check_errors.<locals>.wrapper at 0x7f52595b49d8>
Traceback (most recent call last):
  File "streetcrm/lib/python3.5/site-packages/django/utils/autoreload.py", line 229, in wrapper
    fn(*args, **kwargs)
  File "streetcrm/lib/python3.5/site-packages/django/core/management/commands/runserver.py", line 107, in inner_run
    autoreload.raise_last_exception()
  File "streetcrm/lib/python3.5/site-packages/django/utils/autoreload.py", line 252, in raise_last_exception
    six.reraise(*_exception)
  File "streetcrm/lib/python3.5/site-packages/django/utils/six.py", line 685, in reraise
    raise value.with_traceback(tb)
  File "streetcrm/lib/python3.5/site-packages/django/utils/autoreload.py", line 229, in wrapper
    fn(*args, **kwargs)
  File "streetcrm/lib/python3.5/site-packages/django/__init__.py", line 18, in setup
    apps.populate(settings.INSTALLED_APPS)
  File "streetcrm/lib/python3.5/site-packages/django/apps/registry.py", line 108, in populate
    app_config.import_models(all_models)
  File "streetcrm/lib/python3.5/site-packages/django/apps/config.py", line 198, in import_models
    self.models_module = import_module(models_module_name)
  File "streetcrm/lib/python3.5/importlib/__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 986, in _gcd_import
  File "<frozen importlib._bootstrap>", line 969, in _find_and_load
  File "<frozen importlib._bootstrap>", line 958, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 673, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 697, in exec_module
  File "<frozen importlib._bootstrap>", line 222, in _call_with_frames_removed
  File "streetcrm/streetcrm/models.py", line 24, in <module>
    from streetcrm import mixins, modelfields, managers
  File "streetcrm/streetcrm/modelfields.py", line 20, in <module>
    from streetcrm import formfields
  File "streetcrm/streetcrm/formfields.py", line 31, in <module>
    class LocalPhoneNumberField(formfields.PhoneNumberField):
  File "streetcrm/streetcrm/formfields.py", line 39, in LocalPhoneNumberField
    ValidationResult.INVALID_LENGTH: _("Phone number length is invalid."),
AttributeError: type object 'ValidationResult' has no attribute 'INVALID_LENGTH'

Did you have to do something to add the INVALID_LENGTH attribute? I was able to run everything fine after I commented out that line. Maybe it's a version problem? I'm on Python 3.5.3.

[pjsier]

Thanks for the review, @cecilia-donnelly! I just tried it with 3.5.2 and it worked, but I think it might actually be your version of the django-phonenumber-field library. It looks like INVALID_LENGTH was only added about 6 months ago in phonenumbers, so upgrading that package might fix it?

[cecilia-donnelly]

Okay, thanks, @pjsier! I have to debug some issues with my virtualenv but once I get that straightened out I'll upgrade and test again. :)

[cecilia-donnelly]

Okay, progress: I can now get the correct errors for invalid phone numbers! Hurray!

However, if I enter a valid phone number for an attendee row that did not have a phone number before, the new number is not saved on tab-away. I see the following in the logs:

DEBUG: field nonce does not exist in this object
[08/Aug/2017 17:28:52] "PUT /api/participants/33/ HTTP/1.1" 500 15052

So, this is a remnant of a very tricky bug from a while ago. I suspect that somewhere in this change we started using a version of the object that doesn't include the nonce, which is why I now can't save new phone numbers.

Have you run into that, @pjsier? Any ideas?

[cecilia-donnelly]

Also, I had to add the following lines to requirements.txt:

psycopg2
phonenumbers

Did you install those? Can you add that to this PR? I also ran sudo apt-get install python-psycopg2, though I'm not entirely sure if it was necessary. If it is needed, can you add that to the dependencies in INSTALL.md?

[pjsier]

Hmm, I didn't run into that, but I might have just been editing existing phone numbers because I just ran into it now trying it out locally. I'll have to take a closer look at your notes on that bug, because it looks like you're right, but I'm not quite sure how to fix it yet.

For the dependencies, I think INSTALL.md lists psycopg2 as optional, but it should probably be required. I didn't have to add phonenumbers though, but I can add that as well

[pjsier]

It looks like the error was coming from the fact that I was returning an exception instead of None if the phone number returned was empty. Updated that and it looks like it's working for both creating new numbers and updating older ones.

Also added psycopg2 and phonenumbers to requirements.txt, but I think psycopg2 should work without apt-get

[cecilia-donnelly]

Perfect, that fixed it for me!