Upgrade Rails to 8.1 and Ruby to 3.4.9 - #1338
Conversation
Updates version pins across Docker, CircleCI, and deploy scripts, and enables YAML alias loading where Psych now requires it.
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.
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.
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.
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.
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.
Avoid flaky failures when other published packages match the generic substring "towel" in notes or related search fields.
Bump Ruby version pins across CI, Docker, and deploy/runtime scripts.
Bump Rails and related dependencies, updating the lockfile for Ruby 3.4.
Add compatibility shims and initializer ordering fixes needed to boot under Ruby 3.4 and Rails 8.1, and update cache key timestamp formatting.
Ruby 3.4 ships CSV as a default gem; add it explicitly so rake tasks that require csv load correctly in CI.
- OrderCodeGenerator: use POSIX-safe [0-9] for PostgreSQL ~; inline SUBSTRING start index in SQL so .maximum does not mis-bind placeholders. - AutoFavourite: resolve associations via reflect_on_association(name.to_sym) because _reflections keys are symbols in Rails 8.
Rails 8 removes Rails.application.secrets; expose secrets from config/secrets.yml via config_for so JWT and other callers keep working.
- Use alias_method for revisions (Rails 8 restricts alias_attribute to columns). - Query StocktakeRevision by stocktake_id so processing always sees current DB rows; avoids stale association cache skipping revisions and closing incorrectly.
…e.mail Rails 8 removes the old mail helper API from ActionMailer::Base. Add InternalNotificationMailer.plain and update Twilio and safe-delete jobs.
Blank searchText produced SIMILARITY(..., NULL) and matched no rows. Return an unrestricted relation when there is nothing to search.
Fixes Relation#union failing with undefined method arel_table for nil.
Drop have_db_column(...).of_type(:datetime) assertions that conflict with timestamptz; keep presence checks only.
Avoid brittle global Package/PackagesInventory counts when the test DB carries data from other examples.
- Match admin designations by the six seeded order ids instead of a fixed count. - Compare Package.count to a baseline before creating browse/search fixtures.
Disable transactional tests for this file and truncate packages-related tables before each example so low-sequence next_code tests stay valid. Replace shoulda uniqueness matcher with an explicit duplicate-code example.
Deleting User.system hit FK violations from packages_inventories; suite cleanup is unnecessary with transactional examples.
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughBumps Ruby to 3.4.9 and Rails to ~> 8.1, updates CI/Docker/deploy/Bundler, adds small compatibility shims, introduces InternalNotificationMailer and replaces ad-hoc mail calls with it, adjusts several job/service signatures and model/concern logic, broad schema/timestamp/soft-delete changes, and extensive test and seed/factory updates. ChangesUpgrade, compatibility, wiring, and tests (single cohesive DAG)
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as API Controller
participant Service as TwilioService
participant Job as TwilioJob
participant Mailer as InternalNotificationMailer
participant SMTP as Email Service
rect rgba(100,150,200,0.5)
Note over Client,SMTP: Old flow (pre-change)
Client->>Controller: send SMS request
Controller->>Service: send_sms(options)
Service->>Job: enqueue TwilioJob with options/hash
Job->>Job: env check (staging)
Job->>SMTP: ActionMailer::Base.mail(...).deliver
end
rect rgba(100,200,150,0.5)
Note over Client,SMTP: New flow (post-change)
Client->>Controller: send SMS request
Controller->>Service: send_sms(options, **kwargs)
Service->>Job: enqueue TwilioJob with merged options
Job->>Job: env check (staging)
Job->>Mailer: InternalNotificationMailer.plain(to:, subject:, body:)
Mailer->>SMTP: mail(...).deliver_now
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Clear persisted AppointmentSlots in the 2018 range before supervisor examples so for_date is not skewed by leftover rows; resolve calendar rows by date instead of array index; assert slot sets/timestamps without relying on sort order; scrub Dec 2018 before the timezone locking example.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
spec/models/concerns/share_support_spec.rb (2)
25-27: 💤 Low value
expectassertion insidebeforeblock produces opaque failuresPlacing
expect(...)inside abeforeblock is not idiomatic RSpec. When such an assertion fails, the error surfaces as anaround hook/beforefailure with no example name, making CI output harder to diagnose. The intent here — ensuring exactly 3 new rows were created — would be clearer as a dedicateditexample or as a sharedsubject/context guard.That said, this is a pragmatic trade-off for stabilising count baselines across pre-seeded data, and the same pattern is applied consistently across all three
beforeblocks (lines 25-27, 49-53, 69-72), so this is a minor concern.🤖 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/models/concerns/share_support_spec.rb` around lines 25 - 27, Move the expect assertion out of the before hook into a dedicated example to avoid opaque failures: replace the assertion inside the before block that calls touch(location_1, location_2, location_3) and checks ShareableLocation.count with a new it-block (e.g., "creates three ShareableLocation rows") that runs after the before setup and asserts expect(ShareableLocation.count).to eq(baseline + 3); update the other two similar before blocks (the ones surrounding lines with touch and ShareableLocation.count) the same way so all count checks are performed in explicit examples rather than inside before hooks.
69-76: 💤 Low value
@share_support_location_totalinstance variable approach is correct butbaselineis redundant when stored directlyThe pattern correctly uses an instance variable so the value is accessible across
beforeanditblocks. One small clarity nit:baselineis a throwaway local; the assignment could be inlined.♻️ Optional simplification
- baseline = ShareableLocation.count - touch(location_1, location_2, location_3) - `@share_support_location_total` = baseline + 3 - expect(ShareableLocation.count).to eq(`@share_support_location_total`) + `@share_support_location_total` = ShareableLocation.count + 3 + touch(location_1, location_2, location_3) + expect(ShareableLocation.count).to eq(`@share_support_location_total`)🤖 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/models/concerns/share_support_spec.rb` around lines 69 - 76, The test creates a throwaway local `baseline` just to compute `@share_support_location_total`; simplify by eliminating `baseline` and assign `@share_support_location_total` directly using the current count from ShareableLocation (e.g., set `@share_support_location_total` = ShareableLocation.count + 3 before calling touch(location_1, location_2, location_3)), then assert ShareableLocation.count and base_model.count against `@share_support_location_total`; update the block containing touch, `@share_support_location_total`, and the subsequent expect assertions accordingly.Gemfile (1)
43-43: ⚡ Quick winAdd an upper bound to the
paper_trailconstraint.The gemspec for
paper_trail17.0.0 declares its ActiveRecord compatibility only as a lower bound (>= 7.1) with an internal upper limit (< 8.2) that is used only to warn users, not to enforce installation. With'>= 17.0.0'and no upper bound in the Gemfile, a future major release (18.x) could be silently pulled bybundle update. Use a pessimistic operator to make upgrades intentional.♻️ Suggested change
-gem 'paper_trail', '>= 17.0.0' +gem 'paper_trail', '~> 17.0'🤖 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 `@Gemfile` at line 43, The Gemfile currently allows automatic upgrades of paper_trail beyond major version 17 via the loose constraint "gem 'paper_trail', '>= 17.0.0'"; update the constraint for the gem entry for paper_trail in the Gemfile (the line declaring gem 'paper_trail') to use a pessimistic/upper-bounded requirement (e.g., restrict to the 17.x series) so future major releases (18.x) aren't pulled by bundle update unintentionally; modify the gem declaration to include that upper bound and run bundle install/update to verify.app/controllers/api/v1/deliveries_controller.rb (1)
157-173: 💤 Low valueConsider memoizing the normalized delivery hash to eliminate double computation and reduce ordering fragility.
get_hash(delivery_attrs.to_h)is computed twice per request: once here inscheduled_date(called viavalidate_scheduleat line 102) and again insideget_delivery_detailsat line 149. Additionally,get_delivery_detailsmutatesparams["delivery"]with the result — meaning if call order were ever reversed,scheduled_datewould operate on the already-mutated (and double-underscore-normalized) hash, silently producing wrong results. The comment on lines 158–160 acknowledges this, but a memoized accessor would make the constraint structural rather than documentary.♻️ Suggested refactor: memoize the normalized delivery hash
+ def normalized_delivery_attrs + `@normalized_delivery_attrs` ||= get_hash(delivery_attrs.to_h) + end + def scheduled_date - # Use the same underscore normalization as get_delivery_details (get_hash), but do not - # rely on get_delivery_details — its permit step can differ by Rails version and must not - # mutate params during validation. Client sends scheduleAttributes.scheduledAt. - raw_delivery = get_hash(delivery_attrs.to_h) - sched = raw_delivery["schedule_attributes"] + sched = normalized_delivery_attrs["schedule_attributes"] return nil unless sched.is_a?(Hash) scheduled_at = sched["scheduled_at"] ... end def get_delivery_details - params["delivery"] = get_hash(delivery_attrs.to_h) + params["delivery"] = normalized_delivery_attrs params.require(:delivery).permit(...) 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 `@app/controllers/api/v1/deliveries_controller.rb` around lines 157 - 173, scheduled_date calls get_hash(delivery_attrs.to_h) and get_delivery_details calls get_hash again and mutates params["delivery"], causing double computation and ordering fragility; introduce a memoized accessor (e.g., cached_normalized_delivery or normalized_delivery_hash) that computes get_hash(delivery_attrs.to_h) once and returns the cached hash for both scheduled_date and get_delivery_details (and any caller such as validate_schedule), update scheduled_date to use the memoized accessor instead of calling get_hash directly, and ensure get_delivery_details uses the same accessor rather than re-normalizing or mutating params["delivery"] so the normalization is performed exactly once per request.spec/controllers/api/v1/packages_controller_spec.rb (1)
425-435: 💤 Low valueMinor inconsistency:
mapstill used to generateitblocks here whileeachwas applied at lines 439–448.Both work for dynamically defining examples, but mixing them in adjacent contexts is inconsistent. Consider aligning to
eachthroughout.♻️ Proposed fix
- [true, false].map do |val| + [true, false].each do |val| it "creates package with saleble value #{val}" do🤖 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/packages_controller_spec.rb` around lines 425 - 435, The spec uses [true, false].map to define dynamic it blocks which is inconsistent with nearby examples that use each; change the iterator from map to each for the block that defines the "creates package with saleable value" examples (the block containing item.offer.update(saleable: false), package_params[:saleable] = val, post :create, and the expectations) so it matches the adjacent style and avoids the unnecessary map return value.
🤖 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 `@spec/jobs/twilio_job_spec.rb`:
- Line 82: The test uses ENV["EMAIL_FROM"] which can be nil in CI and makes the
assertion meaningless; change the spec to set or stub the ENV value before
invoking InternalNotificationMailer.plain so the recipient is deterministic
(e.g. set ENV["EMAIL_FROM"] = "test@example.com" in the example or use
allow(ENV).to receive(:[]).with("EMAIL_FROM").and_return("test@example.com")),
then assert expect(mail.to).to eq(["test@example.com"]); reference the example
that calls InternalNotificationMailer.plain (which uses ENV.fetch("EMAIL_FROM"))
and the local mail variable in the spec when making this change.
---
Nitpick comments:
In `@app/controllers/api/v1/deliveries_controller.rb`:
- Around line 157-173: scheduled_date calls get_hash(delivery_attrs.to_h) and
get_delivery_details calls get_hash again and mutates params["delivery"],
causing double computation and ordering fragility; introduce a memoized accessor
(e.g., cached_normalized_delivery or normalized_delivery_hash) that computes
get_hash(delivery_attrs.to_h) once and returns the cached hash for both
scheduled_date and get_delivery_details (and any caller such as
validate_schedule), update scheduled_date to use the memoized accessor instead
of calling get_hash directly, and ensure get_delivery_details uses the same
accessor rather than re-normalizing or mutating params["delivery"] so the
normalization is performed exactly once per request.
In `@Gemfile`:
- Line 43: The Gemfile currently allows automatic upgrades of paper_trail beyond
major version 17 via the loose constraint "gem 'paper_trail', '>= 17.0.0'";
update the constraint for the gem entry for paper_trail in the Gemfile (the line
declaring gem 'paper_trail') to use a pessimistic/upper-bounded requirement
(e.g., restrict to the 17.x series) so future major releases (18.x) aren't
pulled by bundle update unintentionally; modify the gem declaration to include
that upper bound and run bundle install/update to verify.
In `@spec/controllers/api/v1/packages_controller_spec.rb`:
- Around line 425-435: The spec uses [true, false].map to define dynamic it
blocks which is inconsistent with nearby examples that use each; change the
iterator from map to each for the block that defines the "creates package with
saleable value" examples (the block containing item.offer.update(saleable:
false), package_params[:saleable] = val, post :create, and the expectations) so
it matches the adjacent style and avoids the unnecessary map return value.
In `@spec/models/concerns/share_support_spec.rb`:
- Around line 25-27: Move the expect assertion out of the before hook into a
dedicated example to avoid opaque failures: replace the assertion inside the
before block that calls touch(location_1, location_2, location_3) and checks
ShareableLocation.count with a new it-block (e.g., "creates three
ShareableLocation rows") that runs after the before setup and asserts
expect(ShareableLocation.count).to eq(baseline + 3); update the other two
similar before blocks (the ones surrounding lines with touch and
ShareableLocation.count) the same way so all count checks are performed in
explicit examples rather than inside before hooks.
- Around line 69-76: The test creates a throwaway local `baseline` just to
compute `@share_support_location_total`; simplify by eliminating `baseline` and
assign `@share_support_location_total` directly using the current count from
ShareableLocation (e.g., set `@share_support_location_total` =
ShareableLocation.count + 3 before calling touch(location_1, location_2,
location_3)), then assert ShareableLocation.count and base_model.count against
`@share_support_location_total`; update the block containing touch,
`@share_support_location_total`, and the subsequent expect assertions accordingly.
🪄 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: 6afc161e-e869-45cb-8531-c6c98c285f39
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
Gemfileapp/controllers/api/v1/deliveries_controller.rbapp/models/appointment_slot.rbapp/models/package.rbapp/services/subscriptions_reminder.rbapp/services/twilio_service.rbconfig/environments/test.rbdb/migrate/20181121040221_change_order_transport_scheduled_at_to_datetime.rblib/classes/packages_inventories_importer.rblib/goodcity/image_archiver.rbspec/controllers/api/v1/appointment_slots_controller_spec.rbspec/controllers/api/v1/organisations_controller_spec.rbspec/controllers/api/v1/package_types_controller_spec.rbspec/controllers/api/v1/packages_controller_spec.rbspec/jobs/poll_gogovan_order_status_job_spec.rbspec/jobs/twilio_job_spec.rbspec/lib/classes/packages_inventories_importer_spec.rbspec/migrations/change_order_transport_scheduled_at_to_datetime_spec.rbspec/models/concerns/share_support_spec.rbspec/rails_helper.rbspec/services/twilio_service_spec.rbspec/support/migration_helpers.rb
💤 Files with no reviewable changes (1)
- spec/jobs/poll_gogovan_order_status_job_spec.rb
|
|
||
| TwilioJob.new.perform(options) | ||
| mail = ActionMailer::Base.deliveries.last | ||
| expect(mail.to).to eq([ENV["EMAIL_FROM"]]) |
There was a problem hiding this comment.
ENV["EMAIL_FROM"] may be nil in test environments, making the assertion vacuous.
If EMAIL_FROM is not set in CI or the local test environment, ENV["EMAIL_FROM"] returns nil, so the expectation becomes expect(mail.to).to eq([nil]). The mail delivery from InternalNotificationMailer.plain(to: ENV.fetch("EMAIL_FROM"), ...) would raise a KeyError at runtime (since fetch raises on a missing key), so the test would actually fail with an error rather than pass silently — but the assertion itself provides no confidence that the correct recipient was used.
Consider stubbing ENV or using a fixed test value:
🛡️ Proposed fix
+ before { allow(ENV).to receive(:fetch).with("EMAIL_FROM").and_return("test@example.com") }
+
it "should send an email instead of an SMS" do
expect {
TwilioJob.new.perform(options)
}.to change { ActionMailer::Base.deliveries.size }.by(1)
mail = ActionMailer::Base.deliveries.last
- expect(mail.to).to eq([ENV["EMAIL_FROM"]])
+ expect(mail.to).to eq(["test@example.com"])
expect(mail.subject).to eq("SMS to #{options[:to]}")
expect(mail.body.raw_source).to include(options[:body])
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.
| expect(mail.to).to eq([ENV["EMAIL_FROM"]]) | |
| before { allow(ENV).to receive(:fetch).with("EMAIL_FROM").and_return("test@example.com") } | |
| it "should send an email instead of an SMS" do | |
| expect { | |
| TwilioJob.new.perform(options) | |
| }.to change { ActionMailer::Base.deliveries.size }.by(1) | |
| mail = ActionMailer::Base.deliveries.last | |
| expect(mail.to).to eq(["test@example.com"]) | |
| expect(mail.subject).to eq("SMS to #{options[:to]}") | |
| expect(mail.body.raw_source).to include(options[:body]) | |
| 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/jobs/twilio_job_spec.rb` at line 82, The test uses ENV["EMAIL_FROM"]
which can be nil in CI and makes the assertion meaningless; change the spec to
set or stub the ENV value before invoking InternalNotificationMailer.plain so
the recipient is deterministic (e.g. set ENV["EMAIL_FROM"] = "test@example.com"
in the example or use allow(ENV).to
receive(:[]).with("EMAIL_FROM").and_return("test@example.com")), then assert
expect(mail.to).to eq(["test@example.com"]); reference the example that calls
InternalNotificationMailer.plain (which uses ENV.fetch("EMAIL_FROM")) and the
local mail variable in the spec when making this change.
- Parse nested schedule params into plain hashes before computing scheduled_date so public-holiday checks work with ActionController::Parameters in Rails 8. - Appointment slots specs: avoid memoized JSON, align timezone cleanup, and use flexible time assertions. - Package filtering specs: scope expectations to package IDs created in each example to ignore unrelated rows.
- Deliveries: normalize nested ActionController::Parameters in get_hash; derive scheduled_at from schedule_attributes plus a params fallback for camelCase; parse dates with Time.zone and treat blank scheduledAt as absent. - Deliveries spec: resilient holiday setup; fix bad-data examples to use scheduledAt and empty string for missing date. - Appointment slots spec: clear AppointmentSlotPreset rows before seeding presets to match CI without duplicate weekdays. - Donor conditions spec: create rows with unique name_en so the factory’s find_or_initialize_by does not collide with seeded donor conditions.
- Orders: use case/when in epoch_ms and day_epoch_ms so Date/DateTime use in_time_zone (Ruby 3 removed Date#to_i). - Appointment slots: build Oct/Mar fixture times with Time.zone.parse; clear stray HKT 2018-12-19 quota-0 rows before the UTC closure POST example.
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/donor_conditions_controller_spec.rb (1)
45-49:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOrdering assumption in
visible_to_donorassertion may cause flakiness.The
beforeblock (now shared with this example) inserts two new rows withvisible_to_donor: true, so both the response and thepluckcall will include them. However,DonorCondition.pluck(:visible_to_donor)returns values in database-default (typically insertion) order, while the controller's index action may apply a different sort. If the orderings ever diverge the assertion will produce a false failure.Consider asserting on sets/multisets instead of ordered arrays, or explicitly sort both sides:
🛡️ Proposed fix to make the assertion order-independent
- expect( parsed_body['donor_conditions'].map { |condition| condition['visible_to_donor']} ).to eq(DonorCondition.pluck(:visible_to_donor)) + expect( parsed_body['donor_conditions'].map { |condition| condition['visible_to_donor'] }.sort ).to eq(DonorCondition.pluck(:visible_to_donor).map { |v| v ? true : false }.sort)Or, if you only care that every returned record has a
visible_to_donorkey with a boolean value:- expect( parsed_body['donor_conditions'].map { |condition| condition['visible_to_donor']} ).to eq(DonorCondition.pluck(:visible_to_donor)) + returned_values = parsed_body['donor_conditions'].map { |condition| condition['visible_to_donor'] } + expect(returned_values).to all(be(true).or(be(false))) + expect(returned_values.sort).to eq(DonorCondition.pluck(:visible_to_donor).sort)🤖 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/donor_conditions_controller_spec.rb` around lines 45 - 49, The test in donor_conditions_controller_spec.rb is assuming order by comparing parsed_body['donor_conditions'].map { |condition| condition['visible_to_donor'] } to DonorCondition.pluck(:visible_to_donor) which can be flaky if controller index ordering differs; update the assertion in the "returns 'visible_to_donor' in serialized response" example to be order-independent by either using a multiset comparison (e.g. match_array) between the two arrays or by explicitly sorting both arrays before comparing, or alternatively assert each returned condition has a boolean visible_to_donor key (using parsed_body and enumerating values) so the test no longer depends on record order.
🧹 Nitpick comments (5)
spec/models/concerns/package_filtering_spec.rb (1)
21-22: ⚡ Quick winGlobal count assertions are partially migrated — consider completing ID-scoping.
Lines 21–22 still assert on
Package.count/Package.apply_filter.countwithout.where(id:@fil_pkg_ids), while every other assertion in this file was converted to ID-scoped counts. The@package_filter_baselinesnapshot mitigates most of the risk, but these assertions will silently drift if anything else in the suite ever creates aPackagevia abefore(:suite)orbefore(:all)block (which runs outside the transactional rollback window).♻️ Proposed fix to apply consistent ID-scoping
it 'does not filter out anything if no explicit arguments are provided' do - expect(Package.count).to eq(`@package_filter_baseline` + 5) - expect(Package.apply_filter.count).to eq(`@package_filter_baseline` + 5) + expect(Package.apply_filter.where(id: `@fil_pkg_ids`).count).to eq(5) 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/models/concerns/package_filtering_spec.rb` around lines 21 - 22, The two assertions use global counts (Package.count and Package.apply_filter.count) instead of ID-scoped counts like the rest of the file; update both to scope to the filtered package ids by replacing Package.count and Package.apply_filter.count with Package.where(id: `@fil_pkg_ids`).count and Package.apply_filter.where(id: `@fil_pkg_ids`).count respectively so the expectations remain stable against external test-suite package creation while still using `@package_filter_baseline` in the arithmetic.spec/controllers/api/v1/deliveries_controller_spec.rb (2)
165-172: 💤 Low valueLGTM — robust holiday seeding.
Idempotent seeding with a unique name, plus the
RecordInvalidrescue guarded by a re-check ofHoliday.is_holiday?, neatly handles the case where another fixture/spec already seeded the same date. One tiny redundancy:Date.parse(...)already returns aDate, so.to_dateon line 165 is a no-op and can be dropped.♻️ Tiny cleanup
- date = Date.parse(schedule["scheduledAt"]).to_date + date = Date.parse(schedule["scheduledAt"])🤖 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/deliveries_controller_spec.rb` around lines 165 - 172, The Date.parse(...).to_date call is redundant because Date.parse already returns a Date; update the spec to remove the unnecessary .to_date by parsing schedule["scheduledAt"] into date with Date.parse(schedule["scheduledAt"]) and keep the rest (the Holiday.is_holiday? check and the Holiday.create! / rescue block) unchanged; ensure references remain to the local variable date and existing methods Holiday.is_holiday? and Holiday.create!.
191-204: 💤 Low valueNit: re-parsing
response.bodyinstead of usingsubject.
subjectis already defined asJSON.parse(response.body)at the top of this spec file (line 5) and is used in adjacent tests (e.g., thenot a datetest below). Usingsubjecthere would keep the style consistent and remove the localparsedvariable. The actual coverage of the blank-scheduledAt path looks correct.♻️ Suggested cleanup
expect(response.status).to eq(422) - parsed = JSON.parse(response.body) - expect(parsed["errors"].length).to eq(1) - expect(parsed["errors"][0]["message"]).to eq( - "The selected date is either missing or invalid, please try again." - ) + expect(subject["errors"].length).to eq(1) + expect(subject["errors"][0]["message"]).to eq( + "The selected date is either missing or invalid, please try again." + )🤖 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/deliveries_controller_spec.rb` around lines 191 - 204, The test re-parses response.body into a local variable `parsed` even though `subject` is already defined as `JSON.parse(response.body)` for this spec; replace uses of the local `parsed` with `subject` in the "should fail to modify the delivery if scheduled_at is nil" example (keep the existing expects against `response.status` and the Gogovan/GogovanOrder mocks), i.e. remove the `parsed = JSON.parse(response.body)` line and update subsequent assertions to reference `subject["errors"]` and `subject["errors"][0]["message"]` so the style matches adjacent tests.app/controllers/api/v1/deliveries_controller.rb (2)
175-191: 💤 Low valueOptional: the second fallback branch looks effectively redundant.
delivery_attrsisparams.require(:delivery).permit!, so it preserves every nested key fromparams[:delivery], includingscheduleAttributes. Branch 1 then runsget_hash, which underscores keys toschedule_attributes/scheduled_at. If branch 1 doesn't yield a value, the same lookup againstparams.to_unsafe_hagainst camelCase keys will not find anything different, since both originate from the same params payload. Consider collapsing to the single (already exhaustive) branch for clarity.♻️ Possible simplification
def scheduled_at_from_delivery_params raw_delivery = get_hash(parameters_to_plain_hash(delivery_attrs.to_unsafe_h)) sched = raw_delivery["schedule_attributes"] - if sched.is_a?(Hash) - at = sched["scheduled_at"].presence || sched[:scheduled_at].presence - return at if at.present? - end - - plain = parameters_to_plain_hash(params.to_unsafe_h) - d = plain["delivery"] || plain[:delivery] - return nil unless d.is_a?(Hash) - - sa = d["schedule_attributes"] || d["scheduleAttributes"] || d[:schedule_attributes] || d[:scheduleAttributes] - return nil unless sa.is_a?(Hash) - - (sa["scheduled_at"] || sa["scheduledAt"] || sa[:scheduled_at] || sa[:scheduledAt]).presence + return nil unless sched.is_a?(Hash) + + sched["scheduled_at"].presence 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 `@app/controllers/api/v1/deliveries_controller.rb` around lines 175 - 191, The second fallback branch in scheduled_at_from_delivery_params is redundant because delivery_attrs (params.require(:delivery).permit!) already contains the same nested keys and get_hash(parameters_to_plain_hash(delivery_attrs.to_unsafe_h)) will underscore camelCase keys (via get_hash), so remove the whole fallback that reads from parameters_to_plain_hash(params.to_unsafe_h) and its subsequent lookups; instead, rely solely on raw_delivery (from delivery_attrs) to extract schedule_attributes and scheduled_at (using the existing checks for string/symbol keys), keeping the early return behaviour in scheduled_at_from_delivery_params and preserving presence checks.
215-226: ⚖️ Poor tradeoffOptional:
parameters_to_plain_hashmay be redundant on Rails 8.
ActionController::Parameters#to_unsafe_halready performs deep recursive conversion of nested Parameters intoHashWithIndifferentAccess, so this helper's recursion mostly convertsHashWithIndifferentAccessto plainHash. Since downstream code works with both types (including defensive key access likeplain["delivery"] || plain[:delivery]), you could remove this helper and passto_unsafe_hresult directly toget_hash, which already handles defensive Parameter conversion and key normalization. Not a defect, just a chance to reduce the param-shaping surface area.🤖 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/v1/deliveries_controller.rb` around lines 215 - 226, The parameters_to_plain_hash helper is redundant on Rails 8 because ActionController::Parameters#to_unsafe_h already deeply converts nested params; remove the parameters_to_plain_hash method and update any callers to pass params.to_unsafe_h (or the specific ActionController::Parameters instance .to_unsafe_h) directly into get_hash so get_hash continues to handle key normalization and defensive access (ensure references to parameters_to_plain_hash are replaced with .to_unsafe_h and delete the parameters_to_plain_hash definition).
🤖 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 `@spec/controllers/api/v1/appointment_slots_controller_spec.rb`:
- Around line 330-334: The test currently allows "timestamp with time zone";
tighten the assertion in the spec for AppointmentSlot.columns (the example named
'Should use a timestamp column for slot times') to explicitly require the
"without time zone" form while still permitting precision, e.g. change the
sql_type expectation to match a pattern like "timestamp" optionally followed by
precision and then " without time zone" (use a regex such as
/\Atimestamp(?:\(\d+\))? without time zone/ on the column.sql_type).
---
Outside diff comments:
In `@spec/controllers/api/v1/donor_conditions_controller_spec.rb`:
- Around line 45-49: The test in donor_conditions_controller_spec.rb is assuming
order by comparing parsed_body['donor_conditions'].map { |condition|
condition['visible_to_donor'] } to DonorCondition.pluck(:visible_to_donor) which
can be flaky if controller index ordering differs; update the assertion in the
"returns 'visible_to_donor' in serialized response" example to be
order-independent by either using a multiset comparison (e.g. match_array)
between the two arrays or by explicitly sorting both arrays before comparing, or
alternatively assert each returned condition has a boolean visible_to_donor key
(using parsed_body and enumerating values) so the test no longer depends on
record order.
---
Nitpick comments:
In `@app/controllers/api/v1/deliveries_controller.rb`:
- Around line 175-191: The second fallback branch in
scheduled_at_from_delivery_params is redundant because delivery_attrs
(params.require(:delivery).permit!) already contains the same nested keys and
get_hash(parameters_to_plain_hash(delivery_attrs.to_unsafe_h)) will underscore
camelCase keys (via get_hash), so remove the whole fallback that reads from
parameters_to_plain_hash(params.to_unsafe_h) and its subsequent lookups;
instead, rely solely on raw_delivery (from delivery_attrs) to extract
schedule_attributes and scheduled_at (using the existing checks for
string/symbol keys), keeping the early return behaviour in
scheduled_at_from_delivery_params and preserving presence checks.
- Around line 215-226: The parameters_to_plain_hash helper is redundant on Rails
8 because ActionController::Parameters#to_unsafe_h already deeply converts
nested params; remove the parameters_to_plain_hash method and update any callers
to pass params.to_unsafe_h (or the specific ActionController::Parameters
instance .to_unsafe_h) directly into get_hash so get_hash continues to handle
key normalization and defensive access (ensure references to
parameters_to_plain_hash are replaced with .to_unsafe_h and delete the
parameters_to_plain_hash definition).
In `@spec/controllers/api/v1/deliveries_controller_spec.rb`:
- Around line 165-172: The Date.parse(...).to_date call is redundant because
Date.parse already returns a Date; update the spec to remove the unnecessary
.to_date by parsing schedule["scheduledAt"] into date with
Date.parse(schedule["scheduledAt"]) and keep the rest (the Holiday.is_holiday?
check and the Holiday.create! / rescue block) unchanged; ensure references
remain to the local variable date and existing methods Holiday.is_holiday? and
Holiday.create!.
- Around line 191-204: The test re-parses response.body into a local variable
`parsed` even though `subject` is already defined as `JSON.parse(response.body)`
for this spec; replace uses of the local `parsed` with `subject` in the "should
fail to modify the delivery if scheduled_at is nil" example (keep the existing
expects against `response.status` and the Gogovan/GogovanOrder mocks), i.e.
remove the `parsed = JSON.parse(response.body)` line and update subsequent
assertions to reference `subject["errors"]` and
`subject["errors"][0]["message"]` so the style matches adjacent tests.
In `@spec/models/concerns/package_filtering_spec.rb`:
- Around line 21-22: The two assertions use global counts (Package.count and
Package.apply_filter.count) instead of ID-scoped counts like the rest of the
file; update both to scope to the filtered package ids by replacing
Package.count and Package.apply_filter.count with Package.where(id:
`@fil_pkg_ids`).count and Package.apply_filter.where(id: `@fil_pkg_ids`).count
respectively so the expectations remain stable against external test-suite
package creation while still using `@package_filter_baseline` in the arithmetic.
🪄 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: b1051b39-d158-4455-a37d-bbbca3e15f69
📒 Files selected for processing (6)
app/controllers/api/v1/deliveries_controller.rbspec/controllers/api/v1/appointment_slots_controller_spec.rbspec/controllers/api/v1/deliveries_controller_spec.rbspec/controllers/api/v1/donor_conditions_controller_spec.rbspec/controllers/api/v1/orders_controller_spec.rbspec/models/concerns/package_filtering_spec.rb
| it 'Should use a timestamp column for slot times' do | ||
| column = AppointmentSlot.columns.find { |col| col.name == 'timestamp' } | ||
| expect(column).to_not be_nil | ||
| expect(column.sql_type).to eq("timestamp(6) with time zone") | ||
| expect(column.sql_type).to match(/\Atimestamp/) | ||
| end |
There was a problem hiding this comment.
Tighten the column-type assertion
match(/\Atimestamp/) also passes for timestamp with time zone, so this example no longer guards the schema behavior it is trying to pin. Please assert the without time zone form explicitly, while still allowing precision.
Suggested change
column = AppointmentSlot.columns.find { |col| col.name == 'timestamp' }
expect(column).to_not be_nil
- expect(column.sql_type).to match(/\Atimestamp/)
+ expect(column.sql_type).to match(/\Atimestamp(?:\(\d+\))? without time zone\z/)🤖 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/appointment_slots_controller_spec.rb` around lines
330 - 334, The test currently allows "timestamp with time zone"; tighten the
assertion in the spec for AppointmentSlot.columns (the example named 'Should use
a timestamp column for slot times') to explicitly require the "without time
zone" form while still permitting precision, e.g. change the sql_type
expectation to match a pattern like "timestamp" optionally followed by precision
and then " without time zone" (use a regex such as /\Atimestamp(?:\(\d+\))?
without time zone/ on the column.sql_type).
- Clear all AppointmentSlot rows in calendar-related before hooks so examples are not affected by committed data from other specs. - In deliveries modify-existing examples, use old_offer.created_by and align ggv_order offerId with old_offer so confirm_delivery expectations match. - Add :non_transactional metadata helper and use it in inventory_number_spec; document transactional fixtures and merge suite Time.zone with system user.
first_missing_code and max_code cast codes to integer; alphanumeric inventory_numbers rows (allowed by validation) caused PG errors when mixed with auto-generated numeric codes. Filter inventory_numbers with the same ^\d+$ pattern as packages.
Reset holidays for the scheduled HKT date before creating the fixture so full-suite order cannot leave validate_schedule passing and trigger Gogovan.cancel_order. Align the test date with Time.zone.parse like the controller.
Calendar expectations depend on AppointmentSlot.for_date and wrap_slot, which can be affected by committed 2018 HKT slots/holidays and appointment order_transports from other specs. Centralize a reset helper that clears 2018 fixture data and ensure the timezone example is self-contained.
- Orders controller: relax admin designations to include baseline IDs; summary matches controller merge; stock app allows non-GoodCity drafts. - Order model: clear orders in count-sensitive examples. - Organisation populator: assert fixture registrations only. - Organisation types index: include created fixtures vs exact table match. - ManageLocation and ValuationCalculator: use unique PackageType rows instead of factory first_or_create by code.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
spec/controllers/api/v1/orders_controller_spec.rb (1)
359-366: 💤 Low value
day_epoch_mstimezone handling is asymmetric withepoch_ms.For a
Date,Date#beginning_of_dayis materialized in the system local time zone before.in_time_zoneis applied — at that point the epoch is already fixed, so.in_time_zoneis effectively a no-op forto_i. IfTime.zonediffers from the system TZ (common in CI vs. local dev),epoch_msandday_epoch_mswill disagree about the sameDate. Consider zoning first, then takingbeginning_of_day:♻️ Proposed adjustment
def day_epoch_ms(time) case time - when Date, DateTime - time.beginning_of_day.in_time_zone.to_i * 1000 + when Date, DateTime + time.in_time_zone.beginning_of_day.to_i * 1000 else time.beginning_of_day.to_i * 1000 end 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/orders_controller_spec.rb` around lines 359 - 366, day_epoch_ms currently materializes beginning_of_day on a Date/DateTime in system local time then calls .in_time_zone, which can yield a different epoch than epoch_ms when Time.zone differs; change day_epoch_ms to apply the app timezone first (e.g. call time.in_time_zone or Time.zone.local conversion) and then call beginning_of_day and to_i so Date, DateTime and Time paths use the same zoning logic as epoch_ms (update the day_epoch_ms helper to zone-first for Date/DateTime and keep the else branch consistent).spec/controllers/api/v1/deliveries_controller_spec.rb (1)
198-212: 💤 Low valueMinor — reuse
subjectfor consistency.Lines 188-192 and 222-225 use the file-level
subject { JSON.parse(response.body) }; the new branch reintroducesparsed = JSON.parse(response.body). Switching tosubject['errors']keeps this assertion shape consistent with its siblings.🤖 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/deliveries_controller_spec.rb` around lines 198 - 212, The test re-parses response.body into a local parsed variable instead of using the file-level subject { JSON.parse(response.body) }; change the spec inside the "should fail to modify the delivery if scheduled_at is nil" example to use subject['errors'] (and subject directly) for assertions instead of parsed, keeping the existing expectations (Gogovan/GogovanOrder not to receive, post :confirm_delivery, response.status eq 422) but replace parsed["errors"].length and parsed["errors"][0]["message"] with subject['errors'].length and subject['errors'][0]['message'] to match surrounding tests that rely on subject.
🤖 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 `@spec/controllers/api/v1/deliveries_controller_spec.rb`:
- Around line 170-180: The WHERE clause using "date(holiday AT TIME ZONE 'HKT')"
is wrong because holidays.holiday is a timestamp without time zone; remove the
AT TIME ZONE cast and compare the stored timestamp's date directly (e.g. use
"date(holiday) = ?") or explicitly convert the parsed schedule date into the
same timezone as the stored value before comparing; update the
Holiday.where(...) call in the spec that builds date =
Time.zone.parse(schedule["scheduledAt"]).to_date so the delete_all matches the
Holiday.create!(..., holiday: date, ...) row by comparing date(holiday) = date
(or by normalizing both sides to UTC) instead of using AT TIME ZONE 'HKT'.
In `@spec/controllers/api/v1/organisation_types_controller_spec.rb`:
- Around line 23-25: The test uses include(*expected) which requires exact hash
equality and will fail if the serializer adds extra keys; update the assertion
in organisation_types_controller_spec (the expect on
response_body['organisation_types']) to match each expected hash as a subset
using RSpec's a_hash_including (i.e. transform each element of expected into
a_hash_including(expected_elem) and pass those to include) so the test asserts
presence of the specified keys/values without requiring exact equality.
In `@spec/lib/goodcity/organisation_populator_spec.rb`:
- Around line 18-19: The test reads fixture org_ids into the local variable
registrations but compares DB rows to the raw array length which can include
duplicates; change the assertion to use a deduplicated set (e.g., call uniq on
registrations) so the comparison uses unique org_ids when checking
Organisation.where(registration: registrations).count against
registrations.uniq.count.
---
Nitpick comments:
In `@spec/controllers/api/v1/deliveries_controller_spec.rb`:
- Around line 198-212: The test re-parses response.body into a local parsed
variable instead of using the file-level subject { JSON.parse(response.body) };
change the spec inside the "should fail to modify the delivery if scheduled_at
is nil" example to use subject['errors'] (and subject directly) for assertions
instead of parsed, keeping the existing expectations (Gogovan/GogovanOrder not
to receive, post :confirm_delivery, response.status eq 422) but replace
parsed["errors"].length and parsed["errors"][0]["message"] with
subject['errors'].length and subject['errors'][0]['message'] to match
surrounding tests that rely on subject.
In `@spec/controllers/api/v1/orders_controller_spec.rb`:
- Around line 359-366: day_epoch_ms currently materializes beginning_of_day on a
Date/DateTime in system local time then calls .in_time_zone, which can yield a
different epoch than epoch_ms when Time.zone differs; change day_epoch_ms to
apply the app timezone first (e.g. call time.in_time_zone or Time.zone.local
conversion) and then call beginning_of_day and to_i so Date, DateTime and Time
paths use the same zoning logic as epoch_ms (update the day_epoch_ms helper to
zone-first for Date/DateTime and keep the else branch consistent).
🪄 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: 5f8438cf-a2ae-4d5a-9db6-c57b59211e7e
📒 Files selected for processing (12)
app/models/inventory_number.rbspec/controllers/api/v1/appointment_slots_controller_spec.rbspec/controllers/api/v1/deliveries_controller_spec.rbspec/controllers/api/v1/orders_controller_spec.rbspec/controllers/api/v1/organisation_types_controller_spec.rbspec/lib/classes/manage_location_spec.rbspec/lib/goodcity/organisation_populator_spec.rbspec/models/concerns/valuation_calculator_spec.rbspec/models/inventory_number_spec.rbspec/models/order_spec.rbspec/rails_helper.rbspec/support/transactional_test_isolation.rb
| expected = organisation_types.map { |o| { 'name' => o.name_en, 'id' => o.id, 'category' => o.category_en } } | ||
| # The table may already contain seeded organisation types; assert our fixtures are present. | ||
| expect(response_body['organisation_types']).to include(*expected) |
There was a problem hiding this comment.
include(*expected) performs exact hash equality — use a_hash_including for subset matching
RSpec's array include matcher checks membership via ==, so each hash in expected must be exactly equal (same keys and values) to an element in the response. If the serializer returns any additional fields beyond name, id, and category (e.g. timestamps, extra attributes), none of the expected hashes will match and the test will produce a confusing false negative.
The intent of this change — tolerating extra seeded records — is correct, but the element-level comparison should use a_hash_including to be robust against serializer output changes:
🛠️ Proposed fix using a_hash_including
- expected = organisation_types.map { |o| { 'name' => o.name_en, 'id' => o.id, 'category' => o.category_en } }
- # The table may already contain seeded organisation types; assert our fixtures are present.
- expect(response_body['organisation_types']).to include(*expected)
+ # The table may already contain seeded organisation types; assert our fixtures are present.
+ expected = organisation_types.map { |o| a_hash_including('name' => o.name_en, 'id' => o.id, 'category' => o.category_en) }
+ expect(response_body['organisation_types']).to include(*expected)🤖 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/organisation_types_controller_spec.rb` around lines
23 - 25, The test uses include(*expected) which requires exact hash equality and
will fail if the serializer adds extra keys; update the assertion in
organisation_types_controller_spec (the expect on
response_body['organisation_types']) to match each expected hash as a subset
using RSpec's a_hash_including (i.e. transform each element of expected into
a_hash_including(expected_elem) and pass those to include) so the test asserts
presence of the specified keys/values without requiring exact equality.
| registrations = JSON.parse(file).map { |d| d["org_id"] } | ||
| expect(Organisation.where(registration: registrations).count).to eq(registrations.count) |
There was a problem hiding this comment.
Deduplicate fixture registrations before asserting count
Line 18 currently keeps duplicates, but Line 19 compares DB row count to raw array length. If the fixture ever includes repeated org_id, this test can fail even when behavior is correct.
Suggested change
- registrations = JSON.parse(file).map { |d| d["org_id"] }
- expect(Organisation.where(registration: registrations).count).to eq(registrations.count)
+ registrations = JSON.parse(file).map { |d| d["org_id"] }.uniq
+ expect(Organisation.where(registration: registrations).count).to eq(registrations.length)📝 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.
| registrations = JSON.parse(file).map { |d| d["org_id"] } | |
| expect(Organisation.where(registration: registrations).count).to eq(registrations.count) | |
| registrations = JSON.parse(file).map { |d| d["org_id"] }.uniq | |
| expect(Organisation.where(registration: registrations).count).to eq(registrations.length) |
🤖 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/lib/goodcity/organisation_populator_spec.rb` around lines 18 - 19, The
test reads fixture org_ids into the local variable registrations but compares DB
rows to the raw array length which can include duplicates; change the assertion
to use a deduplicated set (e.g., call uniq on registrations) so the comparison
uses unique org_ids when checking Organisation.where(registration:
registrations).count against registrations.uniq.count.
CircleCI parallelism: 3 runs three containers against one Postgres; shared goodcity_server_test caused races (appointment_slots and other DB-heavy specs). Set DATABASE_URL with CIRCLE_NODE_INDEX for db:migrate, cucumber, and rspec so each node has an isolated database.
Remove the 2018 HKT reset helper and OrderTransport cleanup; parallel CI containers use separate test databases, so cross-worker slot/booking races are handled in .circleci without this extra before-hook.
Stub Holiday.is_holiday? for the fixture date after seeding so validate_schedule always rejects holiday bookings before delete_old_associations runs (avoids Gogovan.cancel_order when DB ordering leaves Holiday lookup flaky).
…cleanup - Add describe-level after Timecop.return so nested freezes do not leak between sibling contexts (Order.priority uses Time.zone.now). - Clear awaiting_dispatch orders before awaiting-dispatch examples. - Drop redundant Timecop.return from processing-only context.
- Add reset_hkt_2018_calendar_test_data to clear presets, slots, HKT 2018 holidays, and appointment order_transports that skew for_date/wrap_slot - Reuse helper in Supervisor and timezone contexts; make Dec 19 example self-contained with full slot reset + presets after POST setup
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
spec/controllers/api/v1/deliveries_controller_spec.rb (1)
173-174:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnresolved:
AT TIME ZONE 'HKT'on atimestamp without time zonecolumn still misaligns the cleanup query.The past review on this hunk pointed out that
holidays.holidayistimestamp without time zone, soAT TIME ZONE 'HKT'reinterprets the naive value as HKT and converts it to UTC, shifting the extracted date by up to one day relative to the row inserted on lines 175–179 (whereholiday: datewrites midnight in the app/DB default zone). The result is that thedelete_allwill not match the row this samebeforeblock just created in many sessions, so the "reset only this date" guard against pre-existing rows from other specs is not actually doing what the comment claims. The downstreamHoliday.is_holiday?stub (lines 182–186) hides this in the example under test, but any code path that hits the DB for that date still sees stale rows.Either drop the cast and compare on the stored value directly, or normalize both sides to the same zone:
🛠️ Suggested fix
- date = Time.zone.parse(schedule["scheduledAt"]).to_date - Holiday.where("date(holiday AT TIME ZONE 'HKT') = ?", date).delete_all + date = Time.zone.parse(schedule["scheduledAt"]).to_date + Holiday.where("date(holiday) = ?", date).delete_all#!/bin/bash # Confirm the column type for holidays.holiday is still `timestamp without time zone` # (i.e., the past-review premise still holds in the current schema). fd -t f schema.rb db rg -nP -C2 'create_table\s+["\:]holidays' db rg -nP -C2 't\.(datetime|timestamp|date)\s+["\:]holiday\b' db rg -nP -C2 'change_column\s+:holidays\s*,\s*:holiday' db🤖 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/deliveries_controller_spec.rb` around lines 173 - 174, The cleanup query misuses "AT TIME ZONE 'HKT'" against a timestamp without time zone and thus shifts the date; change the Holiday.where(...) call to compare the stored timestamp's date directly (e.g. use "date(holiday) = ?" or "holiday::date = ?") or normalize both sides to the same zone before comparing so the newly created row (in this before block) is actually matched; update the Holiday.where("date(holiday AT TIME ZONE 'HKT') = ?", date).delete_all invocation accordingly and keep the Holiday.is_holiday? stub as-is.
🧹 Nitpick comments (2)
.circleci/config.yml (2)
56-58: 💤 Low value
POSTGRES_DB: goodcity_server_testis now dead configuration.With the per-node scheme, every node connects to
goodcity_server_test_<index>and creates it viarails db:create. Thegoodcity_server_testDB that the Postgres image auto-creates from this env var is never used, which is mildly misleading for anyone reading the config. Consider dropping it (and optionally documenting the per-node naming next to the comment block on lines 62–64).♻️ Proposed cleanup
- image: cimg/postgres:9.6 environment: POSTGRES_USER: postgres - POSTGRES_DB: goodcity_server_test🤖 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 @.circleci/config.yml around lines 56 - 58, Remove the dead POSTGRES_DB env var entry (POSTGRES_DB: goodcity_server_test) from the CircleCI job environment and, in its place or nearby, add a brief comment explaining the per-node naming scheme (goodcity_server_test_<index>) and that each node creates its own DB via rails db:create so the auto-created goodcity_server_test is unused; update any related references to avoid confusion.
82-100: ⚡ Quick winDRY the per-node
DATABASE_URLviaBASH_ENVinstead of re-exporting in every step.The same
export DATABASE_URL=...line is repeated in the newSetup test database,Cucumber tests, andRspec testssteps. If the suffix scheme ever changes (e.g., adding a_$RAILS_ENVsegment, switching ports, or adding credentials), three places must stay in sync. CircleCI sources$BASH_ENVfor everyrunstep, so setting it once eliminates the duplication and reduces drift risk.♻️ Proposed refactor
- run: name: Wait for DB and Redis containers to start command: dockerize -wait tcp://localhost:5432 -wait tcp://localhost:6379 -timeout 1m + - run: + name: Configure per-node DATABASE_URL + command: | + echo 'export DATABASE_URL="postgres://postgres@localhost:5432/goodcity_server_test_${CIRCLE_NODE_INDEX:-0}"' >> "$BASH_ENV" - run: name: Setup test database (per parallel container) command: | - export DATABASE_URL="postgres://postgres@localhost:5432/goodcity_server_test_${CIRCLE_NODE_INDEX:-0}" bundle exec rails db:create db:migrate - run: name: Cucumber tests command: | - export DATABASE_URL="postgres://postgres@localhost:5432/goodcity_server_test_${CIRCLE_NODE_INDEX:-0}" bundle exec cucumber - run: name: Rspec tests command: | - export DATABASE_URL="postgres://postgres@localhost:5432/goodcity_server_test_${CIRCLE_NODE_INDEX:-0}" mkdir -p ~/rspec bundle exec rspec --format progress \🤖 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 @.circleci/config.yml around lines 82 - 100, Replace the repeated per-step export of DATABASE_URL by defining it once in the CircleCI environment that is sourced for every run step: write the computed DATABASE_URL into $BASH_ENV (so all run steps inherit it) and remove the duplicate `export DATABASE_URL="postgres://postgres@localhost:5432/goodcity_server_test_${CIRCLE_NODE_INDEX:-0}"` lines from the "Setup test database", "Cucumber tests", and "Rspec tests" run steps; reference the variable name DATABASE_URL, the special file $BASH_ENV, and the three run step names to locate and update the config.
🤖 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 @.circleci/config.yml:
- Line 43: Replace the pinned Bundler version used in CI install steps so it
matches Gemfile.lock: update each instance of the step string "- run: gem
install bundler:2.3.27" to install bundler 2.6.9 instead (there are three
occurrences of that run command in the CI config); ensure all CI install steps
use "gem install bundler:2.6.9" so Bundler version aligns with BUNDLED WITH
2.6.9.
- Line 55: Replace the outdated CI PostgreSQL image reference
"cimg/postgres:9.6" in the .circleci/config.yml with a supported version that
matches the project's README and Rails 8.1 requirements (for example
"cimg/postgres:14"); update the image tag value where "image: cimg/postgres:9.6"
appears so CI runs against PostgreSQL 12+ (preferably 14 to match the
doc/production version).
In `@spec/controllers/api/v1/appointment_slots_controller_spec.rb`:
- Around line 335-337: This test duplicates default presets by creating weekday
presets again; either remove the (1..7).each FactoryBot.create
:appointment_slot_preset loop or clear existing presets first (e.g.
AppointmentSlotPreset.unscoped.delete_all) before creating new ones so the
example doesn't change shared describe-level state—locate the block using
AppointmentSlot.unscoped.delete_all and the FactoryBot.create
:appointment_slot_preset loop and apply one of these fixes.
- Around line 339-340: The test currently posts to create an appointment slot
via post :create but never verifies it succeeded; update the spec to assert the
creation before calling get :calendar by checking the POST response (e.g.
expect(response).to have_http_status(:created) or :success) or by asserting a
record change around the call to post :create (e.g. expect { post :create,
params: ... }.to change(AppointmentSlot, :count).by(1)), then proceed to call
get :calendar and assert the calendar contents; reference the existing post
:create and get :calendar calls to locate where to add the assertion.
In `@spec/models/order_spec.rb`:
- Line 328: The test uses Order.delete_all which bypasses callbacks and
dependent cleanup and can leave orphaned rows or FK failures; replace
Order.delete_all with Order.destroy_all or, better, scope the cleanup to only
the records this example group created (e.g., use the same factory IDs or
created_at range used in this spec) so associated models like orders_packages,
goodcity_requests, and order_transport are cleaned via their callbacks; also
update the other occurrences noted (the similar calls referenced near the other
examples) to follow the same pattern.
- Around line 63-71: The spec removed type checks for timestamp columns; update
the expectations in spec/models/order_spec.rb to assert the columns are datetime
by adding .of_type(:datetime) to each timestamp column check (created_at,
updated_at, dispatch_started_at, cancelled_at, process_completed_at,
processed_at) so each it{ is_expected.to have_db_column(:created_at) } style
line becomes an assertion that includes .of_type(:datetime); leave the integer
checks for *_by_id columns unchanged (dispatch_started_by_id, cancelled_by_id,
process_completed_by_id).
---
Duplicate comments:
In `@spec/controllers/api/v1/deliveries_controller_spec.rb`:
- Around line 173-174: The cleanup query misuses "AT TIME ZONE 'HKT'" against a
timestamp without time zone and thus shifts the date; change the
Holiday.where(...) call to compare the stored timestamp's date directly (e.g.
use "date(holiday) = ?" or "holiday::date = ?") or normalize both sides to the
same zone before comparing so the newly created row (in this before block) is
actually matched; update the Holiday.where("date(holiday AT TIME ZONE 'HKT') =
?", date).delete_all invocation accordingly and keep the Holiday.is_holiday?
stub as-is.
---
Nitpick comments:
In @.circleci/config.yml:
- Around line 56-58: Remove the dead POSTGRES_DB env var entry (POSTGRES_DB:
goodcity_server_test) from the CircleCI job environment and, in its place or
nearby, add a brief comment explaining the per-node naming scheme
(goodcity_server_test_<index>) and that each node creates its own DB via rails
db:create so the auto-created goodcity_server_test is unused; update any related
references to avoid confusion.
- Around line 82-100: Replace the repeated per-step export of DATABASE_URL by
defining it once in the CircleCI environment that is sourced for every run step:
write the computed DATABASE_URL into $BASH_ENV (so all run steps inherit it) and
remove the duplicate `export
DATABASE_URL="postgres://postgres@localhost:5432/goodcity_server_test_${CIRCLE_NODE_INDEX:-0}"`
lines from the "Setup test database", "Cucumber tests", and "Rspec tests" run
steps; reference the variable name DATABASE_URL, the special file $BASH_ENV, and
the three run step names to locate and update the config.
🪄 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: 4fcb40da-fc70-4348-911c-bb4f916c25e1
📒 Files selected for processing (4)
.circleci/config.ymlspec/controllers/api/v1/appointment_slots_controller_spec.rbspec/controllers/api/v1/deliveries_controller_spec.rbspec/models/order_spec.rb
| post :create, params: { appointment_slot: { quota: 0, timestamp: "2018-12-19T16:00:00.000Z", notes: "Closed on the 20th of december" } } | ||
| get :calendar, params: { from: '2018-12-19', to: '2018-12-21' } |
There was a problem hiding this comment.
Assert the slot creation succeeded before checking the calendar.
Right now this spec never proves the POST created the closing slot, so the calendar assertions are less diagnostic if the request starts returning 4xx/5xx.
Suggested fix
post :create, params: { appointment_slot: { quota: 0, timestamp: "2018-12-19T16:00:00.000Z", notes: "Closed on the 20th of december" } }
+ expect(response).to have_http_status(:created)
get :calendar, params: { from: '2018-12-19', to: '2018-12-21' }📝 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.
| post :create, params: { appointment_slot: { quota: 0, timestamp: "2018-12-19T16:00:00.000Z", notes: "Closed on the 20th of december" } } | |
| get :calendar, params: { from: '2018-12-19', to: '2018-12-21' } | |
| post :create, params: { appointment_slot: { quota: 0, timestamp: "2018-12-19T16:00:00.000Z", notes: "Closed on the 20th of december" } } | |
| expect(response).to have_http_status(:created) | |
| get :calendar, params: { from: '2018-12-19', to: '2018-12-21' } |
🤖 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/appointment_slots_controller_spec.rb` around lines
339 - 340, The test currently posts to create an appointment slot via post
:create but never verifies it succeeded; update the spec to assert the creation
before calling get :calendar by checking the POST response (e.g.
expect(response).to have_http_status(:created) or :success) or by asserting a
record change around the call to post :create (e.g. expect { post :create,
params: ... }.to change(AppointmentSlot, :count).by(1)), then proceed to call
get :calendar and assert the calendar contents; reference the existing post
:create and get :calendar calls to locate where to add the assertion.
- Run rails db:reset between Cucumber and RSpec so each parallel node gets a clean schema plus seeds before RSpec (avoids committed feature data leaking). - Load seed YAML with safe_load (Psych::Omap, Symbol, aliases) so db:seed works under strict Psych. - Upsert CannedResponse by guid so db:seed is idempotent (unique guid validation).
Use db:drop db:create db:migrate instead of db:reset so we get a clean schema without running db:seed in the test job.
Use db:schema:load when recreating the per-node test database after Cucumber for a clean, fast reset before RSpec.
- Convert stored UTC timestamps to HK local date in SQL for AppointmentSlot.for_date and Holiday.is_holiday? so afternoon slots don't bucket to the prior day. - Update specs to use the same UTC→HK conversion when cleaning 2018 data and make the PUT timestamp-conflict example resilient to prior created slots.
Avoid assuming user id 1 is missing; derive a guaranteed-missing id from (User.maximum(:id) + 1) so the job spec is stable in full-suite runs.
- Avoid assuming a clean DB: clear Packages in the simple index serialization example, use change(...).by(1) for split_package. - Avoid assuming package_id=1 is missing in print_barcode; compute a guaranteed missing id. - Make box/pallet GoodcitySetting fixtures idempotent by deleting existing keys before creating them (prevents uniqueness failures in randomized runs).
Assert Order timestamp columns are datetime types (created_at, updated_at, dispatch_started_at, cancelled_at, process_completed_at, processed_at) while leaving *_by_id integer expectations unchanged.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.circleci/config.yml (1)
43-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBundler 2.3.27 still does not match
Gemfile.lock(BUNDLED WITH 2.6.9).All three install sites (lines 43, 78, 123) still pin
bundler:2.3.27while the lockfile in this PR isBUNDLED WITH 2.6.9. Installing an older Bundler than what generated the lockfile will trigger a Bundler downgrade warning at minimum, and on Ruby 3.4.9 can fail thebundle installoutright. Bump all three to2.6.9(or whatever valueGemfile.lockrecords on this branch).🔧 Proposed fix
- - run: gem install bundler:2.3.27 + - run: gem install bundler:2.6.9- - run: gem install bundler:2.3.27 && bundle config set --local deployment 'true' && (bundle check || bundle install --jobs=2 --retry=3) + - run: gem install bundler:2.6.9 && bundle config set --local deployment 'true' && (bundle check || bundle install --jobs=2 --retry=3)(applies to lines 43, 78, and 123)
Also applies to: 78-78, 123-123
🤖 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 @.circleci/config.yml at line 43, Update all occurrences of the hardcoded Bundler version in the CI steps that currently run "gem install bundler:2.3.27" to match the Gemfile.lock's BUNDLED WITH version (2.6.9); specifically replace each "gem install bundler:2.3.27" invocation (the three run steps that install bundler) with "gem install bundler:2.6.9" (or the exact version shown in Gemfile.lock) so the CI installs the same Bundler used to generate the lockfile.
🧹 Nitpick comments (4)
.circleci/config.yml (2)
82-105: 💤 Low valueOptional: DRY the per-node
DATABASE_URLby exporting it once toBASH_ENV.The same
export DATABASE_URL=...line is repeated in four steps. CircleCI persists anything written to$BASH_ENVacross subsequentrunsteps in the same job, so a single setup step keeps the per-node URL in one place and avoids drift if the naming scheme changes later.♻️ Proposed refactor
- run: name: Wait for DB and Redis containers to start command: dockerize -wait tcp://localhost:5432 -wait tcp://localhost:6379 -timeout 1m + - run: + name: Configure per-node DATABASE_URL + command: | + echo 'export DATABASE_URL="postgres://postgres@localhost:5432/goodcity_server_test_${CIRCLE_NODE_INDEX:-0}"' >> "$BASH_ENV" - run: name: Setup test database (per parallel container) command: | - export DATABASE_URL="postgres://postgres@localhost:5432/goodcity_server_test_${CIRCLE_NODE_INDEX:-0}" bundle exec rails db:create db:migrate - run: name: Cucumber tests command: | - export DATABASE_URL="postgres://postgres@localhost:5432/goodcity_server_test_${CIRCLE_NODE_INDEX:-0}" bundle exec cucumber - run: name: Recreate test database after Cucumber command: | - export DATABASE_URL="postgres://postgres@localhost:5432/goodcity_server_test_${CIRCLE_NODE_INDEX:-0}" bundle exec rails db:drop db:create db:schema:load - run: name: Rspec tests command: | - export DATABASE_URL="postgres://postgres@localhost:5432/goodcity_server_test_${CIRCLE_NODE_INDEX:-0}" mkdir -p ~/rspec bundle exec rspec --format progress \🤖 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 @.circleci/config.yml around lines 82 - 105, Create a single preparatory run step that writes the per-node DATABASE_URL into $BASH_ENV (using the same postgres://postgres@localhost:5432/goodcity_server_test_${CIRCLE_NODE_INDEX:-0} expression) so subsequent steps inherit it, then remove the repeated export lines from the steps named "Setup test database (per parallel container)", "Cucumber tests", "Recreate test database after Cucumber", and "Rspec tests"; ensure the new setup step runs before those four steps so the environment variable is available throughout the job.
82-96: ⚡ Quick winConsider using consistent database reset strategy: both should use
db:schema:loador both should usedb:migrate.The setup step uses
db:create db:migratewhile the recreate step usesdb:drop db:create db:schema:load. Currentlydb/schema.rbis in sync with migrations, so both approaches load an identical schema state. However, using the same method for both maintains consistency and prevents any risk of schema drift if a migration is added without regenerating schema.rb. Usingdb:schema:loadfor both is slightly faster and more explicit about testing against the committed schema state.Optional refactoring suggestion
- run: name: Setup test database (per parallel container) command: | export DATABASE_URL="postgres://postgres@localhost:5432/goodcity_server_test_${CIRCLE_NODE_INDEX:-0}" - bundle exec rails db:create db:migrate + bundle exec rails db:create db:schema:load🤖 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 @.circleci/config.yml around lines 82 - 96, The setup step uses "bundle exec rails db:create db:migrate" while the recreate step uses "bundle exec rails db:drop db:create db:schema:load", causing inconsistency; change the setup command used in the "Setup test database (per parallel container)" run to use db:schema:load instead of db:migrate (keeping the same DATABASE_URL export) so both setup and recreate use db:schema:load and ensure tests run against the committed schema.rb.spec/models/order_spec.rb (1)
362-364: 💤 Low valueRedundant
after { Timecop.return }— the top-level hook already covers this.The
after { Timecop.return }at line 23 applies to every example in the entireRSpec.describe Orderblock, including all examples nested inside'priority rules'. The duplicate hook at line 364 is a harmless no-op, but the comment on lines 362–363 ("without a top-level return, frozen time leaks") is now misleading — the top-level return exists.🧹 Suggested cleanup
- # Several nested contexts call Timecop.freeze; without a top-level return, frozen time leaks - # between sibling examples and breaks Order.priority (last_6pm / one_day_ago use Time.zone.now). - after { Timecop.return } - context 'A submitted order' do🤖 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/models/order_spec.rb` around lines 362 - 364, Remove the redundant after { Timecop.return } hook inside the 'priority rules' context in spec/models/order_spec.rb: the top-level RSpec.describe Order already calls Timecop.return for every example, so delete this duplicate hook and update or remove the misleading comment about needing a top-level return (ensure references to Timecop.freeze/return and Order.priority remain accurate).db/seeds.rb (1)
41-47: 💤 Low valueNice idempotency improvement for canned responses.
Switching to
find_or_initialize_by(guid:)+assign_attributes+save!makes re-runningdb:seedsafe for this collection, matches the PR's "make db seeding idempotent" goal, and surfaces validation errors viasave!. Note that several other blocks in this file (e.g.DonorCondition.create,RejectionReason.create,BookingType.create,Holiday.createat lines 140–141,IdentityType.create,Country.create) still use plaincreateand will keep duplicating rows on re-seed — consider extending the same pattern there in a follow-up if full idempotency is the goal.🤖 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 `@db/seeds.rb` around lines 41 - 47, Several seed blocks still use plain create (e.g., DonorCondition.create, RejectionReason.create, BookingType.create, Holiday.create, IdentityType.create, Country.create) which will duplicate rows on re-seed; update each of these sections to follow the canned_responses pattern by finding or initializing by the unique identifier (e.g., guid or name), calling assign_attributes(...) with the sanitized attrs hash (like h = attrs.stringify_keys), and then save! so seeding becomes idempotent and validation errors surface.
🤖 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/appointment_slot.rb`:
- Line 12: The upcoming scope uses DateTime.now which ignores Time.zone; update
the boundary to use Rails time so the day boundary respects the app timezone —
in appointment_slot.rb change the scope :upcoming lambda to compute the cutoff
with Time.current.beginning_of_day (e.g.,
Time.current.beginning_of_day.utc.to_fs(:db) or
Time.zone.now.beginning_of_day.utc.to_fs(:db)) instead of
DateTime.now.beginning_of_day.utc.to_fs(:db) so the scope uses the configured
Time.zone.
In `@app/models/holiday.rb`:
- Around line 13-18: The is_holiday? class method on Holiday currently uses
Holiday.where(...).count > 0 which is inefficient; change it to use
Holiday.where("date((holiday AT TIME ZONE 'UTC') AT TIME ZONE 'Asia/Hong_Kong')
= ?", date.to_date).exists? to perform an existence check without counting all
rows, keeping the existing timezone SQL expression and the date.to_date
conversion in place.
---
Duplicate comments:
In @.circleci/config.yml:
- Line 43: Update all occurrences of the hardcoded Bundler version in the CI
steps that currently run "gem install bundler:2.3.27" to match the
Gemfile.lock's BUNDLED WITH version (2.6.9); specifically replace each "gem
install bundler:2.3.27" invocation (the three run steps that install bundler)
with "gem install bundler:2.6.9" (or the exact version shown in Gemfile.lock) so
the CI installs the same Bundler used to generate the lockfile.
---
Nitpick comments:
In @.circleci/config.yml:
- Around line 82-105: Create a single preparatory run step that writes the
per-node DATABASE_URL into $BASH_ENV (using the same
postgres://postgres@localhost:5432/goodcity_server_test_${CIRCLE_NODE_INDEX:-0}
expression) so subsequent steps inherit it, then remove the repeated export
lines from the steps named "Setup test database (per parallel container)",
"Cucumber tests", "Recreate test database after Cucumber", and "Rspec tests";
ensure the new setup step runs before those four steps so the environment
variable is available throughout the job.
- Around line 82-96: The setup step uses "bundle exec rails db:create
db:migrate" while the recreate step uses "bundle exec rails db:drop db:create
db:schema:load", causing inconsistency; change the setup command used in the
"Setup test database (per parallel container)" run to use db:schema:load instead
of db:migrate (keeping the same DATABASE_URL export) so both setup and recreate
use db:schema:load and ensure tests run against the committed schema.rb.
In `@db/seeds.rb`:
- Around line 41-47: Several seed blocks still use plain create (e.g.,
DonorCondition.create, RejectionReason.create, BookingType.create,
Holiday.create, IdentityType.create, Country.create) which will duplicate rows
on re-seed; update each of these sections to follow the canned_responses pattern
by finding or initializing by the unique identifier (e.g., guid or name),
calling assign_attributes(...) with the sanitized attrs hash (like h =
attrs.stringify_keys), and then save! so seeding becomes idempotent and
validation errors surface.
In `@spec/models/order_spec.rb`:
- Around line 362-364: Remove the redundant after { Timecop.return } hook inside
the 'priority rules' context in spec/models/order_spec.rb: the top-level
RSpec.describe Order already calls Timecop.return for every example, so delete
this duplicate hook and update or remove the misleading comment about needing a
top-level return (ensure references to Timecop.freeze/return and Order.priority
remain accurate).
🪄 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: fc01e213-f4ce-4a1c-93b6-d11df4e521c8
📒 Files selected for processing (9)
.circleci/config.ymlapp/models/appointment_slot.rbapp/models/holiday.rbdb/seeds.rbspec/controllers/api/v1/appointment_slots_controller_spec.rbspec/controllers/api/v1/deliveries_controller_spec.rbspec/controllers/api/v1/packages_controller_spec.rbspec/jobs/user_safe_delete_job_spec.rbspec/models/order_spec.rb
| push_targets [ Channel::STOCK_CHANNEL ] | ||
|
|
||
| scope :upcoming, -> { where("timestamp >= ?", DateTime.now.beginning_of_day.utc.to_s(:db)) } | ||
| scope :upcoming, -> { where("timestamp >= ?", DateTime.now.beginning_of_day.utc.to_fs(:db)) } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "appointment_slot.rb" | head -5Repository: crossroads/api.goodcity
Length of output: 98
🏁 Script executed:
cat -n app/models/appointment_slot.rb | head -50Repository: crossroads/api.goodcity
Length of output: 2158
🏁 Script executed:
rg "Time\.zone|Time\.current|DateTime\.now" app/models/ -A 2 -B 2Repository: crossroads/api.goodcity
Length of output: 4014
🌐 Web query:
Rails DateTime.now vs Time.current timezone behavior
💡 Result:
Rails time zone behavior differs primarily because Time.current and DateTime.current are Rails time-zone aware, while Time.now/DateTime.now are not. 1) Time.current (Rails) vs Time.now (Ruby) - In Rails, Time.current returns Time.zone.now when Time.zone / config.time_zone is set; otherwise it falls back to Time.now. [1][2] - Time.now is standard Ruby time and uses the system/server environment timezone, not Rails’ configured Time.zone. (By contrast with Rails’ definition of Time.current.) [1] 2) DateTime.current vs DateTime.now - In Rails, DateTime.current returns Time.zone.now.to_datetime when Time.zone / config.time_zone is set; otherwise it falls back to Time.now.to_datetime. [3][4] - DateTime.now is standard Ruby and is not Rails time-zone aware (it does not use Time.zone). (This is implied by Rails’ explicit conditional implementation of DateTime.current.) [3] 3) Practical implications for “timezone behavior” - If your Rails app sets config.time_zone (so Time.zone is present), then: - Time.current and DateTime.current reflect the Rails application time zone (including DST rules for that zone). [1][3] - Time.now and DateTime.now reflect the system timezone, which can differ from the Rails app’s configured zone. [1][3] - If Time.zone is not set in Rails, then Time.current and DateTime.current reduce to the non-zone-aware versions (Time.now / Time.now.to_datetime). [1][3] 4) UTC vs local zone - Calling .utc on any of these values will convert the instant to UTC for that object, but the key difference is what zone-aware instant you started from (Rails zone via *.current vs system zone via *.now). The Rails APIs explicitly base *.current on Time.zone.now. [1][3] Recommendation - Prefer Time.current (and DateTime.current / Time.zone.now) for “current time” in a Rails app where you rely on config.time_zone and DST-correct calculations. [1][3] - Prefer Time.now / DateTime.now only when you specifically want the system’s notion of “now,” not Rails’ configured Time.zone. [1][3]
Citations:
- 1: https://api.rubyonrails.org/v7.0/classes/Time.html
- 2: https://www.rubydoc.info/gems/activesupport/Time
- 3: https://api.rubyonrails.org/classes/DateTime.html
- 4: https://apidock.com/rails/DateTime/current/class
Use Rails time for the upcoming scope boundary.
DateTime.now ignores Time.zone, so this boundary flips at system midnight instead of the application's configured timezone (e.g., Hong Kong). This can hide same-day morning slots for several hours when the server runs in UTC. The codebase consistently uses Time.current elsewhere (e.g., holiday.rb uses Time.zone.now.beginning_of_day for similar logic).
Suggested fix
- scope :upcoming, -> { where("timestamp >= ?", DateTime.now.beginning_of_day.utc.to_fs(:db)) }
+ scope :upcoming, -> { where("timestamp >= ?", Time.current.beginning_of_day.utc) }🤖 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/appointment_slot.rb` at line 12, The upcoming scope uses
DateTime.now which ignores Time.zone; update the boundary to use Rails time so
the day boundary respects the app timezone — in appointment_slot.rb change the
scope :upcoming lambda to compute the cutoff with Time.current.beginning_of_day
(e.g., Time.current.beginning_of_day.utc.to_fs(:db) or
Time.zone.now.beginning_of_day.utc.to_fs(:db)) instead of
DateTime.now.beginning_of_day.utc.to_fs(:db) so the scope uses the configured
Time.zone.
| def self.is_holiday?(date) | ||
| Holiday.where(" date(holiday AT TIME ZONE 'HKT') = ?", date.to_date).count > 0 | ||
| # Use IANA TZ name instead of abbreviation (Postgres 9.6 on CI can treat 'HKT' inconsistently). | ||
| Holiday.where( | ||
| "date((holiday AT TIME ZONE 'UTC') AT TIME ZONE 'Asia/Hong_Kong') = ?", | ||
| date.to_date | ||
| ).count > 0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the holiday.rb file to confirm the current state
cat -n app/models/holiday.rb | head -30Repository: crossroads/api.goodcity
Length of output: 894
🏁 Script executed:
# Search for usages of is_holiday? to understand how it's called
rg "is_holiday\?" --type ruby -B 2 -A 2Repository: crossroads/api.goodcity
Length of output: 2020
🏁 Script executed:
# Check how scheduled_date is defined/used in the deliveries controller
rg "scheduled_date" app/controllers/api/v1/deliveries_controller.rb -B 3 -A 3Repository: crossroads/api.goodcity
Length of output: 766
🏁 Script executed:
# Check Rails time zone configuration
rg "config\.time_zone\|Time\.zone" config/ app/ --type ruby | head -20Repository: crossroads/api.goodcity
Length of output: 49
🏁 Script executed:
# Verify the test setup to understand what time zones are being passed
rg "is_holiday\?" spec/controllers/api/v1/deliveries_controller_spec.rb -B 5 -A 10Repository: crossroads/api.goodcity
Length of output: 1356
🏁 Script executed:
# Get the full scheduled_date method definition
rg "def scheduled_date" app/controllers/api/v1/deliveries_controller.rb -A 20Repository: crossroads/api.goodcity
Length of output: 971
🏁 Script executed:
# Check what Rails time zone is configured
rg "config\.time_zone" --type ruby -r '$0' | head -10Repository: crossroads/api.goodcity
Length of output: 124
🏁 Script executed:
# Also check initializers and environment configs
fd -e rb config/ | xargs grep -l "time_zone\|Time\.zone" 2>/dev/nullRepository: crossroads/api.goodcity
Length of output: 403
🏁 Script executed:
# Comprehensive search for all calls to is_holiday? to verify all usage patterns
rg "\.is_holiday\?" --type ruby -B 1 -A 1Repository: crossroads/api.goodcity
Length of output: 960
Replace .count > 0 with .exists? for efficiency.
The method signature accepts a flexible date argument that calls .to_date on it. However, in current usage (scheduled_date from the controller), the argument is already a Date object extracted via Time.zone.parse(...).to_date where Time.zone is configured as 'Hong Kong'. The timezone normalization concern does not apply to actual callers in this codebase.
Consider the performance improvement:
def self.is_holiday?(date)
Holiday.where(
"date((holiday AT TIME ZONE 'UTC') AT TIME ZONE 'Asia/Hong_Kong') = ?",
date.to_date
- ).count > 0
+ ).exists?
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.
| def self.is_holiday?(date) | |
| Holiday.where(" date(holiday AT TIME ZONE 'HKT') = ?", date.to_date).count > 0 | |
| # Use IANA TZ name instead of abbreviation (Postgres 9.6 on CI can treat 'HKT' inconsistently). | |
| Holiday.where( | |
| "date((holiday AT TIME ZONE 'UTC') AT TIME ZONE 'Asia/Hong_Kong') = ?", | |
| date.to_date | |
| ).count > 0 | |
| def self.is_holiday?(date) | |
| # Use IANA TZ name instead of abbreviation (Postgres 9.6 on CI can treat 'HKT' inconsistently). | |
| Holiday.where( | |
| "date((holiday AT TIME ZONE 'UTC') AT TIME ZONE 'Asia/Hong_Kong') = ?", | |
| date.to_date | |
| ).exists? | |
| 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 `@app/models/holiday.rb` around lines 13 - 18, The is_holiday? class method on
Holiday currently uses Holiday.where(...).count > 0 which is inefficient; change
it to use Holiday.where("date((holiday AT TIME ZONE 'UTC') AT TIME ZONE
'Asia/Hong_Kong') = ?", date.to_date).exists? to perform an existence check
without counting all rows, keeping the existing timezone SQL expression and the
date.to_date conversion in place.
1 participant
Builds on #1337 upgrades Rails to 8.1 and Ruby to 3.4.9
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests