pengyuthon/odoo_source
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[FIX] link_tracker,mass_mailing_sms: fix shorten links

Browse Source 
Purpose
Fix bugs in link replacement (html and text version)

1/ The replacement of urls in text (not html) was buggy when
* a string includes multiple urls, and
* `base_url` is one of them and not the first
* Also in SMS marketing when adding the sms `id`

When it came to base_url's turn, the replace function for the content would
replace the `base_url` part of a previously shortened url instead of the
distinct `base_url` link.

2/ Ampersand character
A) The ampersand character prevented replacement of an url inside a `Markup`.
Previous tests passed as the tracker was created but the url was not replaced.

B) The ampersand character was not recognized as part of an url for simple
strings.

3/ Replacement of already short links
A faulty logic made it possible to replace "/r/" urls when no "blacklist" was
passed.

Commit
* Ensures that only the base_url link would be replaced instead in this case.
* Adds support for urls with "&"
* Does prevent shortening short urls
* Improves link conversion performance (regex + avoid duplication when there
  are several occurrences of the same url)
* Adds multiple unit tests

Task-2783844

closes odoo/odoo#91164

X-original-commit: 61026806d831b49a7cf5c44ffef3d762986781ce
Signed-off-by: Thibault Delavallee (tde) <tde@openerp.com>
This commit is contained in:
Florian Charlier
2022-05-12 13:43:17 +02:00
parent 89e98471fa
commit b0f233ba86
6 changed files with 132 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files
addons/link_tracker/models/mail_render_mixin.py
+18 -12
View File
@@ -35,24 +35,29 @@ class MailRenderMixin(models.AbstractModel):
"""
base_url = base_url or self.env['ir.config_parameter'].sudo().get_param('web.base.url')
short_schema = base_url + '/r/'
for match in re.findall(tools.HTML_TAG_URL_REGEX, html):
href = markupsafe.Markup(match[0])
for match in set(re.findall(tools.HTML_TAG_URL_REGEX, html)):
long_url = match[1]
# Don't shorten already-shortened links
if long_url.startswith(short_schema):
continue
# Don't shorten urls present in blacklist (aka to skip list)
if blacklist and any(s in long_url for s in blacklist):
continue
label = (match[3] or '').strip()
if not blacklist or not [s for s in blacklist if s in long_url] and not long_url.startswith(short_schema):
create_vals = dict(link_tracker_vals, url=utils.unescape(long_url), label=utils.unescape(label))
link = self.env['link.tracker'].search_or_create(create_vals)
if link.short_url:
new_href = href.replace(long_url, link.short_url)
html = html.replace(href, new_href)
create_vals = dict(link_tracker_vals, url=utils.unescape(long_url), label=utils.unescape(label))
link = self.env['link.tracker'].search_or_create(create_vals)
if link.short_url:
# `str` manipulation required to support replacing "&" characters, common in urls
new_href = match[0].replace(long_url, link.short_url)
html = html.replace(markupsafe.Markup(match[0]), markupsafe.Markup(new_href))
return html
@api.model
def _shorten_links_text(self, content, link_tracker_vals, blacklist=None, base_url=None):
""" Shorten links in a string content. Works like ``_shorten_links`` but
targetting string content, not html.
targeting string content, not html.
:return: updated content
"""
@@ -61,7 +66,7 @@ class MailRenderMixin(models.AbstractModel):
base_url = base_url or self.env['ir.config_parameter'].sudo().get_param('web.base.url')
shortened_schema = base_url + '/r/'
unsubscribe_schema = base_url + '/sms/'
for original_url in re.findall(tools.TEXT_URL_REGEX, content):
for original_url in set(re.findall(tools.TEXT_URL_REGEX, content)):
# don't shorten already-shortened links or links towards unsubscribe page
if original_url.startswith(shortened_schema) or original_url.startswith(unsubscribe_schema):
continue
@@ -70,9 +75,10 @@ class MailRenderMixin(models.AbstractModel):
if blacklist and any(item in parsed.path for item in blacklist):
continue
create_vals = dict(link_tracker_vals, url= utils.unescape(original_url))
create_vals = dict(link_tracker_vals, url=utils.unescape(original_url))
link = self.env['link.tracker'].search_or_create(create_vals)
if link.short_url:
content = content.replace(original_url, link.short_url, 1)
# Ensures we only replace the same link and not a subpart of a longer one, multiple times if applicable
content = re.sub(re.escape(original_url) + r'(?![\w@:%.+&~#=/-])', link.short_url, content)
return content
addons/link_tracker/tests/test_mail_render_mixin.py
+100 -1
View File
@@ -1,10 +1,19 @@
# -*- coding: utf-8 -*-
# Part of Odoo. See LICENSE file for full copyright and licensing details.
import re
from odoo.tests import common
from odoo.tests import common, tagged
from odoo.tools import TEXT_URL_REGEX
@tagged('-at_install', 'post_install')
class TestMailRenderMixin(common.TransactionCase):
@classmethod
def setUpClass(cls):
super().setUpClass()
cls.base_url = cls.env["mail.render.mixin"].get_base_url()
def test_shorten_links(self):
test_links = [
'<a href="https://gitlab.com" title="title" fake="fake">test_label</a>',
@@ -50,3 +59,93 @@ class TestMailRenderMixin(common.TransactionCase):
for tracker_to_fail in trackers_to_fail:
self.assertFalse(self.env["link.tracker"].search(tracker_to_fail))
def test_shorten_links_html_skip_shorts(self):
old_content = self.env["mail.render.mixin"]._shorten_links(
'This is a link: <a href="https://test_542152qsdqsd.com">old</a>', {})
created_short_url_match = re.search(TEXT_URL_REGEX, old_content)
self.assertIsNotNone(created_short_url_match)
created_short_url = created_short_url_match[0]
self.assertRegex(created_short_url, "{base_url}/r/[\\w]+".format(base_url=self.base_url))
new_content = self.env["mail.render.mixin"]._shorten_links(
'Reusing this old <a href="{old_short_url}">link</a> with a new <a href="https://odoo.com">one</a>'
.format(old_short_url=created_short_url),
{}
)
expected = re.compile(
'Reusing this old <a href="{old_short_url}">link</a> with a new <a href="{base_url}/r/[\\w]+">one</a>'
.format(old_short_url=created_short_url, base_url=self.base_url)
)
self.assertRegex(new_content, expected)
def test_shorten_links_html_including_base_url(self):
content = (
'This is a link: <a href="https://www.worldcommunitygrid.org">https://www.worldcommunitygrid.org</a><br/>\n'
'This is another: <a href="{base_url}/web#debug=1&more=2">{base_url}</a><br/>\n'
'And a third: <a href="{base_url}">Here</a>\n'
'And a forth: <a href="{base_url}">Here</a>\n'
'And a fifth: <a href="{base_url}">Here too</a>\n'
'And a last, more complex: <a href="https://boinc.berkeley.edu/forum_thread.php?id=14544&postid=106833">There!</a>'
.format(base_url=self.base_url)
)
expected_pattern = re.compile(
'This is a link: <a href="{base_url}/r/[\\w]+">https://www.worldcommunitygrid.org</a><br/>\n'
'This is another: <a href="{base_url}/r/[\\w]+">{base_url}</a><br/>\n'
'And a third: <a href="{base_url}/r/([\\w]+)">Here</a>\n'
'And a forth: <a href="{base_url}/r/([\\w]+)">Here</a>\n'
'And a fifth: <a href="{base_url}/r/([\\w]+)">Here too</a>\n'
'And a last, more complex: <a href="{base_url}/r/([\\w]+)">There!</a>'
.format(base_url=self.base_url)
)
new_content = self.env["mail.render.mixin"]._shorten_links(content, {})
self.assertRegex(new_content, expected_pattern)
matches = expected_pattern.search(new_content).groups()
# 3rd and 4th lead to the same short_url
self.assertEqual(matches[0], matches[1])
# 5th has different label but should currently lead to the same link
self.assertEqual(matches[1], matches[2])
def test_shorten_links_text_including_base_url(self):
content = (
'This is a link: https://www.worldcommunitygrid.org\n'
'This is another: {base_url}/web#debug=1&more=2\n'
'A third: {base_url}\n'
'A forth: {base_url}\n'
'And a last, with question mark: https://boinc.berkeley.edu/forum_thread.php?id=14544&postid=106833'
.format(base_url=self.base_url)
)
expected_pattern = re.compile(
'This is a link: {base_url}/r/[\\w]+\n'
'This is another: {base_url}/r/[\\w]+\n'
'A third: {base_url}/r/([\\w]+)\n'
'A forth: {base_url}/r/([\\w]+)\n'
'And a last, with question mark: {base_url}/r/([\\w]+)'
.format(base_url=self.base_url)
)
new_content = self.env["mail.render.mixin"]._shorten_links_text(content, {})
self.assertRegex(new_content, expected_pattern)
matches = expected_pattern.search(new_content).groups()
# 3rd and 4th lead to the same short_url
self.assertEqual(matches[0], matches[1])
def test_shorten_links_text_skip_shorts(self):
old_content = self.env["mail.render.mixin"]._shorten_links_text(
'This is a link: https://test_542152qsdqsd.com', {})
created_short_url_match = re.search(TEXT_URL_REGEX, old_content)
self.assertIsNotNone(created_short_url_match)
created_short_url = created_short_url_match[0]
self.assertRegex(created_short_url, "{base_url}/r/[\\w]+".format(base_url=self.base_url))
new_content = self.env["mail.render.mixin"]._shorten_links_text(
'Reusing this old link {old_short_url} with a new one, https://odoo.com</a>'
.format(old_short_url=created_short_url),
{}
)
expected = re.compile(
'Reusing this old link {old_short_url} with a new one, {base_url}/r/[\\w]+'
.format(old_short_url=created_short_url, base_url=self.base_url)
)
self.assertRegex(new_content, expected)
addons/mass_mailing/models/mail_mail.py
+1 -1
View File
@@ -36,7 +36,7 @@ class MailMail(models.Model):
body = super(MailMail, self)._send_prepare_body()
if self.mailing_id and body and self.mailing_trace_ids:
for match in re.findall(tools.URL_REGEX, self.body_html):
for match in set(re.findall(tools.URL_REGEX, self.body_html)):
href = match[0]
url = match[1]
addons/mass_mailing_sms/models/sms_sms.py
+2 -2
View File
@@ -22,9 +22,9 @@ class SmsSms(models.Model):
continue
body = sms.body
for url in re.findall(tools.TEXT_URL_REGEX, body):
for url in set(re.findall(tools.TEXT_URL_REGEX, body)):
if url.startswith(sms.get_base_url() + '/r/'):
body = body.replace(url, url + '/s/%s' % sms.id)
body = re.sub(re.escape(url) + r'(?![\w@:%.+&~#=/-])', url + f'/s/{sms.id}', body)
res[sms.id] = body
return res
addons/test_mail_full/tests/test_sms_sms.py
+10 -1
View File
@@ -99,12 +99,21 @@ class TestSMSPost(TestMailFullCommon, MockLinkTracker):
'number': '13',
'mailing_id': mailing.id,
})
sms_4 = self.env['sms.sms'].create({
'body': 'Welcome to https://test.odoo.com/r/RAOUL\nAnd again,\n'
'https://test.odoo.com/r/RAOUL',
'number': '14',
'mailing_id': mailing.id,
})
res = (sms_0 | sms_1 | sms_2 | sms_3)._update_body_short_links()
res = (sms_0 | sms_1 | sms_2 | sms_3 | sms_4)._update_body_short_links()
self.assertEqual(res[sms_0.id], 'Welcome to https://test.odoo.com')
self.assertEqual(res[sms_1.id], 'Welcome to https://test.odoo.com/r/RAOUL')
self.assertEqual(res[sms_2.id], 'Welcome to https://test.odoo.com/r/RAOUL/s/%s' % sms_2.id)
self.assertEqual(res[sms_3.id], 'Welcome to https://test.odoo.com/leodagan/r/RAOUL')
self.assertEqual(
res[sms_4.id],
f'Welcome to https://test.odoo.com/r/RAOUL/s/{sms_4.id}\nAnd again,\nhttps://test.odoo.com/r/RAOUL/s/{sms_4.id}')
def test_sms_send_batch_size(self):
self.count = 0
odoo/tools/mail.py
+1 -1
View File
@@ -257,7 +257,7 @@ def html_sanitize(src, silent=True, sanitize_tags=True, sanitize_attributes=Fals
# ----------------------------------------------------------
URL_REGEX = r'(\bhref=[\'"](?!mailto:|tel:|sms:)([^\'"]+)[\'"])'
TEXT_URL_REGEX = r'https?://[a-zA-Z0-9@:%._\+~#=/-]+(?:\?\S+)?'
TEXT_URL_REGEX = r'https?://[\w@:%.+&~#=/-]+(?:\?\S+)?'
# retrieve inner content of the link
HTML_TAG_URL_REGEX = URL_REGEX + r'([^<>]*>([^<>]+)<\/)?'