search

activeadmin-plugins / active_admin_import

:paperclip: active_admin_import is based on activerecord-import gem - the most efficient way to import for ActiveAdmin
http://activeadmin-plugins.github.io/active_admin_import
MIT License
183 stars 101 forks source link

Allow batch_slice_columns to work when batching #168

Closed doredesign closed 5 years ago

doredesign commented 5 years ago

Currently the use_indexes are set correctly only for the first batch.

You may be wondering why I re-ordered the columns in authors.csv. I did this because it was the best way I could find to get a good failing test for my use case. I could not test removing the name because that column is required. I couldn't test removing the last_name column because that column needs to be unique and validation would fail for the second batch because the first batch (and record) would have a NULL value for last_name. I also could not use the date column in its former order because the bug only manifests when columns other than the last one(s) are sliced.

coveralls commented 5 years ago

Coverage Status

Coverage increased (+0.02%) to 97.778% when pulling 5ccf95c7bd0bb8368aec235021e7087ccd11cc69 on doredesign:fix_batch_slice_columns_while_batching into 7e17eb6f33cdb0329ac98730ab063cd0f31339c9 on activeadmin-plugins:master.

workgena commented 5 years ago

Hello @denisoster and thank you for the contribution 🛠

I've checked this one. The described problem really exists in test scenario.

But I want to check this in real application, next weak. And consult with @Fivell

doredesign commented 5 years ago

@workgena Sounds good. 👍

workgena commented 5 years ago

I've checked the solution manually. The problem and solution confirmed.

@Fivell what do you think about merging this PR, as for me - it is ready.

Fivell commented 5 years ago

@workgena, looks reasonable for me.

workgena commented 5 years ago

Version 4.1.1 has been published, it includes this fix. Gratitude to @doredesign 🎉