Hotfix/inconsistency schema.rb #1172
Conversation
back to the orders table if it does not already exist.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds OTP verification tolerance (drift: 60) in two controllers, switches zatladder to choose between Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## staging #1172 +/- ##
===========================================
- Coverage 77.68% 77.62% -0.06%
===========================================
Files 54 54
Lines 1340 1341 +1
===========================================
Hits 1041 1041
- Misses 299 300 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@copilot write a pr description |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/controllers/callbacks_controller.rb(1 hunks)app/controllers/sofia_accounts_controller.rb(1 hunks)app/controllers/zatladder_controller.rb(1 hunks)db/migrate/20251212000001_add_order_total_back_to_orders.rb(1 hunks)db/schema.rb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/controllers/zatladder_controller.rb (1)
app/models/user.rb (1)
calculate_spendings(157-161)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (4)
db/migrate/20251212000001_add_order_total_back_to_orders.rb (1)
1-5: LGTM! Appropriate guard for hotfix scenario.The
unless column_exists?guard is appropriate for this hotfix addressing schema inconsistencies. It ensures the migration can run safely even if the column already exists in some environments.db/schema.rb (1)
13-13: LGTM! Schema version correctly updated.The schema version correctly reflects the new migration timestamp.
app/controllers/callbacks_controller.rb (1)
46-46: LGTM! Consistent OTP drift handling.The drift parameter is now consistently applied across both OTP verification flows (login and enable).
app/controllers/zatladder_controller.rb (1)
27-29: Clarify the data source selection logic.This introduces a significant behavior change: the zatladder will now use either
User.in_amberORUser.sofia_account, but never both.Questions:
- What is the expected behavior when both types of users exist?
- Why is this change included in a schema inconsistency hotfix?
- Is this related to a migration from Amber authentication to Sofia accounts?
Please clarify the intended business logic and consider documenting this behavior change in the PR description.
There was a problem hiding this comment.
Pull request overview
This pull request appears to be a hotfix addressing schema inconsistencies and making several functional improvements including database migration changes, OTP authentication enhancements, and zatladder controller fallback logic.
- Updates the database schema version from 2025_12_09_160709 to 2025_12_12_000001
- Adds a migration to restore the
order_totalcolumn to the orders table with conditional logic - Introduces fallback logic in zatladder controller to use sofia_account users when amber users don't exist
- Adds drift parameter to OTP authentication calls for more flexible time-based token validation
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| db/schema.rb | Updates the schema version to reflect the new migration |
| db/migrate/20251212000001_add_order_total_back_to_orders.rb | Adds migration to conditionally add order_total column back to orders table |
| app/controllers/zatladder_controller.rb | Implements fallback logic to use sofia_account users when no amber users exist |
| app/controllers/sofia_accounts_controller.rb | Adds drift parameter to OTP authentication for time tolerance |
| app/controllers/callbacks_controller.rb | Adds drift parameter to OTP authentication for time tolerance |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def zatladder_spendings(from, to) | ||
| @users_spendings = User.in_amber.calculate_spendings(from:, to:) | ||
| zatladder = User.in_amber.select(:id, :name).map do |user| | ||
| users = User.in_amber.any? ? User.in_amber : User.sofia_account |
There was a problem hiding this comment.
The fallback logic appears to have a potential issue: if User.in_amber returns an empty ActiveRecord::Relation, the any? check will return false and fall back to User.sofia_account. However, this creates inconsistency because User.in_amber.any? executes a database query, and then you're calling User.in_amber again (another query) instead of reusing the result. Consider storing the result in a variable first to avoid redundant queries and ensure consistent behavior.
| users = User.in_amber.any? ? User.in_amber : User.sofia_account | |
| amber_users = User.in_amber | |
| users = amber_users.any? ? amber_users : User.sofia_account |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/controllers/callbacks_controller.rb (1)
46-46: Good addition of OTP drift tolerance, consider extracting to a constant.Adding
drift: 30addresses clock skew between client and server devices. However, consider:
Value: A past review comment on the related controller suggested
drift: 60or higher based on RFC 6238 recommendations and industry practice. While 30 is an improvement, devices with moderate clock differences may still experience authentication failures.Maintainability: Extract this value to a constant (e.g.,
OTP_DRIFT_TOLERANCE = 30) defined in an initializer or configuration so it can be adjusted without code changes and remains consistent across both controllers.Example refactor:
# config/initializers/otp.rb OTP_DRIFT_TOLERANCE = 60 # secondsThen use it in both controllers:
- elsif sofia_account.authenticate_otp(params[:verification_code], drift: 30) + elsif sofia_account.authenticate_otp(params[:verification_code], drift: OTP_DRIFT_TOLERANCE)Based on past review comments suggesting drift: 60 or higher for practical clock skew accommodation.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/controllers/callbacks_controller.rb(1 hunks)app/controllers/sofia_accounts_controller.rb(1 hunks)app/controllers/zatladder_controller.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/controllers/zatladder_controller.rb
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (1)
app/controllers/sofia_accounts_controller.rb (1)
62-62: Good improvement over previous strict tolerance, though lower than past recommendation.The change from
drift: 1todrift: 30is a clear improvement and will reduce authentication failures from minor clock skew. However, the past review comment on this file specifically recommendeddrift: 60or higher based on RFC 6238 and industry practice for accommodating typical device/server clock differences.Additionally, as noted in the review of
callbacks_controller.rb, consider extracting this to a shared constant for consistency and maintainability.Based on past review comments recommending drift: 60 or 90 seconds for practical clock skew accommodation.
Summary by CodeRabbit
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.