AndyButland / UmbracoPersonalisationGroups

Package for personalisation of content with Umbraco.
MIT License
38 stars 18 forks source link

Cannot match regions in Denmark #21

Closed steffenborup closed 4 years ago

steffenborup commented 4 years ago

I am having problems getting the plugin to match regions in Denmark.

I have tried setting up af personalisation group for the region New York, United States and then enter a New York IP address in personalisationGroups.testFixedIp in Web.config. That works perfectly.

I am also able to set up a group to match only the country Denmark and that works fine too. But when I try to specify groups for any of the five regions inside Denmark, I am not able to match any of the groups.

What could be wrong? Could it have something to do with the fact that in the criteria picker for regions, the Danish regions are represented in their localised/Danish names and not international names?

Example: "Midtjylland" instead of "Central Jutland"

Any help is very much appreciated!

AndyButland commented 4 years ago

Just had a look at this, and think you're right - for certain countries it seems the MaxMind database response contains a name for the region, that I'm checking against, that's in English. So that's not matching. However, there's also a Names collection, and that contains an entry that's matching the local language name. So think if I change things to match on any of those, that should resolve the problem.

I'll see if I can resolve this in the upcoming days and create a new release.

AndyButland commented 4 years ago

This should be resolved now, in release 1.0.3 (for V7) and 2.1.2 (for V8), available on nuget and our.

steffenborup commented 4 years ago

Thank you for the quick response! Unfortunately, I cannot get it to work. The installation fails for me when installing from local zip file or our, because Zone.UmbracoPersonalisationGroups.dll is missing from the V7 package that I am using.

I was able to extract the dll from the nuget package, but to no avail. It still does not match Danish regions.

Could it be because of the failed installation process or is the code still not matching regions correctly?

I noticed that the criteria picker for regions still display the localised names for Denmark (see attached). Does that have anything to do with the issue?

Screenshot 2019-11-12 at 15 20 58
AndyButland commented 4 years ago

Seems I messed up somehow in creating the release for V7 on our. I've updated it now so the dll is there (https://our.umbraco.com/FileDownload?id=20077). Could you try updating to that again please? The updated code was in the Common dll, so I think you'll still be missing what I amended.

steffenborup commented 4 years ago

The package now installs successfully. But still no luck matching regions in Denmark. Is there any way I can debug what region the package thinks I'm in?

AndyButland commented 4 years ago

Hmm... that's annoying, fairly sure it's working for me. To debug you could clone the source from here, add the projects - Zone.UmbracoPersonalisationGroups.Common and Zone.UmbracoPersonalisationGroups to your solution, remove the dlls installed by the package, and reference the projects instead. Then with a breakpoint in RegionPersonalisationGroupCriteria.MatchesVisitor() you should be able to debug what is being matched.

What IP are you using by the way to test against? I can try that locally here too.

steffenborup commented 4 years ago

This is located in Central Jutland, Denmark: 81.95.247.8

This is also the region found by MaxMind's GeoIP2 Database Demo: https://www.maxmind.com/en/geoip-demo

steffenborup commented 4 years ago

I am not experienced in .NET development, so I'd really appreciate if you could test what region is returned for the above mentioned IP. Thanks!

AndyButland commented 4 years ago

I'm finding that IP matches with Denmark as a country, but I don't get any detail on the region from the MaxMind database.

I'm also testing with this IP: 2.106.175.3 - that does match with "Hovedstaden, Denmark". Would you mind trying that with your installation please, just to (hopefully) confirm that does work for you? If you do, that at least means we are seeing the same thing in terms of how the package is (or isn't) working.

When I try both these IPs on the site you linked, I can see the one you are testing with is coming back with a much wider accuracy score, so it might be for that reason we don't get the region information back, only the country.

steffenborup commented 4 years ago

Hi again. Thank you so much for all your help. I did some more testing, and these are my findings.

Testing 2.106.175.3 on my installation This matches Hovedstaden, Denmark. So same behaviour as on your installation.

Testing the GeoLite2-City database using the PHP library I am more experienced with PHP, so I decided to do some testing directly on GeoLite2-City.mmdb using MaxMind's PHP library (https://maxmind.github.io/GeoIP2-php/), just to see what results were returned.

Interestingly, when testing 81.95.247.8, no region (or "subdivision") is returned. It seems like data about this IP's region is only included in MaxMind's paid database, which is most likely what is used on the demo site.

On the other hand, I also tried testing my own IP (not mentioned here for reasons of privacy) against the GeoLite2 database and it correctly return a subdivision named "Central Jutland" (see below image).

Screenshot 2019-11-13 at 10 26 41

I still think localised names are the problem I still think the problem is that the region names used in the criteria picker does not always match region/subdivision names in the MaxMind database.

"Hovedstaden" is the Danish name for "Capital Region". I think the reason that "Hovedstaden" works is that this name appears in both the criteria picker and in the database (even though the database lists the localised name as being French?) – see image below.

Screenshot 2019-11-13 at 10 29 55

But if I select "Midtjylland" (the Danish name for "Central Jutland") in the criteria picker, this is never matched because the database only contains these names:

Screenshot 2019-11-13 at 10 38 10

Is it possible to make the criteria picker use English names instead of localised names?

AndyButland commented 4 years ago

Think it's possible yes. I can't simply update them, as that might be a breaking change for others, and it's feasible that in some countries it's better from a usability and matching perspective to keep the local names.

But I've set up a means to override them. Not released yet as would be good to establish with you that it works if that's OK.

I've built two new dlls that you can download and extract from here, copy into your bin folder replacing the two that have come from the package installation.

You then need to add a configuration key with a path to a file - something like this, in web.config:

<add key="personalisationGroups.geoLocationRegionListPath" value="/App_Data/regions.txt" />

Finally, if you take a copy of this file and save it at the same location - the package will use this list of regions instead of the one that comes embedded in the package dlls.

And you can edit that file - find the five lines starting with DK, and update the names of the regions.

Restart the web application and you should see your English names coming through and being used for matches.

Please let me know how it goes. If all looks good I'll push out a new release.

steffenborup commented 4 years ago

I did as described, but now the region dropdown list is always empty.

Screenshot 2019-11-14 at 10 07 34

If I change the "geoLocationRegionListPath" key to point to a non-existent file and restart the application pool, the default region names appear again. So it seems the package does pick up the regions.txt file, but does not display its contents.

AndyButland commented 4 years ago

I'll take another look. Meantime, could you attach your updated regions.txt file, and let me know what configuration you added to reference it please?

steffenborup commented 4 years ago

Please find attached my regions.txt file.

regions.txt

I downloaded https://www.maxmind.com/download/geoip/misc/region_codes.csv and changed line 749-753 from this:

DK,17,"Hovedstaden" DK,18,"Midtjylland" DK,19,"Nordjylland" DK,20,"Sjelland" DK,21,"Syddanmark"

... to this:

DK,17,"Capital Region" DK,18,"Central Jutland" DK,19,"North Denmark" DK,20,"Zealand" DK,21,"South Denmark"

AndyButland commented 4 years ago

Thanks, I think it's a line endings issue (my test one was using Windows, and yours Unix). There's a couple more dlls here that should handle both formats if you don't mind trying it once more please? Thanks.

steffenborup commented 4 years ago

And it works! Thank you so much for your help. It is very much appreciated!

AndyButland commented 4 years ago

Great - glad we got there in the end! Thanks for your persistence too. I'll push this out in the next release, so you can upgrade to that (but it'll be the same code as the dlls I've provided you).

AndyButland commented 4 years ago

Have pushed this functionality out in release 1.0.4 (V7) and 2.1.3 (V8).