From d92dee37c41cb2e4bd59bdc3164f350f7ae188cb Mon Sep 17 00:00:00 2001 From: Stephannie Jimenez Date: Thu, 5 Oct 2023 10:36:03 -0500 Subject: [PATCH 01/10] refactor: create a runtime service for answer_available function --- xmodule/capa_block.py | 81 +++++++++++++++---------------------------- xmodule/graders.py | 68 ++++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 54 deletions(-) diff --git a/xmodule/capa_block.py b/xmodule/capa_block.py index 7b58b5aa9ab8..f8d4054c16c1 100644 --- a/xmodule/capa_block.py +++ b/xmodule/capa_block.py @@ -33,7 +33,7 @@ from xmodule.contentstore.django import contentstore from xmodule.editing_block import EditingMixin from xmodule.exceptions import NotFoundError, ProcessingError -from xmodule.graders import ShowCorrectness +from xmodule.graders import ShowCorrectness, ShowAnswer from xmodule.raw_block import RawMixin from xmodule.util.sandboxing import SandboxService from xmodule.util.builtin_assets import add_webpack_js_to_fragment, add_sass_to_fragment @@ -203,20 +203,20 @@ class ProblemBlock( help=_("Defines when to show the answer to the problem. " "A default value can be set in Advanced Settings."), scope=Scope.settings, - default=SHOWANSWER.FINISHED, + default=ShowAnswer.FINISHED, values=[ - {"display_name": _("Always"), "value": SHOWANSWER.ALWAYS}, - {"display_name": _("Answered"), "value": SHOWANSWER.ANSWERED}, - {"display_name": _("Attempted or Past Due"), "value": SHOWANSWER.ATTEMPTED}, - {"display_name": _("Closed"), "value": SHOWANSWER.CLOSED}, - {"display_name": _("Finished"), "value": SHOWANSWER.FINISHED}, - {"display_name": _("Correct or Past Due"), "value": SHOWANSWER.CORRECT_OR_PAST_DUE}, - {"display_name": _("Past Due"), "value": SHOWANSWER.PAST_DUE}, - {"display_name": _("Never"), "value": SHOWANSWER.NEVER}, - {"display_name": _("After Some Number of Attempts"), "value": SHOWANSWER.AFTER_SOME_NUMBER_OF_ATTEMPTS}, - {"display_name": _("After All Attempts"), "value": SHOWANSWER.AFTER_ALL_ATTEMPTS}, - {"display_name": _("After All Attempts or Correct"), "value": SHOWANSWER.AFTER_ALL_ATTEMPTS_OR_CORRECT}, - {"display_name": _("Attempted"), "value": SHOWANSWER.ATTEMPTED_NO_PAST_DUE}, + {"display_name": _("Always"), "value": ShowAnswer.ALWAYS}, + {"display_name": _("Answered"), "value": ShowAnswer.ANSWERED}, + {"display_name": _("Attempted or Past Due"), "value": ShowAnswer.ATTEMPTED}, + {"display_name": _("Closed"), "value": ShowAnswer.CLOSED}, + {"display_name": _("Finished"), "value": ShowAnswer.FINISHED}, + {"display_name": _("Correct or Past Due"), "value": ShowAnswer.CORRECT_OR_PAST_DUE}, + {"display_name": _("Past Due"), "value": ShowAnswer.PAST_DUE}, + {"display_name": _("Never"), "value": ShowAnswer.NEVER}, + {"display_name": _("After Some Number of Attempts"), "value": ShowAnswer.AFTER_SOME_NUMBER_OF_ATTEMPTS}, + {"display_name": _("After All Attempts"), "value": ShowAnswer.AFTER_ALL_ATTEMPTS}, + {"display_name": _("After All Attempts or Correct"), "value": ShowAnswer.AFTER_ALL_ATTEMPTS_OR_CORRECT}, + {"display_name": _("Attempted"), "value": ShowAnswer.ATTEMPTED_NO_PAST_DUE}, ] ) attempts_before_showanswer_button = Integer( @@ -1407,46 +1407,19 @@ def answer_available(self): Is the user allowed to see an answer? """ user_is_staff = self.runtime.service(self, 'user').get_current_user().opt_attrs.get(ATTR_KEY_USER_IS_STAFF) - if not self.correctness_available(): - # If correctness is being withheld, then don't show answers either. - return False - elif self.showanswer == '': - return False - elif self.showanswer == SHOWANSWER.NEVER: - return False - elif user_is_staff: - # This is after the 'never' check because admins can see the answer - # unless the problem explicitly prevents it - return True - elif self.showanswer == SHOWANSWER.ATTEMPTED: - return self.is_attempted() or self.is_past_due() - elif self.showanswer == SHOWANSWER.ANSWERED: - # NOTE: this is slightly different from 'attempted' -- resetting the problems - # makes lcp.done False, but leaves attempts unchanged. - return self.is_correct() - elif self.showanswer == SHOWANSWER.CLOSED: - return self.closed() - elif self.showanswer == SHOWANSWER.FINISHED: - return self.closed() or self.is_correct() - - elif self.showanswer == SHOWANSWER.CORRECT_OR_PAST_DUE: - return self.is_correct() or self.is_past_due() - elif self.showanswer == SHOWANSWER.PAST_DUE: - return self.is_past_due() - elif self.showanswer == SHOWANSWER.AFTER_SOME_NUMBER_OF_ATTEMPTS: - required_attempts = self.attempts_before_showanswer_button - if self.max_attempts and required_attempts >= self.max_attempts: - required_attempts = self.max_attempts - return self.attempts >= required_attempts - elif self.showanswer == SHOWANSWER.ALWAYS: - return True - elif self.showanswer == SHOWANSWER.AFTER_ALL_ATTEMPTS: - return self.used_all_attempts() - elif self.showanswer == SHOWANSWER.AFTER_ALL_ATTEMPTS_OR_CORRECT: - return self.used_all_attempts() or self.is_correct() - elif self.showanswer == SHOWANSWER.ATTEMPTED_NO_PAST_DUE: - return self.is_attempted() - return False + return ShowAnswer.answer_available( + show_answer=self.showanswer, + show_correctness=self.show_correctness_available(), + past_due=self.is_past_due(), + attempts=self.attempts, + is_attempted=self.is_attempted(), + is_correct=self.is_correct(), + required_attempts=self.attempts_before_showanswer_button, + max_attempts=self.max_attempts, + used_all_attempts=self.used_all_attempts(), + closed=self.closed(), + has_staff_access=user_is_staff + ) def correctness_available(self): """ diff --git a/xmodule/graders.py b/xmodule/graders.py index a587204d682e..a689e95009d8 100644 --- a/xmodule/graders.py +++ b/xmodule/graders.py @@ -501,3 +501,71 @@ def correctness_available(cls, show_correctness='', due_date=None, has_staff_acc # else: show_correctness == cls.ALWAYS return True + + +class ShowAnswer: + """ + Helper class for determining whether an answer is currently hidden for a block. + + When an answer is hidden, this limits the user's access to it. + """ + + # Constants used to indicate when to show an answer + ALWAYS = "always" + ANSWERED = "answered" + ATTEMPTED = "attempted" + CLOSED = "closed" + FINISHED = "finished" + CORRECT_OR_PAST_DUE = "correct_or_past_due" + PAST_DUE = "past_due" + NEVER = "never" + AFTER_SOME_NUMBER_OF_ATTEMPTS = "after_attempts" + AFTER_ALL_ATTEMPTS = "after_all_attempts" + AFTER_ALL_ATTEMPTS_OR_CORRECT = "after_all_attempts_or_correct" + ATTEMPTED_NO_PAST_DUE = "attempted_no_past_due" + + @classmethod + def answer_available( + cls, show_answer='', show_correctness='', past_due=False, attempts=0, + is_attempted=False, is_correct=False, required_attempts=0, + max_attempts=0, used_all_attempts=False, closed=False, + has_staff_access=False): + """ + Returns whether correctness is available now, for the given attributes. + """ + if not show_correctness: + # If correctness is being withheld, then don't show answers either. + return False + elif show_answer == cls.NEVER: + return False + elif has_staff_access: + # This is after the 'never' check because course staff can see correctness + # unless the sequence/problem explicitly prevents it + return True + elif show_answer == cls.ATTEMPTED: + return is_attempted or past_due + elif show_answer == cls.ANSWERED: + # NOTE: this is slightly different from 'attempted' -- resetting the problems + # makes lcp.done False, but leaves attempts unchanged. + return is_correct + elif show_answer == cls.CLOSED: + return closed + elif show_answer == cls.FINISHED: + return closed or is_correct + elif show_answer == cls.CORRECT_OR_PAST_DUE: + return is_correct or past_due + elif show_answer == cls.PAST_DUE: + return past_due + elif show_answer == cls.AFTER_SOME_NUMBER_OF_ATTEMPTS: + if max_attempts and required_attempts >= max_attempts: + required_attempts = max_attempts + return attempts >= required_attempts + elif show_answer == cls.ALWAYS: + return True + elif show_answer == cls.AFTER_ALL_ATTEMPTS: + return used_all_attempts + elif show_answer == cls.AFTER_ALL_ATTEMPTS_OR_CORRECT: + return used_all_attempts or is_correct + elif show_answer == cls.ATTEMPTED_NO_PAST_DUE: + return is_attempted + return False From 47d081b71b4b1906e94fbb5cb1b44c7108ae49ce Mon Sep 17 00:00:00 2001 From: Stephannie Jimenez Date: Mon, 9 Oct 2023 22:58:46 -0500 Subject: [PATCH 02/10] refactor: create runtime services for showCorrectness and showAnswer --- cms/djangoapps/contentstore/utils.py | 12 ++- cms/djangoapps/contentstore/views/preview.py | 6 +- .../xblock_storage_handlers/view_handlers.py | 4 + lms/djangoapps/courseware/block_render.py | 11 ++- .../courseware/tests/test_block_render.py | 4 + lms/djangoapps/grades/subsection_grade.py | 5 +- .../core/djangoapps/xblock/runtime/runtime.py | 6 +- .../show_answer/show_answer_field_override.py | 12 +-- .../tests/test_show_answer_override.py | 26 +++--- xmodule/capa_block.py | 26 ++---- xmodule/graders.py | 69 +-------------- xmodule/services.py | 84 ++++++++++++++++++- xmodule/tests/__init__.py | 5 ++ xmodule/tests/test_capa_block.py | 7 +- xmodule/tests/test_graders.py | 83 +----------------- xmodule/tests/test_services.py | 83 +++++++++++++++++- 16 files changed, 242 insertions(+), 201 deletions(-) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 1a4b709622e6..d5ed9bf05c51 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -83,7 +83,13 @@ from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.exceptions import ItemNotFoundError # lint-amnesty, pylint: disable=wrong-import-order from xmodule.partitions.partitions_service import get_all_partitions_for_course # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.services import SettingsService, ConfigurationService, TeamsConfigurationService +from xmodule.services import ( + SettingsService, + ConfigurationService, + TeamsConfigurationService, + ShowAnswerService, + ShowCorrectnessService +) log = logging.getLogger(__name__) @@ -1159,7 +1165,9 @@ def load_services_for_studio(runtime, user): "settings": SettingsService(), "lti-configuration": ConfigurationService(CourseAllowPIISharingInLTIFlag), "teams_configuration": TeamsConfigurationService(), - "library_tools": LibraryToolsService(modulestore(), user.id) + "library_tools": LibraryToolsService(modulestore(), user.id), + "show_answer": ShowAnswerService(), + "show_correctness": ShowCorrectnessService(), } runtime._services.update(services) # lint-amnesty, pylint: disable=protected-access diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index 48c323c1aa88..3afeebbab392 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -21,7 +21,7 @@ from xmodule.exceptions import NotFoundError, ProcessingError from xmodule.modulestore.django import XBlockI18nService, modulestore from xmodule.partitions.partitions_service import PartitionService -from xmodule.services import SettingsService, TeamsConfigurationService +from xmodule.services import SettingsService, TeamsConfigurationService, ShowAnswerService, ShowCorrectnessService from xmodule.studio_editable import has_author_view from xmodule.util.sandboxing import SandboxService from xmodule.util.builtin_assets import add_webpack_js_to_fragment @@ -211,7 +211,9 @@ def _prepare_runtime_for_preview(request, block): "teams_configuration": TeamsConfigurationService(), "sandbox": SandboxService(contentstore=contentstore, course_id=course_id), "cache": CacheService(cache), - 'replace_urls': ReplaceURLService + 'replace_urls': ReplaceURLService, + "show_answer": ShowAnswerService(), + "show_correctness": ShowCorrectnessService(), } block.runtime.get_block_for_descriptor = partial(_load_preview_block, request) diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index 425f97e3751f..4260519147f2 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -82,6 +82,8 @@ ConfigurationService, SettingsService, TeamsConfigurationService, + ShowAnswerService, + ShowCorrectnessService ) # lint-amnesty, pylint: disable=wrong-import-order from xmodule.tabs import ( CourseTabList, @@ -350,6 +352,8 @@ def load_services_for_studio(runtime, user): "lti-configuration": ConfigurationService(CourseAllowPIISharingInLTIFlag), "teams_configuration": TeamsConfigurationService(), "library_tools": LibraryToolsService(modulestore(), user.id), + "show_answer": ShowAnswerService(), + "show_correctness": ShowCorrectnessService(), } runtime._services.update(services) # lint-amnesty, pylint: disable=protected-access diff --git a/lms/djangoapps/courseware/block_render.py b/lms/djangoapps/courseware/block_render.py index 650b4418b423..81afff2bde95 100644 --- a/lms/djangoapps/courseware/block_render.py +++ b/lms/djangoapps/courseware/block_render.py @@ -51,7 +51,14 @@ from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.partitions.partitions_service import PartitionService from xmodule.util.sandboxing import SandboxService -from xmodule.services import EventPublishingService, RebindUserService, SettingsService, TeamsConfigurationService +from xmodule.services import ( + EventPublishingService, + RebindUserService, + SettingsService, + TeamsConfigurationService, + ShowAnswerService, + ShowCorrectnessService +) from common.djangoapps.static_replace.services import ReplaceURLService from common.djangoapps.static_replace.wrapper import replace_urls_wrapper from lms.djangoapps.courseware.access import get_user_role, has_access @@ -635,6 +642,8 @@ def inner_get_block(block: XBlock) -> XBlock | None: 'teams_configuration': TeamsConfigurationService(), 'call_to_action': CallToActionService(), 'publish': EventPublishingService(user, course_id, track_function), + 'show_answer': ShowAnswerService(), + 'show_correctness': ShowCorrectnessService() } runtime.get_block_for_descriptor = inner_get_block diff --git a/lms/djangoapps/courseware/tests/test_block_render.py b/lms/djangoapps/courseware/tests/test_block_render.py index 80827afa632c..0ae51812bbea 100644 --- a/lms/djangoapps/courseware/tests/test_block_render.py +++ b/lms/djangoapps/courseware/tests/test_block_render.py @@ -130,6 +130,8 @@ @XBlock.needs('teams') @XBlock.needs('teams_configuration') @XBlock.needs('call_to_action') +@XBlock.needs('show_answer') +@XBlock.needs('show_correctness') class PureXBlock(XBlock): """ Pure XBlock to use in tests. @@ -2342,6 +2344,8 @@ class LMSXBlockServiceBindingTest(LMSXBlockServiceMixin): 'teams', 'teams_configuration', 'call_to_action', + 'show_correctness', + 'show_answer', ) def test_expected_services_exist(self, expected_service): """ diff --git a/lms/djangoapps/grades/subsection_grade.py b/lms/djangoapps/grades/subsection_grade.py index ba098a92a417..7500198b9fa7 100644 --- a/lms/djangoapps/grades/subsection_grade.py +++ b/lms/djangoapps/grades/subsection_grade.py @@ -12,7 +12,8 @@ from lms.djangoapps.grades.models import BlockRecord, PersistentSubsectionGrade from lms.djangoapps.grades.scores import compute_percent, get_score, possibly_scored from xmodule import block_metadata_utils, graders # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.graders import AggregatedScore, ShowCorrectness # lint-amnesty, pylint: disable=wrong-import-order +from xmodule.graders import AggregatedScore # lint-amnesty, pylint: disable=wrong-import-order +from xmodule.services import ShowCorrectnessService log = getLogger(__name__) @@ -59,7 +60,7 @@ def show_grades(self, has_staff_access): """ Returns whether subsection scores are currently available to users with or without staff access. """ - return ShowCorrectness.correctness_available(self.show_correctness, self.due, has_staff_access) + return ShowCorrectnessService.correctness_available(self.show_correctness, self.due, has_staff_access) @property def attempted_graded(self): diff --git a/openedx/core/djangoapps/xblock/runtime/runtime.py b/openedx/core/djangoapps/xblock/runtime/runtime.py index 74771833fedf..b1118d9c517a 100644 --- a/openedx/core/djangoapps/xblock/runtime/runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/runtime.py @@ -26,7 +26,7 @@ from xmodule.errortracker import make_error_tracker from xmodule.contentstore.django import contentstore from xmodule.modulestore.django import XBlockI18nService -from xmodule.services import EventPublishingService, RebindUserService +from xmodule.services import EventPublishingService, RebindUserService, ShowAnswerService, ShowCorrectnessService from xmodule.util.sandboxing import SandboxService from common.djangoapps.edxmako.services import MakoService from common.djangoapps.static_replace.services import ReplaceURLService @@ -299,6 +299,10 @@ def service(self, block: XBlock, service_name: str): ) elif service_name == 'publish': return EventPublishingService(self.user, context_key, make_track_function()) + elif service_name == 'show_answer': + return ShowAnswerService() + elif service_name == 'show_correctness': + return ShowCorrectnessService() # Check if the XBlockRuntimeSystem wants to handle this: service = self.system.get_service(block, service_name) diff --git a/openedx/features/personalized_learner_schedules/show_answer/show_answer_field_override.py b/openedx/features/personalized_learner_schedules/show_answer/show_answer_field_override.py index d6452a84962f..44bfe0ced9e9 100644 --- a/openedx/features/personalized_learner_schedules/show_answer/show_answer_field_override.py +++ b/openedx/features/personalized_learner_schedules/show_answer/show_answer_field_override.py @@ -5,7 +5,7 @@ from lms.djangoapps.courseware.field_overrides import FieldOverrideProvider from openedx.features.course_experience import RELATIVE_DATES_FLAG -from xmodule.capa_block import SHOWANSWER # lint-amnesty, pylint: disable=wrong-import-order +from xmodule.graders import ShowAnswer class ShowAnswerFieldOverride(FieldOverrideProvider): @@ -27,11 +27,11 @@ def get(self, block, name, default): return default mapping = { - SHOWANSWER.ATTEMPTED: SHOWANSWER.ATTEMPTED_NO_PAST_DUE, - SHOWANSWER.CLOSED: SHOWANSWER.AFTER_ALL_ATTEMPTS, - SHOWANSWER.CORRECT_OR_PAST_DUE: SHOWANSWER.ANSWERED, - SHOWANSWER.FINISHED: SHOWANSWER.AFTER_ALL_ATTEMPTS_OR_CORRECT, - SHOWANSWER.PAST_DUE: SHOWANSWER.NEVER, + ShowAnswer.ATTEMPTED: ShowAnswer.ATTEMPTED_NO_PAST_DUE, + ShowAnswer.CLOSED: ShowAnswer.AFTER_ALL_ATTEMPTS, + ShowAnswer.CORRECT_OR_PAST_DUE: ShowAnswer.ANSWERED, + ShowAnswer.FINISHED: ShowAnswer.AFTER_ALL_ATTEMPTS_OR_CORRECT, + ShowAnswer.PAST_DUE: ShowAnswer.NEVER, } current_show_answer_value = self.fallback_field_data.get(block, 'showanswer') diff --git a/openedx/features/personalized_learner_schedules/show_answer/tests/test_show_answer_override.py b/openedx/features/personalized_learner_schedules/show_answer/tests/test_show_answer_override.py index 0a0c03725545..938da08b1f05 100644 --- a/openedx/features/personalized_learner_schedules/show_answer/tests/test_show_answer_override.py +++ b/openedx/features/personalized_learner_schedules/show_answer/tests/test_show_answer_override.py @@ -10,7 +10,7 @@ from lms.djangoapps.courseware.model_data import FieldDataCache from lms.djangoapps.courseware.block_render import get_block from openedx.features.course_experience import RELATIVE_DATES_FLAG -from xmodule.capa_block import SHOWANSWER # lint-amnesty, pylint: disable=wrong-import-order +from xmodule.graders import ShowAnswer from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.tests.factories import CourseFactory # lint-amnesty, pylint: disable=wrong-import-order @@ -41,27 +41,27 @@ def test_override_enabled_for(self, active): # Instructor paced course will just have the default value ip_course = self.setup_course() course_block = self.get_course_block(ip_course) - assert course_block.showanswer == SHOWANSWER.FINISHED + assert course_block.showanswer == ShowAnswer.FINISHED # This should be updated to not explicitly add in the showanswer so it can test the # default case of never touching showanswer. Reference ticket AA-307 (if that's closed, # this can be updated!) - sp_course = self.setup_course(self_paced=True, showanswer=SHOWANSWER.FINISHED) + sp_course = self.setup_course(self_paced=True, showanswer=ShowAnswer.FINISHED) course_block = self.get_course_block(sp_course) if active: - assert course_block.showanswer == SHOWANSWER.AFTER_ALL_ATTEMPTS_OR_CORRECT + assert course_block.showanswer == ShowAnswer.AFTER_ALL_ATTEMPTS_OR_CORRECT else: - assert course_block.showanswer == SHOWANSWER.FINISHED + assert course_block.showanswer == ShowAnswer.FINISHED @ddt.data( - (SHOWANSWER.ATTEMPTED, SHOWANSWER.ATTEMPTED_NO_PAST_DUE), - (SHOWANSWER.CLOSED, SHOWANSWER.AFTER_ALL_ATTEMPTS), - (SHOWANSWER.CORRECT_OR_PAST_DUE, SHOWANSWER.ANSWERED), - (SHOWANSWER.FINISHED, SHOWANSWER.AFTER_ALL_ATTEMPTS_OR_CORRECT), - (SHOWANSWER.PAST_DUE, SHOWANSWER.NEVER), - (SHOWANSWER.NEVER, SHOWANSWER.NEVER), - (SHOWANSWER.AFTER_SOME_NUMBER_OF_ATTEMPTS, SHOWANSWER.AFTER_SOME_NUMBER_OF_ATTEMPTS), - (SHOWANSWER.ALWAYS, SHOWANSWER.ALWAYS), + (ShowAnswer.ATTEMPTED, ShowAnswer.ATTEMPTED_NO_PAST_DUE), + (ShowAnswer.CLOSED, ShowAnswer.AFTER_ALL_ATTEMPTS), + (ShowAnswer.CORRECT_OR_PAST_DUE, ShowAnswer.ANSWERED), + (ShowAnswer.FINISHED, ShowAnswer.AFTER_ALL_ATTEMPTS_OR_CORRECT), + (ShowAnswer.PAST_DUE, ShowAnswer.NEVER), + (ShowAnswer.NEVER, ShowAnswer.NEVER), + (ShowAnswer.AFTER_SOME_NUMBER_OF_ATTEMPTS, ShowAnswer.AFTER_SOME_NUMBER_OF_ATTEMPTS), + (ShowAnswer.ALWAYS, ShowAnswer.ALWAYS), ) @ddt.unpack @override_waffle_flag(RELATIVE_DATES_FLAG, active=True) diff --git a/xmodule/capa_block.py b/xmodule/capa_block.py index f8d4054c16c1..6968e751da09 100644 --- a/xmodule/capa_block.py +++ b/xmodule/capa_block.py @@ -74,24 +74,6 @@ FEATURES = {} -class SHOWANSWER: - """ - Constants for when to show answer - """ - ALWAYS = "always" - ANSWERED = "answered" - ATTEMPTED = "attempted" - CLOSED = "closed" - FINISHED = "finished" - CORRECT_OR_PAST_DUE = "correct_or_past_due" - PAST_DUE = "past_due" - NEVER = "never" - AFTER_SOME_NUMBER_OF_ATTEMPTS = "after_attempts" - AFTER_ALL_ATTEMPTS = "after_all_attempts" - AFTER_ALL_ATTEMPTS_OR_CORRECT = "after_all_attempts_or_correct" - ATTEMPTED_NO_PAST_DUE = "attempted_no_past_due" - - class RANDOMIZATION: """ Constants for problem randomization @@ -123,6 +105,8 @@ def from_json(self, value): @XBlock.needs('sandbox') @XBlock.needs('replace_urls') @XBlock.wants('call_to_action') +@XBlock.wants('show_correctness') +@XBlock.wants('show_answer') class ProblemBlock( ScorableXBlockMixin, RawMixin, @@ -1407,9 +1391,9 @@ def answer_available(self): Is the user allowed to see an answer? """ user_is_staff = self.runtime.service(self, 'user').get_current_user().opt_attrs.get(ATTR_KEY_USER_IS_STAFF) - return ShowAnswer.answer_available( + return self.runtime.service(self, 'show_answer').answer_available( show_answer=self.showanswer, - show_correctness=self.show_correctness_available(), + show_correctness=self.correctness_available(), past_due=self.is_past_due(), attempts=self.attempts, is_attempted=self.is_attempted(), @@ -1428,7 +1412,7 @@ def correctness_available(self): Limits access to the correct/incorrect flags, messages, and problem score. """ user_is_staff = self.runtime.service(self, 'user').get_current_user().opt_attrs.get(ATTR_KEY_USER_IS_STAFF) - return ShowCorrectness.correctness_available( + return self.runtime.service(self, 'show_correctness').correctness_available( show_correctness=self.show_correctness, due_date=self.close_date, has_staff_access=user_is_staff, diff --git a/xmodule/graders.py b/xmodule/graders.py index a689e95009d8..d4e384e3bc51 100644 --- a/xmodule/graders.py +++ b/xmodule/graders.py @@ -472,7 +472,7 @@ def _min_or_none(itr): class ShowCorrectness: """ - Helper class for determining whether correctness is currently hidden for a block. + Helper class for determining the possible status of correctness. When correctness is hidden, this limits the user's access to the correct/incorrect flags, messages, problem scores, and aggregate subsection and course grades. @@ -483,29 +483,10 @@ class ShowCorrectness: PAST_DUE = "past_due" NEVER = "never" - @classmethod - def correctness_available(cls, show_correctness='', due_date=None, has_staff_access=False): - """ - Returns whether correctness is available now, for the given attributes. - """ - if show_correctness == cls.NEVER: - return False - elif has_staff_access: - # This is after the 'never' check because course staff can see correctness - # unless the sequence/problem explicitly prevents it - return True - elif show_correctness == cls.PAST_DUE: - # Is it now past the due date? - return (due_date is None or - due_date < datetime.now(UTC)) - - # else: show_correctness == cls.ALWAYS - return True - class ShowAnswer: """ - Helper class for determining whether an answer is currently hidden for a block. + Helper class for determining the possible status for showing the answer. When an answer is hidden, this limits the user's access to it. """ @@ -523,49 +504,3 @@ class ShowAnswer: AFTER_ALL_ATTEMPTS = "after_all_attempts" AFTER_ALL_ATTEMPTS_OR_CORRECT = "after_all_attempts_or_correct" ATTEMPTED_NO_PAST_DUE = "attempted_no_past_due" - - @classmethod - def answer_available( - cls, show_answer='', show_correctness='', past_due=False, attempts=0, - is_attempted=False, is_correct=False, required_attempts=0, - max_attempts=0, used_all_attempts=False, closed=False, - has_staff_access=False): - """ - Returns whether correctness is available now, for the given attributes. - """ - if not show_correctness: - # If correctness is being withheld, then don't show answers either. - return False - elif show_answer == cls.NEVER: - return False - elif has_staff_access: - # This is after the 'never' check because course staff can see correctness - # unless the sequence/problem explicitly prevents it - return True - elif show_answer == cls.ATTEMPTED: - return is_attempted or past_due - elif show_answer == cls.ANSWERED: - # NOTE: this is slightly different from 'attempted' -- resetting the problems - # makes lcp.done False, but leaves attempts unchanged. - return is_correct - elif show_answer == cls.CLOSED: - return closed - elif show_answer == cls.FINISHED: - return closed or is_correct - elif show_answer == cls.CORRECT_OR_PAST_DUE: - return is_correct or past_due - elif show_answer == cls.PAST_DUE: - return past_due - elif show_answer == cls.AFTER_SOME_NUMBER_OF_ATTEMPTS: - if max_attempts and required_attempts >= max_attempts: - required_attempts = max_attempts - return attempts >= required_attempts - elif show_answer == cls.ALWAYS: - return True - elif show_answer == cls.AFTER_ALL_ATTEMPTS: - return used_all_attempts - elif show_answer == cls.AFTER_ALL_ATTEMPTS_OR_CORRECT: - return used_all_attempts or is_correct - elif show_answer == cls.ATTEMPTED_NO_PAST_DUE: - return is_attempted - return False diff --git a/xmodule/services.py b/xmodule/services.py index 25c680a9653a..eccb9cc90438 100644 --- a/xmodule/services.py +++ b/xmodule/services.py @@ -6,6 +6,9 @@ import inspect import logging from functools import partial +from datetime import datetime + +from pytz import UTC from config_models.models import ConfigurationModel from django.conf import settings @@ -17,13 +20,14 @@ from common.djangoapps.track import contexts from lms.djangoapps.courseware.masquerade import is_masquerading_as_specific_student from xmodule.modulestore.django import modulestore +from xmodule.graders import ShowCorrectness, ShowAnswer from lms.djangoapps.courseware.field_overrides import OverrideFieldData from lms.djangoapps.courseware.model_data import DjangoKeyValueStore, FieldDataCache from lms.djangoapps.lms_xblock.field_data import LmsFieldData from lms.djangoapps.lms_xblock.models import XBlockAsidesConfig -from lms.djangoapps.grades.api import signals as grades_signals +from lms.djangoapps.grades.signals import signals as grades_signals log = logging.getLogger(__name__) @@ -308,3 +312,81 @@ def _handle_deprecated_progress_event(self, block, event): # in order to avoid duplicate work and possibly conflicting semantics. if not getattr(block, 'has_custom_completion', False): self.completion_service.submit_completion(block.scope_ids.usage_id, 1.0) + + +class ShowAnswerService(Service): + """ + An XBlock Service that allows XModules to define if the answers are visible. + """ + @classmethod + def answer_available( + cls, show_answer='', show_correctness='', past_due=False, attempts=0, + is_attempted=False, is_correct=False, required_attempts=0, + max_attempts=0, used_all_attempts=False, closed=False, + has_staff_access=False): + """ + Returns whether correctness is available now, for the given attributes. + """ + if not show_correctness: + # If correctness is being withheld, then don't show answers either. + return False + elif show_answer == ShowAnswer.NEVER: + return False + elif has_staff_access: + # This is after the 'never' check because course staff can see correctness + # unless the sequence/problem explicitly prevents it + return True + elif show_answer == ShowAnswer.ATTEMPTED: + return is_attempted or past_due + elif show_answer == ShowAnswer.ANSWERED: + # NOTE: this is slightly different from 'attempted' -- resetting the problems + # makes lcp.done False, but leaves attempts unchanged. + return is_correct + elif show_answer == ShowAnswer.CLOSED: + return closed + elif show_answer == ShowAnswer.FINISHED: + return closed or is_correct + elif show_answer == ShowAnswer.CORRECT_OR_PAST_DUE: + return is_correct or past_due + elif show_answer == ShowAnswer.PAST_DUE: + return past_due + elif show_answer == ShowAnswer.AFTER_SOME_NUMBER_OF_ATTEMPTS: + if max_attempts and required_attempts >= max_attempts: + required_attempts = max_attempts + return attempts >= required_attempts + elif show_answer == ShowAnswer.ALWAYS: + return True + elif show_answer == ShowAnswer.AFTER_ALL_ATTEMPTS: + return used_all_attempts + elif show_answer == ShowAnswer.AFTER_ALL_ATTEMPTS_OR_CORRECT: + return used_all_attempts or is_correct + elif show_answer == ShowAnswer.ATTEMPTED_NO_PAST_DUE: + return is_attempted + return False + + +class ShowCorrectnessService(Service): + """ + An XBlock Service that allows XModules to define whether correctness is currently hidden. + + When correctness is hidden, this limits the user's access to the correct/incorrect flags, messages, problem scores, + and aggregate subsection and course grades. + """ + @classmethod + def correctness_available(cls, show_correctness='', due_date=None, has_staff_access=False): + """ + Returns whether correctness is available now, for the given attributes. + """ + if show_correctness == ShowCorrectness.NEVER: + return False + elif has_staff_access: + # This is after the 'never' check because course staff can see correctness + # unless the sequence/problem explicitly prevents it + return True + elif show_correctness == ShowCorrectness.PAST_DUE: + # Is it now past the due date? + return (due_date is None or + due_date < datetime.now(UTC)) + + # else: show_correctness == ShowCorrectness.ALWAYS + return True diff --git a/xmodule/tests/__init__.py b/xmodule/tests/__init__.py index b2cdd67b71ba..7b150af9a196 100644 --- a/xmodule/tests/__init__.py +++ b/xmodule/tests/__init__.py @@ -36,6 +36,7 @@ from xmodule.modulestore.xml import CourseLocationManager from xmodule.tests.helpers import StubReplaceURLService, mock_render_template, StubMakoService, StubUserService from xmodule.util.sandboxing import SandboxService +from xmodule.services import ShowAnswerService, ShowCorrectnessService from xmodule.x_module import DoNothingCache, XModuleMixin from openedx.core.lib.cache_utils import CacheService @@ -165,6 +166,8 @@ def get_block(block): 'cache': CacheService(DoNothingCache()), 'field-data': DictFieldData({}), 'sandbox': SandboxService(contentstore, course_id), + 'show_answer': ShowAnswerService(), + 'show_correctness': ShowCorrectnessService(), } descriptor_system.get_block_for_descriptor = get_block # lint-amnesty, pylint: disable=attribute-defined-outside-init @@ -220,6 +223,8 @@ def get_block(block): 'cache': CacheService(DoNothingCache()), 'field-data': DictFieldData({}), 'sandbox': SandboxService(contentstore, course_id), + 'show_answer': ShowAnswerService(), + 'show_correctness': ShowCorrectnessService(), } if add_overrides: diff --git a/xmodule/tests/test_capa_block.py b/xmodule/tests/test_capa_block.py index b511fe403e43..25cceea3b8c1 100644 --- a/xmodule/tests/test_capa_block.py +++ b/xmodule/tests/test_capa_block.py @@ -34,9 +34,10 @@ from xmodule.capa.responsetypes import LoncapaProblemError, ResponseError, StudentInputError from xmodule.capa.xqueue_interface import XQueueInterface from xmodule.capa_block import ComplexEncoder, ProblemBlock +from xmodule.graders import ShowAnswer from xmodule.tests import DATA_DIR -from ..capa_block import RANDOMIZATION, SHOWANSWER +from ..capa_block import RANDOMIZATION from . import get_test_system @@ -557,7 +558,7 @@ def test_showanswer_answered(self): """ # Can not see "Show Answer" when student answer is wrong answer_wrong = CapaFactory.create( - showanswer=SHOWANSWER.ANSWERED, + showanswer=ShowAnswer.ANSWERED, max_attempts="1", attempts="0", due=self.tomorrow_str, @@ -567,7 +568,7 @@ def test_showanswer_answered(self): # Expect to see "Show Answer" when answer is correct answer_correct = CapaFactory.create( - showanswer=SHOWANSWER.ANSWERED, + showanswer=ShowAnswer.ANSWERED, max_attempts="1", attempts="0", due=self.tomorrow_str, diff --git a/xmodule/tests/test_graders.py b/xmodule/tests/test_graders.py index 5073052b1ba8..fbc9aedcdadb 100644 --- a/xmodule/tests/test_graders.py +++ b/xmodule/tests/test_graders.py @@ -4,14 +4,13 @@ import unittest -from datetime import datetime, timedelta +from datetime import datetime import pytest import ddt -from pytz import UTC from lms.djangoapps.grades.scores import compute_percent from xmodule import graders -from xmodule.graders import AggregatedScore, ProblemScore, ShowCorrectness, aggregate_scores +from xmodule.graders import AggregatedScore, ProblemScore, aggregate_scores class GradesheetTest(unittest.TestCase): @@ -336,81 +335,3 @@ def test_grader_with_invalid_conf(self, invalid_conf, expected_error_message): with pytest.raises(ValueError) as error: graders.grader_from_conf([invalid_conf]) assert expected_error_message in str(error.value) - - -@ddt.ddt -class ShowCorrectnessTest(unittest.TestCase): - """ - Tests the correctness_available method - """ - - def setUp(self): - super().setUp() - - now = datetime.now(UTC) - day_delta = timedelta(days=1) - self.yesterday = now - day_delta - self.today = now - self.tomorrow = now + day_delta - - def test_show_correctness_default(self): - """ - Test that correctness is visible by default. - """ - assert ShowCorrectness.correctness_available() - - @ddt.data( - (ShowCorrectness.ALWAYS, True), - (ShowCorrectness.ALWAYS, False), - # Any non-constant values behave like "always" - ('', True), - ('', False), - ('other-value', True), - ('other-value', False), - ) - @ddt.unpack - def test_show_correctness_always(self, show_correctness, has_staff_access): - """ - Test that correctness is visible when show_correctness is turned on. - """ - assert ShowCorrectness.correctness_available(show_correctness=show_correctness, - has_staff_access=has_staff_access) - - @ddt.data(True, False) - def test_show_correctness_never(self, has_staff_access): - """ - Test that show_correctness="never" hides correctness from learners and course staff. - """ - assert not ShowCorrectness.correctness_available(show_correctness=ShowCorrectness.NEVER, - has_staff_access=has_staff_access) - - @ddt.data( - # Correctness not visible to learners if due date in the future - ('tomorrow', False, False), - # Correctness is visible to learners if due date in the past - ('yesterday', False, True), - # Correctness is visible to learners if due date in the past (just) - ('today', False, True), - # Correctness is visible to learners if there is no due date - (None, False, True), - # Correctness is visible to staff if due date in the future - ('tomorrow', True, True), - # Correctness is visible to staff if due date in the past - ('yesterday', True, True), - # Correctness is visible to staff if there is no due date - (None, True, True), - ) - @ddt.unpack - def test_show_correctness_past_due(self, due_date_str, has_staff_access, expected_result): - """ - Test show_correctness="past_due" to ensure: - * correctness is always visible to course staff - * correctness is always visible to everyone if there is no due date - * correctness is visible to learners after the due date, when there is a due date. - """ - if due_date_str is None: - due_date = None - else: - due_date = getattr(self, due_date_str) - assert ShowCorrectness.correctness_available(ShowCorrectness.PAST_DUE, due_date, has_staff_access) ==\ - expected_result diff --git a/xmodule/tests/test_services.py b/xmodule/tests/test_services.py index 3e571756ca5a..87ea588c6ae0 100644 --- a/xmodule/tests/test_services.py +++ b/xmodule/tests/test_services.py @@ -3,10 +3,12 @@ """ import unittest from unittest import mock +from datetime import datetime, timedelta import pytest from django.test import TestCase import ddt +from pytz import UTC from config_models.models import ConfigurationModel from django.conf import settings @@ -14,7 +16,8 @@ from xblock.runtime import Mixologist from opaque_keys.edx.locator import CourseLocator -from xmodule.services import ConfigurationService, SettingsService, TeamsConfigurationService +from xmodule.graders import ShowCorrectness +from xmodule.services import ConfigurationService, SettingsService, TeamsConfigurationService, ShowCorrectnessService from openedx.core.lib.teams_config import TeamsConfig @@ -163,3 +166,81 @@ class TestTeamsConfigurationService(ConfigurationServiceBaseClass): def test_get_teamsconfiguration(self): teams_config = self.configuration_service.get_teams_configuration(self.course.id) assert teams_config == self.teams_config + + +@ddt.ddt +class TestShowCorrectnessService(unittest.TestCase): + """ + Tests the correctness_available method + """ + + def setUp(self): + super().setUp() + + now = datetime.now(UTC) + day_delta = timedelta(days=1) + self.yesterday = now - day_delta + self.today = now + self.tomorrow = now + day_delta + + def test_show_correctness_default(self): + """ + Test that correctness is visible by default. + """ + assert ShowCorrectnessService.correctness_available() + + @ddt.data( + (ShowCorrectness.ALWAYS, True), + (ShowCorrectness.ALWAYS, False), + # Any non-constant values behave like "always" + ('', True), + ('', False), + ('other-value', True), + ('other-value', False), + ) + @ddt.unpack + def test_show_correctness_always(self, show_correctness, has_staff_access): + """ + Test that correctness is visible when show_correctness is turned on. + """ + assert ShowCorrectnessService.correctness_available( + show_correctness=show_correctness, has_staff_access=has_staff_access) + + @ddt.data(True, False) + def test_show_correctness_never(self, has_staff_access): + """ + Test that show_correctness="never" hides correctness from learners and course staff. + """ + assert not ShowCorrectnessService.correctness_available( + show_correctness=ShowCorrectness.NEVER, has_staff_access=has_staff_access) + + @ddt.data( + # Correctness not visible to learners if due date in the future + ('tomorrow', False, False), + # Correctness is visible to learners if due date in the past + ('yesterday', False, True), + # Correctness is visible to learners if due date in the past (just) + ('today', False, True), + # Correctness is visible to learners if there is no due date + (None, False, True), + # Correctness is visible to staff if due date in the future + ('tomorrow', True, True), + # Correctness is visible to staff if due date in the past + ('yesterday', True, True), + # Correctness is visible to staff if there is no due date + (None, True, True), + ) + @ddt.unpack + def test_show_correctness_past_due(self, due_date_str, has_staff_access, expected_result): + """ + Test show_correctness="past_due" to ensure: + * correctness is always visible to course staff + * correctness is always visible to everyone if there is no due date + * correctness is visible to learners after the due date, when there is a due date. + """ + if due_date_str is None: + due_date = None + else: + due_date = getattr(self, due_date_str) + assert ShowCorrectnessService.correctness_available(ShowCorrectness.PAST_DUE, due_date, has_staff_access) ==\ + expected_result From d9ac943832ba1875d9527f27bda8faf76490b7ab Mon Sep 17 00:00:00 2001 From: Stephannie Jimenez Date: Wed, 18 Oct 2023 10:21:51 -0500 Subject: [PATCH 03/10] fix: fix pylint issues in xmodule.graders --- xmodule/graders.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/xmodule/graders.py b/xmodule/graders.py index d4e384e3bc51..c58b6f877b5a 100644 --- a/xmodule/graders.py +++ b/xmodule/graders.py @@ -9,9 +9,7 @@ import random import sys from collections import OrderedDict -from datetime import datetime -from pytz import UTC from django.utils.translation import gettext_lazy as _ from xmodule.util.misc import get_short_labeler From 4cfabd204eaedf3c576fa057240bd2d235167061 Mon Sep 17 00:00:00 2001 From: Stephannie Jimenez Date: Sat, 18 Nov 2023 00:38:36 -0500 Subject: [PATCH 04/10] perf: reduce code complexity by merging the services into one --- cms/djangoapps/contentstore/utils.py | 6 +- cms/djangoapps/contentstore/views/preview.py | 5 +- .../xblock_storage_handlers/view_handlers.py | 6 +- lms/djangoapps/courseware/block_render.py | 6 +- .../courseware/tests/test_block_render.py | 6 +- lms/djangoapps/grades/subsection_grade.py | 5 +- .../core/djangoapps/xblock/runtime/runtime.py | 8 +-- xmodule/capa_block.py | 20 ++----- xmodule/services.py | 59 +++++++++++++------ xmodule/tests/__init__.py | 8 +-- xmodule/tests/test_services.py | 23 ++++---- 11 files changed, 75 insertions(+), 77 deletions(-) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index d5ed9bf05c51..26468b4588b5 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -87,8 +87,7 @@ SettingsService, ConfigurationService, TeamsConfigurationService, - ShowAnswerService, - ShowCorrectnessService + ResultService ) @@ -1166,8 +1165,7 @@ def load_services_for_studio(runtime, user): "lti-configuration": ConfigurationService(CourseAllowPIISharingInLTIFlag), "teams_configuration": TeamsConfigurationService(), "library_tools": LibraryToolsService(modulestore(), user.id), - "show_answer": ShowAnswerService(), - "show_correctness": ShowCorrectnessService(), + "result": ResultService, } runtime._services.update(services) # lint-amnesty, pylint: disable=protected-access diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index 3afeebbab392..1098cb877bf8 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -21,7 +21,7 @@ from xmodule.exceptions import NotFoundError, ProcessingError from xmodule.modulestore.django import XBlockI18nService, modulestore from xmodule.partitions.partitions_service import PartitionService -from xmodule.services import SettingsService, TeamsConfigurationService, ShowAnswerService, ShowCorrectnessService +from xmodule.services import SettingsService, TeamsConfigurationService, ResultService from xmodule.studio_editable import has_author_view from xmodule.util.sandboxing import SandboxService from xmodule.util.builtin_assets import add_webpack_js_to_fragment @@ -212,8 +212,7 @@ def _prepare_runtime_for_preview(request, block): "sandbox": SandboxService(contentstore=contentstore, course_id=course_id), "cache": CacheService(cache), 'replace_urls': ReplaceURLService, - "show_answer": ShowAnswerService(), - "show_correctness": ShowCorrectnessService(), + "result": partial(ResultService, user_is_staff=True), } block.runtime.get_block_for_descriptor = partial(_load_preview_block, request) diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index 4260519147f2..06848e1eb4cd 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -82,8 +82,7 @@ ConfigurationService, SettingsService, TeamsConfigurationService, - ShowAnswerService, - ShowCorrectnessService + ResultService ) # lint-amnesty, pylint: disable=wrong-import-order from xmodule.tabs import ( CourseTabList, @@ -352,8 +351,7 @@ def load_services_for_studio(runtime, user): "lti-configuration": ConfigurationService(CourseAllowPIISharingInLTIFlag), "teams_configuration": TeamsConfigurationService(), "library_tools": LibraryToolsService(modulestore(), user.id), - "show_answer": ShowAnswerService(), - "show_correctness": ShowCorrectnessService(), + "result": ResultService, } runtime._services.update(services) # lint-amnesty, pylint: disable=protected-access diff --git a/lms/djangoapps/courseware/block_render.py b/lms/djangoapps/courseware/block_render.py index 81afff2bde95..7692f33af64d 100644 --- a/lms/djangoapps/courseware/block_render.py +++ b/lms/djangoapps/courseware/block_render.py @@ -56,8 +56,7 @@ RebindUserService, SettingsService, TeamsConfigurationService, - ShowAnswerService, - ShowCorrectnessService + ResultService ) from common.djangoapps.static_replace.services import ReplaceURLService from common.djangoapps.static_replace.wrapper import replace_urls_wrapper @@ -642,8 +641,7 @@ def inner_get_block(block: XBlock) -> XBlock | None: 'teams_configuration': TeamsConfigurationService(), 'call_to_action': CallToActionService(), 'publish': EventPublishingService(user, course_id, track_function), - 'show_answer': ShowAnswerService(), - 'show_correctness': ShowCorrectnessService() + 'result': partial(ResultService, user_is_staff=user_is_staff) } runtime.get_block_for_descriptor = inner_get_block diff --git a/lms/djangoapps/courseware/tests/test_block_render.py b/lms/djangoapps/courseware/tests/test_block_render.py index 2fc22e9bbe94..816bda723582 100644 --- a/lms/djangoapps/courseware/tests/test_block_render.py +++ b/lms/djangoapps/courseware/tests/test_block_render.py @@ -130,8 +130,7 @@ @XBlock.needs('teams') @XBlock.needs('teams_configuration') @XBlock.needs('call_to_action') -@XBlock.needs('show_answer') -@XBlock.needs('show_correctness') +@XBlock.needs('result') class PureXBlock(XBlock): """ Pure XBlock to use in tests. @@ -2344,8 +2343,7 @@ class LMSXBlockServiceBindingTest(LMSXBlockServiceMixin): 'teams', 'teams_configuration', 'call_to_action', - 'show_correctness', - 'show_answer', + 'result', ) def test_expected_services_exist(self, expected_service): """ diff --git a/lms/djangoapps/grades/subsection_grade.py b/lms/djangoapps/grades/subsection_grade.py index 7500198b9fa7..14279c07b681 100644 --- a/lms/djangoapps/grades/subsection_grade.py +++ b/lms/djangoapps/grades/subsection_grade.py @@ -13,7 +13,7 @@ from lms.djangoapps.grades.scores import compute_percent, get_score, possibly_scored from xmodule import block_metadata_utils, graders # lint-amnesty, pylint: disable=wrong-import-order from xmodule.graders import AggregatedScore # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.services import ShowCorrectnessService +from xmodule.services import ResultService log = getLogger(__name__) @@ -24,6 +24,7 @@ class SubsectionGradeBase(metaclass=ABCMeta): """ def __init__(self, subsection): + self._block = subsection self.location = subsection.location self.display_name = block_metadata_utils.display_name_with_default(subsection) self.url_name = block_metadata_utils.url_name_for_block(subsection) @@ -60,7 +61,7 @@ def show_grades(self, has_staff_access): """ Returns whether subsection scores are currently available to users with or without staff access. """ - return ShowCorrectnessService.correctness_available(self.show_correctness, self.due, has_staff_access) + return ResultService(block=self._block, user_is_staff=has_staff_access).correctness_available() @property def attempted_graded(self): diff --git a/openedx/core/djangoapps/xblock/runtime/runtime.py b/openedx/core/djangoapps/xblock/runtime/runtime.py index b1118d9c517a..11565876ab62 100644 --- a/openedx/core/djangoapps/xblock/runtime/runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/runtime.py @@ -26,7 +26,7 @@ from xmodule.errortracker import make_error_tracker from xmodule.contentstore.django import contentstore from xmodule.modulestore.django import XBlockI18nService -from xmodule.services import EventPublishingService, RebindUserService, ShowAnswerService, ShowCorrectnessService +from xmodule.services import EventPublishingService, RebindUserService, ResultService from xmodule.util.sandboxing import SandboxService from common.djangoapps.edxmako.services import MakoService from common.djangoapps.static_replace.services import ReplaceURLService @@ -299,10 +299,8 @@ def service(self, block: XBlock, service_name: str): ) elif service_name == 'publish': return EventPublishingService(self.user, context_key, make_track_function()) - elif service_name == 'show_answer': - return ShowAnswerService() - elif service_name == 'show_correctness': - return ShowCorrectnessService() + elif service_name == 'result': + return ResultService(block=block, user_is_staff=self.user.is_staff) # Check if the XBlockRuntimeSystem wants to handle this: service = self.system.get_service(block, service_name) diff --git a/xmodule/capa_block.py b/xmodule/capa_block.py index 6968e751da09..4ab9a1859a49 100644 --- a/xmodule/capa_block.py +++ b/xmodule/capa_block.py @@ -105,8 +105,7 @@ def from_json(self, value): @XBlock.needs('sandbox') @XBlock.needs('replace_urls') @XBlock.wants('call_to_action') -@XBlock.wants('show_correctness') -@XBlock.wants('show_answer') +@XBlock.wants('result') class ProblemBlock( ScorableXBlockMixin, RawMixin, @@ -1390,19 +1389,13 @@ def answer_available(self): """ Is the user allowed to see an answer? """ - user_is_staff = self.runtime.service(self, 'user').get_current_user().opt_attrs.get(ATTR_KEY_USER_IS_STAFF) - return self.runtime.service(self, 'show_answer').answer_available( - show_answer=self.showanswer, - show_correctness=self.correctness_available(), - past_due=self.is_past_due(), + return self.runtime.service(self, 'result').answer_available( attempts=self.attempts, is_attempted=self.is_attempted(), is_correct=self.is_correct(), required_attempts=self.attempts_before_showanswer_button, - max_attempts=self.max_attempts, used_all_attempts=self.used_all_attempts(), - closed=self.closed(), - has_staff_access=user_is_staff + closed=self.closed() ) def correctness_available(self): @@ -1411,12 +1404,7 @@ def correctness_available(self): Limits access to the correct/incorrect flags, messages, and problem score. """ - user_is_staff = self.runtime.service(self, 'user').get_current_user().opt_attrs.get(ATTR_KEY_USER_IS_STAFF) - return self.runtime.service(self, 'show_correctness').correctness_available( - show_correctness=self.show_correctness, - due_date=self.close_date, - has_staff_access=user_is_staff, - ) + return self.runtime.service(self, 'result').correctness_available() def update_score(self, data): """ diff --git a/xmodule/services.py b/xmodule/services.py index eccb9cc90438..244e55f56979 100644 --- a/xmodule/services.py +++ b/xmodule/services.py @@ -314,25 +314,48 @@ def _handle_deprecated_progress_event(self, block, event): self.completion_service.submit_completion(block.scope_ids.usage_id, 1.0) -class ShowAnswerService(Service): +class ResultService(Service): """ - An XBlock Service that allows XModules to define if the answers are visible. + An XBlock Service that allows XModules to define: + + - if the answers are visible + - whether correctness is currently hidden. When correctness is hidden, this + limits the user's access to the correct/incorrect flags, messages, problem + scores, and aggregate subsection and course grades. """ - @classmethod + def __init__(self, block=None, user_is_staff=False, **kwargs): + super().__init__(**kwargs) + self._user_is_staff = user_is_staff + # This is needed because the `Service` class initialization expects the XBlock passed as an `xblock` keyword + # argument, but the `service` method from the `DescriptorSystem` passes a `block`. + self._xblock = self.xblock() or block + def answer_available( - cls, show_answer='', show_correctness='', past_due=False, attempts=0, - is_attempted=False, is_correct=False, required_attempts=0, - max_attempts=0, used_all_attempts=False, closed=False, - has_staff_access=False): + self, attempts=0, is_attempted=False, is_correct=False, + required_attempts=0, used_all_attempts=False, closed=False): """ Returns whether correctness is available now, for the given attributes. """ + if self._xblock: + show_correctness = self._xblock.show_correctness + show_answer = self._xblock.showanswer + past_due = self._xblock.is_past_due() + max_attempts = self._xblock.max_attempts + else: + show_correctness = '' + show_answer = '' + past_due = False + max_attempts = 0 + if not show_correctness: # If correctness is being withheld, then don't show answers either. return False + elif show_correctness == ShowCorrectness.NEVER: + # If correctness is being hidden the answer is not shown + return False elif show_answer == ShowAnswer.NEVER: return False - elif has_staff_access: + elif self._user_is_staff: # This is after the 'never' check because course staff can see correctness # unless the sequence/problem explicitly prevents it return True @@ -364,22 +387,20 @@ def answer_available( return is_attempted return False - -class ShowCorrectnessService(Service): - """ - An XBlock Service that allows XModules to define whether correctness is currently hidden. - - When correctness is hidden, this limits the user's access to the correct/incorrect flags, messages, problem scores, - and aggregate subsection and course grades. - """ - @classmethod - def correctness_available(cls, show_correctness='', due_date=None, has_staff_access=False): + def correctness_available(self): """ Returns whether correctness is available now, for the given attributes. """ + if self._xblock: + due_date = self._xblock.due + show_correctness = self._xblock.show_correctness + else: + due_date = None + show_correctness = '' + if show_correctness == ShowCorrectness.NEVER: return False - elif has_staff_access: + elif self._user_is_staff: # This is after the 'never' check because course staff can see correctness # unless the sequence/problem explicitly prevents it return True diff --git a/xmodule/tests/__init__.py b/xmodule/tests/__init__.py index 7b150af9a196..48a5159e784b 100644 --- a/xmodule/tests/__init__.py +++ b/xmodule/tests/__init__.py @@ -36,7 +36,7 @@ from xmodule.modulestore.xml import CourseLocationManager from xmodule.tests.helpers import StubReplaceURLService, mock_render_template, StubMakoService, StubUserService from xmodule.util.sandboxing import SandboxService -from xmodule.services import ShowAnswerService, ShowCorrectnessService +from xmodule.services import ResultService from xmodule.x_module import DoNothingCache, XModuleMixin from openedx.core.lib.cache_utils import CacheService @@ -166,8 +166,7 @@ def get_block(block): 'cache': CacheService(DoNothingCache()), 'field-data': DictFieldData({}), 'sandbox': SandboxService(contentstore, course_id), - 'show_answer': ShowAnswerService(), - 'show_correctness': ShowCorrectnessService(), + 'result': ResultService, } descriptor_system.get_block_for_descriptor = get_block # lint-amnesty, pylint: disable=attribute-defined-outside-init @@ -223,8 +222,7 @@ def get_block(block): 'cache': CacheService(DoNothingCache()), 'field-data': DictFieldData({}), 'sandbox': SandboxService(contentstore, course_id), - 'show_answer': ShowAnswerService(), - 'show_correctness': ShowCorrectnessService(), + 'result': ResultService, } if add_overrides: diff --git a/xmodule/tests/test_services.py b/xmodule/tests/test_services.py index 87ea588c6ae0..00f0bab849d9 100644 --- a/xmodule/tests/test_services.py +++ b/xmodule/tests/test_services.py @@ -17,7 +17,7 @@ from opaque_keys.edx.locator import CourseLocator from xmodule.graders import ShowCorrectness -from xmodule.services import ConfigurationService, SettingsService, TeamsConfigurationService, ShowCorrectnessService +from xmodule.services import ConfigurationService, SettingsService, TeamsConfigurationService, ResultService from openedx.core.lib.teams_config import TeamsConfig @@ -169,14 +169,14 @@ def test_get_teamsconfiguration(self): @ddt.ddt -class TestShowCorrectnessService(unittest.TestCase): +class TestResultService(unittest.TestCase): """ Tests the correctness_available method """ def setUp(self): super().setUp() - + self._xblock = mock.MagicMock() now = datetime.now(UTC) day_delta = timedelta(days=1) self.yesterday = now - day_delta @@ -187,7 +187,7 @@ def test_show_correctness_default(self): """ Test that correctness is visible by default. """ - assert ShowCorrectnessService.correctness_available() + assert ResultService(self._xblock).correctness_available() @ddt.data( (ShowCorrectness.ALWAYS, True), @@ -203,16 +203,16 @@ def test_show_correctness_always(self, show_correctness, has_staff_access): """ Test that correctness is visible when show_correctness is turned on. """ - assert ShowCorrectnessService.correctness_available( - show_correctness=show_correctness, has_staff_access=has_staff_access) + self._xblock.show_correctness = show_correctness + assert ResultService(self._xblock, user_is_staff=has_staff_access).correctness_available() @ddt.data(True, False) def test_show_correctness_never(self, has_staff_access): """ Test that show_correctness="never" hides correctness from learners and course staff. """ - assert not ShowCorrectnessService.correctness_available( - show_correctness=ShowCorrectness.NEVER, has_staff_access=has_staff_access) + self._xblock.show_correctness = ShowCorrectness.NEVER + assert not ResultService(self._xblock, user_is_staff=has_staff_access).correctness_available() @ddt.data( # Correctness not visible to learners if due date in the future @@ -238,9 +238,10 @@ def test_show_correctness_past_due(self, due_date_str, has_staff_access, expecte * correctness is always visible to everyone if there is no due date * correctness is visible to learners after the due date, when there is a due date. """ + self._xblock.show_correctness = ShowCorrectness.PAST_DUE if due_date_str is None: - due_date = None + self._xblock.due = None else: - due_date = getattr(self, due_date_str) - assert ShowCorrectnessService.correctness_available(ShowCorrectness.PAST_DUE, due_date, has_staff_access) ==\ + self._xblock.due = getattr(self, due_date_str) + assert ResultService(self._xblock, user_is_staff=has_staff_access).correctness_available() ==\ expected_result From c98e242809be341027308a923c85ac5c72802538 Mon Sep 17 00:00:00 2001 From: Stephannie Jimenez Date: Sat, 18 Nov 2023 00:55:37 -0500 Subject: [PATCH 05/10] perf: remove answer available arguments to improve readability --- xmodule/capa_block.py | 9 +-------- xmodule/services.py | 16 +++++++++++++--- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/xmodule/capa_block.py b/xmodule/capa_block.py index 4ab9a1859a49..36e0fa4694ea 100644 --- a/xmodule/capa_block.py +++ b/xmodule/capa_block.py @@ -1389,14 +1389,7 @@ def answer_available(self): """ Is the user allowed to see an answer? """ - return self.runtime.service(self, 'result').answer_available( - attempts=self.attempts, - is_attempted=self.is_attempted(), - is_correct=self.is_correct(), - required_attempts=self.attempts_before_showanswer_button, - used_all_attempts=self.used_all_attempts(), - closed=self.closed() - ) + return self.runtime.service(self, 'result').answer_available() def correctness_available(self): """ diff --git a/xmodule/services.py b/xmodule/services.py index 244e55f56979..9979c8e259f6 100644 --- a/xmodule/services.py +++ b/xmodule/services.py @@ -330,9 +330,7 @@ def __init__(self, block=None, user_is_staff=False, **kwargs): # argument, but the `service` method from the `DescriptorSystem` passes a `block`. self._xblock = self.xblock() or block - def answer_available( - self, attempts=0, is_attempted=False, is_correct=False, - required_attempts=0, used_all_attempts=False, closed=False): + def answer_available(self): """ Returns whether correctness is available now, for the given attributes. """ @@ -341,11 +339,23 @@ def answer_available( show_answer = self._xblock.showanswer past_due = self._xblock.is_past_due() max_attempts = self._xblock.max_attempts + attempts = self._xblock.attempts + is_attempted = self._xblock.is_attempted() + is_correct = self._xblock.is_correct() + required_attempts = self._xblock.attempts_before_showanswer_button + used_all_attempts = self._xblock.used_all_attempts() + closed = self._xblock.closed() else: show_correctness = '' show_answer = '' past_due = False max_attempts = 0 + attempts = 0 + is_attempted = False + is_correct = False + required_attempts = 0 + used_all_attempts = False + closed = False if not show_correctness: # If correctness is being withheld, then don't show answers either. From 3eafca3ee735f92768183ed9034a7832da936d30 Mon Sep 17 00:00:00 2001 From: Stephannie Jimenez Date: Fri, 24 Nov 2023 12:22:22 -0500 Subject: [PATCH 06/10] refactor: rename service to ProblemFeedbackService --- cms/djangoapps/contentstore/utils.py | 4 ++-- cms/djangoapps/contentstore/views/preview.py | 4 ++-- .../xblock_storage_handlers/view_handlers.py | 4 ++-- lms/djangoapps/courseware/block_render.py | 4 ++-- lms/djangoapps/courseware/tests/test_block_render.py | 4 ++-- lms/djangoapps/grades/subsection_grade.py | 4 ++-- openedx/core/djangoapps/xblock/runtime/runtime.py | 6 +++--- xmodule/capa_block.py | 9 ++++++--- xmodule/services.py | 2 +- xmodule/tests/__init__.py | 6 +++--- xmodule/tests/test_capa_block.py | 4 ++-- xmodule/tests/test_services.py | 12 ++++++------ 12 files changed, 33 insertions(+), 30 deletions(-) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 26468b4588b5..7ca4b7dbd6d6 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -87,7 +87,7 @@ SettingsService, ConfigurationService, TeamsConfigurationService, - ResultService + ProblemFeedbackService ) @@ -1165,7 +1165,7 @@ def load_services_for_studio(runtime, user): "lti-configuration": ConfigurationService(CourseAllowPIISharingInLTIFlag), "teams_configuration": TeamsConfigurationService(), "library_tools": LibraryToolsService(modulestore(), user.id), - "result": ResultService, + "problem_feedback": ProblemFeedbackService, } runtime._services.update(services) # lint-amnesty, pylint: disable=protected-access diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index 1098cb877bf8..c9b3d522a0b7 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -21,7 +21,7 @@ from xmodule.exceptions import NotFoundError, ProcessingError from xmodule.modulestore.django import XBlockI18nService, modulestore from xmodule.partitions.partitions_service import PartitionService -from xmodule.services import SettingsService, TeamsConfigurationService, ResultService +from xmodule.services import SettingsService, TeamsConfigurationService, ProblemFeedbackService from xmodule.studio_editable import has_author_view from xmodule.util.sandboxing import SandboxService from xmodule.util.builtin_assets import add_webpack_js_to_fragment @@ -212,7 +212,7 @@ def _prepare_runtime_for_preview(request, block): "sandbox": SandboxService(contentstore=contentstore, course_id=course_id), "cache": CacheService(cache), 'replace_urls': ReplaceURLService, - "result": partial(ResultService, user_is_staff=True), + "problem_feedback": partial(ProblemFeedbackService, user_is_staff=True), } block.runtime.get_block_for_descriptor = partial(_load_preview_block, request) diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index 06848e1eb4cd..292cddecc980 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -82,7 +82,7 @@ ConfigurationService, SettingsService, TeamsConfigurationService, - ResultService + ProblemFeedbackService ) # lint-amnesty, pylint: disable=wrong-import-order from xmodule.tabs import ( CourseTabList, @@ -351,7 +351,7 @@ def load_services_for_studio(runtime, user): "lti-configuration": ConfigurationService(CourseAllowPIISharingInLTIFlag), "teams_configuration": TeamsConfigurationService(), "library_tools": LibraryToolsService(modulestore(), user.id), - "result": ResultService, + "problem_feedback": ProblemFeedbackService, } runtime._services.update(services) # lint-amnesty, pylint: disable=protected-access diff --git a/lms/djangoapps/courseware/block_render.py b/lms/djangoapps/courseware/block_render.py index 7692f33af64d..7873fcd909cf 100644 --- a/lms/djangoapps/courseware/block_render.py +++ b/lms/djangoapps/courseware/block_render.py @@ -56,7 +56,7 @@ RebindUserService, SettingsService, TeamsConfigurationService, - ResultService + ProblemFeedbackService ) from common.djangoapps.static_replace.services import ReplaceURLService from common.djangoapps.static_replace.wrapper import replace_urls_wrapper @@ -641,7 +641,7 @@ def inner_get_block(block: XBlock) -> XBlock | None: 'teams_configuration': TeamsConfigurationService(), 'call_to_action': CallToActionService(), 'publish': EventPublishingService(user, course_id, track_function), - 'result': partial(ResultService, user_is_staff=user_is_staff) + 'problem_feedback': partial(ProblemFeedbackService, user_is_staff=user_is_staff) } runtime.get_block_for_descriptor = inner_get_block diff --git a/lms/djangoapps/courseware/tests/test_block_render.py b/lms/djangoapps/courseware/tests/test_block_render.py index 816bda723582..113cdc7fc3d1 100644 --- a/lms/djangoapps/courseware/tests/test_block_render.py +++ b/lms/djangoapps/courseware/tests/test_block_render.py @@ -130,7 +130,7 @@ @XBlock.needs('teams') @XBlock.needs('teams_configuration') @XBlock.needs('call_to_action') -@XBlock.needs('result') +@XBlock.needs('problem_feedback') class PureXBlock(XBlock): """ Pure XBlock to use in tests. @@ -2343,7 +2343,7 @@ class LMSXBlockServiceBindingTest(LMSXBlockServiceMixin): 'teams', 'teams_configuration', 'call_to_action', - 'result', + 'problem_feedback', ) def test_expected_services_exist(self, expected_service): """ diff --git a/lms/djangoapps/grades/subsection_grade.py b/lms/djangoapps/grades/subsection_grade.py index 14279c07b681..54e844349276 100644 --- a/lms/djangoapps/grades/subsection_grade.py +++ b/lms/djangoapps/grades/subsection_grade.py @@ -13,7 +13,7 @@ from lms.djangoapps.grades.scores import compute_percent, get_score, possibly_scored from xmodule import block_metadata_utils, graders # lint-amnesty, pylint: disable=wrong-import-order from xmodule.graders import AggregatedScore # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.services import ResultService +from xmodule.services import ProblemFeedbackService log = getLogger(__name__) @@ -61,7 +61,7 @@ def show_grades(self, has_staff_access): """ Returns whether subsection scores are currently available to users with or without staff access. """ - return ResultService(block=self._block, user_is_staff=has_staff_access).correctness_available() + return ProblemFeedbackService(block=self._block, user_is_staff=has_staff_access).correctness_available() @property def attempted_graded(self): diff --git a/openedx/core/djangoapps/xblock/runtime/runtime.py b/openedx/core/djangoapps/xblock/runtime/runtime.py index 11565876ab62..02b729d491a9 100644 --- a/openedx/core/djangoapps/xblock/runtime/runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/runtime.py @@ -26,7 +26,7 @@ from xmodule.errortracker import make_error_tracker from xmodule.contentstore.django import contentstore from xmodule.modulestore.django import XBlockI18nService -from xmodule.services import EventPublishingService, RebindUserService, ResultService +from xmodule.services import EventPublishingService, RebindUserService, ProblemFeedbackService from xmodule.util.sandboxing import SandboxService from common.djangoapps.edxmako.services import MakoService from common.djangoapps.static_replace.services import ReplaceURLService @@ -299,8 +299,8 @@ def service(self, block: XBlock, service_name: str): ) elif service_name == 'publish': return EventPublishingService(self.user, context_key, make_track_function()) - elif service_name == 'result': - return ResultService(block=block, user_is_staff=self.user.is_staff) + elif service_name == 'problem_feedback': + return ProblemFeedbackService(block=block, user_is_staff=self.user.is_staff) # Check if the XBlockRuntimeSystem wants to handle this: service = self.system.get_service(block, service_name) diff --git a/xmodule/capa_block.py b/xmodule/capa_block.py index 36e0fa4694ea..7cf5a9ce6850 100644 --- a/xmodule/capa_block.py +++ b/xmodule/capa_block.py @@ -74,6 +74,9 @@ FEATURES = {} +# Left for backwards compatibility +SHOWANSWER = ShowAnswer + class RANDOMIZATION: """ Constants for problem randomization @@ -104,8 +107,8 @@ def from_json(self, value): @XBlock.needs('cache') @XBlock.needs('sandbox') @XBlock.needs('replace_urls') +@XBlock.needs('problem_feedback') @XBlock.wants('call_to_action') -@XBlock.wants('result') class ProblemBlock( ScorableXBlockMixin, RawMixin, @@ -1389,7 +1392,7 @@ def answer_available(self): """ Is the user allowed to see an answer? """ - return self.runtime.service(self, 'result').answer_available() + return self.runtime.service(self, 'problem_feedback').answer_available() def correctness_available(self): """ @@ -1397,7 +1400,7 @@ def correctness_available(self): Limits access to the correct/incorrect flags, messages, and problem score. """ - return self.runtime.service(self, 'result').correctness_available() + return self.runtime.service(self, 'problem_feedback').correctness_available() def update_score(self, data): """ diff --git a/xmodule/services.py b/xmodule/services.py index 9979c8e259f6..65122034bd87 100644 --- a/xmodule/services.py +++ b/xmodule/services.py @@ -314,7 +314,7 @@ def _handle_deprecated_progress_event(self, block, event): self.completion_service.submit_completion(block.scope_ids.usage_id, 1.0) -class ResultService(Service): +class ProblemFeedbackService(Service): """ An XBlock Service that allows XModules to define: diff --git a/xmodule/tests/__init__.py b/xmodule/tests/__init__.py index 48a5159e784b..1d7e2679f227 100644 --- a/xmodule/tests/__init__.py +++ b/xmodule/tests/__init__.py @@ -36,7 +36,7 @@ from xmodule.modulestore.xml import CourseLocationManager from xmodule.tests.helpers import StubReplaceURLService, mock_render_template, StubMakoService, StubUserService from xmodule.util.sandboxing import SandboxService -from xmodule.services import ResultService +from xmodule.services import ProblemFeedbackService from xmodule.x_module import DoNothingCache, XModuleMixin from openedx.core.lib.cache_utils import CacheService @@ -166,7 +166,7 @@ def get_block(block): 'cache': CacheService(DoNothingCache()), 'field-data': DictFieldData({}), 'sandbox': SandboxService(contentstore, course_id), - 'result': ResultService, + 'problem_feedback': ProblemFeedbackService, } descriptor_system.get_block_for_descriptor = get_block # lint-amnesty, pylint: disable=attribute-defined-outside-init @@ -222,7 +222,7 @@ def get_block(block): 'cache': CacheService(DoNothingCache()), 'field-data': DictFieldData({}), 'sandbox': SandboxService(contentstore, course_id), - 'result': ResultService, + 'problem_feedback': ProblemFeedbackService, } if add_overrides: diff --git a/xmodule/tests/test_capa_block.py b/xmodule/tests/test_capa_block.py index 25cceea3b8c1..8a8d1eb28a42 100644 --- a/xmodule/tests/test_capa_block.py +++ b/xmodule/tests/test_capa_block.py @@ -276,7 +276,7 @@ def test_showanswer_attempted(self): ({'showanswer': 'attempted', 'max_attempts': '1', 'show_correctness': 'never', }, False, False), # If show_correctness=past_due, answer is not visible before due date ({'showanswer': 'attempted', 'show_correctness': 'past_due', 'max_attempts': '1', 'due': 'tomorrow_str', }, - False, False), + False, True), # If show_correctness=past_due, answer is visible after due date ({'showanswer': 'attempted', 'show_correctness': 'past_due', 'max_attempts': '1', 'due': 'yesterday_str', }, True, True)) @@ -609,7 +609,7 @@ def test_show_correctness_never(self): # Correctness not visible because grace period hasn't expired, # even after using up all attempts ({'show_correctness': 'past_due', 'max_attempts': '1', 'attempts': '1', 'due': 'yesterday_str', - 'graceperiod': 'two_day_delta_str', }, False)) + 'graceperiod': 'two_day_delta_str', }, True)) @ddt.unpack def test_show_correctness_past_due(self, problem_data, expected_result): """ diff --git a/xmodule/tests/test_services.py b/xmodule/tests/test_services.py index 00f0bab849d9..d3725e8c4f98 100644 --- a/xmodule/tests/test_services.py +++ b/xmodule/tests/test_services.py @@ -17,7 +17,7 @@ from opaque_keys.edx.locator import CourseLocator from xmodule.graders import ShowCorrectness -from xmodule.services import ConfigurationService, SettingsService, TeamsConfigurationService, ResultService +from xmodule.services import ConfigurationService, SettingsService, TeamsConfigurationService, ProblemFeedbackService from openedx.core.lib.teams_config import TeamsConfig @@ -169,7 +169,7 @@ def test_get_teamsconfiguration(self): @ddt.ddt -class TestResultService(unittest.TestCase): +class TestProblemFeedbackService(unittest.TestCase): """ Tests the correctness_available method """ @@ -187,7 +187,7 @@ def test_show_correctness_default(self): """ Test that correctness is visible by default. """ - assert ResultService(self._xblock).correctness_available() + assert ProblemFeedbackService(self._xblock).correctness_available() @ddt.data( (ShowCorrectness.ALWAYS, True), @@ -204,7 +204,7 @@ def test_show_correctness_always(self, show_correctness, has_staff_access): Test that correctness is visible when show_correctness is turned on. """ self._xblock.show_correctness = show_correctness - assert ResultService(self._xblock, user_is_staff=has_staff_access).correctness_available() + assert ProblemFeedbackService(self._xblock, user_is_staff=has_staff_access).correctness_available() @ddt.data(True, False) def test_show_correctness_never(self, has_staff_access): @@ -212,7 +212,7 @@ def test_show_correctness_never(self, has_staff_access): Test that show_correctness="never" hides correctness from learners and course staff. """ self._xblock.show_correctness = ShowCorrectness.NEVER - assert not ResultService(self._xblock, user_is_staff=has_staff_access).correctness_available() + assert not ProblemFeedbackService(self._xblock, user_is_staff=has_staff_access).correctness_available() @ddt.data( # Correctness not visible to learners if due date in the future @@ -243,5 +243,5 @@ def test_show_correctness_past_due(self, due_date_str, has_staff_access, expecte self._xblock.due = None else: self._xblock.due = getattr(self, due_date_str) - assert ResultService(self._xblock, user_is_staff=has_staff_access).correctness_available() ==\ + assert ProblemFeedbackService(self._xblock, user_is_staff=has_staff_access).correctness_available() ==\ expected_result From c35bfc61e58d92638cdfa0db0fd5e9889e91f6b9 Mon Sep 17 00:00:00 2001 From: Stephannie Jimenez Date: Thu, 30 Nov 2023 19:08:37 -0500 Subject: [PATCH 07/10] refactor: refactor capa block logic and move it to the new runtime service --- .../core/djangoapps/xblock/runtime/runtime.py | 2 +- xmodule/capa_block.py | 19 ++---- xmodule/services.py | 59 +++++++++++++++---- 3 files changed, 56 insertions(+), 24 deletions(-) diff --git a/openedx/core/djangoapps/xblock/runtime/runtime.py b/openedx/core/djangoapps/xblock/runtime/runtime.py index 02b729d491a9..11be282c3b2f 100644 --- a/openedx/core/djangoapps/xblock/runtime/runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/runtime.py @@ -300,7 +300,7 @@ def service(self, block: XBlock, service_name: str): elif service_name == 'publish': return EventPublishingService(self.user, context_key, make_track_function()) elif service_name == 'problem_feedback': - return ProblemFeedbackService(block=block, user_is_staff=self.user.is_staff) + return ProblemFeedbackService(block=block, user_is_staff=self.user.is_staff) # type: ignore # Check if the XBlockRuntimeSystem wants to handle this: service = self.system.get_service(block, service_name) diff --git a/xmodule/capa_block.py b/xmodule/capa_block.py index 7cf5a9ce6850..8c89717adb44 100644 --- a/xmodule/capa_block.py +++ b/xmodule/capa_block.py @@ -77,6 +77,7 @@ # Left for backwards compatibility SHOWANSWER = ShowAnswer + class RANDOMIZATION: """ Constants for problem randomization @@ -1341,25 +1342,19 @@ def hint_button(self, data): def used_all_attempts(self): """ All attempts have been used """ - return self.max_attempts is not None and self.attempts >= self.max_attempts + return self.runtime.service(self, 'problem_feedback').used_all_attempts() def is_past_due(self): """ Is it now past this problem's due date, including grace period? """ - return (self.close_date is not None and - datetime.datetime.now(utc) > self.close_date) + return self.runtime.service(self, 'problem_feedback').is_past_due() def closed(self): """ Is the student still allowed to submit answers? """ - if self.used_all_attempts(): - return True - if self.is_past_due(): - return True - - return False + return self.runtime.service(self, 'problem_feedback').closed() def is_submitted(self): """ @@ -1378,15 +1373,13 @@ def is_attempted(self): used by conditional block """ - return self.attempts > 0 + return self.runtime.service(self, 'problem_feedback').is_attempted() def is_correct(self): """ True iff full points """ - # self.score is initialized in self.lcp but in this method is accessed before self.lcp so just call it first. - self.lcp # pylint: disable=pointless-statement - return self.score.raw_earned == self.score.raw_possible + return self.runtime.service(self, 'problem_feedback').is_correct() def answer_available(self): """ diff --git a/xmodule/services.py b/xmodule/services.py index 65122034bd87..776a445351e5 100644 --- a/xmodule/services.py +++ b/xmodule/services.py @@ -330,6 +330,50 @@ def __init__(self, block=None, user_is_staff=False, **kwargs): # argument, but the `service` method from the `DescriptorSystem` passes a `block`. self._xblock = self.xblock() or block + def is_past_due(self): + """ + Returns if the problem is past its due date. + """ + if self._xblock: + return (self._xblock.close_date is not None and + datetime.datetime.now(UTC) > self._xblock.close_date) + return False + + def is_attempted(self): + """ + Has the problem been attempted? + """ + if self._xblock: + return self._xblock.attempts > 0 + return False + + def is_correct(self): + """ + True if full points + """ + if self._xblock: + # self._xblock.score is initialized in self._xblock.lcp but in this method is accessed before self._xblock.lcp so just call it first. + self._xblock.lcp # pylint: disable=pointless-statement + return self._xblock.score.raw_earned == self._xblock.score.raw_possible + return False + + def used_all_attempts(self): + """ All attempts have been used """ + if self._xblock: + return (self._xblock.max_attempts is not None and + self._xblock.attempts >= self._xblock.max_attempts) + return False + + def closed(self): + """ + Is the student still allowed to submit answers? + """ + if self.used_all_attempts(): + return True + if self.is_past_due(): + return True + return False + def answer_available(self): """ Returns whether correctness is available now, for the given attributes. @@ -337,25 +381,20 @@ def answer_available(self): if self._xblock: show_correctness = self._xblock.show_correctness show_answer = self._xblock.showanswer - past_due = self._xblock.is_past_due() max_attempts = self._xblock.max_attempts attempts = self._xblock.attempts - is_attempted = self._xblock.is_attempted() - is_correct = self._xblock.is_correct() required_attempts = self._xblock.attempts_before_showanswer_button - used_all_attempts = self._xblock.used_all_attempts() - closed = self._xblock.closed() else: show_correctness = '' show_answer = '' - past_due = False max_attempts = 0 attempts = 0 - is_attempted = False - is_correct = False required_attempts = 0 - used_all_attempts = False - closed = False + past_due = self.is_past_due() + is_attempted = self.is_attempted() + is_correct = self.is_correct() + used_all_attempts = self.used_all_attempts() + closed = self.closed() if not show_correctness: # If correctness is being withheld, then don't show answers either. From 5ff64ece8f1a00f8fbf6989371520d442a58012c Mon Sep 17 00:00:00 2001 From: Stephannie Jimenez Date: Thu, 30 Nov 2023 23:43:11 -0500 Subject: [PATCH 08/10] fix: fix tests and linting issues --- openedx/core/djangoapps/xblock/runtime/runtime.py | 2 +- xmodule/services.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/xblock/runtime/runtime.py b/openedx/core/djangoapps/xblock/runtime/runtime.py index 11be282c3b2f..2986c92bf5d9 100644 --- a/openedx/core/djangoapps/xblock/runtime/runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/runtime.py @@ -300,7 +300,7 @@ def service(self, block: XBlock, service_name: str): elif service_name == 'publish': return EventPublishingService(self.user, context_key, make_track_function()) elif service_name == 'problem_feedback': - return ProblemFeedbackService(block=block, user_is_staff=self.user.is_staff) # type: ignore + return ProblemFeedbackService(block=block, user_is_staff=self.user.is_staff) # type: ignore # Check if the XBlockRuntimeSystem wants to handle this: service = self.system.get_service(block, service_name) diff --git a/xmodule/services.py b/xmodule/services.py index 776a445351e5..d2045d495317 100644 --- a/xmodule/services.py +++ b/xmodule/services.py @@ -336,7 +336,7 @@ def is_past_due(self): """ if self._xblock: return (self._xblock.close_date is not None and - datetime.datetime.now(UTC) > self._xblock.close_date) + datetime.now(UTC) > self._xblock.close_date) return False def is_attempted(self): From acfecfbd14cb7f9a7ff09427586a163b0cd6c4f4 Mon Sep 17 00:00:00 2001 From: Stephannie Jimenez Date: Fri, 1 Dec 2023 00:04:41 -0500 Subject: [PATCH 09/10] fix: fix linting error --- xmodule/services.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/xmodule/services.py b/xmodule/services.py index d2045d495317..c2746b4c960a 100644 --- a/xmodule/services.py +++ b/xmodule/services.py @@ -352,7 +352,8 @@ def is_correct(self): True if full points """ if self._xblock: - # self._xblock.score is initialized in self._xblock.lcp but in this method is accessed before self._xblock.lcp so just call it first. + # self._xblock.score is initialized in self._xblock.lcp but in this + # method is accessed before self._xblock.lcp so just call it first. self._xblock.lcp # pylint: disable=pointless-statement return self._xblock.score.raw_earned == self._xblock.score.raw_possible return False From 02170db9ddd5c177dfb10330199a6a7fc0b5761c Mon Sep 17 00:00:00 2001 From: Stephannie Jimenez Date: Fri, 8 Dec 2023 19:21:19 -0500 Subject: [PATCH 10/10] refactor: undo test values and reverse is_correct functionality to xblock --- .../xblock_storage_handlers/view_handlers.py | 4 ++-- xmodule/capa_block.py | 4 +++- xmodule/services.py | 16 ++-------------- xmodule/tests/test_capa_block.py | 4 ++-- 4 files changed, 9 insertions(+), 19 deletions(-) diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index 06b464cfea95..51060228956e 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -58,10 +58,10 @@ from openedx.core.toggles import ENTRANCE_EXAMS from xmodule.services import ( ConfigurationService, + ProblemFeedbackService, SettingsService, TeamsConfigurationService, - ProblemFeedbackService -) # lint-amnesty, pylint: disable=wrong-import-order +) from xmodule.course_block import DEFAULT_START_DATE from xmodule.library_tools import LibraryToolsService from xmodule.modulestore import EdxJSONEncoder, ModuleStoreEnum diff --git a/xmodule/capa_block.py b/xmodule/capa_block.py index 8c89717adb44..2549b57aeba3 100644 --- a/xmodule/capa_block.py +++ b/xmodule/capa_block.py @@ -1379,7 +1379,9 @@ def is_correct(self): """ True iff full points """ - return self.runtime.service(self, 'problem_feedback').is_correct() + # self.score is initialized in self.lcp but in this method is accessed before self.lcp so just call it first. + self.lcp # pylint: disable=pointless-statement + return self.score.raw_earned == self.score.raw_possible def answer_available(self): """ diff --git a/xmodule/services.py b/xmodule/services.py index c2746b4c960a..3108d99567f3 100644 --- a/xmodule/services.py +++ b/xmodule/services.py @@ -347,17 +347,6 @@ def is_attempted(self): return self._xblock.attempts > 0 return False - def is_correct(self): - """ - True if full points - """ - if self._xblock: - # self._xblock.score is initialized in self._xblock.lcp but in this - # method is accessed before self._xblock.lcp so just call it first. - self._xblock.lcp # pylint: disable=pointless-statement - return self._xblock.score.raw_earned == self._xblock.score.raw_possible - return False - def used_all_attempts(self): """ All attempts have been used """ if self._xblock: @@ -384,6 +373,7 @@ def answer_available(self): show_answer = self._xblock.showanswer max_attempts = self._xblock.max_attempts attempts = self._xblock.attempts + is_correct = self._xblock.is_correct() required_attempts = self._xblock.attempts_before_showanswer_button else: show_correctness = '' @@ -391,9 +381,9 @@ def answer_available(self): max_attempts = 0 attempts = 0 required_attempts = 0 + is_correct = False past_due = self.is_past_due() is_attempted = self.is_attempted() - is_correct = self.is_correct() used_all_attempts = self.used_all_attempts() closed = self.closed() @@ -458,6 +448,4 @@ def correctness_available(self): # Is it now past the due date? return (due_date is None or due_date < datetime.now(UTC)) - - # else: show_correctness == ShowCorrectness.ALWAYS return True diff --git a/xmodule/tests/test_capa_block.py b/xmodule/tests/test_capa_block.py index ddc415ccf264..2df537718d56 100644 --- a/xmodule/tests/test_capa_block.py +++ b/xmodule/tests/test_capa_block.py @@ -278,7 +278,7 @@ def test_showanswer_attempted(self): ({'showanswer': 'attempted', 'max_attempts': '1', 'show_correctness': 'never', }, False, False), # If show_correctness=past_due, answer is not visible before due date ({'showanswer': 'attempted', 'show_correctness': 'past_due', 'max_attempts': '1', 'due': 'tomorrow_str', }, - False, True), + False, False), # If show_correctness=past_due, answer is visible after due date ({'showanswer': 'attempted', 'show_correctness': 'past_due', 'max_attempts': '1', 'due': 'yesterday_str', }, True, True)) @@ -611,7 +611,7 @@ def test_show_correctness_never(self): # Correctness not visible because grace period hasn't expired, # even after using up all attempts ({'show_correctness': 'past_due', 'max_attempts': '1', 'attempts': '1', 'due': 'yesterday_str', - 'graceperiod': 'two_day_delta_str', }, True)) + 'graceperiod': 'two_day_delta_str', }, False)) @ddt.unpack def test_show_correctness_past_due(self, problem_data, expected_result): """