Bug in parent - Githubissues

igaster / laravel_cities

Find any country/city in the world. Get Long/Lat etc. Deploy geonames.org database localy. Optimized DB tree

MIT License

179 stars 65 forks source link

Bug in parent #32

Open brunobg opened 4 years ago

brunobg commented 4 years ago

I ran these two commands:

artisan geo:seed US --append
artisan geo:seed BR --append

When using the CityPicker states and cities mixed up, from individual US.txt and BR.txt. Importing the whole countries database did not result in this behavior. Is this something expected or a bug? Any workarounds?

igaster commented 4 years ago

What do you mean by "mixed"? Do they appear to belong to a single country?

brunobg commented 4 years ago

Yes, when I use the vue component, here's what happens:

select country US
states from US are loaded
change country to BR
states from BR are loaded and mixed with US.

I though this might be a bug in the vue component, bu the same behavior does not happen if the entire country DB is loaded at once, only when one country is loaded and then another is appended.

brunobg commented 4 years ago

Actually, even with the entire import I'm getting some strange results.

/geo/ancestors/3469034 returns not only Brazilian states, but also those from Portugal and Poland.

brunobg commented 4 years ago

apparently there's something buggy in parent. /geo/item/3665474 is correct (Acre, state of Brazil), but /geo/parent/3665474 returns Poland (798544) when the correct parent is Brazil (3469034). parent_id is correct in /geo/item, so it does seem like a bug in the code.

brunobg commented 4 years ago

Yep, definitely bugged. The PR #33 fixes Geo::parent, but ancestors is returning data that it shouldn't. I didn't stop to read how you build your tree with left/right. I'd really appreciate if you could fix this or at least give me a few pointers, as this is a showstopper bug with the vue component. Your lib is very helpful, let's fix this :)

igaster commented 4 years ago

Thanks for the PR... I will check this in the weekend and run some tests...

The left/right columns are actually important to calculate the parents/children. This is a reference of how it works: Nested Set Model

brunobg commented 4 years ago

Hey, did you get a chance to look at this?

wrabit commented 4 years ago

Just to add some info and confirmation I am seeing a bug which sounds like it relates to this issue;

I choose 'United Kingdom' and the top level dropdown is replace with Poland.

I choose another such as Republic of Yemen and 3 dropdowns below pre-filled with unrelated places.

brunobg commented 4 years ago

@wrabit Check my for to see if fixes your bugs https://github.com/Corollarium/laravel_cities

@igaster do you still plan to check this? If not please let me know and I'll make a separate package with my fork.

wrabit commented 4 years ago

@brunobg how can I pull in your version to my project? (tried setting up your repo in my composer.json and requiring it but no joy)

brunobg commented 4 years ago

@wrabit follow the composer instructions: https://getcomposer.org/doc/05-repositories.md#loading-a-package-from-a-vcs-repository

If there's no response here I'll release my fork on composer. But I'm not using the tree, so I'd need to remove it from the code and it will take a bit of time that I don't have right now. Would you be up to give some help for that? It's essentially removing code.

BTW, in my application I'm using the '/children/' + geoId and the /countries endpoints, which are working properly. I changed the vue file significantly, removing calls to ancestors which is slower and I think still buggy IIRC. My component is simpler than the code here, but it works as expected to select cities. I didn't commit it yet, let me know if you want it.