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?
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 :)
@@ -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?
4c6a82f
to
aa97822
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.
Just a suggestion to use existing const for this folder, not mandatory though
{ | ||
try { | ||
if (file_exists($filePath)) { | ||
move_uploaded_file($filePath, _PS_IMG_DIR_ . 'co/' . $id . '.jpg'); |
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($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'); |
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.
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'); |
private function modifyRecords(array $records): array | ||
{ | ||
foreach ($records as &$record) { | ||
if (file_exists(_PS_IMG_DIR_ . 'co/' . (int) $record['id_attribute'] . '.jpg')) { |
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.
if (file_exists(_PS_IMG_DIR_ . 'co/' . (int) $record['id_attribute'] . '.jpg')) { | |
if (file_exists(_PS_COL_IMG_DIR_ . (int) $record['id_attribute'] . '.jpg')) { |