Allow seeding in production with environment variable adjustment #581
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 rubocop (1.69.1)db/seeds.rbThere was an error parsing WalkthroughThis pull request introduces updates to two key files: Changes
Possibly related PRs
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
Documentation and Community
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
db/seeds.rb (5)
17-20: Hey, let's make the skip message more informative! 📢The string comparison is great, but we could make the skip message more helpful by including the environment variable name.
Here's a suggestion:
if ENV.fetch("SKIP_SEEDS", "false") == "true" - puts("Skipping seeding data.") + puts("Skipping seeds (SKIP_SEEDS=true)") exit end
Line range hint
28-52: Let's wrap this in a transaction for extra safety! 🔒The account and profile creation logic looks good, but we should ensure it's atomic.
Here's how we could improve it:
def create_account(username: nil, first_name: nil, last_name: nil, email: nil, pronouns: nil, admin: false) first_name ||= Faker::Name.first_name last_name ||= Faker::Name.last_name username ||= Faker::Internet.unique.username email ||= "#{username}@beanie.com" pronouns ||= "they/them" attrs = { email:, first_name:, last_name:, admin:, pronouns: } - # Check if Account already exists - account = Account.find_or_create_by_profile_username(username:, **attrs) - - # Check if profile with the given username already exists - profile = Profile.find_or_create_by!(username:) do |p| - p.account = account - end - - account.update!(default_profile: profile) + Account.transaction do + account = Account.find_or_create_by_profile_username(username:, **attrs) + profile = Profile.find_or_create_by!(username:) do |p| + p.account = account + end + account.update!(default_profile: profile) + end account end
Line range hint
54-76: Let's make those magic numbers configurable! ✨The tournament creation looks solid, but we've got some magic numbers that could be configuration constants.
Here's a suggestion:
+# At the top of the file +TOURNAMENT_CONFIG = { + check_in_buffer: 1.hour, + default_swiss_rounds: 5 +}.freeze def create_tournament(name:, organization:, format:, game:, start_at:, end_at:) Tournament.find_or_create_by!(name:, organization:, format:, game:) do |tournament| tournament.published = true tournament.start_at = start_at - tournament.check_in_start_at = start_at - 1.hour + tournament.check_in_start_at = start_at - TOURNAMENT_CONFIG[:check_in_buffer] tournament.end_at = end_at tournament.phases << Phases::Swiss.create!( name: "#{tournament.name} - Swiss Rounds", tournament:, - number_of_rounds: 5, + number_of_rounds: TOURNAMENT_CONFIG[:default_swiss_rounds], order: 0 )
Line range hint
106-110: Let's speed up those account creations! 🚀When creating multiple accounts, we could use bulk insert for better performance.
Here's how we could improve it:
accounts = Account.all if accounts.count < 50 needed_accounts = 50 - accounts.count - accounts = accounts + needed_accounts.times.map { |i| create_account(username: "seed_user_#{i}") } + new_accounts = needed_accounts.times.map do |i| + username = "seed_user_#{i}" + { + email: "#{username}@beanie.com", + first_name: Faker::Name.first_name, + last_name: Faker::Name.last_name, + pronouns: "they/them", + created_at: Time.current, + updated_at: Time.current + } + end + accounts = accounts + Account.insert_all!(new_accounts) end
Line range hint
133-134: Let's beef up that Pokemon validation! 🎮The current validation is good, but we could make it more thorough.
Here's a more comprehensive validation:
- raise "Invalid pokemon data" unless pokemon_data.all? { |p| p[:species].present? && p[:position].present? } + pokemon_data.each do |pokemon| + missing_fields = [] + %i[species position ability tera_type nature].each do |field| + missing_fields << field unless pokemon[field].present? + end + + if missing_fields.any? + raise "Invalid pokemon data: #{pokemon[:species] || 'Unknown'} missing #{missing_fields.join(', ')}" + end + + unless (1..6).include?(pokemon[:position]) + raise "Invalid position: #{pokemon[:position]} for #{pokemon[:species]}" + end + end
🛑 Comments failed to post (1)
config/environments/production.rb (1)
42-42: 💡 Codebase verification
Hey there! We found another Action Cable ENV usage that could use some love! 🛡️
I spotted that
config.action_cable.url = ENV["ACTION_CABLE_URL"]in production.rb isn't using the safe navigation operator, while its siblingallowed_request_originsis. Let's keep the safety measures consistent!
config/environments/production.rb: Add&toENV["ACTION_CABLE_URL"]to match the defensive style used forallowed_request_origins🔗 Analysis chain
Nice defensive programming! Let's verify the behavior 🛡️
The safe navigation operator (
&.) is a great addition here! It'll prevent those pesky nil errors whenACTION_CABLE_ALLOWED_ORIGINSisn't set.Let's make sure we're handling all the edge cases:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any other Action Cable related environment variables that might need similar protection rg -g '*.rb' 'ENV\["ACTION_CABLE.*"\](?!\s*&)'Length of output: 306
Script:
#!/bin/bash # Let's try a simpler search for Action Cable config patterns rg --pcre2 'ENV\["ACTION_CABLE' -g '*.rb' # Also search with ast-grep to catch any ENV usage with Action Cable ast-grep --pattern 'ENV["ACTION_CABLE$_"]' # And let's check for any other Action Cable config patterns rg 'config\.action_cable' -g '*.rb'Length of output: 879
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #581 +/- ##
=======================================
Coverage 93.77% 93.77%
=======================================
Files 72 72
Lines 1736 1736
Branches 306 306
=======================================
Hits 1628 1628
Misses 108 108
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
db/seeds.rb (3)
Line range hint
28-53: The account creation looks good, but let's make it bulletproof! 🛡️The logic is solid, but we could add some basic validation to ensure the email format is correct and handle potential race conditions in the find_or_create pattern.
Here's a suggested improvement:
def create_account(username: nil, first_name: nil, last_name: nil, email: nil, pronouns: nil, admin: false) first_name ||= Faker::Name.first_name last_name ||= Faker::Name.last_name username ||= Faker::Internet.unique.username email ||= "#{username}@beanie.com" pronouns ||= "they/them" + + # Basic email format validation + raise ArgumentError, "Invalid email format" unless email =~ URI::MailTo::EMAIL_REGEXP attrs = { email:, first_name:, last_name:, admin:, pronouns: } - # Check if Account already exists - account = Account.find_or_create_by_profile_username(username:, **attrs) + # Use transaction to prevent race conditions + Account.transaction do + account = Account.find_or_create_by_profile_username(username:, **attrs) - # Check if profile with the given username already exists - profile = Profile.find_or_create_by!(username:) do |p| - p.account = account - end + # Check if profile with the given username already exists + profile = Profile.find_or_create_by!(username:) do |p| + p.account = account + end - account.update!(default_profile: profile) - account + account.update!(default_profile: profile) + account + end end
Line range hint
55-71: The tournament setup looks neat! Just one tiny suggestion... ⏰The tournament creation logic is solid, but we might want to add some date validation to ensure
start_atis beforeend_atand thatcheck_in_start_atmakes sense.Here's a small improvement:
def create_tournament(name:, organization:, format:, game:, start_at:, end_at:) + raise ArgumentError, "end_at must be after start_at" if end_at <= start_at + Tournament.find_or_create_by!(name:, organization:, format:, game:) do |tournament| tournament.published = true tournament.start_at = start_at tournament.check_in_start_at = start_at - 1.hour tournament.end_at = end_at
Line range hint
115-146: Looking good! Let's make the Pokémon team creation even more robust! 🎮The Pokémon data validation is a great addition! However, the nested creation logic could be simplified and made more resilient to failures.
Here's a suggested improvement:
- # Validate pokemon_data before proceeding - raise "Invalid pokemon data" unless pokemon_data.all? { |p| p[:species].present? && p[:position].present? } + # Enhanced validation with specific error messages + pokemon_data.each do |pokemon| + missing = [:species, :position, :ability, :tera_type, :nature].select { |key| pokemon[key].blank? } + raise ArgumentError, "Missing required fields: #{missing.join(', ')}" if missing.any? + end tour.players = accounts_with_profiles.map do |account| next if tour.players.exists?(account:) - tour.players.create!(account:, in_game_name: account.default_profile.username, profile: account.default_profile).tap do |player| - team = PokemonTeam.create(profile: player.profile, format:, game:, published: true).tap do |pokemon_team| + # Use transaction to ensure data consistency + Player.transaction do + player = tour.players.create!( + account:, + in_game_name: account.default_profile.username, + profile: account.default_profile + ) + + team = PokemonTeam.create!( + profile: player.profile, + format:, + game:, + published: true + ) + + # Bulk create Pokémon for better performance + pokemon_records = pokemon_data.map do |pokemon| + pokemon.merge(pokemon_team: team) + end + Pokemon.create!(pokemon_records) - pokemon_team.pokemon = pokemon_data.map { |pokemon| Pokemon.create!(pokemon_team:, **pokemon) } - pokemon_team.format = format - pokemon_team.game = game - pokemon_team.save! - end - - player.pokemon_team_id = team.id - player.save! + player.update!(pokemon_team_id: team.id) + player end endThis refactor:
- Adds more specific validation error messages
- Uses transactions for data consistency
- Improves performance with bulk creation
- Simplifies the nested creation logic
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
db/seeds.rb(1 hunks)
🔇 Additional comments (2)
db/seeds.rb (2)
17-18: Hey there! Nice update to the environment variable check! 👋
The change from boolean to string comparison ("false") is a solid improvement since environment variables are always strings. This makes the code more reliable and prevents any unexpected type coercion issues.
Line range hint 1-146: Overall, these changes look great for enabling production seeding! 🎉
The modifications align perfectly with the PR objectives of allowing seeding in production environments. The code maintains data integrity while providing flexibility through environment variables.
Just a heads up though - we should verify that the seeding process won't interfere with existing production data.
Let's check for any existing production safeguards:
✅ Verification successful
Let me check for any potential data integrity concerns in the seeding logic.
Hey there! The seeding setup looks safe and sound! 🌱
The code has some really nice safeguards built in:
- Uses
find_or_createpatterns throughout, preventing duplicate records - Has a handy
SKIP_SEEDSenvironment variable as an emergency brake - Production database is properly isolated with its own URL configuration
- Data creation is idempotent and doesn't mess with existing records
Plus, there's a thoughtful touch with the reset_column_information calls before seeding, which helps avoid any schema-related hiccups. The seeding process will play nice with your production data! 👍
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for production environment checks or safeguards
rg -i "production|environment" config/
Length of output: 2773
Script:
#!/bin/bash
# Look for find_or_create patterns and data protection mechanisms
rg -B2 -A2 "find_or_create" db/seeds.rb
# Check for any database protection or environment specific configurations
rg -i "database|production" config/database.yml
Length of output: 2396
Modify the seeding logic to allow data seeding in production based on an environment variable, and update the handling of allowed request origins to prevent errors when the variable is not set.
Summary by CodeRabbit
New Features
Bug Fixes
Configuration Updates