Correctly copy all series info when copying courses #6049
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request modifies the course creation process to improve series copying functionality. The changes focus on preserving advanced series settings when creating a new course based on an existing one. Specifically, the modifications ensure that series are copied with their original advanced options intact, including progress tracking, activity visibility, and activity number settings. The implementation also ensures that series are ordered consistently during the copying process. Changes
Assessment against linked issues
The changes directly address the requirements in issue #5743 by ensuring that advanced options like activity visibility and activity number settings are copied when creating a new course from an existing one. Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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
🧹 Nitpick comments (3)
test/controllers/courses_controller_test.rb (3)
751-760: Test setup could be more explicit about visibility expectations.While the test correctly verifies that staff users can't copy hidden/closed series, consider adding assertions to explicitly verify which series should and shouldn't be copied, rather than just checking the total count.
- assert_equal 0, Course.find(response.parsed_body['id']).series.count + new_course = Course.find(response.parsed_body['id']) + assert_equal 0, new_course.series.count, "Staff user shouldn't be able to copy any series" + assert_equal 5, course.series.count, "Original course should still have all series"
769-789: Consider using test data helpers for better readability.While the test correctly verifies advanced settings, consider extracting the test data setup to helper methods or using a data-driven approach for better maintainability.
+ def create_series_with_settings(course, settings) + create(:series, course: course, **settings) + end + test 'a copied course should contain all series with the same advanced settings' do user = create :user, permission: :staff course = create :course course.administrating_members << user - create :series, course: course, progress_enabled: false, activities_visible: false, activity_numbers_enabled: true, name: 'foo' - create :series, course: course, progress_enabled: true, activities_visible: true, activity_numbers_enabled: false, name: 'bar' + test_cases = [ + { name: 'foo', progress_enabled: false, activities_visible: false, activity_numbers_enabled: true }, + { name: 'bar', progress_enabled: true, activities_visible: true, activity_numbers_enabled: false } + ] + test_cases.each { |settings| create_series_with_settings(course, settings) }
791-811: Improve test clarity by explicitly documenting the expected order.While the test correctly verifies series order, consider adding comments or using more explicit test data to clarify the expected ordering.
test 'a copied course should maintain the same series order' do user = create :user, permission: :staff course = create :course course.administrating_members << user + # Expected order after sorting: + # 1. first (order: nil, sorted by name) + # 2. second (order: nil, sorted by name) + # 3. third (order: 1) + # 4. fourth (order: 2) + # 5. fifth (order: 3) create :series, course: course, name: 'second' create :series, course: course, name: 'first' create :series, course: course, name: 'third', order: 1 create :series, course: course, name: 'fifth', order: 3 create :series, course: course, name: 'fourth', order: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/controllers/courses_controller.rb(2 hunks)test/controllers/courses_controller_test.rb(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test System
- GitHub Check: Test Ruby
🔇 Additional comments (2)
app/controllers/courses_controller.rb (2)
158-158: Ensure deterministic series order during copying.The addition of
.reorder(id: :asc)ensures consistent ordering of series when copying courses, preventing the order from being reversed or randomized.
172-174: Preserve advanced series settings during copying.The addition of
activities_visibleandactivity_numbers_enabledensures that all advanced series settings are properly copied to the new course.
Co-authored-by: Bart Mesuere <[email protected]>
This pull request fixes multiple bugs related to copying courses.
Firstly a bug in the test for students not being able to copy hidden series. This test also didn't copy visible series as the student wasn't correctly added to the course in the test.
Secondly a bug caused the order of series which have not been manually reordered to be reversed in a copied course.
Thirdly I made sure that advanced series settings are also copied when a course is copied: #5743
Closes #5743