Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions lms/envs/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -393,9 +393,6 @@
# Enable OpenBadge support. See the BADGR_* settings later in this file.
'ENABLE_OPENBADGES': False,

# Credit course API
'ENABLE_CREDIT_API': True,

# The block types to disable need to be specified in "x block disable config" in django admin.
'ENABLE_DISABLING_XBLOCK_TYPES': True,

Expand Down
2 changes: 0 additions & 2 deletions lms/envs/test.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,6 @@

FEATURES['ENABLE_VERIFIED_CERTIFICATES'] = True

FEATURES['ENABLE_CREDIT_API'] = True

# Enable this feature for course staff grade downloads, to enable acceptance tests
FEATURES['ENABLE_S3_GRADE_DOWNLOADS'] = True
FEATURES['ALLOW_COURSE_STAFF_GRADE_DOWNLOADS'] = True
Expand Down
2 changes: 2 additions & 0 deletions lms/static/js/commerce/credit.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ var edx = edx || {};
headers: {
'X-CSRFToken': $.cookie('csrftoken')
},
dataType: 'json',
contentType: 'application/json',
data: JSON.stringify({
'course_key': courseKey,
'username': username
Expand Down
6 changes: 5 additions & 1 deletion lms/static/js/commerce/views/receipt_view.js
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,11 @@ var edx = edx || {};
return $.ajax({
url: _.sprintf(providerUrl, providerId),
type: 'GET',
dataType: 'json'
dataType: 'json',
contentType: 'application/json',
headers: {
'X-CSRFToken': $.cookie('csrftoken')
}
}).retry({times: 5, timeout: 2000, statusCodes: [404]});
},
/**
Expand Down
6 changes: 3 additions & 3 deletions lms/templates/commerce/provider.underscore
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@
</div>
<div class="provider-more-info">
<%= interpolate(
gettext("To finalize course credit, %(provider_id)s requires %(platform_name)s learners to submit a credit request."),
{ provider_id: provider_id.toUpperCase(), platform_name: platformName }, true
gettext("To finalize course credit, %(display_name)s requires %(platform_name)s learners to submit a credit request."),
{ display_name: display_name, platform_name: platformName }, true
) %>
</div>
<div class="provider-instructions">
Expand All @@ -21,7 +21,7 @@
<%= interpolate("<img src='%s' alt='%s'></image>", [thumbnail_url, display_name]) %>
</div>
<div class="complete-order">
<%= interpolate('<button data-provider="%s" data-course-key="%s" data-username="%s" class="complete-course" onClick=completeOrder(this)>%s</button>', [provider_id, course_key, username,
<%= interpolate('<button data-provider="%s" data-course-key="%s" data-username="%s" class="complete-course" onClick=completeOrder(this)>%s</button>', [id, course_key, username,
gettext( "Get Credit")]) %>
</div>
</div>
7 changes: 1 addition & 6 deletions lms/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@
url(r'^api/val/v0/', include('edxval.urls')),

url(r'^api/commerce/', include('commerce.api.urls', namespace='commerce_api')),
url(r'^api/credit/', include('openedx.core.djangoapps.credit.urls', app_name="credit", namespace='credit')),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my own edification, why are we removing the feature flag? Was it used before?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was used, but it isn't really needed. If we don't want to use the Credit API...don't use it. Additionally, the feature is fully rolled-out. The odds of us disabling this API are quite low.

)

if settings.FEATURES["ENABLE_COMBINED_LOGIN_REGISTRATION"]:
Expand All @@ -115,12 +116,6 @@
url(r'^register$', 'student.views.register_user', name="register_user"),
)

if settings.FEATURES.get("ENABLE_CREDIT_API"):
# Credit API end-points
urlpatterns += (
url(r'^api/credit/', include('openedx.core.djangoapps.credit.urls', app_name="credit", namespace='credit')),
)

if settings.FEATURES["ENABLE_MOBILE_REST_API"]:
urlpatterns += (
url(r'^api/mobile/v0.5/', include('mobile_api.urls')),
Expand Down
8 changes: 4 additions & 4 deletions openedx/core/djangoapps/credit/api/eligibility.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

import logging

from opaque_keys.edx.keys import CourseKey

from openedx.core.djangoapps.credit.exceptions import InvalidCreditRequirements, InvalidCreditCourse
from openedx.core.djangoapps.credit.email_utils import send_credit_notifications
from openedx.core.djangoapps.credit.models import (
Expand All @@ -14,8 +16,7 @@
CreditEligibility,
)

from opaque_keys.edx.keys import CourseKey

# TODO: Cleanup this mess! ECOM-2908

log = logging.getLogger(__name__)

Expand Down Expand Up @@ -246,8 +247,7 @@ def set_credit_requirement_status(username, course_key, req_namespace, req_name,
# Find the requirement we're trying to set
req_to_update = next((
req for req in reqs
if req.namespace == req_namespace
and req.name == req_name
if req.namespace == req_namespace and req.name == req_name
), None)

# If we can't find the requirement, then the most likely explanation
Expand Down
16 changes: 8 additions & 8 deletions openedx/core/djangoapps/credit/api/provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@

import datetime
import logging
import pytz
import uuid

import pytz
from django.db import transaction
from lms.djangoapps.django_comment_client.utils import JsonResponse

from lms.djangoapps.django_comment_client.utils import JsonResponse
from openedx.core.djangoapps.credit.exceptions import (
UserIsNotEligible,
CreditProviderNotConfigured,
Expand All @@ -28,6 +28,8 @@
from util.date_utils import to_timestamp


# TODO: Cleanup this mess! ECOM-2908

log = logging.getLogger(__name__)


Expand Down Expand Up @@ -257,12 +259,10 @@ def create_credit_request(course_key, provider_id, username):
final_grade = unicode(final_grade)

except (CreditRequirementStatus.DoesNotExist, TypeError, KeyError):
log.exception(
"Could not retrieve final grade from the credit eligibility table "
"for user %s in course %s.",
user.id, course_key
)
raise UserIsNotEligible
msg = 'Could not retrieve final grade from the credit eligibility table for ' \
'user [{user_id}] in course [{course_key}].'.format(user_id=user.id, course_key=course_key)
log.exception(msg)
raise UserIsNotEligible(msg)

parameters = {
"request_uuid": credit_request.uuid,
Expand Down
28 changes: 28 additions & 0 deletions openedx/core/djangoapps/credit/exceptions.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
"""Exceptions raised by the credit API. """
from __future__ import unicode_literals
from django.utils.translation import ugettext_lazy as _
from rest_framework import status
from rest_framework.exceptions import APIException

# TODO: Cleanup this mess! ECOM-2908


class CreditApiBadRequest(Exception):
Expand Down Expand Up @@ -56,3 +62,25 @@ class InvalidCreditStatus(CreditApiBadRequest):
The status is not either "approved" or "rejected".
"""
pass


class InvalidCreditRequest(APIException):
""" API request is invalid. """
status_code = status.HTTP_400_BAD_REQUEST


class UserNotEligibleException(InvalidCreditRequest):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this is similar to UserIsNotEligible. Can it be replaced altogether?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify: can UserIsNotEligible be replaced by UserNotEligibleException?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe. It will involve touching other code, which I initially avoided to limit the scope of these changes. Let's see...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to leave this as-is for now. The API rewrite is risky enough without changing other components.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Seems like the right to do to reduce risk. But do we have / can we create a ticket to track this clean up task once the rewrite is solidly in place?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ECOM-2908 created. TODOs added.

""" User not eligible for credit for a given course. """

def __init__(self, course_key, username):
detail = _('[{username}] is not eligible for credit for [{course_key}].').format(username=username,
course_key=course_key)
super(UserNotEligibleException, self).__init__(detail)


class InvalidCourseKey(InvalidCreditRequest):
""" Course key is invalid. """

def __init__(self, course_key):
detail = _('[{course_key}] is not a valid course key.').format(course_key=course_key)
super(InvalidCourseKey, self).__init__(detail)
8 changes: 3 additions & 5 deletions openedx/core/djangoapps/credit/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
from django.utils.translation import ugettext_lazy


CREDIT_PROVIDER_ID_REGEX = r"[a-z,A-Z,0-9,\-]+"
log = logging.getLogger(__name__)


Expand All @@ -42,7 +43,7 @@ class CreditProvider(TimeStampedModel):
unique=True,
validators=[
RegexValidator(
regex=r"^[a-z,A-Z,0-9,\-]+$",
regex=CREDIT_PROVIDER_ID_REGEX,
message="Only alphanumeric characters and hyphens (-) are allowed",
code="invalid_provider_id",
)
Expand Down Expand Up @@ -498,10 +499,7 @@ def default_deadline_for_credit_eligibility(): # pylint: disable=invalid-name


class CreditEligibility(TimeStampedModel):
"""
A record of a user's eligibility for credit from a specific credit
provider for a specific course.
"""
""" A record of a user's eligibility for credit for a specific course. """
username = models.CharField(max_length=255, db_index=True)
course = models.ForeignKey(CreditCourse, related_name="eligibilities")

Expand Down
99 changes: 93 additions & 6 deletions openedx/core/djangoapps/credit/serializers.py
Original file line number Diff line number Diff line change
@@ -1,16 +1,25 @@
""" Credit API Serializers """

from rest_framework import serializers
from __future__ import unicode_literals
import datetime
import logging

from opaque_keys.edx.keys import CourseKey
from django.conf import settings
from opaque_keys import InvalidKeyError
from openedx.core.djangoapps.credit.models import CreditCourse
from opaque_keys.edx.keys import CourseKey
import pytz
from rest_framework import serializers
from rest_framework.exceptions import PermissionDenied

from openedx.core.djangoapps.credit.models import CreditCourse, CreditProvider, CreditEligibility, CreditRequest
from openedx.core.djangoapps.credit.signature import get_shared_secret_key, signature
from util.date_utils import from_timestamp

log = logging.getLogger(__name__)


class CourseKeyField(serializers.Field):
"""
Serializer field for a model CourseKey field.
"""
""" Serializer field for a model CourseKey field. """

def to_representation(self, data):
"""Convert a course key to unicode. """
Expand All @@ -32,3 +41,81 @@ class CreditCourseSerializer(serializers.ModelSerializer):
class Meta(object):
model = CreditCourse
exclude = ('id',)


class CreditProviderSerializer(serializers.ModelSerializer):
""" CreditProvider """
id = serializers.CharField(source='provider_id') # pylint:disable=invalid-name
description = serializers.CharField(source='provider_description')
status_url = serializers.URLField(source='provider_status_url')
url = serializers.URLField(source='provider_url')

class Meta(object):
model = CreditProvider
fields = ('id', 'display_name', 'url', 'status_url', 'description', 'enable_integration',
'fulfillment_instructions', 'thumbnail_url',)


class CreditEligibilitySerializer(serializers.ModelSerializer):
""" CreditEligibility serializer. """
course_key = serializers.SerializerMethodField()

def get_course_key(self, obj):
""" Returns the course key associated with the course. """
return unicode(obj.course.course_key)

class Meta(object):
model = CreditEligibility
fields = ('username', 'course_key', 'deadline',)


class CreditProviderCallbackSerializer(serializers.Serializer): # pylint:disable=abstract-method
"""
Serializer for input to the CreditProviderCallback view.

This is used solely for validating the input.
"""
request_uuid = serializers.CharField(required=True)
status = serializers.ChoiceField(required=True, choices=CreditRequest.REQUEST_STATUS_CHOICES)
timestamp = serializers.IntegerField(required=True)
signature = serializers.CharField(required=True)

def __init__(self, **kwargs):
self.provider = kwargs.pop('provider', None)
super(CreditProviderCallbackSerializer, self).__init__(**kwargs)

def validate_timestamp(self, value):
""" Ensure the request has been received in a timely manner. """
date_time = from_timestamp(value)

# Ensure we converted the timestamp to a datetime
if not date_time:
msg = '[{}] is not a valid timestamp'.format(value)
log.warning(msg)
raise serializers.ValidationError(msg)

elapsed = (datetime.datetime.now(pytz.UTC) - date_time).total_seconds()
if elapsed > settings.CREDIT_PROVIDER_TIMESTAMP_EXPIRATION:
msg = '[{value}] is too far in the past (over [{elapsed}] seconds).'.format(value=value, elapsed=elapsed)
log.warning(msg)
raise serializers.ValidationError(msg)

return value

def validate_signature(self, value):
""" Validate the signature and ensure the provider is setup properly. """
provider_id = self.provider.provider_id
secret_key = get_shared_secret_key(provider_id)
if secret_key is None:
msg = 'Could not retrieve secret key for credit provider [{}]. ' \
'Unable to validate requests from provider.'.format(provider_id)
log.error(msg)
raise PermissionDenied(msg)

data = self.initial_data
actual_signature = data["signature"]
if signature(data, secret_key) != actual_signature:
msg = 'Request from credit provider [{}] had an invalid signature.'.format(provider_id)
raise PermissionDenied(msg)

return value
2 changes: 1 addition & 1 deletion openedx/core/djangoapps/credit/signature.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,5 +59,5 @@ def signature(params, shared_secret):
for key in sorted(params.keys())
if key != u"signature"
])
hasher = hmac.new(shared_secret, encoded_params.encode('utf-8'), hashlib.sha256)
hasher = hmac.new(shared_secret.encode('utf-8'), encoded_params.encode('utf-8'), hashlib.sha256)
return hasher.hexdigest()
6 changes: 1 addition & 5 deletions openedx/core/djangoapps/credit/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,9 @@
This file contains celery tasks for credit course views.
"""

import datetime
from pytz import UTC

from django.conf import settings

from celery import task
from celery.utils.log import get_task_logger
from django.conf import settings
from opaque_keys import InvalidKeyError
from opaque_keys.edx.keys import CourseKey, UsageKey

Expand Down
Loading