Skip to content
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

Fixes #833 Delete all next-gen versions on restore original image #835

Merged
merged 29 commits into from
Mar 15, 2024

Conversation

remyperona
Copy link
Contributor

Description

Correctly delete all next-gen versions on restore original image by passing the second parameter of the methods to true

Fixes #833

Type of change

  • Bug fix (non-breaking change which fixes an issue).

Checklists

Feature validation

  • I validated all the Acceptance Criteria. If possible, provide sreenshots or videos.
  • I triggered all changed lines of code at least once without new errors/warnings/notices.

Code style

  • I wrote self-explanatory code about what it does.
  • I did not introduce unecessary complexity.

Miraeld and others added 28 commits January 3, 2024 14:16
@remyperona remyperona requested a review from a team March 1, 2024 17:39
Base automatically changed from feature/avif to develop March 6, 2024 14:54
@Mai-Saad
Copy link

@jeawhanlee
Copy link
Contributor

Testing in progress...

@jeawhanlee
Copy link
Contributor

Thank you for the PR @Tabrisrp.
During execution of test plan, with webp versions available and avif enabled, when reoptimize with lossless compression is clicked the webp versions are deleted and avif created, While on trunk webp versions is untouched, Do we need to fix this @piotrbak ?

@piotrbak
Copy link

Yes, that sounds like a regression. @Tabrisrp the lossless/smart compression is related to the image optimization only, not the generation of webp/avif, right?

@remyperona
Copy link
Contributor Author

The lossless level also applies to the next-gen versions, so it makes sense to delete and re-create them too

@piotrbak
Copy link

@Tabrisrp Looks like @Mai-Saad and @jeawhanlee were correct here. Currently in 2.2 we're not touching the version that's not enabled.

Is this a big effort to redo this change?

@remyperona
Copy link
Contributor Author

It would mean to go back to the previous state, so the issue this PR is fixing would not be fixed.

Re-optimizing process is first restore the original, then optimize again with new settings, so it uses the same underlying restore methods.

@MathieuLamiot
Copy link
Contributor

@piotrbak how can we align on the expected behavior to move forward? Should we set up a quick call this afternoon or not needed?

@piotrbak
Copy link

@MathieuLamiot I think we're okay with what @Tabrisrp explained.

@Mai-Saad We'll need to update the TCs

Copy link

@Mai-Saad Mai-Saad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mai-Saad Mai-Saad added this pull request to the merge queue Mar 15, 2024
Merged via the queue into develop with commit 4469a2a Mar 15, 2024
6 of 7 checks passed
@Mai-Saad Mai-Saad deleted the fix833-restore branch March 15, 2024 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Restore original image shall delete all existing next-gen independent on what's enabled
8 participants