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

remove script tag #25418

Merged
merged 4 commits into from
Nov 14, 2019
Merged

remove script tag #25418

merged 4 commits into from
Nov 14, 2019

Conversation

torhoehn
Copy link
Contributor

@torhoehn torhoehn commented Oct 31, 2019

Description (*)

Removed script tag, but tooltip still opens correctly.

Fixed Issues (if relevant)

  1. Fixes Product AdditionalOptions "Read more" dots being escaped on invoice/shipment overview pages #25050 : Product AdditionalOptions "Read more" dots being escaping on invoice/shipment overview pages

Manual testing scenarios (*)

  1. Create a product that includes an additionalOption value that is over 55 characters
  2. Place order for product
  3. View invoice for order in customer dashboard

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)

@torhoehn torhoehn requested a review from YevSent as a code owner October 31, 2019 20:31
@m2-assistant
Copy link

m2-assistant bot commented Oct 31, 2019

Hi @torhoehn. 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.3-develop instance - deploy vanilla Magento instance

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

@kalpmehta
Copy link
Contributor

@magento give me test instance

@magento-engcom-team
Copy link
Contributor

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

@magento-engcom-team
Copy link
Contributor

Hi @kalpmehta, here is your new Magento instance.
Admin access: https://pr-25418.instances.magento-community.engineering/admin
Login: admin Password: 123123q

@kalpmehta kalpmehta added Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests Award: bug fix labels Nov 1, 2019
@magento-engcom-team
Copy link
Contributor

Hi @kalpmehta, thank you for the review.
ENGCOM-6223 has been created to process this Pull Request
✳️ @kalpmehta, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

Copy link
Contributor

@dmytro-ch dmytro-ch left a comment

Choose a reason for hiding this comment

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

Hi @torhoehn,
could you please make sure the tooltip is also initialized for order items renderer as well for shipment, invoice and credit memo.
So we would get the same behavior for all tabs.
Screenshot 2019-11-01 at 09 04 29
Screenshot 2019-11-01 at 09 04 38

E.g.
Shipment Items: Magento/Sales/view/frontend/templates/order/shipment/items/renderer/default.phtml
Order Items: Magento/Sales/view/frontend/templates/order/items/renderer/default.phtml

@torhoehn
Copy link
Contributor Author

torhoehn commented Nov 4, 2019

@dmytro-ch You're absolutely right and I added it.

@torhoehn torhoehn requested a review from dmytro-ch November 5, 2019 14:08
@dmytro-ch
Copy link
Contributor

@torhoehn, great, thank you!

@torhoehn
Copy link
Contributor Author

torhoehn commented Nov 7, 2019

@engcom-Bravo This expected behaviour, because until now the fix #25393 is not inside of this branch. If you want I could merge 2.3-develop into this feature, so you we can see if everything works as it should.

@engcom-Bravo
Copy link
Contributor

Ok @torhoehn, please take care from Your side as much as You can on that all the fixes will persist in the develop. Thank You. ✔️ QA Passed.

@torhoehn
Copy link
Contributor Author

torhoehn commented Nov 7, 2019

@magento give me test instance

@magento-engcom-team
Copy link
Contributor

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

@magento-engcom-team
Copy link
Contributor

Hi @torhoehn, here is your new Magento instance.
Admin access: https://pr-25418.instances.magento-community.engineering/admin
Login: admin Password: 123123q

@torhoehn
Copy link
Contributor Author

torhoehn commented Nov 7, 2019

@engcom-Bravo Everything works as expected now!

@magento-engcom-team
Copy link
Contributor

Hi @dmytro-ch, thank you for the review.
ENGCOM-6223 has been created to process this Pull Request

@m2-assistant
Copy link

m2-assistant bot commented Nov 14, 2019

Hi @torhoehn, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@magento-engcom-team magento-engcom-team added this to the Release: 2.3.5 milestone Nov 14, 2019
@torhoehn torhoehn deleted the feature/additional-product-options branch November 14, 2019 14:34
@VladimirZaets VladimirZaets added Auto-Tests: Covered All changes in Pull Request is covered by auto-tests and removed Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests labels Nov 22, 2019
@@ -17,7 +17,7 @@
<?php if (!$block->getPrintStatus()) : ?>
<?php $_formatedOptionValue = $block->getFormatedOptionValue($_option) ?>
<dd<?= (isset($_formatedOptionValue['full_view']) ? ' class="tooltip wrapper"' : '') ?>>
<?= $block->escapeHtml($_formatedOptionValue['value']) ?>
<?= $block->escapeHtml($_formatedOptionValue['value'], ['a', 'img']) ?>

Choose a reason for hiding this comment

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

img tag is not allowed in the html Escaper class:
https://github.com/magento/magento2/blob/2.4-develop/lib/internal/Magento/Framework/Escaper.php#L40

This generates an error The following tag(s) are not allowed: img when go to the Order page in My Account
The line in the Escaper:
https://github.com/magento/magento2/blob/2.4-develop/lib/internal/Magento/Framework/Escaper.php#L457

Probably removing the img from allowed tags passed as the second parameter should be enough.

Choose a reason for hiding this comment

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

I created pull request, that fixed critical error
#29863

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.

8 participants