Add downloads factories

[jaap3]

Recreate the downloads section by importing data from the python.org API

[jaap3]

This depends on the changes in #1314 and #1315

[jaap3]

Tested this with the new API and made some corrections to support it. While I was at it I also fixed some other issues.

[berkerpeksag]

This is great! I needed this when I was working on #1370 and I noticed that I didn't expose the is_latest field.

[berkerpeksag]

Could you add is_latest as well? We need it to make update_supernav() work. 5ad3ebf should expose it in both APIs.

[jaap3]

There's no need to add is_latest to the factory, if it's on the model then it's also on the factory. So in this case is_latest will default to False. In initial_data the release is created like this ReleaseFactory(**obj), so all fields that are available in the API are set on the newly created instance. So this should already set is_latest correctly.

[berkerpeksag]

Ah, yes, I missed you've added is_published = True because its default value is False. Good point!

[berkerpeksag]

Style nit: Newline can be dropped.

[berkerpeksag]

Did you skip adding release pages for now or did you get blocked by something? I think we can add a # TODO comment if it takes too long to implement it.

[jaap3]

I think I got blocked on something, but I cannot remember what. I'll add a TODO for now

[berkerpeksag]

Great! Feel free to file an issue if you remember what was the blocker.

[berkerpeksag]

I think there's a bug here. After I ran create_initial_data the supernav for Downloads menu didn't change:

I created a temporary update_boxes command:

from django.core.management import BaseCommand, call_command


class Command(BaseCommand):

    def handle(self, **options):
        from downloads.models import update_supernav
        update_supernav()

After I ran it:

I also applied this patch:

diff --git a/downloads/models.py b/downloads/models.py
index 305143b..72f40e4 100644
--- a/downloads/models.py
+++ b/downloads/models.py
@@ -126,35 +126,34 @@ class Release(ContentManageable, NameSlugModel):
 
 
 def update_supernav():
-    try:
-        latest_python2 = Release.objects.latest_python2()
-    except Release.DoesNotExist:
-        latest_python2 = None
-
     try:
         latest_python3 = Release.objects.latest_python3()
     except Release.DoesNotExist:
-        latest_python3 = None
+        # If we don't have a Python 3 release, we don't need to bother updating supernav.
+        return
 
     python_files = []
     for o in OS.objects.all():
         data = {
             'os': o,
-            'python2': None,
             'python3': None,
         }
 
-        if latest_python2:
-            data['python2'] = latest_python2.download_file_for_os(o.slug)
-
         if latest_python3:
-            data['python3'] = latest_python3.download_file_for_os(o.slug)
+            release_file = latest_python3.download_file_for_os(o.slug)
+            if not release_file:
+                return
+            data['python3'] = release_file
 
         python_files.append(data)
 
+    if not python_files:
+        return
+
+    if not all(f['python3'] for f in python_files):
+        return
+
     content = render_to_string('downloads/supernav.html', {
-        'latest_python2': latest_python2,
-        'latest_python3': latest_python3,
         'python_files': python_files,
     })
 
@@ -166,7 +165,20 @@ def update_supernav():
         }
     )
 
+
+def update_download_sources_box():
     # Update latest Sources box on Download landing page
+
+    try:
+        latest_python2 = Release.objects.latest_python2()
+    except Release.DoesNotExist:
+        latest_python2 = None
+
+    try:
+        latest_python3 = Release.objects.latest_python3()
+    except Release.DoesNotExist:
+        latest_python3 = None
+
     if latest_python2:
         latest_python2_source = latest_python2.download_file_for_os('source')
     else:
@@ -177,6 +189,9 @@ def update_supernav():
     else:
         latest_python3_source = None
 
+    if not latest_python2_source or not latest_python3_source:
+        return
+
     source_content = render_to_string('downloads/download-sources-box.html', {
         'latest_python2_source': latest_python2_source,
         'latest_python3_source': latest_python3_source,
@@ -260,14 +275,14 @@ def purge_fastly_download_pages(sender, instance, **kwargs):
 
 
 @receiver(post_save, sender=Release)
-def update_download_supernav(sender, instance, **kwargs):
-    """ Update download supernav """
+def update_download_supernav_and_boxes(sender, instance, **kwargs):
     # Skip in fixtures
     if kwargs.get('raw', False):
         return
 
     if instance.is_published:
         update_supernav()
+        update_download_sources_box()
         update_homepage_download_box()

[berkerpeksag]

I think I figured it out. Due to how initial_data() functions and Django signals work, release.files.all() will always return no records when a Release instance is saved. This can be solved by registering a post_save callback for ReleaseFile. However, I think it's unnecessary and making our signal handlers more robust would be a better way to solve this, so supernav boxes won't be updated unnecessarily (as shown below, without using my inline patch, against current master with this PR applied, after I ran create_initial_data):

I will work on my inline patch to make signal handlers more robust.

[berkerpeksag]

LGTM! I already tested this last week and will merge once Travis is happy.

[jaap3]

Thanks for merging, glad you find it useful :)

I just did a run where I added a print(obj['release_page']) (not sure why I did not do that saturday) and noticed that the field never has a value (always None). I'm fairly sure it used to have a value (maybe it was API V1?).

[berkerpeksag]

I'm fairly sure it used to have a value (maybe it was API V1?).

It could be a bug I introduced while I was porting API v1 to DRF. Could you check https://github.com/python/pythondotorg/blob/master/downloads/tests/test_views.py whether there's a test for the release_page field?

[jaap3]

There's test_post_release that asserts self.assertIn(data['release_page'], content['release_page']).

However, I just did a run against API V1 and the release_page field is always None there as well. So I'm not sure what's going on.

To use API v1 replace these bits:

class APISession(requests.Session):
    base_url = 'https://www.python.org/api/v1/'

and

def initial_data():
    [...]
    with APISession() as session:
        for key, resource_uri in [
            ('oses', 'downloads/os/'),
            ('releases', 'downloads/release/'),
            ('release_files', 'downloads/release_file/'),
        ]:
            while resource_uri:
                response = session.get(resource_uri, params={'limit': 1000}).json()
                for obj in response['objects']:
                    objects[key][_get_id(obj, 'resource_uri')] = obj
                resource_uri = response['meta']['next']

[berkerpeksag]

We don't use the release_page field much these days. You may need to create a release with a release page locally to verify it.

[jaap3]

Yeah, seems like release_page is a legacy field, the fix to #956 actually removes a lot of release pages.