Update games serializer and add slug validation with default generation #575
Conversation
WalkthroughThis pull request introduces several changes across multiple controllers, models, and migration files in the application. The 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
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #575 +/- ##
==========================================
+ Coverage 93.70% 93.77% +0.07%
==========================================
Files 72 72
Lines 1731 1736 +5
Branches 305 306 +1
==========================================
+ Hits 1622 1628 +6
+ Misses 109 108 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![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: 4
🧹 Outside diff range and nitpick comments (8)
db/seeds.rb (4)
88-88: Hey there! Consider making the game data more configurable 🎮While the current setup works, it might be better to move the game data to a configuration file or environment variables for easier maintenance.
Here's a suggestion:
- game = Game.find_or_create_by!(name: "Scarlet & Violet", slug: "sv") + GAME_CONFIG = { + name: ENV.fetch('GAME_NAME', 'Scarlet & Violet'), + slug: ENV.fetch('GAME_SLUG', 'sv') + } + game = Game.find_or_create_by!(**GAME_CONFIG)
Line range hint
38-57: Heads up! Let's add some time validation to the tournament creation 🕒The tournament creation looks good with both Swiss rounds and single elimination bracket, but we might want to add some validation to ensure the end time is always after the start time.
Here's a suggested improvement:
def create_tournament(name:, organization:, format:, game:, start_at:, end_at:) + raise ArgumentError, "End time must be after start time" 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 # ... rest of the method end end
Line range hint
124-156: Nice job on the query optimization! 🚀 Just a few suggestions for the Pokémon dataThe preloading of profiles is great for performance! The Pokémon data validation could be a bit more robust though.
Consider adding more comprehensive validation:
- raise "Invalid pokemon data" unless pokemon_data.all? { |p| p[:species].present? && p[:position].present? } + REQUIRED_POKEMON_FIELDS = %i[species position ability tera_type nature] + pokemon_data.each do |pokemon| + missing_fields = REQUIRED_POKEMON_FIELDS.reject { |field| pokemon[field].present? } + raise "Invalid pokemon data: missing #{missing_fields.join(', ')}" if missing_fields.any? + raise "Invalid position: #{pokemon[:position]}" unless (1..6).include?(pokemon[:position]) + end
Line range hint
1-24: Let's document those environment variables! 📝The code uses several environment variables like
ADMIN_USERNAME,TEST_ORG_NAME, and potentially others. It would be super helpful to document these at the top of the file.Add something like this at the top of the file:
# Required environment variables: # - ADMIN_USERNAME: Username for the admin account (default: thatguyinabeanie) # - TEST_ORG_NAME: Name of the test organization (default: The Rise of Fuecoco) # - SKIP_SEEDS: Set to "true" to skip seeding (optional)spec/requests/api/v1/organizations_controller_spec.rb (1)
328-335: Consider enhancing the validation test coverage.While the empty tournament hash will trigger validation errors, it would be valuable to add more specific test cases that verify individual validation rules (e.g., required fields, format constraints, etc.).
let(:tournament) { { + # Test cases for specific validation scenarios + name: "", # Invalid empty name + game_id: nil, # Missing required field + registration_end_at: Time.current - 1.day # Invalid date (past) } }spec/swagger_helper.rb (1)
254-264: The schema properties look good, but let's add some constraints! 🎯While the properties are well-defined, adding some validation constraints would make the API more robust:
player_capshould probably have a minimum value > 0start_atshould probably be after the current dateregistration_start_atshould be beforeregistration_end_atplayer_cap: { type: :integer, nullable: true }, +player_cap: { + type: :integer, + nullable: true, + minimum: 1 +},app/models/game.rb (1)
8-8: Looking good! Just one thing to consider about the validation.Hey! The validation looks solid, but we might want to add a format validation to ensure the slug follows a consistent pattern (like lowercase alphanumeric with hyphens).
Here's a suggestion:
- validates :slug, presence: true, uniqueness: true + validates :slug, presence: true, uniqueness: true, + format: { with: /\A[a-z0-9-]+\z/, message: "only allows lowercase letters, numbers, and hyphens" }spec/requests/api/v1/games_controller_spec.rb (1)
89-89: Hey there! Let's beef up the slug testing 🎮While adding the slug is great, we could make the tests even more robust by:
- Verifying the slug in response bodies
- Adding test cases for invalid slugs
- Testing slug uniqueness constraints
Here's a suggested addition to strengthen the test coverage:
let(:test_game) { create(:game, name: "Test Game", slug: "tg") } + +context "with invalid slug" do + let(:game) { { game: { name: "New Game", slug: "invalid slug" } } } + + response(400, "bad request") do + let(:request_account) { create(:admin) } + include_context "with Request Specs - Clerk JWT + Vercel OIDC Token Verification" + schema "$ref" => "#/components/schemas/Error" + run_test! + end +end
🛑 Comments failed to post (4)
app/models/game.rb (1)
12-16: 🛠️ Refactor suggestion
Heads up about the callback and slug generation! 🤔
A couple of things to think about:
- The callback only runs on create - what happens if someone changes the name later?
- We're using
&.on name but not on slug - might want to be consistent here.Here's a suggested improvement:
- before_validation :set_defaults, on: :create + before_validation :set_defaults def set_defaults - self.slug = name&.parameterize if slug.blank? && name.present? + self.slug = name&.parameterize if slug&.blank? && name.present? end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.before_validation :set_defaults def set_defaults self.slug = name&.parameterize if slug&.blank? && name.present? enddb/migrate/20241209035641_insert_values_into_games_and_format.rb (2)
9-9:
⚠️ Potential issueOops! Found a typo in the game name! 🎯
There's a small typo in "Brilliand Diamond" - should be "Brilliant Diamond"!
Here's the fix:
- bdsp = Game.create(name: "Brilliand Diamond & Shining Pearl", slug: "bdsp") + bdsp = Game.create(name: "Brilliant Diamond & Shining Pearl", slug: "bdsp")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.bdsp = Game.create(name: "Brilliant Diamond & Shining Pearl", slug: "bdsp")
6-42: 🛠️ Refactor suggestion
Let's make this migration more robust! 💪
A few suggestions to make this migration safer:
- Consider using
create!instead ofcreateto fail fast if something goes wrong- Wrap the data creation in a transaction
- Consider moving the seed data to
db/seeds.rbinsteadHere's how we could improve it:
def change add_column :games, :slug, :string, null: false add_index :games, :slug, unique: true + reversible do |dir| + dir.up do + ActiveRecord::Base.transaction do - sv = Game.create(name: "Scarlet & Violet", slug: "sv") + sv = Game.create!(name: "Scarlet & Violet", slug: "sv") # ... rest of the creation code with create! ... + end + end + end endCommittable suggestion skipped: line range outside the PR's diff.
db/schema.rb (1)
85-87: 💡 Codebase verification
Standardization needed for slug handling across tables 🎯
Found inconsistent null constraints for slug columns across different tables:
games:null: false(required)sluggables:null: false(required)- Two other tables (likely organizations and another): allows null values
This confirms the inconsistency in slug handling. Since slugs are typically used as URL-friendly identifiers, having some as required and others as optional could lead to inconsistent behavior in the application.
🔗 Analysis chain
Heads up about the slug constraints! 🎯
I noticed that
games.slugisNOT NULLwhileorganizations.slugallows null values. This inconsistency might cause confusion or issues down the line, especially if these fields serve similar purposes.Let's check if other tables in the schema use slugs and how they handle null constraints:
Consider standardizing the approach to slug handling across tables. If slugs are meant to be used as identifiers or in URLs, they should probably all be non-null.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all columns named 'slug' and their null constraints rg -A 1 't\.string\s+"slug"' db/schema.rbLength of output: 345
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/models/tournament.rb (2)
30-30: Looking good! Consider using the constant though! 👍The validation change from
greater_than: 0togreater_than: 3aligns perfectly with the minimum player requirements. However, to keep things DRY, we could use theMINIMUM_PLAYER_COUNTconstant here.Here's a small tweak to consider:
- validates :player_cap, numericality: { only_integer: true, greater_than: 3 }, allow_nil: true + validates :player_cap, numericality: { only_integer: true, greater_than_or_equal_to: MINIMUM_PLAYER_COUNT }, allow_nil: true
120-122: Hey there! Let's be consistent with ourselfusage! 🎯I see you're being explicit with
self.start_at(which is great!), but we should apply this consistently to all instance variable references in the method.Here's how we could make it more consistent:
- self.name ||= "#{organization.name}'s Tournament ##{organization.tournaments.count + 1}" if organization.present? + self.name ||= "#{self.organization.name}'s Tournament ##{self.organization.tournaments.count + 1}" if self.organization.present? return if self.start_at.blank? - self.late_registration ||= true + self.late_registration ||= true - self.registration_start_at ||= start_at - 1.week - self.registration_end_at ||= (start_at if late_registration) - self.check_in_start_at ||= start_at - 1.hour + self.registration_start_at ||= self.start_at - 1.week + self.registration_end_at ||= (self.start_at if self.late_registration) + self.check_in_start_at ||= self.start_at - 1.hour
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/controllers/api/v1/organizations_controller.rb(2 hunks)app/models/tournament.rb(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/controllers/api/v1/organizations_controller.rb
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
spec/models/tournament_spec.rb (1)
Line range hint
1-236: Hey! Mind if I suggest adding some slug-related tests? 🏷️I noticed this PR is all about adding slug validation and generation, but I don't see any tests covering that functionality yet. Would be super helpful to add some test cases to make sure those slugs are working as expected!
Here are some scenarios we might want to cover:
- Generating a default slug from the tournament name
- Handling duplicate slugs
- Validating slug format
Want me to help draft those test cases? Just let me know! 😊
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
spec/models/tournament_spec.rb(1 hunks)
🔇 Additional comments (1)
spec/models/tournament_spec.rb (1)
22-22: Hey there! The player cap validation looks good! 🎮
Nice update to ensure tournaments have enough players to be interesting! Setting the minimum to 4 players makes perfect sense - after all, what's a tournament with just 2-3 players, right? 😄 The validation still allows nil values which is great for flexibility.
Let's double-check if this matches the model validation:
✅ Verification successful
Perfect match! The validation is spot on! 🎯
The spec perfectly mirrors the model validation - both require player_cap to be greater than 3 and allow nil values. Everything's aligned just the way it should be! 👍
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the player_cap validation in the Tournament model
rg -A 2 "validates.*player_cap" app/models/tournament.rb
Length of output: 216
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
cspell.config.yaml (2)
Line range hint
2-6: Hey there! Spotted a duplicate entry in ignorePaths 👀
db/schema.rbappears twice in the ignore paths list. We should keep things tidy and remove one of them!ignorePaths: - db/schema.rb - Gemfile - Gemfile.lock - db/migrate - - db/schema.rb
Line range hint
16-58: Quick suggestion to make this file more maintainable! 🎯The words list is growing quite a bit! To make it easier to maintain and scan through, consider:
- Alphabetically sorting the words
- Using comments to group related words (e.g., Pokemon names, technical terms)
Here's how it could look:
words: + # Technical terms - autobuild - bytesize - - fcontext - - fdescribe - - filesize - - ILIKE + - deepcode + - fcontext + - fdescribe + - filesize + - ILIKE + - jacoco + - logpanel + # Pokemon related + - Butterfree + - Clefable + - Fuecoco + - Sandslash + - Staryu + - Victreebel + - tera
Enhance the games model by adding slug validation and default generation based on the name. Update the serializer used in the games controller and adjust related parameters in tournament creation. Additionally, include a migration to add the slug column to the games table and populate it with initial values.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores
slugfields to relevant database tables, ensuring data integrity.Tests
GamesControllerandOrganizationsControllerto include new scenarios and error cases.