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
4 changes: 2 additions & 2 deletions lms/djangoapps/grades/new/course_grade.py
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ def create(self, student, course, collected_block_structure=None, read_only=True

GradeResult = namedtuple('GradeResult', ['student', 'course_grade', 'err_msg'])

def iter(self, course, students):
def iter(self, course, students, read_only=True):
"""
Given a course and an iterable of students (User), yield a GradeResult
for every student enrolled in the course. GradeResult is a named tuple of:
Expand All @@ -388,7 +388,7 @@ def iter(self, course, students):
for student in students:
with dog_stats_api.timer('lms.grades.CourseGradeFactory.iter', tags=[u'action:{}'.format(course.id)]):
try:
course_grade = CourseGradeFactory().create(student, course, collected_block_structure)
course_grade = CourseGradeFactory().create(student, course, collected_block_structure, read_only=read_only)
yield self.GradeResult(student, course_grade, "")

except Exception as exc: # pylint: disable=broad-except
Expand Down
35 changes: 21 additions & 14 deletions lms/djangoapps/grades/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,10 @@
from celery_utils.persist_on_failure import PersistOnFailureTask
from courseware.model_data import get_score
from lms.djangoapps.course_blocks.api import get_course_blocks
from opaque_keys.edx.keys import UsageKey
from lms.djangoapps.courseware import courses
from opaque_keys.edx.keys import CourseKey, UsageKey
from opaque_keys.edx.locator import CourseLocator
from student.models import CourseEnrollment
from submissions import api as sub_api
from track.event_transaction_utils import (
set_event_transaction_type,
Expand All @@ -31,6 +33,7 @@

from .constants import ScoreDatabaseTableEnum
from .new.subsection_grade import SubsectionGradeFactory
from .new.course_grade import CourseGradeFactory
from .signals.signals import SUBSECTION_SCORE_CHANGED
from .transformer import GradesTransformer

Expand Down Expand Up @@ -58,12 +61,24 @@ class _BaseTask(PersistOnFailureTask, LoggedTask): # pylint: disable=abstract-m
abstract = True


@task
def compute_grades_for_course(course_key, offset, batch_size): # pylint: disable=unused-argument
@task(base=_BaseTask)
def compute_grades_for_course(course_key, offset, batch_size):

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.

Can you update this function signature to take kwargs instead? Passing kwargs should now be the best practice for all tasks we create in the future. Otherwise, we always run into backward compatibility issues as @efischer19 and I have encountered. (I was thinking of adding a parameter to this task, related to my PR, but ran into this issue.)
Also, I believe the task isn't retried unless it's encased in a try-except block with a call to self.retry. Please consult @efischer19 on this tomorrow.

"""
TODO: TNL-6690: Fill this task in and remove pylint disables
Compute 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
offset.
"""
pass

course = courses.get_course_by_id(CourseKey.from_string(course_key))
enrollments = CourseEnrollment.objects.filter(course_id=course.id).order_by('created')
student_iter = (enrollment.user for enrollment in enrollments[offset:offset + batch_size])
list(CourseGradeFactory().iter(
course,
students=student_iter,
read_only=False,
))


@task(bind=True, base=_BaseTask, default_retry_delay=30, routing_key=settings.RECALCULATE_GRADES_ROUTING_KEY)
Expand Down Expand Up @@ -136,7 +151,7 @@ def _recalculate_subsection_grade(self, **kwargs):
self.request.id,
kwargs,
))
raise _retry_recalculate_subsection_grade(self, exc=exc, **kwargs)
raise self.retry(kwargs=kwargs, exc=exc)


def _has_db_updated_with_new_score(self, scored_block_usage_key, **kwargs):
Expand Down Expand Up @@ -218,11 +233,3 @@ def _update_subsection_grades(
user=student,
subsection_grade=subsection_grade,
)


def _retry_recalculate_subsection_grade(self, exc=None, **kwargs):
"""
Calls retry for the recalculate_subsection_grade task with the
given inputs.
"""
self.retry(kwargs=kwargs, exc=exc)
80 changes: 67 additions & 13 deletions lms/djangoapps/grades/tests/test_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,11 @@
import itertools
from mock import patch, MagicMock
import pytz
import six
from util.date_utils import to_timestamp

from openedx.core.djangoapps.content.block_structure.exceptions import BlockStructureNotFound
from student.models import anonymous_id_for_user
from student.models import CourseEnrollment, anonymous_id_for_user
from student.tests.factories import UserFactory
from track.event_transaction_utils import (
create_new_event_transaction_id,
Expand All @@ -29,22 +30,17 @@
from lms.djangoapps.grades.constants import ScoreDatabaseTableEnum
from lms.djangoapps.grades.models import PersistentCourseGrade, PersistentSubsectionGrade
from lms.djangoapps.grades.signals.signals import PROBLEM_WEIGHTED_SCORE_CHANGED
from lms.djangoapps.grades.tasks import recalculate_subsection_grade_v3, RECALCULATE_GRADE_DELAY
from lms.djangoapps.grades.tasks import (
compute_grades_for_course,
recalculate_subsection_grade_v3,
RECALCULATE_GRADE_DELAY
)


@patch.dict(settings.FEATURES, {'PERSISTENT_GRADES_ENABLED_FOR_ALL_TESTS': False})
@ddt.ddt
class RecalculateSubsectionGradeTest(ModuleStoreTestCase):
class HasCourseWithProblemsMixin(object):
"""
Ensures that the recalculate subsection grade task functions as expected when run.
Mixin to provide tests with a sample course with graded subsections
"""
ENABLED_SIGNALS = ['course_published', 'pre_publish']

def setUp(self):
super(RecalculateSubsectionGradeTest, self).setUp()
self.user = UserFactory()
PersistentGradesEnabledFlag.objects.create(enabled_for_all_courses=True, enabled=True)

def set_up_course(self, enable_persistent_grades=True, create_multiple_subsections=False):
"""
Configures the course for this test.
Expand Down Expand Up @@ -100,6 +96,20 @@ def set_up_course(self, enable_persistent_grades=True, create_multiple_subsectio
_ = anonymous_id_for_user(self.user, self.course.id)
# pylint: enable=attribute-defined-outside-init,no-member


@patch.dict(settings.FEATURES, {'PERSISTENT_GRADES_ENABLED_FOR_ALL_TESTS': False})
@ddt.ddt
class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTestCase):
"""
Ensures that the recalculate subsection grade task functions as expected when run.
"""
ENABLED_SIGNALS = ['course_published', 'pre_publish']

def setUp(self):
super(RecalculateSubsectionGradeTest, self).setUp()
self.user = UserFactory()
PersistentGradesEnabledFlag.objects.create(enabled_for_all_courses=True, enabled=True)

@contextmanager
def mock_get_score(self, score=MagicMock(grade=1.0, max_grade=2.0)):
"""
Expand Down Expand Up @@ -363,3 +373,47 @@ def _assert_retry_not_called(self, mock_retry):
Verifies the task was not retried.
"""
self.assertFalse(mock_retry.called)


@ddt.ddt
class ComputeGradesForCourseTest(HasCourseWithProblemsMixin, ModuleStoreTestCase):
"""
Test compute_grades_for_course task.
"""

ENABLED_SIGNALS = ['course_published', 'pre_publish']

def setUp(self):
super(ComputeGradesForCourseTest, self).setUp()
self.users = [UserFactory.create() for _ in xrange(12)]
self.set_up_course()
for user in self.users:
CourseEnrollment.enroll(user, self.course.id)

@ddt.data(*xrange(0, 12, 3))
def test_behavior(self, batch_size):
result = compute_grades_for_course.delay(
course_key=six.text_type(self.course.id),
batch_size=batch_size,
offset=4
)
self.assertTrue(result.successful)
self.assertEqual(
PersistentCourseGrade.objects.filter(course_id=self.course.id).count(),
min(batch_size, 8) # No more than 8 due to offset
)
self.assertEqual(
PersistentSubsectionGrade.objects.filter(course_id=self.course.id).count(),
min(batch_size, 8) # No more than 8 due to offset
)

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.

Question for @edx/educator-neem devs: It looks like PersistentCourseGrade objects aren't getting created. Is there still a toggle for that which needs to get turned on for the test?

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 was going to suggest the write-if-engaged work might be at fault here, but that hasn't merged yet. I'd suggest manually stepping through the code in this test.

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's working fine now. I think I must have changed something since I updated the test. Not sure what it was.

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 reintroduced testing the PersistentCourseGrade count.


@ddt.data(*xrange(1, 12, 3))
def test_database_calls(self, batch_size):
per_user_queries = 16 * min(batch_size, 6) # No more than 6 due to offset

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

"per_user_queries" is not referenced. I think you meant to reference it on line 413, so I'll make that change in #14731 because I need to up the number of query calls there.

with self.assertNumQueries(3 + 16 * min(batch_size, 6)):
with check_mongo_calls(1):
compute_grades_for_course.delay(
course_key=six.text_type(self.course.id),
batch_size=batch_size,
offset=6,
)