From 939f268da808698be6265d3f9852dcb867f12712 Mon Sep 17 00:00:00 2001 From: Michael Terry Date: Thu, 2 Jul 2020 16:01:56 -0400 Subject: [PATCH] AA-225: Only consider scored items for past due assignments When considering if an assignment is past due for the dates tab, only look at the scored and graded units in the subsection (i.e. ignore reading and video units). This still leaves the "complete" field alone -- i.e. those subsections will still be left incomplete generally. But for assignment-focused tasks, they will instead be considered complete. --- .../xmodule/xmodule/tests/test_vertical.py | 59 ++++++++++++++----- common/lib/xmodule/xmodule/vertical_block.py | 41 ++++++++++++- lms/djangoapps/courseware/courses.py | 6 +- .../courseware/tests/test_courses.py | 26 ++++++++ lms/djangoapps/courseware/tests/test_views.py | 2 +- openedx/features/course_experience/utils.py | 30 +++++++--- 6 files changed, 135 insertions(+), 29 deletions(-) diff --git a/common/lib/xmodule/xmodule/tests/test_vertical.py b/common/lib/xmodule/xmodule/tests/test_vertical.py index 3287d884d7..117716b982 100644 --- a/common/lib/xmodule/xmodule/tests/test_vertical.py +++ b/common/lib/xmodule/xmodule/tests/test_vertical.py @@ -60,6 +60,9 @@ class StubCompletionService(object): """ return {candidate: self._completion_value for candidate in candidates} + def get_completable_children(self, node): + return node.get_children() + def get_complete_on_view_delay_ms(self): """ Return the completion-by-viewing delay in milliseconds. @@ -72,15 +75,15 @@ class StubCompletionService(object): def vertical_is_complete(self, item): if item.scope_ids.block_type != 'vertical': raise ValueError('The passed in xblock is not a vertical type!') - return self._completion_value + return self._completion_value == 1 if self._enabled else None class BaseVerticalBlockTest(XModuleXmlImportTest): """ Tests for the BaseVerticalBlock. """ - test_html_1 = 'Test HTML 1' - test_html_2 = 'Test HTML 2' + test_html = 'Test HTML' + test_problem = 'Test Problem' def setUp(self): super(BaseVerticalBlockTest, self).setUp() @@ -90,8 +93,8 @@ class BaseVerticalBlockTest(XModuleXmlImportTest): vertical = xml.VerticalFactory.build(parent=sequence) self.course = self.process_xml(course) - xml.HtmlFactory(parent=vertical, url_name='test-html-1', text=self.test_html_1) - xml.HtmlFactory(parent=vertical, url_name='test-html-2', text=self.test_html_2) + xml.HtmlFactory(parent=vertical, url_name='test-html', text=self.test_html) + xml.ProblemFactory(parent=vertical, url_name='test-problem', text=self.test_problem) self.course = self.process_xml(course) course_seq = self.course.get_children()[0] @@ -103,8 +106,10 @@ class BaseVerticalBlockTest(XModuleXmlImportTest): self.vertical = course_seq.get_children()[0] self.vertical.xmodule_runtime = self.module_system - self.html1block = self.vertical.get_children()[0] - self.html2block = self.vertical.get_children()[1] + self.html_block = self.vertical.get_children()[0] + self.problem_block = self.vertical.get_children()[1] + self.problem_block.has_score = True + self.problem_block.graded = True self.username = "bilbo" self.default_context = {"bookmarked": False, "username": self.username} @@ -151,8 +156,11 @@ class VerticalBlockTestCase(BaseVerticalBlockTest): html = self.module_system.render( self.vertical, view, self.default_context if context is None else context ).content - self.assertIn(self.test_html_1, html) - self.assertIn(self.test_html_2, html) + self.assertIn(self.test_html, html) + if view == STUDENT_VIEW: + self.assertIn(self.test_problem, html) + else: + self.assertNotIn(self.test_problem, html) self.assertIn("'due': datetime.datetime({year}, {month}, {day}".format( year=self.vertical.due.year, month=self.vertical.due.month, day=self.vertical.due.day), html) if view == STUDENT_VIEW: @@ -161,9 +169,30 @@ class VerticalBlockTestCase(BaseVerticalBlockTest): self.assert_bookmark_info(self.assertNotIn, html) if context: self.assertIn("'subsection_format': '{}'".format(context['format']), html) - self.assertIn("'completed': {}".format(completion_value), html) + self.assertIn("'completed': {}".format(completion_value == 1), html) self.assertIn("'past_due': {}".format(self.vertical.due < now), html) + @ddt.data(True, False) + def test_render_problem_without_score(self, has_score): + """ + Test the rendering of the student and public view. + """ + self.module_system._services['bookmarks'] = Mock() + self.module_system._services['user'] = StubUserService() + self.module_system._services['completion'] = StubCompletionService(enabled=True, completion_value=0) + + now = datetime.now(pytz.UTC) + self.vertical.due = now + timedelta(days=-1) + self.problem_block.has_score = has_score + + html = self.module_system.render(self.vertical, STUDENT_VIEW, self.default_context).content + if has_score: + self.assertIn("'completed': False", html) + self.assertIn("'past_due': True", html) + else: + self.assertIn("'completed': None", html) + self.assertIn("'past_due': False", html) + @ddt.unpack @ddt.data( (True, 0.9, True), @@ -176,7 +205,7 @@ class VerticalBlockTestCase(BaseVerticalBlockTest): """ Test that mark-completed-on-view-after-delay is only set for relevant child Xblocks. """ - with patch.object(self.html1block, 'render') as mock_student_view: + with patch.object(self.html_block, 'render') as mock_student_view: self.module_system._services['completion'] = StubCompletionService( enabled=completion_enabled, completion_value=completion_value, @@ -198,8 +227,8 @@ class VerticalBlockTestCase(BaseVerticalBlockTest): 'is_unit_page': True } html = self.module_system.render(self.vertical, AUTHOR_VIEW, context).content - self.assertNotIn(self.test_html_1, html) - self.assertNotIn(self.test_html_2, html) + self.assertNotIn(self.test_html, html) + self.assertNotIn(self.test_problem, html) # Vertical should render reorderable children on the container page reorderable_items = set() @@ -208,5 +237,5 @@ class VerticalBlockTestCase(BaseVerticalBlockTest): 'reorderable_items': reorderable_items, } html = self.module_system.render(self.vertical, AUTHOR_VIEW, context).content - self.assertIn(self.test_html_1, html) - self.assertIn(self.test_html_2, html) + self.assertIn(self.test_html, html) + self.assertIn(self.test_problem, html) diff --git a/common/lib/xmodule/xmodule/vertical_block.py b/common/lib/xmodule/xmodule/vertical_block.py index 9654d912e3..9de73be343 100644 --- a/common/lib/xmodule/xmodule/vertical_block.py +++ b/common/lib/xmodule/xmodule/vertical_block.py @@ -91,8 +91,8 @@ class VerticalBlock(SequenceFields, XModuleFields, StudioEditableBlock, XmlParse 'content': rendered_child.content }) - completed = completion_service and completion_service.vertical_is_complete(self) - past_due = not completed and self.due and self.due < datetime.now(pytz.UTC) + completed = self.is_block_complete_for_assignments(completion_service) + past_due = completed is False and self.due and self.due < datetime.now(pytz.UTC) fragment_context = { 'items': contents, 'xblock_context': context, @@ -224,3 +224,40 @@ class VerticalBlock(SequenceFields, XModuleFields, StudioEditableBlock, XmlParse xblock_body["content_type"] = "Sequence" return xblock_body + + # So far, we only need this here. Move it somewhere more sensible if other bits of code want it too. + def is_block_complete_for_assignments(self, completion_service): + """ + Considers a block complete only if all scored & graded leaf blocks are complete. + + This is different from the normal `complete` flag because children of the block that are informative (like + readings or videos) do not count. We only care about actual homework content. + + Compare with is_block_structure_complete_for_assignments in course_experience/utils.py, which does the same + calculation, but for a BlockStructure node and its children. + + Returns: + True if complete + False if not + None if no assignments present or no completion info present (don't show any past-due or complete info) + """ + if not completion_service or not completion_service.completion_tracking_enabled(): + return None + + children = completion_service.get_completable_children(self) + children_locations = [child.scope_ids.usage_id for child in children] + completions = completion_service.get_completions(children_locations) + + all_complete = None + for child in children: + complete = completions[child.scope_ids.usage_id] == 1 + graded = getattr(child, 'graded', False) + has_score = getattr(child, 'has_score', False) + weight = getattr(child, 'weight', 1) + scored = has_score and (weight is None or weight > 0) + if graded and scored: + if not complete: + return False + all_complete = True + + return all_complete diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index 58db73568e..91eea2ba15 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -3,7 +3,6 @@ Functions for accessing and displaying courses within the courseware. """ - import logging from collections import defaultdict, namedtuple from datetime import datetime @@ -12,7 +11,6 @@ import pytz import six from crum import get_current_request from django.conf import settings -from django.db.models import Prefetch from django.http import Http404, QueryDict from django.urls import reverse from django.utils.translation import ugettext as _ @@ -25,7 +23,6 @@ from six import text_type from openedx.core.lib.cache_utils import request_cached import branding -from course_modes.models import CourseMode from lms.djangoapps.courseware.access import has_access from lms.djangoapps.courseware.access_response import ( AuthenticationRequiredAccessError, @@ -60,6 +57,7 @@ from openedx.core.djangoapps.site_configuration import helpers as configuration_ from openedx.core.lib.api.view_utils import LazySequence from openedx.features.course_duration_limits.access import AuditExpiredError from openedx.features.course_experience import RELATIVE_DATES_FLAG +from openedx.features.course_experience.utils import is_block_structure_complete_for_assignments from static_replace import replace_static_urls from survey.utils import SurveyRequiredAccessError, check_survey_required_and_unanswered from util.date_utils import strftime_localized @@ -550,7 +548,7 @@ def get_course_assignments(course_key, user, include_access=False): if assignment_released: url = reverse('jump_to', args=[course_key, subsection_key]) - complete = block_data.get_xblock_field(subsection_key, 'complete', False) + complete = is_block_structure_complete_for_assignments(block_data, subsection_key) past_due = not complete and due < now assignments.append(_Assignment( subsection_key, title, url, due, contains_gated_content, complete, past_due, assignment_type diff --git a/lms/djangoapps/courseware/tests/test_courses.py b/lms/djangoapps/courseware/tests/test_courses.py index 4d716d7421..35a8ec7205 100644 --- a/lms/djangoapps/courseware/tests/test_courses.py +++ b/lms/djangoapps/courseware/tests/test_courses.py @@ -11,6 +11,8 @@ import ddt import mock import pytz import six +from completion.models import BlockCompletion +from completion.test_utils import CompletionWaffleTestMixin from crum import set_current_request from django.conf import settings from django.test.client import RequestFactory @@ -25,6 +27,7 @@ from lms.djangoapps.courseware.courses import ( get_cms_block_link, get_cms_course_link, get_course_about_section, + get_course_assignments, get_course_by_id, get_course_chapter_ids, get_course_info_section, @@ -454,3 +457,26 @@ class TestGetCourseChapters(ModuleStoreTestCase): course_chapter_ids, [six.text_type(child) for child in course.children] ) + + +class TestGetCourseAssignments(CompletionWaffleTestMixin, ModuleStoreTestCase): + """ + Tests for the `get_course_assignments` function. + """ + + def test_completion_ignores_non_scored_items(self): + """ + Test that we treat a sequential with incomplete (but not scored) items (like a video maybe) as complete. + """ + course = CourseFactory() + chapter = ItemFactory(parent=course, category='chapter', graded=True, due=datetime.datetime.now()) + sequential = ItemFactory(parent=chapter, category='sequential') + problem = ItemFactory(parent=sequential, category='problem', has_score=True) + ItemFactory(parent=sequential, category='video', has_score=False) + + self.override_waffle_switch(True) + BlockCompletion.objects.submit_completion(self.user, problem.location, 1) + + assignments = get_course_assignments(course.location.context_key, self.user, None) + self.assertEqual(len(assignments), 1) + self.assertTrue(assignments[0].complete) diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index f70d8f6f6e..0ec4836e00 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -3146,7 +3146,7 @@ class DatesTabTestCase(ModuleStoreTestCase): format='Homework', ) vertical = ItemFactory.create(category='vertical', parent_location=subsection.location) - ItemFactory.create(category='problem', parent_location=vertical.location) + ItemFactory.create(category='problem', parent_location=vertical.location, has_score=True) with patch('lms.djangoapps.courseware.views.views.get_enrollment') as mock_get_enrollment: mock_get_enrollment.return_value = { diff --git a/openedx/features/course_experience/utils.py b/openedx/features/course_experience/utils.py index 9e5085e370..5acb6dc4e4 100644 --- a/openedx/features/course_experience/utils.py +++ b/openedx/features/course_experience/utils.py @@ -299,14 +299,30 @@ def dates_banner_should_display(course_key, user): for section_key in block_data.get_children(course_usage_key): for subsection_key in block_data.get_children(section_key): subsection_due_date = block_data.get_xblock_field(subsection_key, 'due', None) - if subsection_due_date and ( - not block_data.get_xblock_field(subsection_key, 'complete', False) - and block_data.get_xblock_field(subsection_key, 'graded', False) - and subsection_due_date < timezone.now() - ): - # Display the banner if the due date for an incomplete graded subsection - # has passed + if (subsection_due_date and subsection_due_date < timezone.now() and + not is_block_structure_complete_for_assignments(block_data, subsection_key)): + # Display the banner if the due date for an incomplete graded subsection has passed return True, block_data.get_xblock_field(subsection_key, 'contains_gated_content', False) # Don't display the banner if there were no missed deadlines return False, False + + +def is_block_structure_complete_for_assignments(block_data, block_key): + """ + Considers a block complete only if all scored & graded leaf blocks are complete. + + This is different from the normal `complete` flag because children of the block that are informative (like + readings or videos) do not count. We only care about actual homework content. + """ + children = block_data.get_children(block_key) + if children: + return all(is_block_structure_complete_for_assignments(block_data, child_key) for child_key in children) + + complete = block_data.get_xblock_field(block_key, 'complete', False) + graded = block_data.get_xblock_field(block_key, 'graded', False) + has_score = block_data.get_xblock_field(block_key, 'has_score', False) + weight = block_data.get_xblock_field(block_key, 'weight', 1) + scored = has_score and (weight is None or weight > 0) + + return complete or not graded or not scored