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 5 commits 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 5 commits into from

Conversation

bluemwhitew
Copy link
Contributor

@bluemwhitew bluemwhitew commented Feb 1, 2023 •
edited
Loading

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
@bluemwhitew bluemwhitew added Partner: Vaimo https://www.vaimo.com and removed Progress: ready for review labels 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)

@Priyakshic Priyakshic added the Project: Community Picked PRs upvoted by the community label Oct 15, 2024
@engcom-Hotel engcom-Hotel self-requested a review October 21, 2024 09:21
@engcom-Hotel
Copy link
Collaborator

@magento run all tests

engcom-Hotel
engcom-Hotel requested changes Oct 21, 2024
View reviewed changes
Copy link
Collaborator

@engcom-Hotel engcom-Hotel left a comment

Choose a reason for hiding this comment

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

Hello @bluemwhitew,

Thanks for the contribution!

The changes seems good to me, but please fix the static tests failures.

Thanks

@engcom-Hotel
Copy link
Collaborator

@magento run Functional Tests B2B, Static Tests

@engcom-Hotel
Copy link
Collaborator

@magento run Functional Tests B2B, Static Tests

@engcom-Hotel engcom-Hotel added Progress: ready for testing Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it and removed Progress: needs update labels Oct 23, 2024
@engcom-Hotel
Copy link
Collaborator

@magento run all tests

@engcom-Hotel
Copy link
Collaborator

✔️ QA Passed

Preconditions:
Enable PageBuilder

Manual testing scenario:

  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

Before: ✖️

image
image

Actual Result: ✔️
Image(s) should stored under ~/pub/media/.template-manager/

After: ✔️
image
image

Tested all the manual scenarios, no impact on regression testing.

@engcom-Hotel
Copy link
Collaborator

@magento run Functional Tests B2B, Functional Tests EE, Functional Tests CE

@engcom-Hotel
Copy link
Collaborator

@magento run Functional Tests B2B

4 similar comments
@engcom-Hotel
Copy link
Collaborator

@magento run Functional Tests B2B

@engcom-Hotel
Copy link
Collaborator

@magento run Functional Tests B2B

@bluemwhitew
Copy link
Contributor Author

@magento run Functional Tests B2B

@engcom-Hotel
Copy link
Collaborator

@magento run Functional Tests B2B

@engcom-Hotel
Copy link
Collaborator

Moving the PR in Merge in progress

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@engcom-Hotel engcom-Hotel engcom-Hotel approved these changes

Assignees

@engcom-Hotel engcom-Hotel

Labels
Partner: Vaimo https://www.vaimo.com Progress: ready for testing Project: Community Picked PRs upvoted by the community Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it
Projects
Community Dashboard - PageBuilder
Status: Merge in Progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bluemwhitew @dhaecker @engcom-Hotel @Priyakshic