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

Open external links in product description in a new page #11761

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/assets/javascripts/templates/product_modal.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
%filter-selector{ 'selector-set' => "productPropertySelectors", objects: "[product] | propertiesWithValuesOf" }

.product-description{"ng-if" => "product.description_html"}
%p.text-small{"ng-bind-html" => "::product.description_html"}
%p.text-small{"ng-bind-html" => "::product.description_html", "data-controller" => "add-blank-to-link"}

.columns.small-12.medium-6.large-6.product-img
%img{"ng-src" => "{{::product.largeImage}}", "ng-if" => "::product.largeImage"}
Expand Down
9 changes: 9 additions & 0 deletions app/webpacker/controllers/add_blank_to_link_controller.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import { Controller } from "stimulus";

export default class extends Controller {
connect() {
this.element.querySelectorAll("a").forEach(function (link) {
link.target = "_blank";
});
}
}
24 changes: 24 additions & 0 deletions spec/javascripts/stimulus/add_blank_to_link_controller_test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/**
* @jest-environment jsdom
*/

import { Application } from "stimulus";
import add_blank_to_link_controller from "../../../app/webpacker/controllers/add_blank_to_link_controller";

describe("AddBlankToLink", () => {
beforeAll(() => {
const application = Application.start();
application.register("add-blank-to-link", add_blank_to_link_controller);
});

describe("#add-blank-to-link", () => {
beforeEach(() => {
document.body.innerHTML = `<div data-controller="add-blank-to-link"><a href="www.ofn.com">www.ofn.com</a></div>`;
});

it("adds target='_blank' to anchor tags", () => {
const anchorTag = document.querySelector('a')
expect(anchorTag.getAttribute('target')).toBe("_blank");
});
});
});
48 changes: 27 additions & 21 deletions spec/system/consumer/shopping/products_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -76,15 +76,23 @@
product.description = '<p><b>Formatted</b> product description: Lorem ipsum dolor sit amet,
consectetur adipiscing elit. Morbi venenatis metus diam,
eget scelerisque nibh auctor non. </p> Link to an ' \
'<a href="http://google.fr" target="_blank">external site</a>' \
'<a href="http://google.fr">external site</a>' \
'<img src="https://www.openfoodnetwork.org/wp-content/uploads/2019/' \
'05/[email protected]" alt="open food network logo" />'
product.save!

visit shop_path
expect(page).to have_content product.name
expect_product_description_html_to_be_displayed(product, product.description, nil,
truncate: true)

# It truncates a long product description.
within_product_description(product) do
expect(html).to include "<b>Formatted</b> product description: Lorem ipsum"
expect(page).to have_content "..."
end
within_product_modal(product) do
expect(html).to include product.description
expect(find_link('external site')[:target]).to eq('_blank')
end
end

it "does not show unsecure HTML" do
Expand All @@ -94,9 +102,16 @@
visit shop_path
expect(page).to have_content product.name

expect_product_description_html_to_be_displayed(
product, "<p>Safe</p>", "<script>alert('Dangerous!');</script>", truncate: false
)
within_product_description(product) do
expect(html).to include "<p>Safe</p>"
expect(html).not_to include "<script>alert('Dangerous!');</script>"
expect(page).to have_content "alert('Dangerous!'); Safe"
Copy link
Member

Choose a reason for hiding this comment

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

This is what the user sees.

end
within_product_modal(product) do
expect(html).to include "<p>Safe</p>"
expect(html).not_to include "<script>alert('Dangerous!');</script>"
expect(page).to have_content "alert('Dangerous!'); Safe"
end
end
end

Expand Down Expand Up @@ -143,22 +158,13 @@
end
end

def expect_product_description_html_to_be_displayed(product, html, not_include = nil,
truncate: false)
# check inside list of products
within "#product-#{product.id} .product-description" do
expect(html).to include(html)
Copy link
Member

Choose a reason for hiding this comment

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

I now realise that html was supposed to refer to the page html but it got accidentally overridden by a parameter named the same. So I added more commits to show you how I would write it. It's maybe not as DRY but much simpler to read. And it makes it easier to fix up all the little mistakes of the previous code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice. Thanks @mkllnk. This was a good learning experience 🙏
and I understand what you mean, readability is more important than being pedantically DRY 😄

expect(html).not_to include(not_include) if not_include
expect(page).to have_content "..." if truncate # it truncates a long product description
end

# check in product description modal
def within_product_modal(product, &)
click_link product.name
expect(page).to have_selector '.reveal-modal'
modal_should_be_open_for product
within(".reveal-modal") do
expect(html).to include(html)
expect(html).not_to include(not_include) if not_include
end
within(".reveal-modal", &)
end

def within_product_description(product, &)
within("#product-#{product.id} .product-description", &)
end
end
Loading