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.

Sign up for GitHub

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

Jump to bottom

magento/magento2-page-builder#842: Template Preview Images Incorrectly Saved to Media Directory #843

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

magento/magento2-page-builder#842: Template Preview Images Incorrectly Saved to Media Directory #843

wants to merge 1 commit into from

Conversation

bluemwhitew
Copy link
Contributor

@bluemwhitew bluemwhitew commented Feb 1, 2023 •
edited

Description (*)

This fixes an issue where Page Builder would save its preview images into the top-level media directory, as opposed to within its designated .template-manager subdirectory. It also contains some minor refactoring.

Story

N/A

Bug

N/A

Task

N/A

Fixed Issues

  1. Template Preview Images Incorrectly Saved to Media Directory #842: Template Preview Images Incorrectly Saved to Media Directory

Builds

N/A

Related Pull Requests

N/A

Manual Testing Scenarios (*)

  • Validate Saved to Directory
  1. Click on any Page Builder-enabled field (doesn't have to be empty)
  2. Click Save as Template
  3. Define a Template Name (such as "Example")
  4. Click Save
  5. Verify that "The current contents of Page Builder has been successfully saved as a template."
  6. Click Apply Template
  7. Inspect the Preview Image path (can be full-size or thumbnail) of the template you just saved
  8. Validate that the image was saved within the .template-manager directory
  • Validate Deleted from Directory
  1. Follow Steps 1-8 from the "Validate Saved to Directory" scenario
  2. Browse to Content » Templates
  3. Open the Preview Image in a new tab ("Open Image in New Tab")
  4. Click Delete
  5. Verify that you see, "Are you sure you want to permanently delete template Example" (where "Example" is your Template Name)
  6. Click OK
  7. Verify that you see, "Template successfully deleted."
  8. Reload the tab from Step 3 pointing to the Preview Image
  9. Validate that it now returns a 404

Questions or Comments

Other minor improvements that can be made here include:

  • amending the messaging from "The current contents of Page Builder has been successfully saved as a template." to "The current contents of Page Builder have been successfully saved as a template."
  • reducing the complexity of the storePreviewImage method by creating helper methods which handle image naming conventions, etc.
  • improving instances of str_replace('.jpg', '-thumb.jpg', ...) so that other file formats (such as PNG) can be used, particularly via data patches.

Checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
  • All automated tests passed successfully (all builds are green)

@bluemwhitew
magento#842: Template Preview Images Incorrectly Saved to Media Direc…
9fd0895 
…tory

- Adding `use function` for `preg_replace`, `str_replace`, `strpos`, `strtolower`, `substr`, and `uniqid`
- Adding Missing `DIRECTORY_SEPARATOR`
- Adding Missing `@throws`
- Fixing `@return` Type(s)
- Making `phpcs:ignore` Less Generic
@m2-community-project m2-community-project bot added this to Ready for Review in Pull Request Progress Feb 1, 2023
@bluemwhitew bluemwhitew added Partner: Vaimo https://www.vaimo.com and removed Progress: ready for review labels Feb 1, 2023
@m2-community-project m2-community-project bot moved this from Ready for Review to On Hold in Pull Request Progress Feb 1, 2023
@m2-community-project m2-community-project bot moved this from On Hold to Ready for Review in Pull Request Progress Feb 1, 2023
@bluemwhitew
Copy link
Contributor Author

@magento run Integration Tests, Unit Tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@bluemwhitew
Copy link
Contributor Author

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@bluemwhitew
Copy link
Contributor Author

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

@bluemwhitew
Copy link
Contributor Author

@dhaecker,

Sorry to ping directly, are these test failures already known (not related to this PR)? 🙏🏻

@dhaecker
Copy link
Contributor

@bluemwhitew
Hi, long time no see!

Those failures are definitely not expected. Looking at the builds, branches being used, etc., I THINK the issue is that magento/magento2-page-builder/ is not synced with magento-commerce/magento2-page-builder/. Looks like the commits are 1 year behind so a LOT of stuff is missing (like php 8.2 compatibility & whatever else)...
I'll ask around & see how this can be fixed

@dhaecker
Copy link
Contributor

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

@dhaecker
Copy link
Contributor

@bluemwhitew
i think i fixed the issue. magento/magento2-page-builder/develop was out of sync with magento-commerce/magento2-page-builder/develop. it is now in sync. however, you now need to merge in latest develop into your branch. once you do that, i THINK the builds should work

direct message me again if there are more problems (otherwise I might miss it)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Partner: Vaimo https://www.vaimo.com Progress: ready for review
Projects
Pull Request Progress
  
Ready for Review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@bluemwhitew @dhaecker