feat: add bonuses to the points system#104
Conversation
There was a problem hiding this comment.
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
app/models/game_player.rb:46
- [nitpick] Consider using update or update! instead of update_columns within the with_lock block to ensure that the updated_at timestamp and any related callbacks or validations are executed, unless bypassing them is intentional.
with_lock do
There was a problem hiding this comment.
Copilot reviewed 20 out of 22 changed files in this pull request and generated 1 comment.
Files not reviewed (2)
- app/assets/stylesheets/scoreboard.css: Language not supported
- app/assets/stylesheets/streaks.css: Language not supported
Comments suppressed due to low confidence (1)
app/models/game_question.rb:37
- Using current_phase_changed? may be deprecated or behave unexpectedly in Rails 8.1. Consider switching to saved_change_to_current_phase? to ensure the callback reliably detects phase changes.
return unless current_phase_changed? && answering? && started_at.nil?
| time_taken = @game_question.started_at.present? ? (time_at_answer - @game_question.started_at).round : 0 | ||
|
|
There was a problem hiding this comment.
Defaulting time_taken to 0 when started_at is nil may inadvertently award a full bonus for a correct answer. Consider explicitly handling cases where started_at is missing rather than assuming an instantaneous answer.
| time_taken = @game_question.started_at.present? ? (time_at_answer - @game_question.started_at).round : 0 | |
| time_taken = @game_question.started_at.present? ? (time_at_answer - @game_question.started_at).round : nil |
There was a problem hiding this comment.
Copilot reviewed 20 out of 22 changed files in this pull request and generated no comments.
Files not reviewed (2)
- app/assets/stylesheets/scoreboard.css: Language not supported
- app/assets/stylesheets/streaks.css: Language not supported
Comments suppressed due to low confidence (1)
db/migrate/20250403171237_add_current_streak_to_game_players.rb:4
- Consider setting a default value (e.g., 0) for the new current_streak column to avoid potential issues with nil values in production.
add_column :game_players, :current_streak, :integer
There was a problem hiding this comment.
Copilot reviewed 20 out of 22 changed files in this pull request and generated no comments.
Files not reviewed (2)
- app/assets/stylesheets/scoreboard.css: Language not supported
- app/assets/stylesheets/streaks.css: Language not supported
Comments suppressed due to low confidence (2)
app/models/game_player.rb:47
- Using update_columns bypasses validations and callbacks, which might skip important side-effects. Confirm that this behavior is intended.
update_columns(
points: points + question_points + speed_bonus + streak_bonus,
current_streak: streak_length
)
app/models/game_player.rb:71
- [nitpick] Consider clarifying the handling of negative time_taken values in calculate_speed_bonus to enhance code readability and ensure consistent behavior.
return 0 if time_taken.blank? || time_taken > 8
There was a problem hiding this comment.
Copilot reviewed 20 out of 22 changed files in this pull request and generated no comments.
Files not reviewed (2)
- app/assets/stylesheets/scoreboard.css: Language not supported
- app/assets/stylesheets/streaks.css: Language not supported
Comments suppressed due to low confidence (2)
app/models/game_question.rb:37
- In Rails 8.1, 'current_phase_changed?' may be deprecated; consider using 'will_save_change_to_current_phase?' to ensure compatibility.
return unless current_phase_changed? && answering? && started_at.nil?
test/models/game_player_test.rb:149
- [nitpick] There appears to be an overlap with another test for a 5s low-point bonus; consider consolidating to avoid redundant test cases.
test "awards 1 bonus point for low-value question at 5s" do
| @streak_length ||= begin | ||
| recent_answers = player_answers.order(created_at: :desc).pluck(:correct) | ||
| streak = recent_answers.take_while { |correct| correct }.count | ||
| [streak, 1].max | ||
| end |
There was a problem hiding this comment.
The memoization of streak_length using @streak_length may lead to stale values if player_answers are updated later in the lifecycle. Consider removing or appropriately resetting the memoization to ensure accurate bonus calculations.
| @streak_length ||= begin | |
| recent_answers = player_answers.order(created_at: :desc).pluck(:correct) | |
| streak = recent_answers.take_while { |correct| correct }.count | |
| [streak, 1].max | |
| end | |
| recent_answers = player_answers.order(created_at: :desc).pluck(:correct) | |
| streak = recent_answers.take_while { |correct| correct }.count | |
| [streak, 1].max |
There was a problem hiding this comment.
Copilot reviewed 21 out of 23 changed files in this pull request and generated no comments.
Files not reviewed (2)
- app/assets/stylesheets/scoreboard.css: Language not supported
- app/assets/stylesheets/streaks.css: Language not supported
Comments suppressed due to low confidence (3)
test/models/game_player_test.rb:70
- [nitpick] Consider using a consistent comment style (e.g. '# Instance Methods') to match the rest of the file.
############ Instance Methods
app/models/game_question.rb:36
- Consider adding tests to verify that started_at is automatically set when a game question transitions to the answering phase.
def set_started_at_if_answering
app/models/game_player.rb:47
- Using update_columns bypasses callbacks and validations; please confirm that this behavior is intended or consider using update if triggering callbacks is required.
update_columns(
There was a problem hiding this comment.
Copilot reviewed 21 out of 24 changed files in this pull request and generated no comments.
Files not reviewed (3)
- 1: Language not supported
- app/assets/stylesheets/scoreboard.css: Language not supported
- app/assets/stylesheets/streaks.css: Language not supported
Comments suppressed due to low confidence (1)
app/models/game_player.rb:70
- Consider adding tests to verify bonus calculations under different scenarios (e.g., varying time_taken values and question_points) to ensure the logic works as intended.
def calculate_speed_bonus(question_points, time_taken)
There was a problem hiding this comment.
Pull Request Overview
This PR adds bonus functionality to the points system by introducing new columns and logic to support time-based and streak-based rewards. Key changes include:
- Introducing a new time_taken field for player answers and started_at for game questions
- Adding a current_streak column and bonus award logic in game players
- Updating tests, fixtures, and views to incorporate bonus and streak displays
Reviewed Changes
Copilot reviewed 26 out of 28 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/fixtures/player_answers.yml | Added comment for the new time_taken column |
| test/fixtures/game_questions.yml | Added comment for the new started_at column |
| test/fixtures/game_players.yml | Updated fixtures to include the current_streak attribute |
| test/fixtures/answers.yml | Added fixture entries for correct and incorrect answers |
| test/controllers/game_player_answers_controller_test.rb | Added tests for the player answer creation flow |
| db/schema.rb, migrations | Updated schema and added migrations for new fields |
| app/views/layouts/_head.html.erb | Added a Google fonts stylesheet link |
| app/views/games/components/_scoreboard.html.erb | Rendered the current streak badge when applicable |
| app/views/games/components/_players_status.html.erb | Added a players count display |
| app/models/player_answer.rb, game_question.rb | Updated comments to reflect new fields and callbacks |
| app/models/game_player.rb | Added bonus calculation and streak tracking methods |
| app/controllers/game_player_answers_controller.rb | Modified creation endpoint to compute time_taken and save it |
| .rubocop.yml | Updated CyclomaticComplexity limit |
Files not reviewed (2)
- app/assets/stylesheets/scoreboard.css: Language not supported
- app/assets/stylesheets/streaks.css: Language not supported
Comments suppressed due to low confidence (1)
app/controllers/game_player_answers_controller.rb:25
- The error branch references '@player_answer' instead of the local variable 'player_answer', which could lead to runtime errors. Please update the error response to use 'player_answer.errors.full_messages'.
render json: { error: @player_answer.errors.full_messages }, status: :unprocessable_entity
There was a problem hiding this comment.
Pull Request Overview
This PR introduces bonus functionality into the points system while updating related fixtures, models, views, tests, and migrations.
- Introduces new columns and migrations (time_taken, current_streak, and started_at) to support bonus calculations.
- Implements bonus logic for speed and streak in game_player and adjusts UI elements and form validations accordingly.
Reviewed Changes
Copilot reviewed 30 out of 32 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/fixtures/answers.yml | Adds new answer fixture entries for both correct and incorrect cases. |
| test/controllers/game_player_answers_controller_test.rb | Adds a new integration test for creating a player answer. |
| db/schema.rb | Updates schema defaults and adds columns to support bonus features. |
| db/migrate/* | Adds migrations for duration change, time_taken, current_streak, and started_at. |
| app/views/questions/_form.html.erb | Adjusts form input limits for points and duration to match new validations. |
| app/views/layouts/_head.html.erb | Adds a Google Fonts stylesheet reference. |
| app/views/games/components/_scoreboard.html.erb | Displays current streak information in the scoreboard. |
| app/views/games/components/_players_status.html.erb | Displays player count in the players status view. |
| app/models/question.rb | Updates validations for points and duration. |
| app/models/player_answer.rb | Changes answer association to be optional. |
| app/models/game_question.rb | Adds callbacks to set start time and to reward players upon completion. |
| app/models/game_player.rb | Implements bonus calculation logic for speed and streak. |
| app/controllers/game_player_answers_controller.rb | Updates answer submission to record time taken and handles errors. |
| .rubocop.yml | Configures cyclomatic complexity limits. |
Files not reviewed (2)
- app/assets/stylesheets/scoreboard.css: Language not supported
- app/assets/stylesheets/streaks.css: Language not supported
Comments suppressed due to low confidence (1)
app/controllers/game_player_answers_controller.rb:30
- Reference to '@player_answer' is incorrect; the local variable 'player_answer' should be used to report errors.
render json: { error: @player_answer.errors.full_messages }, status: :unprocessable_entity
| all_players.find_each do |player| | ||
| current_question_answer = player.player_answers.find_by(game_question_id: id) | ||
| unless current_question_answer | ||
| player.player_answers.create(game_question: self, answer_id: -1, correct: false) |
There was a problem hiding this comment.
[nitpick] Using a magic number (-1) for 'answer_id' may lead to ambiguity; consider replacing it with a named constant or a more explicit approach.
| player.player_answers.create(game_question: self, answer_id: -1, correct: false) | |
| player.player_answers.create(game_question: self, answer_id: NO_ANSWER_ID, correct: false) |
No description provided.