Create gtin and replace ean13 usages #35697

tleon · 2024-03-20T15:46:46Z

Questions	Answers
Branch?	develop
Description?	Create a Gtin value object and replace ean13 usages to allow 14 characters in the new gtin
Type?	improvement
Category?	BO
BC breaks?	yes
Deprecations?	yes
How to test?	see #25974 for tests scenario.
UI Tests	https://github.com/tleon/ga.tests.ui.pr/actions/runs/8424123779
Fixed issue or discussion?	Fixes #25974
Related PRs	PrestaShop/autoupgrade#680
Sponsor company	PrestaShop SA

BC breaks :

Changed the name and return type of the ean13 property and it's getter / setters in :
- UpdateCombinationCommand.php
- CombinationDetails.php
- UpdateProductCommand.php
- ProductDetails.php

classes/Combination.php

FabienPapet

I see too many BC breaks here. You should / must not rename any method nor arguments.

FabienPapet · 2024-03-25T20:26:36Z

classes/Validate.php

+     *
+     * @return bool
+     */
+    public static function isGtin($gtin): bool


This should construct the value object and the value object should do the validation.

I don't think I should change the way this value object is made. It is supposed to be a copy of the Ean13 Value Object but validated on a 14 lenght.

This is legacy code, we never introduced the VO in this Validation class I don't see why this should change now

This is legacy code,

It maybe is a legacy class, but it is new code, a new method and since you introduced value objects for this, you better use them to avoid copy pasting.

Also, there's one golden rule, the legacy can call the new code, but not the opposite., so it is fine here. You already do this in the email validation

The email is kind of an exception compared to all other validations because it's not a simple regexp But I get your argument, although I don't think we should construct the VO here because it doesn't have the same signature at all (it throws an exception not return a boolean)

But to factorize the regexp at least I guess here we can rely on the one from the VO

FabienPapet · 2024-03-25T20:29:03Z

src/Core/Domain/Product/Combination/QueryResult/CombinationDetails.php

@@ -66,22 +66,22 @@ class CombinationDetails
    private $impactOnWeight;

    /**
-     * @param string $ean13
+     * @param string $gtin comment here explain rename


FabienPapet · 2024-03-25T20:29:27Z

src/Core/Domain/Product/Command/UpdateProductCommand.php

     */
-    public function getEan13(): ?Ean13
+    public function getEan13(): ?Gtin


changing return type is a bc break

Well this contains BC breaks I guess.

FabienPapet · 2024-03-25T20:30:57Z

src/Core/Domain/Product/Combination/QueryResult/CombinationDetails.php

     * @param string $isbn
     * @param string $mpn
     * @param string $reference
     * @param string $upc
     * @param DecimalNumber $impactOnWeight
     */
    public function __construct(
-        string $ean13,
+        string $gtin,


This is also a BC break, with PHP8 you can have named arguments, therefore renaming arguments is a BC.

This is not part of our backward compatibility promise so far https://github.com/PrestaShop/ADR/blob/master/0017-backward-compatibility-promise.md

But you're right we should probably update the ADR for the V9

The BC is acceptable here in favor of the consistency, but it should be detailed in the PR description for future doc update

kpodemski · 2024-03-26T09:13:10Z

install-dev/data/db_structure.sql

@@ -1340,7 +1340,7 @@ CREATE TABLE `PREFIX_order_detail` (
  `reduction_amount_tax_excl` DECIMAL(20, 6) NOT NULL DEFAULT '0.000000',
  `group_reduction` DECIMAL(5, 2) NOT NULL DEFAULT '0.00',
  `product_quantity_discount` decimal(20, 6) NOT NULL DEFAULT '0.000000',
-  `product_ean13` varchar(13) DEFAULT NULL,
+  `product_ean13` varchar(20) DEFAULT NULL,


@tleon why 20 and not 14?

That's a good question we choose to use 20 because even the Gtin is evolving length wise. It use to be 13 now is 14 and could be bigger in the future. I guess the Gtin gets longer as time passes and the uniq combination of barcodes are reached.

src/PrestaShopBundle/Form/Admin/Sell/Product/Details/ReferencesType.php

src/Core/ConstraintValidator/Constraints/TypedRegex.php

Renaming parameter is ok, it's not part of our backward compatibility promise And even if it was this is a major so we're allowed to introduce breaking changes, the consistency prevails in this case

jolelievre

Thanks @tleon

Just two little comment (one optional)

jolelievre · 2024-03-28T09:26:52Z

classes/Validate.php

+     *
+     * @return bool
+     */
+    public static function isGtin($gtin): bool


The email is kind of an exception compared to all other validations because it's not a simple regexp But I get your argument, although I don't think we should construct the VO here because it doesn't have the same signature at all (it throws an exception not return a boolean)

But to factorize the regexp at least I guess here we can rely on the one from the VO

classes/Validate.php

jolelievre · 2024-03-28T09:29:35Z

src/Core/Domain/Product/Combination/QueryResult/CombinationDetails.php

     * @param string $isbn
     * @param string $mpn
     * @param string $reference
     * @param string $upc
     * @param DecimalNumber $impactOnWeight
     */
    public function __construct(
-        string $ean13,
+        string $gtin,


The BC is acceptable here in favor of the consistency, but it should be detailed in the PR description for future doc update

jolelievre · 2024-03-28T09:37:26Z

...on/Behaviour/Features/Context/Domain/Product/Combination/UpdateCombinationFeatureContext.php

@@ -138,7 +138,7 @@ private function fillCommand(UpdateCombinationCommand $command, array $dataRows)
        }
        // References
        if (isset($dataRows['ean13'])) {
-            $command->setEan13($dataRows['ean13']);
+            $command->setGtin($dataRows['ean13']);


Not blocking, but it would have been nice if e update the behat feature scenarios to update this column name If you don't think it's relevant it's fine as well

florine2623

Hello @tleon ,

Saving 14 characters in the field GTIN (EAN, JA, ITF or UCC code) doesn't work :/

13 char OK = 1234567890123
14 char NOK : 12345678901234

Screen.Recording.2024-04-02.at.11.38.54.mov

Could you check ?
Thanks

tleon · 2024-04-02T12:59:33Z

Hello @tleon ,

Saving 14 characters in the field GTIN (EAN, JA, ITF or UCC code) doesn't work :/

13 char OK = 1234567890123 14 char NOK : 12345678901234

Screen.Recording.2024-04-02.at.11.38.54.mov
Could you check ? Thanks

I checked on my side and it is working. Did you reinstall your shop before testing ? ( since the db field for ean has been modified from 13 to 20 length ). You need a fresh install

florine2623

Hello @tleon ,

My mistake. Tested again.
The field allows me to save up to 14 characters.

There's the right error message is my field surpasses the max number of characters.

LGTM ✅
Thanks!

Hlavtox · 2024-04-03T05:25:22Z

@florine2623 @tleon Why is the error sentence doubled?

Hlavtox

Quick question above

matks · 2024-04-03T07:44:36Z

@Hlavtox We've seen this problem before in 2020 -> #28877

It was a nightmare problem where we had to try multiple implementations before finding the right problem. See PRs #29584 and #29959 and #31666 and #31900

I'm quite sure that this plural string bug is unrelated to the code written by @tleon 😉 so I prefer we merge this PR (whose content is valid) and we create another issue for this plural string issue.

What I am saying is: the problem you noticed has been spotted while working on that PR but it does not mark this PR as invalid. I'm sure of that because I worked on that bug in 2020 😄 .

Issue to be created soon after the merge 😉 stay tuned

Dont block that PR for an unrelated bug

tleon · 2024-04-03T14:12:58Z

Issue created #35813

prestonBot added develop Branch Improvement Type: Improvement labels Mar 20, 2024

Hlavtox reviewed Mar 21, 2024

View reviewed changes

classes/Combination.php Show resolved Hide resolved

tleon force-pushed the Issue-25974-allow-ean-14 branch 6 times, most recently from 6f556ad to 243236b Compare March 25, 2024 16:46

tleon marked this pull request as ready for review March 25, 2024 17:54

tleon requested a review from a team as a code owner March 25, 2024 17:54

FabienPapet previously requested changes Mar 25, 2024

View reviewed changes

ps-jarvis added the Waiting for author Status: action required, waiting for author feedback label Mar 25, 2024

FabienPapet reviewed Mar 25, 2024

View reviewed changes

prestonBot added the BC break Type: Introduces a backwards-incompatible break label Mar 26, 2024

tleon force-pushed the Issue-25974-allow-ean-14 branch 2 times, most recently from f218c17 to 93ae1a4 Compare March 26, 2024 08:45

kpodemski reviewed Mar 26, 2024

View reviewed changes

M0rgan01 reviewed Mar 26, 2024

View reviewed changes

src/PrestaShopBundle/Form/Admin/Sell/Product/Details/ReferencesType.php Show resolved Hide resolved

src/Core/ConstraintValidator/Constraints/TypedRegex.php Show resolved Hide resolved

tleon force-pushed the Issue-25974-allow-ean-14 branch from 93ae1a4 to bd1509e Compare March 26, 2024 13:43

tleon requested a review from M0rgan01 March 26, 2024 14:12

Hlavtox mentioned this pull request Mar 27, 2024

Update the ean13 field PrestaShop/autoupgrade#680

Merged

tleon removed the Waiting for author Status: action required, waiting for author feedback label Mar 28, 2024

jolelievre reviewed Mar 28, 2024

View reviewed changes

chore(BO): create gtin and replace ean13 usages

83157f5

tleon force-pushed the Issue-25974-allow-ean-14 branch from bd1509e to 83157f5 Compare March 28, 2024 09:56

jolelievre approved these changes Mar 28, 2024

View reviewed changes

M0rgan01 added this to the 9.0.0 milestone Mar 28, 2024

M0rgan01 approved these changes Mar 28, 2024

View reviewed changes

ps-jarvis added the Waiting for QA Status: action required, waiting for test feedback label Mar 28, 2024

florine2623 self-assigned this Apr 2, 2024

florine2623 reviewed Apr 2, 2024

View reviewed changes

florine2623 added Waiting for author Status: action required, waiting for author feedback and removed Waiting for QA Status: action required, waiting for test feedback labels Apr 2, 2024

florine2623 removed their assignment Apr 2, 2024

prestashop-issue-bot bot removed the Waiting for author Status: action required, waiting for author feedback label Apr 2, 2024

tleon self-assigned this Apr 2, 2024

tleon added the Waiting for QA Status: action required, waiting for test feedback label Apr 2, 2024

florine2623 assigned tleon and florine2623 and unassigned tleon Apr 2, 2024

florine2623 approved these changes Apr 2, 2024

View reviewed changes

florine2623 added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Apr 2, 2024

Hlavtox previously requested changes Apr 3, 2024

View reviewed changes

ps-jarvis added the Waiting for author Status: action required, waiting for author feedback label Apr 3, 2024

matks removed the Waiting for author Status: action required, waiting for author feedback label Apr 3, 2024

matks merged commit a847b9f into PrestaShop:develop Apr 3, 2024
35 checks passed

kpodemski added Documentation ✔️ Developer documentation is up-to-date Needs documentation Needs an update of the developer documentation and removed Documentation ✔️ Developer documentation is up-to-date labels Aug 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create gtin and replace ean13 usages #35697

Create gtin and replace ean13 usages #35697

tleon commented Mar 20, 2024 •

edited by Hlavtox

Loading

FabienPapet left a comment

FabienPapet Mar 25, 2024

tleon Mar 26, 2024

jolelievre Mar 26, 2024

FabienPapet Mar 26, 2024 •

edited

Loading

jolelievre Mar 28, 2024

FabienPapet Mar 25, 2024

FabienPapet Mar 25, 2024

tleon Mar 26, 2024

FabienPapet Mar 25, 2024

jolelievre Mar 26, 2024

jolelievre Mar 28, 2024

kpodemski Mar 26, 2024

tleon Mar 26, 2024

jolelievre left a comment

jolelievre Mar 28, 2024

jolelievre Mar 28, 2024

jolelievre Mar 28, 2024

florine2623 left a comment

tleon commented Apr 2, 2024

florine2623 left a comment

Hlavtox commented Apr 3, 2024

Hlavtox left a comment

matks commented Apr 3, 2024

tleon commented Apr 3, 2024

Create gtin and replace ean13 usages #35697

Create gtin and replace ean13 usages #35697

Conversation

tleon commented Mar 20, 2024 • edited by Hlavtox Loading

FabienPapet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FabienPapet Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jolelievre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

florine2623 left a comment

Choose a reason for hiding this comment

tleon commented Apr 2, 2024

florine2623 left a comment

Choose a reason for hiding this comment

Hlavtox commented Apr 3, 2024

Hlavtox left a comment

Choose a reason for hiding this comment

matks commented Apr 3, 2024

tleon commented Apr 3, 2024

tleon commented Mar 20, 2024 •

edited by Hlavtox

Loading

FabienPapet Mar 26, 2024 •

edited

Loading