From 10b396152c1e7b29d75804445598191312545f5c Mon Sep 17 00:00:00 2001 From: KyryloKireiev Date: Tue, 1 Jul 2025 10:51:16 +0300 Subject: [PATCH] feat: [AXM-2398] Add short label to assignment Rationale: The instructor may create short labels that are longer than 3 characters, and they can be hard to work with in the mobile UI. Thus, on mobile, it was decided to add short labels to the api response by getting them from section breakdown, which ensures they are consistent with the labels the user sees in the Grading section. --- lms/djangoapps/courseware/courses.py | 6 +- .../grades/rest_api/v1/tests/test_views.py | 24 +++-- .../grades/tests/test_course_grade_factory.py | 7 +- .../mobile_api/course_info/views.py | 24 ++++- .../tests/test_course_info_views.py | 6 +- xmodule/graders.py | 9 +- xmodule/tests/test_graders.py | 99 +++++++++++++++++-- 7 files changed, 144 insertions(+), 31 deletions(-) diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index a8333dd52ed2..2c46248456f2 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -811,7 +811,9 @@ def get_assignments_grades(user, course_id, cache_timeout): course_id (CourseLocator): The course key. cache_timeout (int): Cache timeout in seconds Returns: - list (ReadSubsectionGrade, ZeroSubsectionGrade): The list with assignments grades. + tuple: + - list[Union[ReadSubsectionGrade, ZeroSubsectionGrade]]: List of subsection grades. + - list[dict]: List of dictionaries with section-level grade breakdown and assignment info. """ is_staff = bool(has_access(user, 'staff', course_id)) @@ -842,7 +844,7 @@ def get_assignments_grades(user, course_id, cache_timeout): log.warning(f'Could not get grades for the course: {course_id}, error: {err}') return [] - return subsection_grades + return subsection_grades, course_grade.grader_result()['section_breakdown'] def get_first_component_of_block(block_key, block_data): diff --git a/lms/djangoapps/grades/rest_api/v1/tests/test_views.py b/lms/djangoapps/grades/rest_api/v1/tests/test_views.py index 656e7e6b4396..b5a849c1ea05 100644 --- a/lms/djangoapps/grades/rest_api/v1/tests/test_views.py +++ b/lms/djangoapps/grades/rest_api/v1/tests/test_views.py @@ -279,7 +279,8 @@ def setUpClass(cls): { 'category': 'Homework', 'detail': f'Homework {i} Unreleased - 0% (?/?)', - 'label': f'HW {i:02d}', 'percent': .0 + 'label': f'HW {i:02d}', 'percent': .0, + 'sequential_id': None, } for i in range(1, 11) ] @@ -289,14 +290,16 @@ def setUpClass(cls): 'detail': 'Homework 11 Unreleased - 0% (?/?)', 'label': 'HW 11', 'mark': {'detail': 'The lowest 2 Homework scores are dropped.'}, - 'percent': 0.0 + 'percent': 0.0, + 'sequential_id': None, }, { 'category': 'Homework', 'detail': 'Homework 12 Unreleased - 0% (?/?)', 'label': 'HW 12', 'mark': {'detail': 'The lowest 2 Homework scores are dropped.'}, - 'percent': 0.0 + 'percent': 0.0, + 'sequential_id': None, } ] + [ @@ -311,7 +314,8 @@ def setUpClass(cls): { 'category': 'Lab', 'detail': f'Lab {i} Unreleased - 0% (?/?)', - 'label': f'Lab {i:02d}', 'percent': .0 + 'label': f'Lab {i:02d}', 'percent': .0, + 'sequential_id': None, } for i in range(1, 11) ] @@ -321,14 +325,16 @@ def setUpClass(cls): 'detail': 'Lab 11 Unreleased - 0% (?/?)', 'label': 'Lab 11', 'mark': {'detail': 'The lowest 2 Lab scores are dropped.'}, - 'percent': 0.0 + 'percent': 0.0, + 'sequential_id': None, }, { 'category': 'Lab', 'detail': 'Lab 12 Unreleased - 0% (?/?)', 'label': 'Lab 12', 'mark': {'detail': 'The lowest 2 Lab scores are dropped.'}, - 'percent': 0.0 + 'percent': 0.0, + 'sequential_id': None, }, { 'category': 'Lab', @@ -342,14 +348,16 @@ def setUpClass(cls): 'detail': 'Midterm Exam = 0.00%', 'label': 'Midterm', 'percent': 0.0, - 'prominent': True + 'prominent': True, + 'sequential_id': None, }, { 'category': 'Final Exam', 'detail': 'Final Exam = 0.00%', 'label': 'Final', 'percent': 0.0, - 'prominent': True + 'prominent': True, + 'sequential_id': None, } ] ) diff --git a/lms/djangoapps/grades/tests/test_course_grade_factory.py b/lms/djangoapps/grades/tests/test_course_grade_factory.py index 606c935e408f..d7d3a20c1ff0 100644 --- a/lms/djangoapps/grades/tests/test_course_grade_factory.py +++ b/lms/djangoapps/grades/tests/test_course_grade_factory.py @@ -162,6 +162,7 @@ def test_course_grade_summary(self): with mock_get_score(1, 2): self.subsection_grade_factory.update(self.course_structure[self.sequence.location]) course_grade = CourseGradeFactory().update(self.request.user, self.course) + subsection_grades = list(course_grade.subsection_grades.values()) actual_summary = course_grade.summary @@ -187,13 +188,15 @@ def test_course_grade_summary(self): 'category': 'Homework', 'detail': 'Homework 1 - Test Sequential X with an & Ampersand - 50.00% (1/2)', 'label': 'HW 01', - 'percent': 0.5 + 'percent': 0.5, + 'sequential_id': str(subsection_grades[0].location), }, { 'category': 'Homework', 'detail': 'Homework 2 - Test Sequential A - 0.00% (0/1)', 'label': 'HW 02', - 'percent': 0.0 + 'percent': 0.0, + 'sequential_id': str(subsection_grades[1].location), }, { 'category': 'Homework', diff --git a/lms/djangoapps/mobile_api/course_info/views.py b/lms/djangoapps/mobile_api/course_info/views.py index caa6a991dc46..fc818c646825 100644 --- a/lms/djangoapps/mobile_api/course_info/views.py +++ b/lms/djangoapps/mobile_api/course_info/views.py @@ -3,7 +3,7 @@ """ import logging -from typing import Dict, Optional, Union +from typing import Dict, List, Optional, Union import django from django.contrib.auth import get_user_model @@ -383,8 +383,8 @@ def list(self, request, **kwargs): # pylint: disable=W0221 response.data.update(course_data) return response - @staticmethod def _extend_sequential_info_with_assignment_progress( + self, requested_user: User, course_id: CourseKey, blocks_info_data: Dict[str, Dict], @@ -392,8 +392,11 @@ def _extend_sequential_info_with_assignment_progress( """ Extends sequential xblock info with assignment's name and progress. """ - subsection_grades = get_assignments_grades(requested_user, course_id, BLOCK_STRUCTURE_CACHE_TIMEOUT) + subsection_grades, section_breakdown = ( + get_assignments_grades(requested_user, course_id, BLOCK_STRUCTURE_CACHE_TIMEOUT) + ) grades_with_locations = {str(grade.location): grade for grade in subsection_grades} + id_to_label = self._id_to_label(section_breakdown) for block_id, block_info in blocks_info_data.items(): if block_info['type'] == 'sequential': @@ -403,8 +406,9 @@ def _extend_sequential_info_with_assignment_progress( points_earned = graded_total.earned if graded_total else 0 points_possible = graded_total.possible if graded_total else 0 assignment_type = grade.format + label = id_to_label.get(block_id) else: - points_earned, points_possible, assignment_type = 0, 0, None + points_earned, points_possible, assignment_type, label = 0, 0, None, None block_info.update( { @@ -412,10 +416,22 @@ def _extend_sequential_info_with_assignment_progress( 'assignment_type': assignment_type, 'num_points_earned': points_earned, 'num_points_possible': points_possible, + 'short_label': label, } } ) + @staticmethod + def _id_to_label(section_breakdown: List[Dict]) -> Dict[str, str]: + """ + Get `sequential_id` to assignment `label` mapping. + """ + return { + section['sequential_id']: section['label'] + for section in section_breakdown + if 'sequential_id' in section and section['sequential_id'] is not None + } + @mobile_view() class CourseEnrollmentDetailsView(APIView): diff --git a/lms/djangoapps/mobile_api/tests/test_course_info_views.py b/lms/djangoapps/mobile_api/tests/test_course_info_views.py index 56c020ec8fa3..efb3f7d9fdbb 100644 --- a/lms/djangoapps/mobile_api/tests/test_course_info_views.py +++ b/lms/djangoapps/mobile_api/tests/test_course_info_views.py @@ -409,12 +409,14 @@ def test_extend_sequential_info_with_assignment_progress_get_only_sequential(sel { 'assignment_type': 'Lecture Sequence', 'num_points_earned': 0.0, - 'num_points_possible': 0.0 + 'num_points_possible': 0.0, + 'short_label': None, }, { 'assignment_type': None, 'num_points_earned': 0.0, - 'num_points_possible': 0.0 + 'num_points_possible': 0.0, + 'short_label': None, }, ) diff --git a/xmodule/graders.py b/xmodule/graders.py index 5261b9f4479a..001113882438 100644 --- a/xmodule/graders.py +++ b/xmodule/graders.py @@ -380,11 +380,12 @@ def grade(self, grade_sheet, generate_random_scores=False): earned = random.randint(2, 15) possible = random.randint(earned, 15) section_name = _("Generated") - + sequential_id = None else: earned = scores[i].graded_total.earned possible = scores[i].graded_total.possible section_name = scores[i].display_name + sequential_id = str(scores[i].location) percentage = scores[i].percent_graded summary_format = "{section_type} {index} - {name} - {percent:.2%} ({earned:.3n}/{possible:.3n})" @@ -403,10 +404,11 @@ def grade(self, grade_sheet, generate_random_scores=False): index=i + self.starting_index, section_type=self.section_type ) + sequential_id = None short_label = labeler(i + self.starting_index) breakdown.append({'percent': percentage, 'label': short_label, - 'detail': summary, 'category': self.category}) + 'detail': summary, 'category': self.category, 'sequential_id': sequential_id}) total_percent, dropped_indices = self.total_with_drops(breakdown) @@ -427,7 +429,8 @@ def grade(self, grade_sheet, generate_random_scores=False): ) total_label = f"{self.short_label}" breakdown = [{'percent': total_percent, 'label': total_label, - 'detail': total_detail, 'category': self.category, 'prominent': True}, ] + 'detail': total_detail, 'category': self.category, 'prominent': True, + 'sequential_id': str(scores[0].location) if scores else None}, ] else: # Translators: "Homework Average = 0%" total_detail = _("{section_type} Average = {percent:.2%}").format( diff --git a/xmodule/tests/test_graders.py b/xmodule/tests/test_graders.py index 5073052b1ba8..5e2444a05533 100644 --- a/xmodule/tests/test_graders.py +++ b/xmodule/tests/test_graders.py @@ -70,9 +70,10 @@ class MockGrade: """ Mock class for SubsectionGrade object. """ - def __init__(self, graded_total, display_name): + def __init__(self, graded_total, location, display_name): self.graded_total = graded_total self.display_name = display_name + self.location = location @property def percent_graded(self): @@ -81,27 +82,64 @@ def percent_graded(self): common_fields = dict(graded=True, first_attempted=datetime.now()) test_gradesheet = { 'Homework': { - 'hw1': MockGrade(AggregatedScore(tw_earned=2, tw_possible=20.0, **common_fields), display_name='hw1'), - 'hw2': MockGrade(AggregatedScore(tw_earned=16, tw_possible=16.0, **common_fields), display_name='hw2'), + 'hw1': MockGrade( + AggregatedScore(tw_earned=2, tw_possible=20.0, **common_fields), + location='location_hw1_mock', + display_name='hw1' + ), + 'hw2': MockGrade( + AggregatedScore(tw_earned=16, tw_possible=16.0, **common_fields), + location='location_hw2_mock', + display_name='hw2' + ), }, # The dropped scores should be from the assignments that don't exist yet 'Lab': { # Dropped - 'lab1': MockGrade(AggregatedScore(tw_earned=1, tw_possible=2.0, **common_fields), display_name='lab1'), - 'lab2': MockGrade(AggregatedScore(tw_earned=1, tw_possible=1.0, **common_fields), display_name='lab2'), - 'lab3': MockGrade(AggregatedScore(tw_earned=1, tw_possible=1.0, **common_fields), display_name='lab3'), + 'lab1': MockGrade( + AggregatedScore(tw_earned=1, tw_possible=2.0, **common_fields), + location='location_lab1_mock', + display_name='lab1' + ), + 'lab2': MockGrade( + AggregatedScore(tw_earned=1, tw_possible=1.0, **common_fields), + location='location_lab2_mock', + display_name='lab2' + ), + 'lab3': MockGrade( + AggregatedScore(tw_earned=1, tw_possible=1.0, **common_fields), + location='location_lab3_mock', + display_name='lab3' + ), # Dropped - 'lab4': MockGrade(AggregatedScore(tw_earned=5, tw_possible=25.0, **common_fields), display_name='lab4'), + 'lab4': MockGrade( + AggregatedScore(tw_earned=5, tw_possible=25.0, **common_fields), + location='location_lab4_mock', + display_name='lab4' + ), # Dropped - 'lab5': MockGrade(AggregatedScore(tw_earned=3, tw_possible=4.0, **common_fields), display_name='lab5'), - 'lab6': MockGrade(AggregatedScore(tw_earned=6, tw_possible=7.0, **common_fields), display_name='lab6'), - 'lab7': MockGrade(AggregatedScore(tw_earned=5, tw_possible=6.0, **common_fields), display_name='lab7'), + 'lab5': MockGrade( + AggregatedScore(tw_earned=3, tw_possible=4.0, **common_fields), + location='location_lab5_mock', + display_name='lab5' + ), + 'lab6': MockGrade( + AggregatedScore(tw_earned=6, tw_possible=7.0, **common_fields), + location='location_lab6_mock', + display_name='lab6' + ), + 'lab7': MockGrade( + AggregatedScore(tw_earned=5, tw_possible=6.0, **common_fields), + location='location_lab7_mock', + display_name='lab7' + ), }, 'Midterm': { 'midterm': MockGrade( AggregatedScore(tw_earned=50.5, tw_possible=100, **common_fields), + location='location_midterm_mock', display_name="Midterm Exam", ), }, @@ -337,6 +375,47 @@ def test_grader_with_invalid_conf(self, invalid_conf, expected_error_message): graders.grader_from_conf([invalid_conf]) assert expected_error_message in str(error.value) + def test_sequential_location_in_section_breakdown(self): + homework_grader = graders.AssignmentFormatGrader("Homework", 12, 2) + lab_grader = graders.AssignmentFormatGrader("Lab", 7, 3) + midterm_grader = graders.AssignmentFormatGrader("Midterm", 1, 0) + + weighted_grader = graders.WeightedSubsectionsGrader([ + (homework_grader, homework_grader.category, 0.25), + (lab_grader, lab_grader.category, 0.25), + (midterm_grader, midterm_grader.category, 0.5), + ]) + + expected_sequential_ids = [ + 'location_hw1_mock', + 'location_hw2_mock', + None, + None, + None, + None, + None, + None, + None, + None, + None, + None, + None, + 'location_lab1_mock', + 'location_lab2_mock', + 'location_lab3_mock', + 'location_lab4_mock', + 'location_lab5_mock', + 'location_lab6_mock', + 'location_lab7_mock', + None, + 'location_midterm_mock', + ] + + graded = weighted_grader.grade(self.test_gradesheet) + + for i, section_breakdown in enumerate(graded['section_breakdown']): + assert expected_sequential_ids[i] == section_breakdown.get('sequential_id') + @ddt.ddt class ShowCorrectnessTest(unittest.TestCase):