From 78fa034531b04c438a9becd76b2e7b18834191d4 Mon Sep 17 00:00:00 2001 From: Nathan Sprenkle Date: Wed, 31 Jan 2024 12:08:20 -0500 Subject: [PATCH] fix: block submission and assessments when steps closed (#2168) * fix: block submitting or assessing on closed steps * test: add tests for blocking submit/assess when step closed * fix: block loading of page data for closed step with 400 error * refactor: more consistent step data accessor names * chore: bump ORA to 6.0.31 --- openassessment/__init__.py | 2 +- .../staffgrader/staff_grader_mixin.py | 2 +- openassessment/xblock/openassessmentblock.py | 4 +- .../xblock/ui_mixins/legacy/handlers_mixin.py | 4 +- openassessment/xblock/ui_mixins/mfe/mixin.py | 48 ++++++++++++++++++- .../ui_mixins/mfe/page_context_serializer.py | 2 +- .../xblock/ui_mixins/mfe/test_mfe_mixin.py | 22 +++++++++ package-lock.json | 4 +- package.json | 2 +- 9 files changed, 79 insertions(+), 11 deletions(-) diff --git a/openassessment/__init__.py b/openassessment/__init__.py index 94916d98f8..e9981371be 100644 --- a/openassessment/__init__.py +++ b/openassessment/__init__.py @@ -2,4 +2,4 @@ Initialization Information for Open Assessment Module """ -__version__ = '6.0.30' +__version__ = '6.0.31' diff --git a/openassessment/staffgrader/staff_grader_mixin.py b/openassessment/staffgrader/staff_grader_mixin.py index d413b0a4b0..f581914d28 100644 --- a/openassessment/staffgrader/staff_grader_mixin.py +++ b/openassessment/staffgrader/staff_grader_mixin.py @@ -459,7 +459,7 @@ def submit_staff_assessment(self, submission_uuid, data, suffix=''): # pylint: data['overall_feedback'], data.get('assess_type', 'regrade'), self.config_data, - self.staff_data, + self.staff_assessment_data, ) if self.is_team_assignment(): do_team_staff_assessment(*args, team_submission_uuid=submission_uuid) diff --git a/openassessment/xblock/openassessmentblock.py b/openassessment/xblock/openassessmentblock.py index 509ea4bbc7..f59bda2bbb 100644 --- a/openassessment/xblock/openassessmentblock.py +++ b/openassessment/xblock/openassessmentblock.py @@ -310,11 +310,11 @@ def peer_assessment_data(self, continue_grading=False): return PeerAssessmentAPI(self, continue_grading) @property - def self_data(self): + def self_assessment_data(self): return SelfAssessmentAPI(self) @property - def staff_data(self): + def staff_assessment_data(self): return StaffAssessmentAPI(self) @property diff --git a/openassessment/xblock/ui_mixins/legacy/handlers_mixin.py b/openassessment/xblock/ui_mixins/legacy/handlers_mixin.py index 56f067bc99..707fac942d 100644 --- a/openassessment/xblock/ui_mixins/legacy/handlers_mixin.py +++ b/openassessment/xblock/ui_mixins/legacy/handlers_mixin.py @@ -295,7 +295,7 @@ def failure_response(reason): data['overall_feedback'], self.config_data, self.workflow_data, - self.self_data, + self.self_assessment_data, ) except ReviewerMustHaveSubmittedException: return failure_response('You must submit a response before you can perform a self-assessment.') @@ -376,7 +376,7 @@ def failure_response(reason): data['overall_feedback'], data.get('assess_type', 'regrade'), self.config_data, - self.staff_data, + self.staff_assessment_data, ) except (StaffAssessmentRequestError, StaffAssessmentInternalError): return failure_response('Your team assessment could not be submitted.') diff --git a/openassessment/xblock/ui_mixins/mfe/mixin.py b/openassessment/xblock/ui_mixins/mfe/mixin.py index 374b686903..561b32da8f 100644 --- a/openassessment/xblock/ui_mixins/mfe/mixin.py +++ b/openassessment/xblock/ui_mixins/mfe/mixin.py @@ -94,6 +94,10 @@ def get_learner_data(self, data, suffix=""): # pylint: disable=unused-argument if not requested_step: return PageDataSerializer(self, context=serializer_context).data + # Raise error if step is closed + elif not self.is_step_open(requested_step): + raise OraApiException(400, error_codes.INACCESSIBLE_STEP, f"Inaccessible step: {requested_step}") + # Check to see if user can access this workflow step requested_workflow_step = MFE_STEP_TO_WORKFLOW_MAPPINGS[requested_step] if not self.workflow_data.has_reached_given_step( @@ -106,6 +110,39 @@ def get_learner_data(self, data, suffix=""): # pylint: disable=unused-argument serializer_context["requested_step"] = requested_step return PageDataSerializer(self, context=serializer_context).data + def is_step_open(self, step_name): + """ + Determine whether or not the requested step is open + + Return: If the problem is open or not (Bool) + Raises: OraApiException if the step name is invalid + """ + step_data = None + + # Users can always view a submission they've previously submitted + if step_name == "submission" and self.submission_data.has_submitted: + return True + # And whether they can get to grades, depends on the workflow being "done" + elif step_name == "done": + return self.workflow_data.is_done + + # Otherwise, get the info for the current step to determine access + if step_name == "submission": + step_data = self.submission_data + elif step_name == "studentTraining": + step_data = self.student_training_data + elif step_name == "peer": + step_data = self.peer_assessment_data() + elif step_name == "self": + step_data = self.self_assessment_data + elif step_name == "staff": + step_data = self.staff_assessment_data + else: + raise OraApiException(400, error_codes.UNKNOWN_SUFFIX, error_context=f"Bad step name: {step_name}") + + # Return if the step is currently open + return not step_data.problem_closed + def _submission_draft_handler(self, data): try: student_submission_data = data['response']['textResponses'] @@ -146,6 +183,9 @@ def submission(self, data, suffix=""): if suffix == handler_suffixes.SUBMISSION_DRAFT: return self._submission_draft_handler(data) elif suffix == handler_suffixes.SUBMISSION_SUBMIT: + # Return an error if the submission step is not open + if not self.is_step_open("submission"): + raise OraApiException(400, error_codes.INACCESSIBLE_STEP) return self._submission_create_handler(data) else: raise OraApiException(404, error_codes.UNKNOWN_SUFFIX) @@ -302,8 +342,14 @@ def _assessment_submit_handler(self, data): assessment_data = serializer.to_legacy_format(self) requested_step = serializer.data['step'] try: + # Block assessing a closed step + if not self.is_step_open(requested_step): + raise OraApiException(400, error_codes.INACCESSIBLE_STEP, f"Inaccessible step: {requested_step}") + + # Block assessing a cancelled submission if self.workflow_data.is_cancelled: raise InvalidStateToAssess() + if requested_step == 'peer': peer_assess( assessment_data['options_selected'], @@ -320,7 +366,7 @@ def _assessment_submit_handler(self, data): assessment_data['feedback'], self.config_data, self.workflow_data, - self.self_data + self.self_assessment_data ) elif requested_step == 'studentTraining': corrections = training_assess( diff --git a/openassessment/xblock/ui_mixins/mfe/page_context_serializer.py b/openassessment/xblock/ui_mixins/mfe/page_context_serializer.py index ce81b67632..b5907df8e6 100644 --- a/openassessment/xblock/ui_mixins/mfe/page_context_serializer.py +++ b/openassessment/xblock/ui_mixins/mfe/page_context_serializer.py @@ -208,7 +208,7 @@ class StepInfoSerializer(Serializer): submission = SubmissionStepInfoSerializer(source="submission_data") studentTraining = StudentTrainingStepInfoSerializer(source="student_training_data") peer = PeerStepInfoSerializer(source="peer_assessment_data") - _self = SelfStepInfoSerializer(source="self_data") + _self = SelfStepInfoSerializer(source="self_assessment_data") def get_fields(self): # Hack to name one of the output fields "self", a reserved word diff --git a/openassessment/xblock/ui_mixins/mfe/test_mfe_mixin.py b/openassessment/xblock/ui_mixins/mfe/test_mfe_mixin.py index 8d91f157ba..7ed74ef15c 100644 --- a/openassessment/xblock/ui_mixins/mfe/test_mfe_mixin.py +++ b/openassessment/xblock/ui_mixins/mfe/test_mfe_mixin.py @@ -681,6 +681,14 @@ def test_create_submission(self, xblock): assert resp.status_code == 200 assert_called_once_with_helper(mock_submit, self.DEFAULT_SUBMIT_VALUE["submission"]["textResponses"], 3) + @patch("openassessment.xblock.ui_mixins.mfe.mixin.MfeMixin.is_step_open") + @scenario("data/basic_scenario.xml") + def test_blocks_submit_when_step_closed(self, xblock, mock_is_step_open): + mock_is_step_open.return_value = False + with self._mock_create_submission(): + resp = self.request_create_submission(xblock) + assert_error_response(resp, 400, error_codes.INACCESSIBLE_STEP) + @ddt.ddt class FileUploadTest(MFEHandlersTestBase): @@ -1012,3 +1020,17 @@ def test_training_assess_corrections(self, xblock): resp = self.request_assessment_submit(xblock, step='studentTraining') assert_error_response(resp, 400, error_codes.TRAINING_ANSWER_INCORRECT, corrections) + + @patch("openassessment.xblock.ui_mixins.mfe.mixin.MfeMixin.is_step_open") + @ddt.data('self', 'studentTraining', 'peer') + @scenario("data/basic_scenario.xml") + def test_blocks_assess_when_step_closed(self, xblock, mfe_step, mock_is_step_open): + mock_is_step_open.return_value = False + with self.mock_assess_functions() as assess_mocks: + with self.mock_workflow_status(mfe_step): + resp = self.request_assessment_submit(xblock, step=mfe_step) + + assert_error_response(resp, 400, error_codes.INACCESSIBLE_STEP, f"Inaccessible step: {mfe_step}") + assess_mocks.self.assert_not_called() + assess_mocks.training.assert_not_called() + assess_mocks.peer.assert_not_called() diff --git a/package-lock.json b/package-lock.json index e84ea41c38..f5d617a0e8 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "edx-ora2", - "version": "6.0.30", + "version": "6.0.31", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "edx-ora2", - "version": "6.0.29", + "version": "6.0.31", "dependencies": { "@edx/frontend-build": "8.0.6", "@openedx/paragon": "^21.5.7", diff --git a/package.json b/package.json index 62a3390f9d..4124e4ea4c 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "edx-ora2", - "version": "6.0.30", + "version": "6.0.31", "repository": "https://github.com/openedx/edx-ora2.git", "dependencies": { "@edx/frontend-build": "8.0.6",