Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Sample data generation #23558

Merged
merged 4 commits into from
Jul 11, 2018
Merged

Sample data generation #23558

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
de55076
Sample data generation
Jul 6, 2018
2428893
Fix typo in comment
Jul 6, 2018
4a34144
PR feedback
Jul 9, 2018
b0f4ba2
Code review feedback
Jul 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 5 additions & 3 deletions dashboard/lib/sample_data.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,14 @@ def self.seed
# dependency when we delete the teacher; but to not leave a trail of
# old test data behind, we explictly hard-delete.
def self.create_teacher(email, password, name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since deleting happens in this method that could conceivably in the future be called from anywhere, how about duplicating the

raise "Should not be run outside of development" unless CDO.rack_env?(:development)

check at the top of this method too, just to be paranoid?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see you've done this down on line 82. I'd pull it up here - as currently written, you could hit the hard-delete on a section without raising if that section had no students.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Added another one at top of method. I'd like to have defense in depth as the code changes in the future, perhaps inadvertently gets copied/moved into other contexts, etc.

raise "Should not be run outside of development" unless CDO.rack_env?(:development)
# Delete any existing test data
user = User.find_by_email_or_hashed_email(email)
unless user.nil?
user.sections.each do |section|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note:

The User.sections association is designated dependent destroy.

Same with Section.followers.

So, deleting one of these test users will (soft) delete all their sections and followers. It's worth a comment here to explain that this is to also (hard) delete the test users and to hard delete the section.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx, commented

# Hard-delete all students in each section.
section.students.each do |student_user|
raise "Not a sample student" unless student_user.name =~ SAMPLE_STUDENT_NAME_REGEX
raise "Not a sample student - #{student_user.name}" unless student_user.name =~ SAMPLE_STUDENT_NAME_REGEX
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name is not unique. Why not provide something that we can identify the record with like id or hashed_email?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name is not unique, but it IS identifiable. The only way we could accidentally delete, say, prod data is if someone named a user "TestStudent[#] Codeberg" (AND other code here somehow went berzerk and found it). I think is is a sufficiently robust cross-check for now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm referring to the error message, not the check. Getting an error "Not a sample student - Bobby Tables" is not that helpful if we want to find which student that is. I want the error message to have enough context to debug this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other words, I don't think this really satisfies my earlier request

raise "Should not be run outside of development" unless CDO.rack_env?(:development)
UserGeo.where(user_id: student_user.id).destroy_all
student_user.really_destroy!
Expand All @@ -88,8 +89,9 @@ def self.create_teacher(email, password, name)
end
UserGeo.where(user_id: user.id).destroy_all
# Delete the existing test teacher
raise "Not a sample teacher" unless user.name.eql? SAMPLE_TEACHER_NAME
raise "Not a sample teacher" unless user.email.eql? SAMPLE_TEACHER_EMAIL
unless (user.name.eql? SAMPLE_TEACHER_NAME) && (user.email.eql? SAMPLE_TEACHER_EMAIL)
raise "Not a sample teacher - #{user.name}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name is not unique. Why not provide something that we can identify the record with like id or email?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do check for email here as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above. I'm referring to the error message

end
raise "Should not be run outside of development" unless CDO.rack_env?(:development)
user.really_destroy!
end
Expand Down