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

Integrate product quantities form and related command #22986

Merged
merged 12 commits into from Jan 26, 2021

Conversation

zuk3975
Copy link
Contributor

@zuk3975 zuk3975 commented Jan 25, 2021

Questions Answers
Branch? develop
Description? Part of product page migration. Creates form for product quantities and related Command builder. No styling or javascript, if any needed it will be added in another PR.
Type? refacto
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? part of #14771
How to test? CI ✔️
Possible impacts?

This change is Reviewable

@zuk3975 zuk3975 requested a review from a team as a code owner January 25, 2021 16:15
@prestonBot prestonBot added develop Branch Refactoring Type: Refactoring Waiting for wording Status: action required, waiting for wording labels Jan 25, 2021
@zuk3975 zuk3975 changed the title [WIP] Create product quantities form type and command builder [WIP] Integrate product quantities form and related command Jan 25, 2021
@zuk3975 zuk3975 changed the title [WIP] Integrate product quantities form and related command Integrate product quantities form and related command Jan 25, 2021
*
* @return array<string, mixed>
*/
private function extractQuantityData(ProductForEditing $productForEditing): array
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add unit test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jolelievre mentioned He will introduce tests for this class in one of his PR's

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes you asked the same thing in my PR about SEO This is a valid request so I'll implement them in my PR to keep this one no too big (it already add tests on Providers which was never done until now)

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.

Please add missing unit tests

@matks matks added the migration symfony migration project label Jan 26, 2021
Progi1984
Progi1984 previously approved these changes Jan 26, 2021
@Julievrz Julievrz added Wording ✔️ Status: check done, wording approved and removed Waiting for wording Status: action required, waiting for wording labels Jan 26, 2021
@prestonBot prestonBot added Waiting for wording Status: action required, waiting for wording and removed Wording ✔️ Status: check done, wording approved labels Jan 26, 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.

I think we should use stock as a prefix rather than Quantity It matches the command better and it is a bit wider name than quantity Since stock is more than just a quantity
However I'm surprised I don't see the QuantityType form type integrated in the ProductType form, nor do I see any modif in the twig templates

$defaultLabel = sprintf(
'%s (%s)',
$this->translator->trans('Default', [], 'Admin.Global'),
array_search($this->defaultPackStockType, $choices, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice and efficient 👍

/**
* Builds commands from product quantity form type
*/
final class QuantityCommandBuilder implements ProductCommandBuilderInterface
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
final class QuantityCommandBuilder implements ProductCommandBuilderInterface
final class StockCommandBuilder implements ProductCommandBuilderInterface

*/
public function buildCommand(ProductId $productId, array $formData): array
{
if (!isset($formData['quantities'])) {
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
if (!isset($formData['quantities'])) {
if (!isset($formData['stock'])) {

@@ -63,6 +64,7 @@ public function getData($id)
return [
'id' => $id,
'basic' => $this->extractBasicData($productForEditing),
'quantities' => $this->extractQuantityData($productForEditing),
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
'quantities' => $this->extractQuantityData($productForEditing),
'stock' => $this->extractStockData($productForEditing),

*
* @return array<string, mixed>
*/
private function extractQuantityData(ProductForEditing $productForEditing): array
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes you asked the same thing in my PR about SEO This is a valid request so I'll implement them in my PR to keep this one no too big (it already add tests on Providers which was never done until now)

use Symfony\Component\Validator\Constraints\NotBlank;
use Symfony\Component\Validator\Constraints\Type;

class QuantityType extends TranslatorAwareType
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
class QuantityType extends TranslatorAwareType
class StockType extends TranslatorAwareType

{
$builder
->add('quantity', NumberType::class, [
'label' => $this->trans('Quantity', 'Admin.Catalog.Feature'),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure but I think every inputs need 'required' => false, to integrate smoothly with partial update

Copy link
Contributor

Choose a reason for hiding this comment

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

Not blocking though since it needs to be confirmed in the dedicated PR, besides I believe the required option can be inherited by compound forms So maybe we would only need to set it in ProductType

Comment on lines 1278 to 1279
form.type.sell.product.quantity_type:
class: 'PrestaShopBundle\Form\Admin\Sell\Product\QuantityType'
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
form.type.sell.product.quantity_type:
class: 'PrestaShopBundle\Form\Admin\Sell\Product\QuantityType'
form.type.sell.product.stock_type:
class: 'PrestaShopBundle\Form\Admin\Sell\Product\StockType'

Comment on lines 27 to 28
prestashop.core.form.command_builder.product.quantity_command_builder:
class: PrestaShop\PrestaShop\Core\Form\IdentifiableObject\CommandBuilder\Product\QuantityCommandBuilder
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
prestashop.core.form.command_builder.product.quantity_command_builder:
class: PrestaShop\PrestaShop\Core\Form\IdentifiableObject\CommandBuilder\Product\QuantityCommandBuilder
prestashop.core.form.command_builder.product.stock_command_builder:
class: PrestaShop\PrestaShop\Core\Form\IdentifiableObject\CommandBuilder\Product\StockCommandBuilder

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.

Perfect! 👍

@Progi1984 Progi1984 merged commit 16428e3 into PrestaShop:develop Jan 26, 2021
@Progi1984 Progi1984 added this to the 1.7.8.0 milestone Jan 26, 2021
@Progi1984 Progi1984 removed the Waiting for wording Status: action required, waiting for wording label Feb 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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