Merge pull request #12085 from edx/peter-fogg/improve-api-request-ui

API request page improvements.

lms/static/sass/views/_api-access.scss

@@ -7,6 +7,17 @@
     @include text-align(left);
   }
 
+  .api-access-request-subheading {
+    @extend %t-title5;
+    margin: $baseline;
+    @include text-align(left);
+  }
+
+  .api-tos-body {
+    @extend %t-copy-sub1;
+    margin: $baseline;
+  }
+
   .request-status {
     margin: 0 $baseline;
     padding: $baseline;
@@ -25,39 +36,47 @@
 
     padding: 0 $baseline $baseline $baseline;
 
-    #api-form-fields {
+    p {
+      margin: 1.5*$baseline 0;
 
-      li {
-        margin: $baseline 0;
-
-        .helptext {
-          @extend %t-copy-sub1;
-          display: block;
-        }
+      .helptext {
+        @extend %t-copy-sub1;
+        display: block;
       }
+    }
 
-      label {
-        @extend %t-copy-base;
-        display: block;
-        font-style: normal;
+    label {
+      @extend %t-copy-base;
+      display: block;
+      font-style: normal;
+
+      &.tos-checkbox-label {
+        display: inline-block;
       }
+    }
 
-      input, textarea {
-        @extend %t-copy-base;
-        font-family: 'Open Sans';
-        font-style: normal;
+    input, textarea {
+      @extend %t-copy-base;
+      font-family: 'Open Sans';
+      font-style: normal;
+      width: 300px;
+
+      &[type=checkbox] {
+        display: inline-block;
+        width: initial;
+        @include margin-right(0.5*$baseline);
       }
+    }
 
-      .errorlist {
+    .errorlist {
 
-        padding: 0;
-        list-style-type: none;
+      padding: 0;
+      list-style-type: none;
 
-        li {
-          @extend %t-copy-base;
-          margin: 0;
-          color: $red;
-        }
+      li {
+        @extend %t-copy-base;
+        margin: 0;
+        color: $red;
       }
     }
 

lms/templates/api_admin/api_access_request_form.html

 ## mako
 <%page expression_filter="h"/>
 <%inherit file="../main.html"/>
-<%
-from django.utils.translation import ugettext as _
+<%!
+  from django.utils.translation import ugettext as _
 %>
 
-<div id="api-access-wrapper">
-  <h1 id="api-access-request-header">${_("{platform_name} API Access Request").format(platform_name=settings.PLATFORM_NAME)}</h1>
+<%block name="pagetitle">${_("API Access Request")}</%block>
+
+<div id="api-access-wrapper" class="container">
+  <h1 id="api-access-request-header">
+    ${_("{platform_name} API Access Request").format(platform_name=settings.PLATFORM_NAME)}
+  </h1>
 
   <form action="" method="post" id="api-access-request">
     <input type="hidden" id="csrf_token" name="csrfmiddlewaretoken" value="${csrf_token}">
-    <ul id="api-form-fields">
-      ${form.as_ul() | n}
-    </ul>
-    <input id="api-access-submit" type="submit" value="${_('Request API Access')}" />
+    ${form.as_p() | n}
+    <input id="api-access-submit" type="submit" value="${_('Request API Access')}"/>
   </form>
 </div>

openedx/core/djangoapps/api_admin/forms.py

 """Forms for API management."""
 from django import forms
-from django.conf import settings
 from django.utils.translation import ugettext as _
 
 from openedx.core.djangoapps.api_admin.models import ApiAccessRequest
+from openedx.core.djangoapps.api_admin.widgets import TermsOfServiceCheckboxInput
 
 
 class ApiAccessRequestForm(forms.ModelForm):
     """Form to request API access."""
-
-    terms_of_service = forms.BooleanField(
-        label=_('{platform_name} API Terms of Service').format(platform_name=settings.PLATFORM_NAME),
-        help_text=_(
-            'The resulting Package will still be considered part of Covered Code. Your Grants.'
-            ' In consideration of, and distributed, a Modification is: (a) any addition to or loss'
-            ' of data, programs or other fee is charged for the physical act of transferring a copy,'
-            ' and you may do so by its licensors. The Licensor grants to You for damages, including '
-            'any direct, indirect, special, incidental and consequential damages, such as lost profits;'
-            ' iii) states that any such claim is resolved (such as deliberate and grossly negligent acts)'
-            ' or agreed to in writing, the Copyright Holder nor by the laws of the Original Code and'
-            ' any other entity based on the same media as an expression of character texts or the whole'
-            ' of the Licensed Product, and (iv) you make to the general goal of allowing unrestricted '
-            're-use and re-distribute applies to "Community Portal Server" and related software products'
-            ' as well as in related documentation and collateral materials stating that you have modified'
-            ' that component; or it may be copied, modified, distributed, and/or redistributed.'
-        ),
-    )
+    terms_of_service = forms.BooleanField(widget=TermsOfServiceCheckboxInput(), label='')
 
     class Meta(object):
         model = ApiAccessRequest
         fields = ('company_name', 'website', 'company_address', 'reason', 'terms_of_service')
         labels = {
+            'company_name': _('Company Name'),
+            'company_address': _('Company Address'),
             'reason': _('Describe what your application does.'),
         }
         help_texts = {
@@ -42,3 +27,8 @@ class ApiAccessRequestForm(forms.ModelForm):
         widgets = {
             'company_address': forms.Textarea()
         }
+
+    def __init__(self, *args, **kwargs):
+        # Get rid of the colons at the end of the field labels.
+        kwargs.setdefault('label_suffix', '')
+        super(ApiAccessRequestForm, self).__init__(*args, **kwargs)

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

 #pylint: disable=missing-docstring
+import unittest
+
 import ddt
+from django.conf import settings
 from django.test import TestCase
 
 from openedx.core.djangoapps.api_admin.forms import ApiAccessRequestForm
 from openedx.core.djangoapps.api_admin.tests.utils import VALID_DATA
 
 
+@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
 @ddt.ddt
 class ApiAccessFormTest(TestCase):
 

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

@@ -14,8 +14,15 @@ from openedx.core.djangoapps.api_admin.views import log as view_log
 from student.tests.factories import UserFactory
 
 
+class ApiAdminTest(TestCase):
+
+    def setUp(self):
+        super(ApiAdminTest, self).setUp()
+        ApiAccessConfig(enabled=True).save()
+
+
 @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
-class ApiRequestViewTest(TestCase):
+class ApiRequestViewTest(ApiAdminTest):
 
     def setUp(self):
         super(ApiRequestViewTest, self).setUp()
@@ -23,7 +30,6 @@ class ApiRequestViewTest(TestCase):
         password = 'abc123'
         self.user = UserFactory(password=password)
         self.client.login(username=self.user.username, password=password)
-        ApiAccessConfig(enabled=True).save()
 
     def test_get(self):
         """Verify that a logged-in can see the API request form."""
@@ -107,11 +113,10 @@ class ApiRequestViewTest(TestCase):
 
 
 @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
-class ApiRequestStatusViewTest(TestCase):
+class ApiRequestStatusViewTest(ApiAdminTest):
 
     def setUp(self):
         super(ApiRequestStatusViewTest, self).setUp()
-        ApiAccessConfig(enabled=True).save()
         password = 'abc123'
         self.user = UserFactory(password=password)
         self.client.login(username=self.user.username, password=password)
@@ -145,3 +150,14 @@ class ApiRequestStatusViewTest(TestCase):
         ApiAccessConfig(enabled=False).save()
         response = self.client.get(self.url)
         self.assertEqual(response.status_code, 404)
+
+
+@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
+class ApiTosViewTest(ApiAdminTest):
+
+    def test_get_api_tos(self):
+        """Verify that the terms of service can be read."""
+        url = reverse('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/urls.py

@@ -4,16 +4,21 @@ from django.conf.urls import url
 from django.contrib.auth.decorators import login_required
 
 from openedx.core.djangoapps.api_admin.decorators import api_access_enabled_or_404
-from openedx.core.djangoapps.api_admin.views import ApiRequestView, ApiRequestStatusView
+from openedx.core.djangoapps.api_admin.views import ApiRequestView, ApiRequestStatusView, ApiTosView
 
 urlpatterns = (
     url(
-        r'^status$',
+        r'^status/$',
         api_access_enabled_or_404(login_required(ApiRequestStatusView.as_view())),
         name="api-status"
     ),
     url(
-        r'$',
+        r'^terms-of-service/$',
+        api_access_enabled_or_404(ApiTosView.as_view()),
+        name="api-tos"
+    ),
+    url(
+        r'^$',
         api_access_enabled_or_404(login_required(ApiRequestView.as_view())),
         name="api-request"
     ),

openedx/core/djangoapps/api_admin/views.py

@@ -8,9 +8,10 @@ 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 openedx.core.djangoapps.api_admin.forms import ApiAccessRequestForm
 from openedx.core.djangoapps.api_admin.models import ApiAccessRequest
 
@@ -82,3 +83,9 @@ class ApiRequestStatusView(View):
             'api_support_link': _('TODO'),
             'api_support_email': settings.API_ACCESS_MANAGER_EMAIL,
         })
+
+
+class ApiTosView(TemplateView):
+    """View to show the API Terms of Service."""
+
+    template_name = 'api_admin/terms_of_service.html'

openedx/core/djangoapps/api_admin/widgets.py

+""" Form widget classes """
+
+from django.conf import settings
+from django.core.urlresolvers import reverse
+from django.forms.utils import flatatt
+from django.forms.widgets import CheckboxInput
+from django.utils.encoding import force_text
+from django.utils.html import format_html
+from django.utils.translation import ugettext as _
+
+
+class TermsOfServiceCheckboxInput(CheckboxInput):
+    """ Renders a checkbox with a label linking to the terms of service. """
+
+    def render(self, name, value, attrs=None):
+        final_attrs = self.build_attrs(attrs, type='checkbox', name=name)
+        if self.check_test(value):
+            final_attrs['checked'] = 'checked'
+        if not (value is True or value is False or value is None or value == ''):
+            # Only add the 'value' attribute if a value is non-empty.
+            final_attrs['value'] = force_text(value)
+
+        # Translators: link_start and link_end are HTML tags for a link to the terms of service.
+        # 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_end='</a>',
+        )
+
+        html = '<input{{}} /> <label class="tos-checkbox-label" for="{id}">{label}</label>'.format(
+            id=final_attrs['id'],
+            label=label
+        )
+        return format_html(html, flatatt(final_attrs))