Skip to content

Commit

Permalink
Correct logic on building marketing links.
Browse files Browse the repository at this point in the history
The logic used to has a special case for edge in the hostname which
didn't really make sense. So instead we just check to see if the given
url starts with http and if it does we return it directly.  If it doesn't,
then we try to convert it to a valid url and return that.  If that fails
we return .

Add a new custom metric `unresolved_marketing_link` to track when we
run into this scenario.
  • Loading branch information
feanil committed Jul 9, 2020
1 parent b3b8121 commit 4ac2c5e
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 2 deletions.
7 changes: 5 additions & 2 deletions common/djangoapps/edxmako/shortcuts.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
from django.core.validators import URLValidator
from django.core.exceptions import ValidationError

from edx_django_utils.monitoring import set_custom_metric
from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers
from openedx.core.djangoapps.theming.helpers import is_request_in_themed_site
from xmodule.util.xmodule_django import get_current_request_hostname
Expand Down Expand Up @@ -87,13 +88,15 @@ def marketing_link(name):
# don't try to reverse disabled marketing links
if link_map[name] is not None:
host_name = get_current_request_hostname()
if all([host_name and 'edge' in host_name, 'http' in link_map[name]]):
if link_map[name].startswith('http'):
return link_map[name]
else:
try:
return reverse(link_map[name])
except NoReverseMatch:
raise Http404
log.debug(u"Cannot find corresponding link for name: %s", name)
set_custom_metric('unresolved_marketing_link', name)
return '#'
else:
log.debug(u"Cannot find corresponding link for name: %s", name)
return '#'
Expand Down
15 changes: 15 additions & 0 deletions common/djangoapps/edxmako/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,21 @@ def test_override_marketing_link_invalid(self):
link = marketing_link('TOS')
self.assertEqual(link, expected_link)

@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
def test_link_map_url_reverse(self):
url_link_map = {
'ABOUT': 'dashboard',
'BAD_URL': 'foobarbaz',
}

with patch.dict('django.conf.settings.FEATURES', {'ENABLE_MKTG_SITE': False}):
with override_settings(MKTG_URL_LINK_MAP=url_link_map):
link = marketing_link('ABOUT')
assert link == '/dashboard'

link = marketing_link('BAD_URL')
assert link == '#'


class AddLookupTests(TestCase):
"""
Expand Down

0 comments on commit 4ac2c5e

Please sign in to comment.