Migrate "Design > Image Settings" #35192

boherm · 2024-01-27T12:55:06Z

Questions	Answers
Branch?	develop
Description?	Migration of "Design > Image Settings" pages in backoffice. This new page is hidden behind a feature flag.
Type?	improvement
Category?	BO
BC breaks?	no
Deprecations?	no
How to test?	1. Enable "Image settings" feature flag. 2. Go to "Design > Image Settings" pages. 3. Use the pages as usual.
UI Tests	https://github.com/boherm/ga.tests.ui.pr/actions/runs/7678776674
Fixed issue or discussion?	#10563
Related PRs
Sponsor company	PrestaShop SA

For now, I have created an adapter to regenerate images, but I think that we should find/create and use an optimized brand new way to do this task. Maybe, we can create an issue about this change!

src/Adapter/ImageThumbnailsRegenerator.php

Hlavtox · 2024-01-29T09:00:24Z

src/Adapter/ImageThumbnailsRegenerator.php

+                        foreach ($configuredImageFormats as $imageFormat) {
+                            // For JPG images, we let Imagemanager decide what to do and choose between JPG/PNG.
+                            // For webp and avif extensions, we want it to follow our command and ignore the original format.
+                            $forceFormat = ($imageFormat !== 'jpg');


Please check the current code, there have been some changes in #34172. :-)

admin-dev/themes/new-theme/js/pages/image-settings/index.ts

M0rgan01 · 2024-01-29T10:53:05Z

admin-dev/themes/new-theme/js/pages/image-settings/index.ts

+      // Else show the format selector...
+      $parentImageFormat.show();
+      // and the formats by the type selected
+      // @ts-ignore


Why is there a @ts-ignore here?

Good question, is it because ot the formatId: any maybe ti should be formatId: int instead?

src/Core/Domain/ImageSettings/Command/BulkDeleteImageTypeCommand.php

src/Core/Domain/ImageSettings/Command/DeleteImageTypeCommand.php

src/Core/Domain/ImageSettings/Command/DeleteImagesFromTypeCommand.php

src/Core/Domain/ImageSettings/ValueObject/ImageTypeId.php

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

src/Core/Grid/Query/ImageTypeQueryBuilder.php

src/Core/Search/Filters/ImageTypeFilters.php

src/PrestaShopBundle/Controller/Admin/Improve/Design/ImageSettingsController.php

M0rgan01 · 2024-01-29T13:52:28Z

Adding tests could also be helpful.

matks

Good for me, nothing important to add to @Hlavtox and @M0rgan01 reviews

I added 1 typo suggestion and I have 1 question.

src/Adapter/ImageThumbnailsRegenerator.php

src/Core/Grid/Definition/Factory/ImageTypeGridDefinitionFactory.php

boherm · 2024-02-09T08:40:46Z

Tests before merging: https://github.com/boherm/ga.tests.ui.pr/actions/runs/7841551459

jolelievre

Hi, sorry for the late review

I have a few questions maybe not blocking but still worth thinking about if we want to maintain consistency. I still need to review the behat part, but the only thing really blocking for me though is the lack of AdminSecurity on one of the controllers

...js/components/grid/extension/action/row/image_type/delete-image-type-row-action-extension.ts

jolelievre · 2024-02-12T12:39:01Z

admin-dev/themes/new-theme/js/pages/image-settings/index.ts

+  $parentImageFormat.hide();
+
+  // On image type change, show the image format by the type selected
+  $selectImage.on('change', (event) => {


Seems like there is a remaining ESlint warning, even though it doesn't seem to be blocking the CI it'd be nice to fix or all the following PRs will have this warning

jolelievre · 2024-02-12T12:40:41Z

admin-dev/themes/new-theme/js/pages/image-settings/index.ts

+      // Else show the format selector...
+      $parentImageFormat.show();
+      // and the formats by the type selected
+      // @ts-ignore


Good question, is it because ot the formatId: any maybe ti should be formatId: int instead?

jolelievre · 2024-02-12T12:45:54Z

src/Adapter/ImageThumbnailsRegenerator.php

+                                if (!file_exists($dir . $image) || !filesize($dir . $image)) {
+                                    $errors[] = $this->translator->trans('Source file does not exist or is empty (%filepath%)', ['%filepath%' => $dir . $image], 'Admin.Design.Notification');
+                                } else {
+                                    if (!LegacyImageManager::resize(


One day it'd be nice to refactor this legacy tool with more modern code, but not the scope of this PR

Like I wrote in the description, PLEASE YES!
Btw, I will create a new issue about this right after merge this PR.

jolelievre · 2024-02-12T12:53:05Z

src/Adapter/Product/Image/Repository/ProductImageRepository.php

+    /**
+     * @return Image[]
+     */
+    public function getAllImages(): array


This one has risks of returning a lot of data on shops with millions of images, but it was maybe already the case in legacy page and could be improved in another topic dedicated to performance?

Yes, that's the first reason to find a better way in the futur about this.

src/PrestaShopBundle/Controller/Admin/Improve/Design/ImageSettingsController.php

src/PrestaShopBundle/Form/Admin/Improve/Design/ImageSettings/DeleteImageTypeType.php

src/PrestaShopBundle/Form/Admin/Improve/Design/ImageSettings/ImageSettingsFormDataProvider.php

src/PrestaShopBundle/Resources/views/Admin/Common/Grid/Columns/Content/pixel_data.html.twig

jolelievre · 2024-02-12T16:03:07Z

src/Core/Domain/ImageSettings/CommandHandler/DeleteImagesFromTypeHandler.php

+        }
+
+        // Delete all images linked to image type
+        $this->deleteImagesFromType('small_default', _PS_IMG_DIR_ . '{c,m,su,p,st}/');


You need to dynamize the hard-coded value here

jolelievre · 2024-02-12T16:03:32Z

src/Core/Domain/ImageSettings/CommandHandler/DeleteImagesFromTypeHandler.php

+     *
+     * @throws ImageTypeException
+     */
+    public function deleteImagesFromType($imageTypeName, $path): void


This should be in a dedicated service

jolelievre

Thanks @boherm

Almost all good, just a typo in the name of a handler to fix

The other suggestions in behat tests are optional so it's up to you

jolelievre · 2024-02-14T12:01:26Z

src/Core/Domain/ImageSettings/CommandHandler/BulkImageTypeHandlerInterface.php

+/**
+ * Defines contract for BulkImageTypeHandler
+ */
+interface BulkImageTypeHandlerInterface


Suggested change

interface BulkImageTypeHandlerInterface

interface BulkDeleteImageTypeHandlerInterface

jolelievre · 2024-02-14T12:01:48Z

src/Core/Domain/ImageSettings/CommandHandler/BulkImageTypeHandler.php

+ * Handles command that bulk delete image types
+ */
+#[AsCommandHandler]
+class BulkImageTypeHandler extends AbstractBulkCommandHandler implements BulkImageTypeHandlerInterface


Suggested change

class BulkImageTypeHandler extends AbstractBulkCommandHandler implements BulkImageTypeHandlerInterface

class BulkDeleteImageTypeHandler extends AbstractBulkCommandHandler implements BulkImageTypeHandlerInterface

tests/Integration/Behaviour/Features/Context/Domain/ImageSettings/ImageTypeContext.php

jolelievre · 2024-02-14T12:09:38Z

tests/Integration/Behaviour/Features/Context/Domain/ImageSettings/ImageTypeContext.php

+        $this->getSharedStorage()->exists($imageTypeName);
+        $imageTypeId = $this->getSharedStorage()->get($imageTypeName);
+        $command = new DeleteImageTypeCommand($imageTypeId);


Suggested change

$this->getSharedStorage()->exists($imageTypeName);

$imageTypeId = $this->getSharedStorage()->get($imageTypeName);

$command = new DeleteImageTypeCommand($imageTypeId);

$command = new DeleteImageTypeCommand($this->getSharedStorage()->get($imageTypeName));

Just a suggestion, but since the get method already checks if the key exist this code could be simplified (same suggestion for other similar use cases in this PR, but also optional) especially since exists simply return a bool and does not trigger an exception so calling it like this doesn't bring anything

jolelievre · 2024-02-14T12:14:03Z

tests/Integration/Behaviour/Features/Scenario/ImageSettings/image_settings.feature

+      | generation-method  | 1             |
+      | picture-max-size   | 5000          |
+      | picture-max-width  | 123           |
+      | picture-max-height | 345           |


Optional, but maybe you could add an EOL

jolelievre

Thanks @boherm

matks · 2024-02-15T09:04:06Z

🎉 😄

boherm requested a review from a team as a code owner January 27, 2024 12:55

prestonBot added develop Branch Improvement Type: Improvement labels Jan 27, 2024

boherm force-pushed the #10563-migrate-bo-design-image-settings-page branch from d7a6576 to abf5f6e Compare January 27, 2024 13:02

nicosomb reviewed Jan 29, 2024

View reviewed changes

src/Adapter/ImageThumbnailsRegenerator.php Show resolved Hide resolved

Hlavtox reviewed Jan 29, 2024

View reviewed changes

M0rgan01 reviewed Jan 29, 2024

View reviewed changes

boherm force-pushed the #10563-migrate-bo-design-image-settings-page branch from abf5f6e to 5b33a0f Compare January 30, 2024 08:28

matks previously approved these changes Jan 30, 2024

View reviewed changes

src/Adapter/ImageThumbnailsRegenerator.php Outdated Show resolved Hide resolved

src/Core/Grid/Definition/Factory/ImageTypeGridDefinitionFactory.php Outdated Show resolved Hide resolved

boherm dismissed matks’s stale review via cc20e1c January 31, 2024 08:07

boherm force-pushed the #10563-migrate-bo-design-image-settings-page branch 2 times, most recently from cc20e1c to 8beb2b1 Compare January 31, 2024 17:22

M0rgan01 added this to the 9.0.0 milestone Feb 1, 2024

boherm force-pushed the #10563-migrate-bo-design-image-settings-page branch from 0058742 to 924c38c Compare February 6, 2024 18:05

matks previously approved these changes Feb 8, 2024

View reviewed changes

M0rgan01 added the migration symfony migration project label Feb 8, 2024

M0rgan01 previously approved these changes Feb 8, 2024

View reviewed changes

boherm added the Waiting for author Status: action required, waiting for author feedback label Feb 9, 2024

WahbiPS removed the Waiting for author Status: action required, waiting for author feedback label Feb 12, 2024

boherm dismissed stale reviews from M0rgan01 and matks via 683dfa2 February 12, 2024 08:44

boherm force-pushed the #10563-migrate-bo-design-image-settings-page branch from 924c38c to 683dfa2 Compare February 12, 2024 08:44

matks previously approved these changes Feb 12, 2024

View reviewed changes

jolelievre requested changes Feb 12, 2024

View reviewed changes

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

jolelievre reviewed Feb 13, 2024

View reviewed changes

boherm dismissed matks’s stale review via 6ad5b77 February 13, 2024 15:42

boherm force-pushed the #10563-migrate-bo-design-image-settings-page branch from 683dfa2 to 6ad5b77 Compare February 13, 2024 15:42

boherm added 13 commits February 13, 2024 16:42

Add feature flag for the new Design > Image Settings pages

7cdb8e4

Add new ImageType Entity and Repository

aad450f

Add adapter for saving image generation configuration

c9c1539

Create adapter service for image regeneration

8ad06f7

Add routing configuration for image settings pages in bo

96f2b48

Add all forms types and data provider for settings

c5874bc

Add all Queries/Commands for images settings pages

48d2325

Add grid for presenting image types

964721b

Add form data handlers for image types and settings

f84319c

Add controller for image settings pages

1c97b48

Add templates for image settings pages

eb3218e

Add some javascript for image settings pages

f04bf9c

Add some tests

869d3d9

boherm force-pushed the #10563-migrate-bo-design-image-settings-page branch from 6ad5b77 to 869d3d9 Compare February 13, 2024 15:43

boherm removed the Waiting for author Status: action required, waiting for author feedback label Feb 14, 2024

boherm requested a review from jolelievre February 14, 2024 08:36

matks previously approved these changes Feb 14, 2024

View reviewed changes

jolelievre requested changes Feb 14, 2024

View reviewed changes

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

Fix after coments

54bd79f

boherm dismissed matks’s stale review via 54bd79f February 14, 2024 17:05

boherm requested a review from jolelievre February 14, 2024 17:06

WahbiPS removed the Waiting for author Status: action required, waiting for author feedback label Feb 15, 2024

matks approved these changes Feb 15, 2024

View reviewed changes

jolelievre approved these changes Feb 15, 2024

View reviewed changes

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

jolelievre removed the Waiting for QA Status: action required, waiting for test feedback label Feb 15, 2024

jolelievre merged commit f88272a into PrestaShop:develop Feb 15, 2024
23 checks passed

boherm deleted the #10563-migrate-bo-design-image-settings-page branch February 15, 2024 14:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate "Design > Image Settings" #35192

Migrate "Design > Image Settings" #35192

boherm commented Jan 27, 2024 •

edited by matks

Hlavtox Jan 29, 2024

M0rgan01 Jan 29, 2024

jolelievre Feb 12, 2024

M0rgan01 commented Jan 29, 2024

matks left a comment

boherm commented Feb 9, 2024

jolelievre left a comment

jolelievre Feb 12, 2024

jolelievre Feb 12, 2024

jolelievre Feb 12, 2024

boherm Feb 12, 2024 •

edited

jolelievre Feb 12, 2024

boherm Feb 12, 2024

jolelievre Feb 12, 2024

jolelievre Feb 12, 2024

jolelievre left a comment

jolelievre Feb 14, 2024

jolelievre Feb 14, 2024

jolelievre Feb 14, 2024

jolelievre Feb 14, 2024

jolelievre left a comment

matks commented Feb 15, 2024

	interface BulkImageTypeHandlerInterface
	interface BulkDeleteImageTypeHandlerInterface

	class BulkImageTypeHandler extends AbstractBulkCommandHandler implements BulkImageTypeHandlerInterface
	class BulkDeleteImageTypeHandler extends AbstractBulkCommandHandler implements BulkImageTypeHandlerInterface

Migrate "Design > Image Settings" #35192

Migrate "Design > Image Settings" #35192

Conversation

boherm commented Jan 27, 2024 • edited by matks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M0rgan01 commented Jan 29, 2024

matks left a comment

Choose a reason for hiding this comment

boherm commented Feb 9, 2024

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

boherm Feb 12, 2024 • edited

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

Choose a reason for hiding this comment

jolelievre left a comment

Choose a reason for hiding this comment

matks commented Feb 15, 2024

boherm commented Jan 27, 2024 •

edited by matks

boherm Feb 12, 2024 •

edited