Skip to content
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 pack stock issues #8607

Merged
merged 12 commits into from Mar 26, 2018
Merged

Fix pack stock issues #8607

merged 12 commits into from Mar 26, 2018

Conversation

alegout
Copy link
Contributor

@alegout alegout commented Dec 12, 2017

Questions Answers
Branch? 1.7.3.x.
Description? Fix issue with pack quantity.
Type? bug fix
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? BOOM-4361, BOOM-3487
How to test? 1) in preferences > products > choose decrease both for packs
2) Don't allow to order out of stock product
3) create a product A with a stock of 50
4) create a product B with a stock of 5, this product is a pack of 10 products A
5) You cannot order 50 products A and 5 products B.

Important guidelines


This change is Reviewable

@LittleBigDev
Copy link
Contributor

Reviewed 7 of 7 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


src/PrestaShopBundle/Install/DatabaseDump.php, line 60 at r1 (raw file):

        if ($dumpFile === null) {
            $tmpDirPath = sys_get_temp_dir() . '/prestashop';

I think you didn't want to add this file in the commit ! Please make a separate PR for the DB dump fix.


src/PrestaShopBundle/Tests/Utils/Database.php, line 38 at r1 (raw file):

     * Create the initialize database used for test
     */
    public static function createTestDB($overrideIfExists = true)

I think you didn't want to add this file in the commit ! Please make a separate PR for the DB dump fix.


tests/TestCase/IntegrationTestCase.php, line 56 at r1 (raw file):

    {
        require_once(__DIR__ . '/../../config/config.inc.php');
        Database::restoreTestDB();

I think you didn't want to add this file in the commit ! Please make a separate PR for the DB dump fix.


Comments from Reviewable

Copy link
Contributor

@LittleBigDev LittleBigDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @alegout
Please see my comments above

@alegout
Copy link
Contributor Author

alegout commented Dec 12, 2017

Review status: all files reviewed at latest revision, 3 unresolved discussions.


src/PrestaShopBundle/Install/DatabaseDump.php, line 60 at r1 (raw file):

Previously, LittleBigDev (Bastien Bieri) wrote…

I think you didn't want to add this file in the commit ! Please make a separate PR for the DB dump fix.

Done.


src/PrestaShopBundle/Tests/Utils/Database.php, line 38 at r1 (raw file):

Previously, LittleBigDev (Bastien Bieri) wrote…

I think you didn't want to add this file in the commit ! Please make a separate PR for the DB dump fix.

Done.


tests/TestCase/IntegrationTestCase.php, line 56 at r1 (raw file):

Previously, LittleBigDev (Bastien Bieri) wrote…

I think you didn't want to add this file in the commit ! Please make a separate PR for the DB dump fix.

Done.


Comments from Reviewable

@PrestaShop PrestaShop deleted a comment from prestonBot Dec 12, 2017
@PrestaShop PrestaShop deleted a comment Dec 12, 2017
@mickaelandrieu mickaelandrieu added the 1.7.x Branch label Dec 13, 2017
@LittleBigDev
Copy link
Contributor

:lgtm:


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

LittleBigDev
LittleBigDev previously approved these changes Dec 13, 2017
Copy link
Contributor

@LittleBigDev LittleBigDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @alegout 👍

@LittleBigDev LittleBigDev added the Waiting for QA Status: action required, waiting for test feedback label Dec 13, 2017
@marionf
Copy link
Contributor

marionf commented Dec 14, 2017

Hello @alegout
If I add first 50 products A and try to add 5 packs B, I can't -> It's ok
But if I add first 5 packs B and try to add 50 products A, it works while it shouldn't

@marionf marionf added Waiting for author Status: action required, waiting for author feedback and removed Waiting for QA Status: action required, waiting for test feedback labels Dec 14, 2017
@alegout alegout added Waiting for QA Status: action required, waiting for test feedback and removed Waiting for author Status: action required, waiting for author feedback labels Jan 9, 2018
@marionf marionf removed the Waiting for QA Status: action required, waiting for test feedback label Jan 9, 2018
@jocel1
Copy link
Contributor

jocel1 commented Jan 9, 2018

Review status: 0 of 8 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


classes/Cart.php, line 1175 at r3 (raw file):

        if (Configuration::get('PS_ALLOW_MULTISHIPPING') && $this->isMultiAddressDelivery()) {
            $firstUnionSql .= ' AND cp.`id_address_delivery` = '.(int)$id_address_delivery;
            $secondUnionSql .= ' AND cp.`id_address_delivery` = '.(int)$id_address_delivery;

why not putting those in the commonWhere, and add the commonWhere later ?


classes/Cart.php, line 1180 at r3 (raw file):

        if ($id_customization) {
            $firstUnionSql .= ' AND c.`id_customization` = '.(int)$id_customization;
            $secondUnionSql .= ' AND c.`id_customization` = '.(int)$id_customization;

Same comment here, move it into the commonWhere


Comments from Reviewable

@jocel1
Copy link
Contributor

jocel1 commented Jan 9, 2018

Reviewed 8 of 8 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


Comments from Reviewable

@PrestaShop PrestaShop deleted a comment Feb 8, 2018
@alegout
Copy link
Contributor Author

alegout commented Feb 8, 2018

Review status: 3 of 9 files reviewed at latest revision, 3 unresolved discussions.


classes/Cart.php, line 1175 at r3 (raw file):

Previously, jocel1 (Jocelyn Fournier) wrote…

why not putting those in the commonWhere, and add the commonWhere later ?

Done.


classes/Cart.php, line 1180 at r3 (raw file):

Previously, jocel1 (Jocelyn Fournier) wrote…

Same comment here, move it into the commonWhere

Done.


classes/Cart.php, line 1186 at r3 (raw file):

            COALESCE(SUM(first_level_quantity), 0) as quantity 
          FROM (' . $firstUnionSql . ' UNION ' . $secondUnionSql . ') as q';
        $result = Db::getInstance()->getRow($parentSql);

lulz


Comments from Reviewable

@marionf marionf added Waiting for author Status: action required, waiting for author feedback and removed Waiting for QA Status: action required, waiting for test feedback labels Mar 6, 2018
@alegout alegout added Waiting for QA Status: action required, waiting for test feedback and removed Waiting for author Status: action required, waiting for author feedback labels Mar 7, 2018
@marionf marionf removed the Waiting for QA Status: action required, waiting for test feedback label Mar 13, 2018
@eternoendless
Copy link
Member

Reviewed 1 of 1 files at r9.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


Comments from Reviewable

@eternoendless eternoendless added the Waiting for QA Status: action required, waiting for test feedback label Mar 16, 2018
@marionf
Copy link
Contributor

marionf commented Mar 19, 2018

@alegout
I choose to decrement pack only.
I add 5 products B & I try to add 50 products A -> I can, it's ok
I add 50 products A & I try to add 5 products B -> I can't, it's not ok It should be possible

@marionf marionf added Waiting for author Status: action required, waiting for author feedback and removed Waiting for QA Status: action required, waiting for test feedback labels Mar 19, 2018
@alegout alegout added WIP and removed Waiting for author Status: action required, waiting for author feedback labels Mar 22, 2018
@marionf marionf added the QA ✔️ Status: check done, code approved label Mar 26, 2018
@marionf
Copy link
Contributor

marionf commented Mar 26, 2018

It's ok for me, thanks you @alegout

@alegout alegout removed the WIP label Mar 26, 2018
@eternoendless
Copy link
Member

Thank you @alegout 🎉

@eternoendless eternoendless merged commit e807248 into PrestaShop:1.7.3.x Mar 26, 2018
@eternoendless eternoendless changed the title Fix pack quantity issue Fix pack quantity issues Mar 26, 2018
@prestonBot prestonBot added the Waiting for wording Status: action required, waiting for wording label Mar 26, 2018
@eternoendless eternoendless removed the Waiting for wording Status: action required, waiting for wording label Mar 26, 2018
@eternoendless eternoendless changed the title Fix pack quantity issues Fix pack stock issues Mar 26, 2018
@prestonBot prestonBot added the Waiting for wording Status: action required, waiting for wording label Mar 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.7.x Branch QA ✔️ Status: check done, code approved Waiting for wording Status: action required, waiting for wording
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants