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

Introduce UpdateCombinationFromListCommand #22277

Merged

Conversation

zuk3975
Copy link
Contributor

@zuk3975 zuk3975 commented Dec 7, 2020

Questions Answers
Branch? develop
Description? Introduce UpdateCombinationFromListCommand & handler + cover with tests. Also fix previously merged pr naming - cobmbination weight renamed to impactOnWeight. Remove advanced stock management handling in ProductStockUpdater and related test cases. Deprecate advanced_stock_management related properties
Type? refacto
Category? BO
BC breaks? no
Deprecations? yes
Fixed ticket? part of #14773
How to test? Travis ✔️

This change is Reviewable

⚠️ DEPRECATIONS
StockAvailable related deprecations (#23076)

  • Product->quantity & Combination->quantity. Use StockAvailable->quantity instead
  • Product->out_of_stock. Use StockAvailable->out_of_stock instead

Advanced stock related deprecations

  • Product->use_advanced_stock_management - advanced stock feature is no longer maintained (it was removed in ~1.7)
  • Product->depends_on_stock & StockAvailable->depends_on_stock - this property was only relevant with advanced stock feature. That was suggested by following comments found in code and by the fact that usages in code doesn't tell anything against it:
    • "The available quantities for the current product and its combinations are based on the stock in your warehouse (using the advanced stock management system). '")

@zuk3975 zuk3975 requested a review from a team as a code owner December 7, 2020 16:39
@prestonBot prestonBot added develop Branch Refactoring Type: Refactoring labels Dec 7, 2020
@zuk3975 zuk3975 force-pushed the m/product/combination-list-update branch from 07e40a4 to 4c62f0f Compare December 7, 2020 16:46
@Progi1984 Progi1984 changed the title [WIP] Introduce UpdateCombinationFromListCommand [product page migration] [WIP] Introduce UpdateCombinationFromListCommand Dec 8, 2020
Copy link
Contributor

@Progi1984 Progi1984 left a comment

Choose a reason for hiding this comment

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

@matks matks added the migration symfony migration project label Dec 8, 2020
@zuk3975
Copy link
Contributor Author

zuk3975 commented Dec 8, 2020

Travis is https://travis-ci.com/github/PrestaShop/PrestaShop/jobs/455469974#L1152

Thanks, this PR is WIP and is blocked by another PR, so don't bother, I'm aware 😛

@zuk3975 zuk3975 force-pushed the m/product/combination-list-update branch 2 times, most recently from f76fd02 to 3b6e527 Compare February 8, 2021 13:57
@zuk3975 zuk3975 changed the title [WIP] Introduce UpdateCombinationFromListCommand Introduce UpdateCombinationFromListCommand Feb 8, 2021
@@ -92,7 +92,7 @@ public function createCombinations(ProductId $productId, array $groupedAttribute
$this->disableSpecificPriceRulesApplication();

$combinationIds = $this->addCombinations($product, $generatedCombinations);
$this->updateProductDefaultCombination($productId);
$this->combinationRepository->refreshDefaultCombination($productId);
Copy link
Contributor

Choose a reason for hiding this comment

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

This one is not useful any more since in the loop that adds combination you use combinationRepository->create which already calls refreshDefaultCombination internally (at least when no combination is already set as default)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. Actually object model updates default combination after each add/update, so I've removed this completely.

@zuk3975 zuk3975 force-pushed the m/product/combination-list-update branch from df9afd3 to 010d611 Compare February 9, 2021 08:52
Copy link
Contributor

@Progi1984 Progi1984 left a comment

Choose a reason for hiding this comment

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

A small feedback

Progi1984
Progi1984 previously approved these changes Feb 9, 2021
@Progi1984 Progi1984 added the Deprecations Type: Deprecates code label Feb 9, 2021
@Progi1984 Progi1984 added this to the 1.7.8.0 milestone Feb 9, 2021
}

if (null !== $command->getQuantity()) {
//@todo: should we deprecate combination->quantity and product->quantity (as we did with location?) because stock_advanced is the real source
Copy link
Contributor

Choose a reason for hiding this comment

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

@kpodemski do you have some history about it? do you know if it's safe to say stock_advanced is the current standard field for quantity, and combination->quantity and product->quantity are fields used in the past and now deprectaed (but kept for backward compatibility)?

Copy link
Contributor

Choose a reason for hiding this comment

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

@matks

yes, that's true, some ERPs are still integrated into it to put here real product quantity but from what I know this field is not really used in any part of the system anymore, replaced via StockAvailable

use PrestaShop\PrestaShop\Core\Domain\Product\Combination\Command\UpdateCombinationFromListingCommand;

/**
* Defines contract to handle @see UpdateCombinationFromListingCommand
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Defines contract to handle @see UpdateCombinationFromListingCommand
* Defines contract to handle @see UpdateCombinationFromListingCommand
*
* To update combination from listing is the action triggered by a Back Office user from the combinations listing interface.

I'l wondering if we should start to be more descriptive? 🤔

Some Queries/Commands have very explanatory names, they dont need descriptions. But others are more ambiguous.

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case I agree a little bit more description is useful

Copy link
Contributor Author

@zuk3975 zuk3975 Feb 11, 2021

Choose a reason for hiding this comment

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

up until now I thought we describe what command does in the command (not in handler).
UpdateCombinationFromListingCommand has this comment Updates combination in list of combinations. Isn't that enough? Should we start describing usecases inside Handlers from now on?

But, actually I would prefer mentioning handlerInterface inside a command instead of mentioning command inside a handler (because handler already has the command in contract). Then Indeed we could document the handler itself (because that is the class that actually does all the work)

}

/**
* @Given product :productReference should not have a default combination
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @Given product :productReference should not have a default combination
* @Given product :productReference does not have a default combination

*
* @param string $productReference
*/
public function assertProductHasNoCachedDefaultCombination(string $productReference): void
Copy link
Contributor

Choose a reason for hiding this comment

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

assertProductHasNoCachedDefaultCombination
Does that mean we could have a usecase where there is no "cached" default combination but database would have one?

I think (not sure) that Behat tests should ignore cache results 🤔 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is actually related to product->cache_default_attribute field in database.
This field is used as a shortcut to find current default combination id as well as determining if product is combinational

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it's the database field name which is confusing

Progi1984
Progi1984 previously approved these changes Feb 10, 2021
@Progi1984
Copy link
Contributor

@zuk3975 Need rebase ;)

jolelievre
jolelievre previously approved these changes Feb 10, 2021
Copy link
Contributor

@jolelievre jolelievre left a comment

Choose a reason for hiding this comment

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

All good to me, except the suggestions from @matks about comments that can be improved, although it's not blocking to me


$this->combinationStockUpdater->update(
$command->getCombinationId(),
new CombinationStockProperties($command->getQuantity())
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks nice ^^

@@ -42,28 +41,6 @@
*/
class ProductValidator extends AbstractObjectModelValidator
{
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

So much simpler now 😅

use PrestaShop\PrestaShop\Core\Domain\Product\Combination\Command\UpdateCombinationFromListingCommand;

/**
* Defines contract to handle @see UpdateCombinationFromListingCommand
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case I agree a little bit more description is useful

*
* @param string $productReference
*/
public function assertProductHasNoCachedDefaultCombination(string $productReference): void
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it's the database field name which is confusing

zuk3975 and others added 24 commits February 11, 2021 13:47
@zuk3975 zuk3975 dismissed stale reviews from Progi1984 and jolelievre via d75e55b February 11, 2021 11:48
@zuk3975 zuk3975 force-pushed the m/product/combination-list-update branch from 5b4dbca to d75e55b Compare February 11, 2021 11:48
@jolelievre jolelievre merged commit 165a0c6 into PrestaShop:develop Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deprecations Type: Deprecates code develop Branch migration symfony migration project Refactoring Type: Refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants