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 stock location through ps_stock_available table #10422

Merged
merged 8 commits into from Sep 20, 2018

Conversation

@matks
Contributor

matks commented Sep 13, 2018

⚠️ PR mainly approved, only last commit 1abf49c needs review

Questions Answers
Branch? develop
Description? Restore the use of the 'stock location' product field
Type? new feature
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #10113
How to test? The expected behavior is described in the issue

This change is Reviewable

Show outdated Hide outdated classes/order/Order.php Outdated
@@ -54,6 +54,9 @@ class StockAvailableCore extends ObjectModel
/** @var bool determine if a product is out of stock - it was previously in Product class */
public $out_of_stock = false;
/** @var string the location of the stock for this product / combination */
public $location = '';

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 16, 2018

Contributor

👍

@mickaelandrieu

mickaelandrieu Sep 16, 2018

Contributor

👍

Show outdated Hide outdated classes/stock/StockAvailable.php Outdated
Show outdated Hide outdated classes/stock/StockAvailable.php Outdated
*
* @return bool|string
*/
public static function getLocation($id_product, $id_product_attribute = null, $id_shop = null)

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 16, 2018

Contributor

$id_shop should be default evaluated to 0, same for $id_product_attribute.

@mickaelandrieu

mickaelandrieu Sep 16, 2018

Contributor

$id_shop should be default evaluated to 0, same for $id_product_attribute.

This comment has been minimized.

@matks

matks Sep 17, 2018

Contributor

Would it introduce some magic numbers inside this function ? I know in PrestaShop ID 0 is the default one. However adding it as default value would hard-code this rule into the code. What if one day we want to move to uuid SQL IDs ? Even if we extend StockAvailable, the value 0 will be hardcoded into the function signature.

So ... what about null as default value always :) ?

@matks

matks Sep 17, 2018

Contributor

Would it introduce some magic numbers inside this function ? I know in PrestaShop ID 0 is the default one. However adding it as default value would hard-code this rule into the code. What if one day we want to move to uuid SQL IDs ? Even if we extend StockAvailable, the value 0 will be hardcoded into the function signature.

So ... what about null as default value always :) ?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 17, 2018

Contributor

if we move from ids to uuid we will have bigger problems as we expected integer or null here and we will change the functions to use some Uuid objects, isn't it?

It's only 1 implementation, I don't think we will break it in 1.7.x release.

Anytime, when we can do it I want only 1 possible type of input (if we were able to use PHP 7.1 already, I could be fine with ?int but ... :'( ).

Other option: check if $id_product_attribute is a int if it's not a null value, wdyt?

@mickaelandrieu

mickaelandrieu Sep 17, 2018

Contributor

if we move from ids to uuid we will have bigger problems as we expected integer or null here and we will change the functions to use some Uuid objects, isn't it?

It's only 1 implementation, I don't think we will break it in 1.7.x release.

Anytime, when we can do it I want only 1 possible type of input (if we were able to use PHP 7.1 already, I could be fine with ?int but ... :'( ).

Other option: check if $id_product_attribute is a int if it's not a null value, wdyt?

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 18, 2018

Contributor

Even if we use only one type, we are able to set a value to null. Which is logic to have when there is no id attached to the object, because 0 is an id :)
If we move to uuid or anything else, it's easier to test if value is null instead of checking if it's numeric and not 0 (and it costs more resources).

<?php

class A
{
    public function __construct(int $a)
    {
        var_dump($a);
    }
}

new A();
new A(22);
new A('azerty');

Will produce:

$ php7.2 test.php 
/home/prambaud/test.php:7:
NULL
/home/prambaud/test.php:7:
int(22)
PHP Fatal error:  Uncaught TypeError: Argument 1 passed to A::__construct() must be of the type integer or null, string given, called in /home/prambaud/test.php on line 13 and defined in /home/prambaud/test.php:5
Stack trace:
#0 /home/prambaud/test.php(13): A->__construct('azerty')
#1 {main}
  thrown in /home/prambaud/test.php on line 5
@PierreRambaud

PierreRambaud Sep 18, 2018

Contributor

Even if we use only one type, we are able to set a value to null. Which is logic to have when there is no id attached to the object, because 0 is an id :)
If we move to uuid or anything else, it's easier to test if value is null instead of checking if it's numeric and not 0 (and it costs more resources).

<?php

class A
{
    public function __construct(int $a)
    {
        var_dump($a);
    }
}

new A();
new A(22);
new A('azerty');

Will produce:

$ php7.2 test.php 
/home/prambaud/test.php:7:
NULL
/home/prambaud/test.php:7:
int(22)
PHP Fatal error:  Uncaught TypeError: Argument 1 passed to A::__construct() must be of the type integer or null, string given, called in /home/prambaud/test.php on line 13 and defined in /home/prambaud/test.php:5
Stack trace:
#0 /home/prambaud/test.php(13): A->__construct('azerty')
#1 {main}
  thrown in /home/prambaud/test.php on line 5
Show outdated Hide outdated classes/stock/StockAvailable.php Outdated
Show outdated Hide outdated src/Adapter/Product/AdminProductWrapper.php Outdated
Show outdated Hide outdated src/Adapter/Product/ProductDataProvider.php Outdated
Show outdated Hide outdated src/PrestaShopBundle/Controller/Admin/ProductController.php Outdated
@matks

This comment has been minimized.

Show comment
Hide comment
@matks

matks Sep 18, 2018

Contributor

@mickaelandrieu PR feedbacks have been implemented in 5b87438 ;)

Contributor

matks commented Sep 18, 2018

@mickaelandrieu PR feedbacks have been implemented in 5b87438 ;)

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Sep 18, 2018

Member

Code looks good, but tests are failing

Member

eternoendless commented Sep 18, 2018

Code looks good, but tests are failing

@matks

This comment has been minimized.

Show comment
Hide comment
@matks

matks Sep 18, 2018

Contributor

@eternoendless I saw 😢 I fixed it by removing some validations in StockAvailable

According to AdminModelAdapterTest we must accept string values for $id_product_attribute and $id_product in getLocation()

Fixed by 9eae1b1, should fix Travis build

Contributor

matks commented Sep 18, 2018

@eternoendless I saw 😢 I fixed it by removing some validations in StockAvailable

According to AdminModelAdapterTest we must accept string values for $id_product_attribute and $id_product in getLocation()

Fixed by 9eae1b1, should fix Travis build

Requested changes have been applied

@Eolia Eolia referenced this pull request Sep 19, 2018

Closed

Add stock location #10166

'id_product_attribute' => $id_product_attribute,
'location' => $location,
];
throw new \InvalidArgumentException(sprintf(

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 19, 2018

Contributor

isn't weird to throw an exception if the data is actually valid?

@mickaelandrieu

mickaelandrieu Sep 19, 2018

Contributor

isn't weird to throw an exception if the data is actually valid?

@ntiepresta ntiepresta self-assigned this Sep 19, 2018

@mbadrani mbadrani assigned mbadrani and unassigned ntiepresta Sep 19, 2018

@mbadrani

This comment has been minimized.

Show comment
Hide comment
@mbadrani

mbadrani Sep 19, 2018

Contributor

when I fill stock location like this

image

... And I access to order page, it display the number of quantity instead of stock location value

image

Contributor

mbadrani commented Sep 19, 2018

when I fill stock location like this

image

... And I access to order page, it display the number of quantity instead of stock location value

image

@mbadrani mbadrani added Bug and removed waiting for QA labels Sep 19, 2018

@matks

This comment has been minimized.

Show comment
Hide comment
@matks

matks Sep 19, 2018

Contributor

@mbadrani Can you clear your cache on this very page (in chrome, open "developer tools", hold click on the "refresh page button" to make a menu appear and then select "clear cache and perform a forced actualisation of the page" ?

On your screenshot I see the row elements are dashed so I think is a an outdated CSS issue

Contributor

matks commented Sep 19, 2018

@mbadrani Can you clear your cache on this very page (in chrome, open "developer tools", hold click on the "refresh page button" to make a menu appear and then select "clear cache and perform a forced actualisation of the page" ?

On your screenshot I see the row elements are dashed so I think is a an outdated CSS issue

@mbadrani

This comment has been minimized.

Show comment
Hide comment
@mbadrani

mbadrani Sep 20, 2018

Contributor

After force refreshing and clear the cache I can reproduce this weired behaviour

Contributor

mbadrani commented Sep 20, 2018

After force refreshing and clear the cache I can reproduce this weired behaviour

@matks

This comment has been minimized.

Show comment
Hide comment
@matks

matks Sep 20, 2018

Contributor

@mbadrani I found an issue with customized data indeed 😉 nice catch !

⚠️ Last commit 1abf49c should fix it, waiting for review now

Contributor

matks commented Sep 20, 2018

@mbadrani I found an issue with customized data indeed 😉 nice catch !

⚠️ Last commit 1abf49c should fix it, waiting for review now

@mbadrani

This comment has been minimized.

Show comment
Hide comment
@mbadrani
Contributor

mbadrani commented Sep 20, 2018

@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud

PierreRambaud Sep 20, 2018

Contributor

I think it's not a bug due to his modification because I sometimes have the same bug on develop.

Contributor

PierreRambaud commented Sep 20, 2018

I think it's not a bug due to his modification because I sometimes have the same bug on develop.

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Sep 20, 2018

Member

We never know, if you look at the symfony debug bar on the video, it seems an error 500 occurred (the icon becomes red).

@mbadrani can you please hover this icon and click on the sha1 related to the call in error? You will get the details.

Member

Quetzacoalt91 commented Sep 20, 2018

We never know, if you look at the symfony debug bar on the video, it seems an error 500 occurred (the icon becomes red).

@mbadrani can you please hover this icon and click on the sha1 related to the call in error? You will get the details.

@matks matks dismissed stale reviews from jolelievre and Quetzacoalt91 via a0f8404 Sep 20, 2018

@mbadrani mbadrani added Code ✔️ and removed waiting for QA labels Sep 20, 2018

@matks

This comment has been minimized.

Show comment
Hide comment
@matks

matks Sep 20, 2018

Contributor

Again nice catch @mbadrani : the bug happens if you empty the "stock location" field. This was considered a not valid dataset by the code. I fixed:

  • the usecase
  • now even if the dataset is not valid, the BO does not freeze the way it does in your video

Technical insights:
Because of 5b87438#diff-6a810cff1b3b082a1b2a3f1dbca30eeeR384 null data was not accepted anymore as a valid input and was consequently rejected using a \InvalidArgumentException. However since that was an ajax call, JSON was expected although it was the HTML symfony error page that was returned, causing JS to crash.

Contributor

matks commented Sep 20, 2018

Again nice catch @mbadrani : the bug happens if you empty the "stock location" field. This was considered a not valid dataset by the code. I fixed:

  • the usecase
  • now even if the dataset is not valid, the BO does not freeze the way it does in your video

Technical insights:
Because of 5b87438#diff-6a810cff1b3b082a1b2a3f1dbca30eeeR384 null data was not accepted anymore as a valid input and was consequently rejected using a \InvalidArgumentException. However since that was an ajax call, JSON was expected although it was the HTML symfony error page that was returned, causing JS to crash.

@matks matks removed the Code ✔️ label Sep 20, 2018

@jolelievre

nice fix!

@mbadrani

This comment has been minimized.

Show comment
Hide comment
@mbadrani

mbadrani Sep 20, 2018

Contributor

Well done guys!

Contributor

mbadrani commented Sep 20, 2018

Well done guys!

@mbadrani mbadrani added the QA ✔️ label Sep 20, 2018

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Sep 20, 2018

Member

Woot! 💪
Let's merge this once travis is green

Member

eternoendless commented Sep 20, 2018

Woot! 💪
Let's merge this once travis is green

@Quetzacoalt91 Quetzacoalt91 merged commit 7d3d33b into PrestaShop:develop Sep 20, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Sep 20, 2018

Member

Thank you @matks

Member

Quetzacoalt91 commented Sep 20, 2018

Thank you @matks

@matks matks deleted the matks:ISSUE-10113_restore-product-location-usage-2 branch Sep 20, 2018

@matks matks removed their assignment Sep 24, 2018

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