From b29f5ce7f6a26e6fc1bab706fd6827a914e31748 Mon Sep 17 00:00:00 2001 From: Bjoern Lange Date: Tue, 8 Oct 2019 11:42:59 +0200 Subject: [PATCH] OXAP-179 Added street parsing without the config of a country iso code - Fixed the wrong type hint for the parsing return - Refactored the parsing unit test to a data provider and fixture - Removed unused configuration - Made the switches for the relevant address parsing more speaking Signed-off-by: Bjoern Lange --- .../bestitamazonpay4oxidaddressutil.php | 160 +++++++++++-- .../admin/de/bestitamazonpay4oxid_lang.php | 1 - .../admin/en/bestitamazonpay4oxid_lang.php | 1 - metadata.php | 7 - .../bestitAmazonPay4OxidAdressUtilTest.php | 214 ++++++++++-------- 5 files changed, 268 insertions(+), 115 deletions(-) diff --git a/application/models/bestitamazonpay4oxidaddressutil.php b/application/models/bestitamazonpay4oxidaddressutil.php index e7eb335..038fac1 100644 --- a/application/models/bestitamazonpay4oxidaddressutil.php +++ b/application/models/bestitamazonpay4oxidaddressutil.php @@ -7,28 +7,127 @@ */ class bestitAmazonPay4OxidAddressUtil extends bestitAmazonPay4OxidContainer { + /** + * Return the parsing which returns more pattern hits and contains a longer street name. + * + * @param array $followingNumberMatches + * @param array $leadingNumberMatches + * + * @return array + */ + protected function _handleParsingForInconclusiveMatches(array $followingNumberMatches, array $leadingNumberMatches) + { + $leadingNumberMatchesCount = count($leadingNumberMatches); + $followingNumberMatchesCount = count($followingNumberMatches); + $relevantParsing = array(); + + if ($leadingNumberMatchesCount === $followingNumberMatchesCount) { + if ($this->_isStreetParsingLongerThanNumber($followingNumberMatches)) { + $relevantParsing = $followingNumberMatches; + } + } else { + $isFollowingNumberMatchMoreRelevant = $this->_isFollowingNumberMatchMoreRelevant( + $followingNumberMatchesCount, + $leadingNumberMatchesCount + ); + + $relevantParsing = $isFollowingNumberMatchMoreRelevant ? $followingNumberMatches : $leadingNumberMatches; + } + + return $relevantParsing; + } + + /** + * If both parsings match exactly than the parsing was inconclusive. + * + * @param array|bool $followingNumberMatches + * @param array|bool $leadingNumberMatches + * + * @return bool + */ + protected function _isAdressLineParsingInconclusive($followingNumberMatches, $leadingNumberMatches) + { + return $leadingNumberMatches && $followingNumberMatches; + } + + /** + * Can a following number be found? + * + * @param int $followingNumberMatchesCount + * @param int $leadingNumberMatchesCount + * + * @return bool + */ + protected function _isFollowingNumberMatchMoreRelevant($followingNumberMatchesCount, $leadingNumberMatchesCount) + { + return $followingNumberMatchesCount > $leadingNumberMatchesCount; + } + + /** + * A longer street name than the number suggests, that the parsing was correct. + * + * @param array $followingNumberMatches + * + * @return bool + */ + protected function _isStreetParsingLongerThanNumber(array $followingNumberMatches) + { + return strlen((string) @$followingNumberMatches['Name']) > strlen((string) @$followingNumberMatches['Number']); + } + /** * Returns parsed Street name and Street number in array * - * @param string $sString Full address - * @param string $sIsoCountryCode ISO2 code of country of address + * @param string $addressLine + * @param string $iso2CountryCode * - * @return string + * @return array */ - protected function _parseSingleAddress($sString, $sIsoCountryCode = null) + protected function _parseSingleAddress($addressLine, $iso2CountryCode) { - // Array of iso2 codes of countries that have address format - $aStreetNoStreetCountries = $this->getConfig()->getConfigParam('aAmazonStreetNoStreetCountries'); + $leadingNumberMatches = $this->_searchForLeadingNumberInAddressLine($addressLine); + $followingNumberMatches = $this->_searchForFollowingNumberInAddressLine($addressLine); + + $relevantParsing = $leadingNumberMatches; - if (in_array($sIsoCountryCode, $aStreetNoStreetCountries)) { - // matches streetname/streetnumber like "streetnumber streetname" - preg_match('/\s*(?P\d[^\s]*)*\s*(?P[^\d]*[^\d\s])\s*(?P.*)/', $sString, $aResult); + if ($this->_isAdressLineParsingInconclusive($followingNumberMatches, $leadingNumberMatches)) { + $this->getLogger()->debug( + 'The address parsing was not conclusive.', + array( + 'countyCode' => $iso2CountryCode, + 'originalAddressLine' => $addressLine + ) + ); + + $relevantParsing = $this->_handleParsingForInconclusiveMatches( + $followingNumberMatches, + $leadingNumberMatches + ); } else { - // default: matches streetname/streetnumber like "streetname streetnumber" - preg_match('/\s*(?P[^\d]*[^\d\s])\s*((?P\d[^\s]*)\s*(?P.*))*/', $sString, $aResult); + $this->getLogger()->debug( + 'The address parsing was conclusive.', + array( + 'countyCode' => $iso2CountryCode, + 'originalAddressLine' => $addressLine + ) + ); + + if ($followingNumberMatches) { + $relevantParsing = $followingNumberMatches; + } } + // else is hidden thru the default value! - return $aResult; + $this->getLogger()->info( + 'Parsed the given single address line to a value array.', + array( + 'countyCode' => $iso2CountryCode, + 'originalAddressLine' => $addressLine, + 'parsedAddressLine' => $relevantParsing + ) + ); + + return $relevantParsing; } /** @@ -50,10 +149,13 @@ protected function _parseAddressFields($oAmazonData, array &$aResult) $aReverseOrderCountries = $this->getConfig()->getConfigParam('aAmazonReverseOrderCountries'); $aMap = array_flip($aReverseOrderCountries); - $aCheckOrder = isset($aMap[$oAmazonData->CountryCode]) === true ? array (2, 1) : array(1, 2); + $aCheckOrder = isset($aMap[$oAmazonData->CountryCode]) === true ? array(2, 1) : array(1, 2); $sStreet = ''; $sCompany = ''; + // TODO: Fix it with OXAP-292. Understanding this is not mentally-easy. + // The break is used in "reversed order" (company is filled after the street), + // this feels like "pfeil durch die brust ins auge." foreach ($aCheckOrder as $iCheck) { if ($aAmazonAddresses[$iCheck] !== '') { if ($sStreet !== '') { @@ -82,6 +184,38 @@ protected function _parseAddressFields($oAmazonData, array &$aResult) ); } + /** + * Matches a pattern with a following possible number (as a string) against the given address line. + * + * @param string $addressLine + * + * @return bool|array Contains an array with "Number", "Name", "AddInfo" Key or false on no match. + */ + protected function _searchForFollowingNumberInAddressLine($addressLine) + { + return preg_match( + '/\s*(?P[^\d]*[^\d\s])\s*((?P\d[^\s]*)\s*(?P.*))*/', + $addressLine, + $matches + ) ? $matches : false; + } + + /** + * Matches a pattern with a leading possible number (as a string) against the given address line. + * + * @param string $addressLine + * + * @return bool|array Contains an array with "Number", "Name", "AddInfo" Key or false on no match. + */ + protected function _searchForLeadingNumberInAddressLine($addressLine) + { + return preg_match( + '/\s*(?P\d[^\s]*)*\s*(?P[^\d]*[^\d\s])\s*(?P.*)/', + $addressLine, + $matches + ) ? $matches : false; + } + /** * Returns Parsed address from Amazon by specific rules * diff --git a/application/views/admin/de/bestitamazonpay4oxid_lang.php b/application/views/admin/de/bestitamazonpay4oxid_lang.php index 9767c63..7bb75be 100755 --- a/application/views/admin/de/bestitamazonpay4oxid_lang.php +++ b/application/views/admin/de/bestitamazonpay4oxid_lang.php @@ -76,7 +76,6 @@ 'SHOP_MODULE_blShowAmazonPayButtonAtDetails' => 'Amazon Pay-Button auf Produktdetailseite anzeigen', 'SHOP_MODULE_blShowAmazonPayButtonAtCartPopup' => 'Amazon Pay-Button im Warenkorb Popup anzeigen', 'SHOP_MODULE_aAmazonReverseOrderCountries' => 'ISO2 Code der Länder, bei denen die AddressLineX Rückgaben von Amazon vertauscht sind (AddressLine1 == Firma, AddressLine2 == Straße)', - 'SHOP_MODULE_aAmazonStreetNoStreetCountries' => 'ISO2 Code der Länder, die Straße und Hausnummer vertauscht haben', 'SHOP_MODULE_GROUP_bestitAmazonPay4OxidLanguages' => 'Sprach Einstellungen', 'SHOP_MODULE_aAmazonLanguages' => "Sprach Einstellungen ('Oxid Sprachkürzel' => 'Amazon Sprachwert')", diff --git a/application/views/admin/en/bestitamazonpay4oxid_lang.php b/application/views/admin/en/bestitamazonpay4oxid_lang.php index e4895fc..e5a3cb6 100755 --- a/application/views/admin/en/bestitamazonpay4oxid_lang.php +++ b/application/views/admin/en/bestitamazonpay4oxid_lang.php @@ -76,7 +76,6 @@ 'SHOP_MODULE_blShowAmazonPayButtonAtDetails' => 'Show Amazon Pay button on the details page', 'SHOP_MODULE_blShowAmazonPayButtonAtCartPopup' => 'Show Amazon Pay button on the cart popup', 'SHOP_MODULE_aAmazonReverseOrderCountries' => 'ISO2 code of the countries where the AddressLineX returned from Amazon are reversed (AddressLine1 == company, AddressLine2 == street)', - 'SHOP_MODULE_aAmazonStreetNoStreetCountries' => 'ISO2 Code of the countries that have reversed street and house numbers', 'SHOP_MODULE_GROUP_bestitAmazonPay4OxidLanguages' => 'Language Settings', 'SHOP_MODULE_aAmazonLanguages' => "Language mapping ('Oxid language abbreviation' => 'Amazon language value')", diff --git a/metadata.php b/metadata.php index 43ddaaf..d103469 100755 --- a/metadata.php +++ b/metadata.php @@ -352,13 +352,6 @@ 'value' => array('DE', 'AT', 'FR'), 'position' => 11 ), - array( - 'group' => 'bestitAmazonPay4OxidConfiguration', - 'name' => 'aAmazonStreetNoStreetCountries', - 'type' => 'arr', - 'value' => array('FR', 'GB'), - 'position' => 12 - ), ), 'events' => array( 'onActivate' => 'bestitAmazonPay4Oxid_init::onActivate', diff --git a/tests/unit/application/models/bestitAmazonPay4OxidAdressUtilTest.php b/tests/unit/application/models/bestitAmazonPay4OxidAdressUtilTest.php index 4be1156..b952cbc 100644 --- a/tests/unit/application/models/bestitAmazonPay4OxidAdressUtilTest.php +++ b/tests/unit/application/models/bestitAmazonPay4OxidAdressUtilTest.php @@ -2,7 +2,7 @@ use Psr\Log\NullLogger; -require_once dirname(__FILE__).'/../../bestitAmazon4OxidUnitTestCase.php'; +require_once dirname(__FILE__) . '/../../bestitAmazon4OxidUnitTestCase.php'; /** * Unit test for class bestitAmazonPay4OxidAddressUtil @@ -12,6 +12,15 @@ */ class bestitAmazonPay4OxidAddressUtilTest extends bestitAmazon4OxidUnitTestCase { + /** + * Started object to test. + * + * Filled by the setup method. + * + * @var bestitAmazonPay4OxidAddressUtil|null + */ + private $fixture; + /** * @param oxConfig $oConfig * @param DatabaseInterface $oDatabase @@ -31,14 +40,98 @@ private function _getObject(oxConfig $oConfig, DatabaseInterface $oDatabase, oxL return $oBestitAmazonPay4OxidAddressUtil; } + /** + * Returns assert to check if the single address line gets parsed correctly. + * + * @return array + */ + public function getParseSingleAddressAsserts() + { + return array( + // test desc => Test value, country, result array. + 'Test german address' => array('Teststraße 1', 'DE', array('Name' => 'Teststraße', 'Number' => '1')), + 'Test german address, no whitespace' => array( + 'Teststreet1a', + 'DE', + array('Name' => 'Teststreet', 'Number' => '1a') + ), + 'Test german address with add info' => array( + 'Teststreet 1a addinfo', + 'DE', + array('Name' => 'Teststreet', 'Number' => '1a', 'AddInfo' => 'addinfo') + ), + 'Test german separated address with add info' => array( + 'Test street 1a addinfo', + 'DE', + array('Name' => 'Test street', 'Number' => '1a', 'AddInfo' => 'addinfo') + ), + 'Test german address with add info no whitespace' => array( + 'Teststreet1 addinfo', + 'DE', + array('Name' => 'Teststreet', 'Number' => '1', 'AddInfo' => 'addinfo') + ), + 'Test address format "street streetnumber" without streetnumber' => array( + 'Teststreet', + 'DE', + array('Name' => 'Teststreet') + ), + 'Test FR address without streetnumber' => array( + 'Teststreet', + 'FR', + array('Name' => 'Teststreet', 'Number' => '') + ), + 'Test FR address' => array( + '1a Teststreet', + 'FR', + array('Name' => 'Teststreet', 'Number' => '1a') + ), + 'Test FR address no whitespace' => array( + '1Teststreet', + 'FR', + array('Name' => 't', 'Number' => '1Teststree') + ), + 'Test FR address no whitespace with add info' => array( + '1Teststreet addInfo', + 'FR', + array('Number' => '1Teststreet', 'Name' => 'addInfo') + ) + ); + } + + /** + * Sets up the test. + * + * @return void + */ + public function setUp() + { + parent::setUp(); + + $this->fixture = new bestitAmazonPay4OxidAddressUtil(); + + $this->fixture->setLogger(new NullLogger()); + } + /** * @group unit + * + * @return void */ - public function testCreateInstance() + public function testInheritanceOfObject() { $oBestitAmazonPay4OxidAddressUtil = new bestitAmazonPay4OxidAddressUtil(); - $oBestitAmazonPay4OxidAddressUtil->setLogger(new NullLogger()); - self::assertInstanceOf('bestitAmazonPay4OxidAddressUtil', $oBestitAmazonPay4OxidAddressUtil); + + self::assertInstanceOf('bestitAmazonPay4OxidContainer', $this->fixture); + } + + /** + * Checks if the required interface is registered. + * + * @return void + */ + public function testInterfacesOfObject() + { + self::assertInstanceOf('\Psr\Log\LoggerAwareInterface', $this->fixture); } /** @@ -57,16 +150,11 @@ public function testParseAmazonAddress() $oConfig->expects($this->any()) ->method('getConfigParam') - ->with($this->logicalOr( - $this->equalTo('aAmazonReverseOrderCountries'), - $this->equalTo('aAmazonStreetNoStreetCountries') - )) + ->with($this->equalTo('aAmazonReverseOrderCountries')) ->will($this->returnCallback( function($sParameter) { if ($sParameter === 'aAmazonReverseOrderCountries') { return array('DE', 'AT', 'FR'); - } elseif ($sParameter === 'aAmazonStreetNoStreetCountries') { - return array('FR'); } } )); @@ -194,96 +282,36 @@ function($sParameter) { } /** + * Tests if the given address lines are parsed correctly. + * * @covers ::_parseSingleAddress() + * @dataProvider getParseSingleAddressAsserts * @throws ReflectionException + * + * @param string $addressLine + * @param string $countryIso + * @param array $checkedValues The result array of the parsing. + * + * @return void */ - public function test_parseSingleAddress() + public function test_parseSingleAddress($addressLine, $countryIso, array $checkedValues) { - $oConfig = $this->_getConfigMock(); - $oConfig->expects($this->any()) - ->method('getConfigParam') - ->with('aAmazonStreetNoStreetCountries') - ->will($this->returnValue(array('FR'))); - - $oBestitAmazonPay4OxidAddressUtil = $this->_getObject( - $oConfig, - $this->_getDatabaseMock(), - $this->_getLanguageMock() + $testResult = $this->callMethod( + $this->fixture, + '_parseSingleAddress', + array($addressLine, $countryIso) ); - $oBestitAmazonPay4OxidAddressUtil->setLogger(new NullLogger()); + self::assertTrue(is_array($testResult)); - // Test german address - $aTestResult = $this->callMethod( - $oBestitAmazonPay4OxidAddressUtil, - '_parseSingleAddress', - array('Teststreet 1', 'DE') - ); - self::assertEquals(array( - 'Name' => 'Teststreet', - 'Number' => '1' - ), array( - 'Name' => $aTestResult['Name'], - 'Number' => $aTestResult['Number'] - )); - - // Test german address with add info - $aTestResult = $this->callMethod( - $oBestitAmazonPay4OxidAddressUtil, - '_parseSingleAddress', - array('Teststreet 1 addinfo', 'DE') - ); - self::assertEquals(array( - 'Name' => 'Teststreet', - 'Number' => '1', - 'AddInfo' => 'addinfo' - ), array( - 'Name' => $aTestResult['Name'], - 'Number' => $aTestResult['Number'], - 'AddInfo' => $aTestResult['AddInfo'] - )); - - // Test FR address - $aTestResult = $this->callMethod( - $oBestitAmazonPay4OxidAddressUtil, - '_parseSingleAddress', - array('1 Teststreet', 'FR') - ); - self::assertEquals(array( - 'Name' => 'Teststreet', - 'Number' => '1' - ), array( - 'Name' => $aTestResult['Name'], - 'Number' => $aTestResult['Number'] - )); - - // Test FR address without streetnumber - $aTestResult = $this->callMethod( - $oBestitAmazonPay4OxidAddressUtil, - '_parseSingleAddress', - array('Teststreet', 'FR') - ); - self::assertEquals(array( - 'Name' => 'Teststreet', - 'Number' => '' - ), array( - 'Name' => $aTestResult['Name'], - 'Number' => $aTestResult['Number'] - )); - - // Test address format "street streetnumber" without streetnumber - $aTestResult = $this->callMethod( - $oBestitAmazonPay4OxidAddressUtil, - '_parseSingleAddress', - array('Teststreet', 'DE') - ); - self::assertEquals(array( - 'Name' => 'Teststreet', - 'Number' => '' - ), array( - 'Name' => $aTestResult['Name'], - 'Number' => $aTestResult['Number'] - )); + foreach ($checkedValues as $field => $value) { + self::assertArrayHasKey($field, $testResult); + self::assertSame( + $value, + $testResult[$field], + sprintf('The value of field %s does not match.', $field) + ); + } } /**