Open brettlaishley opened 2 years ago
@aligent-lturner I was going to assign this one to Derrick Deng but I can't seem to, maybe he could be added to the repository?
@aligent-lturner the only suggestion from this issue is now the Additional Changes section, if we want to impelment it.
I got a little hyper focused and didn't realise the main problem has since been fixed in a later version of the module: https://github.com/aligent/magento-stockists-module/blob/main/Model/Stockist/Hydrator.php#L119
@brettlaishley originally with google's geocoding, you could only send an address. They later added the ability to send components
, which lets you filter by:
postal_code
country
(ISO-3166-1 code)And influence (but not restrict) results by:
route
locality
(e.g. suburb)administrative_area
(e.g. state)https://developers.google.com/maps/documentation/geocoding/requests-geocoding#component-filtering
For our purposes, just adding the postal_code
and country
components should resolve most issues we have with geocoding.
For the most part, stockists should be geocoded with a full address, including state, country and postcode, so the chances of an incorrect result are slim, anyway.
@aligent-lturner the only suggestion from this issue is now the Additional Changes section, if we want to impelment it.
I got a little hyper focused and didn't realise the main problem has since been fixed in a later version of the module: https://github.com/aligent/magento-stockists-module/blob/main/Model/Stockist/Hydrator.php#L119
From some testing with stockists whose addresses are set in New Zealand, I think the main problem might be worth reopening for some discussion.
Magento2 doesn't seem to have region information for New Zealand by default (a couple of people also raised similar questions in magento2 channel yesterday), so in this case, when a New Zealand stockist is saved in the admin area, no matter what you put in the region
filed, it is always set to be empty because the region_id
is empty (as there's no region drop down for New Zealand):
https://github.com/aligent/magento-stockists-module/blob/main/Model/Stockist/Hydrator.php#L115
I've got a feeling that in the real project this problem could be easily solved by adding the region info to Magento's directory_country_region
table.
But do we need to do some modifications around https://github.com/aligent/magento-stockists-module/blob/main/Model/Stockist/Hydrator.php#L115 so that the region info users put in the admin could still be kept?
@derrick-deng-aligent for countries without require regions (e.g. NZ), the region
field is used instead of region_id
. I imagine it's just a case of not setting the value to ''
on https://github.com/aligent/magento-stockists-module/blob/b0ca8a04e5a0ab3dd1b302ddb4c0d28fbe69066d/Model/Stockist/Hydrator.php#L115
In theory, if there is a region set, then this will have that value. Then, if there is also a region id set, we'll load the region corresponding to that and overwrite with its name.
Would need to test against both AU and NZ addresses.
@aligent-lturner @brettlaishley
for countries without require regions (e.g. NZ), the region field is used instead of region_id. I imagine it's just a case of not setting the value to '' on https://github.com/aligent/magento-stockists-module/blob/b0ca8a04e5a0ab3dd1b302ddb4c0d28fbe69066d/Model/Stockist/Hydrator.php#L115
I think the problem with the Hydrator::hydrate
function is for countries the region
field is used instead of region_id
, the value of region
is always ''
:
https://github.com/aligent/magento-stockists-module/blob/b0ca8a04e5a0ab3dd1b302ddb4c0d28fbe69066d/Model/Stockist/Hydrator.php#L115-L120
I understand that in real life you don't need to include the region in a New Zealand address at all (just like no need to include the county in a UK address) so technically the region value can just be ''
for these countries. It just might confuse users who insist on putting "Oxfordshire" or "Cambridgeshire" in the region
as the value can never be saved in the admin.
And if you test against the admin grid inline edit you will find the region
value is emptied (so as hours
, lat
and lng
because of their data processors are in use) by the Hydrator::hydrate
function even though it's an Australian address, this is because region_id
is not one of the request params from the inline edit.
Have tested that
https://github.com/aligent/magento-stockists-module/blob/b0ca8a04e5a0ab3dd1b302ddb4c0d28fbe69066d/Model/Stockist/Hydrator.php#L115
can be removed as there is always a region
field value (''
by default) in the array $data
.
I'm not sure if I have tested against all the cases but I reckon there is some issue around the Hydrator::hydrate
function needs to be fixed and I'm happy to work on it if you find it necessary :)
@derrick-deng-aligent I'm happy for you to keep working on it :+1:
Main Problem
The region field for Stockists was assumed to be the state name from the geocoding implementation, and this is what we're populating from NIFI, however when a stockist is saved in the admin area the region_id is used instead of the region name. Therefore the address we're sending to the geocoding API is48-70%20MAIN%20ST%2CLILYDALE%2C1712%2CAU
instead of48-70%20MAIN%20ST%2CLILYDALE%2CNSW%2CAU
which maps to somewhere in America instead of to Victoria.When the field is updated directly from the Magento admin area aregion_id
is saved instead of the state name. We need to decide which implementation we move forward with and edit the code accordingly. Discuss how we want to use this module for all of our client's moving forward.I think we want to save the region_id because this is going to be used throughout the rest of Magento with billing and shipping addresses, so we could potentially use a class to load the region by its ID and send the region name to the geocoding API. See Magento's\Magento\Directory\Model\Region
class to see what I mean.The main problem is fixed in v2 of this module.
Additional Changes
According to the google maps geocoding API documentation, we can send an optional region (think country) parameter to the ensure the region biasing is applied. This will help reduce errors when data is inconsistent.
https://developers.google.com/maps/documentation/geocoding/requests-geocoding
Please add the optional
region
parameter to$queryParams
in\Aligent\Stockists\Model\GoogleMapsAdapter::buildRequest
to reduce errors.Note, this API's reference to
region
is the 2 character country code, so you want to send "country" a 2nd time for this parameter as well as on the end of theaddress
parameter.