Add texture in attribute color #36068

tleon · 2024-05-02T08:05:11Z

Questions	Answers
Branch?	develop
Description?	The texture box was not doing anyting and seemed no to be migrated from old page..
Type?	bug fix
Category?	BO
BC breaks?	no
Deprecations?	no
How to test?	See #35591 bug 6
UI Tests	https://github.com/tleon/ga.tests.ui.pr/actions/runs/9117701983
Fixed issue or discussion?	Fixes #35591
Sponsor company	PrestaShop SA

prestonBot · 2024-05-02T08:05:16Z

Hi, thanks for this contribution!

I found some issues with the Pull Request description:

Your pull request does not seem to fix any issue, consider creating one (see note below) and linking it by writing Fixes #1234.

Would you mind having a look at it? This will help us understand how interesting your contribution is, thank you very much!

About linked issues

Please consider opening an issue before submitting a Pull Request:

If it's a bug fix, it helps maintainers verify that the bug is effectively due to a defect in the code, and that it hasn't been fixed already.
It can help trigger a discussion about the best implementation path before a single line of code is written.
It may lead the Core Product team to mark that issue as a priority, further attracting the maintainers' attention.

(Note: this is an automated message, but answering it will reach a real human)

jolelievre

Thanks @tleon

all good for the grid part, the format part however shouldn't handle the upload in the controller, it must be the responsibility of the CQRS commands

src/Core/Form/IdentifiableObject/DataHandler/AttributeFormDataHandler.php

jolelievre

Thanks @tleon

A few comments here and there. However I have a question, do we need the possibility to remove a texture that was previously uploaded in the form? Because it seems like the command can handle the replacement of an image, but not the removal no?

src/Adapter/File/Uploader/AttributeFileUploader.php

src/Adapter/Attribute/CommandHandler/AddAttributeHandler.php

src/Core/Domain/AttributeGroup/Attribute/Command/AddAttributeCommand.php

src/Core/Domain/AttributeGroup/Attribute/Command/EditAttributeCommand.php

tests/Integration/Behaviour/Features/Context/Domain/AttributeFeatureContext.php

tests/Integration/Behaviour/Features/Scenario/Attribute/attribute_management.feature

jolelievre

Thanks @tleon

just one last comment about what the responsibility between the handler and the uploader service. And you didn't answer my question about about the removal of a texture is handled?

src/Adapter/Attribute/CommandHandler/AddAttributeHandler.php

boherm

LGTM, just few comments :)

src/Adapter/File/Uploader/AttributeFileUploader.php

src/Core/Grid/Data/Factory/AttributeGridDataFactory.php

boherm · 2024-05-21T16:15:11Z

src/PrestaShopBundle/Resources/config/services/adapter/attribute.yml

@@ -15,3 +15,5 @@ services:
  PrestaShop\PrestaShop\Adapter\Attribute\CommandHandler\EditAttributeHandler: ~
  PrestaShop\PrestaShop\Adapter\Attribute\QueryHandler\GetAttributeForEditingHandler: ~
  PrestaShop\PrestaShop\Adapter\Attribute\Validate\AttributeValidator: ~
+  PrestaShop\PrestaShop\Adapter\File\Uploader\AttributeFileUploader: ~
+  PrestaShop\PrestaShop\Core\Domain\AttributeGroup\Attribute\AttributeFileUploaderInterface: ~


I think this isn't necessary, no?

jolelievre

Just a suggestion to use existing const for this folder, not mandatory though

jolelievre · 2024-05-24T18:19:25Z

src/Adapter/File/Uploader/AttributeFileUploader.php

+    {
+        try {
+            if (file_exists($filePath)) {
+                move_uploaded_file($filePath, _PS_IMG_DIR_ . 'co/' . $id . '.jpg');


Suggested change

move_uploaded_file($filePath, _PS_IMG_DIR_ . 'co/' . $id . '.jpg');

move_uploaded_file($filePath, _PS_COL_IMG_DIR_ . $id . '.jpg');

jolelievre · 2024-05-24T18:19:43Z

src/Adapter/File/Uploader/AttributeFileUploader.php

+        if (file_exists(_PS_IMG_DIR_ . 'co/' . $id . '.jpg')) {
+            unlink(_PS_IMG_DIR_ . 'co/' . $id . '.jpg');


Suggested change

if (file_exists(_PS_IMG_DIR_ . 'co/' . $id . '.jpg')) {

unlink(_PS_IMG_DIR_ . 'co/' . $id . '.jpg');

if (file_exists(_PS_COL_IMG_DIR_ . $id . '.jpg')) {

unlink(_PS_COL_IMG_DIR_ . $id . '.jpg');

jolelievre · 2024-05-24T18:19:58Z

src/Core/Grid/Data/Factory/AttributeGridDataFactory.php

+    private function modifyRecords(array $records): array
+    {
+        foreach ($records as &$record) {
+            if (file_exists(_PS_IMG_DIR_ . 'co/' . (int) $record['id_attribute'] . '.jpg')) {


Suggested change

if (file_exists(_PS_IMG_DIR_ . 'co/' . (int) $record['id_attribute'] . '.jpg')) {

if (file_exists(_PS_COL_IMG_DIR_ . (int) $record['id_attribute'] . '.jpg')) {

matks

LGTM

paulnoelcholot · 2024-05-29T08:26:23Z

Hello @tleon,

I tested your PR and it's OK for me! 🎉

Thanks!

ps-jarvis · 2024-05-29T08:27:23Z

QA approved, well done! Message to the maintainers: do not forget to milestone it before the merge.

fix(bo): add texture in attribute color

59379c0

tleon self-assigned this May 2, 2024

prestonBot added develop Branch Bug fix Type: Bug fix labels May 2, 2024

Fixed the file move

1d5a4a9

tleon force-pushed the Issue-35591-bug-6-cant-add-texture branch 2 times, most recently from cd6b774 to 8eb24f9 Compare May 2, 2024 13:37

tleon marked this pull request as ready for review May 2, 2024 14:29

tleon requested a review from a team as a code owner May 2, 2024 14:29

tleon changed the title ~~fix(bo): add texture in attribute color~~ Add texture in attribute color May 2, 2024

nicosomb previously approved these changes May 2, 2024

View reviewed changes

jolelievre requested changes May 3, 2024

View reviewed changes

src/Core/Form/IdentifiableObject/DataHandler/AttributeFormDataHandler.php Outdated Show resolved Hide resolved

jolelievre mentioned this pull request May 13, 2024

Regression on the migrated Attributes pages #35591

Closed

20 tasks

tleon dismissed nicosomb’s stale review via 388198d May 16, 2024 15:58

tleon force-pushed the Issue-35591-bug-6-cant-add-texture branch 2 times, most recently from 388198d to 8a0bffa Compare May 16, 2024 16:12

tleon requested review from jolelievre and nicosomb May 20, 2024 07:14

nicosomb previously approved these changes May 20, 2024

View reviewed changes

jolelievre reviewed May 20, 2024

View reviewed changes

tleon dismissed nicosomb’s stale review via 4c6a82f May 20, 2024 13:09

tleon force-pushed the Issue-35591-bug-6-cant-add-texture branch from 8a0bffa to 4c6a82f Compare May 20, 2024 13:09

tleon closed this May 20, 2024

tleon reopened this May 20, 2024

jolelievre reviewed May 20, 2024

View reviewed changes

src/Adapter/Attribute/CommandHandler/AddAttributeHandler.php Outdated Show resolved Hide resolved

boherm reviewed May 21, 2024

View reviewed changes

fix(bo): fix img path for bo

aa97822

tleon force-pushed the Issue-35591-bug-6-cant-add-texture branch from 4c6a82f to aa97822 Compare May 24, 2024 15:00

jolelievre approved these changes May 24, 2024

View reviewed changes

matks approved these changes May 28, 2024

View reviewed changes

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

boherm approved these changes May 29, 2024

View reviewed changes

paulnoelcholot approved these changes May 29, 2024

View reviewed changes

paulnoelcholot added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels May 29, 2024

boherm added this to the 9.0.0 milestone May 29, 2024

tleon merged commit 444feff into PrestaShop:develop May 29, 2024
38 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add texture in attribute color #36068

Add texture in attribute color #36068

tleon commented May 2, 2024 •

edited

prestonBot commented May 2, 2024

jolelievre left a comment

jolelievre left a comment

jolelievre left a comment

boherm left a comment

boherm May 21, 2024

jolelievre left a comment

jolelievre May 24, 2024

jolelievre May 24, 2024

jolelievre May 24, 2024

matks left a comment

paulnoelcholot commented May 29, 2024

ps-jarvis commented May 29, 2024

	move_uploaded_file($filePath, _PS_IMG_DIR_ . 'co/' . $id . '.jpg');
	move_uploaded_file($filePath, _PS_COL_IMG_DIR_ . $id . '.jpg');

		if (file_exists(_PS_IMG_DIR_ . 'co/' . $id . '.jpg')) {
		unlink(_PS_IMG_DIR_ . 'co/' . $id . '.jpg');

	if (file_exists(_PS_IMG_DIR_ . 'co/' . (int) $record['id_attribute'] . '.jpg')) {
	if (file_exists(_PS_COL_IMG_DIR_ . (int) $record['id_attribute'] . '.jpg')) {

Add texture in attribute color #36068

Add texture in attribute color #36068

Conversation

tleon commented May 2, 2024 • edited

prestonBot commented May 2, 2024

About linked issues

jolelievre left a comment

Choose a reason for hiding this comment

jolelievre left a comment

Choose a reason for hiding this comment

jolelievre left a comment

Choose a reason for hiding this comment

boherm left a comment

Choose a reason for hiding this comment

boherm May 21, 2024

Choose a reason for hiding this comment

jolelievre left a comment

Choose a reason for hiding this comment

jolelievre May 24, 2024

Choose a reason for hiding this comment

jolelievre May 24, 2024

Choose a reason for hiding this comment

jolelievre May 24, 2024

Choose a reason for hiding this comment

matks left a comment

Choose a reason for hiding this comment

paulnoelcholot commented May 29, 2024

ps-jarvis commented May 29, 2024

tleon commented May 2, 2024 •

edited