-
-
Notifications
You must be signed in to change notification settings - Fork 708
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
Allow return or line break in shipping / payment method description #12465
Conversation
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.
Nice one, thanks for your help !
could you do the same for payment methods ? Even though the Steps to reproduce don't mention payments methods, the title of the issue implies we should also be fixing payment methods description.
Yes i can. |
@@ -95,7 +95,7 @@ | |||
%div.checkout-input{"data-shippingmethod-target": "shippingMethodDescription", "data-shippingmethodid": shipping_method.id , style: "display: #{ship_method_is_selected ? 'block' : 'none'}" } | |||
#distributor_address.panel | |||
- if shipping_method.description.present? | |||
%span #{shipping_method.description} | |||
%span #{simple_format(shipping_method.description)} |
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.
Nice simple solution. The simple_format
doesn't sanitise HTML though which would otherwise be done automatically. So your change would introduce a security issue by allowing people to insert malicious HTML. The docs recommend using the h
helper which is an alias for html_escape
. Maybe let's make that clear here:
simple_format(html_escape(shipping_method.description))
And we can write this a bit clearer in HAML:
%span #{simple_format(shipping_method.description)} | |
%span | |
= simple_format(html_escape(shipping_method.description)) |
Actually, the simple_format
call is adding a <p>
tag around the description. That shouldn't be within a span. So maybe we should leave it out:
%span #{simple_format(shipping_method.description)} | |
= simple_format(html_escape(shipping_method.description)) |
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.
Went to read the doc again in the original issue that i posted (https://api.rubyonrails.org/classes/ActionView/Helpers/TextHelper.html#M002285) but it says that simple_format would not sanitize the html presenting a security risk like you said. Thought otherwise.
The comment is so good and clear, learned a bit by reading it, thank you.
(Will add the changes in another time, cause in portugal it's quite a bit late.)
…ew/checkout/payment.html.haml:line 32
I think we still need to test in order to check if the order of the methods is right, but in essence the solution might look like the above commit shows. |
We could make a test that simulates adding mailicious html to check if html_escape is working or it's redundant? |
It's not necessary, |
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.
It looks good now, just one last change see my comment.
@@ -95,7 +95,7 @@ | |||
%div.checkout-input{"data-shippingmethod-target": "shippingMethodDescription", "data-shippingmethodid": shipping_method.id , style: "display: #{ship_method_is_selected ? 'block' : 'none'}" } | |||
#distributor_address.panel | |||
- if shipping_method.description.present? | |||
%span #{shipping_method.description} | |||
= simple_format(html_escape(shipping_method.description)) |
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.
You removed the span
here, it should be :
= simple_format(html_escape(shipping_method.description)) | |
%span | |
= simple_format(html_escape(shipping_method.description)) |
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.
It was my suggestion to remove the span. Is it important? It's just that simple_format is wrapping the content in <p>
and a paragraph is invalid within a span. But maybe we need some CSS changes to make this work?
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.
I think we don't need the span as well. But i can change the code to the desired effect upon settling in a specific style.
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.
It was my suggestion to remove the span. Is it important?
I missed that. I just checked and the span is indeed not important, so it's good as it is.
Need to delete this last commit in github. This last commit introduces code that should not be there. |
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.
It's good now, thanks for your help @MrBowmanXD 🙏
You last commit will be sorted out once we merge into master. Note for next time we prefer to rebase the branch instead of merging master.
Sorry for the late response but this last commit introduces code that should not be there. I can make another pull request if necessary. |
Had to remove the code according to #12443 |
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.
Great, thank you!
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.
Looks good, thanks !
Hey @MrBowmanXD , Thanks for another PR 💪 Before staging it, we can see the bug (left, admin section; right, shopfront): After staging your PR we can see the line breaks are displayed correctly: Awesome!! 🎉 |
Ohh, I just noticed there are some merge conflicts. Can you fix these @MrBowmanXD ? Thanks! |
Check the shipping method description (in checkout details) and the payment method description (in checkout payment). |
What? Why?
When setting up a shipping or payment method description, users are allowed to return to line or add line breaks in the admin side.
However this is not done is the shopfront where everything is only separated with a space. If description are long this can be quite ugly.
(copied from the issue)
Simply added the simple_format method in order to introduce line breaks or return in the shipping method description.
(checkout/_details.html.haml: line98)
What should we test?
Note: Check security issues but simple_format sanitizes the html by default according to the documentation.
Release notes
Changelog Category (reviewers may add a label for the release notes):
The title of the pull request will be included in the release notes.