Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically transform phone numbers to international standard #2791

Merged
merged 1 commit into from
May 27, 2024

Conversation

charludo
Copy link
Contributor

@charludo charludo commented May 8, 2024

Short description

Automatically transform phone numbers to international standard.

Proposed changes

  • prepend phone number link targets with +<country code>
  • prettyprint the internationalized phone numbers for better readability

Side effects

  • none

Resolved issues

Fixes: #908


Pull Request Review Guidelines

@charludo charludo requested a review from a team as a code owner May 8, 2024 13:00
@charludo charludo linked an issue May 8, 2024 that may be closed by this pull request
@charludo
Copy link
Contributor Author

charludo commented May 8, 2024

(Sorry, I mis-clicked when filtering for issues, it's not Q3 yet... 🤦🏼‍♀️ By the time I realized it was already done.)

Copy link

codeclimate bot commented May 8, 2024

Code Climate has analyzed commit 07e12c9 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 81.7% (-0.5% change).

View more on Code Climate.

Copy link
Member

@MizukiTemma MizukiTemma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 😸

(Sorry, I mis-clicked when filtering for issues, it's not Q3 yet... 🤦🏼‍♀️ By the time I realized it was already done.)

🙈 Anyway it's an improvement 😉

Copy link
Contributor

@JoeyStk JoeyStk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for this PR. It looks very good already!

While testing this, I noticed that telephone numbers such as 0123 456789 or 0123-456789 aren't caught by this PR (yet). (I found in our test system that municipalities use both cases and even a combination 0123 4567-89 😅). Do you think it's possible to include them too or would that be too difficult/too fuzzy given the empty space/dash between?

Secondly, although this is Javascript code I thought that we might still include a (pytest) test for this? My test idea would be to do a POST request to the page form entering a telephone number in a wrong format and then checking in a database request that the format is right in the database. Do you think that would work reliably and test what it should be testing? 🤔

@charludo charludo force-pushed the enhancement/internationalize-phone-numbers branch from abfef71 to 4c437b6 Compare May 10, 2024 08:59
@charludo
Copy link
Contributor Author

charludo commented May 10, 2024

While testing this, I noticed that telephone numbers such as 0123 456789 or 0123-456789 aren't caught by this PR (yet). (I found in our test system that municipalities use both cases and even a combination 0123 4567-89 😅). Do you think it's possible to include them too or would that be too difficult/too fuzzy given the empty space/dash between?

Yeah, I think this is a good idea. An OK compromise would maybe be to match all strings that

  • consist only of numbers, space, dash
  • start with 0 or +
  • have 6-10 digits in them

right?

Only trouble is it would for example also match + ---- 1 ---- 2 ----- 3 ----- 4 - 55 though I am having trouble finding a usecase for this 😂

Edit: Oh, I just realized that this would also prematurely add a link after, for example, 0174 123 even though the full intended number might have been 0174 123 456, since pressing space triggers the conversion if a match was found. So space is probably a no-go. Dash is fine though.

Secondly, although this is Javascript code I thought that we might still include a (pytest) test for this? My test idea would be to do a POST request to the page form entering a telephone number in a wrong format and then checking in a database request that the format is right in the database. Do you think that would work reliably and test what it should be testing? 🤔

Not so easy, unfortunately. AFAIK this would not trigger the rewrite, since it doesn't happen when POSTing to the backend, only when pressing space/enter after entering such a number.

@charludo charludo requested a review from JoeyStk May 14, 2024 14:14
@JoeyStk
Copy link
Contributor

JoeyStk commented May 20, 2024

So space is probably a no-go

Okay, I think that's fine. We just need to communicate it to the service-team, so they can think about how to communicate to the municipalities.

Dash is fine though.

Depending on the effort I think that would be good, as some municipalities seem to use the 0123-456789

Not so easy, unfortunately. AFAIK this would not trigger the rewrite, since it doesn't happen when POSTing to the backend, only when pressing space/enter after entering such a number.

Ok. I think I get it now. Thank you for explaining :)

@charludo charludo force-pushed the enhancement/internationalize-phone-numbers branch 2 times, most recently from ad43fd1 to 5cf8616 Compare May 27, 2024 06:34
@charludo
Copy link
Contributor Author

@JoeyStk dash is now included. It would also be an option to only allow dashes after a certain amount of numbers (e.g. if we assume all phone numbers start with either a country code +xx, 00xx, or a "Vorwahl" 0xxxx), but not sure if that wouldn't be too strict or could potentially miss some weird ways to write the numbers.

Copy link
Contributor

@JoeyStk JoeyStk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for the change. I double-checked with @vthejay and the most important cases are now being handled. Apparently it's no problem that space is not supported as they discourage municipalities to do use this format in the workshops already.

I found one small last thing: When I type 0123/456789 and hit enter I get "+49 (0) 123/456789" back. @vthejay and I agreed that this isn't ideal as the "/" should not show in the final string anymore :) Do you think it's possible to remove it? :)

@charludo charludo force-pushed the enhancement/internationalize-phone-numbers branch from 5cf8616 to 07e12c9 Compare May 27, 2024 08:15
@charludo charludo requested a review from JoeyStk May 27, 2024 08:15
Copy link
Contributor

@JoeyStk JoeyStk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@charludo charludo merged commit 19d950e into develop May 27, 2024
5 checks passed
@charludo charludo deleted the enhancement/internationalize-phone-numbers branch May 27, 2024 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatically transfom telephone numbers to international format
3 participants