Repository navigation
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.
The label includes the work item name, which users enter; it went into an html_safe flash unescaped. The overlap list on the next line already escaped.
jquery-ujs runs format.js responses as a script, so create/update wrapped the JSON in '...' and modal_create.js eval'ed it. Any apostrophe in a label broke the modal with a SyntaxError and could inject script. - render js: the JSON encoded once more (an escaped string literal) - modal_create.js: JSON.parse twice instead of eval - Completable#js_entry: drop the j() that compensated for the old quoting - regression test: create a client named O'Neil AG from the order form
Every browser that can parse the bundle (arrow functions, const) has both; the endsWith one also carried the only no-extend-native disable.
sort_by_number? accepted any public Entry method, so ?sort=post_hours, order, invoices, major_chance or targets raised in sort_by and answered 500. Both reports now list their numeric columns explicitly, including the ones the views don't link, so bookmarks keep working.
sort_by_string reversed the list for ascending, so "Kunde" sorted Z-A when asked for A-Z; the order report reverses for descending. The test pinned the inverted order.
skip_files excluded _detailrow and details "because brakeman hangs". Brakeman 8 scans them in 2.5 s with no new warnings, so they were unscanned for nothing.
- .haml-lint.yml's InlineStyles/ViewLength excludes replaced the todo's lists (arrays aren't merged), so the todo entries never applied; one list each now, grouped by reason - its `Rubocop` key never matched the `RuboCop` linter; renamed - ClassesBeforeIds: EnforcedStyle id, as the views are written - autocorrect ClassAttributeWithStaticValue, SpaceBeforeScript, UnnecessaryStringOutput, ClassesBeforeIds; drop their todo entries - overcommit: the mailer views lint clean, drop their HamlLint excludes
Nothing renders it since b49b9a5 moved the bar into ProgressBarHelper#order_progress_bar.
- deprecated global Sass functions -> math.percentage, color.adjust (compiled CSS byte-identical; build deprecation warnings ~40 -> 14) - delete an empty .name rule; space two * operators - property order, one selector per line, quotes, leading zero - .orders-cost-total_sum -> .orders-cost-total-sum (+ OrderCostsHelper) - reasoned stylelint-disable for the cascade-order @imports, the icon font family, the visually-hidden clip and the deliberate @extend
The "FIXME: Remove in the end" excludes are stale: the initializer passes LineEndings, Reek, RuboCop and RubySyntax.
scss_lint is unmaintained and stops at a syntax error on dart-sass's math.div, so four stylesheets could never be linted and the exclude list had two dead paths and missed the moved vendor files. - .stylelintrc.json: stylelint-config-recommended-scss for *.scss, the Tailwind at-rule allowlist only for *.css, vendored files ignored - overcommit: ScssLint off (like CssLint), Stylelint covers css and scss - drop scss_lint (and sass, sass-listen) from the overcommit gem group - bin/setup_overcommit installs stylelint-config-recommended-scss
74dc3a5 added the active-period flag to total_row_planned_hours and dropped its ||=, so every call recomputed the grouped totals of all rows; the board calls it twice per row. In a request the query cache hid the SQL, not the work. Memoize per flag again, and make the flag a keyword (active_period_only:) -- the board view passed it as `only_for_active_period=true`, assigning a local.
tickets_table_rows printed tckt_val[:date][0] for both Von and Bis, so Bis always repeated the first date; combine_ticket_date_range stores [min, max]. From 9ec6857, on master too.
`down` sat below `private`; Rails calls public_send(:down), so a rollback died with NoMethodError instead of IrreversibleMigration. `up` is unchanged.
PdfExport#pages compared receipt.metadata[:page_count] > 1, which raises for a PDF attached without that metadata. Read the receipt's page count once, treat a missing one as a single page, and drop the page_count wrapper.
Billing, Invoice and Order reports each carried a copy. Not in Filterable: four controllers include that module, and a public method there would become an action. Order::Report adds its two view params via ignored_filter_params.
NotBilledTimesReminderJob built a 15-line Employee query in a method that used no job state; it is now Employee.responsibles_with_not_billed_times_last_month and the job only sends the mails.
- Billing::Report::Entry: one invoiced_worktime_sum for billed_amount, not_billed_hours and not_billed_amount - Crm::Odoo: one `only(context, ids)` for the four sync methods' id filter - WorktimesCsv: stripped: as a keyword instead of a positional boolean
Reek now accepts e, h (as in Hash.new { |h, k| }), k, t and v; the rest get
names: worktime, order, attrs, hours, hours_by_billable, employee_hours,
entry (the CSV row parameters), reminded_post/unreminded_post and
employment_role_attrs_80_percent (a private helper named like a test).
One line each for the 13 classes reek's IrresponsibleModule listed: the billing and invoice reports with their Csv/Entry/Total, ErrorTracker, Forms::MultiOrdertime, TimeRapportData/PdfGenerator, InvoiceReportsController.
- AbsencesEvalTest#test_sum_total_times checked three department filters in sequence; now three tests sharing create_absences_in_departments - five single-scenario tests/helpers carry a one-line :reek:TooManyStatements note with the reason
427 warnings had no exclude (403 on master) and half the todo matched nothing; reek now reports 0. Detail in upgrade.html section 13. - accept e/h/k/t/v (anchored: reek matches accept patterns unanchored) - test/: off for Minitest idioms; db/migrate: off for comments, repeats, length; app/helpers: no UtilityFunction - UtilityFunction public_methods_only, NestedIterators 2, DuplicateMethodCall max_calls 2, TooManyStatements max_statements 10 - 1098 stale excludes removed; every added one says reviewed, deferred or NOT YET REVIEWED - exclude cssdiff.rb/reconcile.rb, with a removal reminder in each script
- 13.1 the twelve defects found behind exceptions, 13.2 the exception mechanisms that excluded nothing, 13.3 reek 427 -> 0 and its settings - timeline: INT re-verification (02.10) and the audit (03.10) - follow-ups: parked reek reviews, RuboCop Metrics maxes, PDF generator pass, Billing::Report::Loader, shared report skeleton, OrderServicesController#report, porting the master-side fixes - phase 7: unique indexes behind the 17 uniqueness validations - note the brakeman.ignore agent review; scripts' removal with their ignores
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| end | ||
|
|
||
| def row(time, stripped = false) | ||
| def row(time, stripped: false) |
There was a problem hiding this comment.
[Rails-Best-Practices] reported by reviewdog 🐶
move model logic into model (time use_count > 4)
| # an escaped JS string literal (quotes, backslashes, <>&, U+2028/9), which | ||
| # modal_create.js decodes with JSON.parse twice. A hand-quoted '...' breaks on | ||
| # any apostrophe in the label and lets it inject script. | ||
| def js_entry_literal |
There was a problem hiding this comment.
[Rails-Best-Practices] reported by reviewdog 🐶
remove unused methods (CrudController#js_entry_literal)
| Durch Filter angezeigter Zeitbereich: | ||
| .header-planned-amount | ||
| = format_number(@board.total_planned_hours(true), 0) | ||
| = format_number(@board.total_planned_hours(active_period_only: true), 0) |
There was a problem hiding this comment.
[Rails-Best-Practices] reported by reviewdog 🐶
replace instance variable with local variable
| Durch Filter angezeigter Zeitbereich: | ||
| .header-planned-amount | ||
| = format_number(@board.total_planned_hours(true), 0) | ||
| = format_number(@board.total_planned_hours(active_period_only: true), 0) |
There was a problem hiding this comment.
[Rails-Best-Practices] reported by reviewdog 🐶
replace instance variable with local variable
- bust-caches.yaml: delete Actions caches, rebuild :buildcache without cache - reusable-build.yaml: add no_cache input
Kagemaru
force-pushed
the
task/65394-ruby-and-rails-upgrade
branch
from
October 29, 2026 23:24
02a6065 to
8638edb
Compare
- reusable-build: resolve ref to a SHA, build from git context at that SHA - lint, test, sbom: check out the same ref
Kagemaru
had a problem deploying
to
pitc-ptime-int
October 29, 2026 23:48 — with 
GitHub Actions
Failure
GitHub Actions
Failure
This branch had an error being 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
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.