Skip to content

(BB-1389) Update enrollment serializer and Add problem submission history endpoint #20948

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
xirdneh wants to merge 6 commits into from
Closed

(BB-1389) Update enrollment serializer and Add problem submission history endpoint #20948

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ba4ae79
feat: update enrollment serializer and add problem submission history…
xirdneh Jul 3, 2019
3f47481
Fixing imports
viadanna May 25, 2021
6139b66
Fix unrelated test error
viadanna May 25, 2021
a7dcbcb
More import fixes
viadanna May 25, 2021
5bc4415
Missing import
viadanna May 25, 2021
cd11d7d
Quality fixes
viadanna May 25, 2021
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
20 changes: 9 additions & 11 deletions lms/djangoapps/courseware/access_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,23 +9,21 @@

from django.conf import settings
from pytz import UTC
from xmodule.course_module import COURSE_VISIBILITY_PUBLIC
from xmodule.util.xmodule_django import get_current_request_hostname

from common.djangoapps.student.models import CourseEnrollment
from common.djangoapps.student.roles import CourseBetaTesterRole
from lms.djangoapps.courseware.access_response import (
AccessResponse,
StartDateError,
EnrollmentRequiredAccessError,
AuthenticationRequiredAccessError,
EnrollmentRequiredAccessError,
StartDateError
)
from lms.djangoapps.courseware.masquerade import get_course_masquerade, is_masquerading_as_student
from openedx.core.djangoapps.util.user_messages import PageLevelMessages # lint-amnesty, pylint: disable=unused-import
from openedx.core.djangolib.markup import HTML # lint-amnesty, pylint: disable=unused-import
from openedx.features.course_experience import (
COURSE_PRE_START_ACCESS_FLAG,
COURSE_ENABLE_UNENROLLED_ACCESS_FLAG,
)
from common.djangoapps.student.models import CourseEnrollment
from common.djangoapps.student.roles import CourseBetaTesterRole
from xmodule.util.xmodule_django import get_current_request_hostname
from xmodule.course_module import COURSE_VISIBILITY_PUBLIC
from openedx.features.course_experience import COURSE_ENABLE_UNENROLLED_ACCESS_FLAG, COURSE_PRE_START_ACCESS_FLAG

DEBUG_ACCESS = False
log = getLogger(__name__)
Expand Down Expand Up @@ -75,7 +73,7 @@ def check_start_date(user, days_early_for_beta, start, course_key, display_error
Returns:
AccessResponse: Either ACCESS_GRANTED or StartDateError.
"""
start_dates_disabled = settings.FEATURES['DISABLE_START_DATES']
start_dates_disabled = settings.FEATURES.get('DISABLE_START_DATES', False)
masquerading_as_student = is_masquerading_as_student(user, course_key)

if start_dates_disabled and not masquerading_as_student:
Expand Down
2 changes: 1 addition & 1 deletion lms/djangoapps/support/tests/test_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,7 @@ def test_change_enrollment_mode_fullfills_entitlement(self, search_string_type,
'course_id': str(self.course.id),
'old_mode': CourseMode.AUDIT,
'new_mode': CourseMode.VERIFIED,
'reason': 'Financial Assistance'
'reason': u'Financial Assistance'
})
entitlement.refresh_from_db()
assert response.status_code == 200
Expand Down
14 changes: 14 additions & 0 deletions lms/envs/test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -597,3 +597,17 @@

RESET_PASSWORD_TOKEN_VALIDATE_API_RATELIMIT = '2/m'
RESET_PASSWORD_API_RATELIMIT = '2/m'

COURSE_ENROLLMENT_MODES['test'] = {
"id": 8,
"slug": u"test",
"display_name": u"Test",
"min_price": 0
}

COURSE_ENROLLMENT_MODES['test_mode'] = {
"id": 9,
"slug": u"test_mode",
"display_name": u"Test Mode",
"min_price": 0
}
13 changes: 8 additions & 5 deletions openedx/core/djangoapps/content/block_structure/store.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

from logging import getLogger

import six
from django.utils.encoding import python_2_unicode_compatible

from openedx.core.lib.cache_utils import zpickle, zunpickle
Expand Down Expand Up @@ -231,11 +232,13 @@ def _encode_root_cache_key(bs_model):
BlockStructureModel or StubModel.
"""
if config.STORAGE_BACKING_FOR_CACHE.is_enabled():
return str(bs_model)
return "v{version}.root.key.{root_usage_key}".format(
version=str(BlockStructureBlockData.VERSION),
root_usage_key=str(bs_model.data_usage_key),
)
return six.text_type(bs_model)

else:
return u"v{version}.root.key.{root_usage_key}".format(
version=six.text_type(BlockStructureBlockData.VERSION),
root_usage_key=six.text_type(bs_model.data_usage_key),
)

@staticmethod
def _version_data_of_block(root_block):
Expand Down
43 changes: 40 additions & 3 deletions openedx/core/djangoapps/enrollments/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,13 @@

import logging

from django.core.exceptions import PermissionDenied
from rest_framework import serializers
from xmodule.modulestore.django import modulestore

from common.djangoapps.course_modes.models import CourseMode
from common.djangoapps.student.models import CourseEnrollment
from lms.djangoapps.grades.course_grade_factory import CourseGradeFactory

log = logging.getLogger(__name__)

Expand Down Expand Up @@ -83,15 +86,49 @@ class CourseEnrollmentSerializer(serializers.ModelSerializer):

"""
course_details = CourseSerializer(source="course_overview")
user = serializers.SerializerMethodField('get_username')
user = serializers.SerializerMethodField("get_username")
finished = serializers.SerializerMethodField()

@kaizoku kaizoku Jul 15, 2019

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.

We should probably also specify the method name as is done for the user field.

@xirdneh xirdneh Jul 15, 2019 •
edited
Loading

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.

@kaizoku we can't do this because there's a quality error raised:

AssertionError: It is redundant to specify `get_finished` on SerializerMethodField 'finished' in serializer 'CourseEnrollmentSerializer', because it is the same as the default method name. Remove the `method_name` argument.

@kaizoku kaizoku Jul 15, 2019

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.

Ah gotcha, my mistake.

grading = serializers.SerializerMethodField()

def get_username(self, model):
"""Retrieves the username from the associated model."""
return model.username

class Meta:
def get_finished(self, model):
"""Retrieve finished course."""
course = modulestore().get_course(model.course_id)
if course:
try:
coursegrade = CourseGradeFactory().read(model.user, course).passed
except PermissionDenied:
return False
return coursegrade
return False

def get_grading(self, model):
"""Retrieve course grade."""
course = modulestore().get_course(model.course_id)
course_grade = None
summary = []
current_grade = 0
if course:
try:
course_grade = CourseGradeFactory().read(model.user, course)
current_grade = int(course_grade.percent * 100)
for section in course_grade.summary.get(u'section_breakdown'):
if section.get(u'prominent'):
summary.append(section)
except PermissionDenied:
pass

@kaizoku kaizoku Jul 15, 2019

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.

This won't do anything if it fails permission checks, should we instead return an empty response?
This would wind up looking like a 0% grade instead, but perhaps that's the correct behavior, I'm not sure.

@xirdneh xirdneh Jul 15, 2019 •
edited
Loading

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.

The values are initialized at the beginning of the method. If there's a permission issue the response would be:

{
    'current_grade': 0,
    'certificate_eligible': False,
    'summary': [],
}

@kaizoku kaizoku Jul 15, 2019

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.

Right, I'm wondering if it would be better to imply an empty grade like that, or to return an empty list entirely.

@xirdneh xirdneh Jul 16, 2019

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.

@kaizoku I think to avoid issues on the frontend it's better to return an empty grade like this. That way the front-end code doesn't have to change if there's a permission error.
Now, I think probably a better solution would be to return a 403. You think that's acceptable?

@kaizoku kaizoku Jul 17, 2019

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.

I'm not sure, perhaps the edX reviewer will have some ideas on this?

@viadanna viadanna Aug 12, 2019

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.

@ormsbee Can you comment on this change? Is this approach feasible?

return [
{u'current_grade': current_grade,
u'certificate_eligible': course_grade.passed if course_grade else False,

@Agrendalath Agrendalath Oct 2, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is not consistent with the certificates API, so it can return different values than generated grade reports.

@viadanna viadanna Oct 4, 2021

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.

Thanks for catching this @Agrendalath possibly one of the changes that have been impacting this PR since it has been created. I'll schedule some time to look into this, resolve conflicts and fix tests.

u'summary': summary}
]

class Meta(object):
model = CourseEnrollment
fields = ('created', 'mode', 'is_active', 'course_details', 'user')
fields = ('created', 'mode', 'is_active', 'course_details', 'user', 'finished', 'grading')
lookup_field = 'username'


Expand Down
Loading