Allow return or line break in shipping / payment method description #12465
Conversation
rioug
added
the
user facing changes
rioug
changed the title
|
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.
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.
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, |
| @@ -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.
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.
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.
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.
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.
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 |
|
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.