Conversation
- Pin minitest to < 6: Rails 7.1 is incompatible with minitest 6.0 due to a changed run() signature in line_filtering.rb. Fix ships in Rails 8.1.2+ — pin can be removed when we reach that version. - Revert show_exceptions to :none in test.rb; :rescuable was incorrectly accepted from rails app:update and breaks controller tests that expect exceptions to propagate. - Comment out raise_on_missing_callback_actions = true; this exposes a real bug in OrderServicesController (handle_remember_params callback references non-existent index action) — to be fixed separately before re-enabling. - Update log controller test assertions: rails-dom-testing 2.3.0 normalizes whitespace in text assertions, replacing \n between block elements with a space.
In earlier versions, assignments to attr_readonly attributes where ignored silently. Plannings::CustomListsControllerTest sent read_only 'item_type' on update
With marshalling_format_version = 7.1, AR objects serialized to session lose their dirty tracking state on deserialization. Splitable#save reloads existing records from DB before saving so dirty tracking is correctly restored.
Ignores a temporary warning; to be removed later.
Rails 7.1 defaults this to false, meaning autoloaded paths are no longer added to $LOAD_PATH. The initializers used require 'error_tracker' which relied on $LOAD_PATH containing app/domain/. Since config/initializers run before Zeitwerk's setup_main_autoloader, we can't rely on autoloading either. Switch to require_relative so the load is explicit and boot-order independent.
The 7.0 defaults file was holding back the SHA256 default that comes with load_defaults 7.1. No encrypted data in the DB relies on this key — Devise passwords use bcrypt independently. Only impact: existing session/remember-me cookies are invalidated, i.e. logged-in users are signed out once on next request after deploy.
All 7.1 defaults are active via config.load_defaults 7.1 in application.rb. The file contained only comments — no active configuration remaining.
Move secret_key_base from deprecated config/secrets.yml into the environment config files directly. Rails 7.1 deprecated Rails.application.secrets in favor of credentials; since this app has no credentials set up, falling back to secrets.yml triggered deprecation warnings (raised as errors in test/development). Remove config/secrets.yml as it only contained secret_key_base.
- Bump rails to ~> 7.2.0, update load_defaults to 7.2 - Switch activerecord-nulldb-adapter from puzzle/nulldb fork to official gem 1.2.2 (fork's unique_constraint patch is now upstream, and 1.2.2 supports activerecord >= 6.1, < 8.2) - Replace deprecated ActiveRecord::Base.connection with with_connection in StatusController#can_query_database? - Fix captionize in format_helper to guard against blank attribute names (Rails 7.2 changed human_attribute_name behaviour for enum models) - Fix dry_crud form control to use plain label tag for blank attributes, bypassing the Rails label helper's internal human_attribute_name call
Gem changes:
- rails ~> 8.0.0
- annotate (incompatible with Rails 8.0, activerecord < 8.0) replaced by annotaterb
- sprockets-rails explicitly added (no longer included by default in Rails 8.0)
config/load_defaults 8.0
Routes fix:
- `resource :order_plannings` → `resources :order_plannings` (singular resource
with :index in :only now raises ArgumentError in Rails 8.0)
Breaking change fixes:
render_callbacks.rb: In Rails 8.0, implicit render calls `render` with no
arguments — `_normalize_render()` returns `{}`, so `options[:template]` is
always nil. Before this change no before_render callbacks (e.g.
before_render_index, before_render_form) were executed for implicitly rendered
actions. Fix: fall back to `options[:action]` and then `action_name`.
accounting_post.rb: `throw(:abort)` in an `after_create` callback raises
UncaughtThrowError in Rails 8.0. `throw :abort` is only valid in before_*
callbacks to halt the chain. Fix: `raise ActiveRecord::Rollback` which rolls
back the transaction without propagating.
employee.rb: `as_json(_options)` requires exactly one argument. Rails 8.0
calls `as_json` without arguments in some render paths. Fix: default to nil.
create_order_test.rb: `find_person` mock expectation changed from `.twice`
to `.once`. In Rails 7.2 `before_validation` on nested records was triggered
twice per save (once when the parent validated nested records, once when each
nested record was saved individually). Rails 8.0 validates each record exactly
once. The `.twice` expectation was an artifact of that old behavior, not
intentional.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
page.assert_selector and node.assert_selector don't increment Minitest's assertion counter; replaced with assert_selector from Capybara::Minitest::Assertions throughout integration tests and helpers.
Rails 8.0 holds connections more strictly than 7.x; the test thread's connection was never returned between tests, eventually exhausting the pool. Fixed by explicitly releasing it in teardown. Pool size now also follows RAILS_MAX_THREADS as best practice.
- Rename pool: to max_connections: in database.yml (Rails 8.1 deprecation) - Increase test connection pool to 15 to avoid exhaustion with Puma threads - Bump bullet 8.1.2, paper_trail 17.0.0, activeresource 6.2.0, omniauth-rails_csrf_protection 2.0.1 for Rails 8.1 compatibility
sleep 0.5 was not enough under load; assert_no_selector polls until the Bootstrap modal loses its .in class (up to Capybara's wait time).
- dry_crud's authorize_class now honors skip_load_and_authorize_resource via cancan_resource_class#skip? - order_plannings_controller: params.expect(:order_id) (Rails 8 strong params) - routes.rb: order_plannings back to a singular resource
before_action :handle_remember_params, only: [:index] raised ActionNotFound for controllers with no :index action (Rails 7.1's raise_on_missing_callback_actions). Guarded with respond_to?(:index). Note: this guard was itself wrong, corrected in a follow-up commit -- respond_to? at class-eval time checks the class, not the instance, and is always false here.
Also comments out belongs_to_required_by_default = false and time_zone_aware_types. See upgrade.html Phase 0 for the belongs_to fallout (31 associations became required) and the fix.
Also drops the unused webdrivers gem from the test group.
- Cache Gemfile/Gemfile.lock and bundle install separately from the source COPY; --mount=type=cache for bundler's own cache - Prune .c/.o/.h build artifacts and spec/test/doc dirs from vendored gems - Drop root/adduser in favor of useradd; add HEALTHCHECK and EXPOSE 3000 - docker/build-push-action@v4 -> @v6 - nochmal: drop the github: source, now released as a real gem - pin activerecord-nulldb-adapter to NULLDB_VERSION
- ApiSerializer: relationships_to_serialize ||= {}, only initialized by
the gem inside add_relationship; our serializers declare none, so an
unknown ?include indexed nil and raised NoMethodError first
- rescue JSONAPI::Serializer::UnsupportedIncludeError directly instead
of the old fast_jsonapi ArgumentError duck-typed shim
Both needed: fix (1) alone changes nothing, fix (2) alone turns the
crash into an unhandled 500. See upgrade.html Phase 0.
Rails 8.1 sorts columns alphabetically (activerecord-8.1.3 schema_dumper.rb:195); column sets are unchanged. See upgrade.html Phase 0.
- 6 associations loosened to optional: true (nullable column, legitimately so) - authentications.yml fixture rewritten: fixtures bypass validations, so the empty placeholders would have violated the new NOT NULL - migration enforces employee_id NOT NULL on worktimes/employments/ authentications, aborting with offending ids if any remain 52 associations audited against schema.rb nullability; see upgrade.html Phase 0 for the full audit and the release-gate rationale.
Run Chrome with --force-prefers-reduced-motion and honor it in CSS, so transitionend still fires but durations collapse to ~0.
SelectionWatcher and FormUpdater wrapped $.getScript in a hand-rolled promise with no catch, so a request aborted by navigation reached the browser as an uncaught error carrying the jqXHR - the unreadable 'Ferrum::JavaScriptError: Object' that failed unrelated later steps.
- the 'Object' in Ferrum::JavaScriptError is the rejected jqXHR - aborted navigation is the ordinary trigger, not a server fault - three clean 16-worker runs after the fix
- reusable workflow permissions are ignored for workflow_call - quote $(pwd) in sbom merge to fix shellcheck SC2046
- called workflows ignore their own permissions block - one entry in the CI list of section 9.5
- init returned early, leaving a stale Crm::Highrise in the cattr - that leak hid the create-client link and failed CreateOrderTest
- its skip branch compared ENV['CI'] to true and never ran - unwrap all call sites, unskip 'create repetition' - pre-existing plannings flakes noted in plannings_orders_test_failure2.txt
- inline the two meal-compensation toggles, drop a constant argument - guard clause instead of a nil-check, named block variable - exclude the three remaining test-helper smells, revert f87028f
- working notes, never belonged in the repo - their findings already live in upgrade.html
- setup-chrome's build lacks the libs and shadowed the system chrome - every integration test died in Ferrum::ProcessTimeoutError
- temporary: dump page html per failure and upload it - log which chrome the runner uses
- the store hardcoded localhost:11211 while CI maps it to 51121 - sessions were lost, so every integration test ran logged out
- run 36684280996 is green: 1567 tests, 0 failures, 0 errors - new section 11 covers actionlint, chrome and the session store
- list_plannings selected 'Nächste 3 Monate', already the default; the change event submits the remote filter form, so every test began with a board reload in flight that replaced the rows it was dragging on - total time per row waited with sleep 0.5 and a non-waiting assert_equal; use waiting assertions instead - the row lookups' sleep 0.1 was a workaround for the same reload - diagnosis in upgrade.html §11.5
- the seven period_shortcut filters pass no label; aria-label Zeitraum - the vCard export's department select is a bare select_tag, not direct_filter; aria-label Organisationseinheit - space.gif in the weekly graph bars and empty evaluator cells is decorative: alt '' - findings from upgrade.html §10.5
- §11.5: the setup's board reload, the widened-window proof, results - §10.5 and the open-items list: a11y findings fixed, one misdiagnosis corrected; deployment repo synced (f640bc85, e775c9d7) - timeline: two dots pointed at pre-rewrite SHAs; §11.4 said SKIP=All
- a repeat_until with no items, or repeat-only over cells without plannings, passed validation and raised in Creator#repeat (500); the board sends the latter for empty cells with the status toggled off, and the panel then showed nothing - now a form error the panel displays - two form_valid? tests pinned those inputs as valid; they now use a selection that has something to repeat
- the Minitest hook runs `ruby -r<test files>`, which skips test:prepare, so the push gate tested whatever bundle was left in app/assets/builds - RakeTarget runs `bin/rails test`, which builds JS, Sass and Tailwind first; re-sign with `bundle exec overcommit --sign`
- 72 references to rewritten commits now name their current equivalents (author date, confirmed by patch-id); rewrite ranges and INT's image sha stay as measured - open items and §11.5: Creator#repeat fix, RakeTarget pre-push hook, backup-branch item corrected
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| def find_prefetched(group, keys) | ||
| if group == :company_partners | ||
| @prefetched[:partners]&.find_all { _1.parent_id.in? Array.wrap(keys) } | ||
| @prefetched[:partners]&.find_all { it.parent_id.in? Array.wrap(keys) } |
There was a problem hiding this comment.
[Reek] reported by reviewdog 🐶
DuplicateMethodCall: Crm::Odoo#find_prefetched calls 'Array.wrap(keys)' 2 times [https://github.com/troessner/reek/blob/v6.5.0/docs/Duplicate-Method-Call.md]
| Worktime.transaction do | ||
| worktimes.each(&:save!) | ||
| worktimes.each do |w| | ||
| if w.new_record? |
There was a problem hiding this comment.
[Reek] reported by reviewdog 🐶
FeatureEnvy: Forms::Splitable#save refers to 'w' more than self (maybe move it to another class?) [https://github.com/troessner/reek/blob/v6.5.0/docs/Feature-Envy.md]
| def save | ||
| Worktime.transaction do | ||
| worktimes.each(&:save!) | ||
| worktimes.each do |w| |
There was a problem hiding this comment.
[Reek] reported by reviewdog 🐶
UncommunicativeVariableName: Forms::Splitable#save has the variable name 'w' [https://github.com/troessner/reek/blob/v6.5.0/docs/Uncommunicative-Variable-Name.md]
| table.row(0).text_color = '333333' # Dark gray | ||
|
|
||
| (1..table.row_length - 1).each do |index| | ||
| (1..(table.row_length - 1)).each do |index| |
There was a problem hiding this comment.
[Reek] reported by reviewdog 🐶
NestedIterators: Order::Services::TimeRapportPdfGenerator#build_list contains iterators nested 2 deep [https://github.com/troessner/reek/blob/v6.5.0/docs/Nested-Iterators.md]
| .map { |week| @efforts_per_week_cumulated[week][set[:type]][:amount] }, | ||
| .keys | ||
| .sort | ||
| .map { |week| @efforts_per_week_cumulated[week][set[:type]][:amount] }, |
There was a problem hiding this comment.
[Reek] reported by reviewdog 🐶
DuplicateMethodCall: OrderControllingHelper#controlling_chart_datasets calls '@efforts_per_week_cumulated[week]' 2 times [https://github.com/troessner/reek/blob/v6.5.0/docs/Duplicate-Method-Call.md]
| .map { |week| @efforts_per_week_cumulated[week][set[:type]][:amount] }, | ||
| .keys | ||
| .sort | ||
| .map { |week| @efforts_per_week_cumulated[week][set[:type]][:amount] }, |
There was a problem hiding this comment.
[Reek] reported by reviewdog 🐶
DuplicateMethodCall: OrderControllingHelper#controlling_chart_datasets calls '@efforts_per_week_cumulated[week][set[:type]]' 2 times [https://github.com/troessner/reek/blob/v6.5.0/docs/Duplicate-Method-Call.md]
| .map { |week| @efforts_per_week_cumulated[week][set[:type]][:amount] }, | ||
| .keys | ||
| .sort | ||
| .map { |week| @efforts_per_week_cumulated[week][set[:type]][:amount] }, |
There was a problem hiding this comment.
[Reek] reported by reviewdog 🐶
DuplicateMethodCall: OrderControllingHelper#controlling_chart_datasets calls 'set[:type]' 2 times [https://github.com/troessner/reek/blob/v6.5.0/docs/Duplicate-Method-Call.md]
|
|
||
|
|
||
| #add_work_item_id{ hidden: true } | ||
| #add_work_item_id{ style: 'display: none' } |
There was a problem hiding this comment.
Do not use inline style attributes
|
|
||
|
|
||
| #add_employee_id{ hidden: true } | ||
| #add_employee_id{ style: 'display: none' } |
There was a problem hiding this comment.
Do not use inline style attributes
| config.cache_classes = true | ||
| config.enable_reloading = false | ||
|
|
||
| config.secret_key_base = ENV['RAILS_SECRET_TOKEN'] || ENV.fetch('SECRET_KEY_BASE', nil) |
There was a problem hiding this comment.
[Fasterer] reported by reviewdog 🐶
Hash#fetch with second argument is slower than Hash#fetch with block.
- weekly graph: the corner cell of the week grid was an empty th (axe empty-table-header) - planning legend: h5 after the h1 board caption skipped four levels (axe heading-order); .h5 keeps the size
- glyphicon SVG font 108737 -> 92569 bytes, still 278 glyphs - three 8-bit jquery-ui sprites recompressed, 0 differing pixels
- the 30.09 build pushed no image; INT only now runs deacaee - nginx asset rules went live with that sync, axe and the 500 fix re-checked
| authorize!(action_name.to_sym, model_class) | ||
| end | ||
|
|
||
| def skip?(behavior) |
There was a problem hiding this comment.
[Rails-Best-Practices] reported by reviewdog 🐶
remove unused methods (ListController#skip?)
|
|
||
| private | ||
|
|
||
| def search_results |
There was a problem hiding this comment.
[Rails-Best-Practices] reported by reviewdog 🐶
remove unused methods (OrdersController#search_results)
|
|
||
|
|
||
| #headerbar.navbar.navbar-default{role: 'navigation', class: ('lonely' unless @user)} | ||
| #headerbar.navbar.navbar-default{role: 'navigation', 'aria-label' => 'Kopfzeile', class: ('lonely' unless @user)} |
There was a problem hiding this comment.
[Rails-Best-Practices] reported by reviewdog 🐶
replace instance variable with local variable
| = hidden_field_tag :page, 1 | ||
|
|
||
| = direct_filter_select(:period_shortcut, nil, predefined_past_period_options, value: @period.shortcut, prompt: 'benutzerdefiniert') | ||
| = direct_filter_select(:period_shortcut, nil, predefined_past_period_options, value: @period.shortcut, prompt: 'benutzerdefiniert', aria: { label: 'Zeitraum' }) |
There was a problem hiding this comment.
[Rails-Best-Practices] reported by reviewdog 🐶
replace instance variable with local variable
| = direct_filter_date(:start_date, 'Von', @period.start_date) | ||
| = direct_filter_date(:end_date, 'Bis', @period.end_date) | ||
| = direct_filter_select(:period_shortcut, nil, predefined_past_period_options, value: @period.shortcut, prompt: 'benutzerdefiniert') | ||
| = direct_filter_select(:period_shortcut, nil, predefined_past_period_options, value: @period.shortcut, prompt: 'benutzerdefiniert', aria: { label: 'Zeitraum' }) |
There was a problem hiding this comment.
[Rails-Best-Practices] reported by reviewdog 🐶
replace instance variable with local variable
| = direct_filter_date(:start_date, 'Von', @period.start_date, class: 'only-mondays') | ||
| = direct_filter_date(:end_date, 'Bis', @period.end_date, class: 'only-fridays') | ||
| = direct_filter_select(:period_shortcut, nil, predefined_future_period_options, value: @period.shortcut, prompt: 'benutzerdefiniert') | ||
| = direct_filter_select(:period_shortcut, nil, predefined_future_period_options, value: @period.shortcut, prompt: 'benutzerdefiniert', aria: { label: 'Zeitraum' }) |
There was a problem hiding this comment.
[Rails-Best-Practices] reported by reviewdog 🐶
replace instance variable with local variable
| end | ||
|
|
||
| resource :order_plannings, only: %i[index show update destroy] do | ||
| resource :order_plannings, only: %i[show update destroy] do |
There was a problem hiding this comment.
[Rails-Best-Practices] reported by reviewdog 🐶
restrict auto-generated routes orders/order_plannings (only: [])
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ruby 3.4 / Rails 8.1 upgrade, plus the asset pipeline move (Sprockets → Propshaft + esbuild/Tailwind) and the CI/lint work that came with it. 149 commits, 316 files.
What's in it
/assets/.manifest.jsonis no longer served (deployment repof640bc85,e775c9d7).653b8675resolves the Brakeman high-confidence warnings —current_user.idbound viasanitize_sql_array, nested parent paths matched withString#include?instead of a params-built regex. The remaining 14 verified false positives live inconfig/brakeman.ignore.fee62b4dauditsbelongs_toassociations and adds theemployee_id NOT NULLmigration (20260727130000).bin/rails testso assets are rebuilt first.Needs review
config/brakeman.ignore— 14 entries. Second reader required;exit_on_warnstays off until that review lands..btn-primaryand the status badges are visibly darker than master; design sign-off wanted.Blocker before any production release
Four
worktimesrows have a nullemployee_id. Migration20260727130000aborts on them — they must be classified and cleared against the production database first.Deploy state
INT runs the image built from this branch head (
git-deacaeeb…). The nginx cache/manifest rules are in the deployment repo'smain.