-
Notifications
You must be signed in to change notification settings - Fork 9.4k
fixed issue #18724 for 2.3-develop #19177
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
Conversation
Hi @Shubham-Webkul. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
@@ -26,7 +26,7 @@ | |||
<td <?= /* @escapeNotVerified */ $block->getLabelProperties() ?>> | |||
<?= $block->escapeHtml($title) ?> | |||
<?php if (!is_null($percent)): ?> | |||
(<?= (float)$percent ?>%) | |||
(<?= /* @escapeNotVerified */ $_order->formatPrice((float)$percent) ?>%) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not verified here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@orlangur because it was reject in Travis CI process due to XSS vulnerability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Shubham-Webkul there should be another annotation, like noEscape
, to indicate escaping is not needed and logic was verified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@orlangur Please let me know if any changes required. |
@orlangur there is any changes required? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Shubham-Webkul thanks, looks great now!
Hi @orlangur, thank you for the review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Shubham-Webkul looks like formatPrice
is not suitable here as it adds unnecessary dollar sign:
Problem: Percent value is shown with an unnecessary sign
Manual testing scenario:
1: Login to magento admin;
2: Set in admin locale Germany or other European country where decimal character is comma;
3: In Admin -> Stores -> Configuration -> Sales > Tax -> Shopping Cart Display Settings -> Display Full Tax Summary = Yes and in the other section "Orders, Invoices, Credit Memos Display Settings";
4: Set a tax percent in admin with decimals like 50.1;
5: Add any product to cart and go to “Proceed to Checkout”;
6: Fill all required fields and click Next;
7.Check order summary.
8. In Tax tab percent value shown with an unnecessary sign ($).
Notice: The same problem occurs when:
Place the order and see it in My Account
See email sent about order
Please fix implementation.
now i have fixed $ sign in percentage.
Hi @Shubham-Webkul can you please update the branch and resolve conflicts so we can continue work on the pull request. |
Closing due to inactivity. Feel free to reach me out anytime later if you wish to continue work on this pull request and it will be reopened. |
Hi @Shubham-Webkul, thank you for your contribution! |
Description (*)
There was a issue in display tax percent if select any locale where comma(,) use instead of decimal (.) for an example Germany.
Fixed Issues (if relevant)
Manual testing scenarios (*)
1: login magento admin
2: Set in admin locale Germany or other European country where decimal character is comma
3: In Admin > Stores > Configuration > Sales > Tax > Shopping Cart Display Settings > Display Full Tax Summary = Yes and in the other section "Orders, Invoices, Credit Memos Display Settings"
4: Set a tax percent in admin with decimals like 50.1.
5: Add any product to cart and goto cart page.
6: Check order summary.
Contribution checklist (*)