Fix wrong link generation when the ingress URL contains a regex (#1120) #2131
Conversation
| description: "it should return false to a non-translated idn url", | ||
| fullURL: "http://wordpress.local/♨️/shouldBeIDNTranslated", | ||
| expected: false, | ||
| }, |
There was a problem hiding this comment.
I would add a couple of cases more, when the URL is using https and when the host is an IP
| ? GetURLItemFromIngress(ingress.item).URLs.map( | ||
| // check whether each URL is, indeed, a valid URL. | ||
| // If so, render the <a>, othersiwe, render a simple <span> | ||
| url => (IsURL(url) ? getAnchor(url) : getSpan(url)), |
There was a problem hiding this comment.
great, can you create a test in AccessURLTable.test for covering the case in which the path is a regexp? You can verify that there is no anchor in that case.
Tip: You can copy the test setup from "should show the table with available ingresses"
Dockerfile and package.json now use the same major Clarity version.
Add e2e test to detect a span is created instead of an anchor. Add more unit tests to cover ip-based hosts and httpS protocol.
This reverts commit e7b2214. (commit to wrong branch, not corresponding to this PR)
| ); | ||
| expect(wrapper.find("Table")).toExist(); | ||
| expect( | ||
| wrapper.find("a").findWhere(a => a.prop("href") === "http://foo.bar/ready(/|$)(.*)"), |
There was a problem hiding this comment.
I would do expect(wrapper.find("a")).not.toExistst() so we are sure we are not parsing the URL in a weird way and generate a link.
| wrapper.find("a").findWhere(a => a.prop("href") === "http://foo.bar/ready(/|$)(.*)"), | ||
| ).not.toExist(); | ||
| expect(wrapper.find("span").first()).toHaveText("http://foo.bar/ready(/|$)(.*)"); | ||
| expect(wrapper).toMatchSnapshot(); |
There was a problem hiding this comment.
you can omit the snapshot
| expect( | ||
| wrapper.find("a").findWhere(a => a.prop("href") === "http://foo.bar/ready(/|$)(.*)"), | ||
| ).not.toExist(); | ||
| expect(wrapper.find("span").first()).toHaveText("http://foo.bar/ready(/|$)(.*)"); |
There was a problem hiding this comment.
This is a bit risky, it would fail if we add any other span before. Better to do wrapper.find("span").findWhere(s => s.text() === "http://... so we find it wherever it is.
There was a problem hiding this comment.
The span element seems not to have a .text(), so I first have to render the cheerio component and then access the actual text: wrapper.find("span").findWhere(s => s.render().text() ..., if you have a better solution, please let me know.
There was a problem hiding this comment.
I double checked and the method .text() should be present (in fact, it should exist for any element). Tried this and worked:
const matchingSpans = wrapper.find("span").findWhere(s => s.text().includes("foo.bar/ready"));
Not sure why you are not seeing that method?
There was a problem hiding this comment.
Mmm, don't know either... but now it's working as you mention. Perhaps a misconfiguration in my linter led me to think there was an error with the .text() method. Thank you for your comments!
Removed not useful snapshot and improve the logic of the test case for a better detection of the span content and the absence of anchors.
| <div className="margin-b-sm"> | ||
| <a href={URL} target="_blank" rel="noopener noreferrer" key={URL}> |
There was a problem hiding this comment.
running the tests for this, I noticed several warnings like Warning: Each child in a list should have a unique "key" prop.. For this function, the key={URL} needs to be in the div. Note that any component returned in an array should have the key property so you need to update getSpan and getUnknown.
| } | ||
|
|
||
| function getNotes(resource?: IResource) { | ||
| if (!resource) { | ||
| return <span>Unknown</span>; | ||
| return getUnknown; |
There was a problem hiding this comment.
just noticed that this should be getUnknown()
Now each div (getUnknown and getSpan) has a unique 'key' property
Description of the change
In #1120 , it was noticed that a link (i.e., <a ...href=....> tag) is being created for each host/path in the ingress URL tables.
The ingress configuration allows users to enter wildcards and regular expressions, however, the hyperlinks cannot target to these URL since they are not reacheble.
The change proposed in this PR is simply to detect whether the given ingress URL is, actually, a valid URL (see discussion of what mean by a valid URL in #1120). If so, it renders the html tag; otherwise, it just renders an < span>.
Benefits
For users, there will not be any misleading hyperlink targets pointing nowhere.
Possible drawbacks
Due to the current regex, it is possible for some false positives/negatives to be included. Nevertheless, to the besy of my knowledge, there is no upstream accepted solution for doing so.
Applicable issues
Additional information
N/A