From 3457d6dfdd0cc635ea8875eccbac60ce43aa4150 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Wed, 20 Sep 2023 12:52:13 -0400 Subject: [PATCH] refactor!: put implementation beside system feature flag --- openassessment/workflow/models.py | 173 +++++++++++++++++--------- openassessment/xblock/config_mixin.py | 4 +- openassessment/xblock/grade_mixin.py | 62 +++++---- 3 files changed, 155 insertions(+), 84 deletions(-) diff --git a/openassessment/workflow/models.py b/openassessment/workflow/models.py index db54ac76c4..8f806deaf2 100644 --- a/openassessment/workflow/models.py +++ b/openassessment/workflow/models.py @@ -283,22 +283,85 @@ def get_score(self, assessment_requirements, course_settings, step_for_name): else: step_requirements = assessment_requirements.get(assessment_step_name, {}) score = get_score_func(self.identifying_uuid, step_requirements, course_settings) - if score: - score_copy = score.copy() - score_copy.pop('points_earned', None) - score_copy.pop('points_possible', None) - accumulated_score.update(score_copy) - accumulated_score["points_earned"] += score['points_earned'] - accumulated_score["points_possible"] += score['points_possible'] + self.update_accumulated_score(score, accumulated_score) if not score and assessment_step.is_staff_step(): if step_requirements and step_requirements.get('required', False): break # A staff score was not found, and one is required. Return None continue # A staff score was not found, but it is not required, so try the next type of score - # break + if not self.is_accumulative_grading_enabled: + break if accumulated_score["points_earned"] and accumulated_score["points_possible"]: score = accumulated_score return score + @property + def is_accumulative_grading_enabled(self): + """ + Check if the accumulative grading is enabled for the system. + + Returns: + bool: True if accumulative grading is enabled, False otherwise. + """ + return settings.FEATURES.get("ENABLE_ACCUMULATIVE_GRADING", False) + + def update_accumulated_score(self, score, accumulated_score): + """ + Update the accumulated score with the given score. + + Args: + score (dict): A dict containing 'points_earned' and + 'points_possible'. + accumulated_score (dict): A dict containing 'points_earned' and + 'points_possible'. + + Returns: + dict: A dict containing 'points_earned' and 'points_possible'. + """ + if not score or not self.is_accumulative_grading_enabled: + return + score_copy = score.copy() + score_copy.pop('points_earned', None) + score_copy.pop('points_possible', None) + accumulated_score.update(score_copy) + accumulated_score["points_earned"] += score['points_earned'] + accumulated_score["points_possible"] += score['points_possible'] + + def set_new_staff_score(self, assessment_requirements, steps, step_for_name, course_settings, override_submitter_requirements): + """ + Set a new staff score for the workflow. + """ + new_staff_score = self.get_score( + assessment_requirements, + course_settings, + {self.STAFF_STEP_NAME: step_for_name.get(self.STAFF_STEP_NAME, None)} + ) + if new_staff_score: + # new_staff_score is just the most recent staff score, it may already be recorded in sub_api + old_score = sub_api.get_latest_score_for_submission(self.submission_uuid) + if ( + # Does a prior score exist? Is it a staff score? Do the points earned match? + not old_score or self.STAFF_ANNOTATION_TYPE not in [ + annotation['annotation_type'] for annotation in old_score['annotations'] + ] or old_score['points_earned'] != new_staff_score['points_earned'] + ): + # Set the staff score using submissions api, and log that fact + self.set_staff_score(new_staff_score) + self.save() + logger.info( + "Workflow for submission UUID %s has updated score using %s assessment.", + self.submission_uuid, + self.STAFF_STEP_NAME + ) + + # Update the assessment_completed_at field for all steps + # All steps are considered "assessment complete", as the staff score will override all + for step in steps: + common_now = now() + step.assessment_completed_at = common_now + if override_submitter_requirements: + step.submitter_completed_at = common_now + step.save() + def update_from_assessments( self, assessment_requirements, @@ -357,49 +420,31 @@ def update_from_assessments( step_for_name = {step.name: step for step in steps} - new_score = self.get_score( - assessment_requirements, - course_settings, - step_for_name, - ) - if new_score: - self.set_score(new_score) - # Update the assessment_completed_at field for all steps - # All steps are considered "assessment complete", as the staff score will override all - self.save() - for step in steps: - common_now = now() - step.assessment_completed_at = common_now - if override_submitter_requirements: - step.submitter_completed_at = common_now - step.save() - - # if new_staff_score: - # # new_staff_score is just the most recent staff score, it may already be recorded in sub_api - # old_score = sub_api.get_latest_score_for_submission(self.submission_uuid) - # if ( - # # Does a prior score exist? Is it a staff score? Do the points earned match? - # not old_score or self.STAFF_ANNOTATION_TYPE not in [ - # annotation['annotation_type'] for annotation in old_score['annotations'] - # ] or old_score['points_earned'] != new_staff_score['points_earned'] - # ): - # # Set the staff score using submissions api, and log that fact - # self.set_staff_score(new_staff_score) - # self.save() - # logger.info( - # "Workflow for submission UUID %s has updated score using %s assessment.", - # self.submission_uuid, - # self.STAFF_STEP_NAME - # ) - - # # Update the assessment_completed_at field for all steps - # # All steps are considered "assessment complete", as the staff score will override all - # for step in steps: - # common_now = now() - # step.assessment_completed_at = common_now - # if override_submitter_requirements: - # step.submitter_completed_at = common_now - # step.save() + if self.is_accumulative_grading_enabled: + new_score = self.get_score( + assessment_requirements, + course_settings, + step_for_name, + ) + if new_score: + self.set_score(new_score) + # Update the assessment_completed_at field for all steps + # All steps are considered "assessment complete", as the staff score will override all + self.save() + for step in steps: + common_now = now() + step.assessment_completed_at = common_now + if override_submitter_requirements: + step.submitter_completed_at = common_now + step.save() + else: + self.set_new_staff_score( + assessment_requirements, + steps, + step_for_name, + course_settings, + override_submitter_requirements, + ) if self.status == self.STATUS.done: return @@ -464,8 +509,11 @@ def update_from_assessments( # If we found a score, then we're done if score is not None: # Only set the score if it's not a staff score, in which case it will have already been set above - # if score.get("staff_id") is None: - self.set_score(score) + if self.is_accumulative_grading_enabled: + self.set_score(score) + else: + if score.get("staff_id") is None: + self.set_score(score) new_status = self.STATUS.done # Finally save our changes if the status has changed @@ -557,12 +605,19 @@ def set_score(self, score): 'points_possible'. """ - # if not self.staff_score_exists(): - sub_api.set_score( - self.submission_uuid, - score["points_earned"], - score["points_possible"] - ) + if self.is_accumulative_grading_enabled: + sub_api.set_score( + self.submission_uuid, + score["points_earned"], + score["points_possible"] + ) + else: + if not self.staff_score_exists(): + sub_api.set_score( + self.submission_uuid, + score["points_earned"], + score["points_possible"] + ) def staff_score_exists(self): """ diff --git a/openassessment/xblock/config_mixin.py b/openassessment/xblock/config_mixin.py index b36dd0d114..d8b64f6da6 100644 --- a/openassessment/xblock/config_mixin.py +++ b/openassessment/xblock/config_mixin.py @@ -11,6 +11,7 @@ ALL_FILES_URLS = 'all_files_urls' TEAM_SUBMISSIONS = 'team_submissions' +ACCUMULATIVE_GRADING = 'accumulative_grading' USER_STATE_UPLOAD_DATA = 'user_state_upload_data' RUBRIC_REUSE = 'rubric_reuse' ENHANCED_STAFF_GRADER = 'enhanced_staff_grader' @@ -18,6 +19,7 @@ FEATURE_TOGGLES_BY_FLAG_NAME = { ALL_FILES_URLS: 'ENABLE_ORA_ALL_FILE_URLS', TEAM_SUBMISSIONS: 'ENABLE_ORA_TEAM_SUBMISSIONS', + ACCUMULATIVE_GRADING: 'ENABLE_ACCUMULATIVE_GRADING', USER_STATE_UPLOAD_DATA: 'ENABLE_ORA_USER_STATE_UPLOAD_DATA', RUBRIC_REUSE: 'ENABLE_ORA_RUBRIC_REUSE', ENHANCED_STAFF_GRADER: 'ENABLE_ENHANCED_STAFF_GRADER' @@ -166,4 +168,4 @@ def is_accumulative_grading_enabled(self): # .. toggle_use_cases: circuit_breaker # .. toggle_creation_date: 2021-08-29 # .. toggle_tickets: -- - return settings.FEATURES.get('ENABLE_ACCUMULATIVE_GRADING', False) + return self.is_feature_enabled(ACCUMULATIVE_GRADING) diff --git a/openassessment/xblock/grade_mixin.py b/openassessment/xblock/grade_mixin.py index 81ec039b6d..f601d6dc68 100644 --- a/openassessment/xblock/grade_mixin.py +++ b/openassessment/xblock/grade_mixin.py @@ -300,14 +300,20 @@ def has_feedback(assessments): median_scores = {} assessment_steps = self.assessment_steps - if staff_assessment: - median_scores["staff"] = staff_api.get_assessment_scores_by_criteria(submission_uuid) - if "peer-assessment" in assessment_steps: - median_scores["peer"] = peer_api.get_assessment_median_scores(submission_uuid) - if "self-assessment" in assessment_steps: - median_scores["self"] = self_api.get_assessment_scores_by_criteria(submission_uuid) - - accumulated_criteria_scores = {} + if self.is_accumulative_grading_enabled: + if staff_assessment: + median_scores["staff"] = staff_api.get_assessment_scores_by_criteria(submission_uuid) + if "peer-assessment" in assessment_steps: + median_scores["peer"] = peer_api.get_assessment_median_scores(submission_uuid) + if "self-assessment" in assessment_steps: + median_scores["self"] = self_api.get_assessment_scores_by_criteria(submission_uuid) + else: + if staff_assessment: + median_scores = staff_api.get_assessment_scores_by_criteria(submission_uuid) + elif "peer-assessment" in assessment_steps: + median_scores = peer_api.get_assessment_median_scores(submission_uuid) + elif "self-assessment" in assessment_steps: + median_scores = self_api.get_assessment_scores_by_criteria(submission_uuid) for criterion in criteria: criterion_name = criterion['name'] @@ -332,13 +338,20 @@ def has_feedback(assessments): # if course authors directly import a course into Studio. # If this happens, we simply leave the score blank so that the grade # section can render without error. - criterion['median_score'] = sum( - median_scores.get(criterion_name, 0) for median_scores in median_scores.values() - ) - criterion['total_value'] = max_scores.get(criterion_name, '') + if self.is_accumulative_grading_enabled: + # Explanation: the median_score for a criteria, is the sum of all gradable + # steps. + criterion['median_score'] = sum( + median_scores.get(criterion_name, 0) for median_scores in median_scores.values() + ) + criterion['total_value'] = max_scores.get(criterion_name, '') + + criterion['accumulated_total_value'] = max_scores.get(criterion_name, 1) * len(assessment_steps) + criterion['accumulated_score'] = criterion['median_score'] + else: + criterion['median_score'] = median_scores.get(criterion_name, '') + criterion['total_value'] = max_scores.get(criterion_name, '') - criterion['accumulated_total_value'] = max_scores.get(criterion_name, 1) * len(assessment_steps) - criterion['accumulated_score'] = criterion['median_score'] return { 'criteria': criteria, 'additional_feedback': self._additional_feedback( @@ -408,16 +421,17 @@ def _get_assessment_part(title, feedback_title, part_criterion_name, assessment) if self_assessment_part: assessments.append(self_assessment_part) - # Include points only for the first assessment - # if assessments: - # first_assessment = assessments[0] - # option = first_assessment['option'] - # if option and option.get('points', None) is not None: - # first_assessment['points'] = option['points'] - - for assessment in assessments: - if assessment.get('option') and assessment['option'].get('points', None) is not None: - assessment['points'] = assessment['option']['points'] + if self.is_accumulative_grading_enabled: + for assessment in assessments: + if assessment.get('option') and assessment['option'].get('points', None) is not None: + assessment['points'] = assessment['option']['points'] + else: + # Include points only for the first assessment + if assessments: + first_assessment = assessments[0] + option = first_assessment['option'] + if option and option.get('points', None) is not None: + first_assessment['points'] = option['points'] return assessments