Update build list and detail page UX

readthedocs/api/v2/serializers.py

@@ -118,6 +118,7 @@ class BuildSerializer(serializers.ModelSerializer):
     version_slug = serializers.ReadOnlyField(source='version.slug')
     docs_url = serializers.ReadOnlyField(source='version.get_absolute_url')
     state_display = serializers.ReadOnlyField(source='get_state_display')
+    vcs_url = serializers.ReadOnlyField(source='version.vcs_url')
     # Jsonfield needs an explicit serializer
     # https://github.com/dmkoch/django-jsonfield/issues/188#issuecomment-300439829
     config = serializers.JSONField(required=False)

readthedocs/builds/constants.py

@@ -87,3 +87,6 @@
         'description': 'The build succeeded!',
     },
 }
+
+GITHUB_EXTERNAL_VERSION_NAME = 'Pull Request'
+GENERIC_EXTERNAL_VERSION_NAME = 'External Version'

readthedocs/builds/models.py

@@ -22,6 +22,8 @@
 from readthedocs.projects.constants import (
     BITBUCKET_URL,
     GITHUB_URL,
+    GITHUB_PULL_REQUEST_URL,
+    GITHUB_PULL_REQUEST_COMMIT_URL,
     GITLAB_URL,
     PRIVACY_CHOICES,
     PRIVATE,
@@ -36,6 +38,8 @@
     BUILD_STATE_FINISHED,
     BUILD_STATE_TRIGGERED,
     BUILD_TYPES,
+    GENERIC_EXTERNAL_VERSION_NAME,
+    GITHUB_EXTERNAL_VERSION_NAME,
     INTERNAL,
     LATEST,
     NON_REPOSITORY_VERSIONS,
@@ -158,9 +162,22 @@ def vcs_url(self):
         """
         Generate VCS (github, gitlab, bitbucket) URL for this version.
 
-        Branch/Tag Example: https://github.com/rtfd/readthedocs.org/tree/3.4.2/.
-        Pull/merge Request Example: https://github.com/rtfd/readthedocs.org/pull/9999/.
+        Example: https://github.com/rtfd/readthedocs.org/tree/3.4.2/.
+        External Version Example: https://github.com/rtfd/readthedocs.org/pull/99/commits/b630b630/.
         """
+        if self.type == EXTERNAL:
+            if 'github' in self.project.repo:
+                user, repo = get_github_username_repo(self.project.repo)
+                # Get Github Pull Request Commit URL.
+                return GITHUB_PULL_REQUEST_COMMIT_URL.format(
+                    user=user,
+                    repo=repo,
+                    number=self.verbose_name,
+                    commit=self.commit_name
+                )
+            # TODO: Add VCS ULR for other Git Providers
+            return ''
+
         url = ''
         if self.slug == STABLE:
             slug_url = self.ref
@@ -169,25 +186,12 @@ def vcs_url(self):
         else:
             slug_url = self.slug
 
-        if self.type == EXTERNAL:
-            if 'github' in self.project.repo:
-                slug_url = self.verbose_name
-                url = f'/pull/{slug_url}/'
-
-            if 'gitlab' in self.project.repo:
-                slug_url = self.identifier
-                url = f'/merge_requests/{slug_url}/'
+        if ('github' in self.project.repo) or ('gitlab' in self.project.repo):
+            url = f'/tree/{slug_url}/'
 
-            if 'bitbucket' in self.project.repo:
-                slug_url = self.identifier
-                url = f'/pull-requests/{slug_url}'
-        else:
-            if ('github' in self.project.repo) or ('gitlab' in self.project.repo):
-                url = f'/tree/{slug_url}/'
-
-            if 'bitbucket' in self.project.repo:
-                slug_url = self.identifier
-                url = f'/src/{slug_url}'
+        if 'bitbucket' in self.project.repo:
+            slug_url = self.identifier
+            url = f'/src/{slug_url}'
 
         # TODO: improve this replacing
         return self.project.repo.replace('git://', 'https://').replace('.git', '') + url
@@ -539,6 +543,24 @@ def get_bitbucket_url(self, docroot, filename, source_suffix='.rst'):
             source_suffix=source_suffix,
         )
 
+    def get_external_version_url(self):
+        """Return a Pull/Merge Request URL."""
+        repo_url = self.project.repo
+
+        if 'github' in repo_url:
+            user, repo = get_github_username_repo(repo_url)
+            if not user and not repo:
+                return ''
+
+            repo = repo.rstrip('/')
+            return GITHUB_PULL_REQUEST_URL.format(
+                user=user,
+                repo=repo,
+                number=self.verbose_name
+            )
+        # TODO: Add External Version ULR for other Git Providers
+        return ''
+
 
 class APIVersion(Version):
 
@@ -756,6 +778,20 @@ def is_stale(self):
         mins_ago = timezone.now() - datetime.timedelta(minutes=5)
         return self.state == BUILD_STATE_TRIGGERED and self.date < mins_ago
 
+    @property
+    def is_external(self):
+        return self.version.type == EXTERNAL
+
+    @property
+    def external_version_name(self):
+        if self.is_external:
+            try:
+                if self.project.remote_repository.account.provider == 'github':
+                    return GITHUB_EXTERNAL_VERSION_NAME
+            except Exception:
+                return GENERIC_EXTERNAL_VERSION_NAME
+        return None
+
     def using_latest_config(self):
         return int(self.config.get('version', '1')) == LATEST_CONFIGURATION_VERSION
 

readthedocs/builds/static-src/builds/js/detail.js

@@ -52,6 +52,7 @@ function BuildDetailView(instance) {
     });
     self.commit = ko.observable(instance.commit);
     self.docs_url = ko.observable(instance.docs_url);
+    self.vcs_url = ko.observable(instance.vcs_url);
 
     /* Others */
     self.legacy_output = ko.observable(false);
@@ -72,6 +73,7 @@ function BuildDetailView(instance) {
             self.length(data.length);
             self.commit(data.commit);
             self.docs_url(data.docs_url);
+            self.vcs_url(data.vcs_url);
             var n;
             for (n in data.commands) {
                 var command = data.commands[n];

readthedocs/builds/views.py

@@ -84,21 +84,6 @@ def post(self, request, project_slug):
 
 class BuildList(BuildBase, BuildTriggerMixin, ListView):
 
-    def get_queryset(self):
-        # this is used to include only internal version
-        # builds in the build list page
-        self.project_slug = self.kwargs.get('project_slug', None)
-        self.project = get_object_or_404(
-            Project.objects.protected(self.request.user),
-            slug=self.project_slug,
-        )
-        queryset = Build.objects.public(
-            user=self.request.user,
-            project=self.project,
-        ).select_related('project', 'version')
-
-        return queryset
-
     def get_context_data(self, **kwargs):
         context = super().get_context_data(**kwargs)
 

readthedocs/projects/constants.py

@@ -325,6 +325,14 @@
     'https://github.com/{user}/{repo}/'
     '{action}/{version}{docroot}{path}{source_suffix}'
 )
+GITHUB_PULL_REQUEST_URL = (
+    'https://github.com/{user}/{repo}/'
+    'pull/{number}'
+)
+GITHUB_PULL_REQUEST_COMMIT_URL = (
+    'https://github.com/{user}/{repo}/'
+    'pull/{number}/commits/{commit}'
+)
 BITBUCKET_URL = (
     'https://bitbucket.org/{user}/{repo}/'
     'src/{version}{docroot}{path}{source_suffix}'

readthedocs/projects/views/public.py

@@ -98,7 +98,7 @@ def get_context_data(self, **kwargs):
         context = super().get_context_data(**kwargs)
 
         project = self.get_object()
-        context['versions'] = Version.objects.public(
+        context['versions'] = Version.internal.public(
             user=self.request.user,
             project=project,
         )
@@ -274,7 +274,7 @@ def project_versions(request, project_slug):
         slug=project_slug,
     )
 
-    versions = Version.objects.public(
+    versions = Version.internal.public(
         user=request.user,
         project=project,
         only_active=False,

readthedocs/rtd_tests/tests/test_builds.py

@@ -3,14 +3,23 @@
 import os
 
 import mock
+from django.contrib.auth.models import User
 from django.test import TestCase
 from django_dynamic_fixture import fixture, get
 from django.utils import timezone
 
+from allauth.socialaccount.models import SocialAccount
+
+from readthedocs.builds.constants import (
+    EXTERNAL,
+    GITHUB_EXTERNAL_VERSION_NAME,
+    GENERIC_EXTERNAL_VERSION_NAME
+)
 from readthedocs.builds.models import Build, Version
 from readthedocs.doc_builder.config import load_yaml_config
 from readthedocs.doc_builder.environments import LocalBuildEnvironment
 from readthedocs.doc_builder.python_environments import Virtualenv
+from readthedocs.oauth.models import RemoteRepository
 from readthedocs.projects.models import EnvironmentVariable, Project
 from readthedocs.projects.tasks import UpdateDocsTaskStep
 from readthedocs.rtd_tests.tests.test_config_integration import create_load
@@ -369,7 +378,13 @@ def test_get_env_vars(self):
 class BuildModelTests(TestCase):
 
     def setUp(self):
+        self.eric = User(username='eric')
+        self.eric.set_password('test')
+        self.eric.save()
+
         self.project = get(Project)
+        self.project.users.add(self.eric)
+
         self.version = get(Version, project=self.project)
 
     def test_get_previous_build(self):
@@ -582,3 +597,73 @@ def test_using_latest_config(self):
         build.save()
 
         self.assertTrue(build.using_latest_config())
+
+    def test_build_is_external(self):
+        # Turn the build version to EXTERNAL type.
+        self.version.type = EXTERNAL
+        self.version.save()
+
+        external_build = get(
+            Build,
+            project=self.project,
+            version=self.version,
+            config={'version': 1},
+        )
+
+        self.assertTrue(external_build.is_external)
+
+    def test_build_is_not_external(self):
+        build = get(
+            Build,
+            project=self.project,
+            version=self.version,
+            config={'version': 1},
+        )
+
+        self.assertFalse(build.is_external)
+
+    def test_no_external_version_name(self):
+        build = get(
+            Build,
+            project=self.project,
+            version=self.version,
+            config={'version': 1},
+        )
+
+        self.assertEqual(build.external_version_name, None)
+
+    def test_external_version_name_github(self):
+        social_account = get(SocialAccount, provider='github')
+        remote_repo = get(
+            RemoteRepository,
+            account=social_account,
+            project=self.project
+        )
+        remote_repo.users.add(self.eric)
+
+        external_version = get(Version, project=self.project, type=EXTERNAL)
+        external_build = get(
+            Build, project=self.project, version=external_version
+        )
+
+        self.assertEqual(
+            external_build.external_version_name,
+            GITHUB_EXTERNAL_VERSION_NAME
+        )
+
+    def test_external_version_name_generic(self):
+        # Turn the build version to EXTERNAL type.
+        self.version.type = EXTERNAL
+        self.version.save()
+
+        external_build = get(
+            Build,
+            project=self.project,
+            version=self.version,
+            config={'version': 1},
+        )
+
+        self.assertEqual(
+            external_build.external_version_name,
+            GENERIC_EXTERNAL_VERSION_NAME
+        )

readthedocs/rtd_tests/tests/test_project_views.py

@@ -406,18 +406,25 @@ def test_project_download_media(self):
         response = self.client.get(url)
         self.assertEqual(response.status_code, 302)
 
-    def test_project_detail_view_shows_external_versons(self):
+    def test_project_detail_view_only_shows_internal_versons(self):
         url = reverse('projects_detail', args=[self.pip.slug])
         response = self.client.get(url)
         self.assertEqual(response.status_code, 200)
-        self.assertIn(self.external_version, response.context['versions'])
+        self.assertNotIn(self.external_version, response.context['versions'])
 
     def test_project_downloads_only_shows_internal_versons(self):
         url = reverse('project_downloads', args=[self.pip.slug])
         response = self.client.get(url)
         self.assertEqual(response.status_code, 200)
         self.assertNotIn(self.external_version, response.context['versions'])
 
+    def test_project_versions_only_shows_internal_versons(self):
+        url = reverse('project_version_list', args=[self.pip.slug])
+        response = self.client.get(url)
+        self.assertEqual(response.status_code, 200)
+        self.assertNotIn(self.external_version, response.context['active_versions'])
+        self.assertNotIn(self.external_version, response.context['inactive_versions'])
+
 
 class TestPrivateViews(MockBuildTestCase):
     def setUp(self):

readthedocs/rtd_tests/tests/test_version.py

@@ -46,7 +46,10 @@ def setUp(self):
 class TestVersionModel(VersionMixin, TestCase):
 
     def test_vcs_url_for_external_version(self):
-        expected_url = f'https://github.com/pypa/pip/pull/{self.external_version.verbose_name}/'
+        expected_url = 'https://github.com/pypa/pip/pull/{number}/commits/{sha}'.format(
+            number=self.external_version.verbose_name,
+            sha=self.external_version.identifier
+        )
         self.assertEqual(self.external_version.vcs_url, expected_url)
 
     def test_vcs_url_for_latest_version(self):
@@ -66,3 +69,9 @@ def test_commit_name_for_latest_version(self):
 
     def test_commit_name_for_external_version(self):
         self.assertEqual(self.external_version.commit_name, self.external_version.identifier)
+
+    def test_get_external_version_url(self):
+        expected_url = 'https://github.com/pypa/pip/pull/{number}'.format(
+            number=self.external_version.verbose_name,
+        )
+        self.assertEqual(self.external_version.get_external_version_url(), expected_url)

readthedocs/templates/builds/build_detail.html

@@ -79,15 +79,19 @@
 
     <div class="build-version">
       <span class="build-version-slug">
-        {% if request.user|is_admin:project %}
+        {% if request.user|is_admin:project and not build.is_external %}
           <a href="{% url "project_version_detail" build.version.project.slug build.version.slug %}">{{ build.version.slug }}</a>
-        {% else  %}
+        {% elif build.is_external %}
+          {% blocktrans with build.external_version_name as external_version_name %}
+            <b>{{ external_version_name }}</b>
+          {% endblocktrans %}
+            #<a href="{{ build.version.get_external_version_url }}">{{ build.version.verbose_name }}</a>
+        {% else %}
           {{ build.version.slug }}
         {% endif %}
       </span>
-      <span class="build-commit"
-            data-bind="visible: commit">
-        (<span data-bind="text: commit">{{ build.commit }}</span>)
+      <span class="build-commit" data-bind="visible: commit">
+          (<a data-bind="attr: {href: vcs_url}"><span data-bind="text: commit">{{ build.commit }}</span></a>)
       </span>
     </div>