Fix legacy 'exercises' path support #5854
Conversation
WalkthroughThe changes involve updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SubmissionsController
participant ExerciseModel
User->>SubmissionsController: GET /activities/:id/submissions or /exercises/:id/submissions
SubmissionsController->>SubmissionsController: set_submissions(params)
SubmissionsController->>ExerciseModel: Find Exercise by activity_id or exercise_id
ExerciseModel-->>SubmissionsController: Return Exercise object
SubmissionsController-->>User: Render submissions
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: 0
🧹 Outside diff range and nitpick comments (2)
app/controllers/submissions_controller.rb (1)
178-180: LGTM! Consider future-proofing the comment.The changes effectively address the PR objective of fixing legacy 'exercises' path support. The new logic allows for backward compatibility while maintaining existing functionality.
Consider updating the comment to indicate that the '/exercises' path is for legacy support:
- # both /activities/:id/submissions and /exercises/:id/submissions are valid routes + # /activities/:id/submissions is the primary route, /exercises/:id/submissions is supported for legacy compatibilityThis change would make it clearer that '/exercises' is a legacy path, which aligns better with the possibility of removing it in the future, as mentioned in the PR objectives.
test/controllers/submissions_controller_test.rb (1)
464-469: LGTM! Consider enhancing the test for explicit path verification.The test case effectively checks the accessibility of the legacy path containing 'exercise'. It aligns well with the PR objective of fixing legacy 'exercises' path support.
To make the test more robust, consider adding an explicit check to verify that the generated path contains 'exercise' instead of 'activity'. This can be done by asserting on the generated path before making the request. Here's a suggested enhancement:
test 'should be able to use legacy paths containing `exercise`' do course = create :course, series_count: 1, activities_per_series: 1 + path = course_series_exercise_submissions_path(course, course.series.first, course.series.first.exercises.first) + assert_includes path, 'exercise', "Generated path should contain 'exercise'" - get course_series_exercise_submissions_path(course, course.series.first, course.series.first.exercises.first) + get path assert_response :ok endThis addition ensures that the test explicitly verifies the presence of 'exercise' in the generated path, making the test more comprehensive.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- app/controllers/submissions_controller.rb (1 hunks)
- test/controllers/submissions_controller_test.rb (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
app/controllers/submissions_controller.rb (1)
178-180: Verify test coverage for the changes.The changes look good and address the PR objectives. However, it's important to ensure that appropriate tests have been added to cover this new functionality.
Could you please confirm that tests have been added to verify:
- The legacy '/exercises' path works as expected.
- The existing '/activities' path continues to function correctly.
- Both
activity_idandexercise_idparameters are handled properly in various scenarios.If these tests are in a separate file, please provide a link to the test file for review.
test/controllers/submissions_controller_test.rb (1)
Line range hint
1-469: Overall, the changes look good and align with the PR objectives.The addition of the new test case for legacy path support is appropriate and well-implemented. It effectively addresses the PR's main goal of fixing support for the legacy 'exercises' path.
The rest of the test suite remains unchanged and appears to provide comprehensive coverage for various aspects of submission handling, which is commendable.
This pull request fixes the support for the path
/courses/?/series/?/exercises/?/submissions.The
/exercisespaths have all been renamed to/activities, yet our routes still offer legacy support for/exercises. I didn't find any internal references that use the/exercisesname, so I assume the bug was discovered by an external link.I am also open to dropping the support for
/exercisesFixes an issue come in by mail, where a user tried to navigate to https://dodona.be/en/courses/156/series/2328/exercises/1963184607/submissions
For reference, the internally linked path https://dodona.be/en/courses/156/series/2328/activities/1963184607/submissions does work