Skip to content

Issue #30594 - added preventDefault if EventListner is added for a tag #30628

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

Open
wants to merge 9 commits into
base: 2.4-develop
Choose a base branch
from

Conversation

amitsamsukha
Copy link

Description (*)

renderEventListenerAsTag function of class Magento\Framework\View\Helper\SecureHtmlRenderer
execute/apply the event for tag
so the default event must prevent in this case.

Related Pull Requests

Fixed Issues (if relevant)

  1. Fixes Email Template Preview spawns a pop-up window and also directs user away from email templates grid #30594

Manual testing scenarios (*)

  1. Go to Marketing > Communications > Email Templates
  2. Click Add New Template
  3. Fill out the form and save. You'll be sent over to the Email Templates grid.
  4. Click Preview on the newly or old created Email Template.
  5. It will pop-up a preview window
  6. and parent GRID does not redirect to preview window now.

Questions or comments

Contribution 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)
  • All automated tests passed successfully (all builds are green)

@m2-assistant
Copy link

m2-assistant bot commented Oct 23, 2020

Hi @amitsamsukha. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

  • @magento run all tests - run or re-run all required tests against the PR changes
  • @magento run <test-build(s)> - run or re-run specific test build(s)
    For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE,
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

@amitsamsukha
Copy link
Author

@magento give me test instance

@magento-deployment-service
Copy link

Hi @amitsamsukha. Thank you for your request. I'm working on Magento instance for you.

@magento-deployment-service
Copy link

@amitsamsukha
Copy link
Author

@magento run all tests

@sivaschenko sivaschenko added the Priority: P3 May be fixed according to the position in the backlog. label Oct 23, 2020
@amitsamsukha
Copy link
Author

@magento run Functional Tests CE

@mrtuvn
Copy link
Contributor

mrtuvn commented Oct 24, 2020

Can we have test 2 cases in one PR ?
CC: @sivaschenko @amitsamsukha
I just found another PR have change with same area
See #29991

Copy link
Contributor

@rogyar rogyar left a comment

Choose a reason for hiding this comment

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

Hi @amitsamsukha. Thank you for providing the fix. According to the definition of done all changes should be covered buy autotests.

Could I ask you to cover this fix with an MFTF test that will assert that after clicking the "Preview" link the parent page stays unchanged?

Thank you!

@rogyar
Copy link
Contributor

rogyar commented Oct 24, 2020

Hi @mrtuvn. I would suggest not to merge these two solutions. So, leave the "preventDefault" in this PR and other changes in the mentioned PR. The main reason for that - author of #29991 will have to create an additional test that covers email templates preview. So, it's better to have 2 PRs for 2 different issues.

@amitsamsukha
Copy link
Author

@magento run Functional Tests CE

@amitsamsukha
Copy link
Author

@magento run Functional Tests EE

@amitsamsukha
Copy link
Author

@magento run all tests

@amitsamsukha
Copy link
Author

@magento run all tests

@rogyar
Copy link
Contributor

rogyar commented Oct 30, 2020

Hi @amitsamsukha. Would you take care of the test coverage, please? Please, refer to my comment above

Thank you!

@amitsamsukha
Copy link
Author

@rogyar can you guide me on how to resolve the following issues :
I have branched from the magento:2.4-develop branch and the issue listed is not related.

you can check the below link :
https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/30628/d4dc7d8f35182903927b55cc64fcd78c/Functional/allure-report-b2b/index.html#suites/8187718e2fd144987673ef34ee62b41b/8a85e072d2e7cb9a/

Screenshot 2020-11-02 at 1 03 06 PM

@amitsamsukha
Copy link
Author

@magento run Functional Tests CE

@amitsamsukha
Copy link
Author

@magento give me test instance

@magento-deployment-service
Copy link

Hi @amitsamsukha. Thank you for your request. I'm working on Magento instance for you.

@magento-deployment-service
Copy link

@amitsamsukha
Copy link
Author

@magento give me test instance

@magento-deployment-service
Copy link

Hi @amitsamsukha. Thank you for your request. I'm working on Magento instance for you.

@magento-deployment-service
Copy link

@amitsamsukha
Copy link
Author

@magento run all tests

@amitsamsukha
Copy link
Author

@magento run all tests

@amitsamsukha amitsamsukha requested a review from rogyar November 5, 2020 10:38
@rogyar
Copy link
Contributor

rogyar commented Nov 8, 2020

Hi @amitsamsukha. There's nothing wrong with the existing tests at the moment. I was asking you to add an additional functional test that will emulate all the steps to reproduce the issue and make sure that the issue does not appear anymore after your fix.

Thank you.

@m2-community-project m2-community-project bot added the Severity: S2 Major restrictions or short-term circumventions are required until a fix is available. label Nov 11, 2020
}

script;

return $this->renderTag('script', ['type' => 'text/javascript'], $script, false);
Copy link
Contributor

@mrtuvn mrtuvn Nov 25, 2020

Choose a reason for hiding this comment

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

Hard-code logic check nodename is not good way. IMO
You should add change method _toLinkHtml in https://github.com/magento/magento2/blob/2.4-develop/app/code/Magento/Backend/Block/Widget/Grid/Column/Renderer/Action.php#L143
CC: @rogyar @zaximus84

Copy link
Contributor

Choose a reason for hiding this comment

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

Example
Screenshot from 2020-11-26 06-57-55

Copy link
Author

@amitsamsukha amitsamsukha Dec 2, 2020

Choose a reason for hiding this comment

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

@mrtuvn
thanks for the suggestion, but the preventDefault only works if called within function onclick into:

https://github.com/magento/magento2/blob/2.4-develop/lib/internal/Magento/Framework/View/Helper/SecureHtmlRenderer.php#L116

the issue is resolved by just adding onclick if condition but I have put a double-check for the anchor tag.
let me know if I need to remove the anchor tag check in this.
CC: @rogyar @zaximus84

@amitsamsukha
Copy link
Author

@magento run all tests

@rogyar
Copy link
Contributor

rogyar commented Dec 4, 2020

Hi @amitsamsukha. Please, do not forget about the auto tests mentioned in the following comment.

#30628 (review)

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: View Event: MageCONF CD 2020 Priority: P3 May be fixed according to the position in the backlog. Progress: review Release Line: 2.4 Severity: S2 Major restrictions or short-term circumventions are required until a fix is available.
Projects
Status: Review in Progress
Development

Successfully merging this pull request may close these issues.

Email Template Preview spawns a pop-up window and also directs user away from email templates grid
5 participants