feat: Revamp the UX of exercise page #382
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @yammesicka - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟢 Complexity: all looks good
- 🟢 Docstrings: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| solution.mark_as_checked(staff_user) | ||
| solution = Solution.get_by_id(solution.id) | ||
| summary = Solution._get_summary(solution) | ||
| assert summary['is_checked'] |
There was a problem hiding this comment.
suggestion (testing): Consider adding more comprehensive assertions for test_get_solution_summary.
The test currently checks if the solution is marked as checked, but it could be enhanced by verifying other fields in the summary, such as comments_num, checker, and grade_color when applicable. This would ensure the summary method works as expected in various scenarios.
| @@ -686,3 +686,15 @@ def test_solutions_of_user( | |||
| exercises = solution.of_user(student_user.id, from_all_courses=True) | |||
| assert exercises[0].get('assessment') is None | |||
| assert exercises[1].get('assessment') == 'Try again' | |||
|
|
|||
| @staticmethod | |||
There was a problem hiding this comment.
suggestion (testing): Missing test cases for edge conditions in test_get_solution_summary.
It would be beneficial to add test cases covering scenarios where a solution has comments, an assigned checker, and an assessment. This ensures that the summary method accurately reflects all aspects of a solution's state.
| @@ -686,3 +686,15 @@ def test_solutions_of_user( | |||
| exercises = solution.of_user(student_user.id, from_all_courses=True) | |||
| assert exercises[0].get('assessment') is None | |||
| assert exercises[1].get('assessment') == 'Try again' | |||
|
|
|||
| @staticmethod | |||
There was a problem hiding this comment.
suggestion (testing): Add a test case for the new 'Invalid' assessment type.
Given the addition of a new 'Invalid' assessment type in the model changes, it's important to ensure that this assessment type is correctly handled in the solution summaries. A specific test case for this scenario would validate the integration.
| exercise = {} | ||
| exercise['solution_id'] = solution.id | ||
| exercise['is_checked'] = solution.is_checked | ||
| exercise['comments_num'] = len(solution.staff_comments) |
There was a problem hiding this comment.
suggestion (code-quality): Merge dictionary assignment with declaration [×3] (merge-dict-assign)
| exercise = {} | |
| exercise['solution_id'] = solution.id | |
| exercise['is_checked'] = solution.is_checked | |
| exercise['comments_num'] = len(solution.staff_comments) | |
| exercise = { | |
| 'solution_id': solution.id, | |
| 'is_checked': solution.is_checked, | |
| 'comments_num': len(solution.staff_comments), | |
| } |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #382 +/- ##
==========================================
+ Coverage 80.36% 80.43% +0.07%
==========================================
Files 65 65
Lines 3061 3067 +6
==========================================
+ Hits 2460 2467 +7
+ Misses 601 600 -1 ☔ View full report in Codecov by Sentry. |
No description provided.