Upgrade Ruby to 3.1.7 - #1337
Conversation
Updates version pins across Docker, CircleCI, and deploy scripts, and enables YAML alias loading where Psych now requires it.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughUpgrades Ruby from 2.7.3 → 3.1.7 across CI, Docker, deploy, systemd and scripts; standardizes Bundler to 2.3.27 in CircleCI; regenerates db/schema.rb (timestamp precision/nullability changes); enables YAML alias support for permissions/roles loading; sets PORT in the Procfile; plus assorted API/factory/test callsite adjustments. ChangesRuby & Bundler upgrade
YAML alias support for permissions/roles
Schema regeneration & miscellaneous behavior changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Procfile`:
- Line 2: The Procfile entry currently hardcodes PORT=3000 which overrides
injected ports; update the proc command that starts the Rails server (the "api:
PORT=3000 bundle exec rails server --binding=0.0.0.0" line) to respect
environment-provided PORT by using a shell fallback (e.g., use ${PORT:-3000}) or
simply remove the explicit assignment so the process uses the existing PORT env
var, ensuring the rails server still binds to 0.0.0.0.
In `@spec/factories/roles.rb`:
- Around line 40-41: The role factory is breaking because YAML aliases produce
nested arrays; in the block iterating YAML.load_file(...).each do |role_name,
permissions|, flatten the permissions (e.g., call permissions =
permissions.flatten) before the inner permissions.each so trait names aren't
built from arrays and the create(:permission, name: permission) call receives a
string; also defensively coerce permission to a string (permission.to_s) when
building trait identifiers and when passing to create to avoid non-string
values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 074f9187-44e4-4fac-ae7d-35a76cc9d36e
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
.circleci/config.yml.ruby-versionDockerfileGemfileProcfileconfig/deploy.rbconfig/systemd/sidekiq-production.servicedb/schema.rbdb/seeds.rblib/classes/role_permissions_mappings.rbscript/sidekiq_stats.rbspec/factories/roles.rbspec/factories/sequences.rb
Stop using apt-archive.postgresql.org (no Jammy Release file) and install postgresql-client from Ubuntu packages so apt update succeeds.
Use the ActiveRecord migration compatibility signature (2 positional args + keyword options) to avoid Ruby 3 keyword-arg arity errors.
Load donor conditions YAML in a Psych 4 compatible way and call I18n.t with keyword args to avoid Ruby 3 arity failures.
Replace the unmaintained state_machine gem with state_machines-activerecord and remove the legacy initializer patch. Also fix Ruby 3 keyword-arg and factory issues uncovered during the migration.
Vulnerable Libraries (1)
More info on how to fix Vulnerable Libraries in Ruby. 👉 Go to the dashboard for detailed results. 📥 Happy? Share your feedback with us. |
Enqueue Twilio jobs with keyword args while remaining compatible with callers passing an options hash, and update package controller specs to use keyword params for Rails.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/goodcity/errors.rb (1)
79-80: ⚡ Quick winConsider hardening
paramsargument to accept both string and symbol keys.The
**paramspattern requires all keys to be symbols. While the current codebase only passes the default empty hash (safe), a defensive check could protect against future misuse if callers passparams: {'string_key' => 'value'}, which would raise at runtime. The suggested fix addsto_h.symbolize_keysto normalize the hash regardless of key type.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/goodcity/errors.rb` around lines 79 - 80, The initializer defined by define_method(:initialize) currently splats params with **params which fails if callers pass string-keyed hashes; update the initialize block to coerce params into a hash with symbolized keys (e.g., ensure params = params.to_h.symbolize_keys || {} before calling I18n.t) so both string and symbol keys are accepted and then pass the normalized params into I18n.t; adjust the reference in this initialize method accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/goodcity/errors.rb`:
- Around line 79-80: The initializer defined by define_method(:initialize)
currently splats params with **params which fails if callers pass string-keyed
hashes; update the initialize block to coerce params into a hash with symbolized
keys (e.g., ensure params = params.to_h.symbolize_keys || {} before calling
I18n.t) so both string and symbol keys are accepted and then pass the normalized
params into I18n.t; adjust the reference in this initialize method accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 130086ce-2d1e-47dd-88db-4fe5fe837c7e
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.circleci/config.ymlGemfileapp/models/concerns/operations/stock_operations.rbapp/services/twilio_service.rbconfig/initializers/state_machine_patch.rbdb/migrate/20251219000001_add_index_to_messages_on_messageable_and_created_at.rblib/goodcity/errors.rbspec/controllers/api/v1/packages_controller_spec.rbspec/factories/donor_conditions.rbspec/factories/offers.rbspec/factories/orders.rb
💤 Files with no reviewable changes (1)
- config/initializers/state_machine_patch.rb
✅ Files skipped from review due to trivial changes (2)
- spec/factories/offers.rb
- spec/controllers/api/v1/packages_controller_spec.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- Gemfile
- .circleci/config.yml
Allow Token generation helpers and OrganisationsUserBuilder to accept either a hash or keyword args, and update requested packages controller specs to use Rails keyword params.
Avoid asserting a specific order for shareable offers when including expired shareables; the query does not guarantee ordering.
Make user filtering handle symbol/string param keys, stabilize holidays available _dates spec by freezing time, and stub appstore reviewer login number in safe delete specs.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
spec/controllers/api/v1/holidays_controller_spec.rb (1)
23-28:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe
to_not include(holiday_1)assertion on line 27 is vacuously true.
holiday_1is created atTime.zone.now + 30.days, but the request asks forschedule_days: 6, start_from: 2, which returns at most 6 working days starting 2 days from now (a window of ~8 calendar days). A holiday 30 days out can never appear in that window, so thisexpect(body).to_not include(...)will always pass regardless of whether the controller correctly filters holidays.To make this a meaningful assertion, create the holiday within the requested window (e.g.
+ 3.days) and assert it is excluded:🔧 Proposed fix
+ let!(:holiday_in_range) { create(:holiday, holiday: Time.zone.now + 3.days) } + it "return serialized available_dates within given range", :show_in_doc do get :available_dates, params: { schedule_days: 6, start_from: 2 } body = JSON.parse(response.body) expect(body.length).to eq(6) - expect(body).to_not include(JSON.parse(holiday_1.holiday.to_json)) + expect(body).to_not include(JSON.parse(holiday_in_range.holiday.to_json)) end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/controllers/api/v1/holidays_controller_spec.rb` around lines 23 - 28, The test's negative assertion is vacuous because holiday_1 is created at Time.zone.now + 30.days and will never fall in the requested window; update the setup so holiday_1 is created inside the window (e.g., change its timestamp to Time.zone.now + 3.days or similar) and then assert the response body does not include JSON.parse(holiday_1.holiday.to_json) (keeping the existing expect(body).to_not include(...) assertion) so the expectation actually verifies filtering logic for the available_dates action.
🧹 Nitpick comments (2)
spec/controllers/api/v1/holidays_controller_spec.rb (1)
32-38: 💤 Low valueHardcoded freeze date
2026-05-05will silently lose its intent over time.Pinning
Timecop.freezeto an absolute calendar date is fragile: if the test is re-read or copied months/years later the date has no self-evident meaning, and if the underlyingavailable_dateslogic ever becomes sensitive to day-of-week or locale-specific holidays, the Tuesday-specific choice won't be obvious. Consider computing relative to an arbitrary reference or documenting the day-of-week requirement inline.- Timecop.freeze(Time.zone.local(2026, 5, 5, 12, 0, 0)) do + # Freeze to a known Tuesday mid-day so the frozen day is a weekday + Timecop.freeze(Time.zone.local(2026, 5, 5, 12, 0, 0)) doAt minimum, a brief comment explains why this date was chosen.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/controllers/api/v1/holidays_controller_spec.rb` around lines 32 - 38, The test currently hardcodes Timecop.freeze(Time.zone.local(2026, 5, 5, 12, 0, 0)); replace that literal with a named reference (e.g. reference_date) computed to satisfy the test's day-of-week requirement (for example: pick a Date/Time that is explicitly a Tuesday) and use Timecop.freeze(reference_date) and create(:holiday, holiday: reference_date.beginning_of_day) and get :available_dates, params: { schedule_days: 6 } so the intent is clear and robust; if you keep a fixed date instead, add a one-line comment next to Timecop.freeze explaining why that specific date/day-of-week was chosen (referencing Timecop.freeze, create(:holiday, ...) and get :available_dates).app/models/token.rb (1)
45-45: 💤 Low value
(params || {})guard is redundant
paramsalready defaults to{}on line 41, so the|| {}branch is never reached.♻️ Proposed simplification
- params = (params || {}).merge(extra_params) + params = params.merge(extra_params)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/token.rb` at line 45, The `(params || {})` guard is redundant because `params` is already defaulted to `{}` earlier; replace `params = (params || {}).merge(extra_params)` with a direct merge using `params` (i.e., `params = params.merge(extra_params)`), referencing the `params` and `extra_params` variables in token.rb so the code is simplified and clearer.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/models/user.rb`:
- Around line 178-181: The local variable res can be nil when search_text is
blank causing NoMethodError on later chained scope calls; to fix, initialize res
to a base scope (e.g., User.all or self) before applying conditionals, then call
the existing methods search(search_text),
with_organisation_status(organisation_status.split(',')) and
with_roles(role_name) only to narrow that base scope; update the block that
currently assigns to res to start with res = all (or self) and then apply the
three conditional assignments using the existing methods search,
with_organisation_status, and with_roles so the method always returns an
ActiveRecord::Relation instead of nil.
---
Outside diff comments:
In `@spec/controllers/api/v1/holidays_controller_spec.rb`:
- Around line 23-28: The test's negative assertion is vacuous because holiday_1
is created at Time.zone.now + 30.days and will never fall in the requested
window; update the setup so holiday_1 is created inside the window (e.g., change
its timestamp to Time.zone.now + 3.days or similar) and then assert the response
body does not include JSON.parse(holiday_1.holiday.to_json) (keeping the
existing expect(body).to_not include(...) assertion) so the expectation actually
verifies filtering logic for the available_dates action.
---
Nitpick comments:
In `@app/models/token.rb`:
- Line 45: The `(params || {})` guard is redundant because `params` is already
defaulted to `{}` earlier; replace `params = (params || {}).merge(extra_params)`
with a direct merge using `params` (i.e., `params =
params.merge(extra_params)`), referencing the `params` and `extra_params`
variables in token.rb so the code is simplified and clearer.
In `@spec/controllers/api/v1/holidays_controller_spec.rb`:
- Around line 32-38: The test currently hardcodes
Timecop.freeze(Time.zone.local(2026, 5, 5, 12, 0, 0)); replace that literal with
a named reference (e.g. reference_date) computed to satisfy the test's
day-of-week requirement (for example: pick a Date/Time that is explicitly a
Tuesday) and use Timecop.freeze(reference_date) and create(:holiday, holiday:
reference_date.beginning_of_day) and get :available_dates, params: {
schedule_days: 6 } so the intent is clear and robust; if you keep a fixed date
instead, add a one-line comment next to Timecop.freeze explaining why that
specific date/day-of-week was chosen (referencing Timecop.freeze,
create(:holiday, ...) and get :available_dates).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25b084e4-9a38-4960-9bd0-01c20f526ecc
📒 Files selected for processing (7)
app/models/token.rbapp/models/user.rblib/classes/organisations_user_builder.rbspec/controllers/api/v1/holidays_controller_spec.rbspec/controllers/api/v1/offers_controller_spec.rbspec/controllers/api/v1/requested_packages_controller_spec.rbspec/lib/goodcity/user_safe_delete_spec.rb
Flatten YAML-derived permissions in role factory, tighten role filtering to active roles, make the system user setup idempotent, avoid rspec-mocks stubbing outside the lifecycle, and respect injected PORT in Procfile.
|
@steveyken The only outstanding issue I know of is that state_machines-activerecord ~> 0.9.0 has a high vulnerability. I can't seem to find details, but it appears we would need to upgrade Rails to move to a patched version. |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Order shareables by id before paginating to ensure stable page boundaries, and update the message subscriptions concern spec to reference the correct module and avoid building unrelated records.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/controllers/api/v2/shareables_controller.rb (1)
127-132:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winApply filters before
paginateto ensure pagination operates on the filtered scopeThe current code paginates the full
@shareablescollection (line 127), then applies.of_typeand.where(resource_id:)filters afterward (lines 128–129). This meanspaginatedivides the unfiltered set into pages, and then the filters remove items from those pages—potentially leaving you with far fewer results per page than intended and breaking pagination semantics.Apply the filters to
@shareablesbefore callingpaginate:♻️ Proposed fix
- # Ensure stable pagination order - records = paginate(`@shareables.order`(id: :asc)) - records = records.of_type(params[:resource_type]) if params[:resource_type].present? - records = records.where(resource_id: params[:resource_id].split(',')) if params[:resource_id].present? + # Apply filters first so pagination operates on the filtered scope + shareables = `@shareables` + shareables = shareables.of_type(params[:resource_type]) if params[:resource_type].present? + shareables = shareables.where(resource_id: params[:resource_id].split(',')) if params[:resource_id].present? + records = paginate(shareables.order(id: :asc))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/api/v2/shareables_controller.rb` around lines 127 - 132, The code calls paginate on `@shareables` before applying filters, which breaks pagination semantics; update the flow to apply filters (use .of_type when params[:resource_type].present? and .where(resource_id: params[:resource_id].split(',')) when params[:resource_id].present?) to the `@shareables` scope first, then call paginate(...) on that filtered scope, and finally pass the paginated records into serialize_shareables with the existing pagination_meta.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/controllers/api/v2/shareables_controller.rb`:
- Around line 127-132: The code calls paginate on `@shareables` before applying
filters, which breaks pagination semantics; update the flow to apply filters
(use .of_type when params[:resource_type].present? and .where(resource_id:
params[:resource_id].split(',')) when params[:resource_id].present?) to the
`@shareables` scope first, then call paginate(...) on that filtered scope, and
finally pass the paginated records into serialize_shareables with the existing
pagination_meta.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 56cd9989-c612-4910-abd3-aea6e87a591e
📒 Files selected for processing (7)
Procfileapp/controllers/api/v2/shareables_controller.rbapp/models/user.rbspec/factories/roles.rbspec/lib/goodcity/user_safe_delete_spec.rbspec/models/concerns/message_subscriptions_spec.rbspec/rails_helper.rb
🚧 Files skipped from review as they are similar to previous changes (4)
- Procfile
- app/models/user.rb
- spec/factories/roles.rb
- spec/lib/goodcity/user_safe_delete_spec.rb
Avoid flaky failures when other published packages match the generic substring "towel" in notes or related search fields.
1 participant
Updates version pins across Docker, CircleCI, and deploy scripts, and enables YAML alias loading where Psych now requires it.
Summary by CodeRabbit
Chores
Refactor