issue_1671 the first attempt is to capture the sql error and display the error in grade distribution view UI#1674
issue_1671 the first attempt is to capture the sql error and display the error in grade distribution view UI#1674zqian wants to merge 2 commits intotl-its-umich-edu:masterfrom
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds error handling to the grade distribution view to catch and display SQL errors instead of silently failing. When a SQLAlchemy error occurs during grade distribution query execution, the error is now logged and returned to the UI with a user-friendly message.
Changes:
- Added SQLAlchemy error handling with try-except block around the grade distribution SQL query
- Added logging for both successful query parameters and SQL errors
- Returns JSON error response to UI when SQL errors occur
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 'enrollment_type': 'StudentEnrollment' | ||
| }) | ||
| except SQLAlchemyError as e: | ||
| grade_distribution_sql_error_msg = f'Error running grade distribution sql with course_id={course_id}, current_user={current_user} enrollment_type=StudentEnrollment' |
There was a problem hiding this comment.
Missing comma between '{current_user}' and 'enrollment_type'. Should be 'current_user={current_user}, enrollment_type=StudentEnrollment'.
| grade_distribution_sql_error_msg = f'Error running grade distribution sql with course_id={course_id}, current_user={current_user} enrollment_type=StudentEnrollment' | |
| grade_distribution_sql_error_msg = f'Error running grade distribution sql with course_id={course_id}, current_user={current_user}, enrollment_type=StudentEnrollment' |
| except SQLAlchemyError as e: | ||
| grade_distribution_sql_error_msg = f'Error running grade distribution sql with course_id={course_id}, current_user={current_user} enrollment_type=StudentEnrollment' | ||
| logger.error(f"{grade_distribution_sql_error_msg} sql={grade_score_sql} error={e}") | ||
| return HttpResponse(json.dumps({'gd_disable':'true','gd_msg': grade_distribution_sql_error_msg}), content_type='application/json') |
There was a problem hiding this comment.
Use JsonResponse instead of manually constructing JSON with HttpResponse and json.dumps. This provides better consistency with Django conventions and automatically sets the content type.
| except SQLAlchemyError as e: | ||
| grade_distribution_sql_error_msg = f'Error running grade distribution sql with course_id={course_id}, current_user={current_user} enrollment_type=StudentEnrollment' | ||
| logger.error(f"{grade_distribution_sql_error_msg} sql={grade_score_sql} error={e}") | ||
| return HttpResponse(json.dumps({'gd_disable':'true','gd_msg': grade_distribution_sql_error_msg}), content_type='application/json') |
There was a problem hiding this comment.
The boolean value 'true' is returned as a string. Consider using a boolean type (True) in the dictionary for consistency, as json.dumps will correctly serialize it to the JSON boolean 'true'.
Without change to the SQL code, the PR logs the sql error and also displays it in the grade distribution view UI.