Fix filtering on new saved annotation page #5838
Conversation
|
Warning Rate limit exceeded@jorg-vr has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 53 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces changes to the Changes
Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
app/policies/annotation_policy.rb (1)
7-7: Approved: Efficient query addressing the pluralization issue.The changes effectively resolve the filtering issue mentioned in the PR objectives. By using
scope.released.where(submission: { user: user }), you've eliminated the need for an explicit join and consistently used the singularsubmission, addressing the pluralization problem.The addition of
.or(scope.where(submission: { course_id: user.administrating_courses.map(&:id) }))ensures that users can access annotations from courses they administer, which is a good improvement in functionality.Consider caching the result of
user.administrating_courses.map(&:id)if it's used frequently, as it could potentially be an expensive operation for users administering many courses:admin_course_ids = user.administrating_courses.pluck(:id) scope.released.where(submission: { user: user }).or(scope.where(submission: { course_id: admin_course_ids }))This change would reduce the number of database queries if the method is called multiple times.
app/controllers/saved_annotations_controller.rb (3)
45-46: Approved: Good fix for the pluralization issue.The changes effectively address the pluralization issue mentioned in the PR objectives. Using an explicit INNER JOIN with the singular table name 'submission' ensures consistency between the join and where clauses, resolving the filtering problem.
For improved readability, consider extracting the join condition into a separate method or constant:
SUBMISSION_JOIN = 'INNER JOIN submissions AS submission ON submission.id = annotations.submission_id' @courses = Course.where(id: @annotations.joins(SUBMISSION_JOIN).pluck('submission.course_id').uniq) @exercises = Activity.where(id: @annotations.joins(SUBMISSION_JOIN).pluck('submission.exercise_id').uniq)This would make the queries more concise and easier to maintain.
Line range hint
1-108: Summary: Effective fix with room for further optimizationThe changes in this PR successfully address the filtering issue on the new saved annotation page by consistently using singular table names in joins and where clauses. The addition of the
format.jsresponse in theshowaction enhances the controller's flexibility.Given that you're open to suggestions for further improvements, consider the following:
- Implement the suggested refactoring for the join condition in the
newaction to improve readability.- Review the query performance, especially for the
@annotations,@courses, and@exercisesassignments in thenewaction. Consider using database-level joins and selects to optimize these queries if they become performance bottlenecks.- Ensure comprehensive test coverage for both the filtering fix and the new AJAX functionality.
- Consider implementing eager loading (e.g.,
includesorpreload) for associated models if they're frequently accessed in views to reduce N+1 query issues.If you're looking for more substantial improvements:
- Evaluate if some of the query logic can be moved to the model layer to keep the controller lighter.
- Consider implementing a service object for complex annotation filtering if the logic continues to grow in complexity.
- Explore using ActiveRecord's
mergemethod for combining scopes, which might provide a cleaner way to handle complex queries.These suggestions aim to further enhance the code's maintainability and performance. Let me know if you'd like more detailed advice on any of these points.
Missing Integration Tests for AJAX Functionality
No existing integration tests were found for the
format.jsresponse in theshowaction ofSavedAnnotationsController. It's recommended to add tests to ensure the new AJAX functionality behaves as expected.🔗 Analysis chain
Line range hint
28-32: Approved: Enhanced response handling with format.jsThe addition of the
format.jsblock in theshowaction improves the controller's capability to handle AJAX requests, allowing for more dynamic interactions on the client side.To ensure the new AJAX functionality works as expected:
- Verify that there's a corresponding JavaScript view (
show.js.erbor similar) to handle the AJAX response.- Test the AJAX functionality in the browser to confirm it behaves correctly.
- Consider adding integration tests to cover this new AJAX path.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for the existence of a JavaScript view for the show action fd -e js.erb -e js show app/views/saved_annotationsLength of output: 93
Script:
#!/bin/bash # Search for integration tests for the show action handling format.js in saved_annotations_controller rg 'describe .*SavedAnnotationsController' tests/ rg 'describe "GET #show" .*format: :js' tests/Length of output: 194
Script:
#!/bin/bash # Search for tests related to SavedAnnotationsController's show action handling format.js rg 'describe .*SavedAnnotationsController' . rg 'describe "GET #show" .*format: :js' .Length of output: 90
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- app/controllers/saved_annotations_controller.rb (1 hunks)
- app/policies/annotation_policy.rb (1 hunks)
- test/controllers/saved_annotation_controller_test.rb (1 hunks)
🧰 Additional context used
📓 Learnings (3)
📓 Common learnings
Learnt from: jorg-vr PR: dodona-edu/dodona#5648 File: app/controllers/saved_annotations_controller.rb:88-89 Timestamp: 2024-07-29T12:25:56.607Z Learning: When adding new instance variables in controller methods, ensure to update the test suite to cover these variables.app/controllers/saved_annotations_controller.rb (1)
Learnt from: jorg-vr PR: dodona-edu/dodona#5648 File: app/controllers/saved_annotations_controller.rb:88-89 Timestamp: 2024-07-29T12:25:56.607Z Learning: When adding new instance variables in controller methods, ensure to update the test suite to cover these variables.test/controllers/saved_annotation_controller_test.rb (1)
Learnt from: jorg-vr PR: dodona-edu/dodona#5648 File: app/controllers/saved_annotations_controller.rb:88-89 Timestamp: 2024-07-29T12:25:56.607Z Learning: When adding new instance variables in controller methods, ensure to update the test suite to cover these variables.
🔇 Additional comments (3)
app/policies/annotation_policy.rb (2)
Line range hint
26-30: Verify consistency ofrecord.courseusageWhile the
resolvemethod now usessubmissionconsistently, other methods likeshow?andanonymous?still userecord.course. To maintain consistency and avoid potential issues:
- Verify that
record.courseis equivalent torecord.submission.coursein all cases.- Consider updating these methods to use
record.submission.courseif appropriate, to align with the changes in theresolvemethod.Run the following script to check the usage of
record.courseandrecord.submission.course:This will help ensure consistency across the policy methods.
Also applies to: 57-61
✅ Verification successful
Consistency of
record.courseUsage VerifiedAll policy methods consistently use
record.submission.course. No instances ofrecord.coursewere found inapp/policies/annotation_policy.rbor related policy files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of record.course and record.submission.course # Test: Search for record.course usage echo "Occurrences of record.course:" rg --type ruby 'record\.course' app/policies/ # Test: Search for record.submission.course usage echo "Occurrences of record.submission.course:" rg --type ruby 'record\.submission\.course' app/policies/Length of output: 788
Line range hint
1-61: Reminder: Ensure test coverage for policy changesBased on a previous learning note, it's important to update the test suite when making changes to controller methods or policy logic. While this change is in a policy file rather than a controller, the principle still applies.
To ensure adequate test coverage:
- Verify that existing tests cover the new
resolvemethod logic.- Add new tests if necessary to cover different scenarios (e.g., regular users, admins, users with both their own submissions and administered courses).
Run the following script to check for related test files and their content:
This will help identify existing tests and areas where coverage might need to be improved.
test/controllers/saved_annotation_controller_test.rb (1)
Line range hint
1-72: Enhance test coverage for SQL query issueWhile the new tests cover the filtering functionality, they don't directly address the complex pluralization issue with SQL queries mentioned in the PR objectives. To ensure comprehensive coverage, consider adding tests that specifically target this issue.
- Add a test that verifies the correct SQL query is generated when both
joinsandwhereclauses are present. This could be done by usingto_sqlon the query and asserting the correct table names are used.- Include edge cases that might have been affected by the pluralization issue, such as queries with multiple joins or complex where conditions.
Here's a suggested additional test:
test 'should generate correct SQL query with singular table name' do exercise = create(:exercise) annotation = create(:annotation, submission: create(:submission, exercise: exercise)) query = SavedAnnotation.joins(:submission).where(submission: { exercise_id: exercise.id }) assert_includes query.to_sql, "INNER JOIN \"submission\"", "Should use singular 'submission' in JOIN clause" assert_includes query.to_sql, "WHERE \"submission\".\"exercise_id\" = #{exercise.id}", "Should use singular 'submission' in WHERE clause" endAdditionally, ensure that the controller action being tested (
newaction in this case) is actually using the fixed query. If it's not, consider adding or updating a test for that action to verify it's using the correct query structure.To confirm that the controller is using the correct query, we can run the following script:
This will help us verify that the controller is indeed using the singular table name in both the
joinsandwhereclauses.
| test 'staff should be able to filter existing annotations on new page' do | ||
| get new_saved_annotation_path | ||
|
|
||
| assert_response :success | ||
|
|
||
| get new_saved_annotation_path, params: { exercise_id: 1 } | ||
|
|
||
| assert_response :success | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for staff filtering
The test correctly checks if staff can access the new saved annotation page with and without filtering. However, it could be improved to verify the actual filtering functionality.
Consider the following improvements:
- Add setup code to create test data (e.g., saved annotations with different exercise_ids).
- Verify the content of the response, not just the status code.
- Assert that the filtered results only contain annotations for the specified exercise_id.
Here's a suggested improvement:
test 'staff should be able to filter existing annotations on new page' do
exercise1 = create(:exercise)
exercise2 = create(:exercise)
annotation1 = create(:saved_annotation, user: @user, course: @course, exercise: exercise1)
annotation2 = create(:saved_annotation, user: @user, course: @course, exercise: exercise2)
get new_saved_annotation_path
assert_response :success
assert_select 'your-selector-for-annotation', 2
get new_saved_annotation_path, params: { exercise_id: exercise1.id }
assert_response :success
assert_select 'your-selector-for-annotation', 1
assert_select 'your-selector-for-annotation-title', annotation1.title
endReplace 'your-selector-for-annotation' with the actual CSS selector used in your view to represent annotations.
| test 'zeus should be able to filter existing annotations on new page' do | ||
| sign_in users(:zeus) | ||
| get new_saved_annotation_path | ||
|
|
||
| assert_response :success | ||
|
|
||
| get new_saved_annotation_path, params: { exercise_id: 1 } | ||
|
|
||
| assert_response :success | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor zeus test and enhance coverage
The zeus test is very similar to the staff test, which suggests an opportunity for refactoring to reduce code duplication. Additionally, it has the same limitations in terms of verifying the actual filtering functionality.
Consider the following improvements:
- Refactor the common testing logic into a shared method.
- Enhance the test to verify the content of the response, similar to the suggestions for the staff test.
- Ensure that zeus users can only see their own annotations in the filtered results.
Here's a suggested refactoring and improvement:
def assert_can_filter_annotations(user)
sign_in user
exercise1 = create(:exercise)
exercise2 = create(:exercise)
annotation1 = create(:saved_annotation, user: user, course: @course, exercise: exercise1)
annotation2 = create(:saved_annotation, user: user, course: @course, exercise: exercise2)
get new_saved_annotation_path
assert_response :success
assert_select 'your-selector-for-annotation', 2
get new_saved_annotation_path, params: { exercise_id: exercise1.id }
assert_response :success
assert_select 'your-selector-for-annotation', 1
assert_select 'your-selector-for-annotation-title', annotation1.title
end
test 'staff should be able to filter existing annotations on new page' do
assert_can_filter_annotations(@user)
end
test 'zeus should be able to filter existing annotations on new page' do
assert_can_filter_annotations(users(:zeus))
endThis refactoring reduces duplication and ensures consistent testing across user types. Adjust the selectors and assertions as needed to match your actual view structure.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
app/policies/annotation_policy.rb (1)
6-6: Redundant.allcall can be omittedCalling
.allon a relation is redundant sinceannotationsis already anActiveRecord::Relation. Omitting.allsimplifies the code without changing its functionality.Apply this diff to remove the redundancy:
-annotations.all +annotations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- app/controllers/saved_annotations_controller.rb (1 hunks)
- app/policies/annotation_policy.rb (1 hunks)
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
Learnt from: jorg-vr PR: dodona-edu/dodona#5648 File: app/controllers/saved_annotations_controller.rb:88-89 Timestamp: 2024-07-29T12:25:56.607Z Learning: When adding new instance variables in controller methods, ensure to update the test suite to cover these variables.app/controllers/saved_annotations_controller.rb (1)
Learnt from: jorg-vr PR: dodona-edu/dodona#5648 File: app/controllers/saved_annotations_controller.rb:88-89 Timestamp: 2024-07-29T12:25:56.607Z Learning: When adding new instance variables in controller methods, ensure to update the test suite to cover these variables.
🔇 Additional comments (3)
app/controllers/saved_annotations_controller.rb (2)
Line range hint
1-114: Query: Where are the added tests located?The PR objectives mention that tests were added to verify the functionality of the changes. However, these tests are not visible in the current file. Could you please provide the location of the added tests? This will help ensure that the changes are properly covered by the test suite.
To locate the relevant test files, you can run:
If you need any assistance in writing additional tests or improving the existing ones, please let me know.
45-46: Approved: Good fix for the filtering issue. Consider edge cases and alternatives.The changes effectively address the table name inconsistency issue mentioned in the PR objectives. Using
pluckdirectly onsubmission.course_idandsubmission.exercise_idavoids the need for an explicit join and consistently uses the singular table name, resolving the pluralization problem.To ensure the changes don't introduce any unintended side effects, please verify the behavior in edge cases, such as:
- When there are no annotations
- When all annotations have the same course or exercise
You mentioned not being entirely satisfied with this fix. Are there any specific concerns you have about the current implementation? I'd be happy to help brainstorm alternative solutions if you have any ideas in mind.
app/policies/annotation_policy.rb (1)
10-10: Verify thatannotations.nonereturns the correct empty relationEnsure that
annotations.nonebehaves as expected in this context. Whilenoneshould return an empty relation, it's good to confirm that no unintended side effects occur.You can test this by checking the result of
annotations.none:annotations.none.to_a # Should return an empty array
app/policies/annotation_policy.rb
Outdated
| @@ -1,13 +1,13 @@ | |||
| class AnnotationPolicy < ApplicationPolicy | |||
| class Scope < ApplicationPolicy::Scope | |||
| def resolve | |||
| annotations = scope.joins('INNER JOIN submissions AS submission ON submission.id = annotations.submission_id') | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor join to use ActiveRecord methods instead of raw SQL
Using raw SQL in joins reduces readability and may introduce SQL injection risks. Consider utilizing ActiveRecord's query interface to define the join with a table alias.
You can refactor the join as follows:
-annotations = scope.joins('INNER JOIN submissions AS submission ON submission.id = annotations.submission_id')
+annotations = scope.joins(:submission)Since Annotation belongs to Submission, you can use the association to perform the join:
annotations = scope.joins(:submission)If you need to reference the table with an alias due to pluralization issues, you can use joins with a string that's safer by leveraging sanitize_sql:
annotations = scope.joins(ActiveRecord::Base.sanitize_sql(['INNER JOIN submissions submission ON submission.id = annotations.submission_id']))However, it's often better to adjust the query to avoid raw SQL altogether.
app/policies/annotation_policy.rb
Outdated
| annotations = scope.joins('INNER JOIN submissions AS submission ON submission.id = annotations.submission_id') | ||
| if user&.zeus? | ||
| scope.all | ||
| annotations.all | ||
| elsif user | ||
| common = scope.joins(:submission) | ||
| common.released.where(submissions: { user: user }).or(common.where(submissions: { course_id: user.administrating_courses.map(&:id) })) | ||
| annotations.released.where(submission: { user: user }).or(annotations.where(submission: { course_id: user.administrating_courses.map(&:id) })) | ||
| else | ||
| scope.none | ||
| annotations.none |
There was a problem hiding this comment.
Ensure test coverage for the updated resolve method
With changes to the resolve method, it's important to update the test suite to cover the new logic paths, especially since we've introduced a new variable and modified the query structure.
This aligns with previous learnings about updating tests when introducing new variables. Would you like assistance in writing additional tests to ensure comprehensive coverage?
app/policies/annotation_policy.rb
Outdated
| elsif user | ||
| common = scope.joins(:submission) | ||
| common.released.where(submissions: { user: user }).or(common.where(submissions: { course_id: user.administrating_courses.map(&:id) })) | ||
| annotations.released.where(submission: { user: user }).or(annotations.where(submission: { course_id: user.administrating_courses.map(&:id) })) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify complex where conditions for better readability
The where clause combines multiple conditions in a way that can be difficult to read and maintain. Breaking it into smaller scopes or using predicate methods can enhance clarity.
Consider refactoring as follows:
-annotations.released.where(submission: { user: user }).or(annotations.where(submission: { course_id: user.administrating_courses.map(&:id) }))
+user_annotations = annotations.released.where(submission: { user: user })
+admin_annotations = annotations.where(submission: { course_id: user.administrating_courses.ids })
+user_annotations.or(admin_annotations)This separates the conditions and makes the logic more transparent.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| annotations.released.where(submission: { user: user }).or(annotations.where(submission: { course_id: user.administrating_courses.map(&:id) })) | |
| user_annotations = annotations.released.where(submission: { user: user }) | |
| admin_annotations = annotations.where(submission: { course_id: user.administrating_courses.ids }) | |
| user_annotations.or(admin_annotations) |
|
try to avoid pluck and no explicit sql |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/controllers/saved_annotations_controller.rb (1)
Line range hint
1-108: Consider architectural improvements for better maintainability.The controller is well-structured but could benefit from some architectural improvements:
- Consider extracting complex query logic into query objects:
# app/queries/saved_annotation_query.rb class SavedAnnotationQuery def initialize(annotations) @annotations = annotations end def available_for_new_saved_annotation(current_user) @annotations .where(saved_annotation_id: nil) .where(user_id: current_user.id) end def courses_and_exercises submission_ids = @annotations.pluck(:submission_id).uniq submissions = Submission.where(id: submission_ids).pluck(:course_id, :exercise_id) course_ids, exercise_ids = submissions.transpose [ Course.where(id: course_ids.uniq), Activity.where(id: exercise_ids.uniq) ] end end
- Extract breadcrumb logic into a concern:
# app/controllers/concerns/breadcrumbable.rb module Breadcrumbable extend ActiveSupport::Concern private def saved_annotations_crumbs(*additional_crumbs) [[I18n.t('saved_annotations.index.title'), saved_annotations_path]] + additional_crumbs end endThese improvements would:
- Make the code more maintainable and testable
- Reduce duplication
- Improve separation of concerns
Would you like me to provide more detailed examples or help implement these improvements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- app/controllers/saved_annotations_controller.rb (1 hunks)
- app/policies/annotation_policy.rb (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/policies/annotation_policy.rb
🧰 Additional context used
📓 Learnings (1)
app/controllers/saved_annotations_controller.rb (1)
Learnt from: jorg-vr PR: dodona-edu/dodona#5648 File: app/controllers/saved_annotations_controller.rb:88-89 Timestamp: 2024-07-29T12:25:56.607Z Learning: When adding new instance variables in controller methods, ensure to update the test suite to cover these variables.
| submissions = Submission.where(id: @annotations.map(&:submission_id).uniq) | ||
| @courses = Course.where(id: submissions.map(&:course_id).uniq) | ||
| @exercises = Activity.where(id: submissions.map(&:exercise_id).uniq) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing the queries for better performance.
While the current implementation fixes the pluralization issue, it could be optimized for better performance. Consider these improvements:
- submissions = Submission.where(id: @annotations.map(&:submission_id).uniq)
- @courses = Course.where(id: submissions.map(&:course_id).uniq)
- @exercises = Activity.where(id: submissions.map(&:exercise_id).uniq)
+ submission_ids = @annotations.pluck(:submission_id).uniq
+ submissions = Submission.where(id: submission_ids).pluck(:course_id, :exercise_id)
+ course_ids, exercise_ids = submissions.transpose
+ @courses = Course.where(id: course_ids.uniq)
+ @exercises = Activity.where(id: exercise_ids.uniq)This optimization:
- Uses
pluckinstead ofmapto reduce memory usage - Retrieves course_id and exercise_id in a single query
- Maintains the fix for the pluralization issue while improving performance
Alternative approach using eager loading:
@annotations = @annotations.includes(:submission)
submissions = @annotations.map(&:submission).compact
@courses = Course.where(id: submissions.map(&:course_id).uniq)
@exercises = Activity.where(id: submissions.map(&:exercise_id).uniq)Choose the approach that best fits your needs:
- First approach: Better performance, less memory usage
- Second approach: More ActiveRecord-like, better for complex objects
This pull request fixes the filtering of annotations on the 'new saved annotations' page.
This issue was a quite complex rails pluralization issue.
Problem was
joins(:submission)createsINNER JOIN submissions ON submissions.id = annotations.submission_idwhile filters likewhere(submission: { exercise_id: exercise_id })createINNER JOIN submissions AS submission ON submission.id = annotations.submission_id.If both the
joinsand thewhereclause are present, the table name in the query issubmissionsingular. If only the join clause is present the table name issubmissionsplural.Starting to use 'submission' singular in most places and avoiding explicitly naming the table in pluck clauses resolves the issues.