From 12cc525dfad62743483a622b905ae48f8d69573b Mon Sep 17 00:00:00 2001 From: Nathan Sprenkle Date: Fri, 6 Oct 2023 10:12:40 -0400 Subject: [PATCH] feat: BFF Assessments Data (#2063) * refactor: move reused base serializer classes We use a few convenience serializers across our MFE. Currently CharListField, a list of strings, and IsRequiredField, a field converting from text-based "required" string to boolean. This moves them into a base utils file for reuse across our serializers. * feat: add assessment serializers * refactor: move assessment load to page context This lets us load the response for assessment only once and pass that context down to the assessment serializer. This also has the effect of simplifying the assessment serializer. * feat: allow jumping back to peer step --- .../ui_mixins/mfe/assessment_serializers.py | 116 ++++++++++ openassessment/xblock/ui_mixins/mfe/mixin.py | 8 + .../ui_mixins/mfe/ora_config_serializer.py | 17 +- .../ui_mixins/mfe/page_context_serializer.py | 52 ++++- .../xblock/ui_mixins/mfe/serializer_utils.py | 31 +++ .../mfe/test_assessment_serializers.py | 171 ++++++++++++++ .../mfe/test_page_context_serializer.py | 215 ++++++++++++++++++ 7 files changed, 593 insertions(+), 17 deletions(-) create mode 100644 openassessment/xblock/ui_mixins/mfe/serializer_utils.py create mode 100644 openassessment/xblock/ui_mixins/mfe/test_assessment_serializers.py create mode 100644 openassessment/xblock/ui_mixins/mfe/test_page_context_serializer.py diff --git a/openassessment/xblock/ui_mixins/mfe/assessment_serializers.py b/openassessment/xblock/ui_mixins/mfe/assessment_serializers.py index 20650f8374..e3053a78cf 100644 --- a/openassessment/xblock/ui_mixins/mfe/assessment_serializers.py +++ b/openassessment/xblock/ui_mixins/mfe/assessment_serializers.py @@ -3,8 +3,81 @@ """ # pylint: disable=abstract-method +from rest_framework.fields import ( + CharField, + IntegerField, + SerializerMethodField, + URLField, +) from rest_framework.serializers import Serializer +from openassessment.xblock.ui_mixins.mfe.serializer_utils import NullField + + +class SubmissionFileSerializer(Serializer): + fileUrl = URLField(source="file_key") + fileDescription = CharField(source="file_description") + fileName = CharField(source="file_name") + fileSize = IntegerField(source="file_size") + fileIndex = IntegerField(source="file_index") + + +class SubmittedResponseSerializer(Serializer): + """ + Data for a submitted response + + Returns: + { + textResponses: (Array [String]) + [ + (String) Matched with prompts + ], + uploaded_files: (Array [Object]) + [ + { + fileUrl: (URL) S3 location + fileDescription: (String) + fileName: (String) + fileSize: (Bytes?) + fileIndex: (Integer, positive) + } + ] + } + """ + + textResponses = SerializerMethodField() + uploadedFiles = SerializerMethodField() + + def get_textResponses(self, instance): + # An empty response has a different format from a saved response + # Return empty single text part if not yet saved. + answer_text_parts = instance["answer"].get("parts", []) + return [part["text"] for part in answer_text_parts] + + def get_uploadedFiles(self, instance): + # coerce to a similar shape for easier serialization + files = [] + + if not instance["answer"].get("file_keys"): + return None + + for i, file_key in enumerate(instance["answer"]["file_keys"]): + file_data = { + "file_key": file_key, + "file_description": instance["answer"]["files_descriptions"][i], + "file_name": instance["answer"]["files_names"][i], + "file_size": instance["answer"]["files_sizes"][i], + "file_index": i, + } + + # Don't serialize deleted / missing files + if not file_data["file_name"] and not file_data["file_description"]: + continue + + files.append(file_data) + + return [SubmissionFileSerializer(file).data for file in files] + class AssessmentResponseSerializer(Serializer): """ @@ -12,4 +85,47 @@ class AssessmentResponseSerializer(Serializer): gather the appropriate response and serialize. Data same shape as Submission, but coming from different sources. + + Returns: + { + // Null for Assessments + hasSubmitted: None + hasCancelled: None + hasReceivedGrade: None + teamInfo: None + + // The actual response to view + response: (Object) + { + textResponses: (Array [String]) + [ + (String) Matched with prompts + ], + uploadedFiles: (Array [Object]) + [ + { + fileUrl: (URL) S3 location + fileDescription: (String) + fileName: (String) + fileSize: (Bytes?) + fileIndex: (Integer, positive) + } + ] + } + } """ + + hasSubmitted = NullField(source="*") + hasCancelled = NullField(source="*") + hasReceivedGrade = NullField(source="*") + teamInfo = NullField(source="*") + + response = SerializerMethodField() + + def get_response(self, instance): # pylint: disable=unused-argument + # Response is passed in through context, so we don't have to fetch it + # in multiple locations. + response = self.context.get("response") + if not response: + return {} + return SubmittedResponseSerializer(response).data diff --git a/openassessment/xblock/ui_mixins/mfe/mixin.py b/openassessment/xblock/ui_mixins/mfe/mixin.py index 978cf19701..e8389c8e88 100644 --- a/openassessment/xblock/ui_mixins/mfe/mixin.py +++ b/openassessment/xblock/ui_mixins/mfe/mixin.py @@ -28,5 +28,13 @@ def get_block_learner_submission_data(self, data, suffix=""): # pylint: disable @XBlock.json_handler def get_block_learner_assessment_data(self, data, suffix=""): # pylint: disable=unused-argument serializer_context = {"view": "assessment"} + + # Allow jumping to a specific step, within our allowed steps + # NOTE should probably also verify this step is in our assessment steps + # though the serializer also covers for this currently + jumpable_steps = "peer" + if suffix in jumpable_steps: + serializer_context.update({"jump_to_step": suffix}) + page_context = PageDataSerializer(self, context=serializer_context) return page_context.data diff --git a/openassessment/xblock/ui_mixins/mfe/ora_config_serializer.py b/openassessment/xblock/ui_mixins/mfe/ora_config_serializer.py index 6ce4a6341e..c94df2757e 100644 --- a/openassessment/xblock/ui_mixins/mfe/ora_config_serializer.py +++ b/openassessment/xblock/ui_mixins/mfe/ora_config_serializer.py @@ -11,22 +11,13 @@ IntegerField, Serializer, CharField, - ListField, SerializerMethodField, ) - -class CharListField(ListField): - child = CharField() - - -class IsRequiredField(BooleanField): - """ - Utility for checking if a field is "required" to reduce repeated code. - """ - - def to_representation(self, value): - return value == "required" +from openassessment.xblock.ui_mixins.mfe.serializer_utils import ( + CharListField, + IsRequiredField, +) class TextResponseConfigSerializer(Serializer): diff --git a/openassessment/xblock/ui_mixins/mfe/page_context_serializer.py b/openassessment/xblock/ui_mixins/mfe/page_context_serializer.py index 370eb90acd..8bdee86acb 100644 --- a/openassessment/xblock/ui_mixins/mfe/page_context_serializer.py +++ b/openassessment/xblock/ui_mixins/mfe/page_context_serializer.py @@ -147,7 +147,7 @@ def to_representation(self, instance): return TrainingStepInfoSerializer(instance.student_training_data).data elif active_step == "peer": return PeerStepInfoSerializer(instance.peer_assessment_data()).data - elif active_step in ("submission", "done"): + elif active_step in ("submission", "waiting", "done"): return {} else: raise Exception(f"Bad step name: {active_step}") # pylint: disable=broad-exception-raised @@ -199,21 +199,65 @@ class PageDataSerializer(Serializer): def to_representation(self, instance): # Loading workflow status causes a workflow refresh # ... limit this to one refresh per page call - active_step = instance.workflow_data.status or "submission" + workflow_step = instance.workflow_data.status or "submission" - self.context.update({"step": active_step}) + self.context.update({"step": workflow_step}) return super().to_representation(instance) + def _can_jump_to_step(self, workflow_step, workflow_data, step_name): + """ + Helper to determine if a student can jump to a specific step: + 1) Student is on that step. + 2) Student has completed that step. + + NOTE that this should probably happen at the handler level, but for + added safety, check here as well. + """ + if step_name == workflow_step: + return True + step_status = workflow_data.status_details.get(step_name, {}) + return step_status.get("complete", False) + def get_submission(self, instance): """ Has the following different use-cases: 1) In the "submission" view, we get the user's draft / complete submission. 2) In the "assessment" view, we get an assessment for the current assessment step. """ + # pylint: disable=broad-exception-raised + # Submission Views if self.context.get("view") == "submission": return SubmissionSerializer(instance.submission_data).data + + # Assessment Views elif self.context.get("view") == "assessment": + # Can't view assessments without completing submission + if self.context["step"] == "submission": + raise Exception("Cannot view assessments without having completed submission.") + + # If the student is trying to jump to a step, verify they can + jump_to_step = self.context.get("jump_to_step") + workflow_step = self.context["step"] + if jump_to_step and not self._can_jump_to_step( + workflow_step, instance.workflow_data, jump_to_step + ): + raise Exception(f"Can't jump to {jump_to_step} step before completion") + + # Go to the current step, or jump to the selected step + active_step = jump_to_step or workflow_step + + if active_step == "training": + response = instance.student_training_data.example + elif active_step == "peer": + response = instance.peer_assessment_data().get_peer_submission() + elif active_step in ("staff", "waiting", "done"): + response = None + else: + raise Exception(f"Bad step name: {active_step}") + + self.context["response"] = response + return AssessmentResponseSerializer(instance.api_data, context=self.context).data else: - raise Exception("Missing view context for page") # pylint: disable=broad-exception-raised + raise Exception("Missing view context for page") diff --git a/openassessment/xblock/ui_mixins/mfe/serializer_utils.py b/openassessment/xblock/ui_mixins/mfe/serializer_utils.py new file mode 100644 index 0000000000..fd53aa2f07 --- /dev/null +++ b/openassessment/xblock/ui_mixins/mfe/serializer_utils.py @@ -0,0 +1,31 @@ +""" +Some custom serializer types and utils we use across our MFE +""" + +from rest_framework.fields import BooleanField, CharField, ListField + + +class CharListField(ListField): + """ + Shorthand for serializing a list of strings (CharFields) + """ + + child = CharField() + + +class IsRequiredField(BooleanField): + """ + Utility for checking if a field is "required" to reduce repeated code. + """ + + def to_representation(self, value): + return value == "required" + + +class NullField(CharField): + """ + A field which returns a Null/None value + """ + + def to_representation(self, value): + return None diff --git a/openassessment/xblock/ui_mixins/mfe/test_assessment_serializers.py b/openassessment/xblock/ui_mixins/mfe/test_assessment_serializers.py new file mode 100644 index 0000000000..79767ae278 --- /dev/null +++ b/openassessment/xblock/ui_mixins/mfe/test_assessment_serializers.py @@ -0,0 +1,171 @@ +""" +Tests for AssessmentResponseSerializer +""" +from unittest.mock import patch + +from openassessment.fileupload.api import FileUpload +from openassessment.xblock.test.base import ( + SubmissionTestMixin, + XBlockHandlerTestCase, + scenario, +) +from openassessment.xblock.ui_mixins.mfe.assessment_serializers import ( + AssessmentResponseSerializer, +) + + +class TestAssessmentResponseSerializer(XBlockHandlerTestCase, SubmissionTestMixin): + """ + Test for AssessmentResponseSerializer + """ + + # Show full dictionary diff + maxDiff = None + + @scenario("data/basic_scenario.xml", user_id="Alan") + def test_no_response(self, xblock): + # Given we are asking for assessment data too early (still on submission step) + context = {"response": None} + + # When I load my response + data = AssessmentResponseSerializer(xblock.api_data, context=context).data + + # I get the appropriate response + expected_response = {} + self.assertDictEqual(expected_response, data["response"]) + + # ... along with these always-none fields assessments + self.assertIsNone(data["hasSubmitted"]) + self.assertIsNone(data["hasCancelled"]) + self.assertIsNone(data["hasReceivedGrade"]) + self.assertIsNone(data["teamInfo"]) + + @scenario("data/basic_scenario.xml", user_id="Alan") + def test_response(self, xblock): + # Given we have a response + submission_text = ["Foo", "Bar"] + submission = self.create_test_submission( + xblock, submission_text=submission_text + ) + context = {"response": submission} + + # When I load my response + data = AssessmentResponseSerializer(xblock.api_data, context=context).data + + # I get the appropriate response + expected_response = { + "textResponses": submission_text, + "uploadedFiles": None, + } + self.assertDictEqual(expected_response, data["response"]) + + # ... along with these always-none fields assessments + self.assertIsNone(data["hasSubmitted"]) + self.assertIsNone(data["hasCancelled"]) + self.assertIsNone(data["hasReceivedGrade"]) + self.assertIsNone(data["teamInfo"]) + + @scenario("data/file_upload_scenario.xml", user_id="Alan") + def test_files_empty(self, xblock): + # Given we have a response + submission_text = ["Foo", "Bar"] + submission = self.create_test_submission( + xblock, submission_text=submission_text + ) + context = {"response": submission} + + # When I load my response + data = AssessmentResponseSerializer(xblock.api_data, context=context).data + + # I get the appropriate response + expected_response = { + "textResponses": submission_text, + "uploadedFiles": None, + } + self.assertDictEqual(expected_response, data["response"]) + + # ... along with these always-none fields assessments + self.assertIsNone(data["hasSubmitted"]) + self.assertIsNone(data["hasCancelled"]) + self.assertIsNone(data["hasReceivedGrade"]) + self.assertIsNone(data["teamInfo"]) + + def _mock_file(self, xblock, student_item_dict=None, **file_data): + """Turn mock file data into a FileUpload for testing""" + student_item_dict = ( + xblock.get_student_item_dict() + if not student_item_dict + else student_item_dict + ) + return FileUpload(**file_data, **student_item_dict) + + @patch( + "openassessment.xblock.apis.submissions.submissions_api.FileAPI.get_uploads_for_submission" + ) + @scenario("data/file_upload_scenario.xml", user_id="Alan") + def test_files(self, xblock, mock_get_files): + # Given we have a response + submission_text = ["Foo", "Bar"] + submission = None + + # .. with some uploaded files (and a deleted one) + mock_file_data = [ + { + "name": "foo", + "description": "bar", + "size": 1337, + }, + { + "name": None, + "description": None, + "size": None, + }, + { + "name": "baz", + "description": "buzz", + "size": 2049, + }, + ] + + mock_files = [] + for i, file in enumerate(mock_file_data): + file["index"] = i + mock_files.append(self._mock_file(xblock, **file)) + + mock_get_files.return_value = mock_files + submission = self.create_test_submission( + xblock, submission_text=submission_text + ) + + # When I load my response + context = {"response": submission} + data = AssessmentResponseSerializer(xblock.api_data, context=context).data + + # I get the appropriate response (test URLs use usage ID) + expected_url = f"Alan/edX/Enchantment_101/April_1/{xblock.scope_ids.usage_id}" + expected_response = { + "textResponses": submission_text, + "uploadedFiles": [ + { + "fileUrl": expected_url, + "fileDescription": "bar", + "fileName": "foo", + "fileSize": 1337, + "fileIndex": 0, + }, + { + "fileUrl": f"{expected_url}/2", + "fileDescription": "buzz", + "fileName": "baz", + "fileSize": 2049, + "fileIndex": 2, + }, + ], + } + self.assertDictEqual(expected_response, data["response"]) + + # ... along with these always-none fields assessments + self.assertIsNone(data["hasSubmitted"]) + self.assertIsNone(data["hasCancelled"]) + self.assertIsNone(data["hasReceivedGrade"]) + self.assertIsNone(data["teamInfo"]) diff --git a/openassessment/xblock/ui_mixins/mfe/test_page_context_serializer.py b/openassessment/xblock/ui_mixins/mfe/test_page_context_serializer.py new file mode 100644 index 0000000000..667072d796 --- /dev/null +++ b/openassessment/xblock/ui_mixins/mfe/test_page_context_serializer.py @@ -0,0 +1,215 @@ +""" +Tests for PageDataSerializer +""" +from copy import deepcopy + +from openassessment.xblock.test.base import ( + PEER_ASSESSMENTS, + SELF_ASSESSMENT, + SubmitAssessmentsMixin, + XBlockHandlerTestCase, + scenario, +) +from openassessment.xblock.ui_mixins.mfe.page_context_serializer import ( + PageDataSerializer, +) + + +class TestPageDataSerializerAssessment(XBlockHandlerTestCase, SubmitAssessmentsMixin): + """ + Test for PageDataSerializer: Assessment view + """ + + def setUp(self): + """For these tests, we are always in assessment view""" + self.context = {"view": "assessment"} + return super().setUp() + + @scenario("data/basic_scenario.xml", user_id="Alan") + def test_submission(self, xblock): + # Given we are asking for assessment data too early (still on submission step) + # When I load my response + # Then I get an Exception + with self.assertRaises(Exception): + _ = PageDataSerializer(xblock, context=self.context).data + + @scenario("data/student_training.xml", user_id="Alan") + def test_student_training(self, xblock): + # Given we are on the student training step + self.create_test_submission(xblock) + + # When I load my response + response_data = PageDataSerializer(xblock, context=self.context).data[ + "submission" + ] + + # I get the appropriate response + expected_response = { + "textResponses": ["This is my answer."], + "uploadedFiles": None, + } + self.assertDictEqual(expected_response, response_data["response"]) + + # ... along with these always-none fields assessments + self.assertIsNone(response_data["hasSubmitted"]) + self.assertIsNone(response_data["hasCancelled"]) + self.assertIsNone(response_data["hasReceivedGrade"]) + self.assertIsNone(response_data["teamInfo"]) + + @scenario("data/peer_only_scenario.xml", user_id="Alan") + def test_peer_response(self, xblock): + student_item = xblock.get_student_item_dict() + + # Given responses available for peer grading + other_student_item = deepcopy(student_item) + other_student_item["student_id"] = "Joan" + other_text_responses = ["Answer 1", "Answer 2"] + self.create_test_submission( + xblock, + student_item=other_student_item, + submission_text=other_text_responses, + ) + + # ... and that I have submitted and am on the peer grading step + student_item = xblock.get_student_item_dict() + text_responses = ["Answer A", "Answer B"] + self.create_test_submission( + xblock, student_item=student_item, submission_text=text_responses + ) + + # When I load my response + response_data = PageDataSerializer(xblock, context=self.context).data[ + "submission" + ] + + # I get the appropriate response + expected_response = { + "textResponses": other_text_responses, + "uploadedFiles": None, + } + self.assertDictEqual(expected_response, response_data["response"]) + + # ... along with these always-none fields assessments + self.assertIsNone(response_data["hasSubmitted"]) + self.assertIsNone(response_data["hasCancelled"]) + self.assertIsNone(response_data["hasReceivedGrade"]) + self.assertIsNone(response_data["teamInfo"]) + + @scenario("data/peer_only_scenario.xml", user_id="Alan") + def test_peer_response_not_available(self, xblock): + # Given I am on the peer grading step + self.create_test_submission(xblock) + + # ... but with no responses to assess + + # When I load my response + response_data = PageDataSerializer(xblock, context=self.context).data[ + "submission" + ] + + # I get the appropriate response + expected_response = {} + self.assertDictEqual(expected_response, response_data["response"]) + + # ... along with these always-none fields assessments + self.assertIsNone(response_data["hasSubmitted"]) + self.assertIsNone(response_data["hasCancelled"]) + self.assertIsNone(response_data["hasReceivedGrade"]) + self.assertIsNone(response_data["teamInfo"]) + + @scenario("data/staff_grade_scenario.xml", user_id="Alan") + def test_staff_response(self, xblock): + # Given I'm on the staff step + self.create_test_submission(xblock) + + # When I load my response + response_data = PageDataSerializer(xblock, context=self.context).data[ + "submission" + ] + + # Then I get an empty object + expected_response = {} + self.assertDictEqual(expected_response, response_data["response"]) + + @scenario("data/staff_grade_scenario.xml", user_id="Alan") + def test_waiting_response(self, xblock): + # Given I'm on the staff step + self.create_test_submission(xblock) + + # When I load my response + response_data = PageDataSerializer(xblock, context=self.context).data[ + "submission" + ] + + # Then I get an empty object + expected_response = {} + self.assertDictEqual(expected_response, response_data["response"]) + + @scenario("data/self_assessment_scenario.xml", user_id="Alan") + def test_done_response(self, xblock): + # Given I'm on the done step + self.create_submission_and_assessments( + xblock, self.SUBMISSION, [], [], SELF_ASSESSMENT + ) + # When I load my response + response_data = PageDataSerializer(xblock, context=self.context).data[ + "submission" + ] + + # Then I get an empty object + expected_response = {} + self.assertDictEqual(expected_response, response_data["response"]) + + @scenario("data/grade_scenario_peer_only.xml", user_id="Bernard") + def test_jump_to_peer_response(self, xblock): + student_item = xblock.get_student_item_dict() + + # Given responses available for peer grading + other_student_item = deepcopy(student_item) + other_student_item["student_id"] = "Joan" + other_text_responses = ["Answer 1", "Answer 2"] + self.create_test_submission( + xblock, + student_item=other_student_item, + submission_text=other_text_responses, + ) + + # ... and I have completed the peer step of an ORA + self.create_submission_and_assessments(xblock, self.SUBMISSION, self.PEERS, PEER_ASSESSMENTS, None) + + # When I try to jump back to that step + self.context["jump_to_step"] = "peer" + response_data = PageDataSerializer(xblock, context=self.context).data["submission"] + + # Then I can continue to receive peer responses to grade + expected_response = { + "textResponses": other_text_responses, + "uploadedFiles": None, + } + self.assertDictEqual(expected_response, response_data["response"]) + + @scenario("data/grade_scenario_peer_only.xml", user_id="Bernard") + def test_jump_to_bad_step(self, xblock): + # Given I'm on assessment steps + self.create_test_submission(xblock) + + # When I try to jump to a bad step + self.context["jump_to_step"] = "to the left" + + # Then I expect the serializer to raise an exception + # NOTE - this is exceedingly unlikely since the handler should only add + # this context when the step name is valid. + with self.assertRaises(Exception): + _ = PageDataSerializer(xblock, context=self.context).data + + @scenario("data/student_training.xml", user_id="Bernard") + def test_jump_to_inaccessible_step(self, xblock): + # Given I'm on an early step like student training + self.create_test_submission(xblock) + + # When I try to jump ahead to a step I can't yet access + self.context["jump_to_step"] = "peer" + + # Then I expect the serializer to raise an exception + with self.assertRaises(Exception): + _ = PageDataSerializer(xblock, context=self.context).data