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
12 changes: 9 additions & 3 deletions lms/djangoapps/grades/new/course_grade.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class CourseGradeBase(object):
"""
Base class for Course Grades.
"""
def __init__(self, user, course_data, percent=0, letter_grade=None, passed=False):
def __init__(self, user, course_data, percent=0, letter_grade=None, passed=False, force_update_subsections=False):
self.user = user
self.course_data = course_data

Expand All @@ -30,6 +30,7 @@ def __init__(self, user, course_data, percent=0, letter_grade=None, passed=False

# Convert empty strings to None when reading from the table
self.letter_grade = letter_grade or None
self.force_update_subsections = force_update_subsections

def __unicode__(self):
return u'Course Grade: percent: {}, letter_grade: {}, passed: {}'.format(
Expand Down Expand Up @@ -203,7 +204,9 @@ def __init__(self, user, course_data, *args, **kwargs):

def update(self):
"""
Updates the grade for the course.
Updates the grade for the course. Also updates subsection grades
if self.force_update_subsections is true, via the lazy call
to self.grader_result.
"""
grade_cutoffs = self.course_data.course.grade_cutoffs
self.percent = self._compute_percent(self.grader_result)
Expand All @@ -224,7 +227,10 @@ def attempted(self):

def _get_subsection_grade(self, subsection):
# Pass read_only here so the subsection grades can be persisted in bulk at the end.
return self._subsection_grade_factory.create(subsection, read_only=True)
if self.force_update_subsections:
return self._subsection_grade_factory.update(subsection)
else:
return self._subsection_grade_factory.create(subsection, read_only=True)

@staticmethod
def _compute_percent(grader_result):
Expand Down
29 changes: 22 additions & 7 deletions lms/djangoapps/grades/new/course_grade_factory.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,15 @@ def read(self, user, course=None, collected_block_structure=None, course_structu
else:
return None

def update(self, user, course=None, collected_block_structure=None, course_structure=None, course_key=None):
def update(
self,
user,
course=None,
collected_block_structure=None,
course_structure=None,
course_key=None,
force_update_subsections=False,

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.

Q: why does this need to be passed in? Won't we always want this to be true in this update method, per caller's expectations?

@sanfordstudent sanfordstudent Jul 21, 2017

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.

We need to pass it in because we also call CourseGradeFactory().update when a subsection grade is updated. If we assume true for all cases, then all subsection grades will update whenever a learner submits a problem.

https://github.com/edx/edx-platform/blob/master/lms/djangoapps/grades/signals/handlers.py#L246

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.

gotcha! +1

):
"""
Computes, updates, and returns the CourseGrade for the given
user in the course.
Expand All @@ -75,7 +83,7 @@ def update(self, user, course=None, collected_block_structure=None, course_struc
or course_key should be provided.
"""
course_data = CourseData(user, course, collected_block_structure, course_structure, course_key)
return self._update(user, course_data, read_only=False)
return self._update(user, course_data, read_only=False, force_update_subsections=force_update_subsections)

@contextmanager
def _course_transaction(self, course_key):
Expand Down Expand Up @@ -118,10 +126,17 @@ def iter(

def _iter_grade_result(self, user, course_data, force_update):
try:
kwargs = {
'user': user,
'course': course_data.course,
'collected_block_structure': course_data.collected_structure,
'course_key': course_data.course_key
}
if force_update:
kwargs['force_update_subsections'] = True

method = CourseGradeFactory().update if force_update else CourseGradeFactory().create
course_grade = method(
user, course_data.course, course_data.collected_structure, course_key=course_data.course_key,
)
course_grade = method(**kwargs)

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.

You can revert these changes per my comment above regarding not needing to pass force_update_subsections to the CourseGradeFactory's update method - since it's assumed from the method's contract.

return self.GradeResult(user, course_grade, None)
except Exception as exc: # pylint: disable=broad-except
# Keep marching on even if this student couldn't be graded for
Expand Down Expand Up @@ -165,14 +180,14 @@ def _read(user, course_data):
return course_grade, persistent_grade.grading_policy_hash

@staticmethod
def _update(user, course_data, read_only):
def _update(user, course_data, read_only, force_update_subsections=False):
"""
Computes, saves, and returns a CourseGrade object for the
given user and course.
Sends a COURSE_GRADE_CHANGED signal to listeners and a
COURSE_GRADE_NOW_PASSED if learner has passed course.
"""
course_grade = CourseGrade(user, course_data)
course_grade = CourseGrade(user, course_data, force_update_subsections=force_update_subsections)
course_grade.update()

should_persist = (
Expand Down
2 changes: 1 addition & 1 deletion lms/djangoapps/grades/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ def compute_grades_for_course_v2(self, **kwargs):
@task(base=_BaseTask)
def compute_grades_for_course(course_key, offset, batch_size, **kwargs): # pylint: disable=unused-argument
"""
Compute grades for a set of students in the specified course.
Compute and save grades for a set of students in the specified course.

The set of students will be determined by the order of enrollment date, and
limited to at most <batch_size> students, starting from the specified
Expand Down
14 changes: 14 additions & 0 deletions lms/djangoapps/grades/tests/test_new.py
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,20 @@ def test_read_zero(self, assume_zero_enabled):
else:
self.assertIsNone(course_grade)

@ddt.data(True, False)
def test_iter_force_update(self, force_update):
base_string = 'lms.djangoapps.grades.new.subsection_grade_factory.SubsectionGradeFactory.{}'
desired_method_name = base_string.format('update' if force_update else 'create')
undesired_method_name = base_string.format('create' if force_update else 'update')
with patch(desired_method_name) as desired_call:
with patch(undesired_method_name) as undesired_call:
set(CourseGradeFactory().iter(
users=[self.request.user], course=self.course, force_update=force_update
))

self.assertTrue(desired_call.called)
self.assertFalse(undesired_call.called)

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.

Are you planning to add any non-mock tests - to verify that any previously stored Subsection-grades do get updated? And, to capture what happens to old zero grades?

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'd like to write a more robust test but refrained for now since this change is somewhat time-sensitive. I can add that in this PR if we're not comfortable merging without it.

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.

sounds good



@ddt.ddt
class TestSubsectionGradeFactory(ProblemSubmissionTestMixin, GradeTestBase):
Expand Down