avstudnitz / AvS_FastSimpleImport

Wrapper for Magento ImportExport functionality, which imports products and customers from arrays
308 stars 146 forks source link

Potential issue: Wrong approach to default category position #281

Open barbazul opened 8 years ago

barbazul commented 8 years ago

In https://github.com/avstudnitz/AvS_FastSimpleImport/blob/master/src/app/code/community/AvS/FastSimpleImport/Model/Import/Entity/Category.php#L477 you are setting the category position to 10000 when it is not specified.

I am currently tracking an issue that apparently relates to this.

The symptoms are, after an import whenever I rearrange a category within its parent the new position gets set to 10001, sending it to the bottom.

Afterwards whenever I rearrange another category in the same parent, the first category gets reverted back to 10000 and the new category gets assigned to 10001.

This is because of the logic inside Mage_Catalog_Model_Resource_Category::_processPositions

I am unsure about how best to approach this issue. Suggestions?

barbazul commented 8 years ago

We have successfully confirmed this issue for different scenarios (changing parent, rearranging within the same parent and deleting categories with siblings).

I am still not sure what the bes approach would be. Perhaps preload the max(position) for each parent_id and then autoincrement it for each child that gets added?

paales commented 8 years ago

@barbazul Your suggested solution might solve the issue. Please create a PR to solve the issue :) You can create the PR on the develop branch.

mbijnsdorp commented 8 years ago

I ran into this as well and started thinking about preloading the max position. However that might prove difficult as you'd have to recalculate it if a category is moved to a new parent during the import. Why is the position set to 10000 in the first place? If I remove that part, the category is imported just fine at position 0.

barbazul commented 8 years ago

@mbijnsdorp I am not sure about leaving it at 0 would fix the issue. I think trouble beings when more than one category has a repeated position value, be it 0, 100000 or any other.

@paales I have some spare time right now, I'll work on a PR and try to commit later tonight

barbazul commented 7 years ago

I created a new PR request with a more up to date version of @mbijnsdorp solution.

Please consider it for merging as this issue has been open for over a year now

barbazul commented 7 years ago

I would love to have some feedback on this issue as I find myself with some spare time to work some more on this if needed

barbazul commented 7 years ago

Should I change the PR base branch to develop?

barbazul commented 6 years ago

@paales

barbazul commented 6 years ago

@avstudnitz