[OP-19966] Fix 500 when creating working hours with a nil weekday - #24773
Open
jtauschl wants to merge 2 commits into
Open
[OP-19966] Fix 500 when creating working hours with a nil weekday#24773jtauschl wants to merge 2 commits into
jtauschl wants to merge 2 commits into
Conversation
The presence/numericality validation on each *_hours attribute invokes the corresponding dynamically-defined getter, which unconditionally divided the raw (possibly nil) minutes column by 60.0 -- a weekday omitted from a create payload has no DB default and stays nil, so validating it crashed with NoMethodError instead of producing a clean 422. A second, related nil-unsafe site exists in at_least_one_working_day_selected, reachable once every weekday is nil at once. Make both sites nil-safe: the getter returns nil for a nil column instead of dividing it, and the working-day check coerces via to_i first.
|
All contributors have signed the CLA ✍️ ✅ |
Author
|
recheck |
myabc
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a server error when UserWorkingHours weekday minute columns are nil (e.g., when weekday fields are omitted from a create payload) by making the dynamically generated *_hours getters and the at_least_one_working_day_selected validation nil-safe, and adds regression coverage to prevent reintroducing the crash.
Changes:
- Return
nilfrom{day}_hourswhen the underlying{day}minutes column isnil(instead of raising onnil / 60.0). - Make
at_least_one_working_day_selectedrobust tonilweekday values viato_i. - Add model/accessor regression specs for single-day
niland all-daysnilscenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| app/models/user_working_hours.rb | Makes weekday hour getters and “at least one working day” validation nil-safe to avoid 500s on incomplete payloads. |
| spec/models/user_working_hours_spec.rb | Adds regression specs covering nil weekday columns and nil-safe *_hours getters. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
74
to
77
| define_method("#{day}_hours") do | ||
| (public_send(day) / 60.0).round(2) | ||
| minutes = public_send(day) | ||
| minutes.nil? ? nil : (minutes / 60.0).round(2) | ||
| end |
UserWorkingHours#{day}_hours now returns nil for a day with no minutes
value at all (this branch's own earlier fix), but this form component
called .round(2) on it unconditionally -- re-rendering the create form
after a validation failure that left a weekday nil crashed with
NoMethodError instead of showing the validation error banner.
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.
Ticket
https://community.openproject.org/wp/OP-19966
Summary
The presence/numericality validation on each
*_hoursattribute invokes the corresponding dynamically-defined getter, which unconditionally divided the raw (possibly nil) minutes column by 60.0 -- a weekday omitted from a create payload has no DB default and stays nil, so validating it crashed withNoMethodErrorinstead of producing a clean 422. A second, related nil-unsafe site exists inat_least_one_working_day_selected, reachable once every weekday is nil at once.Change
Make both sites nil-safe: the getter returns
nilfor a nil column instead of dividing it, and the working-day check coerces viato_ifirst.Test plan