feat: Add matrix checker page, exercises x users per course #351
Conversation
Sourcery Code Quality Report❌ Merging this PR will decrease code quality in the affected files by 0.30%.
Here are some functions in these files that still need a tune-up:
Legend and ExplanationThe emojis denote the absolute quality of the code:
The 👍 and 👎 indicate whether the quality has improved or gotten worse with this pull request. Please see our documentation here for details on how these metrics are calculated. We are actively working on this report - lots more documentation and extra metrics to come! Help us improve this quality report! |
|
This pull request introduces 4 alerts when merging fc54392 into 9499e89 - view on LGTM.com new alerts:
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #351 +/- ##
==========================================
+ Coverage 80.20% 80.48% +0.27%
==========================================
Files 63 65 +2
Lines 2996 3049 +53
==========================================
+ Hits 2403 2454 +51
- Misses 593 595 +2 ☔ View full report in Codecov by Sentry. |
![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!
General suggestions:
- Ensure the new feature integrates seamlessly with existing functionalities and user workflows.
- Review the performance implications of the new database queries, especially in scenarios with a large number of users or exercises.
- Consider the user experience and interface of the new matrix checker page, ensuring it is intuitive and accessible.
- Verify the new feature's compatibility across different environments, particularly in terms of database and browser support.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 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.
| def get_students(self, fields=None) -> List[int]: | ||
| fields = fields or [User.id] | ||
|
|
||
| return ( | ||
| self | ||
| .select(*fields) | ||
| .join(UserCourse) | ||
| .join(User, on=(User.id == UserCourse.user_id)) | ||
| .join(Role) | ||
| .where( | ||
| (self.id == UserCourse.course_id) | ||
| & (Role.id == Role.get_student_role().id), | ||
| ) | ||
| ) |
There was a problem hiding this comment.
suggestion (performance): Consider optimizing the query for get_students.
This method could potentially return a large number of records. Consider adding pagination or limiting the number of records returned.
| @@ -141,7 +141,7 @@ def _handle_failed_to_execute_tests(self, raw_results: bytes) -> None: | |||
| solution=self._solution, | |||
| test_name=models.ExerciseTestName.FATAL_TEST_NAME, | |||
| user_message=fail_user_message, | |||
| staff_message=_('Bro, did you check your code?'), | |||
| staff_message=_('Woah! Did you check your code?'), | |||
There was a problem hiding this comment.
suggestion (code_refinement): Consider the tone of user-facing messages.
While the change in message tone to be more informal might be intended to be friendly, ensure it aligns with the overall tone and user experience of the application.
| def test_we_get_users_x_exercises_results(cls, db_in_memory): | ||
| all_solutions = cls.course1.get_matrix(db_in_memory) | ||
| solution_count = len(cls.user_ids) * len(cls.exercise_ids) | ||
| assert len(all_solutions) == solution_count |
There was a problem hiding this comment.
suggestion (testing): Consider adding edge case tests for get_matrix method.
It's great that you're testing the get_matrix method under normal conditions. However, consider adding tests for edge cases, such as when there are no exercises or users in a course, or when there are exercises with no solutions. This will help ensure the robustness of the get_matrix method under various conditions.
tests/test_submissions_page.py
Outdated
| @@ -0,0 +1,51 @@ | |||
| from lms.lmsdb.models import Course | |||
There was a problem hiding this comment.
suggestion (testing): Missing tests for error conditions and unauthorized access.
The current tests cover the happy path for the submissions page. However, tests for error conditions (e.g., accessing a non-existent course) and unauthorized access (e.g., a user trying to access a course they are not enrolled in) are missing. Adding these tests would ensure the application behaves correctly under these conditions as well.
| from tests import conftest | ||
|
|
||
|
|
||
| class TestSubmissionsPage: |
There was a problem hiding this comment.
suggestion (testing): Consider adding tests for the submissions_table view function.
While the tests for the get_matrix method are valuable, it would also be beneficial to directly test the submissions_table view function. This could include verifying the HTTP response status code, the presence of expected context variables in the response, and the correct template usage. Such tests would help ensure the view integrates correctly with the underlying logic and templates.
Co authored w/ @NogaOs