Make sure File.get_url_path() returns a relative path

Also make sure get_absolute_url() is used as much as possible when we
want the absolute url, instead of doing it ourselves using absolutify.

settings.py

@@ -42,6 +42,7 @@ ES_DEFAULT_NUM_REPLICAS = 0
 SITE_URL = os.environ.get('OLYMPIA_SITE_URL') or 'http://localhost:8000'
 DOMAIN = SERVICES_DOMAIN = urlparse(SITE_URL).netloc
 SERVICES_URL = SITE_URL
+EXTERNAL_SITE_URL = SITE_URL
 
 CODE_MANAGER_URL = os.environ.get('CODE_MANAGER_URL') or 'http://localhost:3000'
 

settings_test.py

@@ -23,7 +23,7 @@ DEBUG = False
 # We won't actually send an email.
 SEND_REAL_EMAIL = True
 
-SITE_URL = CDN_HOST = 'http://testserver'
+SITE_URL = CDN_HOST = EXTERNAL_SITE_URL = 'http://testserver'
 
 STATIC_URL = '%s/static/' % CDN_HOST
 MEDIA_URL = '%s/user-media/' % CDN_HOST

src/olympia/accounts/serializers.py

@@ -41,7 +41,7 @@ class BaseUserSerializer(serializers.ModelSerializer):
         current_user = getattr(request, 'user', None) if request else None
         # Only return your own profile url, and for developers.
         if obj == current_user or is_adminish(current_user) or obj.is_public:
-            return absolutify(obj.get_url_path())
+            return obj.get_absolute_url()
 
     # Used in subclasses.
     def get_permissions(self, obj):

src/olympia/addons/serializers.py

@@ -47,9 +47,7 @@ class FileSerializer(serializers.ModelSerializer):
                   'platform', 'size', 'status', 'url', 'permissions')
 
     def get_url(self, obj):
-        # File.get_url_path() is a little different, it's already absolute, but
-        # needs a src parameter that is appended as a query string.
-        return obj.get_url_path(src='')
+        return obj.get_absolute_url(src='')
 
 
 class PreviewSerializer(serializers.ModelSerializer):
@@ -427,9 +425,9 @@ class AddonSerializer(serializers.ModelSerializer):
         return getattr(obj, 'tag_list', [])
 
     def get_url(self, obj):
-        # Use get_detail_url(), get_url_path() does an extra check on
-        # current_version that is annoying in subclasses which don't want to
-        # load that version.
+        # Use absolutify(get_detail_url()), get_absolute_url() calls
+        # get_url_path() which does an extra check on current_version that is
+        # annoying in subclasses which don't want to load that version.
         return absolutify(obj.get_detail_url())
 
     def get_edit_url(self, obj):
@@ -714,11 +712,12 @@ class ESAddonAutoCompleteSerializer(ESAddonSerializer):
         model = Addon
 
     def get_url(self, obj):
-        # Addon.get_url_path() wants current_version to exist, but that's just
-        # a safeguard. We don't care and don't want to fetch the current
-        # version field to improve perf, so give it a fake one.
+        # Addon.get_absolute_url() calls get_url_path(), which wants
+        # current_version to exist, but that's just a safeguard. We don't care
+        # and don't want to fetch the current version field to improve perf, so
+        # give it a fake one.
         obj._current_version = Version()
-        return absolutify(obj.get_url_path())
+        return obj.get_absolute_url()
 
 
 class StaticCategorySerializer(serializers.Serializer):

src/olympia/addons/tests/test_serializers.py

@@ -45,7 +45,7 @@ class AddonSerializerOutputTestMixin(object):
             'id': author.pk,
             'name': author.name,
             'picture_url': None,
-            'url': absolutify(author.get_url_path()),
+            'url': author.get_absolute_url(),
             'username': author.username,
         }
 
@@ -93,7 +93,7 @@ class AddonSerializerOutputTestMixin(object):
             amo.PLATFORM_CHOICES_API[file_.platform])
         assert result_file['size'] == file_.size
         assert result_file['status'] == amo.STATUS_CHOICES_API[file_.status]
-        assert result_file['url'] == file_.get_url_path(src='')
+        assert result_file['url'] == file_.get_absolute_url(src='')
         assert result_file['permissions'] == file_.webext_permissions_list
 
         assert data['edit_url'] == absolutify(
@@ -289,7 +289,7 @@ class AddonSerializerOutputTestMixin(object):
         assert 'theme_data' not in result
         assert set(result['tags']) == {'some_tag', 'some_other_tag'}
         assert result['type'] == 'extension'
-        assert result['url'] == absolutify(self.addon.get_url_path())
+        assert result['url'] == self.addon.get_absolute_url()
         assert result['weekly_downloads'] == self.addon.weekly_downloads
 
         return result
@@ -880,7 +880,7 @@ class TestESAddonSerializerOutput(AddonSerializerOutputTestMixin, ESTestCase):
         assert data == {
             'id': author.pk,
             'name': author.name,
-            'url': absolutify(author.get_url_path()),
+            'url': author.get_absolute_url(),
             'username': author.username,
         }
 
@@ -968,7 +968,7 @@ class TestVersionSerializerOutput(TestCase):
         assert result['files'][0]['platform'] == 'windows'
         assert result['files'][0]['size'] == first_file.size
         assert result['files'][0]['status'] == 'public'
-        assert result['files'][0]['url'] == first_file.get_url_path(src='')
+        assert result['files'][0]['url'] == first_file.get_absolute_url(src='')
 
         assert result['files'][1]['id'] == second_file.pk
         assert result['files'][1]['created'] == (
@@ -981,7 +981,8 @@ class TestVersionSerializerOutput(TestCase):
         assert result['files'][1]['platform'] == 'mac'
         assert result['files'][1]['size'] == second_file.size
         assert result['files'][1]['status'] == 'public'
-        assert result['files'][1]['url'] == second_file.get_url_path(src='')
+        assert result['files'][1]['url'] == second_file.get_absolute_url(
+            src='')
 
         assert result['channel'] == 'listed'
         assert result['edit_url'] == absolutify(addon.get_dev_url(
@@ -1168,7 +1169,7 @@ class TestLanguageToolsSerializerOutput(TestCase):
         assert result['slug'] == self.addon.slug
         assert result['target_locale'] == self.addon.target_locale
         assert result['type'] == 'language'
-        assert result['url'] == absolutify(self.addon.get_url_path())
+        assert result['url'] == self.addon.get_absolute_url()
         assert 'current_compatible_version' not in result
         assert 'locale_disambiguation' not in result
 
@@ -1266,7 +1267,7 @@ class TestESAddonAutoCompleteSerializer(ESTestCase):
         assert result['icon_url'] == absolutify(self.addon.get_icon_url(64))
         assert result['is_recommended'] == self.addon.is_recommended is False
         assert result['type'] == 'extension'
-        assert result['url'] == absolutify(self.addon.get_url_path())
+        assert result['url'] == self.addon.get_absolute_url()
 
     def test_translations(self):
         translated_name = {
@@ -1398,7 +1399,7 @@ class TestReplacementAddonSerializer(TestCase):
         assert result['replacement'] == [u'newstuff@mozilla']
 
         # But urls aren't resolved - and don't break everything
-        rep.update(path=absolutify(addon.get_url_path()))
+        rep.update(path=addon.get_absolute_url())
         result = self.serialize(rep)
         assert result['guid'] == u'legacy@mozilla'
         assert result['replacement'] == []

src/olympia/devhub/templates/devhub/addons/submit/done.html

@@ -65,7 +65,7 @@
   </p>
   <p>
     {% set file = uploaded_version.all_files[-1] %}
-    <a class="button" id="download-addon-url" download href="{{ file.get_url_path('devhub') }}">{{
+    <a class="button" id="download-addon-url" download href="{{ file.get_absolute_url('devhub') }}">{{
         _("Download {0}")|format_html(file.pretty_filename()) }}</a>
   </p>
 {% endif %}

src/olympia/devhub/templates/devhub/includes/version_file.html

@@ -1,6 +1,6 @@
 <tr>
   <td>
-    <a href="{{ file.get_url_path('devhub') }}">
+    <a href="{{ file.get_absolute_url('devhub') }}">
       {{ file.pretty_filename() }}</a>
   </td>
   <td>{{ file.size|filesizeformat(binary=True) }}</td>

src/olympia/devhub/tests/test_views_submit.py

@@ -1746,7 +1746,7 @@ class TestAddonSubmitFinish(TestSubmitBase):
         assert len(links) == 2
         # First link is to the file download.
         file_ = latest_version.all_files[-1]
-        assert links[0].attrib['href'] == file_.get_url_path('devhub')
+        assert links[0].attrib['href'] == file_.get_absolute_url('devhub')
         assert links[0].text == (
             'Download %s' % file_.filename)
         # Second back to my submissions.
@@ -1814,7 +1814,7 @@ class TestAddonSubmitFinish(TestSubmitBase):
         assert len(links) == 2
         # First link is to the file download.
         file_ = latest_version.all_files[-1]
-        assert links[0].attrib['href'] == file_.get_url_path('devhub')
+        assert links[0].attrib['href'] == file_.get_absolute_url('devhub')
         assert links[0].text == (
             'Download %s' % file_.filename)
         # Second back to my submissions.

src/olympia/discovery/tests/test_views.py

@@ -42,7 +42,7 @@ class DiscoveryTestMixin(object):
 
         assert result_file['size'] == file_.size
         assert result_file['status'] == amo.STATUS_CHOICES_API[file_.status]
-        assert result_file['url'] == file_.get_url_path(src='')
+        assert result_file['url'] == file_.get_absolute_url(src='')
         assert result_file['permissions'] == file_.webext_permissions_list
 
     def _check_disco_addon(self, result, item, flat_name=False):

src/olympia/files/models.py

@@ -29,7 +29,7 @@ from olympia.amo.fields import PositiveAutoField
 from olympia.amo.models import ManagerBase, ModelBase, OnChangeMixin
 from olympia.amo.storage_utils import copy_stored_file, move_stored_file
 from olympia.amo.templatetags.jinja_helpers import (
-    absolutify, urlparams, user_media_path, user_media_url)
+    urlparams, user_media_path, user_media_url)
 from olympia.amo.urlresolvers import reverse
 from olympia.applications.models import AppVersion
 from olympia.files.utils import SafeZip, get_sha256, write_crx_as_xpi
@@ -134,7 +134,7 @@ class File(OnChangeMixin, ModelBase):
         if attachment:
             kwargs['type'] = 'attachment'
         url = os.path.join(reverse(view_name, kwargs=kwargs), self.filename)
-        return absolutify(urlparams(url, src=src))
+        return urlparams(url, src=src)
 
     @classmethod
     def from_upload(cls, upload, version, platform, parsed_data=None):

src/olympia/files/tests/test_models.py

@@ -70,23 +70,30 @@ class TestFile(TestCase, amo.tests.AMOPaths):
     fixtures = ['base/addon_3615', 'base/addon_5579']
 
     def test_get_absolute_url(self):
-        f = File.objects.get(id=67442)
-        url = f.get_absolute_url(src='src')
+        file_ = File.objects.get(id=67442)
+        url = file_.get_absolute_url(src='foo')
         expected = ('/firefox/downloads/file/67442/'
-                    'delicious_bookmarks-2.1.072-fx.xpi?src=src')
+                    'delicious_bookmarks-2.1.072-fx.xpi?src=foo')
         assert url.endswith(expected), url
 
     def test_get_url_path(self):
         file_ = File.objects.get(id=67442)
-        assert absolutify(file_.get_url_path('src')) == (
-            file_.get_absolute_url(src='src'))
+        assert absolutify(file_.get_url_path('foo')) == (
+            file_.get_absolute_url(src='foo'))
 
     def test_get_url_path_attachment(self):
+        file_ = File.objects.get(id=67442)
+        expected = ('/firefox/downloads/file/67442'
+                    '/type:attachment/delicious_bookmarks-2.1.072-fx.xpi'
+                    '?src=foo')
+        assert file_.get_url_path('foo', attachment=True) == expected
+
+    def test_absolute_url_attachment(self):
         file_ = File.objects.get(id=67442)
         expected = ('http://testserver/firefox/downloads/file/67442'
                     '/type:attachment/delicious_bookmarks-2.1.072-fx.xpi'
-                    '?src=src')
-        assert file_.get_url_path('src', attachment=True) == expected
+                    '?src=foo')
+        assert file_.get_absolute_url('foo', attachment=True) == expected
 
     def check_delete(self, file_, filename):
         """Test that when the File object is deleted, it is removed from the

src/olympia/reviewers/templates/reviewers/includes/files_view.html

@@ -4,7 +4,7 @@
 {% for file in distinct_files %}
 <li class="file-info">
   <span class="light">
-    <strong><a href="{{ file[0].get_url_path('reviewer') }}" class="reviewers-install"
+    <strong><a href="{{ file[0].get_absolute_url('reviewer') }}" class="reviewers-install"
         data-type="{{ amo.ADDON_SLUGS[addon.type] }}">{{ file[1] }}</a></strong>
     <div>
       {{ file_review_status(addon, file[0]) }}

src/olympia/reviewers/tests/test_views.py

@@ -2835,11 +2835,11 @@ class TestReview(ReviewBase):
         items = pq(response.content)('#review-files .files .file-info')
         assert items.length == 1
 
-        f = self.version.all_files[0]
+        file_ = self.version.all_files[0]
         expected = [
-            ('All Platforms', f.get_url_path('reviewer')),
-            ('Validation',
-             reverse('devhub.file_validation', args=[self.addon.slug, f.id])),
+            ('All Platforms', file_.get_absolute_url('reviewer')),
+            ('Validation', reverse(
+                'devhub.file_validation', args=[self.addon.slug, file_.id])),
             ('Contents', None),
         ]
         check_links(expected, items.find('a'), verify=False)