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?

jolelievre · 2024-05-20T15:35:37Z

src/Adapter/Attribute/CommandHandler/AddAttributeHandler.php

@@ -68,6 +70,13 @@ public function handle(AddAttributeCommand $command): AttributeId

 $id = $this->attributeRepository->add($attribute);

+ if (file_exists($command->getTextureFilePath())) {


It should be the uploader that checks if the file exists, the handler is only here to check if the data exists (if it's not null) Besides I'm not sure but maybe file_exists doesn't like null input?

boherm

LGTM, just few comments :)

boherm · 2024-05-21T16:12:54Z

src/Adapter/File/Uploader/AttributeFileUploader.php

+ try {
+ move_uploaded_file($filePath, _PS_IMG_DIR_ . 'co/' . $id . '.jpg');
+ } catch (FileException $e) {
+ throw new AttributeUploadFailedException(sprintf('Failed to copy the file %s.', $filePath));
+ }


move_uploaded_file function doesn't throw exceptions and return boolean if it worked or not, but anyway for testing purpose, we can't change the try catch with a condition... So, is ok for now but we need to change all this type of lines!

boherm · 2024-05-21T16:14:13Z

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

+ /**
+ * @var GridDataFactoryInterface
+ */
+ private $attributeDataFactory;
+
+ private ImageFolderProvider $imageFolderProvider;
+
+ /**
+ * @param GridDataFactoryInterface $attributeDataFactory
+ */
+ public function __construct(GridDataFactoryInterface $attributeDataFactory, ImageFolderProvider $imageFolderProvider)
+ {
+ $this->attributeDataFactory = $attributeDataFactory;
+ $this->imageFolderProvider = $imageFolderProvider;
+ }


Can we use properties promotion here?

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?

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

Open

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

fix(bo): fix img path for bo

4c6a82f

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

boherm reviewed May 21, 2024

View reviewed changes

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

jolelievre May 20, 2024

boherm left a comment

boherm May 21, 2024

boherm May 21, 2024

boherm May 21, 2024

		@@ -68,6 +70,13 @@ public function handle(AddAttributeCommand $command): AttributeId

		$id = $this->attributeRepository->add($attribute);

		if (file_exists($command->getTextureFilePath())) {

Add texture in attribute color #36068

Are you sure you want to change the base?

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

jolelievre May 20, 2024

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

boherm May 21, 2024

Choose a reason for hiding this comment

boherm May 21, 2024

Choose a reason for hiding this comment

tleon commented May 2, 2024 •

edited