Merge pull request #12108 from edx/bderusha/api-status-notification

Email notification

lms/envs/common.py

@@ -2882,3 +2882,5 @@ THEME_CACHE_TIMEOUT = 30 * 60
 # API access management
 API_ACCESS_MANAGER_EMAIL = 'api-access@example.com'
 API_ACCESS_FROM_EMAIL = 'api-requests@example.com'
+API_DOCUMENTATION_URL = 'http://edx.readthedocs.org/projects/edx-platform-api/en/latest/overview.html'
+AUTH_DOCUMENTATION_URL = 'http://edx.readthedocs.org/projects/edx-platform-api/en/latest/authentication.html'

lms/templates/api_admin/api_access_request_email_approved.txt

@@ -0,0 +1,12 @@
+## mako
+Dear ${name},
+
+Your request for access to the ${platform_name} Course Catalog API has been approved. Sign in at ${api_management_url} using the same account you used to request API access to generate your API client credentials.
+
+Notes:
+- ${platform_name} recommends that you bookmark the ${api_management_url} page for easy access.
+- For more information about how to use the API client credentials to authenticate your API request, see the OAuth 2.0 documentation at ${authentication_docs_url}.
+- For questions about using this API, see the documentation at ${api_docs_url} or contact ${support_email_address}.
+
+Thanks!
+The ${platform_name} API team

lms/templates/api_admin/api_access_request_email_denied.txt

@@ -0,0 +1,6 @@
+## mako
+Dear ${name},
+
+Your request for access to the ${platform_name} Course Catalog API has been denied. If you think this is an error, or for other questions about using this API, contact ${support_email_address}.
+
+- The ${platform_name} API team

lms/templates/api_admin/api_access_request_email_new_request.txt

@@ -0,0 +1,8 @@
+## mako
+We have received the following request to use the Course Catalog API. Go to ${approval_url} to approve the user.
+
+Company name: ${api_request.company_name}
+Company contact: ${api_request.user.username}
+Company URL: ${api_request.website}
+Address:  ${api_request.company_address}
+Reason for API usage: ${api_request.reason}

lms/templates/api_admin/email.txt

@@ -1,8 +0,0 @@
-## mako
-We have received the following request to use the Course Discovery API. Please go to ${approval_url} to approve the user.
-
-Company name: ${company_name}
-Company contact: ${username}
-Company URL: ${url}
-Address:  ${company_address}
-Reason for API usage: ${reason}

lms/templates/api_admin/terms_of_service.html

@@ -14,7 +14,7 @@ from django.utils.translation import ugettext as _
 
   <h2 class="api-access-request-subheading">${_("API Access")}</h2>
 
-  <p class="api-tos-body">${_("To access the APIs, you will need to create an {platform_name} user account for your application (not for personal use). This account will provide you with access to our API request page at {request_url}. On that page, you must complete the API request form including a description of your proposed uses for the APIs. Any account and registration information that you provide to {platform_name} must be accurate and up to date, and you agree to inform us promptly of any changes. {platform_name} will review your API request form and, upon approval in {platform_name}'s sole discretion, will provide you with instructions for obtaining your API shared secret and client ID.").format(platform_name=settings.PLATFORM_NAME, request_url=reverse('api-request'))}</p>
+  <p class="api-tos-body">${_("To access the APIs, you will need to create an {platform_name} user account for your application (not for personal use). This account will provide you with access to our API request page at {request_url}. On that page, you must complete the API request form including a description of your proposed uses for the APIs. Any account and registration information that you provide to {platform_name} must be accurate and up to date, and you agree to inform us promptly of any changes. {platform_name} will review your API request form and, upon approval in {platform_name}'s sole discretion, will provide you with instructions for obtaining your API shared secret and client ID.").format(platform_name=settings.PLATFORM_NAME, request_url=reverse('api_admin:api-request'))}</p>
 
   <h2 class="api-access-request-subheading">${_("Permissible Use")}</h2>
 

lms/urls.py

@@ -110,6 +110,9 @@ urlpatterns = (
     # TODO Namespace these!
     url(r'^course_modes/', include('course_modes.urls')),
     url(r'^verify_student/', include('verify_student.urls')),
+
+    # URLs for API access management
+    url(r'^api-admin/', include('openedx.core.djangoapps.api_admin.urls', namespace='api_admin')),
 )
 
 urlpatterns += (
@@ -1000,8 +1003,3 @@ if settings.FEATURES.get('ENABLE_FINANCIAL_ASSISTANCE_FORM'):
             name='submit_financial_assistance_request'
         )
     )
-
-# URLs for API access management
-urlpatterns += (
-    url(r'^api-admin/', include('openedx.core.djangoapps.api_admin.urls')),
-)

openedx/core/djangoapps/api_admin/admin.py

@@ -12,6 +12,8 @@ class ApiAccessRequestAdmin(admin.ModelAdmin):
     list_filter = ('status',)
     search_fields = ('user__email',)
     raw_id_fields = ('user',)
+    readonly_fields = ('user', 'website', 'reason', 'company_name', 'company_address', 'contacted', )
+    exclude = ('site',)
 
 
 admin.site.register(ApiAccessConfig, ConfigurationModelAdmin)

openedx/core/djangoapps/api_admin/migrations/0004_auto_20160412_1506.py

@@ -0,0 +1,37 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+import django.db.models.deletion
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('sites', '0001_initial'),
+        ('api_admin', '0003_auto_20160404_1618'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='apiaccessrequest',
+            name='contacted',
+            field=models.BooleanField(default=False),
+        ),
+        migrations.AddField(
+            model_name='apiaccessrequest',
+            name='site',
+            field=models.ForeignKey(default=1, to='sites.Site'),
+            preserve_default=False,
+        ),
+        migrations.AddField(
+            model_name='historicalapiaccessrequest',
+            name='contacted',
+            field=models.BooleanField(default=False),
+        ),
+        migrations.AddField(
+            model_name='historicalapiaccessrequest',
+            name='site',
+            field=models.ForeignKey(related_name='+', on_delete=django.db.models.deletion.DO_NOTHING, db_constraint=False, blank=True, to='sites.Site', null=True),
+        ),
+    ]

openedx/core/djangoapps/api_admin/models.py

@@ -1,10 +1,19 @@
 """Models for API management."""
 import logging
+from smtplib import SMTPException
+from urlparse import urlunsplit
 
+from django.conf import settings
 from django.contrib.auth.models import User
+from django.contrib.sites.models import Site
+from django.core.mail import send_mail
+from django.core.urlresolvers import reverse
 from django.db import models
+from django.db.models.signals import post_save, pre_save
+from django.dispatch import receiver
 from django.utils.translation import ugettext as _
 from django_extensions.db.models import TimeStampedModel
+from edxmako.shortcuts import render_to_string
 from simple_history.models import HistoricalRecords
 
 from config_models.models import ConfigurationModel
@@ -35,6 +44,8 @@ class ApiAccessRequest(TimeStampedModel):
     reason = models.TextField(help_text=_('The reason this user wants to access the API.'))
     company_name = models.CharField(max_length=255, default='')
     company_address = models.CharField(max_length=255, default='')
+    site = models.ForeignKey(Site)
+    contacted = models.BooleanField(default=False)
 
     history = HistoricalRecords()
 
@@ -88,3 +99,83 @@ class ApiAccessConfig(ConfigurationModel):
 
     def __unicode__(self):
         return u'ApiAccessConfig [enabled={}]'.format(self.enabled)
+
+
+@receiver(post_save, sender=ApiAccessRequest, dispatch_uid="api_access_request_post_save_email")
+def send_request_email(sender, instance, created, **kwargs):   # pylint: disable=unused-argument
+    """ Send request email after new record created. """
+    if created:
+        _send_new_pending_email(instance)
+
+
+@receiver(pre_save, sender=ApiAccessRequest, dispatch_uid="api_access_request_pre_save_email")
+def send_decision_email(sender, instance, **kwargs):  # pylint: disable=unused-argument
+    """ Send decision email after status changed. """
+    if instance.id and not instance.contacted:
+        old_instance = ApiAccessRequest.objects.get(pk=instance.id)
+        if instance.status != old_instance.status:
+            _send_decision_email(instance)
+
+
+def _send_new_pending_email(instance):
+    """ Send an email to settings.API_ACCESS_MANAGER_EMAIL with the contents of this API access request. """
+    context = {
+        'approval_url': urlunsplit(
+            (
+                'https' if settings.HTTPS == 'on' else 'http',
+                instance.site.domain,
+                reverse('admin:api_admin_apiaccessrequest_change', args=(instance.id,)),
+                '',
+                '',
+            )
+        ),
+        'api_request': instance
+    }
+
+    message = render_to_string('api_admin/api_access_request_email_new_request.txt', context)
+    try:
+        send_mail(
+            _('API access request from {company}').format(company=instance.company_name),
+            message,
+            settings.API_ACCESS_FROM_EMAIL,
+            [settings.API_ACCESS_MANAGER_EMAIL],
+            fail_silently=False
+        )
+    except SMTPException:
+        log.exception('Error sending API user notification email for request [%s].', instance.id)
+
+
+def _send_decision_email(instance):
+    """ Send an email to requesting user with the decision made about their request. """
+    context = {
+        'name': instance.user.username,
+        'api_management_url': urlunsplit(
+            (
+                'https' if settings.HTTPS == 'on' else 'http',
+                instance.site.domain,
+                reverse('api_admin:api-status'),
+                '',
+                '',
+            )
+        ),
+        'authentication_docs_url': settings.AUTH_DOCUMENTATION_URL,
+        'api_docs_url': settings.API_DOCUMENTATION_URL,
+        'support_email_address': settings.API_ACCESS_FROM_EMAIL,
+        'platform_name': settings.PLATFORM_NAME
+    }
+
+    message = render_to_string(
+        'api_admin/api_access_request_email_{status}.txt'.format(status=instance.status),
+        context
+    )
+    try:
+        send_mail(
+            _('API access request'),
+            message,
+            settings.API_ACCESS_FROM_EMAIL,
+            [instance.user.email],
+            fail_silently=False
+        )
+        instance.contacted = True
+    except SMTPException:
+        log.exception('Error sending API user notification email for request [%s].', instance.id)

openedx/core/djangoapps/api_admin/tests/factories.py

@@ -2,6 +2,7 @@
 import factory
 from factory.django import DjangoModelFactory
 
+from microsite_configuration.tests.factories import SiteFactory
 from openedx.core.djangoapps.api_admin.models import ApiAccessRequest
 from student.tests.factories import UserFactory
 
@@ -12,3 +13,4 @@ class ApiAccessRequestFactory(DjangoModelFactory):
         model = ApiAccessRequest
 
     user = factory.SubFactory(UserFactory)
+    site = factory.SubFactory(SiteFactory)

openedx/core/djangoapps/api_admin/tests/test_models.py

@@ -1,14 +1,22 @@
 # pylint: disable=missing-docstring
+from smtplib import SMTPException
+
 import ddt
+from django.conf import settings
 from django.db import IntegrityError
 from django.test import TestCase
+import mock
+import unittest
 
+from microsite_configuration.tests.factories import SiteFactory
 from openedx.core.djangoapps.api_admin.models import ApiAccessRequest, ApiAccessConfig
+from openedx.core.djangoapps.api_admin.models import log as model_log
 from openedx.core.djangoapps.api_admin.tests.factories import ApiAccessRequestFactory
 from student.tests.factories import UserFactory
 
 
 @ddt.ddt
+@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
 class ApiAccessRequestTests(TestCase):
 
     def setUp(self):
@@ -69,3 +77,71 @@ class ApiAccessConfigTests(TestCase):
             unicode(ApiAccessConfig(enabled=False)),
             u'ApiAccessConfig [enabled=False]'
         )
+
+
+@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
+class ApiAccessRequestSignalTests(TestCase):
+    def setUp(self):
+        super(ApiAccessRequestSignalTests, self).setUp()
+        self.user = UserFactory()
+        self.api_access_request = ApiAccessRequest(user=self.user, site=SiteFactory())
+        self.send_new_pending_email_function = 'openedx.core.djangoapps.api_admin.models._send_new_pending_email'
+        self.send_decision_email_function = 'openedx.core.djangoapps.api_admin.models._send_decision_email'
+
+    def test_save_signal_success_new_email(self):
+        """ Verify that initial save sends new email and no decision email. """
+        with mock.patch(self.send_new_pending_email_function) as mock_new_email:
+            with mock.patch(self.send_decision_email_function) as mock_decision_email:
+                self.api_access_request.save()
+
+        mock_new_email.assert_called_once_with(self.api_access_request)
+        self.assertFalse(mock_decision_email.called)
+
+    def test_save_signal_success_decision_email(self):
+        """ Verify that updating request status sends decision email and no new email. """
+        self.api_access_request.save()
+
+        with mock.patch(self.send_new_pending_email_function) as mock_new_email:
+            with mock.patch(self.send_decision_email_function) as mock_decision_email:
+                self.api_access_request.approve()
+
+        mock_decision_email.assert_called_once_with(self.api_access_request)
+        self.assertFalse(mock_new_email.called)
+
+    def test_save_signal_success_no_emails(self):
+        """ Verify that updating request status again sends no emails. """
+        self.api_access_request.save()
+        self.api_access_request.approve()
+
+        with mock.patch(self.send_new_pending_email_function) as mock_new_email:
+            with mock.patch(self.send_decision_email_function) as mock_decision_email:
+                self.api_access_request.deny()
+
+        self.assertFalse(mock_decision_email.called)
+        self.assertFalse(mock_new_email.called)
+
+    def test_save_signal_failure_email(self):
+        """ Verify that saving still functions even on email errors. """
+        self.assertIsNone(self.api_access_request.id)
+
+        mail_function = 'openedx.core.djangoapps.api_admin.models.send_mail'
+        with mock.patch(mail_function, side_effect=SMTPException):
+            with mock.patch.object(model_log, 'exception') as mock_model_log_exception:
+                self.api_access_request.save()
+
+        # Verify that initial save logs email errors properly
+        mock_model_log_exception.assert_called_once_with(
+            'Error sending API user notification email for request [%s].', self.api_access_request.id
+        )
+        # Verify object saved
+        self.assertIsNotNone(self.api_access_request.id)
+
+        with mock.patch(mail_function, side_effect=SMTPException):
+            with mock.patch.object(model_log, 'exception') as mock_model_log_exception:
+                self.api_access_request.approve()
+        # Verify that updating request status logs email errors properly
+        mock_model_log_exception.assert_called_once_with(
+            'Error sending API user notification email for request [%s].', self.api_access_request.id
+        )
+        # Verify object saved
+        self.assertEqual(self.api_access_request.status, ApiAccessRequest.APPROVED)

openedx/core/djangoapps/api_admin/tests/test_views.py

@@ -1,16 +1,13 @@
 #pylint: disable=missing-docstring
-from smtplib import SMTPException
 import unittest
 
 from django.conf import settings
 from django.core.urlresolvers import reverse
 from django.test import TestCase
-import mock
 
 from openedx.core.djangoapps.api_admin.models import ApiAccessRequest, ApiAccessConfig
 from openedx.core.djangoapps.api_admin.tests.factories import ApiAccessRequestFactory
 from openedx.core.djangoapps.api_admin.tests.utils import VALID_DATA
-from openedx.core.djangoapps.api_admin.views import log as view_log
 from student.tests.factories import UserFactory
 
 
@@ -26,7 +23,7 @@ class ApiRequestViewTest(ApiAdminTest):
 
     def setUp(self):
         super(ApiRequestViewTest, self).setUp()
-        self.url = reverse('api-request')
+        self.url = reverse('api_admin:api-request')
         password = 'abc123'
         self.user = UserFactory(password=password)
         self.client.login(username=self.user.username, password=password)
@@ -49,14 +46,14 @@ class ApiRequestViewTest(ApiAdminTest):
         """
         ApiAccessRequestFactory(user=self.user)
         response = self.client.get(self.url)
-        self.assertRedirects(response, reverse('api-status'))
+        self.assertRedirects(response, reverse('api_admin:api-status'))
 
     def _assert_post_success(self, response):
         """
         Assert that a successful POST has been made, that the response
         redirects correctly, and that the correct object has been created.
         """
-        self.assertRedirects(response, reverse('api-status'))
+        self.assertRedirects(response, reverse('api_admin:api-status'))
         api_request = ApiAccessRequest.objects.get(user=self.user)
         self.assertEqual(api_request.status, ApiAccessRequest.PENDING)
         return api_request
@@ -64,38 +61,14 @@ class ApiRequestViewTest(ApiAdminTest):
     def test_post_valid(self):
         """Verify that a logged-in user can create an API request."""
         self.assertFalse(ApiAccessRequest.objects.all().exists())
-        with mock.patch('openedx.core.djangoapps.api_admin.views.send_mail') as mock_send_mail:
-            response = self.client.post(self.url, VALID_DATA)
-        mock_send_mail.assert_called_once_with(
-            'API access request from ' + VALID_DATA['company_name'],
-            mock.ANY,
-            settings.API_ACCESS_FROM_EMAIL,
-            [settings.API_ACCESS_MANAGER_EMAIL],
-            fail_silently=False
-        )
+        response = self.client.post(self.url, VALID_DATA)
         self._assert_post_success(response)
 
-    def test_failed_email(self):
-        """
-        Verify that an access request is still created if sending email
-        fails for some reason, and that the necessary information is
-        logged.
-        """
-        mail_function = 'openedx.core.djangoapps.api_admin.views.send_mail'
-        with mock.patch(mail_function, side_effect=SMTPException):
-            with mock.patch.object(view_log, 'exception') as mock_view_log_exception:
-                response = self.client.post(self.url, VALID_DATA)
-        api_request = self._assert_post_success(response)
-        mock_view_log_exception.assert_called_once_with(
-            'Error sending API request email for request [%s].', api_request.id  # pylint: disable=no-member
-        )
-
     def test_post_anonymous(self):
         """Verify that users must be logged in to create an access request."""
         self.client.logout()
-        with mock.patch('openedx.core.djangoapps.api_admin.views.send_mail') as mock_send_mail:
-            response = self.client.post(self.url, VALID_DATA)
-        mock_send_mail.assert_not_called()
+        response = self.client.post(self.url, VALID_DATA)
+
         self.assertEqual(response.status_code, 302)
         self.assertFalse(ApiAccessRequest.objects.all().exists())
 
@@ -120,7 +93,7 @@ class ApiRequestStatusViewTest(ApiAdminTest):
         password = 'abc123'
         self.user = UserFactory(password=password)
         self.client.login(username=self.user.username, password=password)
-        self.url = reverse('api-status')
+        self.url = reverse('api_admin:api-status')
 
     def test_get_without_request(self):
         """
@@ -128,7 +101,7 @@ class ApiRequestStatusViewTest(ApiAdminTest):
         redirected to the API request form.
         """
         response = self.client.get(self.url)
-        self.assertRedirects(response, reverse('api-request'))
+        self.assertRedirects(response, reverse('api_admin:api-request'))
 
     def test_get_with_request(self):
         """
@@ -157,7 +130,7 @@ class ApiTosViewTest(ApiAdminTest):
 
     def test_get_api_tos(self):
         """Verify that the terms of service can be read."""
-        url = reverse('api-tos')
+        url = reverse('api_admin:api-tos')
         response = self.client.get(url)
         self.assertEqual(response.status_code, 200)
         self.assertIn('Terms of Service', response.content)

openedx/core/djangoapps/api_admin/views.py

@@ -1,16 +1,15 @@
 """Views for API management."""
 import logging
-from smtplib import SMTPException
 
 from django.conf import settings
-from django.core.mail import send_mail
+from django.contrib.sites.shortcuts import get_current_site
 from django.core.urlresolvers import reverse_lazy, reverse
 from django.shortcuts import redirect
 from django.utils.translation import ugettext as _
 from django.views.generic import View
 from django.views.generic.base import TemplateView
 from django.views.generic.edit import CreateView
-from edxmako.shortcuts import render_to_response, render_to_string
+from edxmako.shortcuts import render_to_response
 
 from openedx.core.djangoapps.api_admin.forms import ApiAccessRequestForm
 from openedx.core.djangoapps.api_admin.models import ApiAccessRequest
@@ -22,7 +21,7 @@ class ApiRequestView(CreateView):
     """Form view for requesting API access."""
     form_class = ApiAccessRequestForm
     template_name = 'api_admin/api_access_request_form.html'
-    success_url = reverse_lazy('api-status')
+    success_url = reverse_lazy('api_admin:api-status')
 
     def get(self, request):
         """
@@ -30,41 +29,13 @@ class ApiRequestView(CreateView):
         them to the client creation page.
         """
         if ApiAccessRequest.api_access_status(request.user) is not None:
-            return redirect(reverse('api-status'))
+            return redirect(reverse('api_admin:api-status'))
         return super(ApiRequestView, self).get(request)
 
-    def send_email(self, api_request):
-        """
-        Send an email to settings.API_ACCESS_MANAGER_EMAIL with the
-        contents of this API access request.
-        """
-        context = {
-            'approval_url': self.request.build_absolute_uri(
-                reverse('admin:api_admin_apiaccessrequest_change', args=(api_request.id,))
-            ),
-            'company_name': api_request.company_name,
-            'username': api_request.user.username,
-            'url': api_request.website,
-            'company_address': api_request.company_address,
-            'reason': api_request.reason,
-        }
-        message = render_to_string('api_admin/email.txt', context)
-        try:
-            send_mail(
-                _('API access request from {company}').format(company=api_request.company_name),
-                message,
-                settings.API_ACCESS_FROM_EMAIL,
-                [settings.API_ACCESS_MANAGER_EMAIL],
-                fail_silently=False
-            )
-        except SMTPException:
-            log.exception('Error sending API request email for request [%s].', api_request.id)
-
     def form_valid(self, form):
         form.instance.user = self.request.user
-        result = super(ApiRequestView, self).form_valid(form)
-        self.send_email(form.instance)
-        return result
+        form.instance.site = get_current_site(self.request)
+        return super(ApiRequestView, self).form_valid(form)
 
 
 class ApiRequestStatusView(View):
@@ -77,7 +48,7 @@ class ApiRequestStatusView(View):
         """
         status = ApiAccessRequest.api_access_status(request.user)
         if status is None:
-            return redirect(reverse('api-request'))
+            return redirect(reverse('api_admin:api-request'))
         return render_to_response('api_admin/status.html', {
             'status': status,
             'api_support_link': _('TODO'),

openedx/core/djangoapps/api_admin/widgets.py

@@ -24,7 +24,7 @@ class TermsOfServiceCheckboxInput(CheckboxInput):
         # platform_name is the name of this Open edX installation.
         label = _('I, and my company, accept the {link_start}{platform_name} API Terms of Service{link_end}.').format(
             platform_name=settings.PLATFORM_NAME,
-            link_start='<a href="{url}" target="_blank">'.format(url=reverse('api-tos')),
+            link_start='<a href="{url}" target="_blank">'.format(url=reverse('api_admin:api-tos')),
             link_end='</a>',
         )