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

Add multiple feature with the same type but different values to product #8271

Merged
merged 9 commits into from Oct 24, 2017

Conversation

@fatmaBouchekoua
Contributor

fatmaBouchekoua commented Aug 24, 2017

Questions Answers
Branch? develop
Description? If you create a product which has the same feature twice when you reload the page the first one disappears. This PR add the possibility to create feature more than once.
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? http://forge.prestashop.com/browse/BOOM-858
How to test? Reinstall Prestashop, then add the same feature to a product more than once in the BO, Go to the FO and check the product details you will see all features in the same table line.

This change is Reviewable

@eternoendless

Looking good! Some small adjustments though

Show outdated Hide outdated controllers/admin/AdminProductsController.php Outdated
Show outdated Hide outdated controllers/admin/AdminProductsController.php Outdated
Show outdated Hide outdated controllers/admin/AdminProductsController.php Outdated
Show outdated Hide outdated controllers/admin/AdminProductsController.php Outdated
Show outdated Hide outdated src/Core/Product/ProductPresenter.php Outdated
Show outdated Hide outdated src/Core/Product/ProductPresenter.php Outdated
Show outdated Hide outdated src/Core/Product/ProductPresenter.php Outdated
Show outdated Hide outdated install-dev/upgrade/sql/1.7.3.0.sql Outdated

your comments have been adressed

@vincentbz

This comment has been minimized.

Show comment
Hide comment
@vincentbz

vincentbz Sep 18, 2017

Contributor

Hi,

It seems to work perfectly :)
image

I've done 3 cases : two predefined features, one predefined + one customized, two customized

Thanks !

Contributor

vincentbz commented Sep 18, 2017

Hi,

It seems to work perfectly :)
image

I've done 3 cases : two predefined features, one predefined + one customized, two customized

Thanks !

@vincentbz vincentbz added QA ✔️ and removed waiting for QA labels Sep 18, 2017

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Oct 16, 2017

Member

Ported on the StarterTheme: PrestaShop/StarterTheme#206

Member

eternoendless commented Oct 16, 2017

Ported on the StarterTheme: PrestaShop/StarterTheme#206

@LittleBigDev

This comment has been minimized.

Show comment
Hide comment
@LittleBigDev

LittleBigDev Oct 17, 2017

Contributor

Reviewed 7 of 8 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


controllers/admin/AdminProductsController.php, line 869 at r2 (raw file):

Fuzzy-checking

Agreed.


controllers/admin/AdminProductsController.php, line 883 at r2 (raw file):

Quoted 7 lines of code…
if (isset($feature['custom_value'][$language['id_lang']])
    && $custom = $feature['custom_value'][$language['id_lang']]
) {
    $product->addFeaturesCustomToDB($idValue, (int)$language['id_lang'], $custom);
} else {
    $product->addFeaturesCustomToDB($idValue, (int)$language['id_lang'], $defaultValue);
}

Maybe you could just define the value to add, and then call addFeaturesCustomToDB() outside the if/else statement.
Better : set this value to default value, and if custom value exists, then replace this value. This way, you can avoid the else part.


install-dev/upgrade/sql/1.7.3.0.sql, line 16 at r2 (raw file):

ALTER TABLE `PREFIX_product_attribute` ADD `low_stock_threshold` INT(10) NULL DEFAULT NULL AFTER `minimal_quantity`;
ALTER TABLE `PREFIX_product_attribute_shop` ADD `low_stock_threshold` INT(10) NULL DEFAULT NULL AFTER `minimal_quantity`;
ALTER TABLE `PREFIX_feature_product` DROP PRIMARY KEY, ADD PRIMARY KEY (`id_feature`, `id_product`, `id_feature_value`);

Missing single blank line at the end of file


src/Core/Product/ProductPresenter.php, line 851 at r2 (raw file):

                } else {
                    $groupedFeatures[$feature['name']] = $feature;
                }

Here again, you can find a way to avoid the else part.


Comments from Reviewable

Contributor

LittleBigDev commented Oct 17, 2017

Reviewed 7 of 8 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


controllers/admin/AdminProductsController.php, line 869 at r2 (raw file):

Fuzzy-checking

Agreed.


controllers/admin/AdminProductsController.php, line 883 at r2 (raw file):

Quoted 7 lines of code…
if (isset($feature['custom_value'][$language['id_lang']])
    && $custom = $feature['custom_value'][$language['id_lang']]
) {
    $product->addFeaturesCustomToDB($idValue, (int)$language['id_lang'], $custom);
} else {
    $product->addFeaturesCustomToDB($idValue, (int)$language['id_lang'], $defaultValue);
}

Maybe you could just define the value to add, and then call addFeaturesCustomToDB() outside the if/else statement.
Better : set this value to default value, and if custom value exists, then replace this value. This way, you can avoid the else part.


install-dev/upgrade/sql/1.7.3.0.sql, line 16 at r2 (raw file):

ALTER TABLE `PREFIX_product_attribute` ADD `low_stock_threshold` INT(10) NULL DEFAULT NULL AFTER `minimal_quantity`;
ALTER TABLE `PREFIX_product_attribute_shop` ADD `low_stock_threshold` INT(10) NULL DEFAULT NULL AFTER `minimal_quantity`;
ALTER TABLE `PREFIX_feature_product` DROP PRIMARY KEY, ADD PRIMARY KEY (`id_feature`, `id_product`, `id_feature_value`);

Missing single blank line at the end of file


src/Core/Product/ProductPresenter.php, line 851 at r2 (raw file):

                } else {
                    $groupedFeatures[$feature['name']] = $feature;
                }

Here again, you can find a way to avoid the else part.


Comments from Reviewable

@LittleBigDev

See Reviewable comments above

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Oct 19, 2017

Member

Fixed!

Member

eternoendless commented Oct 19, 2017

Fixed!

@PrestaShop PrestaShop deleted a comment from codacy-bot Oct 19, 2017

@LittleBigDev

This comment has been minimized.

Show comment
Hide comment
@LittleBigDev

LittleBigDev Oct 20, 2017

Contributor

:lgtm:


Reviewed 3 of 6 files at r3, 2 of 2 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

Contributor

LittleBigDev commented Oct 20, 2017

:lgtm:


Reviewed 3 of 6 files at r3, 2 of 2 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@LittleBigDev

This comment has been minimized.

Show comment
Hide comment
@LittleBigDev

LittleBigDev Oct 20, 2017

Contributor

Travis is failing tho :

The shopping cart
Add product to cart as a guest

  1. "before all" hook:
    element (#blockcart-modal) still not visible after 2500ms
Contributor

LittleBigDev commented Oct 20, 2017

Travis is failing tho :

The shopping cart
Add product to cart as a guest

  1. "before all" hook:
    element (#blockcart-modal) still not visible after 2500ms
@codacy-bot

This comment has been minimized.

Show comment
Hide comment
@codacy-bot

codacy-bot Oct 20, 2017

Codacy Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- controllers/admin/AdminProductsController.php  2
- src/Core/Product/ProductPresenter.php  6
         

Complexity decreasing per file
==============================
+ src/PrestaShopBundle/Model/Product/AdminModelAdapter.php  -5
         

See the complete overview on Codacy

codacy-bot commented Oct 20, 2017

Codacy Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- controllers/admin/AdminProductsController.php  2
- src/Core/Product/ProductPresenter.php  6
         

Complexity decreasing per file
==============================
+ src/PrestaShopBundle/Model/Product/AdminModelAdapter.php  -5
         

See the complete overview on Codacy

@PrestaShop PrestaShop deleted a comment from codacy-bot Oct 20, 2017

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Oct 23, 2017

Member

Reviewed 2 of 8 files at r1, 1 of 6 files at r3, 1 of 2 files at r4, 1 of 1 files at r5, 2 of 2 files at r6, 1 of 1 files at r7.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

Member

eternoendless commented Oct 23, 2017

Reviewed 2 of 8 files at r1, 1 of 6 files at r3, 1 of 2 files at r4, 1 of 1 files at r5, 2 of 2 files at r6, 1 of 1 files at r7.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless
Member

eternoendless commented Oct 24, 2017

Thank you @fatmaBouchekoua

@eternoendless eternoendless merged commit 9fc85b5 into PrestaShop:develop Oct 24, 2017

3 checks passed

codacy/pr Good work! A positive pull request.
Details
code-review/reviewable 8 files reviewed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kpodemski

This comment has been minimized.

Show comment
Hide comment
@kpodemski

kpodemski Nov 3, 2017

Contributor

@eternoendless please make sure to indicate this in Release Note properly for 1.7.3.0, it's a very useful feature highly requested by community

good job

Contributor

kpodemski commented Nov 3, 2017

@eternoendless please make sure to indicate this in Release Note properly for 1.7.3.0, it's a very useful feature highly requested by community

good job

@p2rcoder

This comment has been minimized.

Show comment
Hide comment
@p2rcoder

p2rcoder Jan 28, 2018

What about Webservice API? How to add multiple features using API? And Layered Navigation Block - does it support multiple feature values?

p2rcoder commented Jan 28, 2018

What about Webservice API? How to add multiple features using API? And Layered Navigation Block - does it support multiple feature values?

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