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
Add texture in attribute color #36068
base: develop
Are you sure you want to change the base?
Add texture in attribute color #36068
Conversation
tleon
commented
May 2, 2024
•
edited
edited
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 |
Hi, thanks for this contribution! I found some issues with the Pull Request description:
Would you mind having a look at it? This will help us understand how interesting your contribution is, thank you very much! About linked issuesPlease consider opening an issue before submitting a Pull Request:
(Note: this is an automated message, but answering it will reach a real human) |
cd6b774
to
8eb24f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
388198d
to
8a0bffa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/Core/Domain/AttributeGroup/Attribute/Command/AddAttributeCommand.php
Outdated
Show resolved
Hide resolved
src/Core/Domain/AttributeGroup/Attribute/Command/EditAttributeCommand.php
Outdated
Show resolved
Hide resolved
tests/Integration/Behaviour/Features/Context/Domain/AttributeFeatureContext.php
Outdated
Show resolved
Hide resolved
tests/Integration/Behaviour/Features/Scenario/Attribute/attribute_management.feature
Outdated
Show resolved
Hide resolved
8a0bffa
to
4c6a82f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@@ -68,6 +70,13 @@ public function handle(AddAttributeCommand $command): AttributeId | |||
|
|||
$id = $this->attributeRepository->add($attribute); | |||
|
|||
if (file_exists($command->getTextureFilePath())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just few comments :)
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)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
/** | ||
* @var GridDataFactoryInterface | ||
*/ | ||
private $attributeDataFactory; | ||
|
||
private ImageFolderProvider $imageFolderProvider; | ||
|
||
/** | ||
* @param GridDataFactoryInterface $attributeDataFactory | ||
*/ | ||
public function __construct(GridDataFactoryInterface $attributeDataFactory, ImageFolderProvider $imageFolderProvider) | ||
{ | ||
$this->attributeDataFactory = $attributeDataFactory; | ||
$this->imageFolderProvider = $imageFolderProvider; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use properties promotion here?
@@ -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: ~ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this isn't necessary, no?