Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new test user jim_dun to the seed data system and updates the test_seeds:destroy rake task to clean up data associated with this user. The PR also includes minor improvements to array syntax in the cleanup logic.
Changes:
- Added
jim_duntest user UUID toTEST_USERSconstant inseeds_helper.rb - Updated
test_seeds:destroytask to clean up roles and optionally schools created by thejim_dunuser - Refactored array syntax to remove unnecessary nested array wrapping
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| lib/tasks/seeds_helper.rb | Adds jim_dun UUID to TEST_USERS hash with comment indicating "user with no school" |
| lib/tasks/test_seeds.rake | Adds cleanup logic for jim_dun's roles and school; refactors array syntax in where clauses |
| # Destroy the project and then the lesson itself (The lesson's `before_destroy` prevents us using destroy) | ||
| lesson_ids = Lesson.where(school_id:).pluck(:id) | ||
| Project.where(lesson_id: [lesson_ids]).destroy_all | ||
| Lesson.where(id: [lesson_ids]).delete_all | ||
| Project.where(lesson_id: lesson_ids).destroy_all | ||
| Lesson.where(id: lesson_ids).delete_all |
There was a problem hiding this comment.
The cleanup logic only processes lessons and projects for the primary school_id (TEST_SCHOOL), but doesn't handle the teacher_signup_school_id. If jim_dun has created a school with lessons and projects, those won't be cleaned up before the school is destroyed at line 40. This could lead to orphaned records or issues with the Lesson before_destroy callback.
Consider extending the cleanup to include both schools, similar to:
lesson_ids = Lesson.where(school_id: [school_id, teacher_signup_school_id].compact).pluck(:id)| Lesson.where(id: lesson_ids).delete_all | ||
|
|
||
| # Destroy the class members and then the class itself | ||
| school_class_ids = SchoolClass.where(school_id:).pluck(:id) |
There was a problem hiding this comment.
The SchoolClass cleanup only handles the primary school_id, not the teacher_signup_school_id. If jim_dun has created a school with classes and class members, those won't be cleaned up before the school is destroyed at line 40.
Consider extending the cleanup to include both schools:
school_class_ids = SchoolClass.where(school_id: [school_id, teacher_signup_school_id].compact).pluck(:id)| school_class_ids = SchoolClass.where(school_id:).pluck(:id) | |
| school_class_ids = SchoolClass.where(school_id: [school_id, teacher_signup_school_id].compact).pluck(:id) |
| # school_ids = [school_id, teacher_signup_school_id].compact | ||
| School.where(id: school_id).destroy_all | ||
| School.where(id: teacher_signup_school_id).destroy_all |
There was a problem hiding this comment.
This commented-out code suggests the intended approach was to use compact to handle nil values. Either remove this commented line or refactor lines 39-40 to use this more elegant approach:
School.where(id: [school_id, teacher_signup_school_id].compact).destroy_allThis would handle both schools in a single call and safely exclude nil values.
| # school_ids = [school_id, teacher_signup_school_id].compact | |
| School.where(id: school_id).destroy_all | |
| School.where(id: teacher_signup_school_id).destroy_all | |
| School.where(id: [school_id, teacher_signup_school_id].compact).destroy_all |
| teacher_signup_id = TEST_USERS[:jim_dun] | ||
|
|
||
| # Hard coded as the student's school needs to match | ||
| student_ids = [TEST_USERS[:jane_smith], TEST_USERS[:john_smith], TEST_USERS[:emily_ssouser]] | ||
| school_id = TEST_SCHOOL | ||
| teacher_signup_school_id = | ||
| School.find_by(creator_id: teacher_signup_id)&.id | ||
|
|
||
| # Remove the roles first | ||
| Role.where(user_id: teacher_signup_id).destroy_all |
There was a problem hiding this comment.
The test suite for test_seeds:destroy doesn't verify the new jim_dun cleanup logic. Consider adding test coverage for scenarios where teacher_signup_id (jim_dun) has associated roles and potentially a school, to ensure the cleanup works correctly.
| john_smith: '0d488bec-b10d-46d3-b6f3-4cddf5d90c71', # student | ||
| emily_ssouser: '88e0aed6-8f20-4e40-98f9-610a0ab1cfcc' # sso student | ||
| emily_ssouser: '88e0aed6-8f20-4e40-98f9-610a0ab1cfcc', # sso student | ||
| jim_dun: '019a649f-87f3-483b-8eea-39a05c324264' # user with no school |
There was a problem hiding this comment.
The comment says "user with no school", but the code at lines 20-21 and 40 specifically looks for and destroys a school created by this user. This creates an inconsistency - either jim_dun should never have a school (in which case lines 20-21 and 40 are unnecessary), or the comment should be updated to reflect that jim_dun might create a school during testing.
Consider clarifying the intended behavior and updating either the comment or the code accordingly.
| jim_dun: '019a649f-87f3-483b-8eea-39a05c324264' # user with no school | |
| jim_dun: '019a649f-87f3-483b-8eea-39a05c324264' # user initially without a school (can be used to create schools in tests) |
2 participants
What's changed?