PR#397: Update hubs/widgets/validators.py for Link validation[solves #352] - fedora-hubs

		`@@ -19,10 +19,9 @@`
		`self.assertEqual(validators.Integer("1"), 1)`
		`self.assertRaises(ValueError, validators.Integer, "text")`

		`- @unittest.skip("Not implemented yet")`
		`def test_link(self):`
		`- value = '<a href="somewhere">dummy</a>'`
		`- self.assertEqual(validators.Link.from_string(value), value)`
		`+ value = "https://pagure.io"`
		`+ self.assertEqual(validators.value, value)`
		`self.assertRaises(ValueError, validators.Link, "text")`

		`def test_username(self):`

hubs/widgets/validators.py

file modified

+15 -4

		`@@ -16,8 +16,17 @@`
		`import kitchen.text.converters`
		`import requests`
		`import six`
		`-`
		`-`
		`+ import re`
		`+`
		`+ #compiling regex for url validation`
		`+ url = re.compile(`
		`+ r'^(?:http\|ftp)s?://\|\bwww.\b'`
		`+ r'(?:(?:[A-Z0-9](?:[A-Z0-9-]{0,61}[A-Z0-9])?\.)+(?:[A-Z]{2,6}\.?\|[A-Z0-9-]{2,}\.?)\|'`
		`+ r'localhost\|'`
		`+ r'\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})'`
		`+ r'(?::\d+)?'`
		`+ r'(?:/?\|[/?]\S+)$',re.I)`
		`+`
		`def Required(value):`
		"""Raises an error if the value is ``False``-like."""
		`if not bool(value):`
		`@@ -40,8 +49,10 @@`

		`def Link(value):`
		`"""Raises an error if the value doesn't look like a link."""`
		`- # TODO -- verify that this is actually a link`
		`- return value`
		`+ if url.search(value):`
		`+ return value`
		`+ else:`
		`+ raise ValueError('Invalid Url')`


		`def Username(value):`

dristybutola commented 7 years ago

LInk validation of url includes https, http,localhost and ip address using regex library of python

shaily commented 7 years ago

This will always return value.
You should run it against some tests.
Probably Django's itself since this is from v1.3.
You should look at v1.11's validator - it seems to cover a lot more cases (specific IPv4/6 checks instead of just digits, internationalized domain names containing unicode characters etc)

Edited 7 years ago by shaily

dristybutola commented 7 years ago

This will always return value.
You should run it against some tests.
Probably Django's itself since this is from v1.3.
You should look at v1.11's validator - it seems to cover a lot more cases (specific IPv4/6 checks instead of just digits, internationalized domain names containing unicode characters etc)

@shaily Thank you, I am working on this :)

abompard commented 7 years ago

A few comments:
- It does not seem to be actually checking the value, you're compiling the regex but not using it
- Please remove the TODO comment.
- The regex could be compiled outside of the function definition, so it's compiled once at import time and not on each call (which is faster)
- Please provide unit tests.

1 new commit added

Update hubs/widgets/validators.py

7 years ago

1 new commit added

Update hubs/widgets/validators.py

7 years ago

1 new commit added

Update hubs/widgets/validators.py

7 years ago

1 new commit added

Update hubs/widgets/validators.py

7 years ago

dristybutola commented 7 years ago

In hubs.tests.test_widget_validators the default test for link is
def test_link(self): value = '<a href="somewhere">dummy</a>' self.assertEqual(validators.Link.from_string(value), value) self.assertRaises(ValueError, validators.Link, "text")
I am not sure , that do we need to provide value as a string of words also containing a link or
simply a link we need to verify like 'http://example.com'.

abompard commented 7 years ago

To answer that question, I looked at where this validator is used. Apparently it's only used in the rules widget, and the filed only contains the URL, not the HTML <a href=...></a> tag. So validating an URL should be sufficient.

Edited 7 years ago by abompard

1 new commit added