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

Fix import products without Force all ID numbers #8255

Merged
merged 2 commits into from Oct 6, 2017

Conversation

Projects
None yet
4 participants
@gabdara
Contributor

gabdara commented Aug 17, 2017

Questions Answers
Description? Importing products without option Force all ID numbers but with the ID column provided resulted into updating the existing products. This fix makes sure that the ID column gets ignored when not using Force all ID numbers. https://www.prestashop.com/forums/topic/566115-prestashop-17-is-now-available/?p=2596950
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
BO: fix import products without Force all ID numbers
Importing products without option Force all ID numbers but with the ID column provided resulted into updating the existing products.
https://www.prestashop.com/forums/topic/566115-prestashop-17-is-now-available/?p=2596950
@gabdara

This comment has been minimized.

Show comment
Hide comment
@gabdara

gabdara Aug 17, 2017

Contributor

The same bug exists in the 1.6.1.x branch and the same modifications could be applied in function productImport() to fix it.

Contributor

gabdara commented Aug 17, 2017

The same bug exists in the 1.6.1.x branch and the same modifications could be applied in function productImport() to fix it.

@vincentbz vincentbz added this to the 1.7.2.2 milestone Aug 18, 2017

@eternoendless eternoendless modified the milestones: 1.7.2.3, 1.7.2.2 Aug 25, 2017

@Quetzacoalt91

Quetzacoalt91 requested changes Aug 30, 2017 edited

Hello,

The idea is good and the changes seems appropriate to the objective, but may I suggest something else in order to keep the original loading order?

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Aug 30, 2017

Member
    protected function productImportOne($info, $default_language_id, $id_lang, $force_ids, $regenerate, $shop_is_feature_active, $shop_ids, $match_ref, &$accessories, $validateOnly = false)
    {
        if (!$force_ids) {
            unset($info['id']);
        }

        $id_product = null;
        // Use product reference as key
        if (isset($info['id']) && (int)$info['id']) {
            $id_product = (int) $info['id'];
        } elseif ($match_ref && array_key_exists('reference', $info)) {
            $idProductByRef = (int) Db::getInstance()->getValue('
                                    SELECT p.`id_product`
                                    FROM `' . _DB_PREFIX_ . 'product` p
                                    ' . Shop::addSqlAssociation('product', 'p') . '
                                    WHERE p.`reference` = "' . pSQL($info['reference']) . '"
                                ', false);
            if ($idProductByRef) {
                $id_product = $idProductByRef;
            }
        }

        $product = new Product($id_product);
        [...]

I haven't tested this code, but the process would be to load first from the ID if requested by the options, then we load from the reference if it exists.

Member

Quetzacoalt91 commented Aug 30, 2017

    protected function productImportOne($info, $default_language_id, $id_lang, $force_ids, $regenerate, $shop_is_feature_active, $shop_ids, $match_ref, &$accessories, $validateOnly = false)
    {
        if (!$force_ids) {
            unset($info['id']);
        }

        $id_product = null;
        // Use product reference as key
        if (isset($info['id']) && (int)$info['id']) {
            $id_product = (int) $info['id'];
        } elseif ($match_ref && array_key_exists('reference', $info)) {
            $idProductByRef = (int) Db::getInstance()->getValue('
                                    SELECT p.`id_product`
                                    FROM `' . _DB_PREFIX_ . 'product` p
                                    ' . Shop::addSqlAssociation('product', 'p') . '
                                    WHERE p.`reference` = "' . pSQL($info['reference']) . '"
                                ', false);
            if ($idProductByRef) {
                $id_product = $idProductByRef;
            }
        }

        $product = new Product($id_product);
        [...]

I haven't tested this code, but the process would be to load first from the ID if requested by the options, then we load from the reference if it exists.

@gabdara

This comment has been minimized.

Show comment
Hide comment
@gabdara

gabdara Aug 30, 2017

Contributor

Hi @Quetzacoalt91
The proposed changes look good to me.

Contributor

gabdara commented Aug 30, 2017

Hi @Quetzacoalt91
The proposed changes look good to me.

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Aug 31, 2017

Member

I just tried my changes with two imports:

The first option with the option "Force IDs" disabled:

  • Product imported with a new ID
    capture du 2017-08-31 16-41-25
  • The first one remains the same
    capture du 2017-08-31 16-42-11

Second import with IDs forced:
capture du 2017-08-31 16-43-51

If you want, you can update your PR in order to keep the contribution. 😉

Member

Quetzacoalt91 commented Aug 31, 2017

I just tried my changes with two imports:

The first option with the option "Force IDs" disabled:

  • Product imported with a new ID
    capture du 2017-08-31 16-41-25
  • The first one remains the same
    capture du 2017-08-31 16-42-11

Second import with IDs forced:
capture du 2017-08-31 16-43-51

If you want, you can update your PR in order to keep the contribution. 😉

@vincentbz vincentbz modified the milestones: 1.7.2.3, 1.7.3.0 Sep 26, 2017

@Quetzacoalt91 Quetzacoalt91 changed the base branch from 1.7.2.x to develop Sep 26, 2017

adjustments to keep the original loading order
code adjusted as suggested by Quetzacoalt91
@gabdara

This comment has been minimized.

Show comment
Hide comment
@gabdara

gabdara Oct 5, 2017

Contributor

Sorry I forgot to adjust the code.
It should be fine now.

Contributor

gabdara commented Oct 5, 2017

Sorry I forgot to adjust the code.
It should be fine now.

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Oct 5, 2017

Member

That's awesome, thank you!

Member

Quetzacoalt91 commented Oct 5, 2017

That's awesome, thank you!

@Quetzacoalt91 Quetzacoalt91 merged commit 40a7e31 into PrestaShop:develop Oct 6, 2017

2 checks passed

codacy/pr Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Quetzacoalt91 Quetzacoalt91 changed the title from fix import products without Force all ID numbers to Fix import products without Force all ID numbers Oct 6, 2017

@gabdara gabdara deleted the gabdara:patch-3 branch Oct 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment