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

Bug in parent #32

Open
brunobg opened this issue May 4, 2020 · 11 comments
Open

Bug in parent #32

brunobg opened this issue May 4, 2020 · 11 comments

Comments

@brunobg
Copy link

brunobg commented May 4, 2020

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
Copy link
Owner

igaster commented May 5, 2020 via email

@brunobg
Copy link
Author

brunobg commented May 5, 2020

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

  1. select country US
  2. states from US are loaded
  3. change country to BR
  4. 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
Copy link
Author

brunobg commented May 5, 2020

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
Copy link
Author

brunobg commented May 5, 2020

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 brunobg changed the title Importing multiple countries individually messes with structure? Bug in parent May 5, 2020
@brunobg
Copy link
Author

brunobg commented May 5, 2020

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
Copy link
Owner

igaster commented May 6, 2020

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
Copy link
Author

brunobg commented Jun 10, 2020

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

@wrabit
Copy link

wrabit commented Jun 21, 2020

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
Copy link
Author

brunobg commented Jun 23, 2020

@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
Copy link

wrabit commented Jun 23, 2020

@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
Copy link
Author

brunobg commented Jun 24, 2020

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants