Merge lp:~openerp-dev/openobject-server/trunk-text2html-chm into lp:openobject-server

[Christophe Matthieu (OpenERP)]

[lp:~openerp-dev/openobject-server/trunk-text2html-chm]

4582. By Christophe Matthieu (OpenERP) on 2012-11-22

[IMP] tools mail: improve regexp for link

4583. By Christophe Matthieu (OpenERP) on 2012-11-22

[IMP] mail: add test for html_regexp_link

4584. By Christophe Matthieu (OpenERP) on 2012-11-23

[IMP] tools mail: improve regex for link and add comment

4585. By Christophe Matthieu (OpenERP) on 2012-11-23

[FIX] tools mail: fix test and regexp

[Vo Minh Thu (thu)]

Howdy (I think this is my first code review for you ;-)

- You have defined a html_regexp_link() function. Please write a proper doc string (not some half-assed comment above).

- Please name the function correctly; its current name doesn't mean anything. In particular its name should make its intent clear. The fact it uses regex for its job is an implementation detail, no need to state it in its name.

- Your variable names are weird: you mix underscore_separated with camelCase, e.g. new_html and innerText. Why not use simple words like original, input, output, processed, expected, and so on.

- Your tests seem weird: why does each link end with $@ ? Why does each line end with a . ?

- Please think your comments are meant to be read by people who don't know what your code do. For instance

  # 7. create link

would probably read better if it was something along the line of

  #7. Convert URIs to HTML links.

- Instead of a big innerText (together with its outText), you can make a list of pairs and loop over it.

Now on the biggest gripe I have: please use a existing tool instead of reinventing the wheel all over again, e.g. https://github.com/jsocol/bleach#readme

If you insist on reimplementing stuff on your own, maybe add a proper test suite, see maybe e.g. https://github.com/jsocol/bleach/tree/master/bleach/tests

In particular, I would try things like

  <a>

alone, or

  <href

alone, or

  hellowww.openerp.com

you know, weird stuff that people actually write.

review: Disapprove

[lp:~openerp-dev/openobject-server/trunk-text2html-chm]

4586. By Christophe Matthieu (OpenERP) on 2012-11-29

[IMP] tools mail: Convert html_regexp_link into URIs_to_html_link; Improve regexp for URIs_to_html_link; Add test for uri.

[Christophe Simonis (OpenERP) (kangol)]

https links wont works...

review: Needs Fixing

[lp:~openerp-dev/openobject-server/trunk-text2html-chm]

4587. By Christophe Matthieu (OpenERP) on 2012-11-29

[IMP] tools mail: add target="_blank" for the external link html.

4588. By Christophe Matthieu (OpenERP) on 2012-11-29

[IMP] tools mail: add test for protocol http, https, ftp for URIs_to_html_link

4589. By Christophe Matthieu (OpenERP) on 2012-11-29

[IMP] tools mail: add tests for test_URIs_to_html_link

4590. By Christophe Matthieu (OpenERP) on 2012-11-29

[IMP] tools mail: change comment

[Christophe Matthieu (OpenERP) (chm-openerp)]

review: Needs Resubmitting

[lp:~openerp-dev/openobject-server/trunk-text2html-chm]

4591. By Christophe Matthieu (OpenERP) on 2012-12-11

[MERGE] from trunk

[Vo Minh Thu (thu)]

Tests seem nice but not enough.

From the top of my head I would add the //www.google.com case, i.e. the protocol part can be left out in href and src attributes.

Make it clear somewhere, like in a spec, if it is normal the www.google.com becomes http://www.google.com in the href attribute (instead of www.google.com).

I believe the code doesn't work for simply google.com.

Does it work for unicode URLs ?

I have pushed a branch at lp:~openerp-dev/openobject-server/trunk-text2html-chm-vmt. It shows additional tests and a way to generate more of them. Some values are commented out because they don't work.

Again, I don't see the point of replicating an existing library. If you do so, you'd better give some good reason in the merge proposal comment (e.g. the desired behavior is not available) and reuse their test suite.

Don't add a Resubmit: it is something the reviewer can say, not the reviewee ;-)

review: Disapprove

[Vo Minh Thu (thu)]

B.t.w, should it work for email address (and use the mailto scheme) ?

[Vo Minh Thu (thu)]

The test using hellowww.abcde.fr is wrong: hellowww is a correct subdomain (even if the user didn't intended to write that).

[Christophe Matthieu (OpenERP) (chm-openerp)]

hellowww.abcde.fr or words.words.it : These links will be processed if they are preceded by http.
Whether to extend the application, I can add processing something.something.something

[Xavier (Open ERP) (xmo-deactivatedaccount)]

> Make it clear somewhere, like in a spec, if it is normal the www.google.com becomes http://www.google.com in the href attribute (instead of www.google.com).

For what it's worth, Apple's auto-likifier (e.g. in Apple Mail) seems to do the same: `google.com` will be hyperlinked to http://google.com/, and `www.google.com` to http://www.google.com/

[Xavier (Open ERP) (xmo-deactivatedaccount)]

And bleach.linkify does the same, on your paragraphs with links in them it outputs:

----

Make it clear somewhere, like in a spec, if it is normal the <a href="http://www.google.com" rel="nofollow">www.google.com</a> becomes <a href="http://www.google.com" rel="nofollow">http://www.google.com</a> in the href attribute (instead of <a href="http://www.google.com" rel="nofollow">www.google.com</a>).

I believe the code doesn't work for simply <a href="http://google.com" rel="nofollow">google.com</a>.

<a href="http://hellowww.abcde.fr" rel="nofollow">hellowww.abcde.fr</a> or <a href="http://words.words.it" rel="nofollow">words.words.it</a> : These links will be processed if they are preceded by http.
Whether to extend the application, I can add processing something.something.something

[lp:~openerp-dev/openobject-server/trunk-text2html-chm]

4592. By Christophe Matthieu (OpenERP) on 2012-12-12

[IMP] tools mail: URIs_to_html_link, convert link with all subdomain (not limited to www and ftp)

4593. By Christophe Matthieu (OpenERP) on 2012-12-12

[IMP] tools mail: URIs_to_html_link, convert email address into link

4594. By Christophe Matthieu (OpenERP) on 2012-12-12

[IMP] tools mail: URIs_to_html_link, add test email

4595. By Christophe Matthieu (OpenERP) on 2012-12-12

[IMP] tools mail: change double quote into quote for email link

[Christophe Matthieu (OpenERP) (chm-openerp)]

review: Needs Resubmitting

[Vo Minh Thu (thu)]

Please read again my previous comments.

[lp:~openerp-dev/openobject-server/trunk-text2html-chm]

4596. By Christophe Matthieu (OpenERP) on 2012-12-13

[IMP]tools mail: add // protocol

4597. By Christophe Matthieu (OpenERP) on 2012-12-13

[IMP] tools mail: refactoring of URIs_to_html_link

4598. By Christophe Matthieu (OpenERP) on 2012-12-13

[IMP] tools mail: add test for URIs_to_html_link

[Thibault Delavallée (OpenERP) (tde-openerp)]

Reported for 7.1 :) .

review: Needs Information

Unmerged revisions

4598. By Christophe Matthieu (OpenERP) on 2012-12-13

[IMP] tools mail: add test for URIs_to_html_link

4597. By Christophe Matthieu (OpenERP) on 2012-12-13

[IMP] tools mail: refactoring of URIs_to_html_link

4596. By Christophe Matthieu (OpenERP) on 2012-12-13

[IMP]tools mail: add // protocol

4595. By Christophe Matthieu (OpenERP) on 2012-12-12

[IMP] tools mail: change double quote into quote for email link

4594. By Christophe Matthieu (OpenERP) on 2012-12-12

[IMP] tools mail: URIs_to_html_link, add test email

4593. By Christophe Matthieu (OpenERP) on 2012-12-12

[IMP] tools mail: URIs_to_html_link, convert email address into link

4592. By Christophe Matthieu (OpenERP) on 2012-12-12

[IMP] tools mail: URIs_to_html_link, convert link with all subdomain (not limited to www and ftp)

4591. By Christophe Matthieu (OpenERP) on 2012-12-11

[MERGE] from trunk

4590. By Christophe Matthieu (OpenERP) on 2012-11-29

[IMP] tools mail: change comment

4589. By Christophe Matthieu (OpenERP) on 2012-11-29

[IMP] tools mail: add tests for test_URIs_to_html_link

=== modified file 'openerp/tests/test_mail.py'
--- openerp/tests/test_mail.py	2012-11-28 13:51:01 +0000
+++ openerp/tests/test_mail.py	2012-12-13 13:27:23 +0000
@@ -23,7 +23,7 @@
 ##############################################################################
 
 import unittest2
-from openerp.tools import html_sanitize, html_email_clean, append_content_to_html, plaintext2html
+from openerp.tools import html_sanitize, html_email_clean, URIs_to_html_link, append_content_to_html, plaintext2html
 
 HTML_SOURCE = """
 <font size="2" style="color: rgb(31, 31, 31); font-family: monospace; font-variant: normal; line-height: normal; ">test1</font>
@@ -177,6 +177,90 @@
         new_html = html_email_clean(False)
         self.assertEqual(new_html, False, 'html_email_cleaner did change a False in an other value.')
 
+    def test_URIs_to_html_link(self):
+        # Test1: Test if the method convert text links and do not convert html links
+
+        schemes = ('http://', 'https://', 'ftp://', '', '//')
+        pages = ('', '/about/Santa.html', '?debug=&ts=4', '#action=1[3,4]&$%~.-;:=,?@+', ':8080', ':8080/mail.php', ':8080?variables=5', ':8080#title3')
+        TLDSs = ('be', 'com', 'ac.be')
+        urls = (
+            'google.',
+            'www.google.',
+            'info.fundp.',
+            't.website.',
+            'ns.phx4.nearlyfreespeech.')
+        links = [
+            ( scheme + url + TLDS + page ,
+            '<a href="' + (scheme or 'http://') + url + TLDS + page + '" target="_blank">' + scheme + url + TLDS + page + '</a>' )
+                for TLDS in TLDSs for page in pages for scheme in schemes for url in urls]
+
+        links = links + [
+            # test standard link begin with a protocol http
+            (   'balbal http://localhost.truc.be ', 
+                'balbal <a href="http://localhost.truc.be" target="_blank">http://localhost.truc.be</a> '),
+            # test standard link begin with a protocol http
+            (   ' //localhost.truc.be ', 
+                ' <a href="//localhost.truc.be" target="_blank">//localhost.truc.be</a> '),
+            # test standard link begin with a protocol https
+            (   'balbal https://localhost.truc.be ', 
+                'balbal <a href="https://localhost.truc.be" target="_blank">https://localhost.truc.be</a> '),
+            # test standard link begin with a protocol ftp
+            (   'balbal ftp://localhost.truc.be ', 
+                'balbal <a href="ftp://localhost.truc.be" target="_blank">ftp://localhost.truc.be</a> '),
+            # test standard link begin with a protocol, with port and special chars
+            (   'balbal http://localhost:8069/?debug=&ts=4#action=1[3,4]&$%~.-;:=,?@+ ', 
+                'balbal <a href="http://localhost:8069/?debug=&ts=4#action=1[3,4]&$%~.-;:=,?@+" target="_blank">http://localhost:8069/?debug=&ts=4#action=1[3,4]&$%~.-;:=,?@+</a> '),
+            # test standard link
+            (   'toto www.onewebsite.be www.truc<a> hellowww.abcde.fr <abc>www.test.fr</abc>', 
+                'toto <a href="http://www.onewebsite.be" target="_blank">www.onewebsite.be</a> www.truc<a> <a href="http://hellowww.abcde.fr" target="_blank">hellowww.abcde.fr</a> <abc><a href="http://www.test.fr" target="_blank">www.test.fr</a></abc>'),
+            # test link wrong html tag
+            (   'toto <>www.onewebsite.be<> www.truc.communication<a>', 
+                'toto <><a href="http://www.onewebsite.be" target="_blank">www.onewebsite.be</a><> www.truc.communication<a>'),
+            (   'toto <span www.onewebsite.be www.truc<a>', 
+                'toto <span <a href="http://www.onewebsite.be" target="_blank">www.onewebsite.be</a> www.truc<a>'),
+            # test with IP website url
+            (   '123 http://234.33.45.01:8069 ???', 
+                '123 <a href="http://234.33.45.01:8069" target="_blank">http://234.33.45.01:8069</a> ???'),
+            # test with smiles
+            (   ':-> <bold>ftp.ftpsite.be:8069/</bold> :->', 
+                ':-> <bold><a href="http://ftp.ftpsite.be:8069/" target="_blank">ftp.ftpsite.be:8069/</a></bold> :->'),
+            # test with images and smiles
+            (   '<:-) <img src="http://www.link.com/tata"/> :->', 
+                '<:-) <img src="http://www.link.com/tata"/> :->'),
+            # test with image in an external html link and smiles
+            (   '<a href="http://www.link.com/tata" class="truc"><:-)<img src="http://www.link.com/tata"/>:-></a>', 
+                '<a href="http://www.link.com/tata" class="truc" target="_blank"><:-)<img src="http://www.link.com/tata"/>:-></a>'),
+            # test image with relative link
+            (   '<img src="./tata"/>', 
+                '<img src="./tata"/>'),
+            # test html link with relative link
+            (   '<a href="./tata">beta</a>', 
+                '<a href="./tata">beta</a>'),
+            # test relative link
+            (   ' ./link.com/to/page ', 
+                ' ./link.com/to/page '),
+            # test short link with good tlds
+            (   ' google.com google.be ', 
+                ' <a href="http://google.com" target="_blank">google.com</a> <a href="http://google.be" target="_blank">google.be</a> '),
+            # test standard email
+            (   '[e-mail] ', 
+                '<a href=\'mailto:[e-mail]\' target=\'_blank\'>[e-mail]</a> '),
+            # test standard email
+            (   'user+mailbox/department=[e-mail] ', 
+                '<a href=\'mailto:user+mailbox/department=[e-mail]\' target=\'_blank\'>user+mailbox/department=[e-mail]</a> '),
+            # test complexe email
+            (   '"Joe.#$%&\'*+-/=? Blow"@example.com ', 
+                '<a href=\'mailto:"Joe.#$%&\'*+-/=? Blow"@example.com\' target=\'_blank\'>"Joe.#$%&\'*+-/=? Blow"@example.com</a> '),
+        ]
+
+        for link in links:
+            # test for convert
+            output = URIs_to_html_link( link[0] )
+            self.assertEqual(output, link[1], 'test_URIs_to_html_link did convert links for case : %s' % link[0])
+            # converted link don't change
+            output = URIs_to_html_link( link[1] )
+            self.assertEqual(output, link[1], 'test_URIs_to_html_link did convert links for case : %s' % link[1] )
+
 class TestHtmlTools(unittest2.TestCase):
     """ Test some of our generic utility functions about html """
 

=== modified file 'openerp/tools/mail.py'
--- openerp/tools/mail.py	2012-11-26 18:15:27 +0000
+++ openerp/tools/mail.py	2012-12-13 13:27:23 +0000
@@ -110,6 +110,47 @@
 #----------------------------------------------------------
 # HTML Cleaner
 #----------------------------------------------------------
+# Convert URIs to HTML links.
+def URIs_to_html_link(html):
+
+    TLDS = "|".join("""ac ad ae aero af ag ai al am an ao aq ar arpa as asia at au aw ax az
+       ba bb bd be bf bg bh bi biz bj bm bn bo br bs bt bv bw by bz ca cat
+       cc cd cf cg ch ci ck cl cm cn co com coop cr cu cv cx cy cz de dj dk
+       dm do dz ec edu ee eg er es et eu fi fj fk fm fo fr ga gb gd ge gf gg
+       gh gi gl gm gn gov gp gq gr gs gt gu gw gy hk hm hn hr ht hu id ie il
+       im in info int io iq ir is it je jm jo jobs jp ke kg kh ki km kn kp
+       kr kw ky kz la lb lc li lk lr ls lt lu lv ly ma mc md me mg mh mil mk
+       ml mm mn mo mobi mp mq mr ms mt mu museum mv mw mx my mz na name nc ne
+       net nf ng ni nl no np nr nu nz om org pa pe pf pg ph pk pl pm pn pr pro
+       ps pt pw py qa re ro rs ru rw sa sb sc sd se sg sh si sj sk sl sm sn so
+       sr st su sv sy sz tc td tel tf tg th tj tk tl tm tn to tp tr travel tt
+       tv tw tz ua ug uk us uy uz va vc ve vg vi vn vu wf ws xn ye yt yu za zm
+       zw""".split())
+    IP = r"[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+"
+    link = r"[\w]([.-]?[\w]+)*"
+    outerlink = r"(:[0-9]+)?([/?#][/\w\#&$\%~.\-;:=,?@\[\]+]*)?"
+    protocol = "|".join("""http:// https:// ftp:// // """.split())
+    email = r"(([\w]+([.+/=-][\w]+)*)|\"[^\"]+\")@(%s)\.(%s)" % (link, TLDS)
+
+    # add target blanck on external link
+    part = re.compile(r"(<a (?:[^t>]|t(?!arget=['\"]_blank['\"]))*?(?:https?|ftp)\://(?:[^t>]|t(?!arget=['\"]_blank['\"]))*?)(>.+?</a>)", re.IGNORECASE | re.DOTALL)
+    html = part.sub(r'\1 target="_blank"\2', html)
+
+    # avoid to have a tag link in an tag image or tag link
+    REidempotent = r"(^|\s|<>|<[^a][^<>]*[>\s]|<a[^\s][^<>]*[>\s])(?!<a\s[^>]+>|href=[\"']|href=[\"']mailto:|src=[\"']|url=[\"'])"
+    end = r"(?![\w])"
+
+    # find the links are beginning with http://, https://, ftp://...
+    part = re.compile(r"%s((%s)(%s|%s)(%s))%s" % (REidempotent, protocol, link, IP, outerlink, end), re.IGNORECASE | re.DOTALL)
+    html = part.sub(r'\1<a href="\2" target="_blank">\2</a>', html)
+    # find the links are beginning with www., ftp. or other subdomain
+    part = re.compile(r"%s((%s)\.(%s)(%s))%s" % (REidempotent, link, TLDS, outerlink, end), re.IGNORECASE | re.DOTALL)
+    html = part.sub(r'\1<a href="http://\2" target="_blank">\2</a>', html)
+    # find email and convert into link with mailto
+    part = re.compile(r"%s(%s)%s" % (REidempotent, email, end), re.IGNORECASE | re.DOTALL)
+    html = part.sub(r"\1<a href='mailto:\2' target='_blank'>\2</a>", html)
+
+    return html
 
 def html_email_clean(html):
     """ html_email_clean: clean the html to display in the web client.
@@ -183,6 +224,9 @@
     br_div_tags = re.compile(r'(<div>\s*<br\s*\/>\s*<\/div>)')
     html = _replace_matching_regex(br_div_tags, html, '<br />')
 
+    # 7. Convert URIs to HTML links.
+    html = URIs_to_html_link(html)
+
     return html