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

Phone number validation and search #24486

Merged
merged 15 commits into from
Dec 8, 2020

Conversation

nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Dec 1, 2020

  • Requires 3rdparty giggsey/libphonenumber-for-php Add giggsey/libphonenumber-for-php 3rdparty#539
  • Validate that the phone number is actually a phone number
  • Find a decent default region code we can use (admin setting? reverse try language selection?)
  • Store phone numbers in a defined format (Recommended https://en.wikipedia.org/wiki/E.164 supported by the library)
  • Store phone numbers (and other entries?) individually so we can search by them in a meaning full way instead of wildcard searching the json blob in oc_accounts.
  • Add an OCS Api to search again account data and return federation ids
  • Add a unit test for the cleaning of the phone number
  • Add a unit test for the search endpoint
  • Add integration tests for the search endpoint
  • Need a way to retrigger a full sync of the accounts table

Use https://giggsey.com/libphonenumber/ to find valid testing numbers

Search API

  • Endpoint: POST ocs/v2.php/cloud/users/search/by-phone
  • Data (all 3 number formats are valid):
{
	"location": "DE",
	"search": {
		"Unique-Idenifier-For-Contact-1": [
			"0711/25 24 28-90"
		],
		"Unique-Idenifier-For-Contact-2": [
			"+49 711/25 24 28-91",
			"0049 711/25 24 28-92"
		]
	}
}
  • Response (map of provided identifier and cloudId):
{
  "ocs": {
    "meta": {
      "status": "ok",
      "statuscode": 200,
      "message": "OK"
    },
    "data": {
      "Unique-Idenifier-For-Contact-1": "admin@localhost/"
    }
  }
}

@nickvergessen nickvergessen added this to the Nextcloud 21 milestone Dec 1, 2020
@nickvergessen nickvergessen removed the 3. to review Waiting for reviews label Dec 1, 2020
@nickvergessen nickvergessen force-pushed the feature/noid/phone-number-validation branch 2 times, most recently from e25ca41 to 0d63b89 Compare December 2, 2020 16:01
@nickvergessen nickvergessen changed the title Phone number validation Phone number validation and search Dec 2, 2020
@nickvergessen
Copy link
Member Author

As per opening post, the lib to validate/unify phone numbers needs a default region.

Best I have in mind is an admin setup warning about the missing region and until that is set only allow +49 phone numbers and lie to the lib that the default region is EN. Falling back to default language "would also work", but e.g. Austrian numbers would be German as they use the same language, so it's not very sensitive/accurate.

Opinions on this? @MorrisJobke @rullzer @ChristophWurst

@ChristophWurst
Copy link
Member

As per opening post, the lib to validate/unify phone numbers needs a default region.

Would the locale be more accurate than the language? Like if one sets de_AT then you can guess the country. Possible not 100% accurate either but it seems like there is a locale for many countries.

@MorrisJobke
Copy link
Member

Best I have in mind is an admin setup warning about the missing region and until that is set only allow +49 phone numbers and lie to the lib that the default region is EN.

Doesn't sound too bad IMO.

@nickvergessen nickvergessen force-pushed the feature/noid/phone-number-validation branch 2 times, most recently from 0fd3621 to f543a19 Compare December 3, 2020 13:11
@nickvergessen
Copy link
Member Author

Anyone an idea about:

  • Need a way to retrigger a full sync of the accounts table

@nickvergessen nickvergessen added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Dec 3, 2020
@nickvergessen nickvergessen marked this pull request as ready for review December 3, 2020 19:46
@nickvergessen nickvergessen force-pushed the feature/noid/phone-number-validation branch 3 times, most recently from 6b4bac2 to d6e3367 Compare December 4, 2020 09:03
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
…er API directly

Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the feature/noid/phone-number-validation branch from d6e3367 to d0750df Compare December 7, 2020 13:19
@nickvergessen
Copy link
Member Author

Added a repairstep to validate the existing entries. This is now ready to review.

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the feature/noid/phone-number-validation branch from 22576e7 to 354c5ff Compare December 7, 2020 19:36
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen requested review from juliusknorr and removed request for MorrisJobke December 8, 2020 12:11
@faily-bot
Copy link

faily-bot bot commented Dec 8, 2020

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 299: failure

sqlite

Show full log
There were 2 warnings:

1) Test\Files\ViewTest::testRenameFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static

2) Test\Files\ViewTest::testCopyFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static

--

There were 4 failures:

1) Test\Preview\BitmapTest::testGetThumbnail with data set #0 (-30, -74)
Failed asserting that null is not equal to false.

/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105

2) Test\Preview\BitmapTest::testGetThumbnail with data set #1 (48, 95)
Failed asserting that null is not equal to false.

/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105

3) Test\Preview\BitmapTest::testGetThumbnail with data set #2 (-64, 13)
Failed asserting that null is not equal to false.

/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105

4) Test\Preview\BitmapTest::testGetThumbnail with data set #3 (57, -93)
Failed asserting that null is not equal to false.

/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105

mariadb10.4-php7.4

Show full log
There were 2 warnings:

1) Test\Files\ViewTest::testRenameFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static

2) Test\Files\ViewTest::testCopyFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static

--

There were 4 failures:

1) Test\Preview\BitmapTest::testGetThumbnail with data set #0 (-63, -90)
Failed asserting that null is not equal to false.

/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105

2) Test\Preview\BitmapTest::testGetThumbnail with data set #1 (30, 20)
Failed asserting that null is not equal to false.

/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105

3) Test\Preview\BitmapTest::testGetThumbnail with data set #2 (-27, 81)
Failed asserting that null is not equal to false.

/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105

4) Test\Preview\BitmapTest::testGetThumbnail with data set #3 (73, -37)
Failed asserting that null is not equal to false.

/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105

mysql8.0-php7.4

Show full log
There were 2 warnings:

1) Test\Files\ViewTest::testRenameFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static

2) Test\Files\ViewTest::testCopyFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static

--

There were 4 failures:

1) Test\Preview\BitmapTest::testGetThumbnail with data set #0 (-63, -99)
Failed asserting that null is not equal to false.

/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105

2) Test\Preview\BitmapTest::testGetThumbnail with data set #1 (17, 54)
Failed asserting that null is not equal to false.

/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105

3) Test\Preview\BitmapTest::testGetThumbnail with data set #2 (-98, 25)
Failed asserting that null is not equal to false.

/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105

4) Test\Preview\BitmapTest::testGetThumbnail with data set #3 (92, -10)
Failed asserting that null is not equal to false.

/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105

postgres11-php7.4

Show full log
There were 2 warnings:

1) Test\Files\ViewTest::testRenameFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static

2) Test\Files\ViewTest::testCopyFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static

--

There were 4 failures:

1) Test\Preview\BitmapTest::testGetThumbnail with data set #0 (-26, -83)
Failed asserting that null is not equal to false.

/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105

2) Test\Preview\BitmapTest::testGetThumbnail with data set #1 (44, 10)
Failed asserting that null is not equal to false.

/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105

3) Test\Preview\BitmapTest::testGetThumbnail with data set #2 (-92, 80)
Failed asserting that null is not equal to false.

/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105

4) Test\Preview\BitmapTest::testGetThumbnail with data set #3 (52, -72)
Failed asserting that null is not equal to false.

/drone/src/tests/lib/Preview/Provider.php:144
/drone/src/tests/lib/Preview/Provider.php:105

@juliusknorr juliusknorr added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Dec 8, 2020
@nickvergessen nickvergessen merged commit 86a3b7e into master Dec 8, 2020
@nickvergessen nickvergessen deleted the feature/noid/phone-number-validation branch December 8, 2020 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement feature: settings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants