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

Error when adding product in cart or editing quantity #9405

Merged
merged 19 commits into from Aug 24, 2018

Conversation

@PierreRambaud
Contributor

PierreRambaud commented Aug 3, 2018

Questions Answers
Branch? develop
Description? - Do not update cart when cart product quantity is under 0
- Prevent double event trigger on front (keyup and focusout) while pressing ret key
- Fix webpack problem in dev / prod
- Allow code coverage with phpunit
Type? bug fix
Category? FO
BC breaks? no
Deprecations? no
Fixed ticket? Fixed #9552
How to test? Follow ticket instruction. And in cart set 0 in input and press enter. Watch out with this PR, it change Cart.php, so we need many manual tests (front & back)

This change is Reviewable

PierreRambaud added some commits Aug 3, 2018

Cart fixes
    - Do not update cart when cart product quantity is under 0
    - Prevent double event trigger on front (keyup and focusout) while
      pressing ret key
    - Fix webpack problem in dev / prod

@PierreRambaud PierreRambaud added the wip label Aug 3, 2018

@PierreRambaud PierreRambaud changed the title from Error when adding product in cart or editing quantity to [WIP] Error when adding product in cart or editing quantity Aug 3, 2018

PierreRambaud added some commits Aug 7, 2018

@PierreRambaud PierreRambaud removed the wip label Aug 7, 2018

@PierreRambaud PierreRambaud changed the title from [WIP] Error when adding product in cart or editing quantity to Error when adding product in cart or editing quantity Aug 7, 2018

@eternoendless eternoendless added this to the 1.7.5.0 milestone Aug 9, 2018

@eternoendless eternoendless self-assigned this Aug 9, 2018

@eternoendless

Reviewed 10 of 10 files at r1.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @PierreRambaud)


admin-dev/themes/default/webpack.config.js, line 77 at r1 (raw file):

  },
  plugins: [
    new webpack.HotModuleReplacementPlugin(),

This should only be done in development (check webpack config for the new theme)


admin-dev/themes/default/webpack.config.js, line 83 at r1 (raw file):

if (process.env.NODE_ENV === 'production') {
  config.plugins.push(

Not sure if this will work if config is a const


admin-dev/themes/new-theme/webpack.config.js, line 31 at r1 (raw file):

const keepLicense = require('uglify-save-license');

const config = {

Same comment about const


classes/Cart.php, line 1284 at r1 (raw file):


        if (Context::getContext()->customer->id) {
            // The $id_address_delivery is null, use the cart delivery address

Maybe move this comment inside the if?


classes/Cart.php, line 1347 at r1 (raw file):

        }

        if (!$product->available_for_order ||

Please add boolean operators at the start of the line. Also, one statement per line if you break them into more than one line.


classes/Cart.php, line 1382 at r1 (raw file):

                $newProductQuantity = $productQuantity + $quantity;

                if ($cartFirstLevelProductQuantity['quantity'] <= 1 ||

Again, please put boolean operators at the start of the line


tests/Unit/Core/Cart/Adding/Product/AddStandardProductTest.php, line 83 at r1 (raw file):

    }

    /**

I think it's good practice to either:

  • Describe your test using human language like "When the number of products in the cart is increased or decreased, a call to getProductQuantity should report the correct number of items of that type in the cart".
  • Name your test in an assertive way like "testNumberOfProductsInCartIsReportedCorrectlyWhenUpdatingTheirQuantity"

Otherwise it makes it very hard to figure out what was the original intent of the test.


tests/Unit/Core/Cart/Adding/Product/AddStandardProductTest.php, line 107 at r1 (raw file):

    }

    public function updateQuantities()

Please add a Provider suffix to dataProviders


themes/classic/_dev/js/cart.js, line 35 at r1 (raw file):

$(document).ready(() => {
  const productLineInCartSelector = '.js-cart-line-product-quantity';

Watch out, I don't think that the es2015 profile has been set up for babel in this project.

@PierreRambaud

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @eternoendless and @PierreRambaud)


admin-dev/themes/default/webpack.config.js, line 77 at r1 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

This should only be done in development (check webpack config for the new theme)

Done


admin-dev/themes/default/webpack.config.js, line 83 at r1 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Not sure if this will work if config is a const

Yes it works, we don't change the config value, we still play with and hash.
If crashed when we have something like const i = 1; i =2;

@PierreRambaud

Reviewable status: 8 of 10 files reviewed, 9 unresolved discussions (waiting on @eternoendless and @PierreRambaud)


classes/Cart.php, line 1284 at r1 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Maybe move this comment inside the if?

I just remove the useless else statement here, but I can change if you want =)


classes/Cart.php, line 1347 at r1 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Please add boolean operators at the start of the line. Also, one statement per line if you break them into more than one line.

I just remove the useless else statement here, but I can change if you want =)


classes/Cart.php, line 1382 at r1 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Again, please put boolean operators at the start of the line

I just remove the useless else statement here, but I can change if you want =)

@PierreRambaud

Reviewable status: 8 of 10 files reviewed, 9 unresolved discussions (waiting on @eternoendless and @PierreRambaud)


themes/classic/_dev/js/cart.js, line 35 at r1 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Watch out, I don't think that the es2015 profile has been set up for babel in this project.

let was used, so we can use const =)

@PierreRambaud

Reviewable status: 8 of 10 files reviewed, 9 unresolved discussions (waiting on @eternoendless and @PierreRambaud)


tests/Unit/Core/Cart/Adding/Product/AddStandardProductTest.php, line 83 at r1 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

I think it's good practice to either:

  • Describe your test using human language like "When the number of products in the cart is increased or decreased, a call to getProductQuantity should report the correct number of items of that type in the cart".
  • Name your test in an assertive way like "testNumberOfProductsInCartIsReportedCorrectlyWhenUpdatingTheirQuantity"

Otherwise it makes it very hard to figure out what was the original intent of the test.

Done.


tests/Unit/Core/Cart/Adding/Product/AddStandardProductTest.php, line 107 at r1 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Please add a Provider suffix to dataProviders

Done.

@eternoendless

Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @eternoendless)


admin-dev/themes/default/webpack.config.js, line 77 at r1 (raw file):

Previously, PierreRambaud (GoT) wrote…

Done

Still true!


themes/classic/_dev/js/cart.js, line 35 at r1 (raw file):

Previously, PierreRambaud (GoT) wrote…

let was used, so we can use const =)

But it will still fail on older browsers. We can add babel later though.

@eternoendless

Looks good 👍
Don't forget to commit the assets!

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @PierreRambaud)

@eternoendless

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@marionf marionf self-assigned this Aug 23, 2018

@marionf

This comment has been minimized.

Show comment
Hide comment
@marionf

marionf Aug 23, 2018

Contributor

@PierreRambaud Does it solve also this issue #9841 ?

Contributor

marionf commented Aug 23, 2018

@PierreRambaud Does it solve also this issue #9841 ?

@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud
Contributor

PierreRambaud commented Aug 23, 2018

@marionf Yes!

@marionf

This comment has been minimized.

Show comment
Hide comment
@marionf

marionf Aug 23, 2018

Contributor

@PierreRambaud

If I put 2 products in cart, enter 0 in qty field and click out the field, I can see quickly a 1 in the qty field before the deletion of the product

pr9405

Contributor

marionf commented Aug 23, 2018

@PierreRambaud

If I put 2 products in cart, enter 0 in qty field and click out the field, I can see quickly a 1 in the qty field before the deletion of the product

pr9405

@marionf marionf removed their assignment Aug 23, 2018

@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud

PierreRambaud Aug 23, 2018

Contributor

@marionf Is it really important? Do you want me to investigate?

Contributor

PierreRambaud commented Aug 23, 2018

@marionf Is it really important? Do you want me to investigate?

@marionf

This comment has been minimized.

Show comment
Hide comment
@marionf

marionf Aug 23, 2018

Contributor

It doesn't seem to cause any other problems, so it's not necessary :)

Contributor

marionf commented Aug 23, 2018

It doesn't seem to cause any other problems, so it's not necessary :)

@mickaelandrieu mickaelandrieu merged commit c2a71fd into PrestaShop:develop Aug 24, 2018

1 of 3 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
code-review/reviewable 7 files left (eternoendless)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 24, 2018

Contributor

Thanks @PierreRambaud and everyone for the review.

PS: I've linked the related GitHub issue, but I don't know what I should do with this issue :/

Contributor

mickaelandrieu commented Aug 24, 2018

Thanks @PierreRambaud and everyone for the review.

PS: I've linked the related GitHub issue, but I don't know what I should do with this issue :/

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