From 78f94940f15f05a332ec3ec333c7661565b14f7c Mon Sep 17 00:00:00 2001
From: Manuel Kaufmann <humitos@gmail.com>
Date: Mon, 12 Nov 2018 15:44:01 +0100
Subject: [PATCH 1/6] Expose environment variables from database into build
 commands

Environment variables can be added from the Admin and they will be
used when running build commands. All the variables for that
particular project will be expose to all the commands.
---
 readthedocs/doc_builder/environments.py       |  4 +-
 readthedocs/projects/admin.py                 | 36 ++++++++++++++----
 .../0034_add_environment_variables.py         | 37 +++++++++++++++++++
 readthedocs/projects/models.py                | 17 +++++++++
 readthedocs/projects/tasks.py                 | 13 +++++--
 readthedocs/restapi/views/model_views.py      |  7 ++++
 6 files changed, 100 insertions(+), 14 deletions(-)
 create mode 100644 readthedocs/projects/migrations/0034_add_environment_variables.py

diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py
index 25443f05322..6a2dd4d71f6 100644
--- a/readthedocs/doc_builder/environments.py
+++ b/readthedocs/doc_builder/environments.py
@@ -440,7 +440,7 @@ class BuildEnvironment(BaseEnvironment):
         ProjectBuildsSkippedError,
         YAMLParseError,
         BuildTimeoutError,
-        MkDocsYAMLParseError
+        MkDocsYAMLParseError,
     )
 
     def __init__(self, project=None, version=None, build=None, config=None,
@@ -466,7 +466,7 @@ def __exit__(self, exc_type, exc_value, tb):
                 project=self.project.slug,
                 version=self.version.slug,
                 msg='Build finished',
-            )
+            ),
         )
         return ret
 
diff --git a/readthedocs/projects/admin.py b/readthedocs/projects/admin.py
index 35ba2bc92cd..b81cb9b3858 100644
--- a/readthedocs/projects/admin.py
+++ b/readthedocs/projects/admin.py
@@ -1,25 +1,38 @@
+# -*- coding: utf-8 -*-
 """Django administration interface for `projects.models`"""
 
-from __future__ import absolute_import
-from django.contrib import admin
-from django.contrib import messages
+from __future__ import (
+    absolute_import,
+    division,
+    print_function,
+    unicode_literals,
+)
+
+from django.contrib import admin, messages
 from django.contrib.admin.actions import delete_selected
 from django.utils.translation import ugettext_lazy as _
 from guardian.admin import GuardedModelAdmin
 
+from readthedocs.builds.models import Version
 from readthedocs.core.models import UserProfile
 from readthedocs.core.utils import broadcast
-from readthedocs.builds.models import Version
-from readthedocs.redirects.models import Redirect
 from readthedocs.notifications.views import SendNotificationView
+from readthedocs.redirects.models import Redirect
 
 from .forms import FeatureForm
-from .models import (Project, ImportedFile, Feature,
-                     ProjectRelationship, EmailHook, WebHook, Domain)
+from .models import (
+    Domain,
+    EmailHook,
+    EnvironmentVariable,
+    Feature,
+    ImportedFile,
+    Project,
+    ProjectRelationship,
+    WebHook,
+)
 from .notifications import ResourceUsageNotification
 from .tasks import remove_dir
 
-
 class ProjectSendNotificationView(SendNotificationView):
     notification_classes = [ResourceUsageNotification]
 
@@ -202,7 +215,14 @@ def project_count(self, feature):
         return feature.projects.count()
 
 
+class EnvironmentVariableAdmin(admin.ModelAdmin):
+    model = EnvironmentVariable
+    list_display = ('name', 'value', 'project', 'created')
+    search_fields = ('name', 'project__slug')
+
+
 admin.site.register(Project, ProjectAdmin)
+admin.site.register(EnvironmentVariable, EnvironmentVariableAdmin)
 admin.site.register(ImportedFile, ImportedFileAdmin)
 admin.site.register(Domain, DomainAdmin)
 admin.site.register(Feature, FeatureAdmin)
diff --git a/readthedocs/projects/migrations/0034_add_environment_variables.py b/readthedocs/projects/migrations/0034_add_environment_variables.py
new file mode 100644
index 00000000000..de9e3d18e5b
--- /dev/null
+++ b/readthedocs/projects/migrations/0034_add_environment_variables.py
@@ -0,0 +1,37 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.16 on 2018-11-12 13:57
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+import django.db.models.deletion
+import django_extensions.db.fields
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('projects', '0032_increase_webhook_maxsize'),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name='EnvironmentVariable',
+            fields=[
+                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
+                ('created', django_extensions.db.fields.CreationDateTimeField(auto_now_add=True, verbose_name='created')),
+                ('modified', django_extensions.db.fields.ModificationDateTimeField(auto_now=True, verbose_name='modified')),
+                ('name', models.CharField(help_text='Name of the environment variable', max_length=128)),
+                ('value', models.CharField(help_text='Value of the environment variable', max_length=256)),
+            ],
+            options={
+                'ordering': ('-modified', '-created'),
+                'get_latest_by': 'modified',
+                'abstract': False,
+            },
+        ),
+        migrations.AddField(
+            model_name='environmentvariable',
+            name='project',
+            field=models.ForeignKey(help_text='Project where this variable will be used', on_delete=django.db.models.deletion.CASCADE, to='projects.Project'),
+        ),
+    ]
diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py
index 78523387234..6c27873e7a8 100644
--- a/readthedocs/projects/models.py
+++ b/readthedocs/projects/models.py
@@ -15,6 +15,7 @@
 from django.db import models
 from django.utils.encoding import python_2_unicode_compatible
 from django.utils.translation import ugettext_lazy as _
+from django_extensions.db.models import TimeStampedModel
 from future.backports.urllib.parse import urlparse  # noqa
 from guardian.shortcuts import assign
 from taggit.managers import TaggableManager
@@ -1109,3 +1110,19 @@ def get_feature_display(self):
         implement this behavior.
         """
         return dict(self.FEATURES).get(self.feature_id, self.feature_id)
+
+
+class EnvironmentVariable(TimeStampedModel, models.Model):
+    name = models.CharField(
+        max_length=128,
+        help_text='Name of the environment variable',
+    )
+    value = models.CharField(
+        max_length=256,
+        help_text='Value of the environment variable',
+    )
+    project = models.ForeignKey(
+        Project,
+        on_delete=models.CASCADE,
+        help_text='Project where this variable will be used',
+    )
diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py
index 7dd35b72a06..69fcf158f17 100644
--- a/readthedocs/projects/tasks.py
+++ b/readthedocs/projects/tasks.py
@@ -548,7 +548,7 @@ def get_build(build_pk):
         if build_pk:
             build = api_v2.build(build_pk).get()
         private_keys = [
-            'project', 'version', 'resource_uri', 'absolute_uri'
+            'project', 'version', 'resource_uri', 'absolute_uri',
         ]
         return {
             key: val
@@ -605,7 +605,7 @@ def get_env_vars(self):
         env = {
             'READTHEDOCS': True,
             'READTHEDOCS_VERSION': self.version.slug,
-            'READTHEDOCS_PROJECT': self.project.slug
+            'READTHEDOCS_PROJECT': self.project.slug,
         }
 
         if self.config.conda is not None:
@@ -616,7 +616,7 @@ def get_env_vars(self):
                     self.project.doc_path,
                     'conda',
                     self.version.slug,
-                    'bin'
+                    'bin',
                 ),
             })
         else:
@@ -625,10 +625,15 @@ def get_env_vars(self):
                     self.project.doc_path,
                     'envs',
                     self.version.slug,
-                    'bin'
+                    'bin',
                 ),
             })
 
+        # Update environment from Project's specific environment variables
+        env.update(
+            api_v2.project(self.project.pk).environment_variables().get(),
+        )
+
         return env
 
     def set_valid_clone(self):
diff --git a/readthedocs/restapi/views/model_views.py b/readthedocs/restapi/views/model_views.py
index af3b573a839..0c785f68fae 100644
--- a/readthedocs/restapi/views/model_views.py
+++ b/readthedocs/restapi/views/model_views.py
@@ -117,6 +117,13 @@ def valid_versions(self, request, **kwargs):
             'flat': version_strings,
         })
 
+    @detail_route(permission_classes=[permissions.IsAdminUser])
+    def environment_variables(self, request, *_, **__):
+        return Response({
+            env.name: env.value
+            for env in self.get_object().environmentvariable_set.all()
+        })
+
     @detail_route()
     def translations(self, *_, **__):
         translations = self.get_object().translations.all()

From f70877335bba4d8d68495ac7550cccadc573a9ea Mon Sep 17 00:00:00 2001
From: Manuel Kaufmann <humitos@gmail.com>
Date: Tue, 13 Nov 2018 11:57:20 +0100
Subject: [PATCH 2/6] Rename migration file

---
 ...environment_variables.py => 0033_add_environment_variables.py} | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename readthedocs/projects/migrations/{0034_add_environment_variables.py => 0033_add_environment_variables.py} (100%)

diff --git a/readthedocs/projects/migrations/0034_add_environment_variables.py b/readthedocs/projects/migrations/0033_add_environment_variables.py
similarity index 100%
rename from readthedocs/projects/migrations/0034_add_environment_variables.py
rename to readthedocs/projects/migrations/0033_add_environment_variables.py

From 6e5af3e01ea44c7577ce09d2ce03ba33a1c7c838 Mon Sep 17 00:00:00 2001
From: Manuel Kaufmann <humitos@gmail.com>
Date: Tue, 13 Nov 2018 12:15:17 +0100
Subject: [PATCH 3/6] Use project detail endpoint with admin serializer

Add the `environment_variables` field to this serializer that will be
returned only when the user is admin.
---
 readthedocs/projects/models.py           | 7 ++++---
 readthedocs/projects/tasks.py            | 4 +---
 readthedocs/restapi/serializers.py       | 8 ++++++++
 readthedocs/restapi/views/model_views.py | 7 -------
 4 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py
index 6c27873e7a8..e333ef72602 100644
--- a/readthedocs/projects/models.py
+++ b/readthedocs/projects/models.py
@@ -900,6 +900,7 @@ class Meta:
 
     def __init__(self, *args, **kwargs):
         self.features = kwargs.pop('features', [])
+        self.environment_variables = kwargs.pop('environment_variables', {})
         ad_free = (not kwargs.pop('show_advertising', True))
         # These fields only exist on the API return, not on the model, so we'll
         # remove them to avoid throwing exceptions due to unexpected fields
@@ -1115,14 +1116,14 @@ def get_feature_display(self):
 class EnvironmentVariable(TimeStampedModel, models.Model):
     name = models.CharField(
         max_length=128,
-        help_text='Name of the environment variable',
+        help_text=_('Name of the environment variable'),
     )
     value = models.CharField(
         max_length=256,
-        help_text='Value of the environment variable',
+        help_text=_('Value of the environment variable'),
     )
     project = models.ForeignKey(
         Project,
         on_delete=models.CASCADE,
-        help_text='Project where this variable will be used',
+        help_text=_('Project where this variable will be used'),
     )
diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py
index 69fcf158f17..30495a23f93 100644
--- a/readthedocs/projects/tasks.py
+++ b/readthedocs/projects/tasks.py
@@ -630,9 +630,7 @@ def get_env_vars(self):
             })
 
         # Update environment from Project's specific environment variables
-        env.update(
-            api_v2.project(self.project.pk).environment_variables().get(),
-        )
+        env.update(self.project.environment_variables)
 
         return env
 
diff --git a/readthedocs/restapi/serializers.py b/readthedocs/restapi/serializers.py
index 0cf346c6d8b..4a53c9e612c 100644
--- a/readthedocs/restapi/serializers.py
+++ b/readthedocs/restapi/serializers.py
@@ -44,10 +44,17 @@ class ProjectAdminSerializer(ProjectSerializer):
     )
 
     show_advertising = serializers.SerializerMethodField()
+    environment_variables = serializers.SerializerMethodField()
 
     def get_show_advertising(self, obj):
         return obj.show_advertising
 
+    def get_environment_variables(self, obj):
+        return {
+            variable.name: variable.value
+            for variable in obj.environmentvariable_set.all()
+        }
+
     class Meta(ProjectSerializer.Meta):
         fields = ProjectSerializer.Meta.fields + (
             'enable_epub_build',
@@ -68,6 +75,7 @@ class Meta(ProjectSerializer.Meta):
             'has_valid_clone',
             'has_valid_webhook',
             'show_advertising',
+            'environment_variables',
         )
 
 
diff --git a/readthedocs/restapi/views/model_views.py b/readthedocs/restapi/views/model_views.py
index 0c785f68fae..af3b573a839 100644
--- a/readthedocs/restapi/views/model_views.py
+++ b/readthedocs/restapi/views/model_views.py
@@ -117,13 +117,6 @@ def valid_versions(self, request, **kwargs):
             'flat': version_strings,
         })
 
-    @detail_route(permission_classes=[permissions.IsAdminUser])
-    def environment_variables(self, request, *_, **__):
-        return Response({
-            env.name: env.value
-            for env in self.get_object().environmentvariable_set.all()
-        })
-
     @detail_route()
     def translations(self, *_, **__):
         translations = self.get_object().translations.all()

From 5e5f239cee43995647a2544a6b35c44665c4ed20 Mon Sep 17 00:00:00 2001
From: Manuel Kaufmann <humitos@gmail.com>
Date: Tue, 13 Nov 2018 15:07:37 +0100
Subject: [PATCH 4/6] Better work around environment_variables on Project and
 APIProject

---
 readthedocs/projects/models.py          | 23 +++++++++++++++++++--
 readthedocs/restapi/serializers.py      | 12 -----------
 readthedocs/rtd_tests/tests/test_api.py | 27 ++++++++++++++++++++++++-
 3 files changed, 47 insertions(+), 15 deletions(-)

diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py
index e333ef72602..2fc0dcf9e0f 100644
--- a/readthedocs/projects/models.py
+++ b/readthedocs/projects/models.py
@@ -870,13 +870,27 @@ def show_advertising(self):
         """
         Whether this project is ad-free
 
-        :return: ``True`` if advertising should be shown and ``False`` otherwise
+        :returns: ``True`` if advertising should be shown and ``False`` otherwise
+        :rtype: bool
         """
         if self.ad_free or self.gold_owners.exists():
             return False
 
         return True
 
+    @property
+    def environment_variables(self):
+        """
+        Environment variables to build this particular project.
+
+        :returns: dictionary with all the variables {name: value}
+        :rtype: dict
+        """
+        return {
+            variable.name: variable.value
+            for variable in self.environmentvariable_set.all()
+        }
+
 
 class APIProject(Project):
 
@@ -900,7 +914,7 @@ class Meta:
 
     def __init__(self, *args, **kwargs):
         self.features = kwargs.pop('features', [])
-        self.environment_variables = kwargs.pop('environment_variables', {})
+        environment_variables = kwargs.pop('environment_variables', {})
         ad_free = (not kwargs.pop('show_advertising', True))
         # These fields only exist on the API return, not on the model, so we'll
         # remove them to avoid throwing exceptions due to unexpected fields
@@ -914,6 +928,7 @@ def __init__(self, *args, **kwargs):
 
         # Overwrite the database property with the value from the API
         self.ad_free = ad_free
+        self._environment_variables = environment_variables
 
     def save(self, *args, **kwargs):
         return 0
@@ -926,6 +941,10 @@ def show_advertising(self):
         """Whether this project is ad-free (don't access the database)"""
         return not self.ad_free
 
+    @property
+    def environment_variables(self):
+        return self._environment_variables
+
 
 @python_2_unicode_compatible
 class ImportedFile(models.Model):
diff --git a/readthedocs/restapi/serializers.py b/readthedocs/restapi/serializers.py
index 4a53c9e612c..1f7607c04c3 100644
--- a/readthedocs/restapi/serializers.py
+++ b/readthedocs/restapi/serializers.py
@@ -43,18 +43,6 @@ class ProjectAdminSerializer(ProjectSerializer):
         slug_field='feature_id',
     )
 
-    show_advertising = serializers.SerializerMethodField()
-    environment_variables = serializers.SerializerMethodField()
-
-    def get_show_advertising(self, obj):
-        return obj.show_advertising
-
-    def get_environment_variables(self, obj):
-        return {
-            variable.name: variable.value
-            for variable in obj.environmentvariable_set.all()
-        }
-
     class Meta(ProjectSerializer.Meta):
         fields = ProjectSerializer.Meta.fields + (
             'enable_epub_build',
diff --git a/readthedocs/rtd_tests/tests/test_api.py b/readthedocs/rtd_tests/tests/test_api.py
index 2981bf5b137..d169f9bc51d 100644
--- a/readthedocs/rtd_tests/tests/test_api.py
+++ b/readthedocs/rtd_tests/tests/test_api.py
@@ -26,7 +26,7 @@
 from readthedocs.builds.models import Build, BuildCommandResult, Version
 from readthedocs.integrations.models import Integration
 from readthedocs.oauth.models import RemoteOrganization, RemoteRepository
-from readthedocs.projects.models import APIProject, Feature, Project
+from readthedocs.projects.models import APIProject, Feature, Project, EnvironmentVariable
 from readthedocs.restapi.views.integrations import GitHubWebhookView
 from readthedocs.restapi.views.task_views import get_status_data
 
@@ -704,6 +704,27 @@ def test_remote_organization_pagination(self):
         self.assertEqual(len(resp.data['results']), 25)  # page_size
         self.assertIn('?page=2', resp.data['next'])
 
+    def test_project_environment_variables(self):
+        user = get(User, is_staff=True)
+        project = get(Project, main_language_project=None)
+        get(
+            EnvironmentVariable,
+            name='TOKEN',
+            value='a1b2c3',
+            project=project,
+        )
+
+        client = APIClient()
+        client.force_authenticate(user=user)
+
+        resp = client.get('/api/v2/project/%s/' % (project.pk))
+        self.assertEqual(resp.status_code, 200)
+        self.assertIn('environment_variables', resp.data)
+        self.assertEqual(
+            resp.data['environment_variables'],
+            {'TOKEN': 'a1b2c3'},
+        )
+
     def test_init_api_project(self):
         project_data = {
             'name': 'Test Project',
@@ -716,13 +737,16 @@ def test_init_api_project(self):
         self.assertEqual(api_project.features, [])
         self.assertFalse(api_project.ad_free)
         self.assertTrue(api_project.show_advertising)
+        self.assertEqual(api_project.environment_variables, {})
 
         project_data['features'] = ['test-feature']
         project_data['show_advertising'] = False
+        project_data['environment_variables'] = {'TOKEN': 'a1b2c3'}
         api_project = APIProject(**project_data)
         self.assertEqual(api_project.features, ['test-feature'])
         self.assertTrue(api_project.ad_free)
         self.assertFalse(api_project.show_advertising)
+        self.assertEqual(api_project.environment_variables, {'TOKEN': 'a1b2c3'})
 
 
 class APIImportTests(TestCase):
@@ -1186,6 +1210,7 @@ def test_get_version_by_id(self):
                 'default_version': 'latest',
                 'description': '',
                 'documentation_type': 'sphinx',
+                'environment_variables': {},
                 'enable_epub_build': True,
                 'enable_pdf_build': True,
                 'features': ['allow_deprecated_webhooks'],

From 5fb61ac617741dc3c9adcacb64d5e7154deedab2 Mon Sep 17 00:00:00 2001
From: Manuel Kaufmann <humitos@gmail.com>
Date: Tue, 13 Nov 2018 15:33:56 +0100
Subject: [PATCH 5/6] Test case for UpdateDocsTaskStep.get_env_vars

---
 readthedocs/rtd_tests/tests/test_builds.py | 52 +++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

diff --git a/readthedocs/rtd_tests/tests/test_builds.py b/readthedocs/rtd_tests/tests/test_builds.py
index 6e91ffba12e..57939fad374 100644
--- a/readthedocs/rtd_tests/tests/test_builds.py
+++ b/readthedocs/rtd_tests/tests/test_builds.py
@@ -6,6 +6,7 @@
 )
 
 import mock
+import os
 from django.test import TestCase
 from django_dynamic_fixture import fixture, get
 
@@ -13,7 +14,7 @@
 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.projects.models import Project
+from readthedocs.projects.models import Project, EnvironmentVariable
 from readthedocs.projects.tasks import UpdateDocsTaskStep
 from readthedocs.rtd_tests.tests.test_config_integration import create_load
 
@@ -250,6 +251,55 @@ def test_save_config_in_build_model(self, load_config, api_v2):
         assert 'sphinx' in build_config
         assert build_config['doctype'] == 'sphinx'
 
+    def test_get_env_vars(self):
+        project = get(
+            Project,
+            slug='project',
+            documentation_type='sphinx',
+        )
+        get(
+            EnvironmentVariable,
+            name='TOKEN',
+            value='a1b2c3',
+            project=project,
+        )
+        build = get(Build)
+        version = get(Version, slug='1.8', project=project)
+        task = UpdateDocsTaskStep(
+            project=project, version=version, build={'id': build.pk},
+        )
+
+        # mock this object to make sure that we are NOT in a conda env
+        task.config = mock.Mock(conda=None)
+
+        env = {
+            'READTHEDOCS': True,
+            'READTHEDOCS_VERSION': version.slug,
+            'READTHEDOCS_PROJECT': project.slug,
+            'BIN_PATH': os.path.join(
+                project.doc_path,
+                'envs',
+                version.slug,
+                'bin',
+            ),
+            'TOKEN': 'a1b2c3',
+        }
+        self.assertEqual(task.get_env_vars(), env)
+
+        # mock this object to make sure that we are in a conda env
+        task.config = mock.Mock(conda=True)
+        env.update({
+            'CONDA_ENVS_PATH': os.path.join(project.doc_path, 'conda'),
+            'CONDA_DEFAULT_ENV': version.slug,
+            'BIN_PATH': os.path.join(
+                project.doc_path,
+                'conda',
+                version.slug,
+                'bin',
+            ),
+        })
+        self.assertEqual(task.get_env_vars(), env)
+
 
 class BuildModelTests(TestCase):
 

From 0edfd4e8b8d5441f08b0e968a73361e26dbfb408 Mon Sep 17 00:00:00 2001
From: Manuel Kaufmann <humitos@gmail.com>
Date: Tue, 13 Nov 2018 18:35:18 +0100
Subject: [PATCH 6/6] Lint ;)

---
 readthedocs/projects/admin.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/readthedocs/projects/admin.py b/readthedocs/projects/admin.py
index b81cb9b3858..f564b65297e 100644
--- a/readthedocs/projects/admin.py
+++ b/readthedocs/projects/admin.py
@@ -33,6 +33,7 @@
 from .notifications import ResourceUsageNotification
 from .tasks import remove_dir
 
+
 class ProjectSendNotificationView(SendNotificationView):
     notification_classes = [ResourceUsageNotification]