PhoneNumberPrefixWidget improvement for regions sharing same country prefix #493
Conversation
|
Early commits look good, could you move them into separate PRs so we can keep this PR focused on the change for regions sharing the same country prefix? |
|
I've extracted whatever was not directly and strictly linked to this fix (which wasn't that much) and I'm going to use it in separate PR. I've also reorganized commits to show as clearly as I could my journey through this case. Regards |
tests/test_widgets.py
Outdated
| } | ||
| value = widget.value_from_datadict(data, {}, "field") | ||
| self.assertEqual(value.as_e164, "+1123") | ||
| self.assertTrue(hasattr(value, "_region")) |
There was a problem hiding this comment.
| self.assertTrue(hasattr(value, "_region")) | |
| self.assertEqual(value._region, "US") |
There was a problem hiding this comment.
Thanks for the rework, this is definitely close to landing. It’s a valuable contribution and the project will definitely be improved by it, thanks a lot for taking the time to upstream it!
The discussion on the patch is only meant to ensure its maintainability, trying hard to avoid mistakes. Your work is definitely appreciated and most welcome 👍.
Remaining open points:
- How can an
strvalue be passed todecompress()? - Please write the new tests at a more functional level, that better explain the use case than a unit test calling into Django multi widget methods. It’s also much easier to call Django multi widget method in a non-representative than when manipulating the form.
Edit: Removed the str guard from decompress and corresponding test.
phonenumber_field/widgets.py
Outdated
| if isinstance(value, str): | ||
| value = to_python(value, region=region) |
There was a problem hiding this comment.
I’m failing to see why this is needed?
The only test failing when this code is removed is test_decompress_valid_str, which does not represent how an str can get to decompress. Can you write a functional test that calls decompress with a non-empty str? I would probably replace test_decompress_valid_str with that functional test.
|
Many thanks @amateja for this quality PR. It seems to fix exactly the issue I am having (Guernsey and UK sharing prefixes). If possible, could you please respond to @francoisfreitag 's issues so that this might be merged? If @amateja is too busy. Can we consider either pushing to this PR (not sure if that's possible) or re-submitting one with code you are 100% okay with @francoisfreitag ? My aim here is not to show anyone disrespect in any way, just to improve the library. Otherwise I will have to apply @amateja 's fix directly to my local copy. |
|
Ideally, @amateja can take this work to the finish line. His contribution is great, and I’m eager to integrate it. The question blocking me from merging currently is “How can an Of course, if others interested in this PR understand the reason for that guard, please share it. |
|
Without response, I have gone ahead and applied the last cleanups to this patch. Many thanks to @amateja, it’s a great contribution to this project! 🌟 |
francoisfreitag
force-pushed
the
widget_fix
branch
from
6ad0768
to
99aef46
Compare
Each choice should be indexed with a unique value, use the region ISO code over the national prefix. Fixes stefanfoulis#133 Fixes stefanfoulis#251 Fixes stefanfoulis#489
francoisfreitag
force-pushed
the
widget_fix
branch
from
99aef46
to
38aa51c
Compare
As promised in #251 this PR improves
PhoneNumberPrefixWidgethandling for regions sharing the same country prefix. Current behavior does not follow requirements forSelect.Choices. Choices should be indexed by unique values and this requirement was not met for 12 country prefixes.Fixes #133
Fixes #251
Fixes #489