-
-
Notifications
You must be signed in to change notification settings - Fork 724
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
Open external links in product description in a new page #11761
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 !
I was bit confused has to why you needed a Stimulus controller, but then I realised you don't have access to the link directly in the template.
richtext
is a bit of a generic name, I think we should use something more specific like : add_blank_to_link
. What do you think ?
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!
I agree that the name is very generic. Were you planning to add other things to it?
We could also be more precise and only select a
tags with href
but it shouldn't really matter in this context.
Will change the name and write specs for this. Give me till tomorrow. I'll re-request review! |
fdc4546
to
1ff6517
Compare
I can see rubocop complaining:
I don't think there's a straightforward way to lower the branch condition size without splitting up that method. Do you think I should? |
1ff6517
to
73c68df
Compare
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.
Thanks for adding the Javascript spec !
I don't think there's a straightforward way to lower the branch condition size without splitting up that method. Do you think I should?
It's due to the extra expect .. if blah
, but yes short of breaking up expect_product_description_html_to_be_displayed
there isn't a quick way to fix it. I don't think it's too hard to understand, so I would be happy with ignoring the warning here. @mkllnk what do you think ?
You can disable the warning by adding something like :
# rubocop:disable Metrics/AbcSize
def expect_product_description_html_to_be_displayed
...
end
# rubocop:enable Metrics/AbcSize
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.
Let's improve the existing code while working on this.
@@ -157,6 +157,7 @@ def expect_product_description_html_to_be_displayed(product, html, not_include = | |||
expect(page).to have_selector '.reveal-modal' | |||
modal_should_be_open_for product | |||
within(".reveal-modal") do | |||
expect(find_link('external site')[:target]).to eq('_blank') if href | |||
expect(html).to include(html) | |||
expect(html).not_to include(not_include) if not_include | |||
end |
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 split this up? There are two distinct chunks of tests here. One is about the product list and the other about the modal. Boolean flags are often a code smell as well. There are some strange things in this method. This line is self-fulfilling:
expect(html).to include(html)
This line also seems unnecessary:
expect(page).to have_selector '.reveal-modal'
The next line should test the same thing.
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 have removed those redundant assertions.
I tried to make the other assertions better but have not found a clear way to do so. I think the intention is to check whether in the rendered HTML the html passed into the method exists but I couldn't come up with a solution that can make the assertion.
Also the spec right after this,
it "does not show unsecure HTML" do
product.description = "<script>alert('Dangerous!');</script><p>Safe</p>"
product.save!
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
)
end
says that unsecure html should not be displayed but is calling the same method at the end, I don't know what the actual behaviour should be but what's currently happening is that the unsecure HTML does show up (as text) in the page.
fbcdca6
to
d20ffb5
Compare
d20ffb5
to
249c002
Compare
This is much easier to read.
@@ -147,14 +147,12 @@ def expect_product_description_html_to_be_displayed(product, html, not_include = | |||
truncate: false, href: false) | |||
# check inside list of products | |||
within "#product-#{product.id} .product-description" do | |||
expect(html).to include(html) |
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 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.
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. Thanks @mkllnk. This was a good learning experience 🙏
and I understand what you mean, readability is more important than being pedantically DRY 😄
|
||
# It truncates a long product description. | ||
within_product_description(product) do | ||
expect(html).to include product.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.
I brought this line back but it's actually wrong because we want to see only part of the description with ...
at the end. It'll be fixed in a later commit.
@@ -105,10 +105,12 @@ | |||
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" |
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.
This is what the user sees.
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.
Thanks both for fixing the complexity !
Thanks @binarygit, I've checked the behavior before staging the PR: indeed, clicking a hyperlink on a product description will not open a new tab, forcing the user to navigate back. After staging this PR: This occurred after hard-reload, re-setting the URL... Not sure what to try next. Any advice? |
@filipefurtad0 I just logged into staging-uk and checked the rendered HTML. The stimulus Are you sure that this PR's code is merged with the Local:Staging:Locally, I have rebased with the latest changes in master so, I know that it's not because of some new code in |
Thanks for double checking @binarygit !
Uff - I was fully trusting the deployment workflow... (well, not fully, I think I've deployed twice, just to be sure :-D ). I'll just try again and/or other staging servers and check if your code changes are there. |
Ok, I see what's happening - there are two ways to access the product description:
This is the improvement this PR adds: So, I'll merge this PR, leaving issue #11721 open and adding a note so scenario 1) is covered as well. Thanks for your help @binarygit ! |
What? Why?
What should we test?
http://localhost:3000/<shop-name>/shop
)Release notes
Changelog Category (reviewers may add a label for the release notes):
Note: Trix does not have an API to toggle links as external which is why the stimulus controller is needed. See: basecamp/trix#55